fix(ext/node): validate fs.watch boolean options#33627
Conversation
…booleans in `fs.watch`
`fs.watch(path, { recursive: '1' })` previously accepted the truthy non-boolean
silently. Match Node's
https://github.com/nodejs/node/blob/main/lib/internal/fs/recursive_watch.js
which calls `validateBoolean(recursive, 'options.recursive')` (and the same
for `persistent`) before starting the watcher.
Enables `parallel/test-fs-watch-recursive-validate.js`.
nathanwhitbot
left a comment
nathanwhitbot
left a comment
There was a problem hiding this comment.
LGTM — matches Node's internal/fs/recursive_watch.js#L58-L64 for the non-native recursive path. Note Node's lib/fs.js#watch (lines 2507–2557) doesn't validate at the JS level — it relies on the C++ binding's coercion for the native path — so this PR is actually slightly stricter than Node here, which I think is fine and is what the test expects.
A couple of follow-ups worth tracking but not blocking:
-
fs.promises.watch(watchPromisein this file, line 3223) doesn't get the same validation. If the recursive-validate test only exercises syncfs.watch, that's why it passes; but a future test or user callingfsPromises.watch(path, { recursive: '1' })would silently accept it. Worth mirroring the same twovalidateBooleancalls there for consistency. -
signalandthrowIfNoEntryaren't validated either (Node validates both per the linked file). Probably out of scope for the enrolled test, but they fit the same pattern when you add them later. -
Minor:
options?.recursive != nullis the same asoptions != null && options.recursive != null— the optional chaining +!= nullform would shorten the two new conditionals to one line each. Stylistic only; what's there is clear too.
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
The substance is right. Verified:
validateBooleanis already imported at the top offs.ts:83and used in two other places (line 1818, 1872), so no new imports needed.- Both checks are guarded by
options != null && options.<field> != nullso passingundefined/null/nooptionskeeps the existing fast path. Only an explicitly-set non-boolean triggers the throw. - Mirrors Node's
lib/internal/fs/recursive_watch.jswherevalidateBoolean(recursive, ...)andvalidateBoolean(persistent, ...)both run before the watcher is created. The new test (test-fs-watch-recursive-validate.js) presumably exercises both fields.
Test enrollment alphabetically positioned correctly (recursive-linux-parallel-remove < recursive-validate < ref-unref).
Holding off on APPROVE per fibibot's CI gate — only 73/n checks complete so far. Lint debug clean on all 3 platforms, but test node_compat debug is only fully green on linux-aarch64 (linux-x86_64, both macOS variants still queued or partially in-flight). Will re-confirm once the remaining node_compat debug jobs land.
4 participants
Summary
fs.watch(path, { recursive: '1' })previously accepted truthy non-boolean values silently. Match Node'slib/internal/fs/recursive_watch.js, which callsvalidateBoolean(recursive, 'options.recursive')(and the same forpersistent) before starting the watcher.Enables
parallel/test-fs-watch-recursive-validate.js.Test plan
parallel/test-fs-watch-recursive-validate.jspasses viacargo test --test node_compatfs-watch*tests still pass