diff --git a/lib/fs.js b/lib/fs.js index 4a03fada49ea8a..6c70488d105b8e 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1179,6 +1179,14 @@ function rm(path, options, callback) { options = undefined; } path = getValidatedPath(path); + if (typeof path === 'string') { + if (StringPrototypeIndexOf(path, '..') !== -1) + path = pathModule.normalize(path); + } else if (isArrayBufferView(path)) { + const str = BufferToString(path, 'utf8'); + if (StringPrototypeIndexOf(str, '..') !== -1) + path = pathModule.normalize(str); + } validateRmOptions(path, options, false, (err, options) => { if (err) { @@ -1202,8 +1210,17 @@ function rm(path, options, callback) { * @returns {void} */ function rmSync(path, options) { + path = getValidatedPath(path); + if (typeof path === 'string') { + if (StringPrototypeIndexOf(path, '..') !== -1) + path = pathModule.normalize(path); + } else if (isArrayBufferView(path)) { + const str = BufferToString(path, 'utf8'); + if (StringPrototypeIndexOf(str, '..') !== -1) + 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..1ae113a19b1e9a 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -14,9 +14,11 @@ const { PromiseResolve, SafeArrayIterator, SafePromisePrototypeFinally, + StringPrototypeIncludes, Symbol, SymbolAsyncDispose, Uint8Array, + uncurryThis, } = primordials; const { fs: constants } = internalBinding('constants'); @@ -30,6 +32,7 @@ const { const binding = internalBinding('fs'); const { Buffer } = require('buffer'); +const BufferToString = uncurryThis(Buffer.prototype.toString); const { AbortError, @@ -802,6 +805,14 @@ async function ftruncate(handle, len = 0) { async function rm(path, options) { path = getValidatedPath(path); + if (typeof path === 'string') { + if (StringPrototypeIncludes(path, '..')) + path = pathModule.normalize(path); + } else if (isArrayBufferView(path)) { + const str = BufferToString(path, 'utf8'); + if (StringPrototypeIncludes(str, '..')) + 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); + })); +}