fix(gateway): fail fast exec approvals when no approvers are reachable

Co-authored-by: fanxian831-netizen <262880470+fanxian831-netizen@users.noreply.github.com>
This commit is contained in:
Peter Steinberger
2026-02-22 22:13:40 +01:00
parent 73fab7e445
commit d24f5c1e3a
8 changed files with 168 additions and 47 deletions

View File

@@ -34,6 +34,7 @@ Docs: https://docs.openclaw.ai
- Plugins/Media sandbox: propagate trusted `mediaLocalRoots` through plugin action dispatch (including Discord/Telegram action adapters) so plugin send paths enforce the same agent-scoped local-media sandbox roots as core outbound sends. (#20258, #22718) - Plugins/Media sandbox: propagate trusted `mediaLocalRoots` through plugin action dispatch (including Discord/Telegram action adapters) so plugin send paths enforce the same agent-scoped local-media sandbox roots as core outbound sends. (#20258, #22718)
- Agents/Workspace guard: map sandbox container-workdir file-tool paths (for example `/workspace/...` and `file:///workspace/...`) to host workspace roots before workspace-only validation, preventing false `Path escapes sandbox root` rejections for sandbox file tools. (#9560) - Agents/Workspace guard: map sandbox container-workdir file-tool paths (for example `/workspace/...` and `file:///workspace/...`) to host workspace roots before workspace-only validation, preventing false `Path escapes sandbox root` rejections for sandbox file tools. (#9560)
- Gateway/Exec approvals: expire approval requests immediately when no approval-capable gateway clients are connected and no forwarding targets are available, avoiding delayed approvals after restarts/offline approver windows. (#22144)
- Slack/Threading: sessions: keep parent-session forking and thread-history context active beyond first turn by removing first-turn-only gates in session init, thread-history fetch, and reply prompt context injection. (#23843, #23090) Thanks @vincentkoc and @Taskle. - Slack/Threading: sessions: keep parent-session forking and thread-history context active beyond first turn by removing first-turn-only gates in session init, thread-history fetch, and reply prompt context injection. (#23843, #23090) Thanks @vincentkoc and @Taskle.
- Slack/Threading: respect `replyToMode` when Slack auto-populates top-level `thread_ts`, and ignore inline `replyToId` directive tags when `replyToMode` is `off` so thread forcing stays disabled unless explicitly configured. (#23839, #23320, #23513) Thanks @vincentkoc and @dorukardahan. - Slack/Threading: respect `replyToMode` when Slack auto-populates top-level `thread_ts`, and ignore inline `replyToId` directive tags when `replyToMode` is `off` so thread forcing stays disabled unless explicitly configured. (#23839, #23320, #23513) Thanks @vincentkoc and @dorukardahan.
- Slack/Extension: forward `message read` `threadId` to `readMessages` and use delivery-context `threadId` as outbound `thread_ts` fallback so extension replies/reads stay in the correct Slack thread. (#22216, #22485, #23836) Thanks @vincentkoc, @lan17 and @dorukardahan. - Slack/Extension: forward `message read` `threadId` to `readMessages` and use delivery-context `threadId` as outbound `thread_ts` fallback so extension replies/reads stay in the correct Slack thread. (#22216, #22485, #23836) Thanks @vincentkoc, @lan17 and @dorukardahan.

View File

@@ -86,18 +86,7 @@ export class ExecApprovalManager {
promise, promise,
}; };
entry.timer = setTimeout(() => { entry.timer = setTimeout(() => {
// Update snapshot fields before resolving (mirror resolve()'s bookkeeping) this.expire(record.id);
record.resolvedAtMs = Date.now();
record.decision = undefined;
record.resolvedBy = null;
resolvePromise(null);
// Keep entry briefly for in-flight awaitDecision calls
setTimeout(() => {
// Compare against captured entry instance, not re-fetched from map
if (this.pending.get(record.id) === entry) {
this.pending.delete(record.id);
}
}, RESOLVED_ENTRY_GRACE_MS);
}, timeoutMs); }, timeoutMs);
this.pending.set(record.id, entry); this.pending.set(record.id, entry);
return promise; return promise;
@@ -138,6 +127,27 @@ export class ExecApprovalManager {
return true; return true;
} }
expire(recordId: string, resolvedBy?: string | null): boolean {
const pending = this.pending.get(recordId);
if (!pending) {
return false;
}
if (pending.record.resolvedAtMs !== undefined) {
return false;
}
clearTimeout(pending.timer);
pending.record.resolvedAtMs = Date.now();
pending.record.decision = undefined;
pending.record.resolvedBy = resolvedBy ?? null;
pending.resolve(null);
setTimeout(() => {
if (this.pending.get(recordId) === pending) {
this.pending.delete(recordId);
}
}, RESOLVED_ENTRY_GRACE_MS);
return true;
}
getSnapshot(recordId: string): ExecApprovalRecord | null { getSnapshot(recordId: string): ExecApprovalRecord | null {
const entry = this.pending.get(recordId); const entry = this.pending.get(recordId);
return entry?.record ?? null; return entry?.record ?? null;

View File

@@ -17,6 +17,14 @@ export function createExecApprovalHandlers(
manager: ExecApprovalManager, manager: ExecApprovalManager,
opts?: { forwarder?: ExecApprovalForwarder }, opts?: { forwarder?: ExecApprovalForwarder },
): GatewayRequestHandlers { ): GatewayRequestHandlers {
const hasApprovalClients = (context: { hasExecApprovalClients?: () => boolean }) => {
if (typeof context.hasExecApprovalClients === "function") {
return context.hasExecApprovalClients();
}
// Fail closed when no operator-scope probe is available.
return false;
};
return { return {
"exec.approval.request": async ({ params, respond, context, client }) => { "exec.approval.request": async ({ params, respond, context, client }) => {
if (!validateExecApprovalRequestParams(params)) { if (!validateExecApprovalRequestParams(params)) {
@@ -96,16 +104,23 @@ export function createExecApprovalHandlers(
}, },
{ dropIfSlow: true }, { dropIfSlow: true },
); );
void opts?.forwarder let forwardedToTargets = false;
?.handleRequested({ if (opts?.forwarder) {
id: record.id, try {
request: record.request, forwardedToTargets = await opts.forwarder.handleRequested({
createdAtMs: record.createdAtMs, id: record.id,
expiresAtMs: record.expiresAtMs, request: record.request,
}) createdAtMs: record.createdAtMs,
.catch((err) => { expiresAtMs: record.expiresAtMs,
});
} catch (err) {
context.logGateway?.error?.(`exec approvals: forward request failed: ${String(err)}`); context.logGateway?.error?.(`exec approvals: forward request failed: ${String(err)}`);
}); }
}
if (!hasApprovalClients(context) && !forwardedToTargets) {
manager.expire(record.id, "auto-expire:no-approver-clients");
}
// Only send immediate "accepted" response when twoPhase is requested. // Only send immediate "accepted" response when twoPhase is requested.
// This preserves single-response semantics for existing callers. // This preserves single-response semantics for existing callers.

View File

@@ -254,6 +254,7 @@ describe("exec approval handlers", () => {
function toExecApprovalRequestContext(context: { function toExecApprovalRequestContext(context: {
broadcast: (event: string, payload: unknown) => void; broadcast: (event: string, payload: unknown) => void;
hasExecApprovalClients?: () => boolean;
}): ExecApprovalRequestArgs["context"] { }): ExecApprovalRequestArgs["context"] {
return context as unknown as ExecApprovalRequestArgs["context"]; return context as unknown as ExecApprovalRequestArgs["context"];
} }
@@ -277,7 +278,10 @@ describe("exec approval handlers", () => {
return params.handlers["exec.approval.request"]({ return params.handlers["exec.approval.request"]({
params: requestParams, params: requestParams,
respond: params.respond as unknown as ExecApprovalRequestArgs["respond"], respond: params.respond as unknown as ExecApprovalRequestArgs["respond"],
context: toExecApprovalRequestContext(params.context), context: toExecApprovalRequestContext({
hasExecApprovalClients: () => true,
...params.context,
}),
client: null, client: null,
req: { id: "req-1", type: "req", method: "exec.approval.request" }, req: { id: "req-1", type: "req", method: "exec.approval.request" },
isWebchatConnect: execApprovalNoop, isWebchatConnect: execApprovalNoop,
@@ -309,6 +313,7 @@ describe("exec approval handlers", () => {
broadcast: (event: string, payload: unknown) => { broadcast: (event: string, payload: unknown) => {
broadcasts.push({ event, payload }); broadcasts.push({ event, payload });
}, },
hasExecApprovalClients: () => true,
}; };
return { handlers, broadcasts, respond, context }; return { handlers, broadcasts, respond, context };
} }
@@ -463,6 +468,46 @@ describe("exec approval handlers", () => {
); );
expect(resolveRespond).toHaveBeenCalledWith(true, { ok: true }, undefined); expect(resolveRespond).toHaveBeenCalledWith(true, { ok: true }, undefined);
}); });
it("expires immediately when no approver clients and no forwarding targets", async () => {
vi.useFakeTimers();
try {
const manager = new ExecApprovalManager();
const forwarder = {
handleRequested: vi.fn(async () => false),
handleResolved: vi.fn(async () => {}),
stop: vi.fn(),
};
const handlers = createExecApprovalHandlers(manager, { forwarder });
const respond = vi.fn();
const context = {
broadcast: (_event: string, _payload: unknown) => {},
hasExecApprovalClients: () => false,
};
const expireSpy = vi.spyOn(manager, "expire");
const requestPromise = requestExecApproval({
handlers,
respond,
context,
params: { timeoutMs: 60_000 },
});
for (let idx = 0; idx < 20; idx += 1) {
await Promise.resolve();
}
expect(forwarder.handleRequested).toHaveBeenCalledTimes(1);
expect(expireSpy).toHaveBeenCalledTimes(1);
await vi.runOnlyPendingTimersAsync();
await requestPromise;
expect(respond).toHaveBeenCalledWith(
true,
expect.objectContaining({ decision: null }),
undefined,
);
} finally {
vi.useRealTimers();
}
});
}); });
describe("gateway healthHandlers.status scope handling", () => { describe("gateway healthHandlers.status scope handling", () => {

View File

@@ -47,6 +47,7 @@ export type GatewayRequestContext = {
nodeUnsubscribe: (nodeId: string, sessionKey: string) => void; nodeUnsubscribe: (nodeId: string, sessionKey: string) => void;
nodeUnsubscribeAll: (nodeId: string) => void; nodeUnsubscribeAll: (nodeId: string) => void;
hasConnectedMobileNode: () => boolean; hasConnectedMobileNode: () => boolean;
hasExecApprovalClients?: () => boolean;
nodeRegistry: NodeRegistry; nodeRegistry: NodeRegistry;
agentRunSeq: Map<string, number>; agentRunSeq: Map<string, number>;
chatAbortControllers: Map<string, ChatAbortControllerEntry>; chatAbortControllers: Map<string, ChatAbortControllerEntry>;

View File

@@ -602,6 +602,17 @@ export async function startGatewayServer(
nodeUnsubscribe, nodeUnsubscribe,
nodeUnsubscribeAll, nodeUnsubscribeAll,
hasConnectedMobileNode: hasMobileNodeConnected, hasConnectedMobileNode: hasMobileNodeConnected,
hasExecApprovalClients: () => {
for (const gatewayClient of clients) {
const scopes = Array.isArray(gatewayClient.connect.scopes)
? gatewayClient.connect.scopes
: [];
if (scopes.includes("operator.admin") || scopes.includes("operator.approvals")) {
return true;
}
}
return false;
},
nodeRegistry, nodeRegistry,
agentRunSeq, agentRunSeq,
chatAbortControllers, chatAbortControllers,

View File

@@ -63,7 +63,7 @@ describe("exec approval forwarder", () => {
resolveSessionTarget: () => ({ channel: "slack", to: "U1" }), resolveSessionTarget: () => ({ channel: "slack", to: "U1" }),
}); });
await forwarder.handleRequested(baseRequest); await expect(forwarder.handleRequested(baseRequest)).resolves.toBe(true);
expect(deliver).toHaveBeenCalledTimes(1); expect(deliver).toHaveBeenCalledTimes(1);
await forwarder.handleResolved({ await forwarder.handleResolved({
@@ -82,7 +82,7 @@ describe("exec approval forwarder", () => {
vi.useFakeTimers(); vi.useFakeTimers();
const { deliver, forwarder } = createForwarder({ cfg: TARGETS_CFG }); const { deliver, forwarder } = createForwarder({ cfg: TARGETS_CFG });
await forwarder.handleRequested(baseRequest); await expect(forwarder.handleRequested(baseRequest)).resolves.toBe(true);
expect(deliver).toHaveBeenCalledTimes(1); expect(deliver).toHaveBeenCalledTimes(1);
await vi.runAllTimersAsync(); await vi.runAllTimersAsync();
@@ -93,7 +93,7 @@ describe("exec approval forwarder", () => {
vi.useFakeTimers(); vi.useFakeTimers();
const { deliver, forwarder } = createForwarder({ cfg: TARGETS_CFG }); const { deliver, forwarder } = createForwarder({ cfg: TARGETS_CFG });
await forwarder.handleRequested(baseRequest); await expect(forwarder.handleRequested(baseRequest)).resolves.toBe(true);
expect(getFirstDeliveryText(deliver)).toContain("Command: `echo hello`"); expect(getFirstDeliveryText(deliver)).toContain("Command: `echo hello`");
}); });
@@ -102,17 +102,50 @@ describe("exec approval forwarder", () => {
vi.useFakeTimers(); vi.useFakeTimers();
const { deliver, forwarder } = createForwarder({ cfg: TARGETS_CFG }); const { deliver, forwarder } = createForwarder({ cfg: TARGETS_CFG });
await forwarder.handleRequested({ await expect(
...baseRequest, forwarder.handleRequested({
request: { ...baseRequest,
...baseRequest.request, request: {
command: "echo `uname`\necho done", ...baseRequest.request,
}, command: "echo `uname`\necho done",
}); },
}),
).resolves.toBe(true);
expect(getFirstDeliveryText(deliver)).toContain("Command:\n```\necho `uname`\necho done\n```"); expect(getFirstDeliveryText(deliver)).toContain("Command:\n```\necho `uname`\necho done\n```");
}); });
it("returns false when forwarding is disabled", async () => {
const { deliver, forwarder } = createForwarder({
cfg: {} as OpenClawConfig,
});
await expect(forwarder.handleRequested(baseRequest)).resolves.toBe(false);
expect(deliver).not.toHaveBeenCalled();
});
it("returns false when all targets are skipped", async () => {
vi.useFakeTimers();
const cfg = {
channels: {
discord: {
execApprovals: {
enabled: true,
approvers: ["123"],
},
},
},
approvals: { exec: { enabled: true, mode: "session" } },
} as OpenClawConfig;
const { deliver, forwarder } = createForwarder({
cfg,
resolveSessionTarget: () => ({ channel: "discord", to: "channel:123" }),
});
await expect(forwarder.handleRequested(baseRequest)).resolves.toBe(false);
expect(deliver).not.toHaveBeenCalled();
});
it("forwards to discord when discord exec approvals handler is disabled", async () => { it("forwards to discord when discord exec approvals handler is disabled", async () => {
vi.useFakeTimers(); vi.useFakeTimers();
const cfg = { const cfg = {
@@ -124,7 +157,7 @@ describe("exec approval forwarder", () => {
resolveSessionTarget: () => ({ channel: "discord", to: "channel:123" }), resolveSessionTarget: () => ({ channel: "discord", to: "channel:123" }),
}); });
await forwarder.handleRequested(baseRequest); await expect(forwarder.handleRequested(baseRequest)).resolves.toBe(true);
expect(deliver).toHaveBeenCalledTimes(1); expect(deliver).toHaveBeenCalledTimes(1);
}); });
@@ -148,7 +181,7 @@ describe("exec approval forwarder", () => {
resolveSessionTarget: () => ({ channel: "discord", to: "channel:123" }), resolveSessionTarget: () => ({ channel: "discord", to: "channel:123" }),
}); });
await forwarder.handleRequested(baseRequest); await expect(forwarder.handleRequested(baseRequest)).resolves.toBe(false);
expect(deliver).not.toHaveBeenCalled(); expect(deliver).not.toHaveBeenCalled();
}); });
@@ -185,13 +218,15 @@ describe("exec approval forwarder", () => {
vi.useFakeTimers(); vi.useFakeTimers();
const { deliver, forwarder } = createForwarder({ cfg: TARGETS_CFG }); const { deliver, forwarder } = createForwarder({ cfg: TARGETS_CFG });
await forwarder.handleRequested({ await expect(
...baseRequest, forwarder.handleRequested({
request: { ...baseRequest,
...baseRequest.request, request: {
command: "echo ```danger```", ...baseRequest.request,
}, command: "echo ```danger```",
}); },
}),
).resolves.toBe(true);
expect(getFirstDeliveryText(deliver)).toContain("Command:\n````\necho ```danger```\n````"); expect(getFirstDeliveryText(deliver)).toContain("Command:\n````\necho ```danger```\n````");
}); });

View File

@@ -29,7 +29,7 @@ type PendingApproval = {
}; };
export type ExecApprovalForwarder = { export type ExecApprovalForwarder = {
handleRequested: (request: ExecApprovalRequest) => Promise<void>; handleRequested: (request: ExecApprovalRequest) => Promise<boolean>;
handleResolved: (resolved: ExecApprovalResolved) => Promise<void>; handleResolved: (resolved: ExecApprovalResolved) => Promise<void>;
stop: () => void; stop: () => void;
}; };
@@ -318,11 +318,11 @@ export function createExecApprovalForwarder(
const resolveSessionTarget = deps.resolveSessionTarget ?? defaultResolveSessionTarget; const resolveSessionTarget = deps.resolveSessionTarget ?? defaultResolveSessionTarget;
const pending = new Map<string, PendingApproval>(); const pending = new Map<string, PendingApproval>();
const handleRequested = async (request: ExecApprovalRequest) => { const handleRequested = async (request: ExecApprovalRequest): Promise<boolean> => {
const cfg = getConfig(); const cfg = getConfig();
const config = cfg.approvals?.exec; const config = cfg.approvals?.exec;
if (!shouldForward({ config, request })) { if (!shouldForward({ config, request })) {
return; return false;
} }
const filteredTargets = resolveForwardTargets({ const filteredTargets = resolveForwardTargets({
cfg, cfg,
@@ -332,7 +332,7 @@ export function createExecApprovalForwarder(
}).filter((target) => !shouldSkipDiscordForwarding(target, cfg)); }).filter((target) => !shouldSkipDiscordForwarding(target, cfg));
if (filteredTargets.length === 0) { if (filteredTargets.length === 0) {
return; return false;
} }
const expiresInMs = Math.max(0, request.expiresAtMs - nowMs()); const expiresInMs = Math.max(0, request.expiresAtMs - nowMs());
@@ -353,17 +353,20 @@ export function createExecApprovalForwarder(
pending.set(request.id, pendingEntry); pending.set(request.id, pendingEntry);
if (pending.get(request.id) !== pendingEntry) { if (pending.get(request.id) !== pendingEntry) {
return; return false;
} }
const text = buildRequestMessage(request, nowMs()); const text = buildRequestMessage(request, nowMs());
await deliverToTargets({ void deliverToTargets({
cfg, cfg,
targets: filteredTargets, targets: filteredTargets,
text, text,
deliver, deliver,
shouldSend: () => pending.get(request.id) === pendingEntry, shouldSend: () => pending.get(request.id) === pendingEntry,
}).catch((err) => {
log.error(`exec approvals: failed to deliver request ${request.id}: ${String(err)}`);
}); });
return true;
}; };
const handleResolved = async (resolved: ExecApprovalResolved) => { const handleResolved = async (resolved: ExecApprovalResolved) => {