From 305a8e9ea379c3872ab7fda46ace19b05fb220be Mon Sep 17 00:00:00 2001 From: RajeshKumar11 Date: Tue, 24 Feb 2026 22:27:53 +0530 Subject: [PATCH] lib: normalize . and .. in path before rm/rmSync When fs.rm, fs.rmSync, or fs.promises.rm receive a path containing '.' or '..' components (e.g. 'a/b/../.'), the sync and async implementations behaved differently: - rmSync (C++ binding): std::filesystem::remove_all deleted the directory's children, but when trying to remove the top-level entry the path became invalid (the '..' referred to a now-deleted directory), causing the parent directory to be left behind. - promises.rm / rm (JS rimraf): rmdir(2) on a path ending in '.' returns EINVAL on POSIX, so the operation failed immediately without removing anything. Fix by calling path.normalize() on string paths immediately after getValidatedPath(), before the path is passed to either rimraf or the C++ binding. This resolves '.' and '..' lexically (e.g. 'a/b/../.' becomes 'a'), giving both code paths a clean, unambiguous path to operate on. Fixes: https://github.com/nodejs/node/issues/61958 --- lib/fs.js | 23 ++++++++++++++++++++++- lib/internal/fs/promises.js | 15 +++++++++++++++ test/parallel/test-fs-rm.js | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 1 deletion(-) diff --git a/lib/fs.js b/lib/fs.js index 4a03fada49ea8a..b0da37c59db473 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -149,6 +149,10 @@ const { const permission = require('internal/process/permission'); +// Matches a dotdot path component (e.g. `..`, `/../`, `../`) but not +// filenames with consecutive dots like `a..b`. +const dotdotRe = /(^|[\\/])\.\.(?:[\\/]|$)/; + let fs; // Lazy loaded @@ -1179,6 +1183,14 @@ function rm(path, options, callback) { options = undefined; } path = getValidatedPath(path); + if (typeof path === 'string') { + if (SideEffectFreeRegExpPrototypeExec(dotdotRe, path) !== null) + path = pathModule.normalize(path); + } else if (isArrayBufferView(path)) { + const str = BufferToString(path, 'utf8'); + if (SideEffectFreeRegExpPrototypeExec(dotdotRe, str) !== null) + path = pathModule.normalize(str); + } validateRmOptions(path, options, false, (err, options) => { if (err) { @@ -1202,8 +1214,17 @@ function rm(path, options, callback) { * @returns {void} */ function rmSync(path, options) { + path = getValidatedPath(path); + if (typeof path === 'string') { + if (SideEffectFreeRegExpPrototypeExec(dotdotRe, path) !== null) + path = pathModule.normalize(path); + } else if (isArrayBufferView(path)) { + const str = BufferToString(path, 'utf8'); + if (SideEffectFreeRegExpPrototypeExec(dotdotRe, str) !== null) + path = pathModule.normalize(str); + } 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); } /** diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 2f95c4b79e17fd..6aea189d80acd3 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -17,6 +17,7 @@ const { Symbol, SymbolAsyncDispose, Uint8Array, + uncurryThis, } = primordials; const { fs: constants } = internalBinding('constants'); @@ -30,6 +31,7 @@ const { const binding = internalBinding('fs'); const { Buffer } = require('buffer'); +const BufferToString = uncurryThis(Buffer.prototype.toString); const { AbortError, @@ -92,6 +94,7 @@ const { promisify, isWindows, isMacOS, + SideEffectFreeRegExpPrototypeExec, } = require('internal/util'); const EventEmitter = require('events'); const { StringDecoder } = require('string_decoder'); @@ -102,6 +105,10 @@ const assert = require('internal/assert'); const permission = require('internal/process/permission'); +// Matches a dotdot path component (e.g. `..`, `/../`, `../`) but not +// filenames with consecutive dots like `a..b`. +const dotdotRe = /(^|[\\/])\.\.(?:[\\/]|$)/; + const kHandle = Symbol('kHandle'); const kFd = Symbol('kFd'); const kRefs = Symbol('kRefs'); @@ -802,6 +809,14 @@ async function ftruncate(handle, len = 0) { async function rm(path, options) { path = getValidatedPath(path); + if (typeof path === 'string') { + if (SideEffectFreeRegExpPrototypeExec(dotdotRe, path) !== null) + path = pathModule.normalize(path); + } else if (isArrayBufferView(path)) { + const str = BufferToString(path, 'utf8'); + if (SideEffectFreeRegExpPrototypeExec(dotdotRe, str) !== null) + path = pathModule.normalize(str); + } options = await validateRmOptionsPromise(path, options, false); return lazyRimRaf()(path, options); } diff --git a/test/parallel/test-fs-rm.js b/test/parallel/test-fs-rm.js index 4104c2e948da0e..c5de1ff66bc577 100644 --- a/test/parallel/test-fs-rm.js +++ b/test/parallel/test-fs-rm.js @@ -569,3 +569,38 @@ if (isGitPresent) { } } } + +// Test that rm/rmSync normalize '.' and '..' in paths before processing. +// Regression test for https://github.com/nodejs/node/issues/61958 +// +// Each variant gets its own base directory to avoid async/sync races. +// The weird path is constructed by joining components with path.sep so that +// the '..' and '.' are preserved and not pre-normalized by path.join. +{ + // --- rmSync: /a/b/../. should remove /a entirely --- + const base = nextDirPath('dotdot-sync'); + fs.mkdirSync(path.join(base, 'a', 'b', 'c', 'd'), common.mustNotMutateObjectDeep({ recursive: true })); + const weirdPath = [base, 'a', 'b', '..', '.'].join(path.sep); + fs.rmSync(weirdPath, common.mustNotMutateObjectDeep({ recursive: true })); + assert.strictEqual(fs.existsSync(path.join(base, 'a')), false); +} + +{ + // --- fs.rm (callback): same path construction --- + const base = nextDirPath('dotdot-cb'); + fs.mkdirSync(path.join(base, 'a', 'b', 'c', 'd'), common.mustNotMutateObjectDeep({ recursive: true })); + const weirdPath = [base, 'a', 'b', '..', '.'].join(path.sep); + fs.rm(weirdPath, common.mustNotMutateObjectDeep({ recursive: true }), common.mustSucceed(() => { + assert.strictEqual(fs.existsSync(path.join(base, 'a')), false); + })); +} + +{ + // --- fs.promises.rm: same path construction --- + const base = nextDirPath('dotdot-prom'); + fs.mkdirSync(path.join(base, 'a', 'b', 'c', 'd'), common.mustNotMutateObjectDeep({ recursive: true })); + const weirdPath = [base, 'a', 'b', '..', '.'].join(path.sep); + fs.promises.rm(weirdPath, common.mustNotMutateObjectDeep({ recursive: true })).then(common.mustCall(() => { + assert.strictEqual(fs.existsSync(path.join(base, 'a')), false); + })); +}