Skip to content

fix(security): close file-system-race (TOCTOU) sites in shared lib#25

Merged
avifenesh merged 2 commits into
mainfrom
fix/toctou-file-race
May 30, 2026
Merged

fix(security): close file-system-race (TOCTOU) sites in shared lib#25
avifenesh merged 2 commits into
mainfrom
fix/toctou-file-race

Conversation

@avifenesh
Copy link
Copy Markdown
Contributor

Summary

Resolves the CodeQL js/file-system-race (high) alerts in the shared collector/analyzer code. These are check-then-use patterns where a separate fs.stat/fs.existsSync/fs.accessSync/fs.lstat on a path precedes a readFileSync/writeFileSync on the same path. Because this lib/ syncs to consumer plugins (deslop, repo-intel, + 11 others via sync.yml), fixing it here clears the alerts in those repos after the next sync + scan.

Changes

  • New lib/utils/fs-safe.js readFileWithLimit() - opens the path once and runs the is-file check, size cap, and read against the same fd. A swap of the path between calls cannot redirect the read.
  • collectors/codebase.js, enhance/suppression.js, enhance/cross-file-analyzer.js - replace stat/exists/access-then-readFileSync with readFileWithLimit (drops the redundant pre-check; missing/oversize throws and is handled by the existing catch).
  • patterns/slop-analyzers.js - inline fd-based fstat+read using the injected fs so mocked tests keep working.
  • enhance/auto-suppression.js - drop existsSync pre-check; read via fd, write via writeJsonAtomic.
  • enhance/docs-analyzer.js, enhance/prompt-analyzer.js - back up from the in-memory original snapshot instead of re-reading the file immediately before the write (removes the read->write race).
  • enhance/fixer.js - write via writeFileAtomic (temp + rename; rename never follows a symlink) while keeping the explicit assertNotSymlink refusal.

Test plan

  • node --test -> 69 pass / 0 fail / 2 skipped (incl. fixer.test.js)
  • All changed modules require() cleanly; readFileWithLimit verified for normal read, EFBIG (oversize), and ENOENT (missing)

CodeQL js/file-system-race fired on check-then-use file access in the
shared collector/analyzer code that syncs to consumer plugins. Fixes:

- New lib/utils/fs-safe.js readFileWithLimit(): opens once and does the
  is-file/size check + read on the same fd, so a path swap cannot redirect
  the read.
- collectors/codebase.js, enhance/suppression.js, enhance/cross-file-analyzer.js:
  replace stat/exists/access-then-readFileSync with readFileWithLimit
  (drops the redundant pre-checks).
- patterns/slop-analyzers.js: inline fd-based stat+read using the injected
  fs (preserves test mocking).
- enhance/auto-suppression.js: drop existsSync pre-check; read via fd and
  write via writeJsonAtomic.
- enhance/docs-analyzer.js, enhance/prompt-analyzer.js: back up from the
  in-memory original snapshot instead of re-reading the file before write.
- enhance/fixer.js: write via writeFileAtomic (temp + rename, never follows
  a symlink) while keeping the explicit assertNotSymlink refusal.

Propagates to consumers (deslop, repo-intel, ...) via the lib sync.
node --test: 69 pass / 0 fail.
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces safe file-reading and atomic-writing mechanisms, specifically adding readFileWithLimit to mitigate TOCTOU (Time-of-Check to Time-of-Use) vulnerabilities and symlink-following attacks across multiple analyzer modules. While these security enhancements are applied successfully in several areas, the review feedback correctly identifies that lib/enhance/docs-analyzer.js and lib/enhance/prompt-analyzer.js still utilize fs.writeFileSync directly for writing files and backups. To fully secure the application against symlink redirection, these files should be updated to use the newly introduced writeFileAtomic utility.

Comment thread lib/enhance/docs-analyzer.js Outdated
Comment thread lib/enhance/prompt-analyzer.js Outdated
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.

1 participant