From 3c95f8966205e5173878d4ce71c0e1034c833def Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Wed, 25 Feb 2026 00:29:43 +0000 Subject: [PATCH] refactor(exec): split system.run phases and align ts/swift validator contracts --- .../OpenClaw/ExecApprovalsSocket.swift | 114 ++++----- .../OpenClaw/ExecHostRequestEvaluator.swift | 84 +++++++ .../ExecHostRequestEvaluatorTests.swift | 76 ++++++ .../ExecSystemRunCommandValidatorTests.swift | 100 +++++--- src/infra/system-run-command.contract.test.ts | 54 ++++ src/node-host/invoke-system-run.ts | 236 ++++++++++++------ .../fixtures/system-run-command-contract.json | 75 ++++++ 7 files changed, 565 insertions(+), 174 deletions(-) create mode 100644 apps/macos/Sources/OpenClaw/ExecHostRequestEvaluator.swift create mode 100644 apps/macos/Tests/OpenClawIPCTests/ExecHostRequestEvaluatorTests.swift create mode 100644 src/infra/system-run-command.contract.test.ts create mode 100644 test/fixtures/system-run-command-contract.json diff --git a/apps/macos/Sources/OpenClaw/ExecApprovalsSocket.swift b/apps/macos/Sources/OpenClaw/ExecApprovalsSocket.swift index 16f8db91305..1417589ae4a 100644 --- a/apps/macos/Sources/OpenClaw/ExecApprovalsSocket.swift +++ b/apps/macos/Sources/OpenClaw/ExecApprovalsSocket.swift @@ -38,7 +38,7 @@ private struct ExecHostSocketRequest: Codable { var requestJson: String } -private struct ExecHostRequest: Codable { +struct ExecHostRequest: Codable { var command: [String] var rawCommand: String? var cwd: String? @@ -59,7 +59,7 @@ private struct ExecHostRunResult: Codable { var error: String? } -private struct ExecHostError: Codable { +struct ExecHostError: Codable, Error { var code: String var message: String var reason: String? @@ -353,55 +353,28 @@ private enum ExecHostExecutor { private typealias ExecApprovalContext = ExecApprovalEvaluation static func handle(_ request: ExecHostRequest) async -> ExecHostResponse { - let command = request.command.map { $0.trimmingCharacters(in: .whitespacesAndNewlines) } - guard !command.isEmpty else { - return self.errorResponse( - code: "INVALID_REQUEST", - message: "command required", - reason: "invalid") - } - - let validatedCommand = ExecSystemRunCommandValidator.resolve( - command: command, - rawCommand: request.rawCommand) - let displayCommand: String - switch validatedCommand { - case let .ok(resolved): - displayCommand = resolved.displayCommand - case let .invalid(message): - return self.errorResponse( - code: "INVALID_REQUEST", - message: message, - reason: "invalid") + let validatedRequest: ExecHostValidatedRequest + switch ExecHostRequestEvaluator.validateRequest(request) { + case .success(let request): + validatedRequest = request + case .failure(let error): + return self.errorResponse(error) } let context = await self.buildContext( request: request, - command: command, - rawCommand: displayCommand) - if context.security == .deny { - return self.errorResponse( - code: "UNAVAILABLE", - message: "SYSTEM_RUN_DISABLED: security=deny", - reason: "security=deny") - } + command: validatedRequest.command, + rawCommand: validatedRequest.displayCommand) - let approvalDecision = request.approvalDecision - if approvalDecision == .deny { - return self.errorResponse( - code: "UNAVAILABLE", - message: "SYSTEM_RUN_DENIED: user denied", - reason: "user-denied") - } - - var approvedByAsk = approvalDecision != nil - if ExecApprovalHelpers.requiresAsk( - ask: context.ask, - security: context.security, - allowlistMatch: context.allowlistMatch, - skillAllow: context.skillAllow), - approvalDecision == nil + switch ExecHostRequestEvaluator.evaluate( + context: context, + approvalDecision: request.approvalDecision) { + case .deny(let error): + return self.errorResponse(error) + case .allow: + break + case .requiresPrompt: let decision = ExecApprovalsPromptPresenter.prompt( ExecApprovalPromptRequest( command: context.displayCommand, @@ -413,32 +386,34 @@ private enum ExecHostExecutor { resolvedPath: context.resolution?.resolvedPath, sessionKey: request.sessionKey)) + let followupDecision: ExecApprovalDecision switch decision { case .deny: - return self.errorResponse( - code: "UNAVAILABLE", - message: "SYSTEM_RUN_DENIED: user denied", - reason: "user-denied") + followupDecision = .deny case .allowAlways: - approvedByAsk = true + followupDecision = .allowAlways self.persistAllowlistEntry(decision: decision, context: context) case .allowOnce: - approvedByAsk = true + followupDecision = .allowOnce + } + + switch ExecHostRequestEvaluator.evaluate( + context: context, + approvalDecision: followupDecision) + { + case .deny(let error): + return self.errorResponse(error) + case .allow: + break + case .requiresPrompt: + return self.errorResponse( + code: "INVALID_REQUEST", + message: "unexpected approval state", + reason: "invalid") } } - self.persistAllowlistEntry(decision: approvalDecision, context: context) - - if context.security == .allowlist, - !context.allowlistSatisfied, - !context.skillAllow, - !approvedByAsk - { - return self.errorResponse( - code: "UNAVAILABLE", - message: "SYSTEM_RUN_DENIED: allowlist miss", - reason: "allowlist-miss") - } + self.persistAllowlistEntry(decision: request.approvalDecision, context: context) if context.allowlistSatisfied { var seenPatterns = Set() @@ -462,7 +437,7 @@ private enum ExecHostExecutor { } return await self.runCommand( - command: command, + command: validatedRequest.command, cwd: request.cwd, env: context.env, timeoutMs: request.timeoutMs) @@ -535,6 +510,17 @@ private enum ExecHostExecutor { return self.successResponse(payload) } + private static func errorResponse( + _ error: ExecHostError) -> ExecHostResponse + { + ExecHostResponse( + type: "response", + id: UUID().uuidString, + ok: false, + payload: nil, + error: error) + } + private static func errorResponse( code: String, message: String, diff --git a/apps/macos/Sources/OpenClaw/ExecHostRequestEvaluator.swift b/apps/macos/Sources/OpenClaw/ExecHostRequestEvaluator.swift new file mode 100644 index 00000000000..fe38d7ea18f --- /dev/null +++ b/apps/macos/Sources/OpenClaw/ExecHostRequestEvaluator.swift @@ -0,0 +1,84 @@ +import Foundation + +struct ExecHostValidatedRequest { + let command: [String] + let displayCommand: String +} + +enum ExecHostPolicyDecision { + case deny(ExecHostError) + case requiresPrompt + case allow(approvedByAsk: Bool) +} + +enum ExecHostRequestEvaluator { + static func validateRequest(_ request: ExecHostRequest) -> Result { + let command = request.command.map { $0.trimmingCharacters(in: .whitespacesAndNewlines) } + guard !command.isEmpty else { + return .failure( + ExecHostError( + code: "INVALID_REQUEST", + message: "command required", + reason: "invalid")) + } + + let validatedCommand = ExecSystemRunCommandValidator.resolve( + command: command, + rawCommand: request.rawCommand) + switch validatedCommand { + case .ok(let resolved): + return .success(ExecHostValidatedRequest(command: command, displayCommand: resolved.displayCommand)) + case .invalid(let message): + return .failure( + ExecHostError( + code: "INVALID_REQUEST", + message: message, + reason: "invalid")) + } + } + + static func evaluate( + context: ExecApprovalEvaluation, + approvalDecision: ExecApprovalDecision?) -> ExecHostPolicyDecision + { + if context.security == .deny { + return .deny( + ExecHostError( + code: "UNAVAILABLE", + message: "SYSTEM_RUN_DISABLED: security=deny", + reason: "security=deny")) + } + + if approvalDecision == .deny { + return .deny( + ExecHostError( + code: "UNAVAILABLE", + message: "SYSTEM_RUN_DENIED: user denied", + reason: "user-denied")) + } + + let approvedByAsk = approvalDecision != nil + let requiresPrompt = ExecApprovalHelpers.requiresAsk( + ask: context.ask, + security: context.security, + allowlistMatch: context.allowlistMatch, + skillAllow: context.skillAllow) && approvalDecision == nil + if requiresPrompt { + return .requiresPrompt + } + + if context.security == .allowlist, + !context.allowlistSatisfied, + !context.skillAllow, + !approvedByAsk + { + return .deny( + ExecHostError( + code: "UNAVAILABLE", + message: "SYSTEM_RUN_DENIED: allowlist miss", + reason: "allowlist-miss")) + } + + return .allow(approvedByAsk: approvedByAsk) + } +} diff --git a/apps/macos/Tests/OpenClawIPCTests/ExecHostRequestEvaluatorTests.swift b/apps/macos/Tests/OpenClawIPCTests/ExecHostRequestEvaluatorTests.swift new file mode 100644 index 00000000000..64ef6a21eda --- /dev/null +++ b/apps/macos/Tests/OpenClawIPCTests/ExecHostRequestEvaluatorTests.swift @@ -0,0 +1,76 @@ +import Foundation +import Testing +@testable import OpenClaw + +struct ExecHostRequestEvaluatorTests { + @Test func validateRequestRejectsEmptyCommand() { + let request = ExecHostRequest(command: [], rawCommand: nil, cwd: nil, env: nil, timeoutMs: nil, needsScreenRecording: nil, agentId: nil, sessionKey: nil, approvalDecision: nil) + switch ExecHostRequestEvaluator.validateRequest(request) { + case .success: + Issue.record("expected invalid request") + case .failure(let error): + #expect(error.code == "INVALID_REQUEST") + #expect(error.message == "command required") + } + } + + @Test func evaluateRequiresPromptOnAllowlistMissWithoutDecision() { + let context = Self.makeContext(security: .allowlist, ask: .onMiss, allowlistSatisfied: false, skillAllow: false) + let decision = ExecHostRequestEvaluator.evaluate(context: context, approvalDecision: nil) + switch decision { + case .requiresPrompt: + break + case .allow: + Issue.record("expected prompt requirement") + case .deny(let error): + Issue.record("unexpected deny: \(error.message)") + } + } + + @Test func evaluateAllowsAllowOnceDecisionOnAllowlistMiss() { + let context = Self.makeContext(security: .allowlist, ask: .onMiss, allowlistSatisfied: false, skillAllow: false) + let decision = ExecHostRequestEvaluator.evaluate(context: context, approvalDecision: .allowOnce) + switch decision { + case .allow(let approvedByAsk): + #expect(approvedByAsk) + case .requiresPrompt: + Issue.record("expected allow decision") + case .deny(let error): + Issue.record("unexpected deny: \(error.message)") + } + } + + @Test func evaluateDeniesOnExplicitDenyDecision() { + let context = Self.makeContext(security: .full, ask: .off, allowlistSatisfied: true, skillAllow: false) + let decision = ExecHostRequestEvaluator.evaluate(context: context, approvalDecision: .deny) + switch decision { + case .deny(let error): + #expect(error.reason == "user-denied") + case .requiresPrompt: + Issue.record("expected deny decision") + case .allow: + Issue.record("expected deny decision") + } + } + + private static func makeContext( + security: ExecSecurity, + ask: ExecAsk, + allowlistSatisfied: Bool, + skillAllow: Bool) -> ExecApprovalEvaluation + { + ExecApprovalEvaluation( + command: ["/usr/bin/echo", "hi"], + displayCommand: "/usr/bin/echo hi", + agentId: nil, + security: security, + ask: ask, + env: [:], + resolution: nil, + allowlistResolutions: [], + allowlistMatches: [], + allowlistSatisfied: allowlistSatisfied, + allowlistMatch: nil, + skillAllow: skillAllow) + } +} diff --git a/apps/macos/Tests/OpenClawIPCTests/ExecSystemRunCommandValidatorTests.swift b/apps/macos/Tests/OpenClawIPCTests/ExecSystemRunCommandValidatorTests.swift index c6ad3319a65..ed3773a44ed 100644 --- a/apps/macos/Tests/OpenClawIPCTests/ExecSystemRunCommandValidatorTests.swift +++ b/apps/macos/Tests/OpenClawIPCTests/ExecSystemRunCommandValidatorTests.swift @@ -2,49 +2,75 @@ import Foundation import Testing @testable import OpenClaw +private struct SystemRunCommandContractFixture: Decodable { + let cases: [SystemRunCommandContractCase] +} + +private struct SystemRunCommandContractCase: Decodable { + let name: String + let command: [String] + let rawCommand: String? + let expected: SystemRunCommandContractExpected +} + +private struct SystemRunCommandContractExpected: Decodable { + let valid: Bool + let displayCommand: String? + let errorContains: String? +} + struct ExecSystemRunCommandValidatorTests { - @Test func rejectsPayloadOnlyRawForPositionalCarrierWrappers() { - let command = ["/bin/sh", "-lc", #"$0 "$1""#, "/usr/bin/touch", "/tmp/marker"] - let result = ExecSystemRunCommandValidator.resolve(command: command, rawCommand: #"$0 "$1""#) - switch result { - case .ok: - Issue.record("expected rawCommand mismatch") - case .invalid(let message): - #expect(message.contains("rawCommand does not match command")) + @Test func matchesSharedSystemRunCommandContractFixture() throws { + for entry in try Self.loadContractCases() { + let result = ExecSystemRunCommandValidator.resolve(command: entry.command, rawCommand: entry.rawCommand) + + if !entry.expected.valid { + switch result { + case .ok(let resolved): + Issue.record("\(entry.name): expected invalid result, got displayCommand=\(resolved.displayCommand)") + case .invalid(let message): + if let expected = entry.expected.errorContains { + #expect( + message.contains(expected), + "\(entry.name): expected error containing \(expected), got \(message)") + } + } + continue + } + + switch result { + case .ok(let resolved): + #expect( + resolved.displayCommand == entry.expected.displayCommand, + "\(entry.name): unexpected display command") + case .invalid(let message): + Issue.record("\(entry.name): unexpected invalid result: \(message)") + } } } - @Test func acceptsCanonicalDisplayForPositionalCarrierWrappers() { - let command = ["/bin/sh", "-lc", #"$0 "$1""#, "/usr/bin/touch", "/tmp/marker"] - let expected = ExecCommandFormatter.displayString(for: command) - let result = ExecSystemRunCommandValidator.resolve(command: command, rawCommand: expected) - switch result { - case .ok(let resolved): - #expect(resolved.displayCommand == expected) - case .invalid(let message): - Issue.record("unexpected validation failure: \(message)") - } + private static func loadContractCases() throws -> [SystemRunCommandContractCase] { + let fixtureURL = try self.findContractFixtureURL() + let data = try Data(contentsOf: fixtureURL) + let decoded = try JSONDecoder().decode(SystemRunCommandContractFixture.self, from: data) + return decoded.cases } - @Test func acceptsShellPayloadRawForTransparentEnvWrapper() { - let command = ["/usr/bin/env", "bash", "-lc", "echo hi"] - let result = ExecSystemRunCommandValidator.resolve(command: command, rawCommand: "echo hi") - switch result { - case .ok(let resolved): - #expect(resolved.displayCommand == "echo hi") - case .invalid(let message): - Issue.record("unexpected validation failure: \(message)") - } - } - - @Test func rejectsShellPayloadRawForEnvModifierPrelude() { - let command = ["/usr/bin/env", "BASH_ENV=/tmp/payload.sh", "bash", "-lc", "echo hi"] - let result = ExecSystemRunCommandValidator.resolve(command: command, rawCommand: "echo hi") - switch result { - case .ok: - Issue.record("expected rawCommand mismatch") - case .invalid(let message): - #expect(message.contains("rawCommand does not match command")) + private static func findContractFixtureURL() throws -> URL { + var cursor = URL(fileURLWithPath: #filePath).deletingLastPathComponent() + for _ in 0..<8 { + let candidate = cursor + .appendingPathComponent("test") + .appendingPathComponent("fixtures") + .appendingPathComponent("system-run-command-contract.json") + if FileManager.default.fileExists(atPath: candidate.path) { + return candidate + } + cursor.deleteLastPathComponent() } + throw NSError( + domain: "ExecSystemRunCommandValidatorTests", + code: 1, + userInfo: [NSLocalizedDescriptionKey: "missing shared system-run command contract fixture"]) } } diff --git a/src/infra/system-run-command.contract.test.ts b/src/infra/system-run-command.contract.test.ts new file mode 100644 index 00000000000..a0555355d42 --- /dev/null +++ b/src/infra/system-run-command.contract.test.ts @@ -0,0 +1,54 @@ +import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, expect, test } from "vitest"; +import { resolveSystemRunCommand } from "./system-run-command.js"; + +type ContractFixture = { + cases: ContractCase[]; +}; + +type ContractCase = { + name: string; + command: string[]; + rawCommand?: string; + expected: { + valid: boolean; + displayCommand?: string; + errorContains?: string; + }; +}; + +const fixturePath = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../test/fixtures/system-run-command-contract.json", +); +const fixture = JSON.parse(fs.readFileSync(fixturePath, "utf8")) as ContractFixture; + +describe("system-run command contract fixtures", () => { + for (const entry of fixture.cases) { + test(entry.name, () => { + const result = resolveSystemRunCommand({ + command: entry.command, + rawCommand: entry.rawCommand, + }); + + if (!entry.expected.valid) { + expect(result.ok).toBe(false); + if (result.ok) { + throw new Error("expected validation failure"); + } + if (entry.expected.errorContains) { + expect(result.message).toContain(entry.expected.errorContains); + } + return; + } + + expect(result.ok).toBe(true); + if (!result.ok) { + throw new Error(`unexpected validation failure: ${result.message}`); + } + expect(result.cmdText).toBe(entry.expected.displayCommand); + }); + } +}); diff --git a/src/node-host/invoke-system-run.ts b/src/node-host/invoke-system-run.ts index c9b107a5d31..39e6766f7d5 100644 --- a/src/node-host/invoke-system-run.ts +++ b/src/node-host/invoke-system-run.ts @@ -55,6 +55,37 @@ type SystemRunAllowlistAnalysis = { segments: ExecCommandSegment[]; }; +type ResolvedExecApprovals = ReturnType; + +type SystemRunParsePhase = { + argv: string[]; + shellCommand: string | null; + cmdText: string; + agentId: string | undefined; + sessionKey: string; + runId: string; + execution: SystemRunExecutionContext; + approvalDecision: ReturnType; + envOverrides: Record | undefined; + env: Record | undefined; + cwd: string | undefined; + timeoutMs: number | undefined; + needsScreenRecording: boolean; + approved: boolean; +}; + +type SystemRunPolicyPhase = SystemRunParsePhase & { + approvals: ResolvedExecApprovals; + security: ExecSecurity; + policy: ReturnType; + allowlistMatches: ExecAllowlistEntry[]; + analysisOk: boolean; + allowlistSatisfied: boolean; + segments: ExecCommandSegment[]; + plannedAllowlistArgv: string[] | undefined; + isWindows: boolean; +}; + const safeBinTrustedDirWarningCache = new Set(); function warnWritableTrustedDirOnce(message: string): void { @@ -270,7 +301,9 @@ function applyOutputTruncation(result: RunResult) { export { formatSystemRunAllowlistMissMessage } from "./exec-policy.js"; -export async function handleSystemRunInvoke(opts: HandleSystemRunInvokeOptions): Promise { +async function parseSystemRunPhase( + opts: HandleSystemRunInvokeOptions, +): Promise { const command = resolveSystemRunCommand({ command: opts.params.command, rawCommand: opts.params.rawCommand, @@ -280,42 +313,62 @@ export async function handleSystemRunInvoke(opts: HandleSystemRunInvokeOptions): ok: false, error: { code: "INVALID_REQUEST", message: command.message }, }); - return; + return null; } if (command.argv.length === 0) { await opts.sendInvokeResult({ ok: false, error: { code: "INVALID_REQUEST", message: "command required" }, }); - return; + return null; } - const argv = command.argv; const shellCommand = command.shellCommand; const cmdText = command.cmdText; const agentId = opts.params.agentId?.trim() || undefined; + const sessionKey = opts.params.sessionKey?.trim() || "node"; + const runId = opts.params.runId?.trim() || crypto.randomUUID(); + const envOverrides = sanitizeSystemRunEnvOverrides({ + overrides: opts.params.env ?? undefined, + shellWrapper: shellCommand !== null, + }); + return { + argv: command.argv, + shellCommand, + cmdText, + agentId, + sessionKey, + runId, + execution: { sessionKey, runId, cmdText }, + approvalDecision: resolveExecApprovalDecision(opts.params.approvalDecision), + envOverrides, + env: opts.sanitizeEnv(envOverrides), + cwd: opts.params.cwd?.trim() || undefined, + timeoutMs: opts.params.timeoutMs ?? undefined, + needsScreenRecording: opts.params.needsScreenRecording === true, + approved: opts.params.approved === true, + }; +} + +async function evaluateSystemRunPolicyPhase( + opts: HandleSystemRunInvokeOptions, + parsed: SystemRunParsePhase, +): Promise { const cfg = loadConfig(); - const agentExec = agentId ? resolveAgentConfig(cfg, agentId)?.tools?.exec : undefined; + const agentExec = parsed.agentId + ? resolveAgentConfig(cfg, parsed.agentId)?.tools?.exec + : undefined; const configuredSecurity = opts.resolveExecSecurity( agentExec?.security ?? cfg.tools?.exec?.security, ); const configuredAsk = opts.resolveExecAsk(agentExec?.ask ?? cfg.tools?.exec?.ask); - const approvals = resolveExecApprovals(agentId, { + const approvals = resolveExecApprovals(parsed.agentId, { security: configuredSecurity, ask: configuredAsk, }); const security = approvals.agent.security; const ask = approvals.agent.ask; const autoAllowSkills = approvals.agent.autoAllowSkills; - const sessionKey = opts.params.sessionKey?.trim() || "node"; - const runId = opts.params.runId?.trim() || crypto.randomUUID(); - const execution: SystemRunExecutionContext = { sessionKey, runId, cmdText }; - const approvalDecision = resolveExecApprovalDecision(opts.params.approvalDecision); - const envOverrides = sanitizeSystemRunEnvOverrides({ - overrides: opts.params.env ?? undefined, - shellWrapper: shellCommand !== null, - }); - const env = opts.sanitizeEnv(envOverrides); const { safeBins, safeBinProfiles, trustedSafeBinDirs } = resolveExecSafeBinRuntimePolicy({ global: cfg.tools?.exec, local: agentExec, @@ -323,99 +376,124 @@ export async function handleSystemRunInvoke(opts: HandleSystemRunInvokeOptions): }); const bins = autoAllowSkills ? await opts.skillBins.current() : []; let { analysisOk, allowlistMatches, allowlistSatisfied, segments } = evaluateSystemRunAllowlist({ - shellCommand, - argv, + shellCommand: parsed.shellCommand, + argv: parsed.argv, approvals, security, safeBins, safeBinProfiles, trustedSafeBinDirs, - cwd: opts.params.cwd ?? undefined, - env, + cwd: parsed.cwd, + env: parsed.env, skillBins: bins, autoAllowSkills, }); const isWindows = process.platform === "win32"; - const cmdInvocation = shellCommand + const cmdInvocation = parsed.shellCommand ? opts.isCmdExeInvocation(segments[0]?.argv ?? []) - : opts.isCmdExeInvocation(argv); + : opts.isCmdExeInvocation(parsed.argv); const policy = evaluateSystemRunPolicy({ security, ask, analysisOk, allowlistSatisfied, - approvalDecision, - approved: opts.params.approved === true, + approvalDecision: parsed.approvalDecision, + approved: parsed.approved, isWindows, cmdInvocation, - shellWrapperInvocation: shellCommand !== null, + shellWrapperInvocation: parsed.shellCommand !== null, }); analysisOk = policy.analysisOk; allowlistSatisfied = policy.allowlistSatisfied; if (!policy.allowed) { - await sendSystemRunDenied(opts, execution, { + await sendSystemRunDenied(opts, parsed.execution, { reason: policy.eventReason, message: policy.errorMessage, }); - return; + return null; } // Fail closed if policy/runtime drift re-allows unapproved shell wrappers. - if (security === "allowlist" && shellCommand && !policy.approvedByAsk) { - await sendSystemRunDenied(opts, execution, { + if (security === "allowlist" && parsed.shellCommand && !policy.approvedByAsk) { + await sendSystemRunDenied(opts, parsed.execution, { reason: "approval-required", message: "SYSTEM_RUN_DENIED: approval required", }); - return; + return null; } const plannedAllowlistArgv = resolvePlannedAllowlistArgv({ security, - shellCommand, + shellCommand: parsed.shellCommand, policy, segments, }); if (plannedAllowlistArgv === null) { - await sendSystemRunDenied(opts, execution, { + await sendSystemRunDenied(opts, parsed.execution, { reason: "execution-plan-miss", message: "SYSTEM_RUN_DENIED: execution plan mismatch", }); - return; + return null; } + return { + ...parsed, + approvals, + security, + policy, + allowlistMatches, + analysisOk, + allowlistSatisfied, + segments, + plannedAllowlistArgv: plannedAllowlistArgv ?? undefined, + isWindows, + }; +} +async function executeSystemRunPhase( + opts: HandleSystemRunInvokeOptions, + phase: SystemRunPolicyPhase, +): Promise { const useMacAppExec = opts.preferMacAppExecHost; if (useMacAppExec) { const execRequest: ExecHostRequest = { - command: plannedAllowlistArgv ?? argv, + command: phase.plannedAllowlistArgv ?? phase.argv, // Forward canonical display text so companion approval/prompt surfaces bind to // the exact command context already validated on the node-host. - rawCommand: cmdText || null, - cwd: opts.params.cwd ?? null, - env: envOverrides ?? null, - timeoutMs: opts.params.timeoutMs ?? null, - needsScreenRecording: opts.params.needsScreenRecording ?? null, - agentId: agentId ?? null, - sessionKey: sessionKey ?? null, - approvalDecision, + rawCommand: phase.cmdText || null, + cwd: phase.cwd ?? null, + env: phase.envOverrides ?? null, + timeoutMs: phase.timeoutMs ?? null, + needsScreenRecording: phase.needsScreenRecording, + agentId: phase.agentId ?? null, + sessionKey: phase.sessionKey ?? null, + approvalDecision: phase.approvalDecision, }; - const response = await opts.runViaMacAppExecHost({ approvals, request: execRequest }); + const response = await opts.runViaMacAppExecHost({ + approvals: phase.approvals, + request: execRequest, + }); if (!response) { if (opts.execHostEnforced || !opts.execHostFallbackAllowed) { - await sendSystemRunDenied(opts, execution, { + await sendSystemRunDenied(opts, phase.execution, { reason: "companion-unavailable", message: "COMPANION_APP_UNAVAILABLE: macOS app exec host unreachable", }); return; } } else if (!response.ok) { - await sendSystemRunDenied(opts, execution, { + await sendSystemRunDenied(opts, phase.execution, { reason: normalizeDeniedReason(response.error.reason), message: response.error.message, }); return; } else { const result: ExecHostRunResult = response.payload; - await opts.sendExecFinishedEvent({ sessionKey, runId, cmdText, result }); + await opts.sendExecFinishedEvent({ + sessionKey: phase.sessionKey, + runId: phase.runId, + cmdText: phase.cmdText, + result, + }); await opts.sendInvokeResult({ ok: true, payloadJSON: JSON.stringify(result), @@ -424,41 +502,41 @@ export async function handleSystemRunInvoke(opts: HandleSystemRunInvokeOptions): } } - if (policy.approvalDecision === "allow-always" && security === "allowlist") { - if (policy.analysisOk) { + if (phase.policy.approvalDecision === "allow-always" && phase.security === "allowlist") { + if (phase.policy.analysisOk) { const patterns = resolveAllowAlwaysPatterns({ - segments, - cwd: opts.params.cwd ?? undefined, - env, + segments: phase.segments, + cwd: phase.cwd, + env: phase.env, platform: process.platform, }); for (const pattern of patterns) { if (pattern) { - addAllowlistEntry(approvals.file, agentId, pattern); + addAllowlistEntry(phase.approvals.file, phase.agentId, pattern); } } } } - if (allowlistMatches.length > 0) { + if (phase.allowlistMatches.length > 0) { const seen = new Set(); - for (const match of allowlistMatches) { + for (const match of phase.allowlistMatches) { if (!match?.pattern || seen.has(match.pattern)) { continue; } seen.add(match.pattern); recordAllowlistUse( - approvals.file, - agentId, + phase.approvals.file, + phase.agentId, match, - cmdText, - segments[0]?.resolution?.resolvedPath, + phase.cmdText, + phase.segments[0]?.resolution?.resolvedPath, ); } } - if (opts.params.needsScreenRecording === true) { - await sendSystemRunDenied(opts, execution, { + if (phase.needsScreenRecording) { + await sendSystemRunDenied(opts, phase.execution, { reason: "permission:screenRecording", message: "PERMISSION_MISSING: screenRecording", }); @@ -466,23 +544,23 @@ export async function handleSystemRunInvoke(opts: HandleSystemRunInvokeOptions): } const execArgv = resolveSystemRunExecArgv({ - plannedAllowlistArgv: plannedAllowlistArgv ?? undefined, - argv, - security, - isWindows, - policy, - shellCommand, - segments, + plannedAllowlistArgv: phase.plannedAllowlistArgv, + argv: phase.argv, + security: phase.security, + isWindows: phase.isWindows, + policy: phase.policy, + shellCommand: phase.shellCommand, + segments: phase.segments, }); - const result = await opts.runCommand( - execArgv, - opts.params.cwd?.trim() || undefined, - env, - opts.params.timeoutMs ?? undefined, - ); + const result = await opts.runCommand(execArgv, phase.cwd, phase.env, phase.timeoutMs); applyOutputTruncation(result); - await opts.sendExecFinishedEvent({ sessionKey, runId, cmdText, result }); + await opts.sendExecFinishedEvent({ + sessionKey: phase.sessionKey, + runId: phase.runId, + cmdText: phase.cmdText, + result, + }); await opts.sendInvokeResult({ ok: true, @@ -496,3 +574,15 @@ export async function handleSystemRunInvoke(opts: HandleSystemRunInvokeOptions): }), }); } + +export async function handleSystemRunInvoke(opts: HandleSystemRunInvokeOptions): Promise { + const parsed = await parseSystemRunPhase(opts); + if (!parsed) { + return; + } + const policyPhase = await evaluateSystemRunPolicyPhase(opts, parsed); + if (!policyPhase) { + return; + } + await executeSystemRunPhase(opts, policyPhase); +} diff --git a/test/fixtures/system-run-command-contract.json b/test/fixtures/system-run-command-contract.json new file mode 100644 index 00000000000..60b76bf1bf4 --- /dev/null +++ b/test/fixtures/system-run-command-contract.json @@ -0,0 +1,75 @@ +{ + "cases": [ + { + "name": "direct argv infers display command", + "command": ["echo", "hi there"], + "expected": { + "valid": true, + "displayCommand": "echo \"hi there\"" + } + }, + { + "name": "direct argv rejects mismatched raw command", + "command": ["uname", "-a"], + "rawCommand": "echo hi", + "expected": { + "valid": false, + "errorContains": "rawCommand does not match command" + } + }, + { + "name": "shell wrapper accepts shell payload raw command when no positional argv carriers", + "command": ["/bin/sh", "-lc", "echo hi"], + "rawCommand": "echo hi", + "expected": { + "valid": true, + "displayCommand": "echo hi" + } + }, + { + "name": "shell wrapper positional argv carrier requires full argv display binding", + "command": ["/bin/sh", "-lc", "$0 \"$1\"", "/usr/bin/touch", "/tmp/marker"], + "rawCommand": "$0 \"$1\"", + "expected": { + "valid": false, + "errorContains": "rawCommand does not match command" + } + }, + { + "name": "shell wrapper positional argv carrier accepts canonical full argv raw command", + "command": ["/bin/sh", "-lc", "$0 \"$1\"", "/usr/bin/touch", "/tmp/marker"], + "rawCommand": "/bin/sh -lc \"$0 \\\"$1\\\"\" /usr/bin/touch /tmp/marker", + "expected": { + "valid": true, + "displayCommand": "/bin/sh -lc \"$0 \\\"$1\\\"\" /usr/bin/touch /tmp/marker" + } + }, + { + "name": "env wrapper shell payload accepted when prelude has no env modifiers", + "command": ["/usr/bin/env", "bash", "-lc", "echo hi"], + "rawCommand": "echo hi", + "expected": { + "valid": true, + "displayCommand": "echo hi" + } + }, + { + "name": "env assignment prelude requires full argv display binding", + "command": ["/usr/bin/env", "BASH_ENV=/tmp/payload.sh", "bash", "-lc", "echo hi"], + "rawCommand": "echo hi", + "expected": { + "valid": false, + "errorContains": "rawCommand does not match command" + } + }, + { + "name": "env assignment prelude accepts canonical full argv raw command", + "command": ["/usr/bin/env", "BASH_ENV=/tmp/payload.sh", "bash", "-lc", "echo hi"], + "rawCommand": "/usr/bin/env BASH_ENV=/tmp/payload.sh bash -lc \"echo hi\"", + "expected": { + "valid": true, + "displayCommand": "/usr/bin/env BASH_ENV=/tmp/payload.sh bash -lc \"echo hi\"" + } + } + ] +}