From 2d53ffdec1da0dc210e75b220749263163fac858 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Wed, 1 Apr 2026 18:07:20 +0900 Subject: [PATCH] fix(exec): resolve remote approval regressions (#58792) * fix(exec): restore remote approval policy defaults * fix(exec): handle headless cron approval conflicts * fix(exec): make allow-always durable * fix(exec): persist exact-command shell trust * fix(doctor): match host exec fallback * fix(exec): preserve blocked and inline approval state * Doctor: surface allow-always ask bypass * Doctor: match effective exec policy * Exec: match node durable command text * Exec: tighten durable approval security * Exec: restore owner approver fallback * Config: refresh Slack approval metadata --------- Co-authored-by: scoootscooob --- CHANGELOG.md | 3 + extensions/discord/src/exec-approvals.test.ts | 36 ++- extensions/discord/src/exec-approvals.ts | 28 +- .../src/monitor/exec-approvals.test.ts | 49 ++- .../discord/src/monitor/exec-approvals.ts | 12 +- extensions/slack/src/config-ui-hints.ts | 2 +- extensions/slack/src/exec-approvals.test.ts | 33 +- extensions/slack/src/exec-approvals.ts | 20 +- .../slack/src/monitor/exec-approvals.test.ts | 43 ++- .../slack/src/monitor/exec-approvals.ts | 25 +- .../bash-tools.exec-host-gateway.test.ts | 124 ++++++++ src/agents/bash-tools.exec-host-gateway.ts | 83 ++++- src/agents/bash-tools.exec-host-node.ts | 245 +++++++++------ src/agents/bash-tools.exec-host-shared.ts | 36 +++ src/agents/bash-tools.exec-types.ts | 1 + .../bash-tools.exec.approval-id.test.ts | 289 +++++++++++++++++- src/agents/bash-tools.exec.ts | 15 +- src/agents/pi-tools.ts | 1 + src/commands/doctor-security.test.ts | 166 ++++++++++ src/commands/doctor-security.ts | 148 +++++++++ ...ndled-channel-config-metadata.generated.ts | 2 +- src/config/types.discord.ts | 2 +- src/config/types.slack.ts | 2 +- src/infra/exec-approvals-allowlist.ts | 41 ++- src/infra/exec-approvals-policy.test.ts | 81 +++++ src/infra/exec-approvals-store.test.ts | 61 ++++ src/infra/exec-approvals.ts | 111 ++++++- src/node-host/exec-policy.test.ts | 14 + src/node-host/exec-policy.ts | 14 + src/node-host/invoke-system-run-allowlist.ts | 3 + src/node-host/invoke-system-run.test.ts | 57 ++++ src/node-host/invoke-system-run.ts | 78 +++-- .../approval-delivery-helpers.test.ts | 7 + src/plugin-sdk/approval-delivery-helpers.ts | 3 +- 34 files changed, 1609 insertions(+), 226 deletions(-) create mode 100644 src/agents/bash-tools.exec-host-gateway.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index f131f3cdba6..bdb7805a56f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ Docs: https://docs.openclaw.ai ### Fixes +- Exec/approvals: honor `exec-approvals.json` security defaults when inline or configured tool policy is unset, and keep Slack and Discord native approval handling aligned with inferred approvers and real channel enablement so remote exec stops falling into false approval timeouts and disabled states. Thanks @scoootscooob and @vincentkoc. +- Exec/cron: resolve isolated cron no-route approval dead-ends from the effective host fallback policy when trusted automation is allowed, and make `openclaw doctor` warn when `tools.exec` is broader than `~/.openclaw/exec-approvals.json` so stricter host-policy conflicts are explicit. Thanks @scoootscooob and @vincentkoc. +- Exec/approvals: make `allow-always` persist as durable user-approved trust instead of behaving like `allow-once`, reuse exact-command trust on shell-wrapper paths that cannot safely persist an executable allowlist entry, keep static allowlist entries from silently bypassing `ask:"always"`, and require explicit approval when Windows cannot build an allowlist execution plan instead of hard-dead-ending remote exec. Thanks @scoootscooob and @vincentkoc. - Gateway/reload: ignore startup config writes by persisted hash in the config reloader so generated auth tokens and seeded Control UI origins do not trigger a restart loop, while real `gateway.auth.*` edits still require restart. (#58678) Thanks @yelog - Discord/inbound media: pass Discord attachment and sticker downloads through the shared idle-timeout and worker-abort path so slow or stuck inbound media fetches stop hanging message processing. (#58593) Thanks @aquaright1 - Telegram/local Bot API: preserve media MIME types for absolute-path downloads so local audio files still trigger transcription and other MIME-based handling. (#54603) Thanks @jzakirov diff --git a/extensions/discord/src/exec-approvals.test.ts b/extensions/discord/src/exec-approvals.test.ts index 57bbc3e2f7a..ee9206ef3f1 100644 --- a/extensions/discord/src/exec-approvals.test.ts +++ b/extensions/discord/src/exec-approvals.test.ts @@ -22,13 +22,26 @@ function buildConfig( } describe("discord exec approvals", () => { - it("requires enablement and an explicit or inferred approver", () => { + it("requires enablement and explicit or owner approvers", () => { expect(isDiscordExecApprovalClientEnabled({ cfg: buildConfig() })).toBe(false); expect(isDiscordExecApprovalClientEnabled({ cfg: buildConfig({ enabled: true }) })).toBe(false); expect( isDiscordExecApprovalClientEnabled({ cfg: buildConfig({ enabled: true }, { allowFrom: ["123"] }), }), + ).toBe(false); + expect( + isDiscordExecApprovalClientEnabled({ + cfg: buildConfig({ enabled: true, approvers: ["123"] }), + }), + ).toBe(true); + expect( + isDiscordExecApprovalClientEnabled({ + cfg: { + ...buildConfig({ enabled: true }), + commands: { ownerAllowFrom: ["discord:789"] }, + } as OpenClawConfig, + }), ).toBe(true); }); @@ -43,7 +56,7 @@ describe("discord exec approvals", () => { expect(isDiscordExecApprovalApprover({ cfg, senderId: "123" })).toBe(false); }); - it("infers approvers from allowFrom, legacy dm.allowFrom, and explicit DM defaultTo", () => { + it("does not infer approvers from allowFrom or default DM routes", () => { const cfg = buildConfig( { enabled: true }, { @@ -53,18 +66,17 @@ describe("discord exec approvals", () => { }, ); - expect(getDiscordExecApprovalApprovers({ cfg })).toEqual(["123", "456", "789"]); - expect(isDiscordExecApprovalApprover({ cfg, senderId: "789" })).toBe(true); + expect(getDiscordExecApprovalApprovers({ cfg })).toEqual([]); + expect(isDiscordExecApprovalApprover({ cfg, senderId: "789" })).toBe(false); }); - it("ignores non-user default targets when inferring approvers", () => { - const cfg = buildConfig( - { enabled: true }, - { - defaultTo: "channel:123", - }, - ); + it("falls back to commands.ownerAllowFrom for exec approvers", () => { + const cfg = { + ...buildConfig({ enabled: true }), + commands: { ownerAllowFrom: ["discord:123", "user:456", "789"] }, + } as OpenClawConfig; - expect(getDiscordExecApprovalApprovers({ cfg })).toEqual([]); + expect(getDiscordExecApprovalApprovers({ cfg })).toEqual(["123", "456", "789"]); + expect(isDiscordExecApprovalApprover({ cfg, senderId: "456" })).toBe(true); }); }); diff --git a/extensions/discord/src/exec-approvals.ts b/extensions/discord/src/exec-approvals.ts index 09d43d4404c..557d9d4cdc2 100644 --- a/extensions/discord/src/exec-approvals.ts +++ b/extensions/discord/src/exec-approvals.ts @@ -22,26 +22,28 @@ function normalizeDiscordApproverId(value: string): string | undefined { } } +function resolveDiscordOwnerApprovers(cfg: OpenClawConfig): string[] { + const ownerAllowFrom = cfg.commands?.ownerAllowFrom; + if (!Array.isArray(ownerAllowFrom) || ownerAllowFrom.length === 0) { + return []; + } + return resolveApprovalApprovers({ + explicit: ownerAllowFrom, + normalizeApprover: (value) => normalizeDiscordApproverId(String(value)), + }); +} + export function getDiscordExecApprovalApprovers(params: { cfg: OpenClawConfig; accountId?: string | null; configOverride?: DiscordExecApprovalConfig | null; }): string[] { - const account = resolveDiscordAccount(params).config; return resolveApprovalApprovers({ - explicit: params.configOverride?.approvers ?? account.execApprovals?.approvers, - allowFrom: account.allowFrom, - extraAllowFrom: account.dm?.allowFrom, - defaultTo: account.defaultTo, + explicit: + params.configOverride?.approvers ?? + resolveDiscordAccount(params).config.execApprovals?.approvers ?? + resolveDiscordOwnerApprovers(params.cfg), normalizeApprover: (value) => normalizeDiscordApproverId(String(value)), - normalizeDefaultTo: (value) => { - try { - const target = parseDiscordTarget(value); - return target?.kind === "user" ? target.id : undefined; - } catch { - return undefined; - } - }, }); } diff --git a/extensions/discord/src/monitor/exec-approvals.test.ts b/extensions/discord/src/monitor/exec-approvals.test.ts index 62f3a73ce20..4280c8cdaf6 100644 --- a/extensions/discord/src/monitor/exec-approvals.test.ts +++ b/extensions/discord/src/monitor/exec-approvals.test.ts @@ -177,12 +177,16 @@ type ExecApprovalButtonContext = import("./exec-approvals.js").ExecApprovalButto // ─── Helpers ────────────────────────────────────────────────────────────────── -function createHandler(config: DiscordExecApprovalConfig, accountId = "default") { +function createHandler( + config: DiscordExecApprovalConfig, + accountId = "default", + cfgOverrides: Record = {}, +) { return new DiscordExecApprovalHandler({ token: "test-token", accountId, config, - cfg: { session: { store: STORE_PATH } }, + cfg: { session: { store: STORE_PATH }, ...cfgOverrides }, }); } @@ -447,6 +451,18 @@ describe("DiscordExecApprovalHandler.shouldHandle", () => { expect(handler.shouldHandle(createRequest())).toBe(false); }); + it("does not treat channel allowFrom as approval authority", () => { + const handler = createHandler({ enabled: true }, "default", { + channels: { + discord: { + token: "discord-token", + allowFrom: ["123"], + }, + }, + }); + expect(handler.shouldHandle(createRequest())).toBe(false); + }); + it("returns true with minimal config", () => { const handler = createHandler({ enabled: true, approvers: ["123"] }); expect(handler.shouldHandle(createRequest())).toBe(true); @@ -617,10 +633,37 @@ describe("DiscordExecApprovalHandler.getApprovers", () => { config: { enabled: true } as DiscordExecApprovalConfig, expected: [], }, + { + name: "allowFrom does not grant approver rights", + config: { enabled: true } as DiscordExecApprovalConfig, + cfgOverrides: { + channels: { + discord: { + token: "discord-token", + allowFrom: ["123"], + }, + }, + }, + expected: [], + }, + { + name: "ownerAllowFrom still grants exec approver rights", + config: { enabled: true } as DiscordExecApprovalConfig, + cfgOverrides: { + commands: { + ownerAllowFrom: ["discord:123"], + }, + }, + expected: ["123"], + }, ] as const; for (const testCase of cases) { - const handler = createHandler(testCase.config); + const handler = createHandler( + testCase.config, + "default", + "cfgOverrides" in testCase ? (testCase.cfgOverrides as Record) : {}, + ); expect(handler.getApprovers(), testCase.name).toEqual(testCase.expected); } }); diff --git a/extensions/discord/src/monitor/exec-approvals.ts b/extensions/discord/src/monitor/exec-approvals.ts index 81d9c12fc85..2b079a1eec6 100644 --- a/extensions/discord/src/monitor/exec-approvals.ts +++ b/extensions/discord/src/monitor/exec-approvals.ts @@ -33,6 +33,7 @@ import type { import type { RuntimeEnv } from "openclaw/plugin-sdk/runtime-env"; import { logDebug, logError } from "openclaw/plugin-sdk/text-runtime"; import { createDiscordNativeApprovalAdapter } from "../approval-native.js"; +import { getDiscordExecApprovalApprovers } from "../exec-approvals.js"; import { createDiscordClient, stripUndefinedFields } from "../send.shared.js"; import { DiscordUiContainer } from "../ui.js"; @@ -454,8 +455,7 @@ export class DiscordExecApprovalHandler { cfg: this.opts.cfg, gatewayUrl: this.opts.gatewayUrl, eventKinds: ["exec", "plugin"], - isConfigured: () => - Boolean(this.opts.config.enabled && (this.opts.config.approvers?.length ?? 0) > 0), + isConfigured: () => Boolean(this.opts.config.enabled && this.getApprovers().length > 0), shouldHandle: (request) => this.shouldHandle(request), deliverRequested: async (request) => await this.deliverRequested(request), finalizeResolved: async ({ request, resolved, entries }) => { @@ -472,7 +472,7 @@ export class DiscordExecApprovalHandler { if (!config.enabled) { return false; } - if (!config.approvers || config.approvers.length === 0) { + if (this.getApprovers().length === 0) { return false; } @@ -763,7 +763,11 @@ export class DiscordExecApprovalHandler { /** Return the list of configured approver IDs. */ getApprovers(): string[] { - return this.opts.config.approvers ?? []; + return getDiscordExecApprovalApprovers({ + cfg: this.opts.cfg, + accountId: this.opts.accountId, + configOverride: this.opts.config, + }); } } diff --git a/extensions/slack/src/config-ui-hints.ts b/extensions/slack/src/config-ui-hints.ts index 5e297d3c30a..ee5dfbb8ab9 100644 --- a/extensions/slack/src/config-ui-hints.ts +++ b/extensions/slack/src/config-ui-hints.ts @@ -59,7 +59,7 @@ export const slackChannelConfigUiHints = { }, "execApprovals.approvers": { label: "Slack Exec Approval Approvers", - help: "Slack user IDs allowed to approve exec requests for this workspace account. Use Slack user IDs or user targets such as `U123`, `user:U123`, or `<@U123>`. If you leave this unset, OpenClaw falls back to owner IDs inferred from channels.slack.allowFrom, channels.slack.dm.allowFrom, and defaultTo when possible.", + help: "Slack user IDs allowed to approve exec requests for this workspace account. Use Slack user IDs or user targets such as `U123`, `user:U123`, or `<@U123>`. If you leave this unset, OpenClaw falls back to commands.ownerAllowFrom when possible.", }, "execApprovals.agentFilter": { label: "Slack Exec Approval Agent Filter", diff --git a/extensions/slack/src/exec-approvals.test.ts b/extensions/slack/src/exec-approvals.test.ts index 634a2ee8887..e63f1f5ebe1 100644 --- a/extensions/slack/src/exec-approvals.test.ts +++ b/extensions/slack/src/exec-approvals.test.ts @@ -29,19 +29,27 @@ function buildConfig( } describe("slack exec approvals", () => { - it("requires enablement and an explicit or inferred approver", () => { + it("requires enablement and explicit or owner approvers", () => { expect(isSlackExecApprovalClientEnabled({ cfg: buildConfig() })).toBe(false); expect(isSlackExecApprovalClientEnabled({ cfg: buildConfig({ enabled: true }) })).toBe(false); expect( isSlackExecApprovalClientEnabled({ cfg: buildConfig({ enabled: true }, { allowFrom: ["U123"] }), }), - ).toBe(true); + ).toBe(false); expect( isSlackExecApprovalClientEnabled({ cfg: buildConfig({ enabled: true, approvers: ["U123"] }), }), ).toBe(true); + expect( + isSlackExecApprovalClientEnabled({ + cfg: { + ...buildConfig({ enabled: true }), + commands: { ownerAllowFrom: ["slack:U123OWNER"] }, + } as OpenClawConfig, + }), + ).toBe(true); }); it("prefers explicit approvers when configured", () => { @@ -55,7 +63,7 @@ describe("slack exec approvals", () => { expect(isSlackExecApprovalApprover({ cfg, senderId: "U123" })).toBe(false); }); - it("infers approvers from allowFrom, dm.allowFrom, and DM defaultTo", () => { + it("does not infer approvers from allowFrom or DM default routes", () => { const cfg = buildConfig( { enabled: true }, { @@ -65,19 +73,18 @@ describe("slack exec approvals", () => { }, ); - expect(getSlackExecApprovalApprovers({ cfg })).toEqual(["U123", "U456", "U789"]); - expect(isSlackExecApprovalApprover({ cfg, senderId: "U789" })).toBe(true); + expect(getSlackExecApprovalApprovers({ cfg })).toEqual([]); + expect(isSlackExecApprovalApprover({ cfg, senderId: "U789" })).toBe(false); }); - it("ignores non-user default targets when inferring approvers", () => { - const cfg = buildConfig( - { enabled: true }, - { - defaultTo: "channel:C123", - }, - ); + it("falls back to commands.ownerAllowFrom for exec approvers", () => { + const cfg = { + ...buildConfig({ enabled: true }), + commands: { ownerAllowFrom: ["slack:U123", "user:U456", "<@U789>"] }, + } as OpenClawConfig; - expect(getSlackExecApprovalApprovers({ cfg })).toEqual([]); + expect(getSlackExecApprovalApprovers({ cfg })).toEqual(["U123", "U456", "U789"]); + expect(isSlackExecApprovalApprover({ cfg, senderId: "U456" })).toBe(true); }); it("defaults target to dm", () => { diff --git a/extensions/slack/src/exec-approvals.ts b/extensions/slack/src/exec-approvals.ts index 8e53f55aa3a..5ac8004a88e 100644 --- a/extensions/slack/src/exec-approvals.ts +++ b/extensions/slack/src/exec-approvals.ts @@ -28,6 +28,17 @@ export function normalizeSlackApproverId(value: string | number): string | undef return /^[UW][A-Z0-9]+$/i.test(trimmed) ? trimmed : undefined; } +function resolveSlackOwnerApprovers(cfg: OpenClawConfig): string[] { + const ownerAllowFrom = cfg.commands?.ownerAllowFrom; + if (!Array.isArray(ownerAllowFrom) || ownerAllowFrom.length === 0) { + return []; + } + return resolveApprovalApprovers({ + explicit: ownerAllowFrom, + normalizeApprover: normalizeSlackApproverId, + }); +} + export function shouldHandleSlackExecApprovalRequest(params: { cfg: OpenClawConfig; accountId?: string | null; @@ -61,14 +72,11 @@ export function getSlackExecApprovalApprovers(params: { cfg: OpenClawConfig; accountId?: string | null; }): string[] { - const account = resolveSlackAccount(params).config; return resolveApprovalApprovers({ - explicit: account.execApprovals?.approvers, - allowFrom: account.allowFrom, - extraAllowFrom: account.dm?.allowFrom, - defaultTo: account.defaultTo, + explicit: + resolveSlackAccount(params).config.execApprovals?.approvers ?? + resolveSlackOwnerApprovers(params.cfg), normalizeApprover: normalizeSlackApproverId, - normalizeDefaultTo: normalizeSlackApproverId, }); } diff --git a/extensions/slack/src/monitor/exec-approvals.test.ts b/extensions/slack/src/monitor/exec-approvals.test.ts index 52a246da942..2040d136fc2 100644 --- a/extensions/slack/src/monitor/exec-approvals.test.ts +++ b/extensions/slack/src/monitor/exec-approvals.test.ts @@ -10,13 +10,18 @@ vi.mock("../send.js", () => ({ let SlackExecApprovalHandler: typeof import("./exec-approvals.js").SlackExecApprovalHandler; -function buildConfig(target: "dm" | "channel" | "both" = "dm"): OpenClawConfig { +function buildConfig( + target: "dm" | "channel" | "both" = "dm", + slackOverrides?: Partial["slack"]>>, +): OpenClawConfig { + const configuredExecApprovals = slackOverrides?.execApprovals; return { channels: { slack: { botToken: "xoxb-test", appToken: "xapp-test", - execApprovals: { + ...slackOverrides, + execApprovals: configuredExecApprovals ?? { enabled: true, approvers: ["U123APPROVER"], target, @@ -159,4 +164,38 @@ describe("SlackExecApprovalHandler", () => { }), ); }); + + it("does not treat allowFrom senders as approvers", async () => { + const app = buildApp(); + const cfg = buildConfig("dm", { + allowFrom: ["U123APPROVER"], + execApprovals: { enabled: true, target: "dm" }, + }); + const handler = new SlackExecApprovalHandler({ + app, + accountId: "default", + config: cfg.channels!.slack!.execApprovals!, + cfg, + }); + + expect(handler.shouldHandle(buildRequest())).toBe(false); + }); + + it("accepts commands.ownerAllowFrom as exec approver fallback", async () => { + const app = buildApp(); + const cfg = { + ...buildConfig("dm", { + execApprovals: { enabled: true, target: "dm" }, + }), + commands: { ownerAllowFrom: ["slack:U123APPROVER"] }, + } as OpenClawConfig; + const handler = new SlackExecApprovalHandler({ + app, + accountId: "default", + config: cfg.channels!.slack!.execApprovals!, + cfg, + }); + + expect(handler.shouldHandle(buildRequest())).toBe(true); + }); }); diff --git a/extensions/slack/src/monitor/exec-approvals.ts b/extensions/slack/src/monitor/exec-approvals.ts index a41afc802bf..182c5db38f5 100644 --- a/extensions/slack/src/monitor/exec-approvals.ts +++ b/extensions/slack/src/monitor/exec-approvals.ts @@ -14,7 +14,11 @@ import { } from "openclaw/plugin-sdk/infra-runtime"; import { logError } from "openclaw/plugin-sdk/text-runtime"; import { slackNativeApprovalAdapter } from "../approval-native.js"; -import { getSlackExecApprovalApprovers, normalizeSlackApproverId } from "../exec-approvals.js"; +import { + getSlackExecApprovalApprovers, + normalizeSlackApproverId, + shouldHandleSlackExecApprovalRequest, +} from "../exec-approvals.js"; import { resolveSlackReplyBlocks } from "../reply-blocks.js"; import { sendMessageSlack } from "../send.js"; @@ -240,20 +244,11 @@ export class SlackExecApprovalHandler { } shouldHandle(request: ExecApprovalRequest): boolean { - if (!this.opts.config.enabled) { - return false; - } - if ((this.opts.config.approvers?.length ?? 0) === 0) { - return false; - } - return ( - slackNativeApprovalAdapter.native?.describeDeliveryCapabilities({ - cfg: this.opts.cfg, - accountId: this.opts.accountId, - approvalKind: "exec", - request, - }).enabled === true - ); + return shouldHandleSlackExecApprovalRequest({ + cfg: this.opts.cfg, + accountId: this.opts.accountId, + request, + }); } async start(): Promise { diff --git a/src/agents/bash-tools.exec-host-gateway.test.ts b/src/agents/bash-tools.exec-host-gateway.test.ts new file mode 100644 index 00000000000..96d1ed7fc88 --- /dev/null +++ b/src/agents/bash-tools.exec-host-gateway.test.ts @@ -0,0 +1,124 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const createAndRegisterDefaultExecApprovalRequestMock = vi.hoisted(() => vi.fn()); +const buildExecApprovalPendingToolResultMock = vi.hoisted(() => vi.fn()); + +vi.mock("../infra/exec-approvals.js", () => ({ + evaluateShellAllowlist: vi.fn(() => ({ + allowlistMatches: [], + analysisOk: true, + allowlistSatisfied: true, + segments: [{ resolution: null, argv: ["echo", "ok"] }], + segmentAllowlistEntries: [{ pattern: "/usr/bin/echo", source: "allow-always" }], + })), + hasDurableExecApproval: vi.fn(() => true), + buildEnforcedShellCommand: vi.fn(() => ({ + ok: false, + reason: "segment execution plan unavailable", + })), + requiresExecApproval: vi.fn(() => false), + recordAllowlistUse: vi.fn(), + resolveApprovalAuditCandidatePath: vi.fn(() => null), + resolveAllowAlwaysPatterns: vi.fn(() => []), + addAllowlistEntry: vi.fn(), + addDurableCommandApproval: vi.fn(), +})); + +vi.mock("./bash-tools.exec-approval-request.js", () => ({ + buildExecApprovalRequesterContext: vi.fn(() => ({})), + buildExecApprovalTurnSourceContext: vi.fn(() => ({})), + registerExecApprovalRequestForHostOrThrow: vi.fn(async () => undefined), +})); + +vi.mock("./bash-tools.exec-host-shared.js", () => ({ + resolveExecHostApprovalContext: vi.fn(() => ({ + approvals: { allowlist: [], file: { version: 1, agents: {} } }, + hostSecurity: "allowlist", + hostAsk: "off", + askFallback: "deny", + })), + buildDefaultExecApprovalRequestArgs: vi.fn(() => ({})), + buildHeadlessExecApprovalDeniedMessage: vi.fn(() => "denied"), + buildExecApprovalFollowupTarget: vi.fn(() => null), + buildExecApprovalPendingToolResult: buildExecApprovalPendingToolResultMock, + createExecApprovalDecisionState: vi.fn(() => ({ + baseDecision: { timedOut: false }, + approvedByAsk: false, + deniedReason: "approval-required", + })), + createAndRegisterDefaultExecApprovalRequest: createAndRegisterDefaultExecApprovalRequestMock, + resolveApprovalDecisionOrUndefined: vi.fn(async () => undefined), + sendExecApprovalFollowupResult: vi.fn(async () => undefined), + shouldResolveExecApprovalUnavailableInline: vi.fn(() => false), +})); + +vi.mock("./bash-tools.exec-runtime.js", () => ({ + DEFAULT_NOTIFY_TAIL_CHARS: 1000, + createApprovalSlug: vi.fn(() => "slug"), + normalizeNotifyOutput: vi.fn((value) => value), + runExecProcess: vi.fn(), +})); + +vi.mock("./bash-process-registry.js", () => ({ + markBackgrounded: vi.fn(), + tail: vi.fn((value) => value), +})); + +vi.mock("../infra/exec-inline-eval.js", () => ({ + describeInterpreterInlineEval: vi.fn(() => "python -c"), + detectInterpreterInlineEvalArgv: vi.fn(() => null), +})); + +vi.mock("../infra/exec-obfuscation-detect.js", () => ({ + detectCommandObfuscation: vi.fn(() => ({ + detected: false, + reasons: [], + matchedPatterns: [], + })), +})); + +let processGatewayAllowlist: typeof import("./bash-tools.exec-host-gateway.js").processGatewayAllowlist; + +describe("processGatewayAllowlist", () => { + beforeEach(async () => { + vi.resetModules(); + buildExecApprovalPendingToolResultMock.mockReset(); + buildExecApprovalPendingToolResultMock.mockReturnValue({ + details: { status: "approval-pending" }, + content: [], + }); + createAndRegisterDefaultExecApprovalRequestMock.mockReset(); + createAndRegisterDefaultExecApprovalRequestMock.mockResolvedValue({ + approvalId: "req-1", + approvalSlug: "slug-1", + warningText: "", + expiresAtMs: Date.now() + 60_000, + preResolvedDecision: null, + initiatingSurface: "origin", + sentApproverDms: false, + unavailableReason: null, + }); + ({ processGatewayAllowlist } = await import("./bash-tools.exec-host-gateway.js")); + }); + + it("still requires approval when allowlist execution plan is unavailable despite durable trust", async () => { + const result = await processGatewayAllowlist({ + command: "echo ok", + workdir: process.cwd(), + env: process.env as Record, + pty: false, + defaultTimeoutSec: 30, + security: "allowlist", + ask: "off", + safeBins: new Set(), + safeBinProfiles: {}, + warnings: [], + approvalRunningNoticeMs: 0, + maxOutput: 1000, + pendingMaxOutput: 1000, + }); + + expect(createAndRegisterDefaultExecApprovalRequestMock).toHaveBeenCalledTimes(1); + expect(result.pendingResult?.details.status).toBe("approval-pending"); + }); +}); diff --git a/src/agents/bash-tools.exec-host-gateway.ts b/src/agents/bash-tools.exec-host-gateway.ts index e03d4e12e73..caeb1ac7dd8 100644 --- a/src/agents/bash-tools.exec-host-gateway.ts +++ b/src/agents/bash-tools.exec-host-gateway.ts @@ -1,10 +1,12 @@ import type { AgentToolResult } from "@mariozechner/pi-agent-core"; import { + addDurableCommandApproval, addAllowlistEntry, type ExecAsk, type ExecSecurity, buildEnforcedShellCommand, evaluateShellAllowlist, + hasDurableExecApproval, recordAllowlistUse, resolveApprovalAuditCandidatePath, requiresExecApproval, @@ -25,6 +27,7 @@ import { } from "./bash-tools.exec-approval-request.js"; import { buildDefaultExecApprovalRequestArgs, + buildHeadlessExecApprovalDeniedMessage, buildExecApprovalFollowupTarget, buildExecApprovalPendingToolResult, createExecApprovalDecisionState, @@ -32,6 +35,7 @@ import { resolveApprovalDecisionOrUndefined, resolveExecHostApprovalContext, sendExecApprovalFollowupResult, + shouldResolveExecApprovalUnavailableInline, } from "./bash-tools.exec-host-shared.js"; import { DEFAULT_NOTIFY_TAIL_CHARS, @@ -54,6 +58,7 @@ export type ProcessGatewayAllowlistParams = { safeBins: Set; safeBinProfiles: Readonly>; strictInlineEval?: boolean; + trigger?: string; agentId?: string; sessionKey?: string; turnSourceChannel?: string; @@ -71,6 +76,7 @@ export type ProcessGatewayAllowlistParams = { export type ProcessGatewayAllowlistResult = { execCommandOverride?: string; + allowWithoutEnforcedCommand?: boolean; pendingResult?: AgentToolResult; }; @@ -97,6 +103,12 @@ export async function processGatewayAllowlist( const analysisOk = allowlistEval.analysisOk; const allowlistSatisfied = hostSecurity === "allowlist" && analysisOk ? allowlistEval.allowlistSatisfied : false; + const durableApprovalSatisfied = hasDurableExecApproval({ + analysisOk, + segmentAllowlistEntries: allowlistEval.segmentAllowlistEntries, + allowlist: approvals.allowlist, + commandText: params.command, + }); const inlineEvalHit = params.strictInlineEval === true ? (allowlistEval.segments @@ -113,6 +125,7 @@ export async function processGatewayAllowlist( ); } let enforcedCommand: string | undefined; + let allowlistPlanUnavailableReason: string | null = null; if (hostSecurity === "allowlist" && analysisOk && allowlistSatisfied) { const enforced = buildEnforcedShellCommand({ command: params.command, @@ -120,9 +133,10 @@ export async function processGatewayAllowlist( platform: process.platform, }); if (!enforced.ok || !enforced.command) { - throw new Error(`exec denied: allowlist execution plan unavailable (${enforced.reason})`); + allowlistPlanUnavailableReason = enforced.reason ?? "unsupported platform"; + } else { + enforcedCommand = enforced.command; } - enforcedCommand = enforced.command; } const obfuscation = detectCommandObfuscation(params.command); if (obfuscation.detected) { @@ -148,13 +162,21 @@ export async function processGatewayAllowlist( const requiresHeredocApproval = hostSecurity === "allowlist" && analysisOk && allowlistSatisfied && hasHeredocSegment; const requiresInlineEvalApproval = inlineEvalHit !== null; + const requiresAllowlistPlanApproval = + hostSecurity === "allowlist" && + analysisOk && + allowlistSatisfied && + !enforcedCommand && + allowlistPlanUnavailableReason !== null; const requiresAsk = requiresExecApproval({ ask: hostAsk, security: hostSecurity, analysisOk, allowlistSatisfied, + durableApprovalSatisfied, }) || + requiresAllowlistPlanApproval || requiresHeredocApproval || requiresInlineEvalApproval || obfuscation.detected; @@ -163,6 +185,11 @@ export async function processGatewayAllowlist( "Warning: heredoc execution requires explicit approval in allowlist mode.", ); } + if (requiresAllowlistPlanApproval) { + params.warnings.push( + `Warning: allowlist auto-execution is unavailable on ${process.platform}; explicit approval is required.`, + ); + } if (requiresAsk) { const requestArgs = buildDefaultExecApprovalRequestArgs({ @@ -204,6 +231,42 @@ export async function processGatewayAllowlist( ...requestArgs, register: registerGatewayApproval, }); + if ( + shouldResolveExecApprovalUnavailableInline({ + trigger: params.trigger, + unavailableReason, + preResolvedDecision, + }) + ) { + const { approvedByAsk, deniedReason } = createExecApprovalDecisionState({ + decision: preResolvedDecision, + askFallback, + obfuscationDetected: obfuscation.detected, + }); + + if (deniedReason || !approvedByAsk) { + throw new Error( + buildHeadlessExecApprovalDeniedMessage({ + trigger: params.trigger, + host: "gateway", + security: hostSecurity, + ask: hostAsk, + askFallback, + }), + ); + } + + recordMatchedAllowlistUse( + resolveApprovalAuditCandidatePath( + allowlistEval.segments[0]?.resolution ?? null, + params.workdir, + ), + ); + return { + execCommandOverride: enforcedCommand, + allowWithoutEnforcedCommand: enforcedCommand === undefined, + }; + } const resolvedPath = resolveApprovalAuditCandidatePath( allowlistEval.segments[0]?.resolution ?? null, params.workdir, @@ -255,7 +318,7 @@ export async function processGatewayAllowlist( approvedByAsk = true; } else if (decision === "allow-always") { approvedByAsk = true; - if (hostSecurity === "allowlist" && !requiresInlineEvalApproval) { + if (!requiresInlineEvalApproval) { const patterns = resolveAllowAlwaysPatterns({ segments: allowlistEval.segments, cwd: params.workdir, @@ -265,13 +328,23 @@ export async function processGatewayAllowlist( }); for (const pattern of patterns) { if (pattern) { - addAllowlistEntry(approvals.file, params.agentId, pattern); + addAllowlistEntry(approvals.file, params.agentId, pattern, { + source: "allow-always", + }); } } + if (patterns.length === 0) { + addDurableCommandApproval(approvals.file, params.agentId, params.command); + } } } - if (hostSecurity === "allowlist" && (!analysisOk || !allowlistSatisfied) && !approvedByAsk) { + if ( + hostSecurity === "allowlist" && + (!analysisOk || !allowlistSatisfied) && + !approvedByAsk && + !durableApprovalSatisfied + ) { deniedReason = deniedReason ?? "allowlist-miss"; } diff --git a/src/agents/bash-tools.exec-host-node.ts b/src/agents/bash-tools.exec-host-node.ts index c9db6973005..8b29109ae01 100644 --- a/src/agents/bash-tools.exec-host-node.ts +++ b/src/agents/bash-tools.exec-host-node.ts @@ -5,6 +5,7 @@ import { type ExecAsk, type ExecSecurity, evaluateShellAllowlist, + hasDurableExecApproval, requiresExecApproval, resolveExecApprovalsFromFile, } from "../infra/exec-approvals.js"; @@ -43,6 +44,7 @@ export type ExecuteNodeHostCommandParams = { turnSourceTo?: string; turnSourceAccountId?: string; turnSourceThreadId?: string | number; + trigger?: string; agentId?: string; security: ExecSecurity; ask: ExecAsk; @@ -134,6 +136,7 @@ export async function executeNodeHostCommand( }); let analysisOk = baseAllowlistEval.analysisOk; let allowlistSatisfied = false; + let durableApprovalSatisfied = false; const inlineEvalHit = params.strictInlineEval === true ? (baseAllowlistEval.segments @@ -149,7 +152,7 @@ export async function executeNodeHostCommand( )}.`, ); } - if (hostAsk === "on-miss" && hostSecurity === "allowlist" && analysisOk) { + if ((hostAsk === "always" || hostSecurity === "allowlist") && analysisOk) { try { const approvalsSnapshot = await callGatewayTool<{ file: string }>( "exec.approvals.node.get", @@ -176,6 +179,12 @@ export async function executeNodeHostCommand( platform: nodeInfo?.platform, trustedSafeBinDirs: params.trustedSafeBinDirs, }); + durableApprovalSatisfied = hasDurableExecApproval({ + analysisOk: allowlistEval.analysisOk, + segmentAllowlistEntries: allowlistEval.segmentAllowlistEntries, + allowlist: resolved.allowlist, + commandText: runRawCommand, + }); allowlistSatisfied = allowlistEval.allowlistSatisfied; analysisOk = allowlistEval.analysisOk; } @@ -196,6 +205,7 @@ export async function executeNodeHostCommand( security: hostSecurity, analysisOk, allowlistSatisfied, + durableApprovalSatisfied, }) || inlineEvalHit !== null || obfuscation.detected; @@ -232,6 +242,9 @@ export async function executeNodeHostCommand( idempotencyKey: crypto.randomUUID(), }) satisfies Record; + let inlineApprovedByAsk = false; + let inlineApprovalDecision: "allow-once" | "allow-always" | null = null; + let inlineApprovalId: string | undefined; if (requiresAsk) { const requestArgs = execHostShared.buildDefaultExecApprovalRequestArgs({ warnings: params.warnings, @@ -269,119 +282,149 @@ export async function executeNodeHostCommand( ...requestArgs, register: registerNodeApproval, }); - const followupTarget = execHostShared.buildExecApprovalFollowupTarget({ - approvalId, - sessionKey: params.notifySessionKey, - turnSourceChannel: params.turnSourceChannel, - turnSourceTo: params.turnSourceTo, - turnSourceAccountId: params.turnSourceAccountId, - turnSourceThreadId: params.turnSourceThreadId, - }); - - void (async () => { - const decision = await execHostShared.resolveApprovalDecisionOrUndefined({ - approvalId, + if ( + execHostShared.shouldResolveExecApprovalUnavailableInline({ + trigger: params.trigger, + unavailableReason, preResolvedDecision, - onFailure: () => - void execHostShared.sendExecApprovalFollowupResult( - followupTarget, - `Exec denied (node=${nodeId} id=${approvalId}, approval-request-failed): ${params.command}`, - ), - }); - if (decision === undefined) { - return; - } - - const { - baseDecision, - approvedByAsk: initialApprovedByAsk, - deniedReason: initialDeniedReason, - } = execHostShared.createExecApprovalDecisionState({ - decision, + }) + ) { + const { approvedByAsk, deniedReason } = execHostShared.createExecApprovalDecisionState({ + decision: preResolvedDecision, askFallback, obfuscationDetected: obfuscation.detected, }); - let approvedByAsk = initialApprovedByAsk; - let approvalDecision: "allow-once" | "allow-always" | null = null; - let deniedReason = initialDeniedReason; - - if (baseDecision.timedOut && askFallback === "full" && approvedByAsk) { - approvalDecision = "allow-once"; - } else if (decision === "allow-once") { - approvedByAsk = true; - approvalDecision = "allow-once"; - } else if (decision === "allow-always") { - approvedByAsk = true; - approvalDecision = "allow-always"; - } - - if (deniedReason) { - await execHostShared.sendExecApprovalFollowupResult( - followupTarget, - `Exec denied (node=${nodeId} id=${approvalId}, ${deniedReason}): ${params.command}`, - ); - return; - } - - try { - const raw = await callGatewayTool<{ - payload?: { - stdout?: string; - stderr?: string; - error?: string | null; - exitCode?: number | null; - timedOut?: boolean; - }; - }>( - "node.invoke", - { timeoutMs: invokeTimeoutMs }, - buildInvokeParams(approvedByAsk, approvalDecision, approvalId, true), - ); - const payload = - raw?.payload && typeof raw.payload === "object" - ? (raw.payload as { - stdout?: string; - stderr?: string; - error?: string | null; - exitCode?: number | null; - timedOut?: boolean; - }) - : {}; - const combined = [payload.stdout, payload.stderr, payload.error].filter(Boolean).join("\n"); - const output = normalizeNotifyOutput(combined.slice(-DEFAULT_NOTIFY_TAIL_CHARS)); - const exitLabel = payload.timedOut ? "timeout" : `code ${payload.exitCode ?? "?"}`; - const summary = output - ? `Exec finished (node=${nodeId} id=${approvalId}, ${exitLabel})\n${output}` - : `Exec finished (node=${nodeId} id=${approvalId}, ${exitLabel})`; - await execHostShared.sendExecApprovalFollowupResult(followupTarget, summary); - } catch { - await execHostShared.sendExecApprovalFollowupResult( - followupTarget, - `Exec denied (node=${nodeId} id=${approvalId}, invoke-failed): ${params.command}`, + if (deniedReason || !approvedByAsk) { + throw new Error( + execHostShared.buildHeadlessExecApprovalDeniedMessage({ + trigger: params.trigger, + host: "node", + security: hostSecurity, + ask: hostAsk, + askFallback, + }), ); } - })(); + inlineApprovedByAsk = approvedByAsk; + inlineApprovalDecision = approvedByAsk ? "allow-once" : null; + inlineApprovalId = approvalId; + } else { + const followupTarget = execHostShared.buildExecApprovalFollowupTarget({ + approvalId, + sessionKey: params.notifySessionKey, + turnSourceChannel: params.turnSourceChannel, + turnSourceTo: params.turnSourceTo, + turnSourceAccountId: params.turnSourceAccountId, + turnSourceThreadId: params.turnSourceThreadId, + }); - return execHostShared.buildExecApprovalPendingToolResult({ - host: "node", - command: params.command, - cwd: params.workdir, - warningText, - approvalId, - approvalSlug, - expiresAtMs, - initiatingSurface, - sentApproverDms, - unavailableReason, - nodeId, - }); + void (async () => { + const decision = await execHostShared.resolveApprovalDecisionOrUndefined({ + approvalId, + preResolvedDecision, + onFailure: () => + void execHostShared.sendExecApprovalFollowupResult( + followupTarget, + `Exec denied (node=${nodeId} id=${approvalId}, approval-request-failed): ${params.command}`, + ), + }); + if (decision === undefined) { + return; + } + + const { + baseDecision, + approvedByAsk: initialApprovedByAsk, + deniedReason: initialDeniedReason, + } = execHostShared.createExecApprovalDecisionState({ + decision, + askFallback, + obfuscationDetected: obfuscation.detected, + }); + let approvedByAsk = initialApprovedByAsk; + let approvalDecision: "allow-once" | "allow-always" | null = null; + let deniedReason = initialDeniedReason; + + if (baseDecision.timedOut && askFallback === "full" && approvedByAsk) { + approvalDecision = "allow-once"; + } else if (decision === "allow-once") { + approvedByAsk = true; + approvalDecision = "allow-once"; + } else if (decision === "allow-always") { + approvedByAsk = true; + approvalDecision = "allow-always"; + } + + if (deniedReason) { + await execHostShared.sendExecApprovalFollowupResult( + followupTarget, + `Exec denied (node=${nodeId} id=${approvalId}, ${deniedReason}): ${params.command}`, + ); + return; + } + + try { + const raw = await callGatewayTool<{ + payload?: { + stdout?: string; + stderr?: string; + error?: string | null; + exitCode?: number | null; + timedOut?: boolean; + }; + }>( + "node.invoke", + { timeoutMs: invokeTimeoutMs }, + buildInvokeParams(approvedByAsk, approvalDecision, approvalId, true), + ); + const payload = + raw?.payload && typeof raw.payload === "object" + ? (raw.payload as { + stdout?: string; + stderr?: string; + error?: string | null; + exitCode?: number | null; + timedOut?: boolean; + }) + : {}; + const combined = [payload.stdout, payload.stderr, payload.error] + .filter(Boolean) + .join("\n"); + const output = normalizeNotifyOutput(combined.slice(-DEFAULT_NOTIFY_TAIL_CHARS)); + const exitLabel = payload.timedOut ? "timeout" : `code ${payload.exitCode ?? "?"}`; + const summary = output + ? `Exec finished (node=${nodeId} id=${approvalId}, ${exitLabel})\n${output}` + : `Exec finished (node=${nodeId} id=${approvalId}, ${exitLabel})`; + await execHostShared.sendExecApprovalFollowupResult(followupTarget, summary); + } catch { + await execHostShared.sendExecApprovalFollowupResult( + followupTarget, + `Exec denied (node=${nodeId} id=${approvalId}, invoke-failed): ${params.command}`, + ); + } + })(); + + return execHostShared.buildExecApprovalPendingToolResult({ + host: "node", + command: params.command, + cwd: params.workdir, + warningText, + approvalId, + approvalSlug, + expiresAtMs, + initiatingSurface, + sentApproverDms, + unavailableReason, + nodeId, + }); + } } const startedAt = Date.now(); const raw = await callGatewayTool( "node.invoke", { timeoutMs: invokeTimeoutMs }, - buildInvokeParams(false, null), + buildInvokeParams(inlineApprovedByAsk, inlineApprovalDecision, inlineApprovalId), ); const payload = raw && typeof raw === "object" ? (raw as { payload?: unknown }).payload : undefined; diff --git a/src/agents/bash-tools.exec-host-shared.ts b/src/agents/bash-tools.exec-host-shared.ts index 9187b7bf58e..0187cda95d2 100644 --- a/src/agents/bash-tools.exec-host-shared.ts +++ b/src/agents/bash-tools.exec-host-shared.ts @@ -65,6 +65,10 @@ export type ExecApprovalUnavailableReason = | "initiating-platform-disabled" | "initiating-platform-unsupported"; +function isHeadlessExecTrigger(trigger?: string): boolean { + return trigger === "cron"; +} + export type RegisteredExecApprovalRequestContext = { approvalId: string; approvalSlug: string; @@ -340,6 +344,38 @@ export function createExecApprovalDecisionState(params: { }; } +export function shouldResolveExecApprovalUnavailableInline(params: { + trigger?: string; + unavailableReason: ExecApprovalUnavailableReason | null; + preResolvedDecision: string | null | undefined; +}): boolean { + return ( + isHeadlessExecTrigger(params.trigger) && + params.unavailableReason === "no-approval-route" && + params.preResolvedDecision === null + ); +} + +export function buildHeadlessExecApprovalDeniedMessage(params: { + trigger?: string; + host: "gateway" | "node"; + security: ExecSecurity; + ask: ExecAsk; + askFallback: ResolvedExecApprovals["agent"]["askFallback"]; +}): string { + const runLabel = params.trigger === "cron" ? "Cron runs" : "Headless runs"; + return [ + `exec denied: ${runLabel} cannot wait for interactive exec approval.`, + `Effective host exec policy: security=${params.security} ask=${params.ask} askFallback=${params.askFallback}`, + "Stricter values from tools.exec and ~/.openclaw/exec-approvals.json both apply.", + "Fix one of these:", + '- align both files to security="full" and ask="off" for trusted local automation', + "- keep allowlist mode and add an explicit allowlist entry for this command", + "- enable Web UI, terminal UI, or chat exec approvals and rerun interactively", + 'Tip: run "openclaw doctor" and "openclaw approvals get --gateway" to inspect the effective policy.', + ].join("\n"); +} + export async function sendExecApprovalFollowupResult( target: ExecApprovalFollowupTarget, resultText: string, diff --git a/src/agents/bash-tools.exec-types.ts b/src/agents/bash-tools.exec-types.ts index 7417da612ac..a23ab24d367 100644 --- a/src/agents/bash-tools.exec-types.ts +++ b/src/agents/bash-tools.exec-types.ts @@ -6,6 +6,7 @@ export type ExecToolDefaults = { host?: ExecTarget; security?: ExecSecurity; ask?: ExecAsk; + trigger?: string; node?: string; pathPrepend?: string[]; safeBins?: string[]; diff --git a/src/agents/bash-tools.exec.approval-id.test.ts b/src/agents/bash-tools.exec.approval-id.test.ts index 72d08afd61b..074fc52f3f1 100644 --- a/src/agents/bash-tools.exec.approval-id.test.ts +++ b/src/agents/bash-tools.exec.approval-id.test.ts @@ -1,3 +1,4 @@ +import crypto from "node:crypto"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -148,6 +149,7 @@ async function expectGatewayExecWithoutApproval(options: { config: Record; command: string; ask?: "always" | "on-miss" | "off"; + security?: "allowlist" | "full"; }) { await writeExecApprovalsConfig(options.config); const calls: string[] = []; @@ -156,7 +158,7 @@ async function expectGatewayExecWithoutApproval(options: { const tool = createExecTool({ host: "gateway", ask: options.ask, - security: "full", + security: options.security, approvalRunningNoticeMs: 0, }); @@ -200,6 +202,18 @@ function mockPendingApprovalRegistration() { }); } +function mockNoApprovalRouteRegistration() { + vi.mocked(callGatewayTool).mockImplementation(async (method) => { + if (method === "exec.approval.request") { + return { id: "approval-id", decision: null }; + } + if (method === "exec.approval.waitDecision") { + return { decision: null }; + } + return { ok: true }; + }); +} + describe("exec approvals", () => { let previousHome: string | undefined; let previousUserProfile: string | undefined; @@ -410,6 +424,171 @@ describe("exec approvals", () => { }); }); + it("inherits security=full from exec-approvals defaults when tool security is unset", async () => { + await expectGatewayExecWithoutApproval({ + config: { + version: 1, + defaults: { security: "full", ask: "off", askFallback: "full" }, + agents: {}, + }, + command: "echo ok", + security: undefined, + }); + }); + + it("keeps ask=always prompts even when durable allow-always trust matches", async () => { + await writeExecApprovalsConfig({ + version: 1, + defaults: { security: "full", ask: "always", askFallback: "full" }, + agents: { + main: { + allowlist: [{ pattern: process.execPath, source: "allow-always" }], + }, + }, + }); + mockPendingApprovalRegistration(); + + const tool = createExecTool({ + host: "gateway", + ask: "always", + security: "full", + approvalRunningNoticeMs: 0, + }); + + const result = await tool.execute("call-gateway-durable-still-prompts", { + command: `${JSON.stringify(process.execPath)} --version`, + }); + + expect(result.details.status).toBe("approval-pending"); + }); + + it("keeps ask=always prompts for static allowlist entries without allow-always trust", async () => { + await writeExecApprovalsConfig({ + version: 1, + defaults: { security: "full", ask: "always", askFallback: "full" }, + agents: { + main: { + allowlist: [{ pattern: process.execPath }], + }, + }, + }); + mockPendingApprovalRegistration(); + + const tool = createExecTool({ + host: "gateway", + ask: "always", + security: "full", + approvalRunningNoticeMs: 0, + }); + + const result = await tool.execute("call-static-allowlist-still-prompts", { + command: `${JSON.stringify(process.execPath)} --version`, + }); + + expect(result.details.status).toBe("approval-pending"); + }); + + it("keeps ask=always prompts for node-host runs even with durable trust", async () => { + const calls: string[] = []; + vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => { + calls.push(method); + if (method === "exec.approvals.node.get") { + return { + file: { + version: 1, + agents: { + main: { + allowlist: [{ pattern: process.execPath, source: "allow-always" }], + }, + }, + }, + }; + } + if (method === "node.invoke") { + const invoke = params as { command?: string }; + if (invoke.command === "system.run.prepare") { + return buildPreparedSystemRunPayload(params); + } + if (invoke.command === "system.run") { + return { payload: { success: true, stdout: "node-ok" } }; + } + } + return { ok: true }; + }); + + const tool = createExecTool({ + host: "node", + ask: "always", + security: "full", + approvalRunningNoticeMs: 0, + }); + + const result = await tool.execute("call-node-durable-allow-always", { + command: `${JSON.stringify(process.execPath)} --version`, + }); + + expect(result.details.status).toBe("approval-pending"); + expect(calls).toContain("exec.approval.request"); + }); + + it("reuses exact-command durable trust for node shell-wrapper reruns", async () => { + const calls: string[] = []; + vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => { + calls.push(method); + if (method === "exec.approvals.node.get") { + const prepared = buildPreparedSystemRunPayload({ + params: { command: ["/bin/sh", "-lc", "cd ."], cwd: process.cwd() }, + }) as { payload?: { plan?: { commandText?: string } } }; + const commandText = prepared.payload?.plan?.commandText ?? ""; + return { + file: { + version: 1, + agents: { + main: { + allowlist: [ + { + pattern: `=command:${crypto + .createHash("sha256") + .update(commandText) + .digest("hex") + .slice(0, 16)}`, + source: "allow-always", + }, + ], + }, + }, + }, + }; + } + if (method === "node.invoke") { + const invoke = params as { command?: string }; + if (invoke.command === "system.run.prepare") { + return buildPreparedSystemRunPayload(params); + } + if (invoke.command === "system.run") { + return { payload: { success: true, stdout: "node-shell-wrapper-ok" } }; + } + } + return { ok: true }; + }); + + const tool = createExecTool({ + host: "node", + ask: "on-miss", + security: "allowlist", + approvalRunningNoticeMs: 0, + }); + + const result = await tool.execute("call-node-shell-wrapper-durable-allow-always", { + command: "cd .", + }); + + expect(result.details.status).toBe("completed"); + expect(getResultText(result)).toContain("node-shell-wrapper-ok"); + expect(calls).not.toContain("exec.approval.request"); + expect(calls).not.toContain("exec.approval.waitDecision"); + }); + it("requires approval for elevated ask when allowlist misses", async () => { const calls: string[] = []; let resolveApproval: (() => void) | undefined; @@ -925,6 +1104,114 @@ describe("exec approvals", () => { ); }); + it("resolves cron no-route approvals inline when askFallback permits trusted automation", async () => { + await writeExecApprovalsConfig({ + version: 1, + defaults: { security: "full", ask: "always", askFallback: "full" }, + agents: {}, + }); + mockNoApprovalRouteRegistration(); + + const tool = createExecTool({ + host: "gateway", + ask: "always", + security: "full", + trigger: "cron", + approvalRunningNoticeMs: 0, + }); + + const result = await tool.execute("call-cron-inline-approval", { + command: "echo cron-ok", + }); + + expect(result.details.status).toBe("completed"); + expect(getResultText(result)).toContain("cron-ok"); + expect(vi.mocked(callGatewayTool)).toHaveBeenCalledWith( + "exec.approval.request", + expect.anything(), + expect.anything(), + expect.objectContaining({ expectFinal: false }), + ); + expect( + vi + .mocked(callGatewayTool) + .mock.calls.some(([method]) => method === "exec.approval.waitDecision"), + ).toBe(false); + }); + + it("forwards inline cron approval state to node system.run", async () => { + await writeExecApprovalsConfig({ + version: 1, + defaults: { security: "full", ask: "always", askFallback: "full" }, + agents: {}, + }); + mockNoApprovalRouteRegistration(); + + let systemRunInvoke: unknown; + vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => { + if (method === "exec.approval.request") { + return { id: "approval-id", decision: null }; + } + if (method === "exec.approval.waitDecision") { + return { decision: null }; + } + if (method === "node.invoke") { + const invoke = params as { command?: string }; + if (invoke.command === "system.run.prepare") { + return buildPreparedSystemRunPayload(params); + } + if (invoke.command === "system.run") { + systemRunInvoke = params; + return { payload: { success: true, stdout: "cron-node-ok" } }; + } + } + return { ok: true }; + }); + + const tool = createExecTool({ + host: "node", + ask: "always", + security: "full", + trigger: "cron", + approvalRunningNoticeMs: 0, + }); + + const result = await tool.execute("call-cron-inline-node-approval", { + command: "echo cron-node-ok", + }); + + expect(result.details.status).toBe("completed"); + expect(getResultText(result)).toContain("cron-node-ok"); + expect(systemRunInvoke).toMatchObject({ + command: "system.run", + params: { + approved: true, + approvalDecision: "allow-once", + }, + }); + expect((systemRunInvoke as { params?: { runId?: string } }).params?.runId).toEqual( + expect.any(String), + ); + }); + + it("explains cron no-route denials with a host-policy fix hint", async () => { + mockNoApprovalRouteRegistration(); + + const tool = createExecTool({ + host: "gateway", + ask: "always", + security: "full", + trigger: "cron", + approvalRunningNoticeMs: 0, + }); + + await expect( + tool.execute("call-cron-denied", { + command: "echo cron-denied", + }), + ).rejects.toThrow("Cron runs cannot wait for interactive exec approval"); + }); + it("shows a local /approve prompt when discord exec approvals are disabled", async () => { await writeOpenClawConfig({ channels: { diff --git a/src/agents/bash-tools.exec.ts b/src/agents/bash-tools.exec.ts index e22e3e7aa2f..41cd26c3510 100644 --- a/src/agents/bash-tools.exec.ts +++ b/src/agents/bash-tools.exec.ts @@ -594,14 +594,18 @@ export function createExecTool( }); const host: ExecHost = target.effectiveHost; - const configuredSecurity = defaults?.security ?? (host === "sandbox" ? "deny" : "allowlist"); + const approvalDefaults = loadExecApprovals().defaults; + const configuredSecurity = + defaults?.security ?? + approvalDefaults?.security ?? + (host === "sandbox" ? "deny" : "allowlist"); const requestedSecurity = normalizeExecSecurity(params.security); let security = minSecurity(configuredSecurity, requestedSecurity ?? configuredSecurity); if (elevatedRequested && elevatedMode === "full") { security = "full"; } - // Keep local exec defaults in sync with exec-approvals.json when tools.exec.ask is unset. - const configuredAsk = defaults?.ask ?? loadExecApprovals().defaults?.ask ?? "on-miss"; + // Keep local exec defaults in sync with exec-approvals.json when tools.exec.* is unset. + const configuredAsk = defaults?.ask ?? approvalDefaults?.ask ?? "on-miss"; const requestedAsk = normalizeExecAsk(params.ask); let ask = maxAsk(configuredAsk, requestedAsk ?? configuredAsk); const bypassApprovals = elevatedRequested && elevatedMode === "full"; @@ -726,6 +730,7 @@ export function createExecTool( security, ask, strictInlineEval: defaults?.strictInlineEval, + trigger: defaults?.trigger, timeoutSec: params.timeout, defaultTimeoutSec, approvalRunningNoticeMs, @@ -749,6 +754,7 @@ export function createExecTool( safeBins, safeBinProfiles, strictInlineEval: defaults?.strictInlineEval, + trigger: defaults?.trigger, agentId, sessionKey: defaults?.sessionKey, turnSourceChannel: defaults?.messageProvider, @@ -767,6 +773,9 @@ export function createExecTool( return gatewayResult.pendingResult; } execCommandOverride = gatewayResult.execCommandOverride; + if (gatewayResult.allowWithoutEnforcedCommand) { + execCommandOverride = undefined; + } } const explicitTimeoutSec = typeof params.timeout === "number" ? params.timeout : null; diff --git a/src/agents/pi-tools.ts b/src/agents/pi-tools.ts index 6fa4ea1f593..64b9d23acd0 100644 --- a/src/agents/pi-tools.ts +++ b/src/agents/pi-tools.ts @@ -444,6 +444,7 @@ export function createOpenClawCodingTools(options?: { host: options?.exec?.host ?? execConfig.host, security: options?.exec?.security ?? execConfig.security, ask: options?.exec?.ask ?? execConfig.ask, + trigger: options?.trigger, node: options?.exec?.node ?? execConfig.node, pathPrepend: options?.exec?.pathPrepend ?? execConfig.pathPrepend, safeBins: options?.exec?.safeBins ?? execConfig.safeBins, diff --git a/src/commands/doctor-security.test.ts b/src/commands/doctor-security.test.ts index ca2bfb2989c..009d2beafc7 100644 --- a/src/commands/doctor-security.test.ts +++ b/src/commands/doctor-security.test.ts @@ -1,3 +1,6 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { OpenClawConfig } from "../config/config.js"; @@ -17,12 +20,14 @@ import { noteSecurityWarnings } from "./doctor-security.js"; describe("noteSecurityWarnings gateway exposure", () => { let prevToken: string | undefined; let prevPassword: string | undefined; + let prevHome: string | undefined; beforeEach(() => { note.mockClear(); pluginRegistry.list = []; prevToken = process.env.OPENCLAW_GATEWAY_TOKEN; prevPassword = process.env.OPENCLAW_GATEWAY_PASSWORD; + prevHome = process.env.HOME; delete process.env.OPENCLAW_GATEWAY_TOKEN; delete process.env.OPENCLAW_GATEWAY_PASSWORD; }); @@ -38,10 +43,29 @@ describe("noteSecurityWarnings gateway exposure", () => { } else { process.env.OPENCLAW_GATEWAY_PASSWORD = prevPassword; } + if (prevHome === undefined) { + delete process.env.HOME; + } else { + process.env.HOME = prevHome; + } }); const lastMessage = () => String(note.mock.calls.at(-1)?.[0] ?? ""); + async function withExecApprovalsFile( + file: Record, + run: () => Promise, + ): Promise { + const home = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-doctor-security-")); + process.env.HOME = home; + await fs.mkdir(path.join(home, ".openclaw"), { recursive: true }); + await fs.writeFile( + path.join(home, ".openclaw", "exec-approvals.json"), + JSON.stringify(file, null, 2), + ); + await run(); + } + it("warns when exposed without auth", async () => { const cfg = { gateway: { bind: "lan" } } as OpenClawConfig; await noteSecurityWarnings(cfg); @@ -136,6 +160,148 @@ describe("noteSecurityWarnings gateway exposure", () => { expect(message).toContain("openclaw approvals get --gateway"); }); + it("warns when tools.exec is broader than host exec defaults", async () => { + await withExecApprovalsFile( + { + version: 1, + defaults: { + security: "allowlist", + ask: "on-miss", + }, + }, + async () => { + await noteSecurityWarnings({ + tools: { + exec: { + security: "full", + ask: "off", + }, + }, + } as OpenClawConfig); + }, + ); + + const message = lastMessage(); + expect(message).toContain("tools.exec is broader than the host exec policy"); + expect(message).toContain('security="full"'); + expect(message).toContain('defaults.security="allowlist"'); + expect(message).toContain("stricter side wins"); + }); + + it("does not invent a deny host policy when exec-approvals defaults.security is unset", async () => { + await withExecApprovalsFile( + { + version: 1, + agents: {}, + }, + async () => { + await noteSecurityWarnings({ + tools: { + exec: { + security: "allowlist", + ask: "on-miss", + }, + }, + } as OpenClawConfig); + }, + ); + + const message = lastMessage(); + expect(message).toContain("No channel security warnings detected"); + expect(message).not.toContain('security="deny"'); + }); + + it("does not invent an on-miss host ask policy when exec-approvals defaults.ask is unset", async () => { + await withExecApprovalsFile( + { + version: 1, + agents: {}, + }, + async () => { + await noteSecurityWarnings({ + tools: { + exec: { + ask: "always", + }, + }, + } as OpenClawConfig); + }, + ); + + const message = lastMessage(); + expect(message).toContain("No channel security warnings detected"); + expect(message).not.toContain('ask="on-miss"'); + }); + + it("warns when a per-agent exec policy is broader than the matching host agent policy", async () => { + await withExecApprovalsFile( + { + version: 1, + agents: { + runner: { + security: "allowlist", + ask: "always", + }, + }, + }, + async () => { + await noteSecurityWarnings({ + agents: { + list: [ + { + id: "runner", + tools: { + exec: { + security: "full", + ask: "off", + }, + }, + }, + ], + }, + } as OpenClawConfig); + }, + ); + + const message = lastMessage(); + expect(message).toContain("agents.list.runner.tools.exec is broader than the host exec policy"); + expect(message).toContain('agents.runner.security="allowlist"'); + expect(message).toContain('agents.runner.ask="always"'); + }); + + it('does not warn about durable allow-always trust when ask="always" is enforced', async () => { + await withExecApprovalsFile( + { + version: 1, + defaults: { + ask: "always", + }, + agents: { + main: { + allowlist: [ + { + pattern: "/usr/bin/echo", + source: "allow-always", + }, + ], + }, + }, + }, + async () => { + await noteSecurityWarnings({ + tools: { + exec: { + ask: "always", + }, + }, + } as OpenClawConfig); + }, + ); + + const message = lastMessage(); + expect(message).not.toContain('tools.exec: ask="always" still bypasses future prompts'); + }); + it("warns when heartbeat delivery relies on implicit directPolicy defaults", async () => { const cfg = { agents: { diff --git a/src/commands/doctor-security.ts b/src/commands/doctor-security.ts index c489682f607..6c65d6112e1 100644 --- a/src/commands/doctor-security.ts +++ b/src/commands/doctor-security.ts @@ -6,6 +6,15 @@ import type { AgentConfig } from "../config/types.agents.js"; import { hasConfiguredSecretInput } from "../config/types.secrets.js"; import { resolveGatewayAuth } from "../gateway/auth.js"; import { isLoopbackHost, resolveGatewayBindHost } from "../gateway/net.js"; +import { + loadExecApprovals, + maxAsk, + minSecurity, + resolveExecApprovalsFromFile, + type ExecApprovalsFile, + type ExecAsk, + type ExecSecurity, +} from "../infra/exec-approvals.js"; import { resolveDmAllowState } from "../security/dm-policy-shared.js"; import { note } from "../terminal/note.js"; import { resolveDefaultChannelAccountContext } from "./channel-account-context.js"; @@ -48,6 +57,143 @@ function collectImplicitHeartbeatDirectPolicyWarnings(cfg: OpenClawConfig): stri return warnings; } +function execSecurityRank(value: ExecSecurity): number { + switch (value) { + case "deny": + return 0; + case "allowlist": + return 1; + case "full": + return 2; + } +} + +function execAskRank(value: ExecAsk): number { + switch (value) { + case "off": + return 0; + case "on-miss": + return 1; + case "always": + return 2; + } +} + +function resolveHostExecPolicy(params: { + approvals: ExecApprovalsFile; + execConfig: { security?: ExecSecurity; ask?: ExecAsk } | undefined; + agentId?: string; +}): { + security: ExecSecurity; + ask: ExecAsk; + securitySource: string; + askSource: string; +} { + const basePath = "~/.openclaw/exec-approvals.json"; + const agentEntry = + params.agentId && params.approvals.agents && params.approvals.agents[params.agentId] + ? params.approvals.agents[params.agentId] + : undefined; + const defaults = params.approvals.defaults; + const configuredSecurity = params.execConfig?.security ?? "allowlist"; + const configuredAsk = params.execConfig?.ask ?? "on-miss"; + const resolved = resolveExecApprovalsFromFile({ + file: params.approvals, + agentId: params.agentId, + overrides: { + security: configuredSecurity, + ask: configuredAsk, + }, + }); + const security = minSecurity(configuredSecurity, resolved.agent.security); + const ask = resolved.agent.ask === "off" ? "off" : maxAsk(configuredAsk, resolved.agent.ask); + return { + security, + ask, + securitySource: agentEntry?.security + ? `${basePath} agents.${params.agentId}.security` + : defaults?.security + ? `${basePath} defaults.security` + : "caller tool policy fallback", + askSource: agentEntry?.ask + ? `${basePath} agents.${params.agentId}.ask` + : defaults?.ask + ? `${basePath} defaults.ask` + : "caller tool policy fallback", + }; +} + +function collectExecPolicyConflictWarnings(cfg: OpenClawConfig): string[] { + const warnings: string[] = []; + const approvals = loadExecApprovals(); + + const maybeWarn = (params: { + scopeLabel: string; + execConfig: { security?: ExecSecurity; ask?: ExecAsk } | undefined; + agentId?: string; + }) => { + const execConfig = params.execConfig; + if (!execConfig || (!execConfig.security && !execConfig.ask)) { + return; + } + const host = resolveHostExecPolicy({ + approvals, + execConfig, + agentId: params.agentId, + }); + const securityConflict = + execConfig.security !== undefined && + execSecurityRank(execConfig.security) > execSecurityRank(host.security); + const askConflict = + execConfig.ask !== undefined && execAskRank(execConfig.ask) < execAskRank(host.ask); + if (!securityConflict && !askConflict) { + return; + } + + const configParts: string[] = []; + const hostParts: string[] = []; + if (execConfig.security !== undefined) { + configParts.push(`security="${execConfig.security}"`); + hostParts.push(`${host.securitySource}="${host.security}"`); + } + if (execConfig.ask !== undefined) { + configParts.push(`ask="${execConfig.ask}"`); + hostParts.push(`${host.askSource}="${host.ask}"`); + } + + warnings.push( + [ + `- ${params.scopeLabel} is broader than the host exec policy.`, + ` Config: ${configParts.join(", ")}`, + ` Host: ${hostParts.join(", ")}`, + ` Effective host exec stays security="${host.security}" ask="${host.ask}" because the stricter side wins.`, + " Headless runs like isolated cron cannot answer approval prompts; align both files or enable Web UI, terminal UI, or chat exec approvals.", + ` Inspect with: ${formatCliCommand("openclaw approvals get --gateway")}`, + ].join("\n"), + ); + }; + + maybeWarn({ + scopeLabel: "tools.exec", + execConfig: cfg.tools?.exec, + }); + + for (const agent of cfg.agents?.list ?? []) { + maybeWarn({ + scopeLabel: `agents.list.${agent.id}.tools.exec`, + execConfig: agent.tools?.exec, + agentId: agent.id, + }); + } + + return warnings; +} + +function collectDurableExecApprovalWarnings(cfg: OpenClawConfig): string[] { + void cfg; + return []; +} + export async function noteSecurityWarnings(cfg: OpenClawConfig) { const warnings: string[] = []; const auditHint = `- Run: ${formatCliCommand("openclaw security audit --deep")}`; @@ -61,6 +207,8 @@ export async function noteSecurityWarnings(cfg: OpenClawConfig) { } warnings.push(...collectImplicitHeartbeatDirectPolicyWarnings(cfg)); + warnings.push(...collectExecPolicyConflictWarnings(cfg)); + warnings.push(...collectDurableExecApprovalWarnings(cfg)); // =========================================== // GATEWAY NETWORK EXPOSURE CHECK diff --git a/src/config/bundled-channel-config-metadata.generated.ts b/src/config/bundled-channel-config-metadata.generated.ts index add92fb0088..0fa2c8bb4a8 100644 --- a/src/config/bundled-channel-config-metadata.generated.ts +++ b/src/config/bundled-channel-config-metadata.generated.ts @@ -11677,7 +11677,7 @@ export const GENERATED_BUNDLED_CHANNEL_CONFIG_METADATA = [ }, "execApprovals.approvers": { label: "Slack Exec Approval Approvers", - help: "Slack user IDs allowed to approve exec requests for this workspace account. Use Slack user IDs or user targets such as `U123`, `user:U123`, or `<@U123>`. If you leave this unset, OpenClaw falls back to owner IDs inferred from channels.slack.allowFrom, channels.slack.dm.allowFrom, and defaultTo when possible.", + help: "Slack user IDs allowed to approve exec requests for this workspace account. Use Slack user IDs or user targets such as `U123`, `user:U123`, or `<@U123>`. If you leave this unset, OpenClaw falls back to commands.ownerAllowFrom when possible.", }, "execApprovals.agentFilter": { label: "Slack Exec Approval Agent Filter", diff --git a/src/config/types.discord.ts b/src/config/types.discord.ts index 2640c7bdfb1..cb03a1a069d 100644 --- a/src/config/types.discord.ts +++ b/src/config/types.discord.ts @@ -142,7 +142,7 @@ export type DiscordVoiceConfig = { export type DiscordExecApprovalConfig = { /** Enable exec approval forwarding to Discord DMs. Default: false. */ enabled?: boolean; - /** Discord user IDs to receive approval prompts. Optional: falls back to owner IDs inferred from allowFrom/defaultTo when possible. */ + /** Discord user IDs to receive approval prompts. Optional: falls back to commands.ownerAllowFrom when possible. */ approvers?: string[]; /** Only forward approvals for these agent IDs. Omit = all agents. */ agentFilter?: string[]; diff --git a/src/config/types.slack.ts b/src/config/types.slack.ts index 7e9eeefb244..2fb6404b7d9 100644 --- a/src/config/types.slack.ts +++ b/src/config/types.slack.ts @@ -54,7 +54,7 @@ export type SlackExecApprovalTarget = "dm" | "channel" | "both"; export type SlackExecApprovalConfig = { /** Enable Slack exec approvals for this account. Default: false. */ enabled?: boolean; - /** Slack user IDs allowed to approve exec requests. Optional: falls back to owner IDs inferred from allowFrom/defaultTo when possible. */ + /** Slack user IDs allowed to approve exec requests. Optional: falls back to commands.ownerAllowFrom when possible. */ approvers?: Array; /** Only forward approvals for these agent IDs. Omit = all agents. */ agentFilter?: string[]; diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index 509631f5330..716386137b7 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -109,6 +109,7 @@ function isPathScopedExecutableToken(token: string): boolean { export type ExecAllowlistEvaluation = { allowlistSatisfied: boolean; allowlistMatches: ExecAllowlistEntry[]; + segmentAllowlistEntries: Array; segmentSatisfiedBy: ExecSegmentSatisfiedBy[]; }; @@ -368,15 +369,18 @@ function evaluateSegments( ): { satisfied: boolean; matches: ExecAllowlistEntry[]; + segmentAllowlistEntries: Array; segmentSatisfiedBy: ExecSegmentSatisfiedBy[]; } { const matches: ExecAllowlistEntry[] = []; const skillBinTrust = buildSkillBinTrustIndex(params.skillBins); const allowSkills = params.autoAllowSkills === true && skillBinTrust.size > 0; + const segmentAllowlistEntries: Array = []; const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = []; const satisfied = segments.every((segment) => { if (segment.resolution?.policyBlocked === true) { + segmentAllowlistEntries.push(null); segmentSatisfiedBy.push(null); return false; } @@ -412,6 +416,7 @@ function evaluateSegments( if (match) { matches.push(match); } + segmentAllowlistEntries.push(match ?? null); const safe = isSafeBinUsage({ argv: effectiveArgv, resolution: resolveExecutionTargetResolution(segment.resolution), @@ -436,7 +441,7 @@ function evaluateSegments( return Boolean(by); }); - return { satisfied, matches, segmentSatisfiedBy }; + return { satisfied, matches, segmentAllowlistEntries, segmentSatisfiedBy }; } function resolveAnalysisSegmentGroups(analysis: ExecCommandAnalysis): ExecCommandSegment[][] { @@ -452,9 +457,15 @@ export function evaluateExecAllowlist( } & ExecAllowlistContext, ): ExecAllowlistEvaluation { const allowlistMatches: ExecAllowlistEntry[] = []; + const segmentAllowlistEntries: Array = []; const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = []; if (!params.analysis.ok || params.analysis.segments.length === 0) { - return { allowlistSatisfied: false, allowlistMatches, segmentSatisfiedBy }; + return { + allowlistSatisfied: false, + allowlistMatches, + segmentAllowlistEntries, + segmentSatisfiedBy, + }; } const allowlistContext = pickExecAllowlistContext(params); @@ -466,15 +477,27 @@ export function evaluateExecAllowlist( return { allowlistSatisfied: false, allowlistMatches: result.matches, + segmentAllowlistEntries: result.segmentAllowlistEntries, segmentSatisfiedBy: result.segmentSatisfiedBy, }; } - return { allowlistSatisfied: false, allowlistMatches: [], segmentSatisfiedBy: [] }; + return { + allowlistSatisfied: false, + allowlistMatches: [], + segmentAllowlistEntries: [], + segmentSatisfiedBy: [], + }; } allowlistMatches.push(...result.matches); + segmentAllowlistEntries.push(...result.segmentAllowlistEntries); segmentSatisfiedBy.push(...result.segmentSatisfiedBy); } - return { allowlistSatisfied: true, allowlistMatches, segmentSatisfiedBy }; + return { + allowlistSatisfied: true, + allowlistMatches, + segmentAllowlistEntries, + segmentSatisfiedBy, + }; } export type ExecAllowlistAnalysis = { @@ -482,6 +505,7 @@ export type ExecAllowlistAnalysis = { allowlistSatisfied: boolean; allowlistMatches: ExecAllowlistEntry[]; segments: ExecCommandSegment[]; + segmentAllowlistEntries: Array; segmentSatisfiedBy: ExecSegmentSatisfiedBy[]; }; @@ -700,6 +724,7 @@ export function evaluateShellAllowlist( allowlistSatisfied: false, allowlistMatches: [], segments: [], + segmentAllowlistEntries: [], segmentSatisfiedBy: [], }); @@ -728,6 +753,7 @@ export function evaluateShellAllowlist( allowlistSatisfied: evaluation.allowlistSatisfied, allowlistMatches: evaluation.allowlistMatches, segments: analysis.segments, + segmentAllowlistEntries: evaluation.segmentAllowlistEntries, segmentSatisfiedBy: evaluation.segmentSatisfiedBy, }; } @@ -793,15 +819,20 @@ export function evaluateShellAllowlist( } const allowlistMatches: ExecAllowlistEntry[] = []; const segments: ExecCommandSegment[] = []; + const segmentAllowlistEntries: Array = []; const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = []; for (const [index, { analysis, evaluation }] of finalizedEvaluations.entries()) { const effectiveSegmentSatisfiedBy = allowSkillPreludeAtIndex.has(index) ? analysis.segments.map(() => "skillPrelude" as const) : evaluation.segmentSatisfiedBy; + const effectiveSegmentAllowlistEntries = allowSkillPreludeAtIndex.has(index) + ? analysis.segments.map(() => null) + : evaluation.segmentAllowlistEntries; segments.push(...analysis.segments); allowlistMatches.push(...evaluation.allowlistMatches); + segmentAllowlistEntries.push(...effectiveSegmentAllowlistEntries); segmentSatisfiedBy.push(...effectiveSegmentSatisfiedBy); if (!evaluation.allowlistSatisfied && !allowSkillPreludeAtIndex.has(index)) { return { @@ -809,6 +840,7 @@ export function evaluateShellAllowlist( allowlistSatisfied: false, allowlistMatches, segments, + segmentAllowlistEntries, segmentSatisfiedBy, }; } @@ -819,6 +851,7 @@ export function evaluateShellAllowlist( allowlistSatisfied: true, allowlistMatches, segments, + segmentAllowlistEntries, segmentSatisfiedBy, }; } diff --git a/src/infra/exec-approvals-policy.test.ts b/src/infra/exec-approvals-policy.test.ts index 861906de017..135d3a664c0 100644 --- a/src/infra/exec-approvals-policy.test.ts +++ b/src/infra/exec-approvals-policy.test.ts @@ -1,5 +1,11 @@ import { describe, expect, it } from "vitest"; import { + makeMockCommandResolution, + makeMockExecutableResolution, +} from "./exec-approvals-test-helpers.js"; +import { + evaluateExecAllowlist, + hasDurableExecApproval, maxAsk, minSecurity, normalizeExecAsk, @@ -77,6 +83,14 @@ describe("exec approvals policy helpers", () => { allowlistSatisfied: true, expected: true, }, + { + ask: "always" as const, + security: "full" as const, + analysisOk: true, + allowlistSatisfied: false, + durableApprovalSatisfied: true, + expected: true, + }, { ask: "off" as const, security: "allowlist" as const, @@ -108,4 +122,71 @@ describe("exec approvals policy helpers", () => { ])("requiresExecApproval respects ask mode and allowlist satisfaction for %j", (testCase) => { expect(requiresExecApproval(testCase)).toBe(testCase.expected); }); + + it("treats exact-command allow-always approvals as durable trust", () => { + expect( + hasDurableExecApproval({ + analysisOk: false, + segmentAllowlistEntries: [], + allowlist: [ + { + pattern: "=command:613b5a60181648fd", + source: "allow-always", + }, + ], + commandText: 'powershell -NoProfile -Command "Write-Output hi"', + }), + ).toBe(true); + }); + + it("marks policy-blocked segments as non-durable allowlist entries", () => { + const executable = makeMockExecutableResolution({ + rawExecutable: "/usr/bin/echo", + resolvedPath: "/usr/bin/echo", + executableName: "echo", + }); + const result = evaluateExecAllowlist({ + analysis: { + ok: true, + segments: [ + { + raw: "/usr/bin/echo ok", + argv: ["/usr/bin/echo", "ok"], + resolution: makeMockCommandResolution({ + execution: executable, + }), + }, + { + raw: "/bin/sh -lc whoami", + argv: ["/bin/sh", "-lc", "whoami"], + resolution: makeMockCommandResolution({ + execution: makeMockExecutableResolution({ + rawExecutable: "/bin/sh", + resolvedPath: "/bin/sh", + executableName: "sh", + }), + policyBlocked: true, + }), + }, + ], + }, + allowlist: [{ pattern: "/usr/bin/echo", source: "allow-always" }], + safeBins: new Set(), + cwd: "/tmp", + platform: process.platform, + }); + + expect(result.allowlistSatisfied).toBe(false); + expect(result.segmentAllowlistEntries).toEqual([ + expect.objectContaining({ pattern: "/usr/bin/echo" }), + null, + ]); + expect( + hasDurableExecApproval({ + analysisOk: true, + segmentAllowlistEntries: result.segmentAllowlistEntries, + allowlist: [{ pattern: "/usr/bin/echo", source: "allow-always" }], + }), + ).toBe(false); + }); }); diff --git a/src/infra/exec-approvals-store.test.ts b/src/infra/exec-approvals-store.test.ts index 6d07f18e647..a603a60aa41 100644 --- a/src/infra/exec-approvals-store.test.ts +++ b/src/infra/exec-approvals-store.test.ts @@ -14,6 +14,7 @@ import type { ExecApprovalsFile } from "./exec-approvals.js"; type ExecApprovalsModule = typeof import("./exec-approvals.js"); let addAllowlistEntry: ExecApprovalsModule["addAllowlistEntry"]; +let addDurableCommandApproval: ExecApprovalsModule["addDurableCommandApproval"]; let ensureExecApprovals: ExecApprovalsModule["ensureExecApprovals"]; let mergeExecApprovalsSocketDefaults: ExecApprovalsModule["mergeExecApprovalsSocketDefaults"]; let normalizeExecApprovals: ExecApprovalsModule["normalizeExecApprovals"]; @@ -29,6 +30,7 @@ const originalOpenClawHome = process.env.OPENCLAW_HOME; beforeAll(async () => { ({ addAllowlistEntry, + addDurableCommandApproval, ensureExecApprovals, mergeExecApprovalsSocketDefaults, normalizeExecApprovals, @@ -168,6 +170,65 @@ describe("exec approvals store helpers", () => { expect(readApprovalsFile(dir).agents?.worker?.allowlist?.[0]?.id).toMatch(/^[0-9a-f-]{36}$/i); }); + it("persists durable command approvals without storing plaintext command text", () => { + const dir = createHomeDir(); + vi.spyOn(Date, "now").mockReturnValue(321_000); + + const approvals = ensureExecApprovals(); + addDurableCommandApproval(approvals, "worker", 'printenv API_KEY="secret-value"'); + + expect(readApprovalsFile(dir).agents?.worker?.allowlist).toEqual([ + expect.objectContaining({ + source: "allow-always", + lastUsedAt: 321_000, + }), + ]); + expect(readApprovalsFile(dir).agents?.worker?.allowlist?.[0]?.pattern).toMatch( + /^=command:[0-9a-f]{16}$/i, + ); + expect(readApprovalsFile(dir).agents?.worker?.allowlist?.[0]).not.toHaveProperty("commandText"); + }); + + it("strips legacy plaintext command text during normalization", () => { + expect( + normalizeExecApprovals({ + version: 1, + agents: { + main: { + allowlist: [ + { + pattern: "=command:test", + source: "allow-always", + commandText: "echo secret-token", + }, + ], + }, + }, + }).agents?.main?.allowlist, + ).toEqual([ + expect.objectContaining({ + pattern: "=command:test", + source: "allow-always", + }), + ]); + expect( + normalizeExecApprovals({ + version: 1, + agents: { + main: { + allowlist: [ + { + pattern: "=command:test", + source: "allow-always", + commandText: "echo secret-token", + }, + ], + }, + }, + }).agents?.main?.allowlist?.[0], + ).not.toHaveProperty("commandText"); + }); + it("records allowlist usage on the matching entry and backfills missing ids", () => { const dir = createHomeDir(); vi.spyOn(Date, "now").mockReturnValue(999_000); diff --git a/src/infra/exec-approvals.ts b/src/infra/exec-approvals.ts index d7d94d007e9..98f8444decb 100644 --- a/src/infra/exec-approvals.ts +++ b/src/infra/exec-approvals.ts @@ -115,6 +115,8 @@ export type ExecApprovalsDefaults = { export type ExecAllowlistEntry = { id?: string; pattern: string; + source?: "allow-always"; + commandText?: string; lastUsedAt?: number; lastUsedCommand?: string; lastResolvedPath?: string; @@ -265,6 +267,24 @@ function ensureAllowlistIds( return changed ? next : allowlist; } +function stripAllowlistCommandText( + allowlist: ExecAllowlistEntry[] | undefined, +): ExecAllowlistEntry[] | undefined { + if (!Array.isArray(allowlist) || allowlist.length === 0) { + return allowlist; + } + let changed = false; + const next = allowlist.map((entry) => { + if (typeof entry.commandText !== "string") { + return entry; + } + changed = true; + const { commandText: _commandText, ...rest } = entry; + return rest; + }); + return changed ? next : allowlist; +} + export function normalizeExecApprovals(file: ExecApprovalsFile): ExecApprovalsFile { const socketPath = file.socket?.path?.trim(); const token = file.socket?.token?.trim(); @@ -277,7 +297,8 @@ export function normalizeExecApprovals(file: ExecApprovalsFile): ExecApprovalsFi } for (const [key, agent] of Object.entries(agents)) { const coerced = coerceAllowlistEntries(agent.allowlist); - const allowlist = ensureAllowlistIds(coerced); + const withIds = ensureAllowlistIds(coerced); + const allowlist = stripAllowlistCommandText(withIds); if (allowlist !== agent.allowlist) { agents[key] = { ...agent, allowlist }; } @@ -495,15 +516,52 @@ export function requiresExecApproval(params: { security: ExecSecurity; analysisOk: boolean; allowlistSatisfied: boolean; + durableApprovalSatisfied?: boolean; }): boolean { + if (params.ask === "always") { + return true; + } + if (params.durableApprovalSatisfied === true) { + return false; + } return ( - params.ask === "always" || - (params.ask === "on-miss" && - params.security === "allowlist" && - (!params.analysisOk || !params.allowlistSatisfied)) + params.ask === "on-miss" && + params.security === "allowlist" && + (!params.analysisOk || !params.allowlistSatisfied) ); } +export function hasDurableExecApproval(params: { + analysisOk: boolean; + segmentAllowlistEntries: Array; + allowlist?: readonly ExecAllowlistEntry[]; + commandText?: string | null; +}): boolean { + const normalizedCommand = params.commandText?.trim(); + const commandPattern = normalizedCommand + ? buildDurableCommandApprovalPattern(normalizedCommand) + : null; + const exactCommandMatch = normalizedCommand + ? (params.allowlist ?? []).some( + (entry) => + entry.source === "allow-always" && + (entry.pattern === commandPattern || + (typeof entry.commandText === "string" && + entry.commandText.trim() === normalizedCommand)), + ) + : false; + const allowlistMatch = + params.analysisOk && + params.segmentAllowlistEntries.length > 0 && + params.segmentAllowlistEntries.every((entry) => entry?.source === "allow-always"); + return exactCommandMatch || allowlistMatch; +} + +function buildDurableCommandApprovalPattern(commandText: string): string { + const digest = crypto.createHash("sha256").update(commandText).digest("hex").slice(0, 16); + return `=command:${digest}`; +} + export function recordAllowlistUse( approvals: ExecApprovalsFile, agentId: string | undefined, @@ -535,6 +593,9 @@ export function addAllowlistEntry( approvals: ExecApprovalsFile, agentId: string | undefined, pattern: string, + options?: { + source?: ExecAllowlistEntry["source"]; + }, ) { const target = agentId ?? DEFAULT_AGENT_ID; const agents = approvals.agents ?? {}; @@ -544,15 +605,49 @@ export function addAllowlistEntry( if (!trimmed) { return; } - if (allowlist.some((entry) => entry.pattern === trimmed)) { + const existingEntry = allowlist.find((entry) => entry.pattern === trimmed); + if (existingEntry && (!options?.source || existingEntry.source === options.source)) { return; } - allowlist.push({ id: crypto.randomUUID(), pattern: trimmed, lastUsedAt: Date.now() }); - agents[target] = { ...existing, allowlist }; + const now = Date.now(); + const nextAllowlist = existingEntry + ? allowlist.map((entry) => + entry.pattern === trimmed + ? { + ...entry, + source: options?.source ?? entry.source, + lastUsedAt: now, + } + : entry, + ) + : [ + ...allowlist, + { + id: crypto.randomUUID(), + pattern: trimmed, + source: options?.source, + lastUsedAt: now, + }, + ]; + agents[target] = { ...existing, allowlist: nextAllowlist }; approvals.agents = agents; saveExecApprovals(approvals); } +export function addDurableCommandApproval( + approvals: ExecApprovalsFile, + agentId: string | undefined, + commandText: string, +) { + const normalized = commandText.trim(); + if (!normalized) { + return; + } + addAllowlistEntry(approvals, agentId, buildDurableCommandApprovalPattern(normalized), { + source: "allow-always", + }); +} + export function minSecurity(a: ExecSecurity, b: ExecSecurity): ExecSecurity { const order: Record = { deny: 0, allowlist: 1, full: 2 }; return order[a] <= order[b] ? a : b; diff --git a/src/node-host/exec-policy.test.ts b/src/node-host/exec-policy.test.ts index f76a2891840..44944e3c27e 100644 --- a/src/node-host/exec-policy.test.ts +++ b/src/node-host/exec-policy.test.ts @@ -91,6 +91,20 @@ describe("evaluateSystemRunPolicy", () => { expect(denied.requiresAsk).toBe(true); }); + it("still requires approval when ask=always even with durable trust", () => { + const denied = expectDeniedDecision( + evaluateSystemRunPolicy( + buildPolicyParams({ + security: "full", + ask: "always", + durableApprovalSatisfied: true, + }), + ), + ); + expect(denied.eventReason).toBe("approval-required"); + expect(denied.requiresAsk).toBe(true); + }); + it("allows allowlist miss when explicit approval is provided", () => { const allowed = expectAllowedDecision( evaluateSystemRunPolicy( diff --git a/src/node-host/exec-policy.ts b/src/node-host/exec-policy.ts index ce4c2d57b4b..6500be269a1 100644 --- a/src/node-host/exec-policy.ts +++ b/src/node-host/exec-policy.ts @@ -54,6 +54,7 @@ export function evaluateSystemRunPolicy(params: { ask: ExecAsk; analysisOk: boolean; allowlistSatisfied: boolean; + durableApprovalSatisfied?: boolean; approvalDecision: ExecApprovalDecision; approved?: boolean; isWindows: boolean; @@ -87,6 +88,7 @@ export function evaluateSystemRunPolicy(params: { security: params.security, analysisOk, allowlistSatisfied, + durableApprovalSatisfied: params.durableApprovalSatisfied, }); if (requiresAsk && !approvedByAsk) { return { @@ -104,6 +106,18 @@ export function evaluateSystemRunPolicy(params: { } if (params.security === "allowlist" && (!analysisOk || !allowlistSatisfied) && !approvedByAsk) { + if (params.durableApprovalSatisfied) { + return { + allowed: true, + analysisOk, + allowlistSatisfied, + shellWrapperBlocked, + windowsShellWrapperBlocked, + requiresAsk, + approvalDecision: params.approvalDecision, + approvedByAsk, + }; + } return { allowed: false, eventReason: "allowlist-miss", diff --git a/src/node-host/invoke-system-run-allowlist.ts b/src/node-host/invoke-system-run-allowlist.ts index d4817453a70..36c737fd2ed 100644 --- a/src/node-host/invoke-system-run-allowlist.ts +++ b/src/node-host/invoke-system-run-allowlist.ts @@ -17,6 +17,7 @@ export type SystemRunAllowlistAnalysis = { allowlistMatches: ExecAllowlistEntry[]; allowlistSatisfied: boolean; segments: ExecCommandSegment[]; + segmentAllowlistEntries: Array; }; export function evaluateSystemRunAllowlist(params: { @@ -53,6 +54,7 @@ export function evaluateSystemRunAllowlist(params: { ? allowlistEval.allowlistSatisfied : false, segments: allowlistEval.segments, + segmentAllowlistEntries: allowlistEval.segmentAllowlistEntries, }; } @@ -73,6 +75,7 @@ export function evaluateSystemRunAllowlist(params: { allowlistSatisfied: params.security === "allowlist" && analysis.ok ? allowlistEval.allowlistSatisfied : false, segments: analysis.segments, + segmentAllowlistEntries: allowlistEval.segmentAllowlistEntries, }; } diff --git a/src/node-host/invoke-system-run.test.ts b/src/node-host/invoke-system-run.test.ts index d1e085a9d75..a0b96b0334e 100644 --- a/src/node-host/invoke-system-run.test.ts +++ b/src/node-host/invoke-system-run.test.ts @@ -1,3 +1,4 @@ +import crypto from "node:crypto"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; @@ -1480,4 +1481,60 @@ describe("handleSystemRunInvoke mac app exec host routing", () => { clearRuntimeConfigSnapshot(); } }); + + it("reuses exact-command durable trust for shell-wrapper reruns", async () => { + if (process.platform === "win32") { + return; + } + + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-shell-wrapper-allow-")); + try { + const prepared = buildSystemRunApprovalPlan({ + command: ["/bin/sh", "-lc", "cd ."], + cwd: tempDir, + }); + expect(prepared.ok).toBe(true); + if (!prepared.ok) { + throw new Error("unreachable"); + } + + await withTempApprovalsHome({ + approvals: { + version: 1, + defaults: { security: "allowlist", ask: "on-miss", askFallback: "full" }, + agents: { + main: { + allowlist: [ + { + pattern: `=command:${crypto + .createHash("sha256") + .update(prepared.plan.commandText) + .digest("hex") + .slice(0, 16)}`, + source: "allow-always", + }, + ], + }, + }, + }, + run: async () => { + const rerun = await runSystemInvoke({ + preferMacAppExecHost: false, + command: prepared.plan.argv, + rawCommand: prepared.plan.commandText, + systemRunPlan: prepared.plan, + cwd: prepared.plan.cwd ?? tempDir, + security: "allowlist", + ask: "on-miss", + runCommand: vi.fn(async () => createLocalRunResult("shell-wrapper-reused")), + }); + + expect(rerun.runCommand).toHaveBeenCalledTimes(1); + expectInvokeOk(rerun.sendInvokeResult, { payloadContains: "shell-wrapper-reused" }); + }, + }); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); }); diff --git a/src/node-host/invoke-system-run.ts b/src/node-host/invoke-system-run.ts index e90fd20a6df..0c118b33b7d 100644 --- a/src/node-host/invoke-system-run.ts +++ b/src/node-host/invoke-system-run.ts @@ -3,7 +3,9 @@ import { resolveAgentConfig } from "../agents/agent-scope.js"; import { loadConfig } from "../config/config.js"; import type { GatewayClient } from "../gateway/client.js"; import { + addDurableCommandApproval, addAllowlistEntry, + hasDurableExecApproval, recordAllowlistUse, resolveApprovalAuditCandidatePath, resolveAllowAlwaysPatterns, @@ -96,6 +98,7 @@ type SystemRunPolicyPhase = SystemRunParsePhase & { approvals: ResolvedExecApprovals; security: ExecSecurity; policy: ReturnType; + durableApprovalSatisfied: boolean; strictInlineEval: boolean; inlineEvalHit: ReturnType; allowlistMatches: ExecAllowlistEntry[]; @@ -332,19 +335,20 @@ async function evaluateSystemRunPolicyPhase( onWarning: warnWritableTrustedDirOnce, }); const bins = autoAllowSkills ? await opts.skillBins.current() : []; - let { analysisOk, allowlistMatches, allowlistSatisfied, segments } = evaluateSystemRunAllowlist({ - shellCommand: parsed.shellPayload, - argv: parsed.argv, - approvals, - security, - safeBins, - safeBinProfiles, - trustedSafeBinDirs, - cwd: parsed.cwd, - env: parsed.env, - skillBins: bins, - autoAllowSkills, - }); + let { analysisOk, allowlistMatches, allowlistSatisfied, segments, segmentAllowlistEntries } = + evaluateSystemRunAllowlist({ + shellCommand: parsed.shellPayload, + argv: parsed.argv, + approvals, + security, + safeBins, + safeBinProfiles, + trustedSafeBinDirs, + cwd: parsed.cwd, + env: parsed.env, + skillBins: bins, + autoAllowSkills, + }); const strictInlineEval = agentExec?.strictInlineEval === true || cfg.tools?.exec?.strictInlineEval === true; const inlineEvalHit = strictInlineEval @@ -358,11 +362,18 @@ async function evaluateSystemRunPolicyPhase( const cmdInvocation = parsed.shellPayload ? opts.isCmdExeInvocation(segments[0]?.argv ?? []) : opts.isCmdExeInvocation(parsed.argv); + const durableApprovalSatisfied = hasDurableExecApproval({ + analysisOk, + segmentAllowlistEntries, + allowlist: approvals.allowlist, + commandText: parsed.commandText, + }); const policy = evaluateSystemRunPolicy({ security, ask, analysisOk, allowlistSatisfied, + durableApprovalSatisfied, approvalDecision: parsed.approvalDecision, approved: parsed.approved, isWindows, @@ -390,7 +401,12 @@ async function evaluateSystemRunPolicyPhase( } // Fail closed if policy/runtime drift re-allows unapproved shell wrappers. - if (security === "allowlist" && parsed.shellPayload && !policy.approvedByAsk) { + if ( + security === "allowlist" && + parsed.shellPayload && + !policy.approvedByAsk && + !durableApprovalSatisfied + ) { await sendSystemRunDenied(opts, parsed.execution, { reason: "approval-required", message: "SYSTEM_RUN_DENIED: approval required", @@ -440,6 +456,7 @@ async function evaluateSystemRunPolicyPhase( approvals, security, policy, + durableApprovalSatisfied, strictInlineEval, inlineEvalHit, allowlistMatches, @@ -546,25 +563,24 @@ async function executeSystemRunPhase( } } - if ( - phase.policy.approvalDecision === "allow-always" && - phase.security === "allowlist" && - phase.inlineEvalHit === null - ) { - if (phase.policy.analysisOk) { - const patterns = resolveAllowAlwaysPatterns({ - segments: phase.segments, - cwd: phase.cwd, - env: phase.env, - platform: process.platform, - strictInlineEval: phase.strictInlineEval, - }); - for (const pattern of patterns) { - if (pattern) { - addAllowlistEntry(phase.approvals.file, phase.agentId, pattern); - } + if (phase.policy.approvalDecision === "allow-always" && phase.inlineEvalHit === null) { + const patterns = resolveAllowAlwaysPatterns({ + segments: phase.segments, + cwd: phase.cwd, + env: phase.env, + platform: process.platform, + strictInlineEval: phase.strictInlineEval, + }); + for (const pattern of patterns) { + if (pattern) { + addAllowlistEntry(phase.approvals.file, phase.agentId, pattern, { + source: "allow-always", + }); } } + if (patterns.length === 0) { + addDurableCommandApproval(phase.approvals.file, phase.agentId, phase.commandText); + } } if (phase.allowlistMatches.length > 0) { diff --git a/src/plugin-sdk/approval-delivery-helpers.test.ts b/src/plugin-sdk/approval-delivery-helpers.test.ts index 4a8efb26108..7687b0a8a89 100644 --- a/src/plugin-sdk/approval-delivery-helpers.test.ts +++ b/src/plugin-sdk/approval-delivery-helpers.test.ts @@ -90,6 +90,13 @@ describe("createApproverRestrictedNativeApprovalAdapter", () => { action: "approve", }), ).toEqual({ kind: "disabled" }); + expect( + getActionAvailabilityState({ + cfg: {} as never, + accountId: "disabled", + action: "approve", + }), + ).toEqual({ kind: "disabled" }); expect(hasConfiguredDmRoute({ cfg: {} as never })).toBe(true); expect(nativeCapabilities).toEqual({ enabled: true, diff --git a/src/plugin-sdk/approval-delivery-helpers.ts b/src/plugin-sdk/approval-delivery-helpers.ts index d3f9ae1578b..37c03eefed2 100644 --- a/src/plugin-sdk/approval-delivery-helpers.ts +++ b/src/plugin-sdk/approval-delivery-helpers.ts @@ -88,7 +88,8 @@ export function createApproverRestrictedNativeApprovalAdapter(params: { accountId?: string | null; action: "approve"; }) => - params.hasApprovers({ cfg, accountId }) + params.hasApprovers({ cfg, accountId }) && + params.isNativeDeliveryEnabled({ cfg, accountId }) ? ({ kind: "enabled" } as const) : ({ kind: "disabled" } as const), },