Repository navigation
Conversation
The export builtin inserted every argument into the exported environment without a check on the name. `export 1abc a-b=5` put `1abc=` and `a-b=5` into the environment of child processes. Validate each name with is_valid_var_name. An invalid word is reported on stderr as "export: `<word>`: not a valid identifier" and is not exported. The valid words are still exported. The exit code is 1 when any word was rejected, like bash.
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 10 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Reproduced with the released bun: Before the fix, nine of the ten new runs fail: stderr is empty, the exit code is 0, and the child sees #32298 was a second PR for the same bug. It is closed. Its test cases pass on this branch, and the three it had that this PR lacked are now included. CI at |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
An empty argument is not a valid identifier either. Drop the skip so the same check reports it, like bash.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 5:55 AM PT - Aug 28th, 2026
❌ @robobun, your commit 510edee has 2 failures in
🧪 To try this PR locally: bunx bun-pr 40710That installs a local version of the PR into your bun-40710 --bun |
POSIX scripts write `export -- NAME=value`. The identifier check would report `--` as an invalid name and exit 1. Treat a leading `--` as the end of options, like bash.
There was a problem hiding this comment.
LGTM — both earlier points (empty-arg bypass and -- being rejected) are now fixed and covered by tests.
Checked that on_io_writer_chunk still returns exit 0 for the print_all/WaitingIo path (only State::Err or a write error yields 1), that write_failing_error's no-io branch already passes exit_code=1 directly so State::Err is only needed for the async fd path, and that is_valid_var_name(b"") returns false so the removed is_empty() skip is safe.
Extended reasoning...
Overview
The PR adds identifier validation to the shell export builtin: each argument's name portion is checked with is_valid_var_name (promoted from pub(crate) to pub in bun_shell_parser and re-exported through crate::shell), invalid words emit a bash-style stderr line and set exit code 1 while valid words in the same invocation are still exported, and a leading -- is treated as the POSIX end-of-options marker. A new State::Err variant lets the async-write completion callback report exit 1. Four new TestBuilder cases in test/js/bun/shell/bunshell.test.ts cover invalid/mixed args, exit code, valid identifiers, and -- handling, with .testMini() exercising the fd path.
Security risks
None. This is input validation on shell-builtin argument names; the change tightens rather than loosens behavior. No new unsafe blocks, no filesystem or network surface, and the byte search for = continues to use the SIMD bun_core::strings::index_of_char_usize helper per src/CLAUDE.md.
Level of scrutiny
Low-to-moderate. The change is ~30 lines in one builtin plus visibility re-exports, follows the exact write_failing_error / state-machine pattern used by sibling builtins (cat, mv, yes), and is well covered by tests that assert stderr content, exit code, and that valid names are still exported. Both concerns I raised on earlier revisions — the s.is_empty() early-continue bypassing validation, and -- being rejected as an invalid identifier — have been addressed in the current diff and each has a dedicated test assertion.
Other factors
I verified the shared Builtin::write_failing_error helper: its no-io branch calls Builtin::done with the passed exit_code directly, so State::Err only matters on the async fd branch where on_io_writer_chunk fires — and that callback still yields exit 0 for the pre-existing print_all → WaitingIo flow. No third-party CHANGES_REQUESTED reviews are outstanding; the author-resolved threads correspond to code that has demonstrably changed. Exit reason was dry_streak.
There was a problem hiding this comment.
LGTM — both earlier concerns (empty-word bypass, -- rejected as an identifier) are fixed and now have dedicated tests.
What was reviewed:
is_valid_var_namegate covers all invalid forms (empty,=x, leading digit, hyphen); valid siblings still exported, exit 1 on any failure.State::Erris read inon_io_writer_chunkbefore being overwritten toDone, so the async-fd stderr path keeps exit 1;.testMini()cases exercise that path.--handling: only a single leading--is skipped,export --falls through toprint_all, and a second--after it is correctly rejected.
Extended reasoning...
Overview
The PR makes the Bun shell export builtin validate variable names against is_valid_var_name (the same [A-Za-z_][A-Za-z0-9_]* check the parser already uses for NAME=value prefixes), accumulates one bash-style error line per bad word, still exports the valid words, and exits 1 if anything was rejected. It also adds POSIX -- end-of-options handling. Source changes are ~30 lines in src/runtime/shell/builtin/export.rs plus two one-line visibility/re-export changes; tests add 7 cases to test/js/bun/shell/bunshell.test.ts.
Security risks
None. This tightens validation of shell builtin arguments and changes an exit code from 0 to 1 for previously-accepted garbage. No new parsing of untrusted external data, no allocation sized by attacker input, no unsafe added. The one pre-existing unsafe block (env map insert) is unchanged.
Level of scrutiny
Low-to-moderate. The builtin is a small self-contained state machine; the new State::Err variant is handled on the only completion path (on_io_writer_chunk), and the sync captured-output path goes through write_failing_error(.., 1) directly. I raised two issues on earlier pushes (empty-arg bypass and -- being rejected) — both were fixed in follow-up commits with tests, and the most recent commit is test-only, adding coverage for the empty word and mixed valid/invalid arg lists.
Other factors
No CODEOWNERS entry matches the shell paths. The bug hunt exited on dry_streak with no findings. Test coverage now spans the variant matrix REVIEW.md asks for: leading digit, hyphen, empty, =x, mixed valid/invalid (both bare-name and assignment forms), -- alone and -- FOO=bar, and &&/|| chaining to observe the exit code. Two tests use .testMini() to exercise the real-fd async write path where State::Err matters.
There was a problem hiding this comment.
LGTM — both earlier concerns (empty-word bypass and -- end-of-options) are now addressed with tests, and the added .quiet() case covers the synchronous stderr path.
Checked: is_valid_var_name re-export is a visibility-only change; State::Err is threaded through on_io_writer_chunk so the async-fd path exits 1; valid assignments in a mixed invocation are still exported before the error write; start >= argc correctly falls through to print_all for both export and export --. The error buffer echoes the full argv word (matching bash) and no EnvStr is allocated for rejected names.
Extended reasoning...
Overview
The PR makes the shell export builtin validate identifier names via the parser's existing is_valid_var_name, buffer per-arg diagnostics to stderr, and exit 1 when any name is invalid — while still exporting the valid ones. It also adds -- end-of-options handling and drops the silent empty-arg skip. Touched files are src/runtime/shell/builtin/export.rs (the actual logic, ~25 lines net), a pub(crate)→pub visibility bump in src/shell_parser/parse.rs, two one-line re-exports, and eight new tests in test/js/bun/shell/bunshell.test.ts.
Security risks
None identified. The change tightens validation of shell builtin arguments and does not introduce new parsing, allocation on untrusted sizes, or FFI. The error message echoes the argv word verbatim, which is the same behavior bash has and is written to the caller's own stderr.
Level of scrutiny
Low-to-moderate. The builtin is a small self-contained state machine; the diff follows the file's existing patterns (Builtin::write_failing_error, the State enum + on_io_writer_chunk completion). The -- skip is a one-liner that correctly degrades to print_all when it is the only arg. Both concerns raised in earlier rounds — the empty-word bypass and -- being rejected as an identifier — have been fixed and each has a dedicated test. The most recent push only adds a .quiet() variant to exercise the synchronous write_no_io branch alongside the async-fd branch.
Other factors
No CODEOWNERS entry covers src/runtime/shell/ or src/shell_parser/. No outstanding CHANGES_REQUESTED reviews. Exit reason was dry_streak. Test coverage spans mixed valid/invalid args, bare names vs assignments, empty word, -- in both forms, || fallthrough proving valid vars were set, and both stderr I/O paths (.testMini() fd write and .quiet() buffer). The -p/-n/-f option words now error instead of being silently exported as -p= — the PR description names #33995 as the tracked follow-up for real option parsing, and erroring is strictly less wrong than the prior behavior.
clippy denies needless_pass_by_value on the err argument. A write error keeps precedence over the identifier failure.
There was a problem hiding this comment.
LGTM — the earlier feedback (empty-arg bypass, -- end-of-options) is now handled and covered by tests, and the latest push is just the clippy-driven match err reshape in on_io_writer_done, which is behavior-preserving.
Checked: is_valid_var_name re-export chain resolves through bun_shell_parser → crate::shell; the = split still uses bun_core::strings::index_of_char_usize; error text/exit code/ordering match the bash reference in the PR description; both the async-fd (on_io_writer_done with State::Err) and .quiet() synchronous paths are exercised; export -- alone falls through to print_all and -- is not itself exported.
Extended reasoning...
Overview
This PR makes the Bun shell's export builtin validate identifier names against is_valid_var_name ([A-Za-z_][A-Za-z0-9_]*), matching bash: invalid words produce export: `WORD`: not a valid identifier on stderr and exit 1, while valid words in the same command are still exported. The change touches src/runtime/shell/builtin/export.rs (validation loop, new State::Err, -- end-of-options handling, exit-code plumbing in on_io_writer_done), promotes is_valid_var_name to pub and re-exports it through src/shell_parser/lib.rs and src/runtime/shell/mod.rs, and adds eight tests in test/js/bun/shell/bunshell.test.ts.
Security risks
None. The change tightens validation of shell-builtin arguments and rejects more inputs than before; it does not introduce new parsing, allocation sizing driven by untrusted lengths, or filesystem/network access. Error messages echo user-supplied argv bytes back to stderr, which is the same behavior as bash and not a disclosure concern.
Level of scrutiny
Low-to-moderate. The Rust change is ~30 net lines in a single builtin, follows the existing Builtin::write_failing_error / State pattern used by sibling builtins, and reuses the parser's existing is_valid_var_name. Earlier review rounds raised two correctness gaps (empty-arg skip acting as a validation bypass; -- being rejected as an invalid identifier and flipping exit 0→1) — both are fixed in the current diff with dedicated tests. The most recent commit only reshapes on_io_writer_done to consume err by value for clippy's needless_pass_by_value, keeping write-error precedence over State::Err; the resulting exit code is identical to the prior err.is_some() || failed form.
Other factors
Test coverage is thorough for the size of the change: mixed valid/invalid args (child env inspected via a spawned bun -e), exit code 1 for bare invalid names and invalid assignments separately, the .quiet() in-memory-buffer path vs the default async fd path, export -- both with and without following args, export "", and || fallthrough confirming valid assignments persist. No CODEOWNERS entries cover these paths. All prior inline threads on this PR correspond to commits that landed the requested change plus a test, so author-resolution is backed by code.
…nd assignments (#40764) Fixes #40763. ### Problem - `export NAME=$(echo "hello world")` sets `NAME=hello` and exports a stray empty variable `world`. bash and dash give `NAME=hello world`. - The expansion of a command argument field splits unquoted command substitution output into several argv words (`src/runtime/shell/states/Cmd.rs`, the `ExpandingArgs` arm of `child_done`). The expansion does not know the word is an assignment operand of `export`. - A plain assignment `NAME=$(cmd)` hides the same split: `Assigns::child_done` re-joins the words with single spaces, so runs of whitespace and newlines collapse. ### Fix - Add an `assign_ctx` flag to `Expansion`. When set, the command substitution output is not field split (the same path as a quoted `"$(...)"`). POSIX 2.9.1: an assignment word undergoes no field splitting. - `Cmd` sets the flag for an operand of `export` (the only declaration builtin) that starts with a literal `NAME=` prefix where `NAME` is a valid identifier. A word whose name comes from an expansion, for example `export $(echo "A=a b")`, still splits, like in bash. - `Assigns` sets the flag for every assignment value, so `VAR=$(echo a && echo b)` now keeps the newline instead of collapsing to `a b`. - Verified: three new tests in `test/js/bun/shell/bunshell.test.ts` fail on current bun and pass with the fix. The full `test/js/bun/shell/` suite passes except pre-existing environment failures (root-user permission tests, ASAN timeouts), which fail the same way without the change. ### Background - The shell interpreter is a tree of state nodes. A `Cmd` expands each argv atom through an `Expansion` child, which returns a buffer plus `bounds`, the offsets that split the buffer into argv words. - An unquoted `$(...)` runs `post_subshell_expansion`, which turns newlines into spaces and splits on space runs. A quoted `"$(...)"` skips that and only trims trailing whitespace. `assign_ctx` reuses the quoted path. - Glob and brace expansion of assignment values are unchanged. Only the field split of command substitution output is suppressed. <details><summary>Notes</summary> - Repro: `bun -e 'await Bun.$`export NAME=$(echo "hello world"); echo "NAME=[$NAME]"`'` prints `NAME=[hello]` before, `NAME=[hello world]` after. - With #40710 (identifier validation in `export`), the split also turns into a hard error when a split word is not a valid identifier, for example `export NAME=$(echo "a b-c")`. This fix removes the split, so the two compose. - `CondExpr` passes `assign_ctx: false` to keep its behavior unchanged. Bash also suppresses splitting inside `[[ ]]`, but that is a separate concern. - Variable expansion (`$X`) was never field split in the Bun shell, so only command substitution output was affected. - Known divergence: the AST folds quoted and unquoted text into the same `SimpleAtom::Text`, so `export "NAME="$(cmd)` also gets assignment treatment, while bash splits there. Restoring the quoting bit needs a parser AST change. The effect is benign: `export` already treats any operand with `=` as an assignment at runtime, so the flag only stops split words from becoming stray exported variables. zsh does not split here either. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/shell/bunshell.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This branch has not been deployed
2 participants
Problem
exportbuiltin accepts any name.export 1abc a-b=5puts1abc=anda-b=5into every child environment, prints nothing, and exits 0. Found by comparison with bash, no user report.src/runtime/shell/builtin/export.rs:33-44splits each word at the first=and inserts the pair intoexport_envwithout callingis_valid_var_name(src/shell_parser/parse.rs:3788).Fix
Export::startchecks each name withis_valid_var_name(nowpubinbun_shell_parser). An invalid word is not exported and stderr getsexport: `1abc`: not a valid identifier. Valid words in the same command are still exported. The exit code is 1, as in bash.Builtin::write_failing_error. A newState::Errvariant makeson_io_writer_chunkreport exit 1 after an async write (a write error still wins). A leading--is skipped.export NAME=$(cmd). Without it, a split word such as28now fails the command (Notes).test/js/bun/shell/bunshell.test.ts, eight new tests inenv variables(two also viabun run script.bun.sh, one with.quiet()). The released bun fails nine of ten runs. Other shell tests andcargo clippy -p bun_runtimepass.Background
IOWriterand yields.on_io_writer_chunkfires when the write completes and passes the exit code toBuiltin::done.$uses this path by default.$.quiet(), stderr is a buffer:write_no_ioappends anddoneruns at once.is_valid_var_nameaccepts[A-Za-z_][A-Za-z0-9_]*, as bash does.Notes
=, returned on the first bad word, and exited 0. The port dropped the check. This change validates both forms, processes every word, and exits 1.export NAME=$(cmd)field-splits the command substitution output #40763, fixed by shell: stop field splitting command substitution in export operands and assignments #40764: the Bun shell field-splits the output of$(cmd)inexport NAME=$(cmd). bash does not. Withexport DATE=$(date), the old code setDATEto the first word only and silently exportedAug=,28=,12:37:27=and so on, exit 0. With this change alone,DATEis still the first word and the split words that are not identifiers (28,12:37:27,2026) are reported, exit 1. On Windows, package.json scripts run in the Bun shell, so such a script would start to fail. The value ofDATEis wrong either way. The workaround isexport DATE="$(date)". shell: stop field splitting command substitution in export operands and assignments #40764 removes the split, so the two changes compose. Both insert tests afterexported vars 2inbunshell.test.ts, so whichever lands second needs a small rebase.on_io_writer_chunk: a write error gives 1, thenState::Errgives 1, else 0. shell: exit with the positive errno when a builtin's output write fails #40702 (open) changes the write error arm to the errno. The two changes are independent. Whichever lands second rebases one arm.export ""exits 1, and the exit code when valid and invalid words are mixed, for bare names and for assignments) are now in this PR. shell(export): reject invalid identifiers #32298 is closed as a duplicate.export NAMEdoes when the variable already exists and how a reassignment reaches the child environment. Those are separate behaviors and are not part of this change.export "") is rejected too, like bash. The old code skipped it.-p,-nand-fare not supported by the Bun shell, before or after this change. Before,export -pput-p=into the environment. Now it is reported as an invalid word. shell: accept--end-of-options delimiter in builtins #33995 (open) adds--handling to every builtin with a shared helper. It will need a rebase on this change.bun run x.bun.sh(stderr is a real fd) exits 1 for each bad name with stderr redirected to/dev/null, to a file, and through a pipe.test/js/bun/shell/shell-load.test.tsandcommands/ls.test.tsthat fail locally also fail on main in this container (root user, timing). They are unrelated.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/shell/bunshell.test.ts