Skip to content

fs: normalize trailing dot segments for rm - #65883

Open
AmarWaqar-TSKLI wants to merge 1 commit into
nodejs:mainfrom
AmarWaqar-TSKLI:fs-rm-dot-segments
Open

fs: normalize trailing dot segments for rm#65883
AmarWaqar-TSKLI wants to merge 1 commit into
nodejs:mainfrom
AmarWaqar-TSKLI:fs-rm-dot-segments

Conversation

@AmarWaqar-TSKLI

@AmarWaqar-TSKLI AmarWaqar-TSKLI commented Sep 7, 2026

Copy link
Copy Markdown

fs.rmSync() dispatches to binding.rmSync() (std::filesystem::remove_all) while
fs.rm() and fsPromises.rm() use the JS rimraf, so the two forms disagree whenever the
trailing path component is . or ... Fixture is a/b/c/d, options are
{ recursive: true, force: true }:

path resolves to fs.rmSync before fsPromises.rm before after (both)
a/b/../. a ok, leaves a EINVAL, removes nothing correct
a/b/.. a ok, leaves a ok, leaves a correct
a/. a throws, code is "" EINVAL, removes nothing correct
a/b/c/. a/b/c throws, code is "" EINVAL, removes nothing correct
a/b/c/../.. a ok, leaves a ok, leaves a a/b correct
a/b/c/d/../../.. a ok, leaves a ok, leaves a a/b a/b/c correct

The last two rows are the ones worth attention: both forms report success and remove
different directories.

Why the async form fails silently

_rmchildren() builds child paths by concatenating onto the unresolved path. Tracing the
fs calls for rm('<root>/a/b/c/../..'):

rmdir(<root>/a/b/c/../..)         -> ENOTEMPTY
readdir(<root>/a/b/c/../..)       -> ok, ['b']
  ... recursion removes <root>/a/b/c/d, then <root>/a/b/c
rmdir(<root>/a/b/c/../../b)       -> ENOENT
rmdir(<root>/a/b/c/../..)         -> ENOENT
=> resolves successfully, leaves a/ and a/b/ behind

The walk removes a/b/c, which the literal path needs in order to resolve. Every later call
then fails ENOENT, and rimraf() treats ENOENT as "already gone", so the failure is
reported as success.

The change

Resolve a trailing dot segment at the three rm entry points so both forms operate on the
same path.

Only a trailing . or .. is rewritten. That is exactly where the two implementations
diverge, and keeping every other path byte for byte identical matters: normalizing
unconditionally also rewrote ./foo to foo, which changed the resource string the
permission model reports on denial and broke test-permission-fs-write. Narrowing to the
trailing component leaves paths that already behaved correctly completely untouched.

This is not a new behaviour so much as making the three documented input types agree:
URL paths are already correct in every row above, because the WHATWG parser resolves dot
segments before the path reaches fs. string and Buffer paths were not.

Buffer paths are round tripped through latin1 rather than utf8, because filenames are
arbitrary byte sequences and a utf8 round trip rewrites invalid sequences to U+FFFD, which
would remove a different path than the caller asked for:

utf8    round-trip: CORRUPTED       612ffffe2e2e2f62 -> 612fefbfbdefbfbd2e2e2f62
latin1  round-trip: byte-identical  612ffffe2e2e2f62 -> 612ffffe2e2e2f62

Test

test/parallel/test-fs-rm-dot-segments.js covers nine path shapes across string, Buffer
and URL input, plus a directory whose name is not valid UTF-8. Each case asserts both that
the two forms agree and that the surviving tree is the one POSIX path resolution implies, so
the test still fails if both forms are wrong in the same way. It was written before the fix
and fails on current main.

Locally: the new test passes on this branch and fails on v26.7.0; all 280
parallel/test-fs-* tests pass; all parallel/test-permission-* tests pass except
test-permission-drop-ffi, which fails on a clean checkout too because the FFI fixture
library is not built. make lint-js is clean for the touched files.

Notes

Supersedes #61968, which has been stale since February. That PR took the same general
approach and the discussion there informed this one, but the implementation here is fresh
rather than carried over, so I have not added a co-author trailer. Happy to add one if that
is preferred.

fs.rmSync() throwing an error with an empty code (rows 3 and 4) is a separate defect in
RmSync() in src/node_file.cc, where the std::error_code translation is a hardcoded list
of four values and everything else becomes UV_UNKNOWN. This change stops a/. from reaching
that path but does not fix the mapping. Filed separately as #65884.

Disclosure: I used an AI coding assistant on this change. The behaviour matrix, the call
trace and the round trip check above are reproducible scripts run locally against v26.7.0 and
against a checkout of main; I have read through the patch and the test and can speak to
both.

fs.rmSync() dispatches to std::filesystem::remove_all() while fs.rm()
and fsPromises.rm() walk the tree in JavaScript, so the two forms
disagree whenever the trailing path component is `.` or `..`.

rmSync() removes the contents of the resolved target but leaves the
target itself behind, and for a trailing `.` it throws an error carrying
no code property. The promise form rejects with EINVAL for a trailing
`.`, and for a trailing `..` it reports success while removing a
directory below the one that was requested.

The silent case happens because _rmchildren() builds child paths by
concatenating onto the unresolved path. The walk removes a directory
that an unresolved `..` still needs in order to resolve, so every later
operation fails with ENOENT, which rimraf() treats as already deleted.

Resolve a trailing dot segment at the three entry points so both forms
operate on the same path. Only a trailing `.` or `..` is rewritten,
since that is where the two implementations diverge; every other path is
passed through unchanged, so paths that already behaved correctly keep
their existing behaviour, including the resource string the permission
model reports for them.

Buffer paths go through latin1 rather than utf8 because filenames are
arbitrary byte sequences, and a utf8 round trip rewrites invalid
sequences to U+FFFD, which would remove a different path than the one
requested.

Fixes: nodejs#61958

Signed-off-by: AmarWaqar-TSKLI <amarwaqar15@gmail.com>
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. labels Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

@codecov

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.17%. Comparing base (8af7545) to head (cd92400).
⚠️ Report is 24 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65883      +/-   ##
==========================================
- Coverage   90.18%   90.17%   -0.01%     
==========================================
  Files         771      771              
  Lines      264619   265126     +507     
  Branches    50231    50357     +126     
==========================================
+ Hits       238653   239085     +432     
- Misses      16961    17005      +44     
- Partials     9005     9036      +31     
Files with missing lines Coverage Δ
lib/fs.js 97.31% <100.00%> (+<0.01%) ⬆️
lib/internal/fs/promises.js 90.98% <100.00%> (-0.12%) ⬇️
lib/internal/fs/utils.js 96.37% <100.00%> (+0.08%) ⬆️

... and 54 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@MikeMcC399

Copy link
Copy Markdown
Contributor

This PR is failing CI tests.

See Pull requests > Step 6: Test

Before submitting your changes in a pull request, always run the full Node.js test suite.

with further details under BUILDING > Running tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants