chore(policy): harden loadPresetFromFile against oversize and symlinked inputs#3020
Conversation
…ed inputs Adds a 1 MiB stat-size guard and a `lstatSync`-based symlink check to loadPresetFromFile, so the function rejects unexpectedly large files before reading and refuses to follow symbolic links. Also collapses the prior existsSync/statSync pair into a single lstat call wrapped in a try/catch. Cleans up the temp directories that the writeTmp helper in loadPresetFromFile's test block was leaking, and adds tests for the new size-limit and symlink-reject paths. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesPreset File Validation Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/policies.ts (1)
622-653:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the TOCTOU gap between metadata checks and file read.
At line 624 you validate with
lstatSync(abs), but at line 652readFileSync(abs, ...)re-resolves the path. A local attacker can replace the file between those steps (symlink swap / target swap), bypassing the earlier checks.Use
fs.openSync(abs, fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW)to open once with atomic symlink rejection, then usefs.fstatSync(fd)for metadata andfs.readFileSync(fd, "utf-8")to read from the open descriptor. This eliminates the race window. Ensure proper fd cleanup with try/finally on all early returns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/policies.ts` around lines 622 - 653, Replace the two-step lstat/read sequence around fs.lstatSync(abs) + fs.readFileSync(abs, ...) with an atomic open+fstat+read pattern: open the file once via fs.openSync(abs, fs.constants.O_RDONLY | fs.constants.O_NOFOLLOW) to get fd, call fs.fstatSync(fd) (use the returned stat in place of stat) to perform isSymbolicLink/isFile/size checks against MAX_PRESET_FILE_BYTES and keep the /\.ya?ml$/i extension check on abs, then read the contents from the descriptor (fs.readFileSync(fd, "utf-8") or appropriate read method). Ensure every early-return path closes fd in a try/finally so the descriptor is always closed; handle open errors and map existing console.error messages to the same failures (use filePath/abs in messages) before parsing with YAML.parse.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/policies.ts`:
- Around line 622-653: Replace the two-step lstat/read sequence around
fs.lstatSync(abs) + fs.readFileSync(abs, ...) with an atomic open+fstat+read
pattern: open the file once via fs.openSync(abs, fs.constants.O_RDONLY |
fs.constants.O_NOFOLLOW) to get fd, call fs.fstatSync(fd) (use the returned stat
in place of stat) to perform isSymbolicLink/isFile/size checks against
MAX_PRESET_FILE_BYTES and keep the /\.ya?ml$/i extension check on abs, then read
the contents from the descriptor (fs.readFileSync(fd, "utf-8") or appropriate
read method). Ensure every early-return path closes fd in a try/finally so the
descriptor is always closed; handle open errors and map existing console.error
messages to the same failures (use filePath/abs in messages) before parsing with
YAML.parse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1e08a33f-d82f-4710-a99e-3bf2818acf02
📒 Files selected for processing (2)
src/lib/policies.tstest/policies.test.ts
…loading Replace the lstat + readFileSync sequence with an atomic openSync(O_RDONLY | O_NOFOLLOW) followed by fstatSync and readFileSync on the descriptor. The new shape removes the TOCTOU window between stat and read, lets the kernel reject symbolic links via ELOOP at open time instead of a separate isSymbolicLink() check, and closes the descriptor in a try/finally on every early-return path. Bump MAX_PRESET_FILE_BYTES from 1 MiB to 10 MB (10_000_000), and adjust the oversize-file test to write enough padding to exceed the new limit. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/policies.ts`:
- Around line 650-663: The current pre-read stat check uses stat.size and
MAX_PRESET_FILE_BYTES but then calls fs.readFileSync(fd, "utf-8") which re-stats
and can read a larger file; replace that unbounded read with a bounded read from
the open descriptor: allocate a Buffer of size Math.min(stat.size,
MAX_PRESET_FILE_BYTES), call fs.readSync(fd, buffer, 0, buffer.length, 0) to
read exactly that many bytes into buffer (ensuring position 0), convert
buffer.toString("utf-8") into content, then pass content to YAML.parse as
before; keep the existing checks and error logging using fd, stat,
MAX_PRESET_FILE_BYTES, content and YAML.parse.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 61aec3db-c475-4071-b694-1b8e44527baf
📒 Files selected for processing (2)
src/lib/policies.tstest/policies.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/policies.test.ts
readFileSync(fd, "utf-8") performs its own internal stat at read time and ignores the size we just checked, so a file that grows between the fstatSync and the read could exceed MAX_PRESET_FILE_BYTES. Replace the unbounded readFileSync with a bounded readSync loop into a Buffer of exactly stat.size bytes, then decode as UTF-8. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Summary
Three follow-up hardening items from PR #2077 review against
loadPresetFromFileinsrc/lib/policies.ts: a 10 MB file-size guard, symlink rejection viaO_NOFOLLOW, and tmp-dir cleanup in the corresponding test block. The size-and-symlink checks now use an atomicopenSync(O_RDONLY | O_NOFOLLOW) → fstatSync → readFileSync(fd)pattern (per CodeRabbit review), so there is no TOCTOU window between the stat and the read and the kernel rejects symbolic links at open time.Related Issue
Closes #2521
Changes
MAX_PRESET_FILE_BYTES = 10_000_000and reject files larger than the limit before reading their contents.existsSync+statSyncpair with an atomicopenSync(O_RDONLY | O_NOFOLLOW)followed byfstatSyncon the descriptor andreadFileSync(fd, "utf-8"), with atry/finallycloseSyncon every early-return path. Symbolic links are rejected viaELOOPfromopenSyncand surfaced with a clear error pointing the user atrealpath.mkdtempSyncdirectories created by thewriteTmphelper inside theloadPresetFromFiledescribe block viaafterEach.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests