From 1edf9579882d427322129dc434d0dadc0699102d Mon Sep 17 00:00:00 2001 From: Brian Mendonca Date: Mon, 23 Feb 2026 02:26:42 -0700 Subject: [PATCH] fix(exec): bind env-prefixed shell wrappers to full approval text --- .../node-invoke-system-run-approval.test.ts | 43 ++++++++ src/infra/exec-wrapper-resolution.ts | 98 +++++++++++++++++++ src/infra/system-run-command.test.ts | 33 +++++++ src/infra/system-run-command.ts | 28 ++++-- 4 files changed, 195 insertions(+), 7 deletions(-) diff --git a/src/gateway/node-invoke-system-run-approval.test.ts b/src/gateway/node-invoke-system-run-approval.test.ts index e0bc8fdd4f4..d93656682aa 100644 --- a/src/gateway/node-invoke-system-run-approval.test.ts +++ b/src/gateway/node-invoke-system-run-approval.test.ts @@ -82,4 +82,47 @@ describe("sanitizeSystemRunParamsForForwarding", () => { expect(params.approved).toBe(true); expect(params.approvalDecision).toBe("allow-once"); }); + + test("rejects env-assignment shell wrapper when approval command omits env prelude", () => { + const result = sanitizeSystemRunParamsForForwarding({ + rawParams: { + command: ["/usr/bin/env", "BASH_ENV=/tmp/payload.sh", "bash", "-lc", "echo SAFE"], + runId: "approval-1", + approved: true, + approvalDecision: "allow-once", + }, + client, + execApprovalManager: manager(makeRecord("echo SAFE")), + nowMs: now, + }); + expect(result.ok).toBe(false); + if (result.ok) { + throw new Error("unreachable"); + } + expect(result.message).toContain("approval id does not match request"); + expect(result.details?.code).toBe("APPROVAL_REQUEST_MISMATCH"); + }); + + test("accepts env-assignment shell wrapper only when approval command matches full argv text", () => { + const result = sanitizeSystemRunParamsForForwarding({ + rawParams: { + command: ["/usr/bin/env", "BASH_ENV=/tmp/payload.sh", "bash", "-lc", "echo SAFE"], + runId: "approval-1", + approved: true, + approvalDecision: "allow-once", + }, + client, + execApprovalManager: manager( + makeRecord('/usr/bin/env BASH_ENV=/tmp/payload.sh bash -lc "echo SAFE"'), + ), + nowMs: now, + }); + expect(result.ok).toBe(true); + if (!result.ok) { + throw new Error("unreachable"); + } + const params = result.params as Record; + expect(params.approved).toBe(true); + expect(params.approvalDecision).toBe("allow-once"); + }); }); diff --git a/src/infra/exec-wrapper-resolution.ts b/src/infra/exec-wrapper-resolution.ts index 2fae35f315e..2712ef8006c 100644 --- a/src/infra/exec-wrapper-resolution.ts +++ b/src/infra/exec-wrapper-resolution.ts @@ -209,6 +209,61 @@ export function unwrapEnvInvocation(argv: string[]): string[] | null { }); } +function envInvocationUsesModifiers(argv: string[]): boolean { + let idx = 1; + let expectsOptionValue = false; + while (idx < argv.length) { + const token = argv[idx]?.trim() ?? ""; + if (!token) { + idx += 1; + continue; + } + if (expectsOptionValue) { + return true; + } + if (token === "--" || token === "-") { + idx += 1; + break; + } + if (isEnvAssignment(token)) { + return true; + } + if (!token.startsWith("-") || token === "-") { + break; + } + const lower = token.toLowerCase(); + const [flag] = lower.split("=", 2); + if (ENV_FLAG_OPTIONS.has(flag)) { + return true; + } + if (ENV_OPTIONS_WITH_VALUE.has(flag)) { + if (lower.includes("=") || lower !== flag) { + return true; + } + expectsOptionValue = true; + idx += 1; + continue; + } + if ( + lower.startsWith("-u") || + lower.startsWith("-c") || + lower.startsWith("-s") || + lower.startsWith("--unset=") || + lower.startsWith("--chdir=") || + lower.startsWith("--split-string=") || + lower.startsWith("--default-signal=") || + lower.startsWith("--ignore-signal=") || + lower.startsWith("--block-signal=") + ) { + return true; + } + // Unknown env flags are treated conservatively as modifiers. + return true; + } + + return false; +} + function unwrapNiceInvocation(argv: string[]): string[] | null { return scanWrapperInvocation(argv, { separators: new Set(["--"]), @@ -345,6 +400,49 @@ export function unwrapDispatchWrappersForResolution( return current; } +function hasEnvManipulationBeforeShellWrapperInternal( + argv: string[], + depth: number, + envManipulationSeen: boolean, +): boolean { + if (depth >= MAX_DISPATCH_WRAPPER_DEPTH) { + return false; + } + + const token0 = argv[0]?.trim(); + if (!token0) { + return false; + } + + const dispatchUnwrap = unwrapKnownDispatchWrapperInvocation(argv); + if (dispatchUnwrap.kind === "blocked") { + return false; + } + if (dispatchUnwrap.kind === "unwrapped") { + const nextEnvManipulationSeen = + envManipulationSeen || (dispatchUnwrap.wrapper === "env" && envInvocationUsesModifiers(argv)); + return hasEnvManipulationBeforeShellWrapperInternal( + dispatchUnwrap.argv, + depth + 1, + nextEnvManipulationSeen, + ); + } + + const wrapper = findShellWrapperSpec(normalizeExecutableToken(token0)); + if (!wrapper) { + return false; + } + const payload = extractShellWrapperPayload(argv, wrapper); + if (!payload) { + return false; + } + return envManipulationSeen; +} + +export function hasEnvManipulationBeforeShellWrapper(argv: string[]): boolean { + return hasEnvManipulationBeforeShellWrapperInternal(argv, 0, false); +} + function extractPosixShellInlineCommand(argv: string[]): string | null { return extractInlineCommandByFlags(argv, POSIX_INLINE_COMMAND_FLAGS, { allowCombinedC: true }); } diff --git a/src/infra/system-run-command.test.ts b/src/infra/system-run-command.test.ts index e7ec9760b89..43a1b6fae79 100644 --- a/src/infra/system-run-command.test.ts +++ b/src/infra/system-run-command.test.ts @@ -106,6 +106,27 @@ describe("system run command helpers", () => { expect(res.ok).toBe(true); }); + test("validateSystemRunCommandConsistency rejects shell-only rawCommand for env assignment prelude", () => { + expectRawCommandMismatch({ + argv: ["/usr/bin/env", "BASH_ENV=/tmp/payload.sh", "bash", "-lc", "echo hi"], + rawCommand: "echo hi", + }); + }); + + test("validateSystemRunCommandConsistency accepts full rawCommand for env assignment prelude", () => { + const raw = '/usr/bin/env BASH_ENV=/tmp/payload.sh bash -lc "echo hi"'; + const res = validateSystemRunCommandConsistency({ + argv: ["/usr/bin/env", "BASH_ENV=/tmp/payload.sh", "bash", "-lc", "echo hi"], + rawCommand: raw, + }); + expect(res.ok).toBe(true); + if (!res.ok) { + throw new Error("unreachable"); + } + expect(res.shellCommand).toBe("echo hi"); + expect(res.cmdText).toBe(raw); + }); + test("validateSystemRunCommandConsistency rejects cmd.exe /c trailing-arg smuggling", () => { expectRawCommandMismatch({ argv: ["cmd.exe", "/d", "/s", "/c", "echo", "SAFE&&whoami"], @@ -143,4 +164,16 @@ describe("system run command helpers", () => { expect(res.shellCommand).toBe("echo SAFE&&whoami"); expect(res.cmdText).toBe("echo SAFE&&whoami"); }); + + test("resolveSystemRunCommand binds cmdText to full argv when env prelude modifies shell wrapper", () => { + const res = resolveSystemRunCommand({ + command: ["/usr/bin/env", "BASH_ENV=/tmp/payload.sh", "bash", "-lc", "echo hi"], + }); + expect(res.ok).toBe(true); + if (!res.ok) { + throw new Error("unreachable"); + } + expect(res.shellCommand).toBe("echo hi"); + expect(res.cmdText).toBe('/usr/bin/env BASH_ENV=/tmp/payload.sh bash -lc "echo hi"'); + }); }); diff --git a/src/infra/system-run-command.ts b/src/infra/system-run-command.ts index 9436836a9d7..c8bbac6e7a9 100644 --- a/src/infra/system-run-command.ts +++ b/src/infra/system-run-command.ts @@ -1,4 +1,7 @@ -import { extractShellWrapperCommand } from "./exec-wrapper-resolution.js"; +import { + extractShellWrapperCommand, + hasEnvManipulationBeforeShellWrapper, +} from "./exec-wrapper-resolution.js"; export type SystemRunCommandValidation = | { @@ -54,8 +57,14 @@ export function validateSystemRunCommandConsistency(params: { typeof params.rawCommand === "string" && params.rawCommand.trim().length > 0 ? params.rawCommand.trim() : null; - const shellCommand = extractShellWrapperCommand(params.argv).command; - const inferred = shellCommand !== null ? shellCommand.trim() : formatExecCommand(params.argv); + const shellWrapperResolution = extractShellWrapperCommand(params.argv); + const shellCommand = shellWrapperResolution.command; + const envManipulationBeforeShellWrapper = + shellWrapperResolution.isWrapper && hasEnvManipulationBeforeShellWrapper(params.argv); + const inferred = + shellCommand !== null && !envManipulationBeforeShellWrapper + ? shellCommand.trim() + : formatExecCommand(params.argv); if (raw && raw !== inferred) { return { @@ -72,10 +81,15 @@ export function validateSystemRunCommandConsistency(params: { return { ok: true, // Only treat this as a shell command when argv is a recognized shell wrapper. - // For direct argv execution, rawCommand is purely display/approval text and - // must match the formatted argv. - shellCommand: shellCommand !== null ? (raw ?? shellCommand) : null, - cmdText: raw ?? shellCommand ?? inferred, + // For direct argv execution and shell wrappers with env prelude modifiers, + // rawCommand is purely display/approval text and must match the formatted argv. + shellCommand: + shellCommand !== null + ? envManipulationBeforeShellWrapper + ? shellCommand + : (raw ?? shellCommand) + : null, + cmdText: raw ?? inferred, }; }