Skip to content

fs: normalize paths with dot segments in rm and rmSync - #66472

Closed
RohithPariki wants to merge 1 commit into
nodejs:mainfrom
RohithPariki:fix/fs-rm-normalize-dotdot-paths
Closed

RohithPariki wants to merge 1 commit into
nodejs:mainfrom
RohithPariki:fix/fs-rm-normalize-dotdot-paths

Conversation

@RohithPariki

@RohithPariki RohithPariki commented Oct 3, 2026 •

Copy link
Copy Markdown
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

Description of change

When paths containing relative components (such as . or ..) are passed to fs.rm(), fs.rmSync(), or fs.promises.rm(), the different underlying implementations (binding.rmSync via std::filesystem::remove_all vs JS rimraf) produce divergent results:

  • In rimraf, child paths are constructed by concatenating onto the unresolved path. When recursion removes intermediate directory components, subsequent calls fail with ENOENT, which rimraf treats as "already deleted," causing silent partial removals.
  • rmSync throws an error without a .code property on paths with trailing /..

This PR normalizes dot segments before reaching the underlying removal routines:

  • Introduces normalizePathForRm() in lib/fs.js and lib/internal/fs/promises.js.
  • Correctly handles both string and Buffer inputs: for Buffer paths, binary byte content is preserved 1:1 using latin1 round-tripping before and after normalization to prevent data corruption on non-UTF-8 paths.
  • WHATWG URL inputs are converted to strings via getValidatedPath() before normalization.
Tests

Added regression tests in test/parallel/test-fs-rm.js covering:

  • The full relative path matrix (a/b/../., a/b/.., a/., a/b/c/., a/b/c/../.., a/b/c/d/../../.., a/b/.././b, ./a) across fs.rmSync, fs.rm (callback), and fs.promises.rm.
  • Buffer paths across sync and promises implementations.
  • Non-UTF-8 binary Buffer paths to ensure byte preservation.

Fixes: #61958

@github-actions

github-actions Bot commented Oct 3, 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.

Caution

AgentScan found account activity patterns that may be consistent with automation. This is a heuristic, not proof that this pull request was opened by an agent or violates policy. AI-assisted contributions are permitted, but automated tooling must not open pull requests without advance approval, and contributors must personally understand, test, verify, and take responsibility for every submitted change. See the AgentScan analysis, AI use policy, and automation policy for additional context.

@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 Oct 3, 2026
@inoway46

inoway46 commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

For a first-time contribution, we recommend focusing on one PR at a time. Could you close one of these PRs for now?
Ref: https://github.com/nodejs/node/blob/HEAD/doc/contributing/first-contributions.md#what-not-to-do-for-your-first-contributions

When paths containing '.' or '..' components (e.g., 'a/b/../.')
are passed to fs.rm(), fs.rmSync(), or fs.promises.rm(), the
different codepaths (native binding.rmSync vs JS rimraf) could
produce inconsistent results because the path was used as-is without
normalization.

Add pathModule.normalize() after getValidatedPath() in all three rm
variants to ensure dot-segment components are resolved before the
path reaches the underlying removal implementation.

Fixes: nodejs#61958
Signed-off-by: Rohith Pariki <[email protected]>
@RohithPariki
RohithPariki force-pushed the fix/fs-rm-normalize-dotdot-paths branch from 0ee0e2e to 0dc372c Compare October 3, 2026 11:56
@RohithPariki

Copy link
Copy Markdown
Author

Thank you for the guidance @inoway46. Since I am a first-time contributor, I will focus on my first PR #66471 and close this one temporarily. I have updated the commit message to follow the guidelines (50 chars summary, 72 chars wrap) for when I reopen it!

@RohithPariki

Copy link
Copy Markdown
Author

Superseded by #66673.

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

Labels

agentscan:mixed-signals 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.

Discrepancy between fs.rmSync and fs/promises.rm behavior with . and .. in paths

3 participants