From bdaba849cfc6d36d8ec8188e5083f30933d8bbc9 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 9 Nov 2026 02:06:01 +0000 Subject: [PATCH] node:fs: read the options before the path is checked readFile, writeFile, appendFile, readdir, readlink, realpath and mkdtemp read their options first, as Node's getOptions() runs before getValidatedPath(). With an invalid path and an invalid encoding, the encoding is reported. fs.watch asserts the encoding before it converts the filename. get_options is the one classifier of the options argument: a primitive string names the encoding, an object can carry one, and a function means the defaults. --- src/js/internal/fs/watch.ts | 20 +- src/js/node/fs.ts | 2 +- src/jsc/CallFrame.rs | 5 + src/runtime/node/node_fs.rs | 221 ++++++------ .../node/fs/fs-encoding-before-path.test.ts | 324 ++++++++++++++++++ test/js/node/fs/fs.test.ts | 26 +- 6 files changed, 470 insertions(+), 128 deletions(-) create mode 100644 test/js/node/fs/fs-encoding-before-path.test.ts diff --git a/src/js/internal/fs/watch.ts b/src/js/internal/fs/watch.ts index 50c48c7a2c40..30bad0777c22 100644 --- a/src/js/internal/fs/watch.ts +++ b/src/js/internal/fs/watch.ts @@ -5,6 +5,8 @@ const { basename } = require("node:path"); // The native `node:fs` binding, shared via `internal/fs/binding`. const fs = require("internal/fs/binding"); +const assertEncoding = $bindgen("node_util_binding.bind.ts", "assertEncodingValid") as (encoding: unknown) => void; + // Creates an ignore matcher function from the `ignore` watch option, // mirroring node lib/internal/fs/watchers.js createIgnoreMatcher. // string -> glob (patterns without a slash also match the basename), @@ -142,17 +144,23 @@ class FSWatcher extends EventEmitter { constructor(path, options, listener) { super(); - if (typeof path !== "string") { - path = require("internal/url").toPathIfFileURL(path); - } else if (path.startsWith("file:")) { - path = Bun.fileURLToPath(path); - } - + let encoding; if (typeof options === "function") { listener = options; options = {}; } else if (typeof options === "string") { + encoding = options; options = { encoding: options }; + } else if (options !== null && typeof options === "object") { + encoding = options.encoding; + } + // Node asserts the encoding in getOptions(), before it validates the filename: https://github.com/nodejs/node/blob/v26.12.1/lib/fs.js#L3155-L3166 + if (encoding) assertEncoding(encoding); + + if (typeof path !== "string") { + path = require("internal/url").toPathIfFileURL(path); + } else if (path.startsWith("file:")) { + path = Bun.fileURLToPath(path); } if (typeof listener !== "function") { diff --git a/src/js/node/fs.ts b/src/js/node/fs.ts index 3bcebaf18220..e24a31e9314a 100644 --- a/src/js/node/fs.ts +++ b/src/js/node/fs.ts @@ -735,7 +735,7 @@ const realpathSync: RealpathSync = let encoding: string | null | undefined; if (options) { if (typeof options === "string") encoding = options; - else encoding = options?.encoding; + else if (!$isCallable(options)) encoding = options?.encoding; if (encoding) { ($bindgen("node_util_binding.bind.ts", "assertEncodingValid") as (encoding: unknown) => void)(encoding); } diff --git a/src/jsc/CallFrame.rs b/src/jsc/CallFrame.rs index 026176af9550..9bb8444c8ade 100644 --- a/src/jsc/CallFrame.rs +++ b/src/jsc/CallFrame.rs @@ -92,6 +92,11 @@ impl<'a> ArgumentsSlice<'a> { self.remaining().first().copied() } + /// Peek the argument `index` places after the next one without eating anything. + pub fn peek_at(&self, index: usize) -> Option { + self.remaining().get(index).copied() + } + pub fn next_eat(&mut self) -> Option { let v = self.remaining().first().copied()?; self.eat(); diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index a70451774870..0eb7b07d6aca 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -3628,9 +3628,13 @@ pub(crate) mod args { ctx: &JSGlobalObject, arguments: &mut ArgumentsSlice, ) -> JsResult { + let options = get_options(ctx, arguments.peek_at(1), Encoding::Utf8)?; let path = PathLike::from_js_required(ctx, arguments, "path")?; - let encoding = parse_encoding_arg(ctx, arguments, Encoding::Utf8)?; - Ok(Readlink { path, encoding }) + arguments.eat(); + Ok(Readlink { + path, + encoding: options.encoding, + }) } } @@ -3643,50 +3647,50 @@ pub(crate) mod args { ctx: &JSGlobalObject, arguments: &mut ArgumentsSlice, ) -> JsResult { + let options = get_options(ctx, arguments.peek_at(1), Encoding::Utf8)?; let path = PathLike::from_js_required(ctx, arguments, "path")?; - let encoding = parse_encoding_arg(ctx, arguments, Encoding::Utf8)?; - Ok(Realpath { path, encoding }) + arguments.eat(); + Ok(Realpath { + path, + encoding: options.encoding, + }) } } - fn get_encoding( - object: JSValue, - global_object: &JSGlobalObject, - default: Encoding, - ) -> JsResult { - if let Some(value) = object.fast_get(global_object, bun_jsc::BuiltinName::Encoding)? { - return Encoding::assert(value, global_object, default); - } - Ok(default) + /// What `get_options` makes of the options argument. + struct Options { + encoding: Encoding, + /// The options object, for the members that each parser reads itself. + object: Option, } - /// Consume the next positional argument as a Node.js fs `encoding` option. - /// Accepts either an encoding string (`"utf8"`, `"buffer"`, ...) or an options - /// object with an `.encoding` property. Any other value (including `undefined` - /// / `null` / numbers / functions) is silently ignored and `default` is returned. - /// Shared by `Readlink`/`Realpath`/`MkdirTemp::from_js`. - fn parse_encoding_arg( + /// Node's `getOptions()`, without its throws for the type of `options` and for `options.signal`: https://github.com/nodejs/node/blob/v26.12.1/lib/internal/fs/utils.js#L407-L428 + fn get_options( ctx: &JSGlobalObject, - arguments: &mut ArgumentsSlice, + options: Option, default: Encoding, - ) -> JsResult { - let mut encoding = default; - if let Some(val) = arguments.next() { - arguments.eat(); - match val.js_type() { - bun_jsc::JSType::String - | bun_jsc::JSType::StringObject - | bun_jsc::JSType::DerivedStringObject => { - encoding = Encoding::assert(val, ctx, encoding)?; - } - _ => { - if val.is_object() { - encoding = get_encoding(val, ctx, encoding)?; - } - } + ) -> JsResult { + let mut parsed = Options { + encoding: default, + object: None, + }; + let Some(options) = options else { + return Ok(parsed); + }; + if options.is_string_literal() { + parsed.encoding = Encoding::assert(options, ctx, default)?; + } else if options.is_object() && !is_function(options) { + if let Some(encoding) = options.fast_get(ctx, bun_jsc::BuiltinName::Encoding)? { + parsed.encoding = Encoding::assert(encoding, ctx, default)?; } + parsed.object = Some(options); } - Ok(encoding) + Ok(parsed) + } + + /// A plain object is never callable: the type test keeps it out of the call into JSC. + fn is_function(object: JSValue) -> bool { + object.js_type() != bun_jsc::JSType::FinalObject && object.is_callable() } pub(crate) struct Unlink<'a> { @@ -3892,6 +3896,7 @@ pub(crate) mod args { ctx: &JSGlobalObject, arguments: &mut ArgumentsSlice, ) -> JsResult { + let options = get_options(ctx, arguments.peek_at(1), Encoding::Utf8)?; let prefix = PathLike::from_js(ctx, arguments)?.ok_or_else(|| { ctx.throw_invalid_argument_type_value( b"prefix", @@ -3899,8 +3904,11 @@ pub(crate) mod args { arguments.next().unwrap_or(JSValue::UNDEFINED), ) })?; - let encoding = parse_encoding_arg(ctx, arguments, Encoding::Utf8)?; - Ok(MkdirTemp { prefix, encoding }) + arguments.eat(); + Ok(MkdirTemp { + prefix, + encoding: options.encoding, + }) } } @@ -3929,34 +3937,22 @@ pub(crate) mod args { ctx: &JSGlobalObject, arguments: &mut ArgumentsSlice, ) -> JsResult { + let options = get_options(ctx, arguments.peek_at(1), Encoding::Utf8)?; let path = PathLike::from_js_required(ctx, arguments, "path")?; - let mut encoding = Encoding::Utf8; + arguments.eat(); let mut with_file_types = false; let mut recursive = false; - if let Some(val) = arguments.next() { - arguments.eat(); - match val.js_type() { - bun_jsc::JSType::String - | bun_jsc::JSType::StringObject - | bun_jsc::JSType::DerivedStringObject => { - encoding = Encoding::assert(val, ctx, encoding)?; - } - _ => { - if val.is_object() { - encoding = get_encoding(val, ctx, encoding)?; - if let Some(r) = val.get_boolean_strict(ctx, "recursive")? { - recursive = r; - } - if let Some(w) = val.get_boolean_strict(ctx, "withFileTypes")? { - with_file_types = w; - } - } - } + if let Some(val) = options.object { + if let Some(r) = val.get_boolean_strict(ctx, "recursive")? { + recursive = r; + } + if let Some(w) = val.get_boolean_strict(ctx, "withFileTypes")? { + with_file_types = w; } } Ok(Readdir { path, - encoding, + encoding: options.encoding, with_file_types, recursive, }) @@ -4521,6 +4517,7 @@ pub(crate) mod args { ctx: &JSGlobalObject, arguments: &mut ArgumentsSlice, ) -> JsResult { + let options = get_options(ctx, arguments.peek_at(1), Encoding::Buffer)?; // `Drop` on `path` covers every // `?`-propagated JsError below. let path = PathOrFileDescriptor::from_js(ctx, arguments)?.ok_or_else(|| { @@ -4528,40 +4525,34 @@ pub(crate) mod args { "path must be a string or a file descriptor" )) })?; - let mut encoding = Encoding::Buffer; + arguments.eat(); let mut flag = FileSystemFlags::R; let mut abort_signal = scopeguard::guard(None::, |s| { if let Some(signal) = s { signal.pending_activity_unref(); /* unref via Drop */ } }); - if let Some(arg) = arguments.next() { - arguments.eat(); - if arg.is_string() { - encoding = Encoding::assert(arg, ctx, encoding)?; - } else if arg.is_object() { - encoding = get_encoding(arg, ctx, encoding)?; - if let Some(flag_) = arg.get_truthy(ctx, "flag")? { - flag = FileSystemFlags::from_js(ctx, flag_)?.unwrap_or(flag); - } - if let Some(value) = arg.get_truthy(ctx, "signal")? { - if let Some(signal) = AbortSignal::ref_from_js(value) { - signal.pending_activity_ref(); - *abort_signal = Some(signal); - } else { - return Err(ctx.throw_invalid_argument_type_value( - b"signal", - b"AbortSignal", - value, - )); - } + if let Some(arg) = options.object { + if let Some(flag_) = arg.get_truthy(ctx, "flag")? { + flag = FileSystemFlags::from_js(ctx, flag_)?.unwrap_or(flag); + } + if let Some(value) = arg.get_truthy(ctx, "signal")? { + if let Some(signal) = AbortSignal::ref_from_js(value) { + signal.pending_activity_ref(); + *abort_signal = Some(signal); + } else { + return Err(ctx.throw_invalid_argument_type_value( + b"signal", + b"AbortSignal", + value, + )); } } } let abort_signal = scopeguard::ScopeGuard::into_inner(abort_signal); Ok(ReadFile { path, - encoding, + encoding: options.encoding, flag, limit_size_for_javascript: true, signal: abort_signal, @@ -4608,6 +4599,11 @@ pub(crate) mod args { arguments: &mut ArgumentsSlice, default_flag: FileSystemFlags, ) -> JsResult { + let default_encoding = match arguments.peek_at(1) { + Some(data) if data.is_string() => Encoding::Utf8, + _ => Encoding::Buffer, + }; + let options = get_options(ctx, arguments.peek_at(2), default_encoding)?; // `Drop` on `path` covers every // `?`-propagated JsError below. let path = PathOrFileDescriptor::from_js(ctx, arguments)?.ok_or_else(|| { @@ -4618,7 +4614,7 @@ pub(crate) mod args { let data_value = arguments .next_eat() .ok_or_else(|| ctx.throw_invalid_arguments(format_args!("data is required")))?; - let mut encoding = Encoding::Buffer; + arguments.eat(); let mut flag = default_flag; let mut mode: Mode = DEFAULT_PERMISSION; let mut abort_signal = scopeguard::guard(None::, |s| { @@ -4627,41 +4623,32 @@ pub(crate) mod args { } }); let mut flush = false; - if data_value.is_string() { - encoding = Encoding::Utf8; - } - if let Some(arg) = arguments.next() { - arguments.eat(); - if arg.is_string() { - encoding = Encoding::assert(arg, ctx, encoding)?; - } else if arg.is_object() { - encoding = get_encoding(arg, ctx, encoding)?; - if let Some(flag_) = arg.get_truthy(ctx, "flag")? { - flag = FileSystemFlags::from_js(ctx, flag_)?.unwrap_or(flag); - } - if let Some(mode_) = arg.get_truthy(ctx, "mode")? { - mode = node::mode_from_js(ctx, mode_)?.unwrap_or(mode); - } - if let Some(value) = arg.get_truthy(ctx, "signal")? { - if let Some(signal) = AbortSignal::ref_from_js(value) { - signal.pending_activity_ref(); - *abort_signal = Some(signal); - } else { - return Err(ctx.throw_invalid_argument_type_value( - b"signal", - b"AbortSignal", - value, - )); - } + if let Some(arg) = options.object { + if let Some(flag_) = arg.get_truthy(ctx, "flag")? { + flag = FileSystemFlags::from_js(ctx, flag_)?.unwrap_or(flag); + } + if let Some(mode_) = arg.get_truthy(ctx, "mode")? { + mode = node::mode_from_js(ctx, mode_)?.unwrap_or(mode); + } + if let Some(value) = arg.get_truthy(ctx, "signal")? { + if let Some(signal) = AbortSignal::ref_from_js(value) { + signal.pending_activity_ref(); + *abort_signal = Some(signal); + } else { + return Err(ctx.throw_invalid_argument_type_value( + b"signal", + b"AbortSignal", + value, + )); } - if let Some(flush_) = arg.get(ctx, "flush")? { - if flush_.is_boolean() || flush_.is_undefined_or_null() { - flush = flush_ == JSValue::TRUE; - } else { - return Err( - ctx.throw_invalid_argument_type_value(b"flush", b"boolean", flush_) - ); - } + } + if let Some(flush_) = arg.get(ctx, "flush")? { + if flush_.is_boolean() || flush_.is_undefined_or_null() { + flush = flush_ == JSValue::TRUE; + } else { + return Err( + ctx.throw_invalid_argument_type_value(b"flush", b"boolean", flush_) + ); } } } @@ -4672,7 +4659,7 @@ pub(crate) mod args { }; // String objects not allowed (typeof new String("hi") === "object") // https://github.com/nodejs/node/blob/6f946c95b9da75c70e868637de8161bc8d048379/lib/internal/fs/utils.js#L916 - let data = StringOrBuffer::from_js_with_encoding_maybe_async(ctx, data_value, encoding, flavor, StringObjects::Reject)? + let data = StringOrBuffer::from_js_with_encoding_maybe_async(ctx, data_value, options.encoding, flavor, StringObjects::Reject)? .ok_or_else(|| validators::throw_err_invalid_arg_type_with_message(ctx, format_args!("The \"data\" argument must be of type string or an instance of Buffer, TypedArray, or DataView")))?; let abort_signal = scopeguard::ScopeGuard::into_inner(abort_signal); Ok(WriteFile { diff --git a/test/js/node/fs/fs-encoding-before-path.test.ts b/test/js/node/fs/fs-encoding-before-path.test.ts new file mode 100644 index 000000000000..45f1ff1aad56 --- /dev/null +++ b/test/js/node/fs/fs-encoding-before-path.test.ts @@ -0,0 +1,324 @@ +/** + * readFile, writeFile, appendFile, readdir, readlink, realpath, mkdtemp and + * watch read their options before they check the path. With an invalid path + * and an invalid encoding, Bun reported the path. Node runs getOptions(), which + * asserts the encoding, before getValidatedPath(), so it reports the encoding: + * https://github.com/nodejs/node/blob/v26.12.1/lib/internal/fs/utils.js#L407-L428 (getOptions) + * https://github.com/nodejs/node/blob/v26.12.1/lib/fs.js#L2250-L2251 (readlinkSync) + * https://github.com/nodejs/node/blob/v26.12.1/lib/fs.js#L3155-L3166 (watch) + * + * This file uses node:test/node:assert so the identical file also runs under + * `node --test`, which is where the expected values come from. It cannot + * import from "harness" for the same reason. + */ +import assert from "node:assert"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { after, before, describe, test } from "node:test"; + +const anyFs = fs as any; +const noop = () => {}; + +const encodingError = { code: "ERR_INVALID_ARG_VALUE", name: "TypeError", namesTheEncoding: true }; +const describeError = (err: any) => ({ + code: err.code, + name: err.name, + // A NUL byte in the path is ERR_INVALID_ARG_VALUE too. Only the message tells them apart. + namesTheEncoding: String(err.message).includes("encoding"), +}); + +const throwing: Record unknown> = { + "readFileSync": (p, o) => fs.readFileSync(p, o), + "writeFileSync": (p, o) => fs.writeFileSync(p, "x", o), + "appendFileSync": (p, o) => fs.appendFileSync(p, "x", o), + "readdirSync": (p, o) => fs.readdirSync(p, o), + "readlinkSync": (p, o) => fs.readlinkSync(p, o), + "realpathSync": (p, o) => fs.realpathSync(p, o), + "realpathSync.native": (p, o) => fs.realpathSync.native(p, o), + "mkdtempSync": (p, o) => fs.mkdtempSync(p, o), + "mkdtempDisposableSync": (p, o) => anyFs.mkdtempDisposableSync(p, o), + "watch": (p, o) => fs.watch(p, o, noop).close(), + // The callback forms throw an argument error. They do not pass it to the callback. + "readFile": (p, o) => fs.readFile(p, o, noop), + "writeFile": (p, o) => fs.writeFile(p, "x", o, noop), + "appendFile": (p, o) => fs.appendFile(p, "x", o, noop), + "readdir": (p, o) => fs.readdir(p, o, noop), + "readlink": (p, o) => fs.readlink(p, o, noop), + "realpath": (p, o) => fs.realpath(p, o, noop), + "realpath.native": (p, o) => fs.realpath.native(p, o, noop), + "mkdtemp": (p, o) => fs.mkdtemp(p, o, noop), +}; +const rejecting: Record Promise> = { + "promises.readFile": (p, o) => fs.promises.readFile(p, o), + "promises.writeFile": (p, o) => fs.promises.writeFile(p, "x", o), + "promises.appendFile": (p, o) => fs.promises.appendFile(p, "x", o), + "promises.readdir": (p, o) => fs.promises.readdir(p, o), + "promises.readlink": (p, o) => fs.promises.readlink(p, o), + "promises.realpath": (p, o) => fs.promises.realpath(p, o), + "promises.mkdtemp": (p, o) => fs.promises.mkdtemp(p, o), + "promises.mkdtempDisposable": (p, o) => anyFs.promises.mkdtempDisposable(p, o), +}; +const everyCall = [...Object.keys(throwing), ...Object.keys(rejecting)]; + +// What a call throws, or what its promise rejects with. +async function outcomeOf(name: string, path: any, options: any, summarize: (err: any) => unknown) { + if (name in rejecting) return rejecting[name](path, options).then(() => "did not reject", summarize); + try { + throwing[name](path, options); + return "did not throw"; + } catch (err) { + return summarize(err); + } +} + +describe("an invalid encoding is reported before an invalid path", () => { + let dir: string; + let file: string; + let entries: string; + let closedFileHandle: fs.promises.FileHandle; + + before(async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "fs-encoding-before-path-")); + file = path.join(dir, "file.txt"); + fs.writeFileSync(file, "6869"); + entries = path.join(dir, "entries"); + fs.mkdirSync(entries); + fs.writeFileSync(path.join(entries, "entry.txt"), ""); + closedFileHandle = await fs.promises.open(file, "r"); + await closedFileHandle.close(); + }); + + after(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + const invalidPaths = (): [label: string, path: any][] => [ + ["bigint", 123n], + ["object", {}], + ["null", null], + ["undefined", undefined], + ["boolean", true], + ["symbol", Symbol("path")], + ["string with a NUL byte", "a\0b"], + ["String object with a NUL byte", new String("a\0b")], + ["http: URL", new URL("http://example.com/x")], + ["file: URL with a host", new URL("file://host/x")], + ["file: URL with an encoded NUL byte", new URL("file:///tmp/a%00b")], + ["file: URL with an encoded slash", new URL("file:///tmp/a%2Fb")], + ["file: URL whose path is not UTF-8", new URL("file:///tmp/%FF")], + ["empty Buffer", Buffer.alloc(0)], + ["Buffer with a NUL byte", Buffer.from("a\0b")], + ["string longer than any path", Buffer.alloc(200000, "a").toString()], + ["Buffer longer than any path", Buffer.alloc(200000, "a")], + ["negative number", -1], + ["fraction", 1.5], + ["number above int32", 2 ** 31], + ["closed FileHandle", closedFileHandle], + ]; + const invalidOptions: [label: string, options: any][] = [ + ["'bogus'", "bogus"], + ["{ encoding: 'bogus' }", { encoding: "bogus" }], + ]; + + for (const name of everyCall) { + test(name, async () => { + const actual: Record = {}; + const expected: Record = {}; + for (const [pathLabel, invalidPath] of invalidPaths()) { + for (const [optionsLabel, options] of invalidOptions) { + const label = `${pathLabel}, ${optionsLabel}`; + expected[label] = encodingError; + actual[label] = await outcomeOf(name, invalidPath, options, describeError); + } + } + assert.deepStrictEqual(actual, expected); + }); + } + + // fs.watch validates more options than the other calls. Node reports the + // encoding before each of them too, with a valid path or an invalid one. + test("watch, with a second invalid option", () => { + const otherOptions = { persistent: 1, recursive: 1, verbose: 1, signal: 1, ignore: 5 }; + const actual: Record = {}; + const expected: Record = {}; + for (const [pathLabel, watched] of [ + ["valid path", dir], + ["bigint", 123n], + ] as [string, any][]) { + for (const [name, value] of Object.entries(otherOptions)) { + const label = `${pathLabel}, { encoding: 'bogus', ${name}: ${value} }`; + expected[label] = encodingError; + try { + fs.watch(watched, { encoding: "bogus", [name]: value } as any, noop).close(); + actual[label] = "did not throw"; + } catch (err) { + actual[label] = describeError(err); + } + } + } + assert.deepStrictEqual(actual, expected); + }); + + // fs.watch converts a "file:" string to a path. This one has a host, which POSIX refuses. + test("watch, with a 'file:' string", async () => { + const actual: unknown[] = []; + for (const [, options] of invalidOptions) + actual.push(await outcomeOf("watch", "file://host/x", options, describeError)); + assert.deepStrictEqual(actual, [encodingError, encodingError]); + }); + + // A listener in the options slot means the defaults: nothing is read from it or for it. + test("watch, with a listener in the options slot", () => { + const inherited = Object.getOwnPropertyDescriptor(Object.prototype, "encoding"); + Object.defineProperty(Object.prototype, "encoding", { value: "bogus", configurable: true, writable: true }); + let validPath: unknown; + let invalidPath: unknown; + try { + try { + fs.watch(dir, noop).close(); + validPath = "did not throw"; + } catch (err) { + validPath = describeError(err); + } + try { + fs.watch(123n as any, noop).close(); + invalidPath = "did not throw"; + } catch (err) { + invalidPath = describeError(err); + } + } finally { + if (inherited) Object.defineProperty(Object.prototype, "encoding", inherited); + else delete (Object.prototype as any).encoding; + } + assert.deepStrictEqual( + { validPath, invalidPath }, + { + validPath: "did not throw", + invalidPath: { code: "ERR_INVALID_ARG_TYPE", name: "TypeError", namesTheEncoding: false }, + }, + ); + }); + + test("an error from the encoding getter is reported before an invalid path", async () => { + const actual: Record = {}; + const expected: Record = {}; + for (const name of everyCall) { + const options = { + get encoding() { + throw new RangeError("from the encoding getter"); + }, + }; + expected[name] = "from the encoding getter"; + actual[name] = await outcomeOf(name, 123n, options, err => err.message); + } + assert.deepStrictEqual(actual, expected); + }); + + // The type error for a prefix describes the value, which reads its `constructor`. + test("a refused prefix is not inspected when the encoding is invalid", async () => { + const actual: Record = {}; + const expected: Record = {}; + for (const name of everyCall.filter(name => name.includes("mkdtemp"))) { + let inspected = 0; + const prefix = { + get constructor() { + inspected++; + return Object; + }, + }; + expected[name] = { error: encodingError, inspected: 0 }; + actual[name] = { error: await outcomeOf(name, prefix, "bogus", describeError), inspected }; + } + assert.deepStrictEqual(actual, expected); + }); + + // getOptions() takes an encoding name from a primitive string only. A String + // object is an options object, and a function means the defaults. + describe("a value that is not an encoding name", () => { + const notEncodingNames: [label: string, make: (name: string) => any][] = [ + ["String object", name => new String(name)], + ["function", name => Object.assign(() => {}, { encoding: name })], + ]; + + test("leaves the path error in place", async () => { + const pathError = { code: "ERR_INVALID_ARG_VALUE", name: "TypeError", namesTheEncoding: false }; + const actual: Record = {}; + const expected: Record = {}; + for (const [optionsLabel, make] of notEncodingNames) { + for (const name of everyCall) { + const label = `${name}, ${optionsLabel}`; + expected[label] = pathError; + actual[label] = await outcomeOf(name, "a\0b", make("bogus"), describeError); + } + } + assert.deepStrictEqual(actual, expected); + }); + + test("does not select an encoding", async () => { + const kind = (value: unknown) => (Buffer.isBuffer(value) ? "Buffer" : typeof value); + const written = (options: any) => { + const target = path.join(dir, "written.txt"); + fs.writeFileSync(target, "6869", options); + return fs.readFileSync(target, "latin1"); + }; + const actual: Record = {}; + const expected: Record = {}; + for (const [label, make] of notEncodingNames) { + expected[label] = { + readFileSync: "Buffer", + "promises.readFile": "Buffer", + readdirSync: ["string"], + "promises.readdir": ["string"], + "realpathSync.native": "string", + writeFileSync: "6869", + }; + actual[label] = { + readFileSync: kind(fs.readFileSync(file, make("utf8"))), + "promises.readFile": kind(await fs.promises.readFile(file, make("utf8"))), + readdirSync: fs.readdirSync(entries, make("buffer")).map(kind), + "promises.readdir": (await fs.promises.readdir(entries, make("buffer"))).map(kind), + "realpathSync.native": kind(fs.realpathSync.native(file, make("buffer"))), + writeFileSync: written(make("hex")), + }; + } + assert.deepStrictEqual(actual, expected); + }); + + test("a callback is not read as an options object", async () => { + const target = path.join(dir, "callback.txt"); + fs.writeFileSync(target, "hello"); + const reads: string[] = []; + const watched = (callback: T): T => { + for (const key of ["encoding", "flag", "mode", "signal", "flush"]) { + Object.defineProperty(callback, key, { get: () => void reads.push(key) }); + } + return callback; + }; + + const read = Promise.withResolvers(); + fs.readFile( + target, + watched((err: Error | null, data: unknown) => (err ? read.reject(err) : read.resolve(data))), + ); + assert.strictEqual(Buffer.isBuffer(await read.promise), true); + + const write = Promise.withResolvers(); + fs.writeFile( + target, + "written", + watched((err: Error | null) => (err ? write.reject(err) : write.resolve())), + ); + await write.promise; + assert.strictEqual(fs.readFileSync(target, "utf8"), "written"); + + assert.deepStrictEqual(reads, []); + }); + }); + + // opendir is the exception: Node validates the path first there. + // https://github.com/nodejs/node/blob/v26.12.1/lib/internal/fs/dir.js#L378-L379 + test.todo("opendirSync reports an invalid path before an invalid encoding", () => { + assert.throws(() => fs.opendirSync(123n as any, "bogus" as any), { code: "ERR_INVALID_ARG_TYPE" }); + }); +}); diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index 4107357a905d..b1faa5d3b5b8 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -7464,7 +7464,8 @@ it("async fs.write copies only the bytes it writes out of a WebAssembly.Memory o }); // A sync call reads the path after the option getters ran. It reads the bytes captured at call time -// when a getter shrinks the buffer. +// when a getter shrinks the buffer. The `encoding` getter is the exception: it runs before the path +// is captured. Node runs every getter before it reads the path, and throws on each of these calls. it("sync fs calls read a Buffer path captured at call time when an option getter shrinks its resizable ArrayBuffer", async () => { using dir = tempDir("fs-resizable-path", {}); // The unfixed build segfaults on the main thread, so this runs in a child process. @@ -7497,13 +7498,28 @@ it("sync fs calls read a Buffer path captured at call time when an option getter { const ab = resizablePath(written); const text = fs.readFileSync(new DataView(ab), { - get encoding() { + encoding: "utf8", + get flag() { ab.resize(0); - return "utf8"; + return "r"; }, }); console.log("readFileSync:", text); } + { + const ab = resizablePath(written); + try { + fs.readFileSync(new DataView(ab), { + get encoding() { + ab.resize(0); + return "utf8"; + }, + }); + console.log("readFileSync, encoding getter: returned"); + } catch (e) { + console.log("readFileSync, encoding getter:", e.code); + } + } { const made = path.join(dir, "made"); const ab = resizablePath(made); @@ -7523,7 +7539,9 @@ it("sync fs calls read a Buffer path captured at call time when an option getter stderr: "pipe", }); const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - expect(stdout).toBe("writeFileSync: sync-write\nreadFileSync: sync-write\nmkdirSync: true\n"); + expect(stdout).toBe( + "writeFileSync: sync-write\nreadFileSync: sync-write\nreadFileSync, encoding getter: ERR_INVALID_ARG_TYPE\nmkdirSync: true\n", + ); expect(stderr).toBe(""); expect(exitCode).toBe(0); });