fix: decouple approval startup from replay

This commit is contained in:
Peter Steinberger
2026-04-23 18:43:58 +01:00
parent 62a0cd8acd
commit 393a71991f
4 changed files with 302 additions and 123 deletions

View File

@@ -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.

View File

@@ -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.
</Accordion>
</Accordion>
<Accordion title="Trusted binary directories">
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`.
</Accordion>
<Accordion title="Trusted binary directories">
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`.
</Accordion>
<Accordion title="Shell chaining, wrappers, and multiplexers">
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.
<Accordion title="Shell chaining, wrappers, and multiplexers">
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.
</Accordion>
</AccordionGroup>

View File

@@ -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<Array<{ id: string }>>();
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<Array<{ id: string }>>();
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<ExecApprovalRequest[]>();
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<Array<{ id: string }>>();
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 () => {

View File

@@ -59,6 +59,7 @@ export function createExecApprovalChannelRuntime<
let started = false;
let shouldRun = false;
let startPromise: Promise<void> | null = null;
let replayPromise: Promise<void> | null = null;
const shouldKeepRunning = (): boolean => shouldRun;
@@ -212,6 +213,53 @@ export function createExecApprovalChannelRuntime<
}
};
const replayPendingApprovals = async (client: GatewayClient): Promise<void> => {
try {
for (const method of resolveApprovalReplayMethods(eventKinds)) {
if (stopClientIfInactive(client)) {
return;
}
const pendingRequests = await client.request<Array<TRequest>>(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<void> => {
const replay = replayPromise;
if (!replay) {
return;
}
await replay.catch(() => {});
};
return {
async start(): Promise<void> {
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<Array<TRequest>>(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");
},