Skip to content

Commit 9e032a9

Browse files
committed
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: #61958
1 parent 9145cc6 commit 9e032a9

File tree

3 files changed

+44
-1
lines changed

3 files changed

+44
-1
lines changed

lib/fs.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1179,6 +1179,8 @@ function rm(path, options, callback) {
11791179
options = undefined;
11801180
}
11811181
path = getValidatedPath(path);
1182+
if (typeof path === 'string' && StringPrototypeIndexOf(path, '..') !== -1)
1183+
path = pathModule.normalize(path);
11821184

11831185
validateRmOptions(path, options, false, (err, options) => {
11841186
if (err) {
@@ -1202,8 +1204,11 @@ function rm(path, options, callback) {
12021204
* @returns {void}
12031205
*/
12041206
function rmSync(path, options) {
1207+
path = getValidatedPath(path);
1208+
if (typeof path === 'string' && StringPrototypeIndexOf(path, '..') !== -1)
1209+
path = pathModule.normalize(path);
12051210
const opts = validateRmOptionsSync(path, options, false);
1206-
return binding.rmSync(getValidatedPath(path), opts.maxRetries, opts.recursive, opts.retryDelay);
1211+
return binding.rmSync(path, opts.maxRetries, opts.recursive, opts.retryDelay);
12071212
}
12081213

12091214
/**

lib/internal/fs/promises.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ const {
1414
PromiseResolve,
1515
SafeArrayIterator,
1616
SafePromisePrototypeFinally,
17+
StringPrototypeIncludes,
1718
Symbol,
1819
SymbolAsyncDispose,
1920
Uint8Array,
@@ -802,6 +803,8 @@ async function ftruncate(handle, len = 0) {
802803

803804
async function rm(path, options) {
804805
path = getValidatedPath(path);
806+
if (typeof path === 'string' && StringPrototypeIncludes(path, '..'))
807+
path = pathModule.normalize(path);
805808
options = await validateRmOptionsPromise(path, options, false);
806809
return lazyRimRaf()(path, options);
807810
}

test/parallel/test-fs-rm.js

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -569,3 +569,38 @@ if (isGitPresent) {
569569
}
570570
}
571571
}
572+
573+
// Test that rm/rmSync normalize '.' and '..' in paths before processing.
574+
// Regression test for https://github.com/nodejs/node/issues/61958
575+
//
576+
// Each variant gets its own base directory to avoid async/sync races.
577+
// The weird path is constructed by joining components with path.sep so that
578+
// the '..' and '.' are preserved and not pre-normalized by path.join.
579+
{
580+
// --- rmSync: <base>/a/b/../. should remove <base>/a entirely ---
581+
const base = nextDirPath('dotdot-sync');
582+
fs.mkdirSync(path.join(base, 'a', 'b', 'c', 'd'), common.mustNotMutateObjectDeep({ recursive: true }));
583+
const weirdPath = [base, 'a', 'b', '..', '.'].join(path.sep);
584+
fs.rmSync(weirdPath, common.mustNotMutateObjectDeep({ recursive: true }));
585+
assert.strictEqual(fs.existsSync(path.join(base, 'a')), false);
586+
}
587+
588+
{
589+
// --- fs.rm (callback): same path construction ---
590+
const base = nextDirPath('dotdot-cb');
591+
fs.mkdirSync(path.join(base, 'a', 'b', 'c', 'd'), common.mustNotMutateObjectDeep({ recursive: true }));
592+
const weirdPath = [base, 'a', 'b', '..', '.'].join(path.sep);
593+
fs.rm(weirdPath, common.mustNotMutateObjectDeep({ recursive: true }), common.mustSucceed(() => {
594+
assert.strictEqual(fs.existsSync(path.join(base, 'a')), false);
595+
}));
596+
}
597+
598+
{
599+
// --- fs.promises.rm: same path construction ---
600+
const base = nextDirPath('dotdot-prom');
601+
fs.mkdirSync(path.join(base, 'a', 'b', 'c', 'd'), common.mustNotMutateObjectDeep({ recursive: true }));
602+
const weirdPath = [base, 'a', 'b', '..', '.'].join(path.sep);
603+
fs.promises.rm(weirdPath, common.mustNotMutateObjectDeep({ recursive: true })).then(common.mustCall(() => {
604+
assert.strictEqual(fs.existsSync(path.join(base, 'a')), false);
605+
}));
606+
}

0 commit comments

Comments
 (0)