fs: normalize trailing dot segments for rm

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: #61958

Signed-off-by: AmarWaqar-TSKLI <amarwaqar15@gmail.com>

Author: AmarWaqar-TSKLI

lib/fs.js

@@ -109,6 +109,7 @@ const {
   getValidatedFd,
   getValidatedPath,
   handleErrorFromBinding,
+  normalizeRmPath,
   preprocessSymlinkDestination,
   Stats,
   getReadFileBuffer,
@@ -1533,7 +1534,7 @@ function rm(path, options, callback) {
   const h = vfsState.handlers;
   if (h !== null && vfsVoid(h.rm(path, options), callback)) return;
 
-  path = getValidatedPath(path);
+  path = normalizeRmPath(getValidatedPath(path));
 
   validateRmOptions(path, options, false, (err, options) => {
     if (err) {
@@ -1562,8 +1563,9 @@ function rmSync(path, options) {
     const result = h.rmSync(path, options);
     if (result !== undefined) return;
   }
+  path = normalizeRmPath(getValidatedPath(path));
   const opts = validateRmOptionsSync(path, options, false);
-  return binding.rmSync(getValidatedPath(path), opts.maxRetries, opts.recursive, opts.retryDelay);
+  return binding.rmSync(path, opts.maxRetries, opts.recursive, opts.retryDelay);
 }
 
 /**

lib/internal/fs/promises.js

@@ -71,6 +71,7 @@ const {
   getValidatedPath,
   getReadFileBuffer,
   getReadFileBufferByteLengthName,
+  normalizeRmPath,
   preprocessSymlinkDestination,
   stringToFlags,
   stringToSymlinkType,
@@ -1548,7 +1549,7 @@ async function rm(path, options) {
     const promise = h.rm(path, options);
     if (promise !== undefined) { await promise; return; }
   }
-  path = getValidatedPath(path);
+  path = normalizeRmPath(getValidatedPath(path));
   options = await validateRmOptionsPromise(path, options, false);
   return lazyRimRaf()(path, options);
 }

lib/internal/fs/utils.js

@@ -940,6 +940,34 @@ const getValidatedPath = hideStackFrames((fileURLOrPath, propName = 'path') => {
   return path;
 });
 
+// fs.rm(), fs.rmSync() and fsPromises.rm() resolve a trailing `.` or `..` before
+// removing anything. Without this the two forms disagree, because rmSync()
+// dispatches to std::filesystem::remove_all() while rm() walks the tree in JS.
+// The JS walk can also invalidate its own path: it builds child paths by
+// concatenation, so removing a directory that an unresolved `..` still needs in
+// order to resolve makes every later operation fail with ENOENT, which is then
+// reported as success. Refs: https://github.com/nodejs/node/issues/61958
+//
+// Only a trailing dot segment is rewritten. That is where rmdir(2) rejects the
+// path outright and where the walk can outlive its own resolution; leaving every
+// other path byte for byte identical keeps this from changing unrelated
+// behaviour, such as the resource string the permission model reports.
+const normalizeRmPath = (path) => {
+  if (typeof path === 'string') {
+    const base = pathModule.basename(path);
+    return base === '.' || base === '..' ? pathModule.normalize(path) : path;
+  }
+  // Filenames are arbitrary byte sequences on POSIX. latin1 maps every byte to a
+  // distinct code point and back, so the bytes survive the round trip; utf8
+  // would rewrite invalid sequences to U+FFFD and change which path is removed.
+  const asString = Buffer.from(path).toString('latin1');
+  const base = pathModule.basename(asString);
+  if (base !== '.' && base !== '..') {
+    return path;
+  }
+  return Buffer.from(pathModule.normalize(asString), 'latin1');
+};
+
 const getValidatedFd = hideStackFrames((fd, propName = 'fd') => {
   if (ObjectIs(fd, -0)) {
     return 0;
@@ -1214,6 +1242,7 @@ module.exports = {
   getValidatedFd,
   getValidatedPath,
   handleErrorFromBinding,
+  normalizeRmPath,
   preprocessSymlinkDestination,
   realpathCacheKey: Symbol('realpathCacheKey'),
   getStatFsFromBinding,

test/parallel/test-fs-rm-dot-segments.js

@@ -0,0 +1,219 @@
+'use strict';
+
+// Regression test for https://github.com/nodejs/node/issues/61958
+//
+// fs.rmSync() and fsPromises.rm() are documented as the synchronous and
+// asynchronous forms of one API, but they do not share an implementation:
+// rmSync() dispatches to binding.rmSync() (std::filesystem::remove_all) while
+// rm() and fsPromises.rm() use the JS rimraf in lib/internal/fs/rimraf.js.
+// They disagree for paths whose trailing component is `.` or `..`: the sync
+// form removes the directory contents and reports success, while the async
+// form rejects with EINVAL and removes nothing.
+//
+// Each case asserts two things. First the invariant from the bug report: for
+// identical input the two forms must succeed or fail the same way and leave
+// the filesystem in the same state. Second, that the surviving tree is the one
+// POSIX path resolution implies, so that the test still fails if both forms
+// are wrong in the same way.
+
+const common = require('../common');
+const tmpdir = require('../common/tmpdir');
+const assert = require('node:assert');
+const fs = require('node:fs');
+const fsPromises = require('node:fs/promises');
+const path = require('node:path');
+const { pathToFileURL } = require('node:url');
+
+tmpdir.refresh();
+
+const RM_OPTIONS = { recursive: true, force: true };
+
+// Path shapes whose trailing component is `.` or `..`, plus controls that
+// should be unaffected. Kept as raw strings: path.join() would normalize the
+// dot segments away and destroy the case under test.
+const DOT_SEGMENT_PATHS = [
+  'a/b/../.',      // The shape reported in the issue.
+  'a/b/..',
+  'a/b/.././b',
+  'a/.',
+  'a/b/c/.',
+  'a/b/c/../..',
+  'a/b/c/d/../../..',
+  './a',           // Control: leading `.` only.
+  'a/b',           // Control: no dot segments at all.
+];
+
+// The fixture tree, in the shape listTree() reports it.
+const FIXTURE_ENTRIES = ['a', 'a/b', 'a/b/c', 'a/b/c/d'];
+
+// What must still exist afterwards: every entry that is neither the resolved
+// target nor below it. Derived from POSIX path resolution rather than from what
+// either implementation happens to do, so that agreeing on a wrong answer still
+// fails the test.
+function expectedSurvivors(relative) {
+  const target = path.posix.normalize(relative);
+  return FIXTURE_ENTRIES.filter(
+    (entry) => entry !== target && !entry.startsWith(`${target}/`));
+}
+
+let fixtureCounter = 0;
+
+// Builds <tmpdir>/rm-fixture-N/a/b/c/d and returns the fixture root.
+function makeFixture() {
+  const root = tmpdir.resolve(`rm-fixture-${fixtureCounter++}`);
+  fs.mkdirSync(path.join(root, 'a', 'b', 'c', 'd'), { recursive: true });
+  return root;
+}
+
+// Sorted, root-relative listing of everything under `root`, with `/` as the
+// separator so two runs can be compared directly on any platform.
+function listTree(root) {
+  const entries = [];
+  (function walk(dir) {
+    for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
+      const absolute = path.join(dir, entry.name);
+      entries.push(path.relative(root, absolute).split(path.sep).join('/'));
+      if (entry.isDirectory()) walk(absolute);
+    }
+  })(root);
+  return entries.sort();
+}
+
+// Joins by hand rather than with path.join() to preserve dot segments.
+function targetPath(root, relative, encoding) {
+  const raw = `${root}/${relative}`;
+  if (encoding === 'string') return raw;
+  if (encoding === 'buffer') return Buffer.from(raw);
+  if (encoding === 'url') return pathToFileURL(raw);
+  assert.fail(`unknown encoding ${encoding}`);
+}
+
+// Normalizes an outcome to something comparable across the two code paths.
+function settled(fn) {
+  try {
+    fn();
+    return { outcome: 'success' };
+  } catch (err) {
+    return { outcome: 'failure', code: err.code };
+  }
+}
+
+async function settledAsync(fn) {
+  try {
+    await fn();
+    return { outcome: 'success' };
+  } catch (err) {
+    return { outcome: 'failure', code: err.code };
+  }
+}
+
+// Runs the sync and async forms against separate but identical fixtures and
+// reports what each did.
+async function compareForms(relative, encoding) {
+  const syncRoot = makeFixture();
+  const syncResult = settled(
+    () => fs.rmSync(targetPath(syncRoot, relative, encoding), RM_OPTIONS));
+  const syncTree = listTree(syncRoot);
+
+  const asyncRoot = makeFixture();
+  const asyncResult = await settledAsync(
+    () => fsPromises.rm(targetPath(asyncRoot, relative, encoding), RM_OPTIONS));
+  const asyncTree = listTree(asyncRoot);
+
+  return { syncResult, syncTree, asyncResult, asyncTree };
+}
+
+async function assertFormsAgree(relative, encoding) {
+  const label = `rm('${relative}') with a ${encoding} path`;
+  const { syncResult, syncTree, asyncResult, asyncTree } =
+    await compareForms(relative, encoding);
+
+  assert.deepStrictEqual(
+    asyncResult, syncResult,
+    `${label}: fsPromises.rm() and fs.rmSync() disagree. ` +
+    `sync=${JSON.stringify(syncResult)} async=${JSON.stringify(asyncResult)}`);
+
+  assert.deepStrictEqual(
+    asyncTree, syncTree,
+    `${label}: fsPromises.rm() and fs.rmSync() left different trees behind. ` +
+    `sync=${JSON.stringify(syncTree)} async=${JSON.stringify(asyncTree)}`);
+
+  const expected = expectedSurvivors(relative);
+
+  assert.deepStrictEqual(
+    syncResult, { outcome: 'success' },
+    `${label}: fs.rmSync() should remove the resolved target, but reported ` +
+    JSON.stringify(syncResult));
+
+  assert.deepStrictEqual(
+    syncTree, expected,
+    `${label}: fs.rmSync() left the wrong tree. ` +
+    `got=${JSON.stringify(syncTree)} want=${JSON.stringify(expected)}`);
+
+  assert.deepStrictEqual(
+    asyncTree, expected,
+    `${label}: fsPromises.rm() left the wrong tree. ` +
+    `got=${JSON.stringify(asyncTree)} want=${JSON.stringify(expected)}`);
+}
+
+// A path whose bytes are not valid UTF-8. Normalizing a Buffer path through a
+// UTF-8 round trip rewrites these bytes to U+FFFD, which would resolve to a
+// different path than the caller asked for -- silently, in an API that deletes.
+// POSIX filenames are arbitrary bytes; Windows filenames are not, so this is
+// skipped there.
+async function assertNonUtf8BufferPathsSurvive() {
+  const oddName = Buffer.from([0xff, 0xfe]);
+
+  for (const form of ['sync', 'async']) {
+    // A bare directory, not makeFixture(): this case only needs the oddly named
+    // subtree, and anything else in the root would just be noise here.
+    const root = tmpdir.resolve(`rm-nonutf8-${fixtureCounter++}`);
+    fs.mkdirSync(root, { recursive: true });
+    const oddDir = Buffer.concat([Buffer.from(`${root}/`), oddName]);
+    fs.mkdirSync(oddDir);
+    fs.mkdirSync(Buffer.concat([oddDir, Buffer.from('/child')]));
+
+    // Resolves to oddDir itself, so the whole directory should be removed.
+    const target = Buffer.concat([oddDir, Buffer.from('/child/..')]);
+
+    if (form === 'sync') {
+      fs.rmSync(target, RM_OPTIONS);
+    } else {
+      await fsPromises.rm(target, RM_OPTIONS);
+    }
+
+    assert.strictEqual(
+      fs.existsSync(oddDir), false,
+      `fs.rm (${form}) left a directory with non-UTF-8 bytes in its name behind; ` +
+      'the path was probably rewritten during normalization');
+    assert.deepStrictEqual(
+      listTree(root), [],
+      `fs.rm (${form}) left something behind under the fixture root`);
+  }
+}
+
+(async () => {
+  // String paths: the form reported in the issue.
+  for (const relative of DOT_SEGMENT_PATHS) {
+    await assertFormsAgree(relative, 'string');
+  }
+
+  // Buffer paths. fs.rm() documents `string | Buffer | URL`, so the dot
+  // segment handling must not depend on how the path was supplied. This is
+  // the case the reviewer asked about on PR #61968 and that was never
+  // answered.
+  for (const relative of DOT_SEGMENT_PATHS) {
+    await assertFormsAgree(relative, 'buffer');
+  }
+
+  // URL paths. The WHATWG URL parser resolves dot segments itself, so both
+  // forms should receive an already-normalized path here; this pins that
+  // assumption so a future change to path handling cannot silently break it.
+  for (const relative of DOT_SEGMENT_PATHS) {
+    await assertFormsAgree(relative, 'url');
+  }
+
+  if (!common.isWindows) {
+    await assertNonUtf8BufferPathsSurvive();
+  }
+})().then(common.mustCall());