mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 13:40:44 +00:00
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 <drobison@nvidia.com>
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -32,7 +32,7 @@ const {
|
||||
} = getFeishuLifecycleTestMocks();
|
||||
|
||||
let _handlers: Record<string, (data: unknown) => Promise<void>> = {};
|
||||
let lastRuntime: ReturnType<typeof createRuntimeEnv> | null = null;
|
||||
let lastRuntime: { error: ReturnType<typeof vi.fn> } | 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();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<void> {
|
||||
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;
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user