fix(shell): actually pass spawn its arguments

packages/typescript/src/api/process.ts

@@ -217,9 +217,15 @@ export interface SpawnOptions {
   cwd?: string
   env?: Record<string, string>
   /**
-   * Run the command through a shell. The Craft bridge translates this
-   * automatically (cmd.exe on Windows, /bin/sh elsewhere). On the Node
-   * fallback the command is rewritten to `cmd.exe /c <cmd>` on win32.
+   * Run `command` as a command line through a shell (cmd.exe on Windows,
+   * /bin/sh elsewhere) instead of executing it directly.
+   *
+   * Off by default, as in Node. Without it `command` is a program and `args`
+   * are its arguments, so a `;` or `|` inside an argument reaches the program
+   * as a plain character rather than as shell syntax.
+   *
+   * A shell takes one string, so `args` cannot be passed with it — supplying
+   * both is refused rather than silently honouring one of them.
    */
   shell?: boolean
 }

packages/zig/build.zig

@@ -732,6 +732,18 @@ pub fn build(b: *std.Build) void {
         }),
     });
 
+    // The shell bridge's argv construction: what a `spawn` actually runs.
+    // Untested until now, which is how it shipped parsing an `args` array and
+    // throwing it away.
+    const bridge_shell_tests = b.addTest(.{
+        .root_module = b.createModule(.{
+            .root_source_file = b.path("src/bridge_shell.zig"),
+            .target = target,
+            .optimize = optimize,
+        }),
+    });
+    bridge_shell_tests.root_module.link_libc = true;
+
     // The conformance test: what craft declares, against what it dispatches.
     const capability_conformance_tests = b.addTest(.{
         .root_module = b.createModule(.{
@@ -1103,6 +1115,7 @@ pub fn build(b: *std.Build) void {
     const run_local_tls_tests = b.addRunArtifact(local_tls_tests);
     const run_menu_roles_tests = b.addRunArtifact(menu_roles_tests);
     const run_capabilities_tests = b.addRunArtifact(capabilities_tests);
+    const run_bridge_shell_tests = b.addRunArtifact(bridge_shell_tests);
     const run_window_lifecycle_tests = b.addRunArtifact(window_lifecycle_tests);
     const run_window_registry_tests = b.addRunArtifact(window_registry_tests);
     const run_external_link_tests = b.addRunArtifact(external_link_tests);
@@ -1224,6 +1237,7 @@ pub fn build(b: *std.Build) void {
     test_step.dependOn(&run_local_tls_tests.step);
     test_step.dependOn(&run_menu_roles_tests.step);
     test_step.dependOn(&run_capabilities_tests.step);
+    test_step.dependOn(&run_bridge_shell_tests.step);
     test_step.dependOn(&run_window_lifecycle_tests.step);
     test_step.dependOn(&run_window_registry_tests.step);
     test_step.dependOn(&run_external_link_tests.step);

packages/zig/src/bridge_shell.zig

@@ -111,6 +111,13 @@ pub const ShellBridge = struct {
     }
 
     /// Shell argument tuple for cross-platform support.
+    /// Most arguments a page may hand to one `spawn`.
+    ///
+    /// Generous for any real command and bounded because the array comes from
+    /// the page: without a limit a single message can ask craft to build an
+    /// argv as large as the JSON it is willing to parse.
+    const max_spawn_args = 256;
+
     const ShellArgs = struct { argv: [3][]const u8 };
 
     /// Return the platform-appropriate shell invocation for a command string.
@@ -121,6 +128,48 @@ pub const ShellBridge = struct {
         };
     }
 
+    /// The argv a spawn should run, and the only place that decision is made.
+    ///
+    /// Pure and separated from `spawn` so it can be tested without starting a
+    /// process: the interesting behaviour is entirely in what ends up in the
+    /// array, and nothing about that needs a running child to check.
+    fn buildSpawnArgv(
+        allocator: std.mem.Allocator,
+        command: []const u8,
+        args: []const []const u8,
+        use_shell: bool,
+    ) BridgeError!std.ArrayListUnmanaged([]const u8) {
+        var argv: std.ArrayListUnmanaged([]const u8) = .empty;
+        errdefer argv.deinit(allocator);
+
+        if (use_shell) {
+            // A command line run by the platform shell. Arguments have no
+            // meaning here — `sh -c` takes one string — and silently ignoring
+            // them is the exact bug this change exists to fix, so refuse
+            // rather than half-honour them.
+            if (args.len > 0) {
+                std.log.warn("bridge_shell: spawn with shell:true cannot take args; pass one command string", .{});
+                return BridgeError.InvalidParameter;
+            }
+            try validateCommand(command);
+            const shell = getShellArgs(command);
+            argv.appendSlice(allocator, &shell.argv) catch return BridgeError.AllocationFailed;
+            return argv;
+        }
+
+        // Executed directly: argv[0] is the program, the rest are its
+        // arguments, and no shell ever sees them.
+        //
+        // Deliberately not `validateCommand`. That blocklist exists to stop
+        // `sh -c` interpreting metacharacters; with no shell in the picture a
+        // `;` in an argument is a semicolon the program receives, and
+        // rejecting it would refuse ordinary commands to defend against
+        // nothing.
+        argv.append(allocator, command) catch return BridgeError.AllocationFailed;
+        argv.appendSlice(allocator, args) catch return BridgeError.AllocationFailed;
+        return argv;
+    }
+
     /// Validate a command string for dangerous shell metacharacters.
     /// Returns error.UnsafeCommand if the command contains injection patterns.
     ///
@@ -260,13 +309,44 @@ pub const ShellBridge = struct {
         self.sendShellResult(callback_id, "exec", exit_code, stdout, stderr);
     }
 
-    /// Spawn a process without waiting
-    /// JSON: {"id": "proc1", "command": "long-running-cmd", "cwd": "/path"}
+    /// Spawn a process without waiting.
+    ///
+    /// JSON: {"id", "command", "args": [..], "cwd", "shell": false}
+    ///
+    /// `command` is an executable and `args` are its arguments — the shape both
+    /// `craft.shell.spawn(id, cmd, args)` and the SDK's
+    /// `spawn(command, args?, options?)` have always advertised. The arguments
+    /// were parsed and thrown away: `SpawnParams` did not declare `args`, and
+    /// `ignore_unknown_fields` dropped it without a word, so
+    /// `craft.shell.spawn('p', 'git', ['status'])` ran `git` bare and reported
+    /// success.
+    ///
+    /// ## Why this no longer goes through a shell
+    ///
+    /// It used to run `/bin/sh -c <command>`, which is why the arguments had
+    /// nowhere to go — a shell takes one string, not an argv. Appending them to
+    /// that string would have meant quoting every argument correctly forever,
+    /// on two platforms, against page-supplied input, to avoid handing a web
+    /// page a shell. Executing the program directly removes the shell, and with
+    /// it the entire class: `;`, `|` and `$(...)` inside an argument are now
+    /// just bytes the program receives.
+    ///
+    /// `shell: true` asks for the old behaviour explicitly, for a caller who
+    /// genuinely wants a command line. That path keeps the metacharacter
+    /// blocklist, because that is the path that needs it.
+    ///
+    /// This changes what `spawn('p', 'git status')` does: it used to work,
+    /// because `sh` split the string, and now looks for a program named
+    /// "git status". That is the same break Node made deliberate — the fix is
+    /// `spawn('p', 'git', ['status'])`, which is what the signature asked for
+    /// all along.
     fn spawn(self: *Self, data: []const u8) !void {
         const SpawnParams = struct {
             id: []const u8 = "",
             command: []const u8 = "",
             cwd: []const u8 = "",
+            args: []const []const u8 = &.{},
+            shell: bool = false,
         };
 
         const parsed = std.json.parseFromSlice(SpawnParams, self.allocator, data, .{
@@ -282,18 +362,23 @@ pub const ShellBridge = struct {
 
         if (id.len == 0 or command.len == 0) return BridgeError.MissingData;
 
-        try validateCommand(command);
+        // Bounded so a page cannot ask craft to build an unbounded argv.
+        if (params.args.len > max_spawn_args) {
+            std.log.warn("bridge_shell: spawn refused, {d} arguments exceeds {d}", .{ params.args.len, max_spawn_args });
+            return BridgeError.InvalidParameter;
+        }
+
+        var argv = try buildSpawnArgv(self.allocator, command, params.args, params.shell);
+        defer argv.deinit(self.allocator);
 
         if (comptime builtin.mode == .debug)
-            std.debug.print("[ShellBridge] spawn: {s} -> {s}\n", .{ id, command });
+            std.debug.print("[ShellBridge] spawn: {s} -> {s} ({d} args, shell={})\n", .{ id, command, params.args.len, params.shell });
 
-        // Build argv array using cross-platform shell args
-        const shell = getShellArgs(command);
         const io = io_context.get();
 
         // Create child process using Zig 0.16 API
         const child = std.process.spawn(io, .{
-            .argv = &shell.argv,
+            .argv = argv.items,
             .cwd = if (cwd.len > 0) .{ .path = cwd } else .inherit,
             .stdout = .inherit,
             .stderr = .inherit,
@@ -798,3 +883,158 @@ pub fn getGlobalShellBridge() ?*ShellBridge {
 pub fn setGlobalShellBridge(bridge: *ShellBridge) void {
     global_state.instance.setShellBridge(bridge);
 }
+
+// =============================================================================
+// Tests
+// =============================================================================
+//
+// `spawn` had no tests, which is how it shipped parsing an `args` array and
+// throwing it away. The decision worth testing is what ends up in the argv, and
+// none of that needs a child process.
+
+const testing = std.testing;
+
+fn argvFor(command: []const u8, args: []const []const u8, use_shell: bool) !std.ArrayListUnmanaged([]const u8) {
+    return ShellBridge.buildSpawnArgv(testing.allocator, command, args, use_shell);
+}
+
+test "arguments reach the program instead of being dropped" {
+    // The bug: `craft.shell.spawn('p', 'git', ['status'])` ran `git` bare and
+    // reported success, because SpawnParams never declared `args`.
+    var argv = try argvFor("git", &.{ "status", "--porcelain" }, false);
+    defer argv.deinit(testing.allocator);
+
+    try testing.expectEqual(@as(usize, 3), argv.items.len);
+    try testing.expectEqualStrings("git", argv.items[0]);
+    try testing.expectEqualStrings("status", argv.items[1]);
+    try testing.expectEqualStrings("--porcelain", argv.items[2]);
+}
+
+test "no shell is involved, so no shell metacharacters are interpreted" {
+    // These would each have been an injection through `sh -c` had the
+    // arguments been appended to a command string. Executed directly they are
+    // bytes the program receives, so they are passed through untouched rather
+    // than rejected.
+    const nasty = [_][]const u8{ "; rm -rf /", "$(whoami)", "a|b", "x>y", "`id`", "&& curl evil" };
+    var argv = try argvFor("echo", &nasty, false);
+    defer argv.deinit(testing.allocator);
+
+    try testing.expectEqual(nasty.len + 1, argv.items.len);
+    for (nasty, 0..) |expected, i| {
+        try testing.expectEqualStrings(expected, argv.items[i + 1]);
+    }
+    // And nothing that could run them is in the argv.
+    try testing.expectEqualStrings("echo", argv.items[0]);
+    for (argv.items) |a| {
+        try testing.expect(!std.mem.eql(u8, a, "/bin/sh"));
+        try testing.expect(!std.mem.eql(u8, a, "cmd.exe"));
+        try testing.expect(!std.mem.eql(u8, a, "-c"));
+    }
+}
+
+test "a program with no arguments is just the program" {
+    var argv = try argvFor("/usr/bin/true", &.{}, false);
+    defer argv.deinit(testing.allocator);
+    try testing.expectEqual(@as(usize, 1), argv.items.len);
+    try testing.expectEqualStrings("/usr/bin/true", argv.items[0]);
+}
+
+test "shell:true still runs a command line through the shell" {
+    var argv = try argvFor("echo hello", &.{}, true);
+    defer argv.deinit(testing.allocator);
+
+    try testing.expectEqual(@as(usize, 3), argv.items.len);
+    const shell_path = if (builtin.os.tag == .windows) "cmd.exe" else "/bin/sh";
+    const shell_flag = if (builtin.os.tag == .windows) "/c" else "-c";
+    try testing.expectEqualStrings(shell_path, argv.items[0]);
+    try testing.expectEqualStrings(shell_flag, argv.items[1]);
+    try testing.expectEqualStrings("echo hello", argv.items[2]);
+}
+
+test "shell:true keeps the blocklist that path needs" {
+    // The metacharacter check is not dropped, it is moved to the only place it
+    // defends anything: the path that hands a string to a shell.
+    try testing.expectError(BridgeError.UnsafeCommand, argvFor("ls; rm -rf /", &.{}, true));
+    try testing.expectError(BridgeError.UnsafeCommand, argvFor("echo $(whoami)", &.{}, true));
+    try testing.expectError(BridgeError.UnsafeCommand, argvFor("a | b", &.{}, true));
+}
+
+test "asking for a shell and arguments at once is refused, not half-honoured" {
+    // `sh -c` takes one string. Quietly ignoring the arguments is exactly the
+    // failure this change exists to remove, so it must not be reintroduced by
+    // the compatibility path.
+    try testing.expectError(
+        BridgeError.InvalidParameter,
+        argvFor("git", &.{"status"}, true),
+    );
+}
+
+test "the argv is built in order, and order is the whole meaning" {
+    var argv = try argvFor("cmd", &.{ "1", "2", "3", "4", "5" }, false);
+    defer argv.deinit(testing.allocator);
+    const expected = [_][]const u8{ "cmd", "1", "2", "3", "4", "5" };
+    try testing.expectEqual(expected.len, argv.items.len);
+    for (expected, 0..) |e, i| try testing.expectEqualStrings(e, argv.items[i]);
+}
+
+test "an empty argument is still an argument" {
+    // `find . -name ""` is meaningful; dropping empty strings would silently
+    // change the command.
+    var argv = try argvFor("find", &.{ ".", "-name", "" }, false);
+    defer argv.deinit(testing.allocator);
+    try testing.expectEqual(@as(usize, 4), argv.items.len);
+    try testing.expectEqualStrings("", argv.items[3]);
+}
+
+test "the JSON the page sends decodes into the arguments the argv is built from" {
+    // The seam the bug lived in: the payload was parsed correctly and the
+    // struct simply had nowhere to put `args`, so `ignore_unknown_fields`
+    // dropped them without a word.
+    const Params = struct {
+        id: []const u8 = "",
+        command: []const u8 = "",
+        cwd: []const u8 = "",
+        args: []const []const u8 = &.{},
+        shell: bool = false,
+    };
+    const payload =
+        \\{"id":"p1","command":"git","args":["status","--porcelain"],"cwd":"/tmp"}
+    ;
+    const parsed = try std.json.parseFromSlice(Params, testing.allocator, payload, .{
+        .ignore_unknown_fields = true,
+        .allocate = .alloc_always,
+    });
+    defer parsed.deinit();
+
+    try testing.expectEqual(@as(usize, 2), parsed.value.args.len);
+    try testing.expectEqualStrings("status", parsed.value.args[0]);
+    try testing.expect(!parsed.value.shell);
+
+    var argv = try argvFor(parsed.value.command, parsed.value.args, parsed.value.shell);
+    defer argv.deinit(testing.allocator);
+    try testing.expectEqual(@as(usize, 3), argv.items.len);
+    try testing.expectEqualStrings("--porcelain", argv.items[2]);
+}
+
+test "a payload from before this change still spawns the same program" {
+    // No `args` key at all: an older page, or any caller that never used them.
+    const Params = struct {
+        id: []const u8 = "",
+        command: []const u8 = "",
+        args: []const []const u8 = &.{},
+        shell: bool = false,
+    };
+    const payload =
+        \\{"id":"p1","command":"/usr/bin/true"}
+    ;
+    const parsed = try std.json.parseFromSlice(Params, testing.allocator, payload, .{
+        .ignore_unknown_fields = true,
+        .allocate = .alloc_always,
+    });
+    defer parsed.deinit();
+
+    var argv = try argvFor(parsed.value.command, parsed.value.args, parsed.value.shell);
+    defer argv.deinit(testing.allocator);
+    try testing.expectEqual(@as(usize, 1), argv.items.len);
+    try testing.expectEqualStrings("/usr/bin/true", argv.items[0]);
+}

packages/zig/src/js/craft-bridge.js

@@ -633,9 +633,22 @@
     openExternal: function (url)              { return _send('shell', 'openUrl', _stringify({ url: String(url) })) },
     openPath:     function (path)             { return _send('shell', 'openPath', _stringify({ path: String(path) })) },
     showInFinder: function (path)             { return _send('shell', 'showInFinder', _stringify({ path: String(path) })) },
+    // spawn(id, command, args, opts)
+    //
+    // `command` is a program and `args` are its arguments — no shell is
+    // involved, so a `;` or `|` inside an argument reaches the program as a
+    // plain character rather than as syntax. Native used to run
+    // `sh -c command` and drop `args` on the floor entirely, so
+    // `spawn('p', 'git', ['status'])` ran `git` bare and reported success.
+    //
+    // `opts.shell = true` runs `command` as a command line through the
+    // platform shell instead. Arguments cannot be passed that way — a shell
+    // takes one string — so passing both is refused rather than half-honoured.
     spawn:        function (id, cmd, args, opts) {
       return _send('shell', 'spawn', _stringify(Object.assign({
-        id: String(id), command: String(cmd), args: args || [],
+        id: String(id),
+        command: String(cmd),
+        args: (args || []).map(String),
       }, opts || {})))
     },
     kill:         function (id)               { return _send('shell', 'kill', _stringify({ id: String(id) })) },