Skip to content

node:fs: read the options before the path is checked #43563

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
+470 −128
Open

node:fs: read the options before the path is checked #43563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions src/js/internal/fs/watch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
}
// 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") {
Expand Down
2 changes: 1 addition & 1 deletion src/js/node/fs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
5 changes: 5 additions & 0 deletions src/jsc/CallFrame.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<JSValue> {
self.remaining().get(index).copied()
}

pub fn next_eat(&mut self) -> Option<JSValue> {
let v = self.remaining().first().copied()?;
self.eat();
Expand Down
221 changes: 104 additions & 117 deletions src/runtime/node/node_fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3628,9 +3628,13 @@ pub(crate) mod args {
ctx: &JSGlobalObject,
arguments: &mut ArgumentsSlice,
) -> JsResult<Self> {
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,
})
}
}

Expand All @@ -3643,50 +3647,50 @@ pub(crate) mod args {
ctx: &JSGlobalObject,
arguments: &mut ArgumentsSlice,
) -> JsResult<Self> {
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<Encoding> {
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<JSValue>,
}

/// 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<JSValue>,
default: Encoding,
) -> JsResult<Encoding> {
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<Options> {
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);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
}
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> {
Expand Down Expand Up @@ -3892,15 +3896,19 @@ pub(crate) mod args {
ctx: &JSGlobalObject,
arguments: &mut ArgumentsSlice,
) -> JsResult<Self> {
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",
b"string, Buffer, or URL",
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,
})
}
}

Expand Down Expand Up @@ -3929,34 +3937,22 @@ pub(crate) mod args {
ctx: &JSGlobalObject,
arguments: &mut ArgumentsSlice,
) -> JsResult<Self> {
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,
})
Expand Down Expand Up @@ -4521,47 +4517,42 @@ pub(crate) mod args {
ctx: &JSGlobalObject,
arguments: &mut ArgumentsSlice,
) -> JsResult<Self> {
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(|| {
ctx.throw_invalid_arguments(format_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::<AbortSignalRef>, |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,
Expand Down Expand Up @@ -4608,6 +4599,11 @@ pub(crate) mod args {
arguments: &mut ArgumentsSlice,
default_flag: FileSystemFlags,
) -> JsResult<Self> {
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(|| {
Expand All @@ -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::<AbortSignalRef>, |s| {
Expand All @@ -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_)
);
}
}
}
Expand All @@ -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 {
Expand Down
Loading
Loading