Skip to content

Commit 9ce510e

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 9ce510e

File tree

3 files changed

+64
-1
lines changed

3 files changed

+64
-1
lines changed

lib/fs.js

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1179,6 +1179,14 @@ function rm(path, options, callback) {
11791179
options = undefined;
11801180
}
11811181
path = getValidatedPath(path);
1182+
if (typeof path === 'string') {
1183+
if (StringPrototypeIndexOf(path, '..') !== -1)
1184+
path = pathModule.normalize(path);
1185+
} else if (isArrayBufferView(path)) {
1186+
const str = BufferToString(path, 'utf8');
1187+
if (StringPrototypeIndexOf(str, '..') !== -1)
1188+
path = pathModule.normalize(str);
1189+
}
11821190

11831191
validateRmOptions(path, options, false, (err, options) => {
11841192
if (err) {
@@ -1202,8 +1210,17 @@ function rm(path, options, callback) {
12021210
* @returns {void}
12031211
*/
12041212
function rmSync(path, options) {
1213+
path = getValidatedPath(path);
1214+
if (typeof path === 'string') {
1215+
if (StringPrototypeIndexOf(path, '..') !== -1)
1216+
path = pathModule.normalize(path);
1217+
} else if (isArrayBufferView(path)) {
1218+
const str = BufferToString(path, 'utf8');
1219+
if (StringPrototypeIndexOf(str, '..') !== -1)
1220+
path = pathModule.normalize(str);
1221+
}
12051222
const opts = validateRmOptionsSync(path, options, false);
1206-
return binding.rmSync(getValidatedPath(path), opts.maxRetries, opts.recursive, opts.retryDelay);
1223+
return binding.rmSync(path, opts.maxRetries, opts.recursive, opts.retryDelay);
12071224
}
12081225

12091226
/**

lib/internal/fs/promises.js

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,11 @@ const {
1414
PromiseResolve,
1515
SafeArrayIterator,
1616
SafePromisePrototypeFinally,
17+
StringPrototypeIncludes,
1718
Symbol,
1819
SymbolAsyncDispose,
1920
Uint8Array,
21+
uncurryThis,
2022
} = primordials;
2123

2224
const { fs: constants } = internalBinding('constants');
@@ -30,6 +32,7 @@ const {
3032

3133
const binding = internalBinding('fs');
3234
const { Buffer } = require('buffer');
35+
const BufferToString = uncurryThis(Buffer.prototype.toString);
3336

3437
const {
3538
AbortError,
@@ -802,6 +805,14 @@ async function ftruncate(handle, len = 0) {
802805

803806
async function rm(path, options) {
804807
path = getValidatedPath(path);
808+
if (typeof path === 'string') {
809+
if (StringPrototypeIncludes(path, '..'))
810+
path = pathModule.normalize(path);
811+
} else if (isArrayBufferView(path)) {
812+
const str = BufferToString(path, 'utf8');
813+
if (StringPrototypeIncludes(str, '..'))
814+
path = pathModule.normalize(str);
815+
}
805816
options = await validateRmOptionsPromise(path, options, false);
806817
return lazyRimRaf()(path, options);
807818
}

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)