| @@ -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. |
| @@ -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 | /// |
| @@ -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, .{ |
| @@ -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, |
| @@ -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 | +} |