Skip to content

Conversation

fabasoad
Copy link
Contributor

Description

This pull request fixes js/file-system-race security issue found by CodeQL in list.ts file.

Verification

Before fix

  1. Run the command below.
git checkout main
codeql database create .db-codeql --language=javascript --build-mode=none
codeql database analyze .db-codeql javascript-security-extended.qls --format=sarifv2.1.0 --output=codeql.sarif
jq -r '.runs[].results[] | "\(.ruleId): \(.locations[].physicalLocation.artifactLocation.uri)"' codeql.sarif
  1. You will see the following lines:
js/polynomial-redos: dist/commonjs/header.js
js/polynomial-redos: dist/esm/header.js
js/polynomial-redos: src/header.ts
js/file-system-race: dist/commonjs/replace.js
js/file-system-race: dist/esm/replace.js
js/file-system-race: src/list.ts
js/file-system-race: src/list.ts
js/file-system-race: src/replace.ts

After fix

  1. Run the command below.
git checkout <current-branch>
codeql database create .db-codeql --language=javascript --build-mode=none
codeql database analyze .db-codeql javascript-security-extended.qls --format=sarifv2.1.0 --output=codeql.sarif
jq -r '.runs[].results[] | "\(.ruleId): \(.locations[].physicalLocation.artifactLocation.uri)"' codeql.sarif
  1. You will see the following lines:
js/polynomial-redos: dist/commonjs/header.js
js/polynomial-redos: dist/esm/header.js
js/polynomial-redos: src/header.ts
js/file-system-race: dist/commonjs/replace.js
js/file-system-race: dist/esm/replace.js
js/file-system-race: src/replace.ts

Note that list.ts is gone from the scanning results.

References

@isaacs
Copy link
Owner

isaacs commented Sep 23, 2025

Hm, there actually is still an unavoidable TOCTOU here, but this patch make the failure mode match the failure mode of the asynchronous case and the behavior of extract (ie, any data added to the file after the stat is ignored).

@isaacs isaacs closed this in 5330eb0 Sep 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants