From 33fefdda6bc92574f37e5e12f36610ca9ff2a12d Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Tue, 28 Jan 2025 13:38:28 -0800 Subject: [PATCH] fix(node/fs): better validation in `fs.Dir` (#16806) --- src/js/internal/validators.ts | 17 ++++- src/js/node/fs.ts | 31 +++++--- test/js/node/fs/dir.test.ts | 139 +++++++++++++++++++++++++++------- 3 files changed, 148 insertions(+), 39 deletions(-) diff --git a/src/js/internal/validators.ts b/src/js/internal/validators.ts index 2df37ba6ea..87b6d004a5 100644 --- a/src/js/internal/validators.ts +++ b/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), diff --git a/src/js/node/fs.ts b/src/js/node/fs.ts index 319930fa7d..771d0dd4e1 100644 --- a/src/js/node/fs.ts +++ b/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; } diff --git a/test/js/node/fs/dir.test.ts b/test/js/node/fs/dir.test.ts index 7dcaf41b7d..3474679c83 100644 --- a/test/js/node/fs/dir.test.ts +++ b/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(); + }); + }); // + + 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((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"); + }); + }); // + }); // + }); // +}); //