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