ReviewOS

also looking at this

craft-native/craft

fix(shell): actually pass spawn its arguments

#77
Merged glennmichael123 wants to merge fix/shell-spawn-args into main
4 files +284 -11
11 days ago

Closes #69.

craft.shell.spawn(id, cmd, args) has advertised an arguments array since it was written. The SDK's spawn(command, args?, options?) says the same. Native declared no args field, so ignore_unknown_fields parsed the array and dropped it without a word:

craft.shell.spawn('p', 'git', ['status'])   // ran `git` bare, reported success

Why the arguments had nowhere to go

spawn ran /bin/sh -c <command>. A shell takes one string, not an argv — so there was no slot to put them in.

The issue offers "append them to the argv", but the argv was {"/bin/sh", "-c", command}. Appending to that means appending to the command string, which means quoting every argument correctly, forever, on two platforms, against input supplied by a web page — in order to avoid handing that page a shell.

So the default stops using one:

beforeafter
what runs/bin/sh -c "git"execve("git", ["git", "status"])
argsparsed, discardedpassed
; in an argumentshell syntaxa semicolon the program receives
injection surfacea blocklist between a page and sh -cnone — no shell exists

shell: true — and it was already documented

The SDK's SpawnOptions has declared shell?: boolean all along, saying "The Craft bridge translates this automatically". It was as dead as args — native had no field for it either. It's real now.

That path keeps the metacharacter blocklist, because it's the only path that needs one. Passing args alongside it is refused, not half-honoured — silently ignoring arguments is the exact bug being fixed, and the compatibility path must not reintroduce it.

The compatibility break, stated plainly

spawn('p', 'git status') used to work, because sh split the string. It now looks for a program named "git status".

That is the same break Node made deliberately, and the fix is spawn('p', 'git', ['status']) — what the signature asked for in the first place. Worth noting how little the shell was contributing: validateCommand already rejected ;, |, &, >, <, \n, $( and backticks, so it provided word-splitting and essentially nothing else. Callers who need more can pass shell: true.

Tests

bridge_shell.zig had no tests and no test artifact — which is how it shipped parsing an array and throwing it away. The argv decision is now a pure function with 10 tests, wired into zig build test.

Controlled: reintroducing the original behaviour — parsing the arguments and dropping them, compiling cleanly — fails 7 tests by name:

error: 'bridge_shell.test.arguments reach the program instead of being dropped' failed
error: 'bridge_shell.test.no shell is involved, so no shell metacharacters are interpreted' failed
error: 'bridge_shell.test.a program with no arguments is just the program' failed
...

(My first attempt at that control failed to compile rather than failing an assertion, which proves nothing about the test. Redone so the broken version builds.)

Also covered: an older payload with no args key still spawns the same program, empty-string arguments survive, argument order is pinned, and the JSON→argv seam is tested end to end with the struct the bridge actually decodes with.

Noted, not fixed

The SDK's ChildProcess.start() posts to process.spawn, and process is not a routed bridge type anywhere in the dispatcher — that path answers nothing at all. Separate from this issue; flagging rather than widening scope into it.

Verified

zig build · zig build test · zig build test-js · x86_64-windows cross-build · zig fmt --check · tsc --noEmit · 511 bun tests · pickier 38 warnings, unchanged from main.

4 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.