From c8003f1b33ed2924be5f62131bd28742c5a41aae Mon Sep 17 00:00:00 2001 From: Agustin Rivera <31522568+eleqtrizit@users.noreply.github.com> Date: Tue, 14 Apr 2026 12:50:41 -0700 Subject: [PATCH] Harden Feishu webhook replay guards (#66707) * fix(feishu): harden webhook replay guards * changelog: note Feishu webhook + card-action fail-closed hardening (#66707) * fix(feishu): move blank-token check above decodeFeishuCardAction Run the early-return guard against a missing/blank card-action token before decoding the card-action payload. Decoding is side-effect-free so this is a readability + tiny-perf nit, not a correctness change. Matches Greptile's P2 suggestion. --------- Co-authored-by: Devin Robison --- CHANGELOG.md | 1 + extensions/feishu/src/bot.card-action.test.ts | 23 +++++++++++ extensions/feishu/src/card-action.ts | 8 +++- .../src/monitor.card-action.lifecycle.test.ts | 39 ++++++++++++++++++- extensions/feishu/src/monitor.transport.ts | 10 +++-- .../src/monitor.webhook-security.test.ts | 30 ++++++++++++++ 6 files changed, 106 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 649bfbe7ae8..4ccad11ee76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ Docs: https://docs.openclaw.ai - Agents/failover: classify OpenAI-compatible `finish_reason: network_error` stream failures as timeout so model fallback retries continue instead of stopping with an unknown failover reason. (#61784) thanks @lawrence3699. - Onboarding/channels: normalize channel setup metadata before discovery and validation so malformed or mixed-shape channel plugin metadata no longer breaks setup and onboarding channel lists. (#66706) Thanks @darkamenosa. - Slack/native commands: fix option menus for slash commands such as `/verbose` when Slack renders native buttons by giving each button a unique action ID while still routing them through the shared `openclaw_cmdarg*` listener. Thanks @Wangmerlyn. +- Feishu/webhook: harden the webhook transport and card-action replay guards to fail closed on missing `encryptKey` and blank callback tokens — refuse to start the webhook transport without an `encryptKey`, reject unsigned requests when no key is present instead of accepting them, and drop blank card-action tokens before the dedupe claim and dispatcher. Defense-in-depth over the already-closed monitor-account layer. (#66707) Thanks @eleqtrizit. ## 2026.4.14 diff --git a/extensions/feishu/src/bot.card-action.test.ts b/extensions/feishu/src/bot.card-action.test.ts index e470b897d0a..783342ff979 100644 --- a/extensions/feishu/src/bot.card-action.test.ts +++ b/extensions/feishu/src/bot.card-action.test.ts @@ -367,6 +367,29 @@ describe("Feishu Card Action Handler", () => { expect(handleFeishuMessage).toHaveBeenCalledTimes(1); }); + it("rejects empty callback tokens before dispatch", async () => { + const log = vi.fn(); + const event = createStructuredQuickActionEvent({ + token: " ", + action: "feishu.quick_actions.help", + command: "/help", + }); + + await handleFeishuCardAction({ + cfg, + event, + runtime: { + ...runtime, + log, + }, + }); + + expect(handleFeishuMessage).not.toHaveBeenCalled(); + expect(log).toHaveBeenCalledWith( + "feishu[mock-account]: rejected card action from u123: missing token", + ); + }); + it("keeps a claimed token completed after a non-retryable dispatch failure", async () => { const event = createStructuredQuickActionEvent({ token: "tok11", diff --git a/extensions/feishu/src/card-action.ts b/extensions/feishu/src/card-action.ts index 2d46bb8c069..dc88d51cadb 100644 --- a/extensions/feishu/src/card-action.ts +++ b/extensions/feishu/src/card-action.ts @@ -63,7 +63,7 @@ function beginFeishuCardActionToken(params: { pruneProcessedCardActionTokens(now); const normalizedToken = params.token.trim(); if (!normalizedToken) { - return true; + return false; } const key = `${params.accountId}:${normalizedToken}`; const existing = processedCardActionTokens.get(key); @@ -183,6 +183,12 @@ export async function handleFeishuCardAction(params: { const { cfg, event, runtime, accountId } = params; const account = resolveFeishuRuntimeAccount({ cfg, accountId }); const log = runtime?.log ?? console.log; + if (!event.token.trim()) { + log( + `feishu[${account.accountId}]: rejected card action from ${event.operator.open_id}: missing token`, + ); + return; + } const decoded = decodeFeishuCardAction({ event }); const claimedToken = beginFeishuCardActionToken({ token: event.token, diff --git a/extensions/feishu/src/monitor.card-action.lifecycle.test.ts b/extensions/feishu/src/monitor.card-action.lifecycle.test.ts index 10682df8841..46438f2ec16 100644 --- a/extensions/feishu/src/monitor.card-action.lifecycle.test.ts +++ b/extensions/feishu/src/monitor.card-action.lifecycle.test.ts @@ -32,7 +32,7 @@ const { } = getFeishuLifecycleTestMocks(); let _handlers: Record Promise> = {}; -let lastRuntime: ReturnType | null = null; +let lastRuntime: { error: ReturnType } | null = null; const originalStateDir = process.env.OPENCLAW_STATE_DIR; const lifecycleConfig = createFeishuLifecycleConfig({ accountId: "acct-card", @@ -219,4 +219,41 @@ describe("Feishu card-action lifecycle", () => { expect(dispatchReplyFromConfigMock).toHaveBeenCalledTimes(1); expectFeishuReplyDispatcherSentFinalReplyOnce({ createFeishuReplyDispatcherMock }); }); + + it("drops malformed card-action events with empty tokens before handler dispatch", async () => { + const onCardAction = await setupLifecycleMonitor(); + + await onCardAction({ + operator: { + open_id: "ou_user1", + user_id: "user_1", + union_id: "union_1", + }, + token: "", + action: { + tag: "button", + value: createFeishuCardInteractionEnvelope({ + k: "quick", + a: "feishu.quick_actions.help", + q: "/help", + c: { + u: "ou_user1", + h: "p2p:ou_user1", + t: "p2p", + e: Date.now() + 60_000, + }, + }), + }, + context: { + open_id: "ou_user1", + user_id: "user_1", + chat_id: "p2p:ou_user1", + }, + }); + + expect(lastRuntime?.error).toHaveBeenCalledWith( + "feishu[acct-card]: ignoring malformed card action payload", + ); + expect(dispatchReplyFromConfigMock).not.toHaveBeenCalled(); + }); }); diff --git a/extensions/feishu/src/monitor.transport.ts b/extensions/feishu/src/monitor.transport.ts index bbd946696a7..0de68b5c9d3 100644 --- a/extensions/feishu/src/monitor.transport.ts +++ b/extensions/feishu/src/monitor.transport.ts @@ -56,7 +56,7 @@ function isFeishuWebhookSignatureValid(params: { }): boolean { const encryptKey = params.encryptKey?.trim(); if (!encryptKey) { - return true; + return false; } const timestampHeader = params.headers["x-lark-request-timestamp"]; @@ -149,6 +149,10 @@ export async function monitorWebhook({ }: MonitorTransportParams): Promise { const log = runtime?.log ?? console.log; const error = runtime?.error ?? console.error; + const encryptKey = account.encryptKey?.trim(); + if (!encryptKey) { + throw new Error(`Feishu account "${accountId}" webhook mode requires encryptKey`); + } const port = account.config.webhookPort ?? 3000; const path = account.config.webhookPath ?? "/feishu/events"; @@ -208,7 +212,7 @@ export async function monitorWebhook({ !isFeishuWebhookSignatureValid({ headers: req.headers, rawBody, - encryptKey: account.encryptKey, + encryptKey, }) ) { respondText(res, 401, "Invalid signature"); @@ -222,7 +226,7 @@ export async function monitorWebhook({ } const { isChallenge, challenge } = Lark.generateChallenge(payload, { - encryptKey: account.encryptKey ?? "", + encryptKey, }); if (isChallenge) { res.statusCode = 200; diff --git a/extensions/feishu/src/monitor.webhook-security.test.ts b/extensions/feishu/src/monitor.webhook-security.test.ts index 60b5517e66c..fbb0a780ef9 100644 --- a/extensions/feishu/src/monitor.webhook-security.test.ts +++ b/extensions/feishu/src/monitor.webhook-security.test.ts @@ -28,6 +28,7 @@ vi.mock("@larksuiteoapi/node-sdk", () => ({ ), })); +import type { RuntimeEnv } from "../runtime-api.js"; import { clearFeishuWebhookRateLimitStateForTest, getFeishuWebhookRateLimitStateSizeForTest, @@ -35,6 +36,8 @@ import { monitorFeishuProvider, stopFeishuMonitor, } from "./monitor.js"; +import { monitorWebhook } from "./monitor.transport.js"; +import type { ResolvedFeishuAccount } from "./types.js"; async function waitForSlowBodyTimeoutResponse( url: string, @@ -110,6 +113,33 @@ describe("Feishu webhook security hardening", () => { await expect(monitorFeishuProvider({ config: cfg })).rejects.toThrow(/requires encryptKey/i); }); + it("refuses to start the webhook transport without encryptKey", async () => { + const account = { + accountId: "transport-missing-encrypt-key", + config: { + enabled: true, + connectionMode: "webhook", + webhookHost: "127.0.0.1", + webhookPort: await getFreePort(), + webhookPath: "/hook-transport-missing-encrypt", + }, + } as ResolvedFeishuAccount; + + await expect( + monitorWebhook({ + account, + accountId: account.accountId, + runtime: { + log: vi.fn(), + error: vi.fn(), + exit: vi.fn(), + } as RuntimeEnv, + abortSignal: new AbortController().signal, + eventDispatcher: {} as never, + }), + ).rejects.toThrow(/requires encryptKey/i); + }); + it("returns 415 for POST requests without json content type", async () => { probeFeishuMock.mockResolvedValue({ ok: true, botOpenId: "bot_open_id" }); await withRunningWebhookMonitor(