diff --git a/lib/fs.js b/lib/fs.js index b858902cf44a..9398b71daf1b 100644 --- a/lib/fs.js +++ b/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); } /** diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index d5a6b9a2c853..3ca02798245b 100644 --- a/lib/internal/fs/promises.js +++ b/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); } diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 3aadc19c4732..771278906b10 100644 --- a/lib/internal/fs/utils.js +++ b/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, diff --git a/test/parallel/test-fs-rm-dot-segments.js b/test/parallel/test-fs-rm-dot-segments.js new file mode 100644 index 000000000000..ab34b43e8fbf --- /dev/null +++ b/test/parallel/test-fs-rm-dot-segments.js @@ -0,0 +1,216 @@ +'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 /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']) { + const root = makeFixture(); + 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}) did not remove the fixture cleanly`); + } +} + +(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());