fix(node/fs): better validation in fs.Dir (#16806)

src/js/internal/validators.ts

@@ -89,9 +89,22 @@ export default {
   validateObject: validateObject,
   validateLinkHeaderValue: validateLinkHeaderValue,
   checkIsHttpToken: checkIsHttpToken,
-  /** `(value, name, min = NumberMIN_SAFE_INTEGER, max = NumberMAX_SAFE_INTEGER)` */
+  /**
+   * @param value the value that should be an int
+   * @paran name the name of the parameter. Used when creating error codes
+   * @param min minimum value, inclusive. Defaults to {@link Number.MIN_SAFE_INTEGER}.
+   * @param max maximum value, inclusive. Defaults to {@link Number.MAX_SAFE_INTEGER}.
+   *
+   * @throws if `value` is not an int
+   * @throws if `value` is outside `[min, max]`
+   */
   validateInteger: $newCppFunction("NodeValidator.cpp", "jsFunction_validateInteger", 0),
-  /** `(value, name, min = undefined, max)` */
+  /**
+   * @param value the value that should be an int
+   * @paran name the name of the parameter. Used when creating error codes
+   * @param min minimum value, exclusive. Defaults to {@link Number.MIN_SAFE_INTEGER}.
+   * @param max maximum value, exclusive. Defaults to {@link Number.MAX_SAFE_INTEGER}.
+   */
   validateNumber: $newCppFunction("NodeValidator.cpp", "jsFunction_validateNumber", 0),
   /** `(value, name)` */
   validateString: $newCppFunction("NodeValidator.cpp", "jsFunction_validateString", 0),

src/js/node/fs.ts

@@ -1,8 +1,9 @@
 // Hardcoded module "node:fs"
-import type { Stats as StatsType } from "fs";
+import type { Stats as StatsType, Dirent as DirentType, PathLike } from "fs";
 const EventEmitter = require("node:events");
 const promises = require("node:fs/promises");
 const types = require("node:util/types");
+const { validateString, validateFunction, validateInteger } = require("internal/validators");
 
 const isDate = types.isDate;
 
@@ -599,12 +600,15 @@ var access = function access(path, mode, callback) {
     return new FSWatcher(path, options, listener);
   },
   opendir = function opendir(path, options, callback) {
+    // TODO: validatePath
+    // validateString(path, "path");
     if (typeof options === "function") {
       callback = options;
       options = undefined;
     }
+    validateFunction(callback, "callback");
     const result = new Dir(1, path, options);
-    if (callback) callback(null, result);
+    callback(null, result);
   };
 
 const { defineCustomPromisifyArgs } = require("internal/promisify");
@@ -700,7 +704,7 @@ function encodeRealpathResult(result, encoding) {
 }
 
 let assertEncodingForWindows: any = undefined;
-const realpathSync: any =
+const realpathSync =
   process.platform !== "win32"
     ? fs.realpathSync.bind(fs)
     : function realpathSync(p, options) {
@@ -1009,6 +1013,8 @@ function _toUnixTimestamp(time: any, name = "time") {
 }
 
 function opendirSync(path, options) {
+  // TODO: validatePath
+  // validateString(path, "path");
   return new Dir(1, path, options);
 }
 
@@ -1018,13 +1024,14 @@ class Dir {
    * {@link close} or {@link closeSync}.
    */
   #handle: number;
-  #path;
+  #path: PathLike;
   #options;
-  #entries: any[] | null = null;
+  #entries: DirentType[] | null = null;
 
-  constructor(handle, path, options) {
+  constructor(handle, path: PathLike, options) {
     if ($isUndefinedOrNull(handle)) throw $ERR_MISSING_ARGS("handle");
-    this.#handle = handle;
+    validateInteger(handle, "handle", 0);
+    this.#handle = $toLength(handle);
     this.#path = path;
     this.#options = options;
   }
@@ -1040,10 +1047,11 @@ class Dir {
     return entries.shift() ?? null;
   }
 
-  read(cb?): any {
+  read(cb?: (err: Error | null, entry: DirentType) => void): any {
     if (this.#handle < 0) throw $ERR_DIR_CLOSED();
 
-    if (cb) {
+    if (!$isUndefinedOrNull(cb)) {
+      validateFunction(cb, "callback");
       return this.read().then(entry => cb(null, entry));
     }
 
@@ -1063,7 +1071,9 @@ class Dir {
 
   close(cb?: () => void) {
     const handle = this.#handle;
-    if (cb) {
+    if (handle < 0) throw $ERR_DIR_CLOSED();
+    if (!$isUndefinedOrNull(cb)) {
+      validateFunction(cb, "callback");
       process.nextTick(cb);
     }
     if (handle > 2) fs.closeSync(handle);
@@ -1072,6 +1082,7 @@ class Dir {
 
   closeSync() {
     const handle = this.#handle;
+    if (handle < 0) throw $ERR_DIR_CLOSED();
     if (handle > 2) fs.closeSync(handle);
     this.#handle = -1;
   }

test/js/node/fs/dir.test.ts

@@ -1,34 +1,119 @@
-import { describe, beforeAll, afterAll, it, expect } from "bun:test";
+import { describe, beforeAll, afterAll, beforeEach, afterEach, it, expect } from "bun:test";
 import fs from "node:fs";
 import os from "node:os";
 import path from "node:path";
 
-describe("given a directory that exists", () => {
-  let dirname: string;
+function noop() {}
+describe("fs.opendir", () => {
+  // TODO: validatePath
+  // it.each([1, 0, null, undefined, function foo() {}, Symbol.for("foo")])(
+  //   "throws if the path is not a string: %p",
+  //   (path: any) => {
+  //     expect(() => fs.opendir(path, noop)).toThrow(/The "path" argument must be of type string/);
+  //   },
+  // );
 
-  beforeAll(() => {
-    const name = "dir-sync.test." + String(Math.random() * 100).substring(0, 6);
-    dirname = path.join(os.tmpdir(), name);
-    fs.mkdirSync(dirname);
-  });
-
-  afterAll(() => {
-    fs.rmdirSync(dirname, { recursive: true });
-  });
-
-  it("can be opened/closed synchronously", () => {
-    const dir = fs.opendirSync(dirname);
-    expect(dir).toBeDefined();
-    expect(dir).toBeInstanceOf(fs.Dir);
-    expect(dir.closeSync()).toBeUndefined();
-    expect(() => dir.readSync()).toThrow("Directory handle was closed");
-  });
-
-  it("can be opened/closed asynchronously", async () => {
-    const dir = await fs.promises.opendir(dirname);
-    expect(dir).toBeDefined();
-    expect(dir).toBeInstanceOf(fs.Dir);
-    expect(await dir.close()).toBeUndefined();
-    expect(() => dir.read()).toThrow("Directory handle was closed");
+  it("throws if callback is not provided", () => {
+    expect(() => fs.opendir("foo")).toThrow(/The "callback" argument must be of type function/);
   });
 });
+
+describe("fs.Dir", () => {
+  describe("given an empty temp directory", () => {
+    let dirname: string;
+
+    beforeAll(() => {
+      const name = "dir-sync.test." + String(Math.random() * 100).substring(0, 6);
+      dirname = path.join(os.tmpdir(), name);
+      fs.mkdirSync(dirname);
+    });
+
+    afterAll(() => {
+      fs.rmdirSync(dirname, { recursive: true });
+    });
+
+    describe("when an empty directory is opened", () => {
+      let dir: fs.Dir;
+
+      beforeEach(() => {
+        dir = fs.opendirSync(dirname);
+      });
+
+      afterEach(() => {
+        try {
+          dir.closeSync();
+        } catch {
+          /* suppress */
+        }
+      });
+
+      it("returns a Dir instance", () => {
+        expect(dir).toBeDefined();
+        expect(dir).toBeInstanceOf(fs.Dir);
+      });
+
+      describe("reading from the directory", () => {
+        it.each([0, 1, false, "foo", {}])("throws if passed a non-function callback (%p)", badCb => {
+          expect(() => dir.read(badCb)).toThrow(/The "callback" argument must be of type function/);
+        });
+
+        it("it can be read synchronously, even though no entries exist", () => {
+          for (let i = 0; i < 5; i++) {
+            const actual = dir.readSync();
+            expect(actual).toBeNull();
+          }
+        });
+
+        it("can be read asynchronously, even though no entries exist", async () => {
+          const actual = await dir.read();
+          expect(actual).toBeNull();
+        });
+
+        it("can be read asynchronously with callbacks, even though no entries exist", async () => {
+          const actual = await new Promise((resolve, reject) => {
+            dir.read((err, ent) => {
+              if (err) reject(err);
+              else resolve(ent);
+            });
+          });
+          expect(actual).toBeNull();
+        });
+      }); // </reading from the directory>
+
+      it("can be closed asynchronously", async () => {
+        const actual = await dir.close();
+        expect(actual).toBeUndefined();
+      });
+
+      it("can be closed asynchronously with callbacks", async () => {
+        const actual = await new Promise<void>((resolve, reject) => {
+          dir.close(err => {
+            if (err) reject(err);
+            else resolve();
+          });
+        });
+        expect(actual).toBeUndefined();
+      });
+
+      it("can be closed synchronously", () => {
+        expect(dir.closeSync()).toBeUndefined();
+      });
+
+      describe("when closed", () => {
+        beforeEach(async () => {
+          await dir.close();
+        });
+
+        it('attempts to close again will throw "Directory handle was closed"', () => {
+          expect(() => dir.closeSync()).toThrow("Directory handle was closed");
+          expect(() => dir.close()).toThrow("Directory handle was closed");
+        });
+
+        it("attempts to read will throw", () => {
+          expect(() => dir.readSync()).toThrow("Directory handle was closed");
+          expect(() => dir.read()).toThrow("Directory handle was closed");
+        });
+      }); // </when closed>
+    }); // </when an empty directory is opened>
+  }); // </given an empty temp directory>
+}); // </fs.Dir>