Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
203 changes: 203 additions & 0 deletions src/bun.zig
Original file line number Diff line number Diff line change
Expand Up @@ -2148,6 +2148,195 @@
}
}

/// Kind of flag in `NODE_OPTIONS`. Controls how the filter treats a bare
/// (no `=value`) occurrence of the flag:
/// - `bool_flag`: no value expected; emit bare.
/// - `required_value`: must have a value. If no value token follows in
/// `NODE_OPTIONS`, the flag is DROPPED — never emitted bare, to avoid
/// binding a later argv token (e.g. the entrypoint) as the missing value.
const NodeOptionKind = enum { bool_flag, required_value };

/// Allowlist of flags supported by Bun that may appear in `NODE_OPTIONS`.
///
/// Matches Node.js's behavior of restricting `NODE_OPTIONS` to a subset of
/// flags (for security and predictability). We include only flags that Bun
/// actually implements — unknown flags are silently ignored.
pub const node_options_allowlist = ComptimeStringMap(NodeOptionKind, .{
// Boolean flags (no value)
.{ "--expose-gc", .bool_flag },
.{ "--no-addons", .bool_flag },
.{ "--no-deprecation", .bool_flag },
.{ "--preserve-symlinks", .bool_flag },
.{ "--preserve-symlinks-main", .bool_flag },
.{ "--throw-deprecation", .bool_flag },
.{ "--use-bundled-ca", .bool_flag },
.{ "--use-openssl-ca", .bool_flag },
.{ "--use-system-ca", .bool_flag },
.{ "--zero-fill-buffers", .bool_flag },
.{ "--cpu-prof", .bool_flag },
.{ "--cpu-prof-md", .bool_flag },
.{ "--heap-prof", .bool_flag },
.{ "--heap-prof-md", .bool_flag },

// Options that take a value. All treated as required_value: bare form
// (no `=value` and no following value token) is dropped to prevent
// binding the user's entrypoint as the missing value.
.{ "--conditions", .required_value },
.{ "-C", .required_value },

Check failure on line 2185 in src/bun.zig

View check run for this annotation

Claude / Claude Code Review

-C in allowlist but Bun has no -C short flag — startup failure

The allowlist includes `-C` (Node's short alias for `--conditions`), but Bun's clap definition only declares `--conditions <STR>...` with no `-C` short form. Setting `NODE_OPTIONS='-C browser'` (valid in Node.js) will inject `['-C', 'browser']` into argv, and clap will fail with `InvalidArgument` and exit(1) at startup — worse than the pre-PR behavior of ignoring NODE_OPTIONS. Either drop `-C` from the allowlist or add the `-C` short alias to the `--conditions` clap param.
Comment thread
robobun marked this conversation as resolved.
.{ "--cpu-prof-dir", .required_value },
.{ "--cpu-prof-interval", .required_value },
.{ "--cpu-prof-name", .required_value },
.{ "--dns-result-order", .required_value },
.{ "--heap-prof-dir", .required_value },
.{ "--heap-prof-name", .required_value },
.{ "--import", .required_value },
.{ "--inspect", .required_value },
.{ "--inspect-brk", .required_value },
.{ "--inspect-wait", .required_value },

Check failure on line 2195 in src/bun.zig

View check run for this annotation

Claude / Claude Code Review

Bare --inspect/--inspect-brk/--inspect-wait silently dropped from NODE_OPTIONS

Bare `--inspect`, `--inspect-brk`, and `--inspect-wait` are classified as `.required_value` in `node_options_allowlist`, so the filter at lines 2310–2315 drops them when no value follows — but bare `NODE_OPTIONS='--inspect'` / `'--inspect-brk'` is the canonical Node.js usage, and clap defines these as `<STR>?` (optional value, src/cli/Arguments.zig:87-89). Add an `.optional_value` variant to `NodeOptionKind` for these three flags (emit bare when no value follows, otherwise emit flag+value); this
Comment thread
robobun marked this conversation as resolved.
.{ "--max-http-header-size", .required_value },
.{ "--require", .required_value },
.{ "-r", .required_value },
.{ "--title", .required_value },
.{ "--unhandled-rejections", .required_value },
});

/// Parses `NODE_OPTIONS` env var into command-line tokens filtered by
/// `node_options_allowlist` and inserts them into `args` starting at
/// index 1 (after argv[0]).
///
/// Splits on whitespace with support for single/double quotes and backslash
/// escapes (POSIX semantics: backslash has no special meaning inside single
/// quotes). Positional (non-flag) tokens and unrecognized flags are dropped
/// to prevent unintended script execution via the environment. Required-value
/// flags that appear without a value are also dropped, so a bare
/// `NODE_OPTIONS="--require"` cannot hijack the user's entrypoint.
pub fn appendNodeOptionsEnv(env: []const u8, args: *std.array_list.Managed([:0]const u8)) !void {
// First, tokenize the NODE_OPTIONS string (quote- and escape-aware).
var tokens = std.array_list.Managed([]u8).init(bun.default_allocator);
defer {
for (tokens.items) |t| bun.default_allocator.free(t);
tokens.deinit();
}

var buf = std.array_list.Managed(u8).init(bun.default_allocator);
defer buf.deinit();

var in_quote: ?u8 = null;
var escape = false;
var has_token = false;

var i: usize = 0;
while (i < env.len) : (i += 1) {
const ch = env[i];

if (escape) {
try buf.append(ch);
escape = false;
has_token = true;
continue;
}

// Single quotes suppress all escaping (POSIX). Handle the in-quote
// branch before the backslash branch so `\` inside `'…'` is literal.
if (in_quote) |q| {
if (q == '\'') {
// Single-quoted: no escapes, only the matching quote closes.
if (ch == '\'') {
in_quote = null;
} else {
try buf.append(ch);
has_token = true;
}
continue;
}
// Double-quoted: backslash escapes the next character.
if (ch == '\\') {
escape = true;
continue;
}
if (ch == q) {
in_quote = null;
} else {
try buf.append(ch);
has_token = true;
}
continue;
}

if (ch == '\\') {
escape = true;
continue;

Check failure on line 2268 in src/bun.zig

View check run for this annotation

Claude / Claude Code Review

Unquoted backslash treated as escape — diverges from Node, breaks Windows paths

The unquoted-context backslash branch (lines 2266-2268) treats `\` as an escape and drops it, but Node.js's `ParseNodeOptionsEnvVar` only honors backslash escapes *inside double quotes* — outside quotes, backslash is literal. So on Windows, `set NODE_OPTIONS=--require C:\Users\foo\preload.js` (the natural unquoted form, used by IDEs and APM agents) gets tokenized as `C:Usersfoopreload.js` here while Node passes the path verbatim, silently breaking `--require`/`--import`/`--cpu-prof-dir`/etc. Rem
Comment thread
robobun marked this conversation as resolved.
}

if (ch == '\'' or ch == '"') {
in_quote = ch;
// Mark that we've seen a token even if the quoted body is empty,
// so `--flag ""` preserves the empty string as a distinct token.
has_token = true;
continue;
}

if (std.ascii.isWhitespace(ch)) {
if (has_token) {
try tokens.append(try buf.toOwnedSlice());
has_token = false;
}
continue;
}

try buf.append(ch);
has_token = true;
}
if (has_token) {
try tokens.append(try buf.toOwnedSlice());
}

// Now filter tokens through the allowlist and insert into args.
var offset: usize = 1;
var j: usize = 0;
while (j < tokens.items.len) : (j += 1) {
const token = tokens.items[j];
if (token.len == 0 or token[0] != '-') {
// Positional args are not allowed in NODE_OPTIONS.
continue;
}

// Split --flag=value into name and value.
const eq_idx = std.mem.indexOfScalar(u8, token, '=');
const flag_name = if (eq_idx) |idx| token[0..idx] else token;

const kind = node_options_allowlist.get(flag_name) orelse continue;

if (kind == .required_value and eq_idx == null) {
// Required-value flag without inline =value: look for a following
// value token. If there isn't one (or the next token is itself a
// flag), DROP this flag entirely — never emit a bare required-value
// flag, or clap would bind the user's entrypoint as the value.
if (j + 1 >= tokens.items.len) continue;
const next_tok = tokens.items[j + 1];
// An empty quoted value (`--flag ""`) is legitimate; reject only
// tokens that start with `-`, which would be another flag.
if (next_tok.len > 0 and next_tok[0] == '-') continue;

const token_z = try bun.default_allocator.dupeZ(u8, token);
try args.insert(offset, token_z);
offset += 1;

const next_z = try bun.default_allocator.dupeZ(u8, next_tok);
try args.insert(offset, next_z);
offset += 1;
j += 1;
continue;
}

// bool_flag, or required_value with inline `--flag=value`: safe to
// emit the token as-is.
const token_z = try bun.default_allocator.dupeZ(u8, token);
try args.insert(offset, token_z);
offset += 1;
}
}

pub fn initArgv() !void {
if (comptime Environment.isPosix) {
argv = try bun.default_allocator.alloc([:0]const u8, std.os.argv.len);
Expand Down Expand Up @@ -2215,6 +2404,20 @@
argv = argv_list.items;
bun_options_argc = argv.len - original_len;
}

// NODE_OPTIONS: Node.js-compatible env var for injecting CLI flags.
// Filtered through an allowlist — unknown flags are dropped.
// Counted toward `bun_options_argc` so standalone binaries compute the
// correct passthrough offset (see offset_for_passthrough in cli.zig).
if (bun.env_var.NODE_OPTIONS.get()) |opts| {
if (opts.len > 0) {
const before = argv.len;
var argv_list = std.array_list.Managed([:0]const u8).fromOwnedSlice(bun.default_allocator, argv);
try appendNodeOptionsEnv(opts, &argv_list);
argv = argv_list.items;
bun_options_argc += argv.len - before;
}
}
Comment thread
robobun marked this conversation as resolved.
}

pub const spawn = @import("./runtime/api/bun/spawn.zig").PosixSpawn;
Expand Down
1 change: 1 addition & 0 deletions src/bun_core/env_var.zig
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ pub const JENKINS_URL = New(kind.string, "JENKINS_URL", .{});
pub const MI_VERBOSE = New(kind.boolean, "MI_VERBOSE", .{ .default = false });
pub const NO_COLOR = New(kind.boolean, "NO_COLOR", .{ .default = false });
pub const NODE_CHANNEL_FD = New(kind.string, "NODE_CHANNEL_FD", .{});
pub const NODE_OPTIONS = New(kind.string, "NODE_OPTIONS", .{});
/// Set by HostProcess.zig when spawning the WebView host subprocess. The
/// child's cli.zig checks this before anything else and hands off to C++
/// Bun__WebView__hostMain. Never returns — no JSC, no VM.
Expand Down
108 changes: 108 additions & 0 deletions test/regression/issue/28817.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
// https://github.com/oven-sh/bun/issues/28817
import { describe, expect, test } from "bun:test";
import { bunEnv, bunExe, isWindows } from "harness";

async function runWith(nodeOptions: string, script: string) {
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", script],
env: { ...bunEnv, NODE_OPTIONS: nodeOptions },
stderr: "pipe",
stdout: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
return { stdout: stdout.trim(), stderr, exitCode };
}

describe("NODE_OPTIONS", () => {

Check warning on line 16 in test/regression/issue/28817.test.ts

View check run for this annotation

Claude / Claude Code Review

Use describe.concurrent for process-spawning tests (per test/CLAUDE.md)

nit: Per `test/CLAUDE.md`, tests that spawn multiple subprocesses should use `describe.concurrent` — this file spawns ~13 independent subprocesses across 8 tests with no shared state, so changing `describe("NODE_OPTIONS", ...)` to `describe.concurrent("NODE_OPTIONS", ...)` would match the repo convention and cut test runtime.
Comment thread
robobun marked this conversation as resolved.
test("--dns-result-order honored via env (= and space forms)", async () => {
const getOrder = 'import dns from "node:dns"; console.log(dns.getDefaultResultOrder());';

// --flag=value form
const eq = await runWith("--dns-result-order=ipv4first", getOrder);
expect(eq.stdout).toBe("ipv4first");
expect(eq.exitCode).toBe(0);

// --flag value (space separated) form
const sp = await runWith("--dns-result-order ipv6first", getOrder);
expect(sp.stdout).toBe("ipv6first");
expect(sp.exitCode).toBe(0);

// quoted value
const q = await runWith(`--dns-result-order='verbatim'`, getOrder);
expect(q.stdout).toBe("verbatim");
expect(q.exitCode).toBe(0);
});

test("default order is verbatim without NODE_OPTIONS", async () => {
const r = await runWith("", 'import dns from "node:dns"; console.log(dns.getDefaultResultOrder());');
expect(r.stdout).toBe("verbatim");
expect(r.exitCode).toBe(0);
});

test("getDefaultResultOrder returns a string, not a function", async () => {
const r = await runWith(
"--dns-result-order=ipv4first",
'import dns from "node:dns"; const v = dns.getDefaultResultOrder(); console.log(typeof v, v);',
);
expect(r.stdout).toBe("string ipv4first");
expect(r.exitCode).toBe(0);
});

test("unknown flags and positional args are dropped", async () => {
// Unknown flag is ignored; known flag still works.
const r1 = await runWith(
"--unknown-flag --dns-result-order=ipv4first",
'import dns from "node:dns"; console.log(dns.getDefaultResultOrder());',
);
expect(r1.stdout).toBe("ipv4first");
expect(r1.exitCode).toBe(0);

// Positional arg (non-flag) cannot inject a script/entrypoint.
const r2 = await runWith("/etc/passwd", 'console.log("safe");');
expect(r2.stdout).toBe("safe");
expect(r2.exitCode).toBe(0);

// --eval is not in the allowlist, so this must not execute injected code.
const r3 = await runWith("--eval console.log('HIJACKED')", 'console.log("original");');
expect(r3.stdout).not.toContain("HIJACKED");
expect(r3.stdout).toBe("original");
expect(r3.exitCode).toBe(0);
});

test("--expose-gc exposes gc() via env", async () => {
const r = await runWith("--expose-gc", "console.log(typeof gc);");
expect(r.stdout).toBe("function");
expect(r.exitCode).toBe(0);
});

test.skipIf(isWindows)("--title sets process.title via env", async () => {
// process.title is unreliable on Windows (varies by OS version); skip there.
const r = await runWith("--title=my-bun-app", "console.log(process.title);");
expect(r.stdout).toBe("my-bun-app");
expect(r.exitCode).toBe(0);
});

test("bare required-value flag does not hijack the entrypoint", async () => {
// Regression guard for a `--require` / `--dns-result-order` etc. with no
// value following in NODE_OPTIONS. A bare required-value flag must be
// DROPPED; otherwise clap would bind the user's -e script (or entrypoint)
// as the flag's value and the script would never run.
const r = await runWith("--dns-result-order", 'console.log("ran");');
expect(r.stdout).toBe("ran");
expect(r.exitCode).toBe(0);

// Same with --require — must not consume the -e script as the path.
const r2 = await runWith("--require", 'console.log("still ran");');
expect(r2.stdout).toBe("still ran");
expect(r2.exitCode).toBe(0);
});

test("bare required-value flag followed by another flag is dropped", async () => {
// `--dns-result-order` has no value, then `--expose-gc` follows — which
// starts with `-`, so the first flag must not consume it. Result: the
// first flag is dropped; --expose-gc still takes effect.
const r = await runWith("--dns-result-order --expose-gc", "console.log(typeof gc);");
expect(r.stdout).toBe("function");
expect(r.exitCode).toBe(0);
});
});
Loading