From 7f373823b02ee77e41db6bc34bbb61a912340b43 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 23 Mar 2026 19:33:29 -0700 Subject: [PATCH] refactor: separate exec policy and execution targets --- src/agents/bash-tools.exec-host-gateway.ts | 8 +- src/commands/doctor/shared/exec-safe-bins.ts | 6 +- src/infra/exec-approvals-allow-always.test.ts | 123 ++++++++++------ src/infra/exec-approvals-allowlist.ts | 36 ++--- src/infra/exec-approvals-analysis.test.ts | 3 +- src/infra/exec-approvals-analysis.ts | 8 +- src/infra/exec-approvals-parity.test.ts | 2 +- src/infra/exec-approvals-safe-bins.test.ts | 21 ++- src/infra/exec-approvals-test-helpers.ts | 51 +++++++ src/infra/exec-approvals.test.ts | 104 +++++++------ src/infra/exec-command-resolution.test.ts | 138 +++++++++++++++--- src/infra/exec-command-resolution.ts | 107 +++++++++++--- src/node-host/invoke-system-run-plan.ts | 3 +- src/node-host/invoke-system-run.ts | 4 +- 14 files changed, 444 insertions(+), 170 deletions(-) diff --git a/src/agents/bash-tools.exec-host-gateway.ts b/src/agents/bash-tools.exec-host-gateway.ts index d082df03922..ee172bdff5d 100644 --- a/src/agents/bash-tools.exec-host-gateway.ts +++ b/src/agents/bash-tools.exec-host-gateway.ts @@ -6,9 +6,9 @@ import { buildEnforcedShellCommand, evaluateShellAllowlist, recordAllowlistUse, + resolveApprovalAuditCandidatePath, requiresExecApproval, resolveAllowAlwaysPatterns, - resolvePolicyAllowlistCandidatePath, } from "../infra/exec-approvals.js"; import { describeInterpreterInlineEval, @@ -185,7 +185,7 @@ export async function processGatewayAllowlist( agentId: params.agentId, sessionKey: params.sessionKey, }), - resolvedPath: resolvePolicyAllowlistCandidatePath( + resolvedPath: resolveApprovalAuditCandidatePath( allowlistEval.segments[0]?.resolution ?? null, params.workdir, ), @@ -204,7 +204,7 @@ export async function processGatewayAllowlist( ...requestArgs, register: registerGatewayApproval, }); - const resolvedPath = resolvePolicyAllowlistCandidatePath( + const resolvedPath = resolveApprovalAuditCandidatePath( allowlistEval.segments[0]?.resolution ?? null, params.workdir, ); @@ -345,7 +345,7 @@ export async function processGatewayAllowlist( } recordMatchedAllowlistUse( - resolvePolicyAllowlistCandidatePath( + resolveApprovalAuditCandidatePath( allowlistEval.segments[0]?.resolution ?? null, params.workdir, ), diff --git a/src/commands/doctor/shared/exec-safe-bins.ts b/src/commands/doctor/shared/exec-safe-bins.ts index a06877b87bd..6ad3378830d 100644 --- a/src/commands/doctor/shared/exec-safe-bins.ts +++ b/src/commands/doctor/shared/exec-safe-bins.ts @@ -145,12 +145,12 @@ export function scanExecSafeBinTrustedDirHints( for (const scope of collectExecSafeBinScopes(cfg)) { for (const bin of scope.safeBins) { const resolution = resolveCommandResolutionFromArgv([bin]); - if (!resolution?.resolvedPath) { + if (!resolution?.execution.resolvedPath) { continue; } if ( isTrustedSafeBinPath({ - resolvedPath: resolution.resolvedPath, + resolvedPath: resolution.execution.resolvedPath, trustedDirs: scope.trustedSafeBinDirs, }) ) { @@ -159,7 +159,7 @@ export function scanExecSafeBinTrustedDirHints( hits.push({ scopePath: scope.scopePath, bin, - resolvedPath: resolution.resolvedPath, + resolvedPath: resolution.execution.resolvedPath, }); } } diff --git a/src/infra/exec-approvals-allow-always.test.ts b/src/infra/exec-approvals-allow-always.test.ts index cc001235787..7f372683939 100644 --- a/src/infra/exec-approvals-allow-always.test.ts +++ b/src/infra/exec-approvals-allow-always.test.ts @@ -1,7 +1,12 @@ import fs from "node:fs"; import path from "node:path"; import { describe, expect, it } from "vitest"; -import { makePathEnv, makeTempDir } from "./exec-approvals-test-helpers.js"; +import { + makeMockCommandResolution, + makeMockExecutableResolution, + makePathEnv, + makeTempDir, +} from "./exec-approvals-test-helpers.js"; import { evaluateShellAllowlist, requiresExecApproval, @@ -122,7 +127,13 @@ describe("resolveAllowAlwaysPatterns", () => { { raw: exe, argv: [exe], - resolution: { rawExecutable: exe, resolvedPath: exe, executableName: "openclaw-tool" }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: exe, + resolvedPath: exe, + executableName: "openclaw-tool", + }), + }), }, ], }); @@ -140,11 +151,13 @@ describe("resolveAllowAlwaysPatterns", () => { { raw: "/bin/zsh -lc 'whoami'", argv: ["/bin/zsh", "-lc", "whoami"], - resolution: { - rawExecutable: "/bin/zsh", - resolvedPath: "/bin/zsh", - executableName: "zsh", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "/bin/zsh", + resolvedPath: "/bin/zsh", + executableName: "zsh", + }), + }), }, ], cwd: dir, @@ -167,11 +180,13 @@ describe("resolveAllowAlwaysPatterns", () => { { raw: "/bin/zsh -lc 'whoami && ls && whoami'", argv: ["/bin/zsh", "-lc", "whoami && ls && whoami"], - resolution: { - rawExecutable: "/bin/zsh", - resolvedPath: "/bin/zsh", - executableName: "zsh", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "/bin/zsh", + resolvedPath: "/bin/zsh", + executableName: "zsh", + }), + }), }, ], cwd: dir, @@ -400,11 +415,13 @@ $0 \\"$1\\"" touch ${marker}`, { raw: "/bin/zsh -s", argv: ["/bin/zsh", "-s"], - resolution: { - rawExecutable: "/bin/zsh", - resolvedPath: "/bin/zsh", - executableName: "zsh", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "/bin/zsh", + resolvedPath: "/bin/zsh", + executableName: "zsh", + }), + }), }, ], platform: process.platform, @@ -423,11 +440,13 @@ $0 \\"$1\\"" touch ${marker}`, { raw: "/usr/local/bin/zsh -lc whoami", argv: ["/usr/local/bin/zsh", "-lc", "whoami"], - resolution: { - rawExecutable: "/usr/local/bin/zsh", - resolvedPath: undefined, - executableName: "/usr/local/bin/zsh", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "/usr/local/bin/zsh", + resolvedPath: undefined, + executableName: "/usr/local/bin/zsh", + }), + }), }, ], cwd: dir, @@ -448,11 +467,13 @@ $0 \\"$1\\"" touch ${marker}`, { raw: "/usr/bin/nice /bin/zsh -lc whoami", argv: ["/usr/bin/nice", "/bin/zsh", "-lc", "whoami"], - resolution: { - rawExecutable: "/usr/bin/nice", - resolvedPath: "/usr/bin/nice", - executableName: "nice", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "/usr/bin/nice", + resolvedPath: "/usr/bin/nice", + executableName: "nice", + }), + }), }, ], cwd: dir, @@ -474,11 +495,13 @@ $0 \\"$1\\"" touch ${marker}`, { raw: "/usr/bin/time -p /bin/zsh -lc whoami", argv: ["/usr/bin/time", "-p", "/bin/zsh", "-lc", "whoami"], - resolution: { - rawExecutable: "/usr/bin/time", - resolvedPath: "/usr/bin/time", - executableName: "time", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "/usr/bin/time", + resolvedPath: "/usr/bin/time", + executableName: "time", + }), + }), }, ], cwd: dir, @@ -503,11 +526,13 @@ $0 \\"$1\\"" touch ${marker}`, { raw: `${busybox} sh -lc whoami`, argv: [busybox, "sh", "-lc", "whoami"], - resolution: { - rawExecutable: busybox, - resolvedPath: busybox, - executableName: "busybox", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: busybox, + resolvedPath: busybox, + executableName: "busybox", + }), + }), }, ], cwd: dir, @@ -529,11 +554,13 @@ $0 \\"$1\\"" touch ${marker}`, { raw: `${busybox} sed -n 1p`, argv: [busybox, "sed", "-n", "1p"], - resolution: { - rawExecutable: busybox, - resolvedPath: busybox, - executableName: "busybox", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: busybox, + resolvedPath: busybox, + executableName: "busybox", + }), + }), }, ], cwd: dir, @@ -549,11 +576,13 @@ $0 \\"$1\\"" touch ${marker}`, { raw: "sudo /bin/zsh -lc whoami", argv: ["sudo", "/bin/zsh", "-lc", "whoami"], - resolution: { - rawExecutable: "sudo", - resolvedPath: "/usr/bin/sudo", - executableName: "sudo", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "sudo", + resolvedPath: "/usr/bin/sudo", + executableName: "sudo", + }), + }), }, ], platform: process.platform, diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index 703931b6a47..4226ebe2162 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -3,13 +3,15 @@ import { analyzeShellCommand, isWindowsPlatform, matchAllowlist, - resolveAllowlistCandidatePath, + resolveExecutionTargetCandidatePath, + resolveExecutionTargetResolution, resolveCommandResolutionFromArgv, - resolvePolicyAllowlistCandidatePath, + resolvePolicyTargetCandidatePath, + resolvePolicyTargetResolution, splitCommandChain, type ExecCommandAnalysis, - type CommandResolution, type ExecCommandSegment, + type ExecutableResolution, } from "./exec-approvals-analysis.js"; import type { ExecAllowlistEntry } from "./exec-approvals.js"; import { @@ -51,7 +53,7 @@ export function resolveSafeBins(entries?: readonly string[] | null): Set export function isSafeBinUsage(params: { argv: string[]; - resolution: CommandResolution | null; + resolution: ExecutableResolution | null; safeBins: Set; platform?: string | null; trustedSafeBinDirs?: ReadonlySet; @@ -183,15 +185,16 @@ function isSkillAutoAllowedSegment(params: { return false; } const resolution = params.segment.resolution; - if (!resolution?.resolvedPath) { + const execution = resolveExecutionTargetResolution(resolution); + if (!execution?.resolvedPath) { return false; } - const rawExecutable = resolution.rawExecutable?.trim() ?? ""; + const rawExecutable = execution.rawExecutable?.trim() ?? ""; if (!rawExecutable || isPathScopedExecutableToken(rawExecutable)) { return false; } - const executableName = normalizeSkillBinName(resolution.executableName); - const resolvedPath = normalizeSkillBinResolvedPath(resolution.resolvedPath); + const executableName = normalizeSkillBinName(execution.executableName); + const resolvedPath = normalizeSkillBinResolvedPath(execution.resolvedPath); if (!executableName || !resolvedPath) { return false; } @@ -222,8 +225,8 @@ function evaluateSegments( : segment.argv; const allowlistSegment = effectiveArgv === segment.argv ? segment : { ...segment, argv: effectiveArgv }; - const executableResolution = segment.resolution?.policyResolution ?? segment.resolution; - const candidatePath = resolvePolicyAllowlistCandidatePath(segment.resolution, params.cwd); + const executableResolution = resolvePolicyTargetResolution(segment.resolution); + const candidatePath = resolvePolicyTargetCandidatePath(segment.resolution, params.cwd); const candidateResolution = candidatePath && executableResolution ? { ...executableResolution, resolvedPath: candidatePath } @@ -262,7 +265,7 @@ function evaluateSegments( } const safe = isSafeBinUsage({ argv: effectiveArgv, - resolution: segment.resolution, + resolution: resolveExecutionTargetResolution(segment.resolution), safeBins: params.safeBins, safeBinProfiles: params.safeBinProfiles, platform: params.platform, @@ -337,11 +340,8 @@ function hasSegmentExecutableMatch( segment: ExecCommandSegment, predicate: (token: string) => boolean, ): boolean { - const candidates = [ - segment.resolution?.executableName, - segment.resolution?.rawExecutable, - segment.argv[0], - ]; + const execution = resolveExecutionTargetResolution(segment.resolution); + const candidates = [execution?.executableName, execution?.rawExecutable, segment.argv[0]]; for (const candidate of candidates) { const trimmed = candidate?.trim(); if (!trimmed) { @@ -464,7 +464,7 @@ function resolveShellWrapperPositionalArgvCandidatePath(params: { } const resolution = resolveCommandResolutionFromArgv([carriedExecutable], params.cwd, params.env); - return resolveAllowlistCandidatePath(resolution, params.cwd); + return resolveExecutionTargetCandidatePath(resolution, params.cwd); } function isDirectShellPositionalCarrierInvocation(command: string): boolean { @@ -509,7 +509,7 @@ function collectAllowAlwaysPatterns(params: { resolution: resolveCommandResolutionFromArgv(trustPlan.argv, params.cwd, params.env), }; - const candidatePath = resolveAllowlistCandidatePath(segment.resolution, params.cwd); + const candidatePath = resolveExecutionTargetCandidatePath(segment.resolution, params.cwd); if (!candidatePath) { return; } diff --git a/src/infra/exec-approvals-analysis.test.ts b/src/infra/exec-approvals-analysis.test.ts index 461b15a281e..38505010a10 100644 --- a/src/infra/exec-approvals-analysis.test.ts +++ b/src/infra/exec-approvals-analysis.test.ts @@ -94,7 +94,8 @@ describe("exec approvals shell analysis", () => { const planned = resolvePlannedSegmentArgv(segment); expect(planned).toEqual([ - segment.resolution?.resolvedRealPath ?? segment.resolution?.resolvedPath, + segment.resolution?.execution.resolvedRealPath ?? + segment.resolution?.execution.resolvedPath, "-lc", "echo hi", ]); diff --git a/src/infra/exec-approvals-analysis.ts b/src/infra/exec-approvals-analysis.ts index 66f29ce0c00..1a169f154db 100644 --- a/src/infra/exec-approvals-analysis.ts +++ b/src/infra/exec-approvals-analysis.ts @@ -9,9 +9,14 @@ export { matchAllowlist, parseExecArgvToken, resolveAllowlistCandidatePath, + resolveApprovalAuditCandidatePath, resolveCommandResolution, resolveCommandResolutionFromArgv, + resolveExecutionTargetCandidatePath, + resolveExecutionTargetResolution, resolvePolicyAllowlistCandidatePath, + resolvePolicyTargetCandidatePath, + resolvePolicyTargetResolution, type CommandResolution, type ExecutableResolution, type ExecArgvToken, @@ -667,8 +672,9 @@ export function resolvePlannedSegmentArgv(segment: ExecCommandSegment): string[] return null; } const argv = [...baseArgv]; + const execution = segment.resolution?.execution; const resolvedExecutable = - segment.resolution?.resolvedRealPath?.trim() ?? segment.resolution?.resolvedPath?.trim() ?? ""; + execution?.resolvedRealPath?.trim() ?? execution?.resolvedPath?.trim() ?? ""; if (resolvedExecutable) { argv[0] = resolvedExecutable; } diff --git a/src/infra/exec-approvals-parity.test.ts b/src/infra/exec-approvals-parity.test.ts index 177e64c8c99..d86c0410771 100644 --- a/src/infra/exec-approvals-parity.test.ts +++ b/src/infra/exec-approvals-parity.test.ts @@ -31,7 +31,7 @@ describe("exec approvals wrapper resolution parity fixture", () => { for (const fixture of fixtures) { it(`matches wrapper fixture: ${fixture.id}`, () => { const resolution = resolveCommandResolutionFromArgv(fixture.argv); - expect(resolution?.rawExecutable ?? null).toBe(fixture.expectedRawExecutable); + expect(resolution?.execution.rawExecutable ?? null).toBe(fixture.expectedRawExecutable); }); } }); diff --git a/src/infra/exec-approvals-safe-bins.test.ts b/src/infra/exec-approvals-safe-bins.test.ts index e8fb40877a6..4306e89a68e 100644 --- a/src/infra/exec-approvals-safe-bins.test.ts +++ b/src/infra/exec-approvals-safe-bins.test.ts @@ -1,7 +1,12 @@ import fs from "node:fs"; import path from "node:path"; import { describe, expect, it } from "vitest"; -import { makePathEnv, makeTempDir } from "./exec-approvals-test-helpers.js"; +import { + makeMockCommandResolution, + makeMockExecutableResolution, + makePathEnv, + makeTempDir, +} from "./exec-approvals-test-helpers.js"; import { evaluateExecAllowlist, evaluateShellAllowlist, @@ -428,11 +433,13 @@ describe("exec approvals safe bins", () => { { raw: "jq .foo", argv: ["jq", ".foo"], - resolution: { - rawExecutable: "jq", - resolvedPath: "/custom/bin/jq", - executableName: "jq", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "jq", + resolvedPath: "/custom/bin/jq", + executableName: "jq", + }), + }), }, ], }; @@ -476,7 +483,7 @@ describe("exec approvals safe bins", () => { expect(result.analysisOk).toBe(true); expect(result.allowlistSatisfied).toBe(false); expect(result.segmentSatisfiedBy).toEqual([null]); - expect(result.segments[0]?.resolution?.resolvedPath).toBe(fakeHead); + expect(result.segments[0]?.resolution?.execution.resolvedPath).toBe(fakeHead); }); it("fails closed for semantic env wrappers in allowlist mode", () => { diff --git a/src/infra/exec-approvals-test-helpers.ts b/src/infra/exec-approvals-test-helpers.ts index fb616410b35..e39e93c5e3c 100644 --- a/src/infra/exec-approvals-test-helpers.ts +++ b/src/infra/exec-approvals-test-helpers.ts @@ -1,6 +1,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; +import type { CommandResolution, ExecutableResolution } from "./exec-command-resolution.js"; export function makePathEnv(binDir: string): NodeJS.ProcessEnv { if (process.platform !== "win32") { @@ -13,6 +14,56 @@ export function makeTempDir(): string { return fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-exec-approvals-")); } +export function makeMockExecutableResolution(params: { + rawExecutable: string; + executableName: string; + resolvedPath?: string; + resolvedRealPath?: string; +}): ExecutableResolution { + return { + rawExecutable: params.rawExecutable, + resolvedPath: params.resolvedPath, + resolvedRealPath: params.resolvedRealPath, + executableName: params.executableName, + }; +} + +export function makeMockCommandResolution(params: { + execution: ExecutableResolution; + policy?: ExecutableResolution; + effectiveArgv?: string[]; + wrapperChain?: string[]; + policyBlocked?: boolean; + blockedWrapper?: string; +}): CommandResolution { + const policy = params.policy ?? params.execution; + const resolution: CommandResolution = { + execution: params.execution, + policy, + effectiveArgv: params.effectiveArgv, + wrapperChain: params.wrapperChain, + policyBlocked: params.policyBlocked, + blockedWrapper: params.blockedWrapper, + }; + return Object.defineProperties(resolution, { + rawExecutable: { + get: () => params.execution.rawExecutable, + }, + resolvedPath: { + get: () => params.execution.resolvedPath, + }, + resolvedRealPath: { + get: () => params.execution.resolvedRealPath, + }, + executableName: { + get: () => params.execution.executableName, + }, + policyResolution: { + get: () => (policy === params.execution ? undefined : policy), + }, + }); +} + export type ShellParserParityFixtureCase = { id: string; command: string; diff --git a/src/infra/exec-approvals.test.ts b/src/infra/exec-approvals.test.ts index 75cf2b115b6..c98a337b966 100644 --- a/src/infra/exec-approvals.test.ts +++ b/src/infra/exec-approvals.test.ts @@ -1,5 +1,9 @@ import { describe, expect, it } from "vitest"; import { normalizeSafeBins } from "./exec-approvals-allowlist.js"; +import { + makeMockCommandResolution, + makeMockExecutableResolution, +} from "./exec-approvals-test-helpers.js"; import { evaluateExecAllowlist, type ExecAllowlistEntry } from "./exec-approvals.js"; describe("exec approvals allowlist evaluation", () => { @@ -9,11 +13,7 @@ describe("exec approvals allowlist evaluation", () => { segments: Array<{ raw: string; argv: string[]; - resolution: { - rawExecutable: string; - executableName: string; - resolvedPath?: string; - }; + resolution: ReturnType; }>; }; resolvedPath: string; @@ -40,11 +40,13 @@ describe("exec approvals allowlist evaluation", () => { { raw: "tool", argv: ["tool"], - resolution: { - rawExecutable: "tool", - resolvedPath: "/usr/bin/tool", - executableName: "tool", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "tool", + resolvedPath: "/usr/bin/tool", + executableName: "tool", + }), + }), }, ], }; @@ -66,11 +68,13 @@ describe("exec approvals allowlist evaluation", () => { { raw: "jq .foo", argv: ["jq", ".foo"], - resolution: { - rawExecutable: "jq", - resolvedPath: "/usr/bin/jq", - executableName: "jq", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "jq", + resolvedPath: "/usr/bin/jq", + executableName: "jq", + }), + }), }, ], }; @@ -96,11 +100,13 @@ describe("exec approvals allowlist evaluation", () => { { raw: "skill-bin", argv: ["skill-bin", "--help"], - resolution: { - rawExecutable: "skill-bin", - resolvedPath: "/opt/skills/skill-bin", - executableName: "skill-bin", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "skill-bin", + resolvedPath: "/opt/skills/skill-bin", + executableName: "skill-bin", + }), + }), }, ], }; @@ -118,11 +124,13 @@ describe("exec approvals allowlist evaluation", () => { { raw: "./skill-bin", argv: ["./skill-bin", "--help"], - resolution: { - rawExecutable: "./skill-bin", - resolvedPath: "/tmp/skill-bin", - executableName: "skill-bin", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "./skill-bin", + resolvedPath: "/tmp/skill-bin", + executableName: "skill-bin", + }), + }), }, ], }; @@ -140,10 +148,12 @@ describe("exec approvals allowlist evaluation", () => { { raw: "skill-bin --help", argv: ["skill-bin", "--help"], - resolution: { - rawExecutable: "skill-bin", - executableName: "skill-bin", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "skill-bin", + executableName: "skill-bin", + }), + }), }, ], }; @@ -158,11 +168,13 @@ describe("exec approvals allowlist evaluation", () => { const segment = { raw: "tool", argv: ["tool"], - resolution: { - rawExecutable: "tool", - resolvedPath: "/usr/bin/tool", - executableName: "tool", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "tool", + resolvedPath: "/usr/bin/tool", + executableName: "tool", + }), + }), }; const analysis = { ok: true, @@ -184,20 +196,24 @@ describe("exec approvals allowlist evaluation", () => { const allowlistSegment = { raw: "tool", argv: ["tool"], - resolution: { - rawExecutable: "tool", - resolvedPath: "/usr/bin/tool", - executableName: "tool", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "tool", + resolvedPath: "/usr/bin/tool", + executableName: "tool", + }), + }), }; const safeBinSegment = { raw: "jq .foo", argv: ["jq", ".foo"], - resolution: { - rawExecutable: "jq", - resolvedPath: "/usr/bin/jq", - executableName: "jq", - }, + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "jq", + resolvedPath: "/usr/bin/jq", + executableName: "jq", + }), + }), }; const analysis = { ok: true, diff --git a/src/infra/exec-command-resolution.test.ts b/src/infra/exec-command-resolution.test.ts index 3d6981ff720..17feccc60b2 100644 --- a/src/infra/exec-command-resolution.test.ts +++ b/src/infra/exec-command-resolution.test.ts @@ -4,12 +4,13 @@ import { describe, expect, it } from "vitest"; import { makePathEnv, makeTempDir } from "./exec-approvals-test-helpers.js"; import { evaluateExecAllowlist, + resolvePlannedSegmentArgv, normalizeSafeBins, parseExecArgvToken, - resolveAllowlistCandidatePath, resolveCommandResolution, resolveCommandResolutionFromArgv, - resolvePolicyAllowlistCandidatePath, + resolveExecutionTargetCandidatePath, + resolvePolicyTargetCandidatePath, } from "./exec-approvals.js"; function buildNestedEnvShellCommand(params: { @@ -119,9 +120,9 @@ describe("exec-command-resolution", () => { for (const testCase of cases) { const setup = testCase.setup(); const res = resolveCommandResolution(setup.command, setup.cwd, setup.envPath); - expect(res?.resolvedPath, testCase.name).toBe(setup.expectedPath); + expect(res?.execution.resolvedPath, testCase.name).toBe(setup.expectedPath); if (setup.expectedExecutableName) { - expect(res?.executableName, testCase.name).toBe(setup.expectedExecutableName); + expect(res?.execution.executableName, testCase.name).toBe(setup.expectedExecutableName); } } }); @@ -134,8 +135,8 @@ describe("exec-command-resolution", () => { undefined, makePathEnv(fixture.binDir), ); - expect(envResolution?.resolvedPath).toBe(fixture.exePath); - expect(envResolution?.executableName).toBe(fixture.exeName); + expect(envResolution?.execution.resolvedPath).toBe(fixture.exePath); + expect(envResolution?.execution.executableName).toBe(fixture.exeName); const niceResolution = resolveCommandResolutionFromArgv([ "/usr/bin/nice", @@ -143,16 +144,16 @@ describe("exec-command-resolution", () => { "-lc", "echo hi", ]); - expect(niceResolution?.rawExecutable).toBe("bash"); - expect(niceResolution?.executableName.toLowerCase()).toContain("bash"); + expect(niceResolution?.execution.rawExecutable).toBe("bash"); + expect(niceResolution?.execution.executableName.toLowerCase()).toContain("bash"); const timeResolution = resolveCommandResolutionFromArgv( ["/usr/bin/time", "-p", "rg", "-n", "needle"], undefined, makePathEnv(fixture.binDir), ); - expect(timeResolution?.resolvedPath).toBe(fixture.exePath); - expect(timeResolution?.executableName).toBe(fixture.exeName); + expect(timeResolution?.execution.resolvedPath).toBe(fixture.exePath); + expect(timeResolution?.execution.executableName).toBe(fixture.exeName); }); it("keeps shell multiplexer wrappers as a separate policy target", () => { @@ -165,13 +166,13 @@ describe("exec-command-resolution", () => { fs.chmodSync(busybox, 0o755); const resolution = resolveCommandResolutionFromArgv([busybox, "sh", "-lc", "echo hi"]); - expect(resolution?.rawExecutable).toBe("sh"); + expect(resolution?.execution.rawExecutable).toBe("sh"); expect(resolution?.effectiveArgv).toEqual(["sh", "-lc", "echo hi"]); expect(resolution?.wrapperChain).toEqual(["busybox"]); - expect(resolution?.policyResolution?.rawExecutable).toBe(busybox); - expect(resolution?.policyResolution?.resolvedPath).toBe(busybox); - expect(resolvePolicyAllowlistCandidatePath(resolution ?? null, dir)).toBe(busybox); - expect(resolution?.executableName.toLowerCase()).toContain("sh"); + expect(resolution?.policy.rawExecutable).toBe(busybox); + expect(resolution?.policy.resolvedPath).toBe(busybox); + expect(resolvePolicyTargetCandidatePath(resolution ?? null, dir)).toBe(busybox); + expect(resolution?.execution.executableName.toLowerCase()).toContain("sh"); }); it("does not satisfy inner-shell allowlists when invoked through busybox wrappers", () => { @@ -184,7 +185,7 @@ describe("exec-command-resolution", () => { fs.chmodSync(busybox, 0o755); const shellResolution = resolveCommandResolutionFromArgv(["sh", "-lc", "echo hi"]); - expect(shellResolution?.resolvedPath).toBeTruthy(); + expect(shellResolution?.execution.resolvedPath).toBeTruthy(); const wrappedResolution = resolveCommandResolutionFromArgv([busybox, "sh", "-lc", "echo hi"]); const evalResult = evaluateExecAllowlist({ @@ -198,7 +199,7 @@ describe("exec-command-resolution", () => { }, ], }, - allowlist: [{ pattern: shellResolution?.resolvedPath ?? "" }], + allowlist: [{ pattern: shellResolution?.execution.resolvedPath ?? "" }], safeBins: normalizeSafeBins([]), cwd: dir, }); @@ -215,7 +216,7 @@ describe("exec-command-resolution", () => { "needle", ]); expect(blockedEnv?.policyBlocked).toBe(true); - expect(blockedEnv?.rawExecutable).toBe("/usr/bin/env"); + expect(blockedEnv?.execution.rawExecutable).toBe("/usr/bin/env"); if (process.platform === "win32") { return; @@ -252,7 +253,7 @@ describe("exec-command-resolution", () => { it("resolves allowlist candidate paths from unresolved raw executables", () => { expect( - resolveAllowlistCandidatePath( + resolveExecutionTargetCandidatePath( { rawExecutable: "~/bin/tool", executableName: "tool", @@ -262,7 +263,7 @@ describe("exec-command-resolution", () => { ).toContain("/bin/tool"); expect( - resolveAllowlistCandidatePath( + resolveExecutionTargetCandidatePath( { rawExecutable: "./scripts/run.sh", executableName: "run.sh", @@ -272,7 +273,7 @@ describe("exec-command-resolution", () => { ).toBe(path.resolve("/repo", "./scripts/run.sh")); expect( - resolveAllowlistCandidatePath( + resolveExecutionTargetCandidatePath( { rawExecutable: "rg", executableName: "rg", @@ -282,6 +283,101 @@ describe("exec-command-resolution", () => { ).toBeUndefined(); }); + it("keeps execution and policy targets coherent across wrapper classes", () => { + if (process.platform === "win32") { + return; + } + + const dir = makeTempDir(); + const binDir = path.join(dir, "bin"); + fs.mkdirSync(binDir, { recursive: true }); + const envPath = path.join(binDir, "env"); + const rgPath = path.join(binDir, "rg"); + const busybox = path.join(dir, "busybox"); + for (const file of [envPath, rgPath, busybox]) { + fs.writeFileSync(file, ""); + fs.chmodSync(file, 0o755); + } + + const cases = [ + { + name: "transparent env wrapper", + argv: [envPath, "rg", "-n", "needle"], + env: makePathEnv(binDir), + expectedExecutionPath: rgPath, + expectedPolicyPath: rgPath, + expectedPlannedArgv: [fs.realpathSync(rgPath), "-n", "needle"], + allowlistPattern: rgPath, + allowlistSatisfied: true, + }, + { + name: "busybox shell multiplexer", + argv: [busybox, "sh", "-lc", "echo hi"], + env: { PATH: `${binDir}${path.delimiter}/bin:/usr/bin` }, + expectedExecutionPath: "/bin/sh", + expectedPolicyPath: busybox, + expectedPlannedArgv: ["/bin/sh", "-lc", "echo hi"], + allowlistPattern: busybox, + allowlistSatisfied: true, + }, + { + name: "semantic env wrapper", + argv: [envPath, "FOO=bar", "rg", "-n", "needle"], + env: makePathEnv(binDir), + expectedExecutionPath: envPath, + expectedPolicyPath: envPath, + expectedPlannedArgv: null, + allowlistPattern: envPath, + allowlistSatisfied: false, + }, + { + name: "wrapper depth overflow", + argv: buildNestedEnvShellCommand({ + envExecutable: envPath, + depth: 5, + payload: "echo hi", + }), + env: makePathEnv(binDir), + expectedExecutionPath: envPath, + expectedPolicyPath: envPath, + expectedPlannedArgv: null, + allowlistPattern: envPath, + allowlistSatisfied: false, + }, + ] as const; + + for (const testCase of cases) { + const argv = [...testCase.argv]; + const resolution = resolveCommandResolutionFromArgv(argv, dir, testCase.env); + const segment = { + raw: argv.join(" "), + argv, + resolution, + }; + expect( + resolveExecutionTargetCandidatePath(resolution ?? null, dir), + `${testCase.name} execution`, + ).toBe(testCase.expectedExecutionPath); + expect( + resolvePolicyTargetCandidatePath(resolution ?? null, dir), + `${testCase.name} policy`, + ).toBe(testCase.expectedPolicyPath); + expect(resolvePlannedSegmentArgv(segment), `${testCase.name} planned argv`).toEqual( + testCase.expectedPlannedArgv, + ); + const evaluation = evaluateExecAllowlist({ + analysis: { ok: true, segments: [segment] }, + allowlist: [{ pattern: testCase.allowlistPattern }], + safeBins: normalizeSafeBins([]), + cwd: dir, + env: testCase.env, + }); + expect(evaluation.allowlistSatisfied, `${testCase.name} allowlist`).toBe( + testCase.allowlistSatisfied, + ); + } + }); + it("normalizes argv tokens for short clusters, long options, and special sentinels", () => { expect(parseExecArgvToken("")).toEqual({ kind: "empty", raw: "" }); expect(parseExecArgvToken("--")).toEqual({ kind: "terminator", raw: "--" }); diff --git a/src/infra/exec-command-resolution.ts b/src/infra/exec-command-resolution.ts index 9ebb7482d2c..242e904a043 100644 --- a/src/infra/exec-command-resolution.ts +++ b/src/infra/exec-command-resolution.ts @@ -13,14 +13,21 @@ export type ExecutableResolution = { executableName: string; }; -export type CommandResolution = ExecutableResolution & { +export type CommandResolution = { + execution: ExecutableResolution; + policy: ExecutableResolution; effectiveArgv?: string[]; wrapperChain?: string[]; policyBlocked?: boolean; blockedWrapper?: string; - policyResolution?: ExecutableResolution; }; +function isCommandResolution( + resolution: CommandResolution | ExecutableResolution | null, +): resolution is CommandResolution { + return Boolean(resolution && "execution" in resolution && "policy" in resolution); +} + function parseFirstToken(command: string): string | null { const trimmed = command.trim(); if (!trimmed) { @@ -80,19 +87,36 @@ function buildCommandResolution(params: { policyBlocked: boolean; blockedWrapper?: string; }): CommandResolution { - const resolution = buildExecutableResolution(params.rawExecutable, params); - const policyResolution = - params.policyRawExecutable && params.policyRawExecutable !== params.rawExecutable - ? buildExecutableResolution(params.policyRawExecutable, params) - : undefined; - return { - ...resolution, + const execution = buildExecutableResolution(params.rawExecutable, params); + const policy = params.policyRawExecutable + ? buildExecutableResolution(params.policyRawExecutable, params) + : execution; + const resolution: CommandResolution = { + execution, + policy, effectiveArgv: params.effectiveArgv, wrapperChain: params.wrapperChain, policyBlocked: params.policyBlocked, blockedWrapper: params.blockedWrapper, - policyResolution, }; + // Compatibility getters for JS/tests while TS callers migrate to explicit targets. + return Object.defineProperties(resolution, { + rawExecutable: { + get: () => execution.rawExecutable, + }, + resolvedPath: { + get: () => execution.resolvedPath, + }, + resolvedRealPath: { + get: () => execution.resolvedRealPath, + }, + executableName: { + get: () => execution.executableName, + }, + policyResolution: { + get: () => (policy === execution ? undefined : policy), + }, + }); } export function resolveCommandResolution( @@ -137,13 +161,6 @@ export function resolveCommandResolutionFromArgv( }); } -export function resolveAllowlistCandidatePath( - resolution: CommandResolution | null, - cwd?: string, -): string | undefined { - return resolveExecutableCandidatePathFromResolution(resolution, cwd); -} - function resolveExecutableCandidatePathFromResolution( resolution: ExecutableResolution | null | undefined, cwd?: string, @@ -169,16 +186,66 @@ function resolveExecutableCandidatePathFromResolution( return path.resolve(base, expanded); } -export function resolvePolicyAllowlistCandidatePath( - resolution: CommandResolution | null, +export function resolveExecutionTargetResolution( + resolution: CommandResolution | ExecutableResolution | null, +): ExecutableResolution | null { + if (!resolution) { + return null; + } + return isCommandResolution(resolution) ? resolution.execution : resolution; +} + +export function resolvePolicyTargetResolution( + resolution: CommandResolution | ExecutableResolution | null, +): ExecutableResolution | null { + if (!resolution) { + return null; + } + return isCommandResolution(resolution) ? resolution.policy : resolution; +} + +export function resolveExecutionTargetCandidatePath( + resolution: CommandResolution | ExecutableResolution | null, cwd?: string, ): string | undefined { return resolveExecutableCandidatePathFromResolution( - resolution?.policyResolution ?? resolution, + isCommandResolution(resolution) ? resolution.execution : resolution, cwd, ); } +export function resolvePolicyTargetCandidatePath( + resolution: CommandResolution | ExecutableResolution | null, + cwd?: string, +): string | undefined { + return resolveExecutableCandidatePathFromResolution( + isCommandResolution(resolution) ? resolution.policy : resolution, + cwd, + ); +} + +export function resolveApprovalAuditCandidatePath( + resolution: CommandResolution | null, + cwd?: string, +): string | undefined { + return resolvePolicyTargetCandidatePath(resolution, cwd); +} + +// Legacy alias kept while callers migrate to explicit target naming. +export function resolveAllowlistCandidatePath( + resolution: CommandResolution | ExecutableResolution | null, + cwd?: string, +): string | undefined { + return resolveExecutionTargetCandidatePath(resolution, cwd); +} + +export function resolvePolicyAllowlistCandidatePath( + resolution: CommandResolution | ExecutableResolution | null, + cwd?: string, +): string | undefined { + return resolvePolicyTargetCandidatePath(resolution, cwd); +} + export function matchAllowlist( entries: ExecAllowlistEntry[], resolution: ExecutableResolution | null, diff --git a/src/node-host/invoke-system-run-plan.ts b/src/node-host/invoke-system-run-plan.ts index 6d90c8a7eb6..bf22b9e0c10 100644 --- a/src/node-host/invoke-system-run-plan.ts +++ b/src/node-host/invoke-system-run-plan.ts @@ -1004,7 +1004,8 @@ export function hardenApprovedExecutionPaths(params: { }; } - const pinnedExecutable = resolution?.resolvedRealPath ?? resolution?.resolvedPath; + const pinnedExecutable = + resolution?.execution.resolvedRealPath ?? resolution?.execution.resolvedPath; if (!pinnedExecutable) { return { ok: false, diff --git a/src/node-host/invoke-system-run.ts b/src/node-host/invoke-system-run.ts index 04d90736cd7..facd3215bc4 100644 --- a/src/node-host/invoke-system-run.ts +++ b/src/node-host/invoke-system-run.ts @@ -5,9 +5,9 @@ import type { GatewayClient } from "../gateway/client.js"; import { addAllowlistEntry, recordAllowlistUse, + resolveApprovalAuditCandidatePath, resolveAllowAlwaysPatterns, resolveExecApprovals, - resolvePolicyAllowlistCandidatePath, type ExecAllowlistEntry, type ExecAsk, type ExecCommandSegment, @@ -576,7 +576,7 @@ async function executeSystemRunPhase( phase.agentId, match, phase.commandText, - resolvePolicyAllowlistCandidatePath(phase.segments[0]?.resolution ?? null, phase.cwd), + resolveApprovalAuditCandidatePath(phase.segments[0]?.resolution ?? null, phase.cwd), ); } }