From 7cc29faf0f8b1e85b9557fa00fc5b362696c96d9 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 16 Aug 2026 02:19:04 +0000 Subject: [PATCH 1/3] node:fs: validate options.signal in readFile, writeFile and watch like node The native option parsers for readFile/readFileSync, writeFile/appendFile (and their sync and promises forms) and fs.watch reported a bad signal as 'The "signal" argument must be of type AbortSignal' (fs.watch: 'signal is not of type AbortSignal') and silently ignored null and "". Node's getOptions() runs validateAbortSignal(options.signal, "options.signal") on every defined value, so the message is 'The "options.signal" property must be an instance of AbortSignal. Received ...' and only undefined means no signal. The three sites now read the option with get() instead of get_truthy() and throw through a shared validators::throw_err_invalid_abort_signal. The Rust ERR_INVALID_ARG_TYPE renderers in JSGlobalObject render a dotted name as a property, the rule Node and the C++ Message::addParameter already apply. --- src/jsc/JSGlobalObject.rs | 35 +++++++++-- src/runtime/node/node_fs.rs | 16 ++--- src/runtime/node/node_fs_watcher.rs | 11 ++-- src/runtime/node/util/validators.rs | 13 ++++ test/js/node/fs/promises.test.js | 93 +++++++++++++++++++++++++++++ test/js/node/watch/fs.watch.test.ts | 26 ++++++++ 6 files changed, 176 insertions(+), 18 deletions(-) diff --git a/src/jsc/JSGlobalObject.rs b/src/jsc/JSGlobalObject.rs index 804bdabdaa80..6033ffe24728 100644 --- a/src/jsc/JSGlobalObject.rs +++ b/src/jsc/JSGlobalObject.rs @@ -541,6 +541,7 @@ impl JSGlobalObject { } /// "The {argname} argument must be of type {typename}. Received {value}" + /// (a dotted `argname` is a "property", see `InvalidArgTypeName`). /// /// Accepts `&str`, `&[u8]`, or `b"..."` for `argname`/`typename`. pub fn throw_invalid_argument_type_value( @@ -556,8 +557,8 @@ impl JSGlobalObject { self.err( JscError::INVALID_ARG_TYPE, format_args!( - "The \"{}\" argument must be of type {}. Received {}", - bstr::BStr::new(argname.as_ref()), + "The {} must be of type {}. Received {}", + InvalidArgTypeName(argname.as_ref()), bstr::BStr::new(typename.as_ref()), actual_string_value ), @@ -565,6 +566,8 @@ impl JSGlobalObject { .throw() } + /// Like [`Self::throw_invalid_argument_type_value`], but `typename` is the whole + /// phrase after "must be" (e.g. `"an instance of Array"`). pub fn throw_invalid_argument_type_value2( &self, argname: impl AsRef<[u8]>, @@ -578,8 +581,8 @@ impl JSGlobalObject { self.err( JscError::INVALID_ARG_TYPE, format_args!( - "The \"{}\" argument must be {}. Received {}", - bstr::BStr::new(argname.as_ref()), + "The {} must be {}. Received {}", + InvalidArgTypeName(argname.as_ref()), bstr::BStr::new(typename.as_ref()), actual_string_value ), @@ -627,8 +630,8 @@ impl JSGlobalObject { self.err( JscError::INVALID_ARG_TYPE, format_args!( - "The \"{}\" argument must be one of type {}. Received {}", - bstr::BStr::new(argname.as_ref()), + "The {} must be one of type {}. Received {}", + InvalidArgTypeName(argname.as_ref()), bstr::BStr::new(typename.as_ref()), actual_string_value ), @@ -1380,6 +1383,26 @@ pub struct SysErrOptions { pub name: Option<&'static [u8]>, } +/// The subject of Node's `ERR_INVALID_ARG_TYPE` message: `"name" argument`, +/// `"options.name" property` for a dotted name, or the name verbatim when it +/// already ends in ` argument` ("first argument"). Same rule as +/// `Bun::Message::addParameter` in ErrorCode.cpp. +/// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/errors.js#L1407-L1414 +struct InvalidArgTypeName<'a>(&'a [u8]); + +impl core::fmt::Display for InvalidArgTypeName<'_> { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + let name = bstr::BStr::new(self.0); + if self.0.ends_with(b" argument") { + write!(f, "{name}") + } else if strings::contains_char(self.0, b'.') { + write!(f, "\"{name}\" property") + } else { + write!(f, "\"{name}\" argument") + } + } +} + // Unified with the crate-root definitions (lib.rs) — re-exported here so // `bun_jsc::js_global_object::{IntegerRange, ValidateObjectOpts}` keep // resolving for any caller that named them via this path. The previous local diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index dd6773bdc442..b49e904c39b9 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -4074,14 +4074,14 @@ pub mod args { 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(value) = arg.get(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", + return Err(validators::throw_err_invalid_abort_signal( + ctx, + "options.signal", value, )); } @@ -4183,14 +4183,14 @@ pub mod args { 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(value) = arg.get(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", + return Err(validators::throw_err_invalid_abort_signal( + ctx, + "options.signal", value, )); } diff --git a/src/runtime/node/node_fs_watcher.rs b/src/runtime/node/node_fs_watcher.rs index b581a1c3ddf7..a745f6312554 100644 --- a/src/runtime/node/node_fs_watcher.rs +++ b/src/runtime/node/node_fs_watcher.rs @@ -24,6 +24,7 @@ use bun_sys::{self, SystemErrno}; use bun_threading::Mutex; use crate::node::types::{Encoding, PathLikeExt}; +use crate::node::util::validators; use crate::webcore::encoding as Encoder; bun_output::declare_scope!(fs_watch, hidden); @@ -701,7 +702,7 @@ impl<'a> Arguments<'a> { } // abort signal - if let Some(signal_) = options_or_callable.get_truthy(ctx, "signal")? { + if let Some(signal_) = options_or_callable.get(ctx, "signal")? { if let Some(signal_obj) = AbortSignal::from_js(signal_) { // Keep it alive signal_.ensure_still_alive(); @@ -712,9 +713,11 @@ impl<'a> Arguments<'a> { // centralised deref proof. signal = Some(AbortSignal::opaque_ref(signal_obj)); } else { - return Err(ctx.throw_invalid_arguments(format_args!( - "signal is not of type AbortSignal" - ))); + return Err(validators::throw_err_invalid_abort_signal( + ctx, + "options.signal", + signal_, + )); } } diff --git a/src/runtime/node/util/validators.rs b/src/runtime/node/util/validators.rs index a256a750aa1f..2d4a26bb8021 100644 --- a/src/runtime/node/util/validators.rs +++ b/src/runtime/node/util/validators.rs @@ -53,6 +53,19 @@ pub(crate) fn throw_err_invalid_arg_type( ) } +/// The error `validateAbortSignal(value, name)` throws. Callers pass every +/// defined value of the option (`JSValue::get`, not `get_truthy`): like Node, +/// only `undefined` means "no signal", so `null` or `""` end up here too. +/// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/validators.js#L444-L451 +#[cold] +pub(crate) fn throw_err_invalid_abort_signal( + global_this: &JSGlobalObject, + name: &str, + value: JSValue, +) -> JsError { + global_this.throw_invalid_argument_type_value2(name, "an instance of AbortSignal", value) +} + #[cold] fn throw_range_error(global_this: &JSGlobalObject, args: fmt::Arguments<'_>) -> JsError { global_this.err(jsc::ErrorCode::OUT_OF_RANGE, args).throw() diff --git a/test/js/node/fs/promises.test.js b/test/js/node/fs/promises.test.js index 06811b04cba8..0e84079a6cbb 100644 --- a/test/js/node/fs/promises.test.js +++ b/test/js/node/fs/promises.test.js @@ -418,6 +418,99 @@ it("teardown waits for every concurrent in-flight write", async () => { expect(fh.fd).toBe(-1); }); +// node's getOptions() runs validateAbortSignal(options.signal, "options.signal") +// for readFile, writeFile and appendFile: anything other than undefined that is +// not a signal is an ERR_INVALID_ARG_TYPE, including null and "". +// Every expected string below is node v26.3.0's output for the same call. +describe("options.signal validation matches node", () => { + const invalid = received => + `TypeError ERR_INVALID_ARG_TYPE: The "options.signal" property must be an instance of AbortSignal. Received ${received}`; + + function outcome(fn) { + try { + const result = fn(); + if (typeof result?.then === "function") { + return result.then( + () => "resolved", + e => `${e.constructor.name} ${e.code}: ${e.message}`, + ); + } + return "resolved"; + } catch (e) { + return `${e.constructor.name} ${e.code}: ${e.message}`; + } + } + + test("readFileSync reports every non-signal value like node", async () => { + await using dir = tempDir("fs-signal-values", { "f.txt": "hello" }); + const file = join(dir, "f.txt"); + const values = [1, 0, null, "", "abc", true, false, {}, [], new AbortController()]; + expect(values.map(signal => outcome(() => fs.readFileSync(file, { signal })))).toEqual([ + invalid("type number (1)"), + invalid("type number (0)"), + invalid("null"), + invalid("type string ('')"), + invalid("type string ('abc')"), + invalid("type boolean (true)"), + invalid("type boolean (false)"), + invalid("an instance of Object"), + invalid("an instance of Array"), + invalid("an instance of AbortController"), + ]); + // Only undefined means "no signal". + expect(fs.readFileSync(file, { signal: undefined, encoding: "utf8" })).toBe("hello"); + expect(fs.readFileSync(file, { signal: new AbortController().signal, encoding: "utf8" })).toBe("hello"); + }); + + test("every readFile, writeFile and appendFile entry point rejects an invalid signal the same way", async () => { + await using dir = tempDir("fs-signal-entry-points", { "in.txt": "hello" }); + const input = join(dir, "in.txt"); + const output = join(dir, "out.txt"); + const bad = { signal: null }; + const noop = () => {}; + const fh = await fsPromises.open(input, "r+"); + let results; + try { + results = { + readFileSync: outcome(() => fs.readFileSync(input, bad)), + readFile: outcome(() => fs.readFile(input, bad, noop)), + "promises.readFile": await outcome(() => fsPromises.readFile(input, bad)), + "promises.readFile(handle)": await outcome(() => fsPromises.readFile(fh, bad)), + "handle.readFile": await outcome(() => fh.readFile(bad)), + writeFileSync: outcome(() => fs.writeFileSync(output, "x", bad)), + writeFile: outcome(() => fs.writeFile(output, "x", bad, noop)), + "promises.writeFile": await outcome(() => fsPromises.writeFile(output, "x", bad)), + "promises.writeFile(handle)": await outcome(() => fsPromises.writeFile(fh, "x", bad)), + appendFileSync: outcome(() => fs.appendFileSync(output, "x", bad)), + appendFile: outcome(() => fs.appendFile(output, "x", bad, noop)), + "promises.appendFile": await outcome(() => fsPromises.appendFile(output, "x", bad)), + "promises.appendFile(handle)": await outcome(() => fsPromises.appendFile(fh, "x", bad)), + }; + } finally { + await fh.close(); + } + const rejectedNull = invalid("null"); + expect(results).toEqual({ + readFileSync: rejectedNull, + readFile: rejectedNull, + "promises.readFile": rejectedNull, + "promises.readFile(handle)": rejectedNull, + "handle.readFile": rejectedNull, + writeFileSync: rejectedNull, + writeFile: rejectedNull, + "promises.writeFile": rejectedNull, + "promises.writeFile(handle)": rejectedNull, + appendFileSync: rejectedNull, + appendFile: rejectedNull, + "promises.appendFile": rejectedNull, + "promises.appendFile(handle)": rejectedNull, + }); + // The option is validated before any I/O happens. + expect(fs.existsSync(output)).toBe(false); + expect(fs.readFileSync(input, "utf8")).toBe("hello"); + }); +}); + // node rejects abortable fs APIs with an AbortError (an Error whose code is the // string "ABORT_ERR" and whose cause is signal.reason), never with the raw // DOMException held in signal.reason (whose .code is the number 20). diff --git a/test/js/node/watch/fs.watch.test.ts b/test/js/node/watch/fs.watch.test.ts index 0b55ba1f9a80..ef43d8b73b47 100644 --- a/test/js/node/watch/fs.watch.test.ts +++ b/test/js/node/watch/fs.watch.test.ts @@ -462,6 +462,32 @@ describe("fs.watch", () => { }); }); + // node validates options.signal in getOptions() before creating the watcher; + // the strings below are node v26.3.0's output for the same calls (null + // included: only undefined means "no signal"). + test("a signal option that is not an AbortSignal throws node's ERR_INVALID_ARG_TYPE", () => { + const filepath = path.join(testDir, "abort.txt"); + const invalid = (received: string) => + `TypeError ERR_INVALID_ARG_TYPE: The "options.signal" property must be an instance of AbortSignal. Received ${received}`; + const outcome = (signal: unknown) => { + try { + fs.watch(filepath, { signal } as any).close(); + return "watcher created"; + } catch (e: any) { + return `${e.constructor.name} ${e.code}: ${e.message}`; + } + }; + expect([1, null, "", {}, new AbortController()].map(outcome)).toEqual([ + invalid("type number (1)"), + invalid("null"), + invalid("type string ('')"), + invalid("an instance of Object"), + invalid("an instance of AbortController"), + ]); + expect(outcome(undefined)).toBe("watcher created"); + expect(outcome(new AbortController().signal)).toBe("watcher created"); + }); + test("an abort with no 'error' listener only closes; an 'error' listener that throws is an uncaught exception", async () => { await using proc = Bun.spawn({ cmd: [ From c9a16de0f3a088f50a49e35d19907b4407e1b970 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 16 Aug 2026 07:21:39 +0000 Subject: [PATCH 2/3] node:fs: parse the signal option through one validate_abort_signal helper validators::validate_abort_signal returns the native AbortSignalRef or throws node's validateAbortSignal error, so the three fs sites share the accept condition and the message. fs.watch's Arguments now owns a ref to the signal instead of borrowing it from the JS value until FSWatcher::init takes its own. --- src/runtime/node/node_fs.rs | 28 ++++++--------------- src/runtime/node/node_fs_watcher.rs | 38 ++++++++--------------------- src/runtime/node/util/validators.rs | 33 ++++++++++++++----------- 3 files changed, 37 insertions(+), 62 deletions(-) diff --git a/src/runtime/node/node_fs.rs b/src/runtime/node/node_fs.rs index b49e904c39b9..9036427434b1 100644 --- a/src/runtime/node/node_fs.rs +++ b/src/runtime/node/node_fs.rs @@ -4075,16 +4075,10 @@ pub mod args { flag = FileSystemFlags::from_js(ctx, flag_)?.unwrap_or(flag); } if let Some(value) = arg.get(ctx, "signal")? { - if let Some(signal) = AbortSignal::ref_from_js(value) { - signal.pending_activity_ref(); - *abort_signal = Some(signal); - } else { - return Err(validators::throw_err_invalid_abort_signal( - ctx, - "options.signal", - value, - )); - } + let signal = + validators::validate_abort_signal(ctx, value, "options.signal")?; + signal.pending_activity_ref(); + *abort_signal = Some(signal); } } } @@ -4184,16 +4178,10 @@ pub mod args { mode = node::mode_from_js(ctx, mode_)?.unwrap_or(mode); } if let Some(value) = arg.get(ctx, "signal")? { - if let Some(signal) = AbortSignal::ref_from_js(value) { - signal.pending_activity_ref(); - *abort_signal = Some(signal); - } else { - return Err(validators::throw_err_invalid_abort_signal( - ctx, - "options.signal", - value, - )); - } + let signal = + validators::validate_abort_signal(ctx, value, "options.signal")?; + signal.pending_activity_ref(); + *abort_signal = Some(signal); } if let Some(flush_) = arg.get(ctx, "flush")? { if flush_.is_boolean() || flush_.is_undefined_or_null() { diff --git a/src/runtime/node/node_fs_watcher.rs b/src/runtime/node/node_fs_watcher.rs index a745f6312554..afceff2ead7d 100644 --- a/src/runtime/node/node_fs_watcher.rs +++ b/src/runtime/node/node_fs_watcher.rs @@ -15,7 +15,7 @@ use bun_jsc::JsCell; use bun_jsc::abort_signal::AbortListener; use bun_jsc::node::PathLike; use bun_jsc::{ - self as jsc, AbortSignal, AbortSignalRef, ArgumentsSlice, CallFrame, CommonAbortReason, + self as jsc, AbortSignalRef, ArgumentsSlice, CallFrame, CommonAbortReason, CommonAbortReasonExt as _, GlobalRef, JSGlobalObject, JSValue, JsRef, JsResult, SysErrorJsc, VirtualMachineRef as VirtualMachine, ZigStringJsc as _, }; @@ -641,7 +641,7 @@ pub struct Arguments<'a> { pub path: PathLike, pub(crate) listener: JSValue, pub global_this: &'a JSGlobalObject, - pub(crate) signal: Option<&'a AbortSignal>, + pub(crate) signal: Option, pub(crate) persistent: bool, pub(crate) recursive: bool, pub(crate) encoding: Encoding, @@ -661,7 +661,7 @@ impl<'a> Arguments<'a> { // drops `path` automatically. let mut listener: JSValue = JSValue::ZERO; - let mut signal: Option<&AbortSignal> = None; + let mut signal: Option = None; let mut persistent: bool = true; let mut recursive: bool = false; let mut encoding: Encoding = Encoding::Utf8; @@ -701,24 +701,12 @@ impl<'a> Arguments<'a> { recursive = recursive_.to_boolean(); } - // abort signal if let Some(signal_) = options_or_callable.get(ctx, "signal")? { - if let Some(signal_obj) = AbortSignal::from_js(signal_) { - // Keep it alive - signal_.ensure_still_alive(); - // `signal_obj` is the live C++ AbortSignal owned by - // `signal_` (kept reachable for the duration of the call - // by `ensure_still_alive`). `AbortSignal` is an - // `opaque_ffi!` ZST handle; `opaque_ref` is the - // centralised deref proof. - signal = Some(AbortSignal::opaque_ref(signal_obj)); - } else { - return Err(validators::throw_err_invalid_abort_signal( - ctx, - "options.signal", - signal_, - )); - } + signal = Some(validators::validate_abort_signal( + ctx, + signal_, + "options.signal", + )?); } // listener @@ -1183,13 +1171,7 @@ impl FSWatcher { ..Default::default() }), mutex: Mutex::default(), - // SAFETY: `args.signal` is a live borrow of the JS AbortSignal (kept - // reachable by the caller's frame); `ref_()` bumps the C++ intrusive - // refcount and `adopt` takes ownership of that +1. - signal: JsCell::new( - args.signal - .map(|s| unsafe { AbortSignalRef::adopt(s.ref_()) }), - ), + signal: JsCell::new(args.signal.clone()), persistent: Cell::new(args.persistent), path_watcher: Cell::new(None), global_this: GlobalRef::from(args.global_this), @@ -1209,7 +1191,7 @@ impl FSWatcher { ctx_ref .path_watcher - .set(if args.signal.is_none_or(|s| !s.aborted()) { + .set(if args.signal.as_deref().is_none_or(|s| !s.aborted()) { // The two backends take different arities (the Windows // backend dropped the callback parameters — only one valid // value each), so the call is cfg-split. diff --git a/src/runtime/node/util/validators.rs b/src/runtime/node/util/validators.rs index 2d4a26bb8021..b20fb7427d09 100644 --- a/src/runtime/node/util/validators.rs +++ b/src/runtime/node/util/validators.rs @@ -1,7 +1,9 @@ use core::fmt; use bun_core::ZigString; -use bun_jsc::{self as jsc, JSGlobalObject, JSValue, JsError, JsResult}; +use bun_jsc::{ + self as jsc, AbortSignal, AbortSignalRef, JSGlobalObject, JSValue, JsError, JsResult, +}; fn get_type_name(global_object: &JSGlobalObject, value: JSValue) -> ZigString { let js_type = value.js_type(); @@ -53,19 +55,6 @@ pub(crate) fn throw_err_invalid_arg_type( ) } -/// The error `validateAbortSignal(value, name)` throws. Callers pass every -/// defined value of the option (`JSValue::get`, not `get_truthy`): like Node, -/// only `undefined` means "no signal", so `null` or `""` end up here too. -/// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/validators.js#L444-L451 -#[cold] -pub(crate) fn throw_err_invalid_abort_signal( - global_this: &JSGlobalObject, - name: &str, - value: JSValue, -) -> JsError { - global_this.throw_invalid_argument_type_value2(name, "an instance of AbortSignal", value) -} - #[cold] fn throw_range_error(global_this: &JSGlobalObject, args: fmt::Arguments<'_>) -> JsError { global_this.err(jsc::ErrorCode::OUT_OF_RANGE, args).throw() @@ -483,6 +472,22 @@ pub(crate) fn validate_function( Ok(value) } +/// `validateAbortSignal(value, name)`, returning a ref to the native signal. +/// Callers skip `undefined` themselves (Node's `options.signal !== undefined`) +/// and pass everything else, so `null` and `""` are rejected like any other +/// non-signal. Only a real `AbortSignal` passes: it is the only kind the native +/// abort paths can listen to. +/// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/validators.js#L444-L451 +pub(crate) fn validate_abort_signal( + global_this: &JSGlobalObject, + value: JSValue, + name: &str, +) -> JsResult { + AbortSignal::ref_from_js(value).ok_or_else(|| { + global_this.throw_invalid_argument_type_value2(name, "an instance of AbortSignal", value) + }) +} + /// Rust has no field reflection; enums opt in via this trait. /// Implementors should typically `#[derive(strum::EnumString, strum::VariantNames)]` /// and provide `VALUES_INFO` as the `|`-joined variant names. From 9863825baa5130d9b9634169012a70ae07da3ea4 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 16 Aug 2026 07:24:17 +0000 Subject: [PATCH 3/3] validators: tighten the validate_abort_signal doc comment --- src/runtime/node/util/validators.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/runtime/node/util/validators.rs b/src/runtime/node/util/validators.rs index b20fb7427d09..2a611d254f06 100644 --- a/src/runtime/node/util/validators.rs +++ b/src/runtime/node/util/validators.rs @@ -472,11 +472,8 @@ pub(crate) fn validate_function( Ok(value) } -/// `validateAbortSignal(value, name)`, returning a ref to the native signal. -/// Callers skip `undefined` themselves (Node's `options.signal !== undefined`) -/// and pass everything else, so `null` and `""` are rejected like any other -/// non-signal. Only a real `AbortSignal` passes: it is the only kind the native -/// abort paths can listen to. +/// Node's `validateAbortSignal`, except that an `aborted`-shaped object is rejected +/// too: the native abort paths need a real `AbortSignal`. /// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/validators.js#L444-L451 pub(crate) fn validate_abort_signal( global_this: &JSGlobalObject,