diff --git a/CHANGELOG.md b/CHANGELOG.md index 96873ad32ef..223277559ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ Docs: https://docs.openclaw.ai - QQBot/security: require framework auth for `/bot-approve` so unauthorized QQ senders cannot change exec approval settings through the unauthenticated pre-dispatch slash-command path. (#70706) Thanks @vincentkoc. - MCP/tools: stop the ACPX OpenClaw tools bridge from listing or invoking owner-only tools such as `cron`, closing a privilege-escalation path for non-owner MCP callers. (#70698) Thanks @vincentkoc. - Feishu/onboarding: load Feishu setup surfaces through a setup-only barrel so first-run setup no longer imports Feishu's Lark SDK before bundled runtime deps are staged. (#70339) Thanks @andrejtr. +- Approvals/startup: let native approval handlers report ready after gateway authentication while replaying pending approvals in the background, so slow or failing replay delivery no longer blocks handler startup or amplifies reconnect storms. - WhatsApp/security: keep contact/vCard/location structured-object free text out of the inline message body and render it through fenced untrusted metadata JSON, limiting hidden prompt-injection payloads in names, phone fields, and location labels/comments. - Group-chat/security: keep channel-sourced group names and participant labels out of inline group system prompts and render them through fenced untrusted metadata JSON. - Plugins/startup: restore bundled plugin `openclaw/plugin-sdk/*` resolution from packaged installs and external runtime-deps stage roots, so Telegram/Discord no longer crash-loop with `Cannot find package 'openclaw'` after missing dependency repair. diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index 7550caf4318..75cd2c00e5c 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -316,58 +316,58 @@ so file operands cannot be smuggled as ambiguous positionals. [//]: # "SAFE_BIN_DENIED_FLAGS:START" - - `grep`: `--dereference-recursive`, `--directories`, `--exclude-from`, `--file`, `--recursive`, `-R`, `-d`, `-f`, `-r` - - `jq`: `--argfile`, `--from-file`, `--library-path`, `--rawfile`, `--slurpfile`, `-L`, `-f` - - `sort`: `--compress-program`, `--files0-from`, `--output`, `--random-source`, `--temporary-directory`, `-T`, `-o` - - `wc`: `--files0-from` +- `grep`: `--dereference-recursive`, `--directories`, `--exclude-from`, `--file`, `--recursive`, `-R`, `-d`, `-f`, `-r` +- `jq`: `--argfile`, `--from-file`, `--library-path`, `--rawfile`, `--slurpfile`, `-L`, `-f` +- `sort`: `--compress-program`, `--files0-from`, `--output`, `--random-source`, `--temporary-directory`, `-T`, `-o` +- `wc`: `--files0-from` [//]: # "SAFE_BIN_DENIED_FLAGS:END" Safe bins also force argv tokens to be treated as **literal text** at - execution time (no globbing and no `$VARS` expansion) for stdin-only - segments, so patterns like `*` or `$HOME/...` cannot be used to smuggle - file reads. + execution time (no globbing and no `$VARS` expansion) for stdin-only + segments, so patterns like `*` or `$HOME/...` cannot be used to smuggle + file reads. - + - - Safe bins must resolve from trusted binary directories (system defaults - plus optional `tools.exec.safeBinTrustedDirs`). `PATH` entries are never - auto-trusted. Default trusted directories are intentionally minimal: - `/bin`, `/usr/bin`. If your safe-bin executable lives in - package-manager/user paths (for example `/opt/homebrew/bin`, - `/usr/local/bin`, `/opt/local/bin`, `/snap/bin`), add them explicitly to - `tools.exec.safeBinTrustedDirs`. - + + Safe bins must resolve from trusted binary directories (system defaults + plus optional `tools.exec.safeBinTrustedDirs`). `PATH` entries are never + auto-trusted. Default trusted directories are intentionally minimal: + `/bin`, `/usr/bin`. If your safe-bin executable lives in + package-manager/user paths (for example `/opt/homebrew/bin`, + `/usr/local/bin`, `/opt/local/bin`, `/snap/bin`), add them explicitly to + `tools.exec.safeBinTrustedDirs`. + - - Shell chaining (`&&`, `||`, `;`) is allowed when every top-level segment - satisfies the allowlist (including safe bins or skill auto-allow). - Redirections remain unsupported in allowlist mode. Command substitution - (`$()` / backticks) is rejected during allowlist parsing, including inside - double quotes; use single quotes if you need literal `$()` text. + + Shell chaining (`&&`, `||`, `;`) is allowed when every top-level segment + satisfies the allowlist (including safe bins or skill auto-allow). + Redirections remain unsupported in allowlist mode. Command substitution + (`$()` / backticks) is rejected during allowlist parsing, including inside + double quotes; use single quotes if you need literal `$()` text. - On macOS companion-app approvals, raw shell text containing shell control - or expansion syntax (`&&`, `||`, `;`, `|`, `` ` ``, `$`, `<`, `>`, `(`, - `)`) is treated as an allowlist miss unless the shell binary itself is - allowlisted. + On macOS companion-app approvals, raw shell text containing shell control + or expansion syntax (`&&`, `||`, `;`, `|`, `` ` ``, `$`, `<`, `>`, `(`, + `)`) is treated as an allowlist miss unless the shell binary itself is + allowlisted. - For shell wrappers (`bash|sh|zsh ... -c/-lc`), request-scoped env - overrides are reduced to a small explicit allowlist (`TERM`, `LANG`, - `LC_*`, `COLORTERM`, `NO_COLOR`, `FORCE_COLOR`). + For shell wrappers (`bash|sh|zsh ... -c/-lc`), request-scoped env + overrides are reduced to a small explicit allowlist (`TERM`, `LANG`, + `LC_*`, `COLORTERM`, `NO_COLOR`, `FORCE_COLOR`). - For `allow-always` decisions in allowlist mode, known dispatch wrappers - (`env`, `nice`, `nohup`, `stdbuf`, `timeout`) persist the inner executable - path instead of the wrapper path. Shell multiplexers (`busybox`, `toybox`) - are unwrapped for shell applets (`sh`, `ash`, etc.) the same way. If a - wrapper or multiplexer cannot be safely unwrapped, no allowlist entry is - persisted automatically. + For `allow-always` decisions in allowlist mode, known dispatch wrappers + (`env`, `nice`, `nohup`, `stdbuf`, `timeout`) persist the inner executable + path instead of the wrapper path. Shell multiplexers (`busybox`, `toybox`) + are unwrapped for shell applets (`sh`, `ash`, etc.) the same way. If a + wrapper or multiplexer cannot be safely unwrapped, no allowlist entry is + persisted automatically. - If you allowlist interpreters like `python3` or `node`, prefer - `tools.exec.strictInlineEval=true` so inline eval still requires an - explicit approval. In strict mode, `allow-always` can still persist benign - interpreter/script invocations, but inline-eval carriers are not persisted - automatically. + If you allowlist interpreters like `python3` or `node`, prefer + `tools.exec.strictInlineEval=true` so inline eval still requires an + explicit approval. In strict mode, `allow-always` can still persist benign + interpreter/script invocations, but inline-eval carriers are not persisted + automatically. diff --git a/src/infra/exec-approval-channel-runtime.test.ts b/src/infra/exec-approval-channel-runtime.test.ts index 43aceb9201f..16925693304 100644 --- a/src/infra/exec-approval-channel-runtime.test.ts +++ b/src/infra/exec-approval-channel-runtime.test.ts @@ -1,5 +1,6 @@ import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import type { GatewayClient } from "../gateway/client.js"; +import type { ExecApprovalRequest } from "./exec-approvals.js"; import type { PluginApprovalRequest, PluginApprovalResolved } from "./plugin-approvals.js"; const mockGatewayClientStarts = vi.hoisted(() => vi.fn()); @@ -46,15 +47,45 @@ function lastGatewayEventClientParams(): GatewayEventClientParams | undefined { function emitPluginApprovalRequested(clientParams = lastGatewayEventClientParams()) { clientParams?.onEvent?.({ event: "plugin.approval.requested", - payload: { - id: "plugin:abc", - request: { - title: "Plugin approval", - description: "Let plugin proceed", - }, - createdAtMs: 1000, - expiresAtMs: 2000, + payload: createPluginReplayRequest("plugin:abc"), + }); +} + +function createExecReplayRequest(id = "abc"): ExecApprovalRequest { + return { + id, + request: { + command: "echo abc", }, + createdAtMs: 1000, + expiresAtMs: 2000, + }; +} + +function createPluginReplayRequest(id = "plugin:abc"): PluginApprovalRequest { + return { + id, + request: { + title: "Plugin approval", + description: "Let plugin proceed", + }, + createdAtMs: 1000, + expiresAtMs: 2000, + }; +} + +function mockReplayLists(params: { + exec?: ExecApprovalRequest[]; + plugin?: PluginApprovalRequest[]; +}) { + mockGatewayClientRequests.mockImplementation(async (method: string) => { + if (method === "exec.approval.list") { + return params.exec ?? []; + } + if (method === "plugin.approval.list") { + return params.plugin ?? []; + } + return { ok: true }; }); } @@ -400,21 +431,7 @@ describe("createExecApprovalChannelRuntime", () => { }); it("replays pending approvals after the gateway connection is ready", async () => { - mockGatewayClientRequests.mockImplementation(async (method: string) => { - if (method === "exec.approval.list") { - return [ - { - id: "abc", - request: { - command: "echo abc", - }, - createdAtMs: 1000, - expiresAtMs: 2000, - }, - ]; - } - return { ok: true }; - }); + mockReplayLists({ exec: [createExecReplayRequest()] }); const deliverRequested = vi.fn(async (request) => [{ id: request.id }]); const runtime = createExecApprovalChannelRuntime({ label: "test/replay", @@ -428,31 +445,46 @@ describe("createExecApprovalChannelRuntime", () => { await runtime.start(); - expect(mockGatewayClientRequests).toHaveBeenCalledWith("exec.approval.list", {}); - expect(deliverRequested).toHaveBeenCalledWith( - expect.objectContaining({ - id: "abc", - }), - ); + await vi.waitFor(() => { + expect(mockGatewayClientRequests).toHaveBeenCalledWith("exec.approval.list", {}); + expect(deliverRequested).toHaveBeenCalledWith( + expect.objectContaining({ + id: "abc", + }), + ); + }); + }); + + it("does not block start on pending approval replay delivery", async () => { + mockReplayLists({ exec: [createExecReplayRequest()] }); + const pendingDelivery = createDeferred>(); + const deliverRequested = vi.fn(async () => pendingDelivery.promise); + const runtime = createExecApprovalChannelRuntime({ + label: "test/replay-start", + clientDisplayName: "Test Replay Start", + cfg: {} as never, + nowMs: () => 1000, + isConfigured: () => true, + shouldHandle: () => true, + deliverRequested, + finalizeResolved: async () => undefined, + }); + + await runtime.start(); + + await vi.waitFor(() => { + expect(deliverRequested).toHaveBeenCalledWith( + expect.objectContaining({ + id: "abc", + }), + ); + }); + pendingDelivery.resolve([{ id: "abc" }]); + await runtime.stop(); }); it("ignores live duplicate approval events after replay", async () => { - mockGatewayClientRequests.mockImplementation(async (method: string) => { - if (method === "plugin.approval.list") { - return [ - { - id: "plugin:abc", - request: { - title: "Plugin approval", - description: "Let plugin proceed", - }, - createdAtMs: 1000, - expiresAtMs: 2000, - }, - ]; - } - return { ok: true }; - }); + mockReplayLists({ plugin: [createPluginReplayRequest()] }); const deliverRequested = vi.fn(async (request) => [{ id: request.id }]); const runtime = createExecApprovalChannelRuntime< { id: string }, @@ -470,21 +502,50 @@ describe("createExecApprovalChannelRuntime", () => { }); await runtime.start(); + await vi.waitFor(() => { + expect(deliverRequested).toHaveBeenCalledTimes(1); + }); emitPluginApprovalRequested(); await Promise.resolve(); expect(deliverRequested).toHaveBeenCalledTimes(1); }); + it("ignores live duplicate approval events while replay delivery is still in flight", async () => { + mockReplayLists({ plugin: [createPluginReplayRequest()] }); + const pendingDelivery = createDeferred>(); + const deliverRequested = vi.fn(async () => pendingDelivery.promise); + const runtime = createExecApprovalChannelRuntime< + { id: string }, + PluginApprovalRequest, + PluginApprovalResolved + >({ + label: "test/plugin-replay-live-duplicate", + clientDisplayName: "Test Plugin Replay Live Duplicate", + cfg: {} as never, + nowMs: () => 1000, + eventKinds: ["plugin"], + isConfigured: () => true, + shouldHandle: () => true, + deliverRequested, + finalizeResolved: async () => undefined, + }); + + await runtime.start(); + await vi.waitFor(() => { + expect(deliverRequested).toHaveBeenCalledTimes(1); + }); + + emitPluginApprovalRequested(); + await Promise.resolve(); + expect(deliverRequested).toHaveBeenCalledTimes(1); + + pendingDelivery.resolve([{ id: "plugin:abc" }]); + await runtime.stop(); + }); + it("does not replay approvals after stop wins once hello is already complete", async () => { - const replayDeferred = createDeferred< - Array<{ - id: string; - request: { command: string }; - createdAtMs: number; - expiresAtMs: number; - }> - >(); + const replayDeferred = createDeferred(); mockGatewayClientRequests.mockImplementation(async (method: string) => { if (method === "exec.approval.list") { return replayDeferred.promise; @@ -508,22 +569,103 @@ describe("createExecApprovalChannelRuntime", () => { }); const stopPromise = runtime.stop(); - replayDeferred.resolve([ - { - id: "abc", - request: { - command: "echo abc", - }, - createdAtMs: 1000, - expiresAtMs: 2000, - }, - ]); + replayDeferred.resolve([createExecReplayRequest()]); await startPromise; await stopPromise; expect(deliverRequested).not.toHaveBeenCalled(); expect(mockGatewayClientStops).toHaveBeenCalled(); + expect(loggerMocks.error).not.toHaveBeenCalled(); + }); + + it("waits for in-flight replay delivery before running stopped cleanup", async () => { + mockReplayLists({ exec: [createExecReplayRequest()] }); + const pendingDelivery = createDeferred>(); + const deliverRequested = vi.fn(async () => pendingDelivery.promise); + const onStopped = vi.fn(async () => undefined); + const runtime = createExecApprovalChannelRuntime({ + label: "test/replay-stop-waits", + clientDisplayName: "Test Replay Stop Waits", + cfg: {} as never, + nowMs: () => 1000, + isConfigured: () => true, + shouldHandle: () => true, + deliverRequested, + finalizeResolved: async () => undefined, + onStopped, + }); + + await runtime.start(); + await vi.waitFor(() => { + expect(deliverRequested).toHaveBeenCalledTimes(1); + }); + + let stopResolved = false; + const stopPromise = runtime.stop().then(() => { + stopResolved = true; + }); + await Promise.resolve(); + expect(stopResolved).toBe(false); + expect(onStopped).not.toHaveBeenCalled(); + + pendingDelivery.resolve([{ id: "abc" }]); + await stopPromise; + + expect(stopResolved).toBe(true); + expect(onStopped).toHaveBeenCalledTimes(1); + expect(loggerMocks.error).not.toHaveBeenCalled(); + }); + + it("logs replay delivery failures without failing startup", async () => { + mockReplayLists({ exec: [createExecReplayRequest()] }); + const runtime = createExecApprovalChannelRuntime({ + label: "test/replay-error", + clientDisplayName: "Test Replay Error", + cfg: {} as never, + isConfigured: () => true, + shouldHandle: () => true, + deliverRequested: async () => { + throw new Error("deliver failed"); + }, + finalizeResolved: async () => undefined, + }); + + await expect(runtime.start()).resolves.toBeUndefined(); + + await vi.waitFor(() => { + expect(loggerMocks.error).toHaveBeenCalledWith( + "error replaying pending approvals: deliver failed", + ); + }); + }); + + it("logs replay list failures without failing startup", async () => { + mockGatewayClientRequests.mockImplementation(async (method: string) => { + if (method === "exec.approval.list") { + throw new Error("list failed"); + } + return { ok: true }; + }); + const deliverRequested = vi.fn(async (request) => [{ id: request.id }]); + const runtime = createExecApprovalChannelRuntime({ + label: "test/replay-list-error", + clientDisplayName: "Test Replay List Error", + cfg: {} as never, + isConfigured: () => true, + shouldHandle: () => true, + deliverRequested, + finalizeResolved: async () => undefined, + }); + + await expect(runtime.start()).resolves.toBeUndefined(); + + await vi.waitFor(() => { + expect(loggerMocks.error).toHaveBeenCalledWith( + "error replaying pending approvals: list failed", + ); + }); + expect(deliverRequested).not.toHaveBeenCalled(); }); it("clears pending state when delivery throws", async () => { diff --git a/src/infra/exec-approval-channel-runtime.ts b/src/infra/exec-approval-channel-runtime.ts index 3a140910694..938d2332843 100644 --- a/src/infra/exec-approval-channel-runtime.ts +++ b/src/infra/exec-approval-channel-runtime.ts @@ -59,6 +59,7 @@ export function createExecApprovalChannelRuntime< let started = false; let shouldRun = false; let startPromise: Promise | null = null; + let replayPromise: Promise | null = null; const shouldKeepRunning = (): boolean => shouldRun; @@ -212,6 +213,53 @@ export function createExecApprovalChannelRuntime< } }; + const replayPendingApprovals = async (client: GatewayClient): Promise => { + try { + for (const method of resolveApprovalReplayMethods(eventKinds)) { + if (stopClientIfInactive(client)) { + return; + } + const pendingRequests = await client.request>(method, {}); + if (stopClientIfInactive(client)) { + return; + } + for (const request of pendingRequests) { + if (stopClientIfInactive(client)) { + return; + } + await handleRequested(request, { ignoreIfInactive: true }); + } + } + } catch (error) { + if (!shouldKeepRunning()) { + return; + } + throw error; + } + }; + + const startPendingApprovalReplay = (client: GatewayClient): void => { + const promise = replayPendingApprovals(client) + .catch((err: unknown) => { + const message = formatErrorMessage(err); + log.error(`error replaying pending approvals: ${message}`); + }) + .finally(() => { + if (replayPromise === promise) { + replayPromise = null; + } + }); + replayPromise = promise; + }; + + const waitForPendingApprovalReplay = async (): Promise => { + const replay = replayPromise; + if (!replay) { + return; + } + await replay.catch(() => {}); + }; + return { async start(): Promise { if (started) { @@ -275,22 +323,8 @@ export function createExecApprovalChannelRuntime< if (stopClientIfInactive(client)) { return; } - for (const method of resolveApprovalReplayMethods(eventKinds)) { - if (stopClientIfInactive(client)) { - return; - } - const pendingRequests = await client.request>(method, {}); - if (stopClientIfInactive(client)) { - return; - } - for (const request of pendingRequests) { - if (stopClientIfInactive(client)) { - return; - } - await handleRequested(request, { ignoreIfInactive: true }); - } - } started = true; + startPendingApprovalReplay(client); } catch (error) { gatewayClient = null; started = false; @@ -309,19 +343,21 @@ export function createExecApprovalChannelRuntime< if (startPromise) { await startPromise.catch(() => {}); } - if (!started && !gatewayClient) { + const wasActive = started || gatewayClient !== null || replayPromise !== null; + started = false; + gatewayClient?.stop(); + gatewayClient = null; + await waitForPendingApprovalReplay(); + if (!wasActive) { await adapter.onStopped?.(); return; } - started = false; for (const entry of pending.values()) { if (entry.timeoutId) { clearTimeout(entry.timeoutId); } } pending.clear(); - gatewayClient?.stop(); - gatewayClient = null; await adapter.onStopped?.(); log.debug("stopped"); },