From eea7e9e15a603bf46028c992d87b97d3853b74d9 Mon Sep 17 00:00:00 2001
From: B <6723574+louisgv@users.noreply.github.com>
Date: Thu, 12 Mar 2026 13:35:03 +0000
Subject: [PATCH] security: harden shellQuote and consolidate shell escaping in
 gcp.ts

- Add null-byte rejection to shellQuote (defense-in-depth)
- Export shellQuote for testability
- Refactor interactiveSession to use shellQuote instead of inline escaping
- Add comprehensive test suite for shellQuote security properties

Fixes #2529

Agent: security-auditor
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
---
 packages/cli/package.json                     |  2 +-
 .../cli/src/__tests__/gcp-shellquote.test.ts  | 71 +++++++++++++++++++
 packages/cli/src/gcp/gcp.ts                   | 15 ++--
 3 files changed, 83 insertions(+), 5 deletions(-)
 create mode 100644 packages/cli/src/__tests__/gcp-shellquote.test.ts

diff --git a/packages/cli/package.json b/packages/cli/package.json
index d411b8c7..027123ba 100644
--- a/packages/cli/package.json
+++ b/packages/cli/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@openrouter/spawn",
-  "version": "0.16.16",
+  "version": "0.16.17",
   "type": "module",
   "bin": {
     "spawn": "cli.js"
diff --git a/packages/cli/src/__tests__/gcp-shellquote.test.ts b/packages/cli/src/__tests__/gcp-shellquote.test.ts
new file mode 100644
index 00000000..9c973f12
--- /dev/null
+++ b/packages/cli/src/__tests__/gcp-shellquote.test.ts
@@ -0,0 +1,71 @@
+import { describe, expect, it } from "bun:test";
+import { shellQuote } from "../gcp/gcp.js";
+
+describe("shellQuote", () => {
+  it("should wrap simple strings in single quotes", () => {
+    expect(shellQuote("hello")).toBe("'hello'");
+    expect(shellQuote("ls -la")).toBe("'ls -la'");
+  });
+
+  it("should escape embedded single quotes", () => {
+    expect(shellQuote("it's")).toBe("'it'\\''s'");
+    expect(shellQuote("a'b'c")).toBe("'a'\\''b'\\''c'");
+  });
+
+  it("should handle strings with no special characters", () => {
+    expect(shellQuote("simple")).toBe("'simple'");
+    expect(shellQuote("/usr/bin/env")).toBe("'/usr/bin/env'");
+  });
+
+  it("should safely quote shell metacharacters", () => {
+    expect(shellQuote("$(whoami)")).toBe("'$(whoami)'");
+    expect(shellQuote("`id`")).toBe("'`id`'");
+    expect(shellQuote("a; rm -rf /")).toBe("'a; rm -rf /'");
+    expect(shellQuote("a | cat /etc/passwd")).toBe("'a | cat /etc/passwd'");
+    expect(shellQuote("a && curl evil.com")).toBe("'a && curl evil.com'");
+    expect(shellQuote("${HOME}")).toBe("'${HOME}'");
+  });
+
+  it("should handle double quotes inside single-quoted string", () => {
+    expect(shellQuote('echo "hello"')).toBe("'echo \"hello\"'");
+  });
+
+  it("should handle empty string", () => {
+    expect(shellQuote("")).toBe("''");
+  });
+
+  it("should reject null bytes (defense-in-depth)", () => {
+    expect(() => shellQuote("hello\x00world")).toThrow("null bytes");
+    expect(() => shellQuote("\x00")).toThrow("null bytes");
+    expect(() => shellQuote("cmd\x00; rm -rf /")).toThrow("null bytes");
+  });
+
+  it("should handle strings with newlines", () => {
+    const result = shellQuote("line1\nline2");
+    expect(result).toBe("'line1\nline2'");
+  });
+
+  it("should handle strings with tabs", () => {
+    const result = shellQuote("col1\tcol2");
+    expect(result).toBe("'col1\tcol2'");
+  });
+
+  it("should handle backslashes", () => {
+    expect(shellQuote("a\\b")).toBe("'a\\b'");
+  });
+
+  it("should handle multiple consecutive single quotes", () => {
+    expect(shellQuote("''")).toBe("''\\'''\\'''");
+  });
+
+  it("should produce output that is safe for bash -c", () => {
+    // Verify the quoting pattern: the result, when interpreted by bash,
+    // should yield the original string without executing anything
+    const dangerous = "$(rm -rf /)";
+    const quoted = shellQuote(dangerous);
+    // The quoted string wraps in single quotes, preventing expansion
+    expect(quoted).toBe("'$(rm -rf /)'");
+    expect(quoted.startsWith("'")).toBe(true);
+    expect(quoted.endsWith("'")).toBe(true);
+  });
+});
diff --git a/packages/cli/src/gcp/gcp.ts b/packages/cli/src/gcp/gcp.ts
index dfb91db3..51868ac3 100644
--- a/packages/cli/src/gcp/gcp.ts
+++ b/packages/cli/src/gcp/gcp.ts
@@ -1022,9 +1022,8 @@ export async function interactiveSession(cmd: string): Promise<number> {
   }
   const username = resolveUsername();
   const term = sanitizeTermValue(process.env.TERM || "xterm-256color");
-  // Single-quote escaping prevents premature shell expansion of $variables in cmd
-  const shellEscapedCmd = cmd.replace(/'/g, "'\\''");
-  const fullCmd = `export TERM=${term} PATH="$HOME/.npm-global/bin:$HOME/.claude/local/bin:$HOME/.local/bin:$HOME/.bun/bin:$PATH" && exec bash -l -c '${shellEscapedCmd}'`;
+  // Use shellQuote for consistent single-quote escaping (prevents shell expansion of $variables in cmd)
+  const fullCmd = `export TERM=${term} PATH="$HOME/.npm-global/bin:$HOME/.claude/local/bin:$HOME/.local/bin:$HOME/.bun/bin:$PATH" && exec bash -l -c ${shellQuote(cmd)}`;
   const keyOpts = getSshKeyOpts(await ensureSshKeys());
 
   const exitCode = spawnInteractive([
@@ -1084,6 +1083,14 @@ export async function destroyInstance(name?: string): Promise<void> {
 
 // ─── Shell Quoting ──────────────────────────────────────────────────────────
 
-function shellQuote(s: string): string {
+/** POSIX single-quote escaping: wraps `s` in single quotes and escapes any
+ *  embedded single quotes with the standard `'\''` technique.
+ *
+ *  Defense-in-depth: rejects null bytes which could truncate the string at
+ *  the C/OS level even though callers already validate for them. */
+export function shellQuote(s: string): string {
+  if (/\0/.test(s)) {
+    throw new Error("shellQuote: input must not contain null bytes");
+  }
   return "'" + s.replace(/'/g, "'\\''") + "'";
 }