Conversation
readFile, writeFile, appendFile, readdir, readlink, realpath, mkdtemp, watch and the FileHandle readFile/writeFile/appendFile methods treated any options value that was not a string or an object as "no options". `fs.writeFileSync(path, secret, 0o600)` wrote a 0o644 file. Node's getOptions() throws ERR_INVALID_ARG_TYPE for these values. Do the same.
|
Updated 8:04 AM PT - Sep 8th, 2026
❌ @robobun, your commit 95c62c4 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 41928That installs a local version of the PR into your bun-41928 --bun |
|
Reproduced on canary Test: CI: build 112657 passed every lane except |
|
Warning Review limit reached
On-demand reviews are free for the next 12 days. After that, they cost $0.25 per reviewed file. Or wait 12 seconds for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughFilesystem APIs now reject invalid ChangesFilesystem options validation
Merge Risk: ⚪ Minimal · up to Filesystem APIs now consistently reject unsupported options values while retaining supported option forms. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it tightens user-facing validation across a dozen node:fs entry points (previously-accepted calls will now throw) and the motivating case is security-adjacent (a mode argument being silently dropped), a human look would still be worthwhile.
What was reviewed:
- Traced each new
elseguard innode_fs.rsand the JS wrappers — functions still fall through asis_object()/typeof === "function"sofs.readFile(path, cb)is unaffected. - Checked the win32
realpathEncodingOptionextraction — the??→??=fix now actually caches the validator, and the callback overload peels the callback before calling the helper. - Compared the Rust
throw_invalid_options_typemessage against the JS$ERR_INVALID_ARG_TYPE(["string", "Object"])path — both render the same string the test asserts. - Test coverage: sync/callback/promises/FileHandle matrix, negative contract (valid forms still work, no leaked writes),
isWindowsbranch on the mode assertion.
Extended reasoning...
Overview
This PR closes a Node.js compat gap where Bun silently ignored a non-string, non-object options argument across the node:fs surface. The motivating case — fs.writeFileSync(p, "KEY", 0o600) writing a 0o644 file instead of throwing — is a real footgun. The fix adds a shared throw_invalid_options_type helper on the Rust side wired into four arg parsers (parse_encoding_arg, Readdir, ReadFile, WriteFile), matching guards in the JS-side fs.promises writeFile/appendFile/readFile paths (including the async-iterator branch and FileHandle methods) and fs.watch, and extracts a realpathEncodingOption helper for the win32 JS realpath ports that also fixes a pre-existing ?? (should have been ??=) caching bug. Tests in fs.test.ts sweep six bad primitive values across ~30 entry-point/form combinations plus the negative contract.
Security risks
The change is security-positive: the bug it fixes could cause a caller who thought they were passing a restrictive mode to writeFileSync to get a default-permission file instead. The new behavior throws rather than silently mis-applying, which is strictly safer. No new attack surface is introduced — validation is added, not relaxed. The one thing worth a human eye is that this is a behavior change: code that previously "worked" (with wrong semantics) will now throw, which could surface in downstream projects.
Level of scrutiny
Medium-high. This is a Node compat change, and REVIEW.md is explicit that the full error contract (code, message text, delivery channel, check ordering) must match Node exactly, and that tightened validation must enumerate every legitimate input class and prove each still passes. The PR does this well — functions are still accepted (matching Node's getOptions()), null/undefined/string/object all still work, and the tests assert exact code + message. The message-wording concern (Rust helper vs $ERR_INVALID_ARG_TYPE(["string", "Object"])) was raised twice during the hunt and ruled out: both paths produce must be one of type string or object, which is what the test asserts across both native and JS entry points. A human confirming that against a live Node build would be the last mile.
Other factors
The change follows the repo's own review rules closely: it fixes the whole bug class in one PR (all sibling entry points, sync/async/promises/FileHandle, the win32 branch), deduplicates within its own diff (two extracted helpers), uses the centralized $ERR_* machinery rather than hand-rolled errors, and the test uses tempDir with using, awaits all .rejects, asserts side-effect absence, and branches the mode-bit assertion on isWindows. The ?? → ??= fix in the win32 realpath is a genuine drive-by correctness improvement. The bug hunt ran to dry_streak with no findings. I'm deferring rather than approving only because behavior-changing Node-compat validation across native + JS with a security-adjacent motivation is exactly the category where a maintainer sign-off is appropriate.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This branch has not been deployed
2 participants
Problem
fs.writeFileSync(path, "KEY", 0o600)writes a0o644file. Bun treats anyoptionsvalue that is not a string or an object as "no options". Node throwsERR_INVALID_ARG_TYPE: The "options" argument must be one of type string or object. Received type number (384)fromgetOptions()(lib/internal/fs/utils.js).options: string | object:readFile,writeFile,appendFile,readdir,readlink,realpath,mkdtemp,watch(sync, callback andfs.promisesforms) andFileHandle#readFile/writeFile/appendFile. The native parsers insrc/runtime/node/node_fs.rs(parse_encoding_arg,Readdir::from_js,ReadFile::from_js,WriteFile::from_js_with_default_flag) had noelsebranch for a non-object, and the JS wrappers infs.promises.ts,internal/fs/watch.tsand the win32realpathport swallowed it too.Fix
ERR_INVALID_ARG_TYPEwith node's message whenoptionsis not a string, an object,null/undefined, or a function. A function stays accepted because the callback wrappers (fs.readFile(path, cb)) leave the callback in that slot, as node'sgetOptions()does.opendir/opendirSync/Dirget the same contract: they already threw, but with a different message, and rejected a function.fs.promises.writeFilewith iterable data and a badoptionsnow rejects with the same error instead ofUnknown encoding: undefined.opendircontract above, and the notes below no longer claim it already matched).test/js/node/fs/fs.test.ts(fs options argument, two tests, both fail on canary). Also the rest offs.test.ts,promises.test.js,dir.test.ts,fs.watch.test.ts,fs-promises-writeFile-async-iterator.test.tsand 54test-fs-*node parallel tests.Background
optionsargument of these functions through one helper,getOptions(options, defaults). It returns the defaults fornull,undefinedor a function, turns a string into{ encoding }, uses an object as is, and throws for anything else. Sofs.mkdirSync(p, 0o700)andfs.openSync(p, "w", 0o600)take a positional mode, butwriteFileSync(p, data, 0o600)does not. Under Bun that last call silently produced a world-readable file.rm,cp,createReadStream/createWriteStream(getStreamOptions) andglob. The functions above were the remaining lenient ones.node_fs.rs,JSValue::is_object()is true for functions (JSType >= Object), so the newelsebranches only see primitives: numbers, booleans, symbols and bigints.Notes
Repro matrix (bun canary
f42e98025before, this branch after, node v26.3.0):Not changed:
fs.promises.watch. Node usesvalidateObjectthere (a string is rejected too), which is a different contract fromgetOptions; Bun throws aTypeErrorabout the listener for a number today.createReadStream/createWriteStreamalready match node throughgetStreamOptions.The JS side now has the
getOptionsdiscrimination in a handful of places (getStreamOptions,FSWatcher,Dir, the twoFileHandlewriters, the iterablewriteFilepath, the win32realpathhelper). Folding them into onegetOptions(options, defaults)ininternal/validatorsis a reasonable follow-up. It is not in this PR to keep the diff to the behavior change.Two pre-existing slow tests in
fs.test.ts(stat > async calls do not keep a Buffer path alive,readdirSync(path, {recursive: true}) x 100) time out under the full debug+ASAN run in this container and pass in isolation.abort-signal-leak-read-write-file.test.ts(100k iterations) also exceeds its 300 s budget under debug+ASAN here; a 5k-iteration copy of the fixture passes.