From dd9d0bdd8ef6bbf6a3eb932c2c854522dcdab6b4 Mon Sep 17 00:00:00 2001 From: scoootscooob <167050519+scoootscooob@users.noreply.github.com> Date: Mon, 30 Mar 2026 15:49:24 -0700 Subject: [PATCH] fix(exec): harden shell-side approval guardrails (#57839) * fix(exec): harden approval handling * fix(exec): tighten approval guardrails * fix(exec): reject prefixed approval commands * fix(exec): isolate shell approval guardrails * fix(exec): recurse through wrapped approval commands * fix(exec): restore allowlist wrapper import * fix(exec): strip env wrappers before approval detection * fix(exec): inspect nested shell wrapper options --- .../bash-tools.exec-approval-followup.test.ts | 119 ++++---- .../bash-tools.exec.approval-id.test.ts | 51 +++- src/agents/bash-tools.exec.path.test.ts | 27 ++ src/agents/bash-tools.exec.ts | 258 ++++++++++++++++++ src/agents/system-prompt.test.ts | 10 + src/agents/system-prompt.ts | 3 +- src/infra/exec-approvals-allowlist.ts | 237 +++++++++++++++- src/infra/exec-approvals-analysis.test.ts | 94 +++++++ src/infra/exec-approvals-analysis.ts | 2 +- 9 files changed, 732 insertions(+), 69 deletions(-) diff --git a/src/agents/bash-tools.exec-approval-followup.test.ts b/src/agents/bash-tools.exec-approval-followup.test.ts index 5a6388c65e7..c5786735f83 100644 --- a/src/agents/bash-tools.exec-approval-followup.test.ts +++ b/src/agents/bash-tools.exec-approval-followup.test.ts @@ -1,69 +1,80 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("./tools/gateway.js", () => ({ + callGatewayTool: vi.fn(async () => ({ ok: true })), +})); -let sendExecApprovalFollowup: typeof import("./bash-tools.exec-approval-followup.js").sendExecApprovalFollowup; let callGatewayTool: typeof import("./tools/gateway.js").callGatewayTool; +let buildExecApprovalFollowupPrompt: typeof import("./bash-tools.exec-approval-followup.js").buildExecApprovalFollowupPrompt; +let sendExecApprovalFollowup: typeof import("./bash-tools.exec-approval-followup.js").sendExecApprovalFollowup; -describe("sendExecApprovalFollowup", () => { - beforeEach(async () => { - vi.resetModules(); - vi.doMock("./tools/gateway.js", () => ({ - callGatewayTool: vi.fn(), - })); - ({ sendExecApprovalFollowup } = await import("./bash-tools.exec-approval-followup.js")); - ({ callGatewayTool } = await import("./tools/gateway.js")); - vi.mocked(callGatewayTool).mockReset(); - vi.mocked(callGatewayTool).mockResolvedValue({}); +beforeEach(async () => { + vi.resetModules(); + ({ callGatewayTool } = await import("./tools/gateway.js")); + ({ buildExecApprovalFollowupPrompt, sendExecApprovalFollowup } = await import( + "./bash-tools.exec-approval-followup.js" + )); +}); + +afterEach(() => { + vi.resetAllMocks(); +}); + +describe("exec approval followup", () => { + it("uses an explicit denial prompt when the command did not run", () => { + const prompt = buildExecApprovalFollowupPrompt( + "Exec denied (gateway id=req-1, user-denied): uname -a", + ); + + expect(prompt).toContain("did not run"); + expect(prompt).toContain("Do not mention, summarize, or reuse output"); + expect(prompt).not.toContain("already approved has completed"); }); - it("keeps followup session-only when no external delivery target exists", async () => { - const ok = await sendExecApprovalFollowup({ - approvalId: "approval-1", + it("keeps followups internal when no external route is available", async () => { + await sendExecApprovalFollowup({ + approvalId: "req-1", sessionKey: "agent:main:main", - resultText: "Exec finished (gateway id=approval-1, code 0)", + resultText: "Exec completed: echo ok", }); - expect(ok).toBe(true); - expect(callGatewayTool).toHaveBeenCalledTimes(1); - const payload = vi.mocked(callGatewayTool).mock.calls[0]?.[2] as Record; - expect(payload.deliver).toBe(false); - expect(payload).not.toHaveProperty("bestEffortDeliver"); - expect(payload.channel).toBeUndefined(); - expect(payload.to).toBeUndefined(); + expect(callGatewayTool).toHaveBeenCalledWith( + "agent", + expect.any(Object), + expect.objectContaining({ + sessionKey: "agent:main:main", + deliver: false, + channel: undefined, + to: undefined, + }), + { expectFinal: true }, + ); }); - it("keeps followup session-only when turn source is internal webchat", async () => { - const ok = await sendExecApprovalFollowup({ - approvalId: "approval-2", - sessionKey: "agent:main:main", - turnSourceChannel: "webchat", - turnSourceTo: "chat:123", - resultText: "Exec finished (gateway id=approval-2, code 0)", - }); - - expect(ok).toBe(true); - const payload = vi.mocked(callGatewayTool).mock.calls[0]?.[2] as Record; - expect(payload.deliver).toBe(false); - expect(payload).not.toHaveProperty("bestEffortDeliver"); - expect(payload.channel).toBe("webchat"); - expect(payload.to).toBe("chat:123"); - }); - - it("enables delivery for valid external turn source targets", async () => { - const ok = await sendExecApprovalFollowup({ - approvalId: "approval-3", - sessionKey: "agent:main:main", - turnSourceChannel: " discord ", - turnSourceTo: "channel:123", + it("uses external delivery when a deliverable route is available", async () => { + await sendExecApprovalFollowup({ + approvalId: "req-2", + sessionKey: "agent:main:discord:channel:123", + turnSourceChannel: "discord", + turnSourceTo: "123", turnSourceAccountId: "default", - resultText: "Exec finished (gateway id=approval-3, code 0)", + turnSourceThreadId: "456", + resultText: "Exec completed: echo ok", }); - expect(ok).toBe(true); - const payload = vi.mocked(callGatewayTool).mock.calls[0]?.[2] as Record; - expect(payload.deliver).toBe(true); - expect(payload.bestEffortDeliver).toBe(true); - expect(payload.channel).toBe("discord"); - expect(payload.to).toBe("channel:123"); - expect(payload.accountId).toBe("default"); + expect(callGatewayTool).toHaveBeenCalledWith( + "agent", + expect.any(Object), + expect.objectContaining({ + sessionKey: "agent:main:discord:channel:123", + deliver: true, + bestEffortDeliver: true, + channel: "discord", + to: "123", + accountId: "default", + threadId: "456", + }), + { expectFinal: true }, + ); }); }); diff --git a/src/agents/bash-tools.exec.approval-id.test.ts b/src/agents/bash-tools.exec.approval-id.test.ts index f490a5deb38..5347089a092 100644 --- a/src/agents/bash-tools.exec.approval-id.test.ts +++ b/src/agents/bash-tools.exec.approval-id.test.ts @@ -433,7 +433,7 @@ describe("exec approvals", () => { expect(calls).toContain("exec.approval.waitDecision"); }); - it("starts a direct agent follow-up after approved gateway exec completes", async () => { + it("starts an internal agent follow-up after approved gateway exec completes without an external route", async () => { const agentCalls: Array> = []; mockAcceptedApprovalFlow({ @@ -639,6 +639,55 @@ describe("exec approvals", () => { expect(calls).toContain("exec.approval.request"); }); + it("runs a skill wrapper chain without prompting when the wrapper is allowlisted", async () => { + if (process.platform === "win32") { + return; + } + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skill-wrapper-")); + try { + const skillDir = path.join(tempDir, ".openclaw", "skills", "gog"); + const skillPath = path.join(skillDir, "SKILL.md"); + const binDir = path.join(tempDir, "bin"); + const wrapperPath = path.join(binDir, "gog-wrapper"); + await fs.mkdir(skillDir, { recursive: true }); + await fs.mkdir(binDir, { recursive: true }); + await fs.writeFile(skillPath, "# gog skill\n"); + await fs.writeFile(wrapperPath, "#!/bin/sh\necho '{\"events\":[]}'\n"); + await fs.chmod(wrapperPath, 0o755); + + await writeExecApprovalsConfig({ + version: 1, + defaults: { security: "allowlist", ask: "off", askFallback: "deny" }, + agents: { + main: { + allowlist: [{ pattern: wrapperPath }], + }, + }, + }); + + const calls: string[] = []; + mockGatewayOkCalls(calls); + + const tool = createExecTool({ + host: "gateway", + ask: "off", + security: "allowlist", + approvalRunningNoticeMs: 0, + }); + + const result = await tool.execute("call-skill-wrapper", { + command: `cat ${JSON.stringify(skillPath)} && printf '\\n---CMD---\\n' && ${JSON.stringify(wrapperPath)} calendar events primary --today --json`, + workdir: tempDir, + }); + + expect(result.details.status).toBe("completed"); + expect(getResultText(result)).toContain('{"events":[]}'); + expect(calls).not.toContain("exec.approval.request"); + } finally { + await fs.rm(tempDir, { recursive: true, force: true }); + } + }); + it("shows full chained node commands in approval-pending message", async () => { const calls: string[] = []; vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => { diff --git a/src/agents/bash-tools.exec.path.test.ts b/src/agents/bash-tools.exec.path.test.ts index 9fa68d24443..e4299c3136c 100644 --- a/src/agents/bash-tools.exec.path.test.ts +++ b/src/agents/bash-tools.exec.path.test.ts @@ -269,4 +269,31 @@ describe("exec host env validation", () => { }), ).rejects.toThrow(/requires a sandbox runtime/); }); + + it.each([ + "echo ok && /approve abc123 allow-once", + "echo ok | /approve abc123 deny", + "echo ok\n/approve abc123 allow-once", + "FOO=1 /approve abc123 allow-once", + "env -i /approve abc123 deny", + "env --ignore-environment /approve abc123 allow-once", + "env -i FOO=1 /approve abc123 allow-once", + "env -S '/approve abc123 deny'", + "command /approve abc123 deny", + "command -p /approve abc123 deny", + "exec -a openclaw /approve abc123 deny", + "sudo /approve abc123 allow-once", + "sudo -E /approve abc123 allow-once", + "bash -lc '/approve abc123 deny'", + "bash -c 'sudo /approve abc123 allow-once'", + "sh -c '/approve abc123 allow-once'", + ])("rejects /approve shell commands in %s", async (command) => { + const tool = createExecTool({ host: "gateway", security: "full", ask: "off" }); + + await expect( + tool.execute("call-approve", { + command, + }), + ).rejects.toThrow(/exec cannot run \/approve commands/); + }); }); diff --git a/src/agents/bash-tools.exec.ts b/src/agents/bash-tools.exec.ts index 7010be52272..e22e3e7aa2f 100644 --- a/src/agents/bash-tools.exec.ts +++ b/src/agents/bash-tools.exec.ts @@ -1,6 +1,7 @@ import fs from "node:fs/promises"; import path from "node:path"; import type { AgentTool, AgentToolResult } from "@mariozechner/pi-agent-core"; +import { analyzeShellCommand } from "../infra/exec-approvals-analysis.js"; import { type ExecHost, loadExecApprovals, maxAsk, minSecurity } from "../infra/exec-approvals.js"; import { resolveExecSafeBinRuntimePolicy } from "../infra/exec-safe-bin-runtime-policy.js"; import { sanitizeHostExecEnvWithDiagnostics } from "../infra/host-env-security.js"; @@ -10,6 +11,7 @@ import { } from "../infra/shell-env.js"; import { logInfo } from "../logger.js"; import { parseAgentSessionKey, resolveAgentIdFromSessionKey } from "../routing/session-key.js"; +import { splitShellArgs } from "../utils/shell-argv.js"; import { markBackgrounded } from "./bash-process-registry.js"; import { processGatewayAllowlist } from "./bash-tools.exec-host-gateway.js"; import { executeNodeHostCommand } from "./bash-tools.exec-host-node.js"; @@ -173,6 +175,261 @@ async function validateScriptFileForShellBleed(params: { } } +type ParsedExecApprovalCommand = { + approvalId: string; + decision: "allow-once" | "allow-always" | "deny"; +}; + +function parseExecApprovalShellCommand(raw: string): ParsedExecApprovalCommand | null { + const normalized = raw.trimStart(); + const match = normalized.match( + /^\/approve(?:@[^\s]+)?\s+([A-Za-z0-9][A-Za-z0-9._:-]*)\s+(allow-once|allow-always|always|deny)\b/i, + ); + if (!match) { + return null; + } + return { + approvalId: match[1], + decision: + match[2].toLowerCase() === "always" + ? "allow-always" + : (match[2].toLowerCase() as ParsedExecApprovalCommand["decision"]), + }; +} + +function rejectExecApprovalShellCommand(command: string): void { + const isEnvAssignmentToken = (token: string): boolean => + /^[A-Za-z_][A-Za-z0-9_]*=.*$/u.test(token); + const shellWrappers = new Set(["bash", "dash", "fish", "ksh", "sh", "zsh"]); + const commandStandaloneOptions = new Set(["-p", "-v", "-V"]); + const envOptionsWithValues = new Set([ + "-C", + "-S", + "-u", + "--argv0", + "--block-signal", + "--chdir", + "--default-signal", + "--ignore-signal", + "--split-string", + "--unset", + ]); + const execOptionsWithValues = new Set(["-a"]); + const execStandaloneOptions = new Set(["-c", "-l"]); + const sudoOptionsWithValues = new Set([ + "-C", + "-D", + "-g", + "-p", + "-R", + "-T", + "-U", + "-u", + "--chdir", + "--close-from", + "--group", + "--host", + "--other-user", + "--prompt", + "--role", + "--type", + "--user", + ]); + const sudoStandaloneOptions = new Set(["-A", "-E", "--askpass", "--preserve-env"]); + const extractEnvSplitStringPayload = (argv: string[]): string[] => { + const remaining = [...argv]; + while (remaining[0] && isEnvAssignmentToken(remaining[0])) { + remaining.shift(); + } + if (remaining[0] !== "env") { + return []; + } + remaining.shift(); + const payloads: string[] = []; + while (remaining.length > 0) { + while (remaining[0] && isEnvAssignmentToken(remaining[0])) { + remaining.shift(); + } + const token: string | undefined = remaining[0]; + if (!token) { + break; + } + if (token === "--") { + remaining.shift(); + continue; + } + if (!token.startsWith("-") || token === "-") { + break; + } + const option = remaining.shift()!; + const normalized = option.split("=", 1)[0]; + if (normalized === "-S" || normalized === "--split-string") { + const value = option.includes("=") + ? option.slice(option.indexOf("=") + 1) + : remaining.shift(); + if (value?.trim()) { + payloads.push(value); + } + continue; + } + if (envOptionsWithValues.has(normalized) && !option.includes("=") && remaining[0]) { + remaining.shift(); + } + } + return payloads; + }; + const stripApprovalCommandPrefixes = (argv: string[]): string[] => { + const remaining = [...argv]; + while (remaining.length > 0) { + while (remaining[0] && isEnvAssignmentToken(remaining[0])) { + remaining.shift(); + } + + const token = remaining[0]; + if (!token) { + break; + } + if (token === "--") { + remaining.shift(); + continue; + } + if (token === "env") { + remaining.shift(); + while (remaining.length > 0) { + while (remaining[0] && isEnvAssignmentToken(remaining[0])) { + remaining.shift(); + } + const envToken = remaining[0]; + if (!envToken) { + break; + } + if (envToken === "--") { + remaining.shift(); + continue; + } + if (!envToken.startsWith("-") || envToken === "-") { + break; + } + const option = remaining.shift()!; + const normalized = option.split("=", 1)[0]; + if (envOptionsWithValues.has(normalized) && !option.includes("=") && remaining[0]) { + remaining.shift(); + } + } + continue; + } + if (token === "command" || token === "builtin") { + remaining.shift(); + while (remaining[0]?.startsWith("-")) { + const option = remaining.shift()!; + if (option === "--") { + break; + } + if (!commandStandaloneOptions.has(option.split("=", 1)[0])) { + continue; + } + } + continue; + } + if (token === "exec") { + remaining.shift(); + while (remaining[0]?.startsWith("-")) { + const option = remaining.shift()!; + if (option === "--") { + break; + } + const normalized = option.split("=", 1)[0]; + if (execStandaloneOptions.has(normalized)) { + continue; + } + if (execOptionsWithValues.has(normalized) && !option.includes("=") && remaining[0]) { + remaining.shift(); + } + } + continue; + } + if (token === "sudo") { + remaining.shift(); + while (remaining[0]?.startsWith("-")) { + const option = remaining.shift()!; + if (option === "--") { + break; + } + const normalized = option.split("=", 1)[0]; + if (sudoStandaloneOptions.has(normalized)) { + continue; + } + if (sudoOptionsWithValues.has(normalized) && !option.includes("=") && remaining[0]) { + remaining.shift(); + } + } + continue; + } + break; + } + return remaining; + }; + const extractShellWrapperPayload = (argv: string[]): string[] => { + const [commandName, ...rest] = argv; + if (!commandName || !shellWrappers.has(path.basename(commandName))) { + return []; + } + for (let i = 0; i < rest.length; i += 1) { + const token = rest[i]; + if (!token) { + continue; + } + if (token === "-c" || token === "-lc" || token === "-ic" || token === "-xc") { + return rest[i + 1] ? [rest[i + 1]] : []; + } + if (/^-[^-]*c[^-]*$/u.test(token)) { + return rest[i + 1] ? [rest[i + 1]] : []; + } + } + return []; + }; + const buildCandidates = (argv: string[]): string[] => { + const envSplitCandidates = extractEnvSplitStringPayload(argv).flatMap((payload) => { + const innerArgv = splitShellArgs(payload); + return innerArgv ? buildCandidates(innerArgv) : [payload]; + }); + const stripped = stripApprovalCommandPrefixes(argv); + const shellWrapperCandidates = extractShellWrapperPayload(stripped).flatMap((payload) => { + const innerArgv = splitShellArgs(payload); + return innerArgv ? buildCandidates(innerArgv) : [payload]; + }); + return [ + ...(stripped.length > 0 ? [stripped.join(" ")] : []), + ...envSplitCandidates, + ...shellWrapperCandidates, + ]; + }; + + const rawCommand = command.trim(); + const analysis = analyzeShellCommand({ command: rawCommand }); + const candidates = analysis.ok + ? analysis.segments.flatMap((segment) => buildCandidates(segment.argv)) + : rawCommand + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean) + .flatMap((line) => { + const argv = splitShellArgs(line); + return argv ? buildCandidates(argv) : [line]; + }); + for (const candidate of candidates) { + if (!parseExecApprovalShellCommand(candidate)) { + continue; + } + throw new Error( + [ + "exec cannot run /approve commands.", + "Show the /approve command to the user as chat text, or route it through the approval command handler instead of shell execution.", + ].join(" "), + ); + } +} + export function createExecTool( defaults?: ExecToolDefaults, // oxlint-disable-next-line typescript/no-explicit-any @@ -378,6 +635,7 @@ export function createExecTool( // fall back to the gateway's cwd. The node is responsible for validating its own cwd. workdir = resolveWorkdir(rawWorkdir, warnings); } + rejectExecApprovalShellCommand(params.command); const inheritedBaseEnv = coerceEnv(process.env); const hostEnvResult = diff --git a/src/agents/system-prompt.test.ts b/src/agents/system-prompt.test.ts index 7b5feff8581..57a1ae37486 100644 --- a/src/agents/system-prompt.test.ts +++ b/src/agents/system-prompt.test.ts @@ -149,6 +149,16 @@ describe("buildAgentSystemPrompt", () => { ); }); + it("tells the agent not to execute /approve through exec", () => { + const prompt = buildAgentSystemPrompt({ + workspaceDir: "/tmp/openclaw", + }); + + expect(prompt).toContain( + "Never execute /approve through exec or any other shell/tool path; /approve is a user-facing approval command, not a shell command.", + ); + }); + it("omits skills in minimal prompt mode when skillsPrompt is absent", () => { const prompt = buildAgentSystemPrompt({ workspaceDir: "/tmp/openclaw", diff --git a/src/agents/system-prompt.ts b/src/agents/system-prompt.ts index d5f2893ee25..1d94076f4bd 100644 --- a/src/agents/system-prompt.ts +++ b/src/agents/system-prompt.ts @@ -450,7 +450,8 @@ export function buildAgentSystemPrompt(params: { "Keep narration brief and value-dense; avoid repeating obvious steps.", "Use plain human language for narration unless in a technical context.", "When a first-class tool exists for an action, use the tool directly instead of asking the user to run equivalent CLI or slash commands.", - "When exec returns approval-pending, include the concrete /approve command from tool output (with allow-once|allow-always|deny) and do not ask for a different or rotated code.", + "When exec returns approval-pending, include the concrete /approve command from tool output (with allow-once|allow-always|deny) as plain chat text for the user, and do not ask for a different or rotated code.", + "Never execute /approve through exec or any other shell/tool path; /approve is a user-facing approval command, not a shell command.", "Treat allow-once as single-command only: if another elevated command needs approval, request a fresh /approve and do not claim prior approval covered it.", "When approvals are required, preserve and show the full command/script exactly as provided (including chained operators like &&, ||, |, ;, or multiline shells) so the user can approve what will actually run.", "", diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index ccc3b259aba..ee6ce26ae5b 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -8,10 +8,11 @@ import { resolveCommandResolutionFromArgv, resolvePolicyTargetCandidatePath, resolvePolicyTargetResolution, - splitCommandChain, + splitCommandChainWithOperators, type ExecCommandAnalysis, type ExecCommandSegment, type ExecutableResolution, + type ShellChainOperator, } from "./exec-approvals-analysis.js"; import type { ExecAllowlistEntry } from "./exec-approvals.js"; import { isInterpreterLikeAllowlistPattern } from "./exec-inline-eval.js"; @@ -25,6 +26,7 @@ import { isTrustedSafeBinPath } from "./exec-safe-bin-trust.js"; import { extractShellWrapperInlineCommand, isShellWrapperExecutable, + normalizeExecutableToken, } from "./exec-wrapper-resolution.js"; import { resolveExecWrapperTrustPlan } from "./exec-wrapper-trust-plan.js"; import { expandHomePrefix } from "./home-dir.js"; @@ -107,7 +109,7 @@ export type ExecAllowlistEvaluation = { segmentSatisfiedBy: ExecSegmentSatisfiedBy[]; }; -export type ExecSegmentSatisfiedBy = "allowlist" | "safeBins" | "skills" | null; +export type ExecSegmentSatisfiedBy = "allowlist" | "safeBins" | "skills" | "skillPrelude" | null; export type SkillBinTrustEntry = { name: string; resolvedPath: string; @@ -200,6 +202,163 @@ function isSkillAutoAllowedSegment(params: { return Boolean(params.skillBinTrust.get(executableName)?.has(resolvedPath)); } +function resolveSkillPreludePath(rawPath: string, cwd?: string): string { + const expanded = rawPath.startsWith("~") ? expandHomePrefix(rawPath) : rawPath; + if (path.isAbsolute(expanded)) { + return path.resolve(expanded); + } + return path.resolve(cwd?.trim() || process.cwd(), expanded); +} + +function isSkillMarkdownPreludePath(filePath: string): boolean { + const normalized = filePath.replace(/\\/g, "/"); + const lowerNormalized = normalized.toLowerCase(); + if (!lowerNormalized.endsWith("/skill.md")) { + return false; + } + const parts = lowerNormalized.split("/").filter(Boolean); + if (parts.length < 2) { + return false; + } + for (let index = parts.length - 2; index >= 0; index -= 1) { + if (parts[index] !== "skills") { + continue; + } + const segmentsAfterSkills = parts.length - index - 1; + if (segmentsAfterSkills === 1 || segmentsAfterSkills === 2) { + return true; + } + } + return false; +} + +function resolveSkillMarkdownPreludeId(filePath: string): string | null { + const normalized = filePath.replace(/\\/g, "/"); + const lowerNormalized = normalized.toLowerCase(); + if (!lowerNormalized.endsWith("/skill.md")) { + return null; + } + const parts = lowerNormalized.split("/").filter(Boolean); + if (parts.length < 3) { + return null; + } + for (let index = parts.length - 2; index >= 0; index -= 1) { + if (parts[index] !== "skills") { + continue; + } + if (parts.length - index - 1 !== 2) { + continue; + } + const skillId = parts[index + 1]?.trim(); + return skillId || null; + } + return null; +} + +function isSkillPreludeReadSegment(segment: ExecCommandSegment, cwd?: string): boolean { + const execution = resolveExecutionTargetResolution(segment.resolution); + if (execution?.executableName?.toLowerCase() !== "cat") { + return false; + } + // Keep the display-prelude exception narrow: only a plain `cat <...>/SKILL.md` + // qualifies, not extra argv forms or arbitrary file reads. + if (segment.argv.length !== 2) { + return false; + } + const rawPath = segment.argv[1]?.trim(); + if (!rawPath) { + return false; + } + return isSkillMarkdownPreludePath(resolveSkillPreludePath(rawPath, cwd)); +} + +function isSkillPreludeMarkerSegment(segment: ExecCommandSegment): boolean { + const execution = resolveExecutionTargetResolution(segment.resolution); + if (execution?.executableName?.toLowerCase() !== "printf") { + return false; + } + if (segment.argv.length !== 2) { + return false; + } + const marker = segment.argv[1]; + return marker === "\\n---CMD---\\n" || marker === "\n---CMD---\n"; +} + +function isSkillPreludeSegment(segment: ExecCommandSegment, cwd?: string): boolean { + return isSkillPreludeReadSegment(segment, cwd) || isSkillPreludeMarkerSegment(segment); +} + +function isSkillPreludeOnlyEvaluation( + segments: ExecCommandSegment[], + cwd: string | undefined, +): boolean { + return segments.length > 0 && segments.every((segment) => isSkillPreludeSegment(segment, cwd)); +} + +function resolveSkillPreludeIds( + segments: ExecCommandSegment[], + cwd: string | undefined, +): ReadonlySet { + const skillIds = new Set(); + for (const segment of segments) { + if (!isSkillPreludeReadSegment(segment, cwd)) { + continue; + } + const rawPath = segment.argv[1]?.trim(); + if (!rawPath) { + continue; + } + const skillId = resolveSkillMarkdownPreludeId(resolveSkillPreludePath(rawPath, cwd)); + if (skillId) { + skillIds.add(skillId); + } + } + return skillIds; +} + +function resolveAllowlistedSkillWrapperId(segment: ExecCommandSegment): string | null { + const execution = resolveExecutionTargetResolution(segment.resolution); + const executableName = normalizeExecutableToken( + execution?.executableName ?? segment.argv[0] ?? "", + ); + if (!executableName.endsWith("-wrapper")) { + return null; + } + const skillId = executableName.slice(0, -"-wrapper".length).trim(); + return skillId || null; +} + +function resolveTrustedSkillExecutionIds(params: { + analysis: ExecCommandAnalysis; + evaluation: ExecAllowlistEvaluation; +}): ReadonlySet { + const skillIds = new Set(); + if (!params.evaluation.allowlistSatisfied) { + return skillIds; + } + for (const [index, segment] of params.analysis.segments.entries()) { + const satisfiedBy = params.evaluation.segmentSatisfiedBy[index]; + if (satisfiedBy === "skills") { + const execution = resolveExecutionTargetResolution(segment.resolution); + const executableName = normalizeExecutableToken( + execution?.executableName ?? execution?.rawExecutable ?? segment.argv[0] ?? "", + ); + if (executableName) { + skillIds.add(executableName); + } + continue; + } + if (satisfiedBy !== "allowlist") { + continue; + } + const wrapperSkillId = resolveAllowlistedSkillWrapperId(segment); + if (wrapperSkillId) { + skillIds.add(wrapperSkillId); + } + } + return skillIds; +} + function evaluateSegments( segments: ExecCommandSegment[], params: ExecAllowlistContext, @@ -531,7 +690,9 @@ export function evaluateShellAllowlist( return analysisFailure(); } - const chainParts = isWindowsPlatform(params.platform) ? null : splitCommandChain(params.command); + const chainParts = isWindowsPlatform(params.platform) + ? null + : splitCommandChainWithOperators(params.command); if (!chainParts) { const analysis = analyzeShellCommand({ command: params.command, @@ -552,11 +713,7 @@ export function evaluateShellAllowlist( }; } - const allowlistMatches: ExecAllowlistEntry[] = []; - const segments: ExecCommandSegment[] = []; - const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = []; - - for (const part of chainParts) { + const chainEvaluations = chainParts.map(({ part, opToNext }) => { const analysis = analyzeShellCommand({ command: part, cwd: params.cwd, @@ -564,14 +721,70 @@ export function evaluateShellAllowlist( platform: params.platform, }); if (!analysis.ok) { - return analysisFailure(); + return null; + } + return { + analysis, + evaluation: evaluateExecAllowlist({ analysis, ...allowlistContext }), + opToNext, + }; + }); + if (chainEvaluations.some((entry) => entry === null)) { + return analysisFailure(); + } + + const finalizedEvaluations = chainEvaluations as Array<{ + analysis: ExecCommandAnalysis; + evaluation: ExecAllowlistEvaluation; + opToNext: ShellChainOperator | null; + }>; + const allowSkillPreludeAtIndex = new Set(); + const reachableSkillIds = new Set(); + // Only allow the `cat SKILL.md && printf ...` display prelude when it sits on a + // contiguous `&&` chain that actually reaches a later trusted skill-wrapper execution. + for (let index = finalizedEvaluations.length - 1; index >= 0; index -= 1) { + const { analysis, evaluation, opToNext } = finalizedEvaluations[index]; + const trustedSkillIds = resolveTrustedSkillExecutionIds({ + analysis, + evaluation, + }); + if (trustedSkillIds.size > 0) { + for (const skillId of trustedSkillIds) { + reachableSkillIds.add(skillId); + } + continue; } + const isPreludeOnly = + !evaluation.allowlistSatisfied && isSkillPreludeOnlyEvaluation(analysis.segments, params.cwd); + const preludeSkillIds = isPreludeOnly + ? resolveSkillPreludeIds(analysis.segments, params.cwd) + : new Set(); + const reachesTrustedSkillExecution = + opToNext === "&&" && + (preludeSkillIds.size === 0 + ? reachableSkillIds.size > 0 + : [...preludeSkillIds].some((skillId) => reachableSkillIds.has(skillId))); + if (isPreludeOnly && reachesTrustedSkillExecution) { + allowSkillPreludeAtIndex.add(index); + continue; + } + + reachableSkillIds.clear(); + } + const allowlistMatches: ExecAllowlistEntry[] = []; + const segments: ExecCommandSegment[] = []; + const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = []; + + for (const [index, { analysis, evaluation }] of finalizedEvaluations.entries()) { + const effectiveSegmentSatisfiedBy = allowSkillPreludeAtIndex.has(index) + ? analysis.segments.map(() => "skillPrelude" as const) + : evaluation.segmentSatisfiedBy; + segments.push(...analysis.segments); - const evaluation = evaluateExecAllowlist({ analysis, ...allowlistContext }); allowlistMatches.push(...evaluation.allowlistMatches); - segmentSatisfiedBy.push(...evaluation.segmentSatisfiedBy); - if (!evaluation.allowlistSatisfied) { + segmentSatisfiedBy.push(...effectiveSegmentSatisfiedBy); + if (!evaluation.allowlistSatisfied && !allowSkillPreludeAtIndex.has(index)) { return { analysisOk: true, allowlistSatisfied: false, diff --git a/src/infra/exec-approvals-analysis.test.ts b/src/infra/exec-approvals-analysis.test.ts index b64c7b284a1..40c4a439344 100644 --- a/src/infra/exec-approvals-analysis.test.ts +++ b/src/infra/exec-approvals-analysis.test.ts @@ -293,6 +293,100 @@ describe("exec approvals shell analysis", () => { expect(result.allowlistSatisfied).toBe(testCase.expectedAllowlistSatisfied); }); + it("allows the skill display prelude when a later skill wrapper is allowlisted", () => { + if (process.platform === "win32") { + return; + } + const skillRoot = makeTempDir(); + const skillDir = path.join(skillRoot, "skills", "gog"); + const skillPath = path.join(skillDir, "SKILL.md"); + const wrapperPath = path.join(skillRoot, "bin", "gog-wrapper"); + fs.mkdirSync(path.dirname(skillPath), { recursive: true }); + fs.mkdirSync(path.dirname(wrapperPath), { recursive: true }); + fs.writeFileSync(skillPath, "# gog\n"); + fs.writeFileSync(wrapperPath, "#!/bin/sh\n", { mode: 0o755 }); + + const result = evaluateShellAllowlist({ + command: `cat ${skillPath} && printf '\\n---CMD---\\n' && ${wrapperPath} calendar events primary --today --json`, + allowlist: [{ pattern: wrapperPath }], + safeBins: new Set(), + cwd: skillRoot, + }); + + expect(result.analysisOk).toBe(true); + expect(result.allowlistSatisfied).toBe(true); + expect(result.segmentSatisfiedBy).toEqual(["skillPrelude", "skillPrelude", "allowlist"]); + }); + + it("does not treat arbitrary allowlisted binaries as trusted skill wrappers", () => { + if (process.platform === "win32") { + return; + } + const skillRoot = makeTempDir(); + const skillDir = path.join(skillRoot, "skills", "gog"); + const skillPath = path.join(skillDir, "SKILL.md"); + fs.mkdirSync(skillDir, { recursive: true }); + fs.writeFileSync(skillPath, "# gog\n"); + + const result = evaluateShellAllowlist({ + command: `cat ${skillPath} && printf '\\n---CMD---\\n' && /bin/echo calendar events primary --today --json`, + allowlist: [{ pattern: "/bin/echo" }], + safeBins: new Set(), + cwd: skillRoot, + }); + + expect(result.analysisOk).toBe(true); + expect(result.allowlistSatisfied).toBe(false); + expect(result.segmentSatisfiedBy).toEqual([null]); + }); + + it("still rejects the skill display prelude when no trusted skill command follows", () => { + if (process.platform === "win32") { + return; + } + const skillRoot = makeTempDir(); + const skillDir = path.join(skillRoot, "skills", "gog"); + const skillPath = path.join(skillDir, "SKILL.md"); + fs.mkdirSync(skillDir, { recursive: true }); + fs.writeFileSync(skillPath, "# gog\n"); + + const result = evaluateShellAllowlist({ + command: `cat ${skillPath} && printf '\\n---CMD---\\n'`, + allowlist: [], + safeBins: new Set(), + cwd: skillRoot, + }); + + expect(result.analysisOk).toBe(true); + expect(result.allowlistSatisfied).toBe(false); + expect(result.segmentSatisfiedBy).toEqual([null]); + }); + + it("rejects the skill display prelude when a trusted wrapper is not reachable", () => { + if (process.platform === "win32") { + return; + } + const skillRoot = makeTempDir(); + const skillDir = path.join(skillRoot, "skills", "gog"); + const skillPath = path.join(skillDir, "SKILL.md"); + const wrapperPath = path.join(skillRoot, "bin", "gog-wrapper"); + fs.mkdirSync(path.dirname(skillPath), { recursive: true }); + fs.mkdirSync(path.dirname(wrapperPath), { recursive: true }); + fs.writeFileSync(skillPath, "# gog\n"); + fs.writeFileSync(wrapperPath, "#!/bin/sh\n", { mode: 0o755 }); + + const result = evaluateShellAllowlist({ + command: `cat ${skillPath} && printf '\\n---CMD---\\n' && false && ${wrapperPath} calendar events primary --today --json`, + allowlist: [{ pattern: wrapperPath }], + safeBins: new Set(), + cwd: skillRoot, + }); + + expect(result.analysisOk).toBe(true); + expect(result.allowlistSatisfied).toBe(false); + expect(result.segmentSatisfiedBy).toEqual([null]); + }); + it.each(['/usr/bin/echo "foo && bar"', '/usr/bin/echo "foo\\" && bar"'])( "respects quoted chain separator for %s", (command) => { diff --git a/src/infra/exec-approvals-analysis.ts b/src/infra/exec-approvals-analysis.ts index 1a169f154db..a64cac2209a 100644 --- a/src/infra/exec-approvals-analysis.ts +++ b/src/infra/exec-approvals-analysis.ts @@ -697,7 +697,7 @@ function renderSafeBinSegmentArgv(segment: ExecCommandSegment): string | null { export function buildSafeBinsShellCommand(params: { command: string; segments: ExecCommandSegment[]; - segmentSatisfiedBy: ("allowlist" | "safeBins" | "skills" | null)[]; + segmentSatisfiedBy: ("allowlist" | "safeBins" | "skills" | "skillPrelude" | null)[]; platform?: string | null; }): { ok: boolean; command?: string; reason?: string } { if (params.segments.length !== params.segmentSatisfiedBy.length) {