fix(shell): actually pass spawn its arguments

packages/zig/src/bridge_shell.zigmodified+247-7

Changes to packages/zig/src/bridge_shell.zig

		⋯110 linesShow the 110 lines above this hunk@@ -111,6 +111,13 @@ pub const ShellBridge = struct {
111	111	}
112	112
113	113	/// Shell argument tuple for cross-platform support.
	114	+ /// Most arguments a page may hand to one `spawn`.
	115	+ ///
	116	+ /// Generous for any real command and bounded because the array comes from
	117	+ /// the page: without a limit a single message can ask craft to build an
	118	+ /// argv as large as the JSON it is willing to parse.
	119	+ const max_spawn_args = 256;
	120	+
114	121	const ShellArgs = struct { argv: [3][]const u8 };
115	122
116	123	/// Return the platform-appropriate shell invocation for a command string.
		⋯4 linesShow the 4 lines above this hunk@@ -121,6 +128,48 @@ pub const ShellBridge = struct {
121	128	};
122	129	}
123	130
	131	+ /// The argv a spawn should run, and the only place that decision is made.
	132	+ ///
	133	+ /// Pure and separated from `spawn` so it can be tested without starting a
	134	+ /// process: the interesting behaviour is entirely in what ends up in the
	135	+ /// array, and nothing about that needs a running child to check.
	136	+ fn buildSpawnArgv(
	137	+ allocator: std.mem.Allocator,
	138	+ command: []const u8,
	139	+ args: []const []const u8,
	140	+ use_shell: bool,
	141	+ ) BridgeError!std.ArrayListUnmanaged([]const u8) {
	142	+ var argv: std.ArrayListUnmanaged([]const u8) = .empty;
	143	+ errdefer argv.deinit(allocator);
	144	+
	145	+ if (use_shell) {
	146	+ // A command line run by the platform shell. Arguments have no
	147	+ // meaning here — `sh -c` takes one string — and silently ignoring
	148	+ // them is the exact bug this change exists to fix, so refuse
	149	+ // rather than half-honour them.
	150	+ if (args.len > 0) {
	151	+ std.log.warn("bridge_shell: spawn with shell:true cannot take args; pass one command string", .{});
	152	+ return BridgeError.InvalidParameter;
	153	+ }
	154	+ try validateCommand(command);
	155	+ const shell = getShellArgs(command);
	156	+ argv.appendSlice(allocator, &shell.argv) catch return BridgeError.AllocationFailed;
	157	+ return argv;
	158	+ }
	159	+
	160	+ // Executed directly: argv[0] is the program, the rest are its
	161	+ // arguments, and no shell ever sees them.
	162	+ //
	163	+ // Deliberately not `validateCommand`. That blocklist exists to stop
	164	+ // `sh -c` interpreting metacharacters; with no shell in the picture a
	165	+ // `;` in an argument is a semicolon the program receives, and
	166	+ // rejecting it would refuse ordinary commands to defend against
	167	+ // nothing.
	168	+ argv.append(allocator, command) catch return BridgeError.AllocationFailed;
	169	+ argv.appendSlice(allocator, args) catch return BridgeError.AllocationFailed;
	170	+ return argv;
	171	+ }
	172	+
124	173	/// Validate a command string for dangerous shell metacharacters.
125	174	/// Returns error.UnsafeCommand if the command contains injection patterns.
126	175	///
		⋯133 linesShow the 133 lines above this hunk@@ -260,13 +309,44 @@ pub const ShellBridge = struct {
260	309	self.sendShellResult(callback_id, "exec", exit_code, stdout, stderr);
261	310	}
262	311
263		- /// Spawn a process without waiting
264		- /// JSON: {"id": "proc1", "command": "long-running-cmd", "cwd": "/path"}
	312	+ /// Spawn a process without waiting.
	313	+ ///
	314	+ /// JSON: {"id", "command", "args": [..], "cwd", "shell": false}
	315	+ ///
	316	+ /// `command` is an executable and `args` are its arguments — the shape both
	317	+ /// `craft.shell.spawn(id, cmd, args)` and the SDK's
	318	+ /// `spawn(command, args?, options?)` have always advertised. The arguments
	319	+ /// were parsed and thrown away: `SpawnParams` did not declare `args`, and
	320	+ /// `ignore_unknown_fields` dropped it without a word, so
	321	+ /// `craft.shell.spawn('p', 'git', ['status'])` ran `git` bare and reported
	322	+ /// success.
	323	+ ///
	324	+ /// ## Why this no longer goes through a shell
	325	+ ///
	326	+ /// It used to run `/bin/sh -c <command>`, which is why the arguments had
	327	+ /// nowhere to go — a shell takes one string, not an argv. Appending them to
	328	+ /// that string would have meant quoting every argument correctly forever,
	329	+ /// on two platforms, against page-supplied input, to avoid handing a web
	330	+ /// page a shell. Executing the program directly removes the shell, and with
	331	+ /// it the entire class: `;`, `|` and `$(...)` inside an argument are now
	332	+ /// just bytes the program receives.
	333	+ ///
	334	+ /// `shell: true` asks for the old behaviour explicitly, for a caller who
	335	+ /// genuinely wants a command line. That path keeps the metacharacter
	336	+ /// blocklist, because that is the path that needs it.
	337	+ ///
	338	+ /// This changes what `spawn('p', 'git status')` does: it used to work,
	339	+ /// because `sh` split the string, and now looks for a program named
	340	+ /// "git status". That is the same break Node made deliberate — the fix is
	341	+ /// `spawn('p', 'git', ['status'])`, which is what the signature asked for
	342	+ /// all along.
265	343	fn spawn(self: *Self, data: []const u8) !void {
266	344	const SpawnParams = struct {
267	345	id: []const u8 = "",
268	346	command: []const u8 = "",
269	347	cwd: []const u8 = "",
	348	+ args: []const []const u8 = &.{},
	349	+ shell: bool = false,
270	350	};
271	351
272	352	const parsed = std.json.parseFromSlice(SpawnParams, self.allocator, data, .{
		⋯9 linesShow the 9 lines above this hunk@@ -282,18 +362,23 @@ pub const ShellBridge = struct {
282	362
283	363	if (id.len == 0 or command.len == 0) return BridgeError.MissingData;
284	364
285		- try validateCommand(command);
	365	+ // Bounded so a page cannot ask craft to build an unbounded argv.
	366	+ if (params.args.len > max_spawn_args) {
	367	+ std.log.warn("bridge_shell: spawn refused, {d} arguments exceeds {d}", .{ params.args.len, max_spawn_args });
	368	+ return BridgeError.InvalidParameter;
	369	+ }
	370	+
	371	+ var argv = try buildSpawnArgv(self.allocator, command, params.args, params.shell);
	372	+ defer argv.deinit(self.allocator);
286	373
287	374	if (comptime builtin.mode == .debug)
288		- std.debug.print("[ShellBridge] spawn: {s} -> {s}\n", .{ id, command });
	375	+ std.debug.print("[ShellBridge] spawn: {s} -> {s} ({d} args, shell={})\n", .{ id, command, params.args.len, params.shell });
289	376
290		- // Build argv array using cross-platform shell args
291		- const shell = getShellArgs(command);
292	377	const io = io_context.get();
293	378
294	379	// Create child process using Zig 0.16 API
295	380	const child = std.process.spawn(io, .{
296		- .argv = &shell.argv,
	381	+ .argv = argv.items,
297	382	.cwd = if (cwd.len > 0) .{ .path = cwd } else .inherit,
298	383	.stdout = .inherit,
299	384	.stderr = .inherit,
		⋯498 linesShow the 498 lines above this hunk@@ -798,3 +883,158 @@ pub fn getGlobalShellBridge() ?*ShellBridge {
798	883	pub fn setGlobalShellBridge(bridge: *ShellBridge) void {
799	884	global_state.instance.setShellBridge(bridge);
800	885	}
	886	+
	887	+// =============================================================================
	888	+// Tests
	889	+// =============================================================================
	890	+//
	891	+// `spawn` had no tests, which is how it shipped parsing an `args` array and
	892	+// throwing it away. The decision worth testing is what ends up in the argv, and
	893	+// none of that needs a child process.
	894	+
	895	+const testing = std.testing;
	896	+
	897	+fn argvFor(command: []const u8, args: []const []const u8, use_shell: bool) !std.ArrayListUnmanaged([]const u8) {
	898	+ return ShellBridge.buildSpawnArgv(testing.allocator, command, args, use_shell);
	899	+}
	900	+
	901	+test "arguments reach the program instead of being dropped" {
	902	+ // The bug: `craft.shell.spawn('p', 'git', ['status'])` ran `git` bare and
	903	+ // reported success, because SpawnParams never declared `args`.
	904	+ var argv = try argvFor("git", &.{ "status", "--porcelain" }, false);
	905	+ defer argv.deinit(testing.allocator);
	906	+
	907	+ try testing.expectEqual(@as(usize, 3), argv.items.len);
	908	+ try testing.expectEqualStrings("git", argv.items[0]);
	909	+ try testing.expectEqualStrings("status", argv.items[1]);
	910	+ try testing.expectEqualStrings("--porcelain", argv.items[2]);
	911	+}
	912	+
	913	+test "no shell is involved, so no shell metacharacters are interpreted" {
	914	+ // These would each have been an injection through `sh -c` had the
	915	+ // arguments been appended to a command string. Executed directly they are
	916	+ // bytes the program receives, so they are passed through untouched rather
	917	+ // than rejected.
	918	+ const nasty = [_][]const u8{ "; rm -rf /", "$(whoami)", "a|b", "x>y", "`id`", "&& curl evil" };
	919	+ var argv = try argvFor("echo", &nasty, false);
	920	+ defer argv.deinit(testing.allocator);
	921	+
	922	+ try testing.expectEqual(nasty.len + 1, argv.items.len);
	923	+ for (nasty, 0..) |expected, i| {
	924	+ try testing.expectEqualStrings(expected, argv.items[i + 1]);
	925	+ }
	926	+ // And nothing that could run them is in the argv.
	927	+ try testing.expectEqualStrings("echo", argv.items[0]);
	928	+ for (argv.items) |a| {
	929	+ try testing.expect(!std.mem.eql(u8, a, "/bin/sh"));
	930	+ try testing.expect(!std.mem.eql(u8, a, "cmd.exe"));
	931	+ try testing.expect(!std.mem.eql(u8, a, "-c"));
	932	+ }
	933	+}
	934	+
	935	+test "a program with no arguments is just the program" {
	936	+ var argv = try argvFor("/usr/bin/true", &.{}, false);
	937	+ defer argv.deinit(testing.allocator);
	938	+ try testing.expectEqual(@as(usize, 1), argv.items.len);
	939	+ try testing.expectEqualStrings("/usr/bin/true", argv.items[0]);
	940	+}
	941	+
	942	+test "shell:true still runs a command line through the shell" {
	943	+ var argv = try argvFor("echo hello", &.{}, true);
	944	+ defer argv.deinit(testing.allocator);
	945	+
	946	+ try testing.expectEqual(@as(usize, 3), argv.items.len);
	947	+ const shell_path = if (builtin.os.tag == .windows) "cmd.exe" else "/bin/sh";
	948	+ const shell_flag = if (builtin.os.tag == .windows) "/c" else "-c";
	949	+ try testing.expectEqualStrings(shell_path, argv.items[0]);
	950	+ try testing.expectEqualStrings(shell_flag, argv.items[1]);
	951	+ try testing.expectEqualStrings("echo hello", argv.items[2]);
	952	+}
	953	+
	954	+test "shell:true keeps the blocklist that path needs" {
	955	+ // The metacharacter check is not dropped, it is moved to the only place it
	956	+ // defends anything: the path that hands a string to a shell.
	957	+ try testing.expectError(BridgeError.UnsafeCommand, argvFor("ls; rm -rf /", &.{}, true));
	958	+ try testing.expectError(BridgeError.UnsafeCommand, argvFor("echo $(whoami)", &.{}, true));
	959	+ try testing.expectError(BridgeError.UnsafeCommand, argvFor("a | b", &.{}, true));
	960	+}
	961	+
	962	+test "asking for a shell and arguments at once is refused, not half-honoured" {
	963	+ // `sh -c` takes one string. Quietly ignoring the arguments is exactly the
	964	+ // failure this change exists to remove, so it must not be reintroduced by
	965	+ // the compatibility path.
	966	+ try testing.expectError(
	967	+ BridgeError.InvalidParameter,
	968	+ argvFor("git", &.{"status"}, true),
	969	+ );
	970	+}
	971	+
	972	+test "the argv is built in order, and order is the whole meaning" {
	973	+ var argv = try argvFor("cmd", &.{ "1", "2", "3", "4", "5" }, false);
	974	+ defer argv.deinit(testing.allocator);
	975	+ const expected = [_][]const u8{ "cmd", "1", "2", "3", "4", "5" };
	976	+ try testing.expectEqual(expected.len, argv.items.len);
	977	+ for (expected, 0..) |e, i| try testing.expectEqualStrings(e, argv.items[i]);
	978	+}
	979	+
	980	+test "an empty argument is still an argument" {
	981	+ // `find . -name ""` is meaningful; dropping empty strings would silently
	982	+ // change the command.
	983	+ var argv = try argvFor("find", &.{ ".", "-name", "" }, false);
	984	+ defer argv.deinit(testing.allocator);
	985	+ try testing.expectEqual(@as(usize, 4), argv.items.len);
	986	+ try testing.expectEqualStrings("", argv.items[3]);
	987	+}
	988	+
	989	+test "the JSON the page sends decodes into the arguments the argv is built from" {
	990	+ // The seam the bug lived in: the payload was parsed correctly and the
	991	+ // struct simply had nowhere to put `args`, so `ignore_unknown_fields`
	992	+ // dropped them without a word.
	993	+ const Params = struct {
	994	+ id: []const u8 = "",
	995	+ command: []const u8 = "",
	996	+ cwd: []const u8 = "",
	997	+ args: []const []const u8 = &.{},
	998	+ shell: bool = false,
	999	+ };
	1000	+ const payload =
	1001	+ \\{"id":"p1","command":"git","args":["status","--porcelain"],"cwd":"/tmp"}
	1002	+ ;
	1003	+ const parsed = try std.json.parseFromSlice(Params, testing.allocator, payload, .{
	1004	+ .ignore_unknown_fields = true,
	1005	+ .allocate = .alloc_always,
	1006	+ });
	1007	+ defer parsed.deinit();
	1008	+
	1009	+ try testing.expectEqual(@as(usize, 2), parsed.value.args.len);
	1010	+ try testing.expectEqualStrings("status", parsed.value.args[0]);
	1011	+ try testing.expect(!parsed.value.shell);
	1012	+
	1013	+ var argv = try argvFor(parsed.value.command, parsed.value.args, parsed.value.shell);
	1014	+ defer argv.deinit(testing.allocator);
	1015	+ try testing.expectEqual(@as(usize, 3), argv.items.len);
	1016	+ try testing.expectEqualStrings("--porcelain", argv.items[2]);
	1017	+}
	1018	+
	1019	+test "a payload from before this change still spawns the same program" {
	1020	+ // No `args` key at all: an older page, or any caller that never used them.
	1021	+ const Params = struct {
	1022	+ id: []const u8 = "",
	1023	+ command: []const u8 = "",
	1024	+ args: []const []const u8 = &.{},
	1025	+ shell: bool = false,
	1026	+ };
	1027	+ const payload =
	1028	+ \\{"id":"p1","command":"/usr/bin/true"}
	1029	+ ;
	1030	+ const parsed = try std.json.parseFromSlice(Params, testing.allocator, payload, .{
	1031	+ .ignore_unknown_fields = true,
	1032	+ .allocate = .alloc_always,
	1033	+ });
	1034	+ defer parsed.deinit();
	1035	+
	1036	+ var argv = try argvFor(parsed.value.command, parsed.value.args, parsed.value.shell);
	1037	+ defer argv.deinit(testing.allocator);
	1038	+ try testing.expectEqual(@as(usize, 1), argv.items.len);
	1039	+ try testing.expectEqualStrings("/usr/bin/true", argv.items[0]);
	1040	+}