diff --git a/src/js/node/fs.promises.ts b/src/js/node/fs.promises.ts index f973f229726..60f75d224d0 100644 --- a/src/js/node/fs.promises.ts +++ b/src/js/node/fs.promises.ts @@ -9,6 +9,8 @@ const { validateObject, validateAbortSignal, validateEncoding, + validateInt32, + validateUint32, } = require("internal/validators"); const constants = $processBindingConstants.fs; @@ -207,6 +209,70 @@ async function opendir(dir: string, options) { return promise; } +// fs.rm, fs.rmSync and fs.promises.rm all go through node's validateRmOptions +// (https://github.com/nodejs/node/blob/v26.3.0/lib/internal/fs/utils.js#L905-L1006): +// the options are checked first, then the path is lstat'ed and a directory is +// refused with ERR_FS_EISDIR unless `recursive` is set. Keeping that order is +// what makes a bad option win over ERR_FS_EISDIR when both apply. The option +// half is shared here; the lstat half differs per flavor (sync below, async in +// rmValidated). Any lstat failure is left for the native rm to report, since it +// already implements node's `force`/ENOENT handling. +const defaultRmOptions = { + recursive: false, + force: false, + retryDelay: 100, + maxRetries: 0, +}; + +function validateRmOptions(options) { + if (options === undefined) return { ...defaultRmOptions }; + validateObject(options, "options"); + // Like node, the spread copies own enumerable keys only, so an own + // `recursive: undefined` overrides the default and is rejected below. + options = { ...defaultRmOptions, ...options }; + validateBoolean(options.force, "options.force"); + validateBoolean(options.recursive, "options.recursive"); + validateInt32(options.retryDelay, "options.retryDelay", 0); + validateUint32(options.maxRetries, "options.maxRetries"); + return options; +} + +function rmEisdirError(path) { + return require("internal/fs/cp-sync").fsEisdirError({ + code: "EISDIR", + message: "is a directory", + path, + syscall: "rm", + errno: $processBindingConstants.os.errno.EISDIR, + }); +} + +function validateRmOptionsSync(path, options) { + options = validateRmOptions(options); + if (!options.recursive) { + let stats; + try { + stats = fs.lstatSync(path); + } catch {} + if (stats?.isDirectory()) throw rmEisdirError(path); + } + return options; +} + +// `options` must already have been through validateRmOptions. fs.rm runs that +// itself before calling this so that, as in node, invalid options throw +// synchronously instead of reaching the callback. +async function rmValidated(path, options) { + if (!options.recursive) { + let stats; + try { + stats = await fs.lstat(path); + } catch {} + if (stats?.isDirectory()) throw rmEisdirError(path); + } + return fs.rm(path, options); +} + // Node.js closes a FileHandle's fd in its native finalizer and raises // ERR_INVALID_STATE (DEP0137 end-of-life) when collected without close(). // Mirror that with a FinalizationRegistry so dropped handles don't leak fds. @@ -235,6 +301,10 @@ const private_symbols = { kTransferList, kDeserialize, FileHandle: null as any, + // shared with node:fs, whose rm/rmSync are the callback and sync flavors of rm below + validateRmOptions, + validateRmOptionsSync, + rmValidated, }; const _readFile = fs.readFile.bind(fs); @@ -333,32 +403,7 @@ const exports = { utimes: asyncWrap(fs.utimes, "utimes"), lutimes: asyncWrap(fs.lutimes, "lutimes"), rm: async function rm(path, options) { - if (typeof options === "object" && options !== null) { - // Node merges the caller's options over the defaults with a spread, which - // copies own enumerable keys only -- including ones holding `undefined`. - // Normalize here so the native parser sees exactly that set. - options = { ...options }; - } - if (!options?.recursive) { - // node validates in JS and reports ERR_FS_EISDIR for directories - // (same check as rmSync) - let stats; - try { - stats = await fs.lstat(path); - } catch { - // let the native call produce the error (respects force/ENOENT) - } - if (stats?.isDirectory()) { - throw require("internal/fs/cp-sync").fsEisdirError({ - code: "EISDIR", - message: "is a directory", - path, - syscall: "rm", - errno: $processBindingConstants.os.errno.EISDIR, - }); - } - } - return fs.rm(path, options); + return rmValidated(path, validateRmOptions(options)); }, rmdir: async function rmdir(path, options) { // node throws for any defined `recursive`, not just truthy ones @@ -387,7 +432,7 @@ const exports = { opendir, // "$data" is reuse of private symbol - // this is used to export the private symbols to internal/fs/streams and node:http2 without making them public. + // this is used to export the private symbols and helpers to node:fs, internal/fs/streams and node:http2 without making them public. $data: private_symbols, }; export default exports; diff --git a/src/js/node/fs.ts b/src/js/node/fs.ts index 3e1569eb188..1a84a3692b4 100644 --- a/src/js/node/fs.ts +++ b/src/js/node/fs.ts @@ -17,6 +17,9 @@ const isDate = types.isDate; // The native `node:fs` binding, shared via `internal/fs/binding`. const fs = require("internal/fs/binding"); +// rm's option validation and ERR_FS_EISDIR check live next to promises.rm. +const { validateRmOptions, validateRmOptionsSync, rmValidated } = promises.$data; + const constants = $processBindingConstants.fs; var _lazyGlob; function lazyGlob() { @@ -92,8 +95,9 @@ var access = function access(path, mode, callback) { } callback = ensureCallback(callback); - // route through promises.rm for the JS-side ERR_FS_EISDIR validation - promises.rm(path, options).then(nullcallback(callback), callback); + // node's callback form throws synchronously on invalid options + options = validateRmOptions(options); + rmValidated(path, options).then(nullcallback(callback), callback); }, rmdir = function rmdir(path, options, callback) { if ($isCallable(options)) { @@ -555,31 +559,7 @@ var access = function access(path, mode, callback) { utimesSync = fs.utimesSync.bind(fs), lutimesSync = fs.lutimesSync.bind(fs), rmSync = function rmSync(path, options) { - if (typeof options === "object" && options !== null) { - // Node merges the caller's options over the defaults with a spread, which - // copies own enumerable keys only -- including ones holding `undefined`. - // Normalize here so the native parser sees exactly that set. - options = { ...options }; - } - if (!options?.recursive) { - // node validates in JS and reports ERR_FS_EISDIR for directories - let stats; - try { - stats = fs.lstatSync(path); - } catch { - // let the native call produce the error (respects force/ENOENT) - } - if (stats?.isDirectory()) { - throw require("internal/fs/cp-sync").fsEisdirError({ - code: "EISDIR", - message: "is a directory", - path, - syscall: "rm", - errno: $processBindingConstants.os.errno.EISDIR, - }); - } - } - return fs.rmSync(path, options); + return fs.rmSync(path, validateRmOptionsSync(path, options)); }, rmdirSync = function rmdirSync(path, options) { // node throws for any defined `recursive`, not just truthy ones diff --git a/test/js/node/fs/promises.test.js b/test/js/node/fs/promises.test.js index 47a025595ea..a01249c15bb 100644 --- a/test/js/node/fs/promises.test.js +++ b/test/js/node/fs/promises.test.js @@ -359,6 +359,96 @@ it("rm and promises.rm report ERR_FS_EISDIR for directories like rmSync", async expect(fs.existsSync(target)).toBe(false); }); +// node validates the options (validateRmOptions in lib/internal/fs/utils.js) +// before the lstat that produces ERR_FS_EISDIR, so on a directory target an +// invalid option is what gets reported. Expected codes and messages are node +// v26.3.0's. +describe("rm option validation runs before the ERR_FS_EISDIR check", () => { + const cases = [ + [ + { force: "x" }, + "ERR_INVALID_ARG_TYPE", + `The "options.force" property must be of type boolean. Received type string ('x')`, + ], + [ + { recursive: 0 }, + "ERR_INVALID_ARG_TYPE", + `The "options.recursive" property must be of type boolean. Received type number (0)`, + ], + [ + { recursive: undefined }, + "ERR_INVALID_ARG_TYPE", + `The "options.recursive" property must be of type boolean. Received undefined`, + ], + [ + { maxRetries: "x" }, + "ERR_INVALID_ARG_TYPE", + `The "options.maxRetries" property must be of type number. Received type string ('x')`, + ], + [ + { maxRetries: -1 }, + "ERR_OUT_OF_RANGE", + `The value of "options.maxRetries" is out of range. It must be >= 0 && <= 4294967295. Received -1`, + ], + [ + { retryDelay: "x" }, + "ERR_INVALID_ARG_TYPE", + `The "options.retryDelay" property must be of type number. Received type string ('x')`, + ], + [ + { retryDelay: 2 ** 31 }, + "ERR_OUT_OF_RANGE", + `The value of "options.retryDelay" is out of range. It must be >= 0 && <= 2147483647. Received 2147483648`, + ], + ["x", "ERR_INVALID_ARG_TYPE", `The "options" argument must be of type object. Received type string ('x')`], + [null, "ERR_INVALID_ARG_TYPE", `The "options" argument must be of type object. Received null`], + [[], "ERR_INVALID_ARG_TYPE", `The "options" argument must be of type object. Received an instance of Array`], + // node checks force first, then recursive, retryDelay, maxRetries + [ + { recursive: "x", force: "x" }, + "ERR_INVALID_ARG_TYPE", + `The "options.force" property must be of type boolean. Received type string ('x')`, + ], + [ + { maxRetries: "x", retryDelay: "x" }, + "ERR_INVALID_ARG_TYPE", + `The "options.retryDelay" property must be of type number. Received type string ('x')`, + ], + ]; + + test.each(cases)("options %p", async (options, code, message) => { + using dir = tempDir("rm-options-before-eisdir", { "sub/a.txt": "x" }); + const target = join(String(dir), "sub"); + const expected = { name: code === "ERR_OUT_OF_RANGE" ? "RangeError" : "TypeError", code, message }; + + expect(() => fs.rmSync(target, options)).toThrow(expect.objectContaining(expected)); + // the callback form reports invalid options by throwing, not through the callback + expect(() => fs.rm(target, options, () => expect.unreachable("callback must not run"))).toThrow( + expect.objectContaining(expected), + ); + await expect(fsPromises.rm(target, options)).rejects.toMatchObject(expected); + + expect(fs.existsSync(join(target, "a.txt"))).toBe(true); + }); + + test("valid options still reach the native rm", async () => { + using dir = tempDir("rm-options-valid", { "a.txt": "", "b.txt": "", "c.txt": "", "sub/a.txt": "x" }); + const root = String(dir); + fs.rmSync(join(root, "a.txt"), { force: true, maxRetries: 2, retryDelay: 0 }); + const { promise, resolve } = Promise.withResolvers(); + fs.rm(join(root, "b.txt"), { recursive: false }, resolve); + expect(await promise).toBeNull(); + await fsPromises.rm(join(root, "c.txt"), {}); + // a missing target is still an lstat ENOENT unless force is set + expect(() => fs.rmSync(join(root, "a.txt"))).toThrow(expect.objectContaining({ code: "ENOENT", syscall: "lstat" })); + fs.rmSync(join(root, "a.txt"), { force: true }); + // and a directory is still refused once the options are fine + expect(() => fs.rmSync(join(root, "sub"))).toThrow(expect.objectContaining({ code: "ERR_FS_EISDIR" })); + await fsPromises.rm(join(root, "sub"), { recursive: true }); + expect(fs.readdirSync(root)).toEqual([]); + }); +}); + it("close() while an operation is in flight actually closes the fd", async () => { await using dir = tempDir("deferred-close", { "x.txt": "hello" }); const fh = await fsPromises.open(join(dir, "x.txt"), "r");