fs: validate position argument before length === 0 early return#62674
Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom Apr 14, 2026
Merged
fs: validate position argument before length === 0 early return#62674nodejs-github-bot merged 1 commit intonodejs:mainfrom
nodejs-github-bot merged 1 commit intonodejs:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62674 +/- ##
==========================================
+ Coverage 89.77% 89.80% +0.03%
==========================================
Files 673 699 +26
Lines 203820 216235 +12415
Branches 39175 41331 +2156
==========================================
+ Hits 182987 194199 +11212
- Misses 13152 14148 +996
- Partials 7681 7888 +207
🚀 New features to boost your workflow:
|
|
Moving One question on the validation ordering: Previously, On the new test cases: The tests with empty buffer and an invalid position object are good — but note that |
avivkeller
approved these changes
Apr 12, 2026
Collaborator
lpinca
approved these changes
Apr 13, 2026
Collaborator
Collaborator
Collaborator
Commit Queue failed- Loading data for nodejs/node/pull/62674 ✔ Done loading data for nodejs/node/pull/62674 ----------------------------------- PR info ------------------------------------ Title fs: validate position argument before length === 0 early return (#62674) Author Edy Silva <[email protected]> (@geeksilva97) Branch geeksilva97:fix-fs-read-sync -> nodejs:main Labels fs, author ready, needs-ci Commits 1 - fs: validate position argument before length === 0 early return Committers 1 - Edy Silva <[email protected]> PR-URL: https://github.com/nodejs/node/pull/62674 Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/62674 Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Fri, 10 Apr 2026 16:46:18 GMT ✔ Approvals: 2 ✔ - Aviv Keller (@avivkeller): https://github.com/nodejs/node/pull/62674#pullrequestreview-4094424429 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/62674#pullrequestreview-4101570935 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2026-04-14T09:32:37Z: https://ci.nodejs.org/job/node-test-pull-request/72680/ - Querying data for job/node-test-pull-request/72680/ ✔ Build data downloaded ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 62674 From https://github.com/nodejs/node * branch refs/pull/62674/merge -> FETCH_HEAD ✔ Fetched commits as d0fa608c0796..de1c2438a977 -------------------------------------------------------------------------------- Auto-merging lib/fs.js Auto-merging lib/internal/fs/promises.js [main d32bcd2881] fs: validate position argument before length === 0 early return Author: Edy Silva <[email protected]> Date: Fri Apr 10 13:42:53 2026 -0300 5 files changed, 84 insertions(+), 19 deletions(-) ✔ Patches applied -------------------------------------------------------------------------------- --------------------------------- New Message ---------------------------------- PR-URL: https://github.com/nodejs/node/pull/62674 Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> -------------------------------------------------------------------------------- [main 908808fa7e] PR-URL: https://github.com/nodejs/node/pull/62674 Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Author: Edy Silva <[email protected]> Date: Fri Apr 10 13:42:53 2026 -0300 5 files changed, 84 insertions(+), 19 deletions(-) ✖ 908808fa7e8ca1d8b750911952a60a27d5d8302d ✔ 0:0 no Co-authored-by metadata co-authored-by-is-trailer ✔ 0:0 skipping fixes-url fixes-url ✖ 1:0 blank line expected after title line-after-title ✔ 0:0 line-lengths are valid line-length ✔ 0:0 metadata is at end of message metadata-end ✖ 0:0 Commit must have a PR-URL. pr-url ✔ 0:0 reviewers are valid reviewers ✖ 0:0 Invalid subsystem: "PR-URL" subsystem ✔ 0:0 Title is formatted correctly. title-format ✔ 0:0 Title is <= 50 columns. title-length ℹ Please fix the commit message and try again. Please manually ammend the commit message, by running `git commit --amend` Once commit message is fixed, finish the landing command running `git node land --continue`https://github.com/nodejs/node/actions/runs/24407872429 |
Collaborator
|
Landed in ed05549 |
Cosmic-Game-studios
pushed a commit
to Cosmic-Game-studios/node
that referenced
this pull request
Apr 17, 2026
fs.read() and fs.readSync() validate the `position` argument via validatePosition() (tightened in commit ed05549). The symmetric write path was never updated: fs.write(), fs.writeSync() and fsPromises.FileHandle.write() silently coerced any non-number `position` (strings, objects, booleans, NaN, out-of-range numbers, out-of-range bigints) to `null`, which means "use the current file offset". Impact: callers relying on an ERR_OUT_OF_RANGE / ERR_INVALID_ARG_TYPE throw to reject malformed inputs instead silently got a stream-mode write at the current file offset — bypassing validation and potentially overwriting file content the caller thought it had refused. Inconsistent between read and write is a direct input- validation failure that is trivially triggerable from userland: fs.writeSync(fd, Buffer.from('PWN'), 0, 3, -2); // accepted fs.writeSync(fd, Buffer.from('PWN'), 0, 3, 'str'); // accepted fs.writeSync(fd, Buffer.from('PWN'), 0, 3, { not: 'num' }); // accepted Mirror the read-side validation in all three write entry points, add a regression test covering positional-arg, options-object and mutation-guarded-options-object invocations for sync, async and promise variants. Refs: nodejs#62674
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Move position validation upwards to match documented behavior.
Fixes #62638