fix(arithmetic): run nested command substitution in the current shell state

[trieloff]

Fails open to a wrong number

A $(...) / backtick command substitution nested inside $(( )) or (( )) ran in a detached shell seeded from the initial environment. It could not see any variable the running script had set — not even exported ones. Because the missing value produced empty output rather than an error, the arithmetic silently evaluated to a plausible wrong number:

X=abc
echo $(( $(echo "$X" | wc -c) ))           # -> 1    GNU bash: 4
echo $(( $(echo ${X:-DEFAULT} | wc -c) ))  # -> 8    — proves X was unset in there
export EE=zz
echo $(( $(echo "${EE:-none}" | wc -c) ))  # -> 5    — exported vars invisible too
echo $(( $(echo $HOME | wc -c) ))          # -> 11   — the *initial* env WAS visible

The shape it was reported as, where "$X" vanishing shifted +%s into -d's argument slot:

X=2026-09-08T00:19:09Z
date -d "$X" +%s                  # 1788826749  (correct on its own)
echo $(( $(date -d "$X" +%s) - 60 ))
# before: -60, plus `date: invalid date '+%s'` on stderr
# GNU bash / after: 1788826689

Root cause

arithmetic.ts:452 (numeric path) and arithmetic.ts:962 (string path). ArithCommandSubst keeps the substitution as raw text and ran it via ctx.execFn, the Bash-level exec (Bash.ts:818 → execInScope) — a fresh top-level execution seeded from the instance's base state, not the running interpreter's.

Ordinary command substitution does the right thing and is the model followed here: expansion.ts:755+ executes the parsed body via ctx.executeScript, sharing the current state. That is why $(echo $(echo "$X")) always worked, and why the let form worked (it expands as a word first).

The backtick form appeared correct only by accident — expandDollarVarsInArithText textually pre-expanded $var inside backticks before the detached shell ran, which is its own problem (see below).

What changed

1. runCommandSubstitution extracted from expansion.ts into expansion/command-substitution.ts — the depth guard, env/array/cwd save-restore, expansionStderr plumbing, ExitError handling and output-length check, unchanged. Both ArithCommandSubst sites and the array-subscript sites in expandSubscriptForAssocArray (same ctx.execFn defect, reachable via ${arr[$(...)]}) now go through it. ctx.execFn no longer appears anywhere in the expansion path.

2. Number.parseInt(output, 10) || 0 removed — behaviour change, called out deliberately. bash splices the substitution's output into the expression as text and parses that as arithmetic. parseInt was a second fail-open: $(( $(echo "1 + 2") )) silently became 2, and $(( $(echo "hello world") )) silently became 0. The output is now parsed as an arithmetic expression, matching bash:

   expression	before	after	GNU bash
   $(( $(echo "1 + 2") ))	2	3	3
   $(( $(echo " 7 ") ))	7	7	7
   v=9; $(( $(echo v) ))	0	9	9
   $(( $(echo "0x10") ))	0	16	16
   $(( $(echo "010") ))	10	8	8
   $(( $(echo "hello world") ))	0	syntax error, exit 1	syntax error, exit 1
   $(( $(echo "") ))	0	0	0

   Note the empty case: I checked this against bash 5.3 and 3.2 rather than assuming. bash does not error on an empty operand when it is the whole expression — $(( $(echo "") )), $(( $(true) )) and $(( $(echo abc) )) are all 0, because the spliced text is empty / an unset variable name. The operand-expected error only fires in operand position ($(( 1 + $(echo "") ))), which falls out of the normal parser. So the new failure mode is a genuine syntax error on unparsable output, not a blanket error on emptiness.

   bash also does not re-expand the spliced text, so a $(...) in the output is rejected rather than executed — this also stops f() { echo '$(f)'; }; echo $(( $(f) )) from recursing forever.

3. Substitution text is kept verbatim through preprocessArithInput and expandDollarVarsInArithText. Both used to rewrite the inside of the substitution, which was harmless when the text went to a detached shell but is not once it is parsed as a real shell script:

   • preprocessArithInput stripped double quotes everywhere, turning $(printf %s "$Q") into $(printf %s $Q) and word-splitting the value. Q="a b" gave 2 instead of 4.
   • expandDollarVarsInArithText spliced variable values into backtick commands. With Q='$(echo PWNED)', $(( \printf %s "$Q" | wc -c` ))returned 5 — the length ofPWNED— because the variable's data was re-parsed and executed as shell syntax. It now returns 13, the length of the literal, matching bash.$((` still falls through to be preprocessed as before.

Known limitation, unchanged

Splicing is done at the value level, not the text level, so precedence across the substitution boundary still differs: $(( $(echo "2 + 3") * 4 )) is 20 here and 14 in bash. That is the same limitation already documented at the top of arithmetic.ts for $var expansion; making it exact needs the substitution expanded before the arithmetic is parsed, which is a broader change than this fix.

Tests

• src/comparison-tests/arithmetic-command-substitution.comparison.test.ts — 27 cases recorded against real bash: variable visibility (plain, ${:-}, exported, function, local, cwd), all five entry points ($(( )), (( )), backticks, let, a=$(( )), array subscript), quoting preservation, nesting, output splicing, empty/failing substitutions, and subshell isolation. Verified identical on bash 3.2 and 5.3 before recording.
• src/interpreter/arithmetic-command-substitution.test.ts — 10 unit tests asserting full stdout and stderr for what fixtures don't cover: stderr forwarding at expansion time, the syntax-error paths, the nesting guard, and that a Bash({ env: { X: ... } }) initial value no longer shadows script state.

Validation

pnpm typecheck && pnpm lint && pnpm knip clean. pnpm test:run: 15647 passed, 98 skipped. The 6 remaining failures are pre-existing CPython-WASM/worker tests (python3 lazy-load, sys.path, traceback, worker-protocol desync) that fail identically on the base commit on this machine — confirmed by re-running them against unmodified src/interpreter.

Scope is this bug only; expansion/array-prefix-suffix.ts and control-flow.ts are untouched.

Credit to the downstream report ai-ecoverse/slicc#2978 for the reproduction.

Review follow-up (3d04a6f)

• Subshell isolation. runCommandSubstitution now uses the existing beginIsolatedShellState transaction instead of hand-rolling a partial rollback, so functions, set -u, shopt, variable attributes and previousDir set inside $() are discarded like bash discards a subshell's. The partial rollback was inherited verbatim from the old expansion.ts and leaked identically on plain $() on main; routing arithmetic substitutions onto that path is what made it reachable from $(( )), so this closes the pre-existing leak too.
• Data-to-code path. The text scan for $( in substitution output was bypassable with one level of variable indirection (v='$(cmd)'; $(( $(echo v) )) executed cmd). Replaced with provenance tracking on the resolution context: anything parsed out of data — a variable's value, an array element's value, a substitution's output — evaluates with command substitution disabled, at any indirection depth. Array subscripts are the deliberate exception, because bash does expand a[$(cmd)] reached through data; spec-tests bugs.test.sh and array-assign.test.sh both pin that, and treating it as an error broke them.

Isolation and provenance coverage lives in arithmetic-command-substitution-isolation.comparison.test.ts (9 cases recorded against real bash) plus 3 unit tests. Both commits are GPG-signed and verified.

🤖 Generated with Claude Code

[vercel]

@claude is attempting to deploy a commit to the Vercel Labs Team on Vercel.

A member of the Team first needs to authorize it.

[auto-maintain]

🤖 auto-maintain review

Automated, advisory triage for @trieloff's PR. Facts below are read from the GitHub API.

Check	Result
Author's merged PRs (this repo)	18
Account established	✅ (age 6482d · 135 followers · 203 public repos)
Commits signed/verified	✅ 3/3
Changeset included	✅ (.changeset/arith-command-substitution-state.md)

Review panel: 🔴 high highest severity

just-bash maintainer code review: 🔴 high

Command substitution state handling improved, but a concatenation path still permits command execution from arithmetic data.

• packages/just-bash/src/interpreter/arithmetic.ts:1039 — The data-to-code guard is bypassed for concatenated arithmetic values. For example, `a='1$(echo PWNED >&2; echo 2)'; echo $((a))` parses as `ArithConcat`; despite `fromData`, this branch executes the substitution directly. Bash instead raises a syntax error, so attacker-controlled variable data can still execute commands. Apply the provenance guard here too.

General code review: 🔴 high

The change leaves a command-injection bypass and regresses inherited file-descriptor offsets.

• packages/just-bash/src/interpreter/arithmetic.ts:1039 — The `fromData` protection is bypassed for concatenations: `evalConcatPartToStringAsync` executes `ArithCommandSubst` unconditionally. For example, `a='x$(echo PWNED >&2)'; echo $((a))` still executes variable data as shell code.
• packages/just-bash/src/interpreter/expansion/command-substitution.ts:108 — Isolating the descriptor table discards reads from inherited file descriptors. Unlike the subshell implementation, this path does not propagate consumed offsets before restoring state, so a read after `x=$(read -u 3 ...)` incorrectly rereads the same input instead of advancing to the next record.

Adversarial security: 🔴 high

The data-to-code fix remains bypassable and exposes live shell state to injected commands.

• packages/just-bash/src/interpreter/arithmetic.ts:1039 — The provenance guard is bypassed for concatenations: `payload='$(steal "$SECRET")$x'; echo $((payload))` parses as `ArithConcat`, and this branch executes the embedded command without checking `resolution.fromData`. Attacker-controlled arithmetic data can therefore execute commands in the live shell and access non-exported state.

Adversarial security (second opinion): 🟡 medium

The state-sharing fix and subshell isolation refactor look correct and well covered, but the new data-provenance guard against command execution from arithmetic data is incomplete — the ArithConcat path still executes `$(...)` reached through variable values and substitution output, diverging from bash and defeating the hardening the PR advertises.

• packages/just-bash/src/interpreter/arithmetic.ts:1039 — The new `fromData` provenance guard is bypassed by the concatenation path: `evalConcatPartToStringAsync` handles `ArithCommandSubst` by calling `runCommandSubstitutionText` unconditionally, ignoring `resolution.fromData`. Any data-sourced arithmetic text that parses as an `ArithConcat` therefore still executes commands — e.g. `v='x$(id)'; echo $((v))` (via `resolveArithVariable` → `evaluateAsData` → `ArithConcat`) and `echo $(( $(echo 'x$(id)') ))` (via `evaluateSubstitutionOutput`) both run `id`, now against the live shell state, whereas GNU bash raises `syntax error: operand expected`. This is the exact data-to-code path commit 3d04a6f claims to close, and the case the removed text scan for `$(` in substitution output did catch, so the hardening is a net regression for `$( )`-in-output while the guarded bare-operand form (`v='$(id)'`) is the only variant covered by the new tests/fixtures.

Standard Bash and host portability: 🟡 medium

Arithmetic substitutions now inherit errexit contrary to default Bash behavior.

• packages/just-bash/src/interpreter/expansion/command-substitution.ts:108 — Command substitutions retain `options.errexit`, but standard Bash clears `-e` unless `inherit_errexit` or POSIX mode is enabled. After this PR, `set -e; echo $(( $(false; echo 5) ))` aborts instead of printing 5.

_{Posted by auto-maintain. This automated code review is advisory; a human maintainer makes the call.}

[trieloff]

Thanks — both findings were real and are fixed in 3d04a6f. One correction on the second one, where the suggested rule turned out to be too strict.

1. Incomplete subshell isolation — fixed

Confirmed, and worse than the summary suggested: functions, set -u, shopt, variable attributes and previousDir all leaked. Reproduced:

echo $(( $(f() { :; }; set -u; cd /tmp; echo 1) ))
type f            # -> "f is a function"   bash: not found
echo $UNSET_VAR   # -> unbound variable    bash: empty, status 0

Now uses the existing beginIsolatedShellState transaction, which snapshots every mutable namespace. All of the above match bash exactly.

Worth noting for the record: this leak is not new to this PR — the extracted block is verbatim from the old expansion.ts, and plain $(...) leaked identically on main (verified against the base commit). What the PR did was route arithmetic substitutions onto that path, where the old detached execution had been isolated. So the finding is fair, and the fix also closes the pre-existing leak on plain $().

2. Guard bypassable via variable indirection — fixed, with a caveat

Confirmed:

v='$(touch /tmp/pwn)'; echo $(( $(echo v) ))   # ran touch

The text scan for $( was the wrong mechanism. Replaced with provenance tracking on the resolution context: expressions parsed out of data (a variable's value, an array element's value, a substitution's output) evaluate with command substitution disabled, so the guard holds through any depth of indirection. Error text now matches bash character-for-character:

bash: $(echo 1): syntax error: operand expected (error token is "$(echo 1)")

The caveat: "reject ArithCommandSubst originating from re-parsed data" as stated is too strict, and implementing it literally broke two spec tests. bash distinguishes operand position from array subscripts:

v='$(echo 1)';            echo $(( v ))    # syntax error, nothing runs
x='a[$(echo 42)]=1';      echo $(( x ))    # 1 — bash RUNS the substitution

Verified on bash 3.2 and 5.3; spec-tests/bash/cases/bugs.test.sh ("command execution $(echo 42 | tee PWNED) not allowed") pins the second as ## BUG bash/mksh/zsh status: 0, and array-assign.test.sh L121 pins the ${b[expr]} read shape. So subscript evaluation clears the flag, and both spec tests pass again. Full spec suite is green.

For a defensive-sandbox posture that's arguably still one execution path too many, but it is bash's documented behaviour and the spec suite asserts it — happy to gate it behind an option if you'd prefer to diverge deliberately.

Also fixed

The Commits signed/verified 0/1 line was correct — the commits were unsigned. Both are now signed and verified=true.

Coverage added

9 comparison cases in a new arithmetic-command-substitution-isolation.comparison.test.ts (recorded against real bash) covering each leak class and each provenance case, plus 3 unit tests asserting full stdout/stderr. pnpm test:run: 15655 passed; the only failures are the pre-existing CPython-WASM tests that also fail on main on this machine.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[auto-maintain]

⚠️ auto-maintain: review severity raised to 🔴 high

A new push changed this PR and the review now contains a higher-severity finding. See the updated review comment above.