node:fs: validate rm options before the ERR_FS_EISDIR check

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;

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

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");