From 43cad8268d46a3663c79be831f2cacb015114882 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 2 Mar 2026 00:11:49 +0000 Subject: [PATCH] fix(security): harden webhook memory guards across channels --- CHANGELOG.md | 1 + extensions/feishu/src/monitor.ts | 65 +++------ .../nostr/src/nostr-profile-http.test.ts | 23 +++ extensions/nostr/src/nostr-profile-http.ts | 40 +++--- extensions/synology-chat/src/security.test.ts | 9 ++ extensions/synology-chat/src/security.ts | 65 +++------ .../synology-chat/src/webhook-handler.test.ts | 6 +- .../synology-chat/src/webhook-handler.ts | 14 +- extensions/zalo/src/monitor.ts | 9 ++ extensions/zalo/src/monitor.webhook.test.ts | 67 ++++++++- extensions/zalo/src/monitor.webhook.ts | 57 ++++---- src/plugin-sdk/index.ts | 2 + src/plugin-sdk/webhook-memory-guards.test.ts | 95 ++++++++++++ src/plugin-sdk/webhook-memory-guards.ts | 136 ++++++++++++++++++ 14 files changed, 451 insertions(+), 138 deletions(-) create mode 100644 src/plugin-sdk/webhook-memory-guards.test.ts create mode 100644 src/plugin-sdk/webhook-memory-guards.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 923f55427..21db9cd63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,6 +105,7 @@ Docs: https://docs.openclaw.ai - Security/Logging utility hardening: remove `eval`-based command execution from `scripts/clawlog.sh`, switch to argv-safe command construction, and escape predicate literals for user-supplied search/category filters to block local command/predicate injection paths. - Security/ACPX Windows spawn hardening: resolve `.cmd/.bat` wrappers via PATH/PATHEXT and execute unwrapped Node/EXE entrypoints without shell parsing when possible, while preserving compatibility fallback for unknown custom wrappers by default and adding an opt-in strict mode (`strictWindowsCmdWrapper`) to fail closed for unresolvable wrappers. - Security/Inbound metadata stripping: tighten sentinel matching and JSON-fence validation for inbound metadata stripping so user-authored lookalike lines no longer trigger unintended metadata removal. +- Security/Zalo webhook memory hardening: bound webhook security tracking state and normalize security keying to matched webhook paths (excluding attacker query-string churn) to prevent unauthenticated memory growth pressure on reachable webhook endpoints. Thanks @Somet2mes. - Channels/Command parsing parity: align command-body parsing fields with channel command-gating text for Slack, Signal, Microsoft Teams, Mattermost, and BlueBubbles to avoid mention-strip mismatches and inconsistent command detection. - CLI/Startup (Raspberry Pi + small hosts): speed up startup by avoiding unnecessary plugin preload on fast routes, adding root `--version` fast-path bootstrap bypass, parallelizing status JSON/non-JSON scans where safe, and enabling Node compile cache at startup with env override compatibility (`NODE_COMPILE_CACHE`, `NODE_DISABLE_COMPILE_CACHE`). (#5871) Thanks @BookCatKid and @vincentkoc for raising startup reports, and @lupuletic for related startup work in #27973. - Doctor/macOS state-dir safety: warn when OpenClaw state resolves inside iCloud Drive (`~/Library/Mobile Documents/com~apple~CloudDocs/...`) or `~/Library/CloudStorage/...`, because sync-backed paths can cause slower I/O and lock/sync races. (#31004) Thanks @vincentkoc. diff --git a/extensions/feishu/src/monitor.ts b/extensions/feishu/src/monitor.ts index 5425f84b1..5a7e777cd 100644 --- a/extensions/feishu/src/monitor.ts +++ b/extensions/feishu/src/monitor.ts @@ -3,6 +3,8 @@ import * as http from "http"; import * as Lark from "@larksuiteoapi/node-sdk"; import { type ClawdbotConfig, + createBoundedCounter, + createFixedWindowRateLimiter, type RuntimeEnv, type HistoryEntry, installRequestBodyLimitGuard, @@ -32,6 +34,8 @@ const FEISHU_WEBHOOK_RATE_LIMIT_WINDOW_MS = 60_000; const FEISHU_WEBHOOK_RATE_LIMIT_MAX_REQUESTS = 120; const FEISHU_WEBHOOK_RATE_LIMIT_MAX_TRACKED_KEYS = 4_096; const FEISHU_WEBHOOK_COUNTER_LOG_EVERY = 25; +const FEISHU_WEBHOOK_COUNTER_MAX_TRACKED_KEYS = 4_096; +const FEISHU_WEBHOOK_COUNTER_TTL_MS = 6 * 60 * 60_000; const FEISHU_REACTION_VERIFY_TIMEOUT_MS = 1_500; export type FeishuReactionCreatedEvent = { @@ -55,9 +59,15 @@ type ResolveReactionSyntheticEventParams = { uuid?: () => string; }; -const feishuWebhookRateLimits = new Map(); -const feishuWebhookStatusCounters = new Map(); -let lastWebhookRateLimitCleanupMs = 0; +const feishuWebhookRateLimiter = createFixedWindowRateLimiter({ + windowMs: FEISHU_WEBHOOK_RATE_LIMIT_WINDOW_MS, + maxRequests: FEISHU_WEBHOOK_RATE_LIMIT_MAX_REQUESTS, + maxTrackedKeys: FEISHU_WEBHOOK_RATE_LIMIT_MAX_TRACKED_KEYS, +}); +const feishuWebhookStatusCounters = createBoundedCounter({ + maxTrackedKeys: FEISHU_WEBHOOK_COUNTER_MAX_TRACKED_KEYS, + ttlMs: FEISHU_WEBHOOK_COUNTER_TTL_MS, +}); function isJsonContentType(value: string | string[] | undefined): boolean { const first = Array.isArray(value) ? value[0] : value; @@ -68,55 +78,17 @@ function isJsonContentType(value: string | string[] | undefined): boolean { return mediaType === "application/json" || Boolean(mediaType?.endsWith("+json")); } -function trimWebhookRateLimitState(): void { - while (feishuWebhookRateLimits.size > FEISHU_WEBHOOK_RATE_LIMIT_MAX_TRACKED_KEYS) { - const oldestKey = feishuWebhookRateLimits.keys().next().value; - if (typeof oldestKey !== "string") { - break; - } - feishuWebhookRateLimits.delete(oldestKey); - } -} - -function maybePruneWebhookRateLimitState(nowMs: number): void { - if ( - feishuWebhookRateLimits.size === 0 || - nowMs - lastWebhookRateLimitCleanupMs < FEISHU_WEBHOOK_RATE_LIMIT_WINDOW_MS - ) { - return; - } - lastWebhookRateLimitCleanupMs = nowMs; - for (const [key, state] of feishuWebhookRateLimits) { - if (nowMs - state.windowStartMs >= FEISHU_WEBHOOK_RATE_LIMIT_WINDOW_MS) { - feishuWebhookRateLimits.delete(key); - } - } -} - export function clearFeishuWebhookRateLimitStateForTest(): void { - feishuWebhookRateLimits.clear(); - lastWebhookRateLimitCleanupMs = 0; + feishuWebhookRateLimiter.clear(); + feishuWebhookStatusCounters.clear(); } export function getFeishuWebhookRateLimitStateSizeForTest(): number { - return feishuWebhookRateLimits.size; + return feishuWebhookRateLimiter.size(); } export function isWebhookRateLimitedForTest(key: string, nowMs: number): boolean { - maybePruneWebhookRateLimitState(nowMs); - - const state = feishuWebhookRateLimits.get(key); - if (!state || nowMs - state.windowStartMs >= FEISHU_WEBHOOK_RATE_LIMIT_WINDOW_MS) { - feishuWebhookRateLimits.set(key, { count: 1, windowStartMs: nowMs }); - trimWebhookRateLimitState(); - return false; - } - - state.count += 1; - if (state.count > FEISHU_WEBHOOK_RATE_LIMIT_MAX_REQUESTS) { - return true; - } - return false; + return feishuWebhookRateLimiter.isRateLimited(key, nowMs); } function isWebhookRateLimited(key: string, nowMs: number): boolean { @@ -133,8 +105,7 @@ function recordWebhookStatus( return; } const key = `${accountId}:${path}:${statusCode}`; - const next = (feishuWebhookStatusCounters.get(key) ?? 0) + 1; - feishuWebhookStatusCounters.set(key, next); + const next = feishuWebhookStatusCounters.increment(key); if (next === 1 || next % FEISHU_WEBHOOK_COUNTER_LOG_EVERY === 0) { const log = runtime?.log ?? console.log; log(`feishu[${accountId}]: webhook anomaly path=${path} status=${statusCode} count=${next}`); diff --git a/extensions/nostr/src/nostr-profile-http.test.ts b/extensions/nostr/src/nostr-profile-http.test.ts index 5e2d3c838..7d5968a96 100644 --- a/extensions/nostr/src/nostr-profile-http.test.ts +++ b/extensions/nostr/src/nostr-profile-http.test.ts @@ -6,7 +6,10 @@ import { IncomingMessage, ServerResponse } from "node:http"; import { Socket } from "node:net"; import { describe, it, expect, vi, beforeEach } from "vitest"; import { + clearNostrProfileRateLimitStateForTest, createNostrProfileHttpHandler, + getNostrProfileRateLimitStateSizeForTest, + isNostrProfileRateLimitedForTest, type NostrProfileHttpContext, } from "./nostr-profile-http.js"; @@ -136,6 +139,7 @@ function mockSuccessfulProfileImport() { describe("nostr-profile-http", () => { beforeEach(() => { vi.clearAllMocks(); + clearNostrProfileRateLimitStateForTest(); }); describe("route matching", () => { @@ -358,6 +362,25 @@ describe("nostr-profile-http", () => { } } }); + + it("caps tracked rate-limit keys to prevent unbounded growth", () => { + const now = 1_000_000; + for (let i = 0; i < 2_500; i += 1) { + isNostrProfileRateLimitedForTest(`rate-cap-${i}`, now); + } + expect(getNostrProfileRateLimitStateSizeForTest()).toBeLessThanOrEqual(2_048); + }); + + it("prunes stale rate-limit keys after the window elapses", () => { + const now = 2_000_000; + for (let i = 0; i < 100; i += 1) { + isNostrProfileRateLimitedForTest(`rate-stale-${i}`, now); + } + expect(getNostrProfileRateLimitStateSizeForTest()).toBe(100); + + isNostrProfileRateLimitedForTest("fresh", now + 60_001); + expect(getNostrProfileRateLimitStateSizeForTest()).toBe(1); + }); }); describe("POST /api/channels/nostr/:accountId/profile/import", () => { diff --git a/extensions/nostr/src/nostr-profile-http.ts b/extensions/nostr/src/nostr-profile-http.ts index 082b67b44..d42d8e52e 100644 --- a/extensions/nostr/src/nostr-profile-http.ts +++ b/extensions/nostr/src/nostr-profile-http.ts @@ -9,6 +9,7 @@ import type { IncomingMessage, ServerResponse } from "node:http"; import { + createFixedWindowRateLimiter, isBlockedHostnameOrIp, readJsonBodyWithLimit, requestBodyErrorToText, @@ -41,30 +42,29 @@ export interface NostrProfileHttpContext { // Rate Limiting // ============================================================================ -interface RateLimitEntry { - count: number; - windowStart: number; -} - -const rateLimitMap = new Map(); const RATE_LIMIT_WINDOW_MS = 60_000; // 1 minute const RATE_LIMIT_MAX_REQUESTS = 5; // 5 requests per minute +const RATE_LIMIT_MAX_TRACKED_KEYS = 2_048; +const profileRateLimiter = createFixedWindowRateLimiter({ + windowMs: RATE_LIMIT_WINDOW_MS, + maxRequests: RATE_LIMIT_MAX_REQUESTS, + maxTrackedKeys: RATE_LIMIT_MAX_TRACKED_KEYS, +}); + +export function clearNostrProfileRateLimitStateForTest(): void { + profileRateLimiter.clear(); +} + +export function getNostrProfileRateLimitStateSizeForTest(): number { + return profileRateLimiter.size(); +} + +export function isNostrProfileRateLimitedForTest(accountId: string, nowMs: number): boolean { + return profileRateLimiter.isRateLimited(accountId, nowMs); +} function checkRateLimit(accountId: string): boolean { - const now = Date.now(); - const entry = rateLimitMap.get(accountId); - - if (!entry || now - entry.windowStart > RATE_LIMIT_WINDOW_MS) { - rateLimitMap.set(accountId, { count: 1, windowStart: now }); - return true; - } - - if (entry.count >= RATE_LIMIT_MAX_REQUESTS) { - return false; - } - - entry.count++; - return true; + return !profileRateLimiter.isRateLimited(accountId); } // ============================================================================ diff --git a/extensions/synology-chat/src/security.test.ts b/extensions/synology-chat/src/security.test.ts index f77fd21ca..a3e445e79 100644 --- a/extensions/synology-chat/src/security.test.ts +++ b/extensions/synology-chat/src/security.test.ts @@ -134,4 +134,13 @@ describe("RateLimiter", () => { // user2 should still be allowed expect(limiter.check("user2")).toBe(true); }); + + it("caps tracked users to prevent unbounded growth", () => { + const limiter = new RateLimiter(1, 60, 3); + expect(limiter.check("user1")).toBe(true); + expect(limiter.check("user2")).toBe(true); + expect(limiter.check("user3")).toBe(true); + expect(limiter.check("user4")).toBe(true); + expect(limiter.size()).toBeLessThanOrEqual(3); + }); }); diff --git a/extensions/synology-chat/src/security.ts b/extensions/synology-chat/src/security.ts index 22883babb..7c4f646b6 100644 --- a/extensions/synology-chat/src/security.ts +++ b/extensions/synology-chat/src/security.ts @@ -3,6 +3,7 @@ */ import * as crypto from "node:crypto"; +import { createFixedWindowRateLimiter, type FixedWindowRateLimiter } from "openclaw/plugin-sdk"; export type DmAuthorizationResult = | { allowed: true } @@ -86,55 +87,35 @@ export function sanitizeInput(text: string): string { * Sliding window rate limiter per user ID. */ export class RateLimiter { - private requests: Map = new Map(); - private limit: number; - private windowMs: number; - private lastCleanup = 0; - private cleanupIntervalMs: number; + private readonly limiter: FixedWindowRateLimiter; + private readonly limit: number; - constructor(limit = 30, windowSeconds = 60) { + constructor(limit = 30, windowSeconds = 60, maxTrackedUsers = 5_000) { this.limit = limit; - this.windowMs = windowSeconds * 1000; - this.cleanupIntervalMs = this.windowMs * 5; // cleanup every 5 windows + this.limiter = createFixedWindowRateLimiter({ + windowMs: Math.max(1, Math.floor(windowSeconds * 1000)), + maxRequests: Math.max(1, Math.floor(limit)), + maxTrackedKeys: Math.max(1, Math.floor(maxTrackedUsers)), + }); } /** Returns true if the request is allowed, false if rate-limited. */ check(userId: string): boolean { - const now = Date.now(); - const windowStart = now - this.windowMs; - - // Periodic cleanup of stale entries to prevent memory leak - if (now - this.lastCleanup > this.cleanupIntervalMs) { - this.cleanup(windowStart); - this.lastCleanup = now; - } - - let timestamps = this.requests.get(userId); - if (timestamps) { - timestamps = timestamps.filter((ts) => ts > windowStart); - } else { - timestamps = []; - } - - if (timestamps.length >= this.limit) { - this.requests.set(userId, timestamps); - return false; - } - - timestamps.push(now); - this.requests.set(userId, timestamps); - return true; + return !this.limiter.isRateLimited(userId); } - /** Remove entries with no recent activity. */ - private cleanup(windowStart: number): void { - for (const [userId, timestamps] of this.requests) { - const active = timestamps.filter((ts) => ts > windowStart); - if (active.length === 0) { - this.requests.delete(userId); - } else { - this.requests.set(userId, active); - } - } + /** Exposed for tests and diagnostics. */ + size(): number { + return this.limiter.size(); + } + + /** Exposed for tests and account lifecycle cleanup. */ + clear(): void { + this.limiter.clear(); + } + + /** Exposed for tests. */ + maxRequests(): number { + return this.limit; } } diff --git a/extensions/synology-chat/src/webhook-handler.test.ts b/extensions/synology-chat/src/webhook-handler.test.ts index 1c8ef393c..b79b313c8 100644 --- a/extensions/synology-chat/src/webhook-handler.test.ts +++ b/extensions/synology-chat/src/webhook-handler.test.ts @@ -2,7 +2,10 @@ import { EventEmitter } from "node:events"; import type { IncomingMessage, ServerResponse } from "node:http"; import { describe, it, expect, vi, beforeEach } from "vitest"; import type { ResolvedSynologyChatAccount } from "./types.js"; -import { createWebhookHandler } from "./webhook-handler.js"; +import { + clearSynologyWebhookRateLimiterStateForTest, + createWebhookHandler, +} from "./webhook-handler.js"; // Mock sendMessage to prevent real HTTP calls vi.mock("./client.js", () => ({ @@ -73,6 +76,7 @@ describe("createWebhookHandler", () => { let log: { info: any; warn: any; error: any }; beforeEach(() => { + clearSynologyWebhookRateLimiterStateForTest(); log = { info: vi.fn(), warn: vi.fn(), diff --git a/extensions/synology-chat/src/webhook-handler.ts b/extensions/synology-chat/src/webhook-handler.ts index b077e61fc..08666a352 100644 --- a/extensions/synology-chat/src/webhook-handler.ts +++ b/extensions/synology-chat/src/webhook-handler.ts @@ -14,13 +14,25 @@ const rateLimiters = new Map(); function getRateLimiter(account: ResolvedSynologyChatAccount): RateLimiter { let rl = rateLimiters.get(account.accountId); - if (!rl) { + if (!rl || rl.maxRequests() !== account.rateLimitPerMinute) { + rl?.clear(); rl = new RateLimiter(account.rateLimitPerMinute); rateLimiters.set(account.accountId, rl); } return rl; } +export function clearSynologyWebhookRateLimiterStateForTest(): void { + for (const limiter of rateLimiters.values()) { + limiter.clear(); + } + rateLimiters.clear(); +} + +export function getSynologyWebhookRateLimiterCountForTest(): number { + return rateLimiters.size; +} + /** Read the full request body as a string. */ function readBody(req: IncomingMessage): Promise { return new Promise((resolve, reject) => { diff --git a/extensions/zalo/src/monitor.ts b/extensions/zalo/src/monitor.ts index e1887c488..8cf9f7efb 100644 --- a/extensions/zalo/src/monitor.ts +++ b/extensions/zalo/src/monitor.ts @@ -28,6 +28,9 @@ import { resolveZaloRuntimeGroupPolicy, } from "./group-access.js"; import { + clearZaloWebhookSecurityStateForTest, + getZaloWebhookRateLimitStateSizeForTest, + getZaloWebhookStatusCounterSizeForTest, handleZaloWebhookRequest as handleZaloWebhookRequestInternal, registerZaloWebhookTarget as registerZaloWebhookTargetInternal, type ZaloWebhookTarget, @@ -73,6 +76,12 @@ export function registerZaloWebhookTarget(target: ZaloWebhookTarget): () => void return registerZaloWebhookTargetInternal(target); } +export { + clearZaloWebhookSecurityStateForTest, + getZaloWebhookRateLimitStateSizeForTest, + getZaloWebhookStatusCounterSizeForTest, +}; + export async function handleZaloWebhookRequest( req: IncomingMessage, res: ServerResponse, diff --git a/extensions/zalo/src/monitor.webhook.test.ts b/extensions/zalo/src/monitor.webhook.test.ts index af998bee6..9dd63d988 100644 --- a/extensions/zalo/src/monitor.webhook.test.ts +++ b/extensions/zalo/src/monitor.webhook.test.ts @@ -1,8 +1,14 @@ import { createServer, type RequestListener } from "node:http"; import type { AddressInfo } from "node:net"; import type { OpenClawConfig, PluginRuntime } from "openclaw/plugin-sdk"; -import { describe, expect, it, vi } from "vitest"; -import { handleZaloWebhookRequest, registerZaloWebhookTarget } from "./monitor.js"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + clearZaloWebhookSecurityStateForTest, + getZaloWebhookRateLimitStateSizeForTest, + getZaloWebhookStatusCounterSizeForTest, + handleZaloWebhookRequest, + registerZaloWebhookTarget, +} from "./monitor.js"; import type { ResolvedZaloAccount } from "./types.js"; async function withServer(handler: RequestListener, fn: (baseUrl: string) => Promise) { @@ -56,6 +62,10 @@ function registerTarget(params: { } describe("handleZaloWebhookRequest", () => { + afterEach(() => { + clearZaloWebhookSecurityStateForTest(); + }); + it("returns 400 for non-object payloads", async () => { const unregister = registerTarget({ path: "/hook" }); @@ -196,4 +206,57 @@ describe("handleZaloWebhookRequest", () => { unregister(); } }); + + it("does not grow status counters when query strings churn on unauthorized requests", async () => { + const unregister = registerTarget({ path: "/hook-query-status" }); + + try { + await withServer(webhookRequestHandler, async (baseUrl) => { + for (let i = 0; i < 200; i += 1) { + const response = await fetch(`${baseUrl}/hook-query-status?nonce=${i}`, { + method: "POST", + headers: { + "x-bot-api-secret-token": "invalid-token", + "content-type": "application/json", + }, + body: "{}", + }); + expect(response.status).toBe(401); + } + + expect(getZaloWebhookStatusCounterSizeForTest()).toBe(1); + }); + } finally { + unregister(); + } + }); + + it("rate limits authenticated requests even when query strings churn", async () => { + const unregister = registerTarget({ path: "/hook-query-rate" }); + + try { + await withServer(webhookRequestHandler, async (baseUrl) => { + let saw429 = false; + for (let i = 0; i < 130; i += 1) { + const response = await fetch(`${baseUrl}/hook-query-rate?nonce=${i}`, { + method: "POST", + headers: { + "x-bot-api-secret-token": "secret", + "content-type": "application/json", + }, + body: "{}", + }); + if (response.status === 429) { + saw429 = true; + break; + } + } + + expect(saw429).toBe(true); + expect(getZaloWebhookRateLimitStateSizeForTest()).toBe(1); + }); + } finally { + unregister(); + } + }); }); diff --git a/extensions/zalo/src/monitor.webhook.ts b/extensions/zalo/src/monitor.webhook.ts index dd2b0c655..9957d116a 100644 --- a/extensions/zalo/src/monitor.webhook.ts +++ b/extensions/zalo/src/monitor.webhook.ts @@ -2,7 +2,9 @@ import { timingSafeEqual } from "node:crypto"; import type { IncomingMessage, ServerResponse } from "node:http"; import type { OpenClawConfig } from "openclaw/plugin-sdk"; import { + createBoundedCounter, createDedupeCache, + createFixedWindowRateLimiter, readJsonBodyWithLimit, registerWebhookTarget, rejectNonPostWebhookRequest, @@ -14,12 +16,13 @@ import type { ResolvedZaloAccount } from "./accounts.js"; import type { ZaloFetch, ZaloUpdate } from "./api.js"; import type { ZaloRuntimeEnv } from "./monitor.js"; -type WebhookRateLimitState = { count: number; windowStartMs: number }; - const ZALO_WEBHOOK_RATE_LIMIT_WINDOW_MS = 60_000; const ZALO_WEBHOOK_RATE_LIMIT_MAX_REQUESTS = 120; +const ZALO_WEBHOOK_RATE_LIMIT_MAX_TRACKED_KEYS = 4_096; const ZALO_WEBHOOK_REPLAY_WINDOW_MS = 5 * 60_000; const ZALO_WEBHOOK_COUNTER_LOG_EVERY = 25; +const ZALO_WEBHOOK_COUNTER_MAX_TRACKED_KEYS = 4_096; +const ZALO_WEBHOOK_COUNTER_TTL_MS = 6 * 60 * 60_000; export type ZaloWebhookTarget = { token: string; @@ -40,12 +43,32 @@ export type ZaloWebhookProcessUpdate = (params: { }) => Promise; const webhookTargets = new Map(); -const webhookRateLimits = new Map(); +const webhookRateLimiter = createFixedWindowRateLimiter({ + windowMs: ZALO_WEBHOOK_RATE_LIMIT_WINDOW_MS, + maxRequests: ZALO_WEBHOOK_RATE_LIMIT_MAX_REQUESTS, + maxTrackedKeys: ZALO_WEBHOOK_RATE_LIMIT_MAX_TRACKED_KEYS, +}); const recentWebhookEvents = createDedupeCache({ ttlMs: ZALO_WEBHOOK_REPLAY_WINDOW_MS, maxSize: 5000, }); -const webhookStatusCounters = new Map(); +const webhookStatusCounters = createBoundedCounter({ + maxTrackedKeys: ZALO_WEBHOOK_COUNTER_MAX_TRACKED_KEYS, + ttlMs: ZALO_WEBHOOK_COUNTER_TTL_MS, +}); + +export function clearZaloWebhookSecurityStateForTest(): void { + webhookRateLimiter.clear(); + webhookStatusCounters.clear(); +} + +export function getZaloWebhookRateLimitStateSizeForTest(): number { + return webhookRateLimiter.size(); +} + +export function getZaloWebhookStatusCounterSizeForTest(): number { + return webhookStatusCounters.size(); +} function isJsonContentType(value: string | string[] | undefined): boolean { const first = Array.isArray(value) ? value[0] : value; @@ -73,20 +96,6 @@ function timingSafeEquals(left: string, right: string): boolean { return timingSafeEqual(leftBuffer, rightBuffer); } -function isWebhookRateLimited(key: string, nowMs: number): boolean { - const state = webhookRateLimits.get(key); - if (!state || nowMs - state.windowStartMs >= ZALO_WEBHOOK_RATE_LIMIT_WINDOW_MS) { - webhookRateLimits.set(key, { count: 1, windowStartMs: nowMs }); - return false; - } - - state.count += 1; - if (state.count > ZALO_WEBHOOK_RATE_LIMIT_MAX_REQUESTS) { - return true; - } - return false; -} - function isReplayEvent(update: ZaloUpdate, nowMs: number): boolean { const messageId = update.message?.message_id; if (!messageId) { @@ -105,8 +114,7 @@ function recordWebhookStatus( return; } const key = `${path}:${statusCode}`; - const next = (webhookStatusCounters.get(key) ?? 0) + 1; - webhookStatusCounters.set(key, next); + const next = webhookStatusCounters.increment(key); if (next === 1 || next % ZALO_WEBHOOK_COUNTER_LOG_EVERY === 0) { runtime?.log?.( `[zalo] webhook anomaly path=${path} status=${statusCode} count=${String(next)}`, @@ -127,7 +135,7 @@ export async function handleZaloWebhookRequest( if (!resolved) { return false; } - const { targets } = resolved; + const { targets, path } = resolved; if (rejectNonPostWebhookRequest(req, res)) { return true; @@ -140,21 +148,20 @@ export async function handleZaloWebhookRequest( if (matchedTarget.kind === "none") { res.statusCode = 401; res.end("unauthorized"); - recordWebhookStatus(targets[0]?.runtime, req.url ?? "", res.statusCode); + recordWebhookStatus(targets[0]?.runtime, path, res.statusCode); return true; } if (matchedTarget.kind === "ambiguous") { res.statusCode = 401; res.end("ambiguous webhook target"); - recordWebhookStatus(targets[0]?.runtime, req.url ?? "", res.statusCode); + recordWebhookStatus(targets[0]?.runtime, path, res.statusCode); return true; } const target = matchedTarget.target; - const path = req.url ?? ""; const rateLimitKey = `${path}:${req.socket.remoteAddress ?? "unknown"}`; const nowMs = Date.now(); - if (isWebhookRateLimited(rateLimitKey, nowMs)) { + if (webhookRateLimiter.isRateLimited(rateLimitKey, nowMs)) { res.statusCode = 429; res.end("Too Many Requests"); recordWebhookStatus(target.runtime, path, res.statusCode); diff --git a/src/plugin-sdk/index.ts b/src/plugin-sdk/index.ts index b36632845..cf6745fa6 100644 --- a/src/plugin-sdk/index.ts +++ b/src/plugin-sdk/index.ts @@ -297,6 +297,8 @@ export { readRequestBodyWithLimit, requestBodyErrorToText, } from "../infra/http-body.js"; +export { createBoundedCounter, createFixedWindowRateLimiter } from "./webhook-memory-guards.js"; +export type { BoundedCounter, FixedWindowRateLimiter } from "./webhook-memory-guards.js"; export { fetchWithSsrFGuard } from "../infra/net/fetch-guard.js"; export { diff --git a/src/plugin-sdk/webhook-memory-guards.test.ts b/src/plugin-sdk/webhook-memory-guards.test.ts new file mode 100644 index 000000000..296cc6fc3 --- /dev/null +++ b/src/plugin-sdk/webhook-memory-guards.test.ts @@ -0,0 +1,95 @@ +import { describe, expect, it } from "vitest"; +import { createBoundedCounter, createFixedWindowRateLimiter } from "./webhook-memory-guards.js"; + +describe("createFixedWindowRateLimiter", () => { + it("enforces a fixed-window request limit", () => { + const limiter = createFixedWindowRateLimiter({ + windowMs: 60_000, + maxRequests: 3, + maxTrackedKeys: 100, + }); + + expect(limiter.isRateLimited("k", 1_000)).toBe(false); + expect(limiter.isRateLimited("k", 1_001)).toBe(false); + expect(limiter.isRateLimited("k", 1_002)).toBe(false); + expect(limiter.isRateLimited("k", 1_003)).toBe(true); + }); + + it("resets counters after the window elapses", () => { + const limiter = createFixedWindowRateLimiter({ + windowMs: 10, + maxRequests: 1, + maxTrackedKeys: 100, + }); + + expect(limiter.isRateLimited("k", 100)).toBe(false); + expect(limiter.isRateLimited("k", 101)).toBe(true); + expect(limiter.isRateLimited("k", 111)).toBe(false); + }); + + it("caps tracked keys", () => { + const limiter = createFixedWindowRateLimiter({ + windowMs: 60_000, + maxRequests: 10, + maxTrackedKeys: 5, + }); + + for (let i = 0; i < 20; i += 1) { + limiter.isRateLimited(`key-${i}`, 1_000 + i); + } + + expect(limiter.size()).toBeLessThanOrEqual(5); + }); + + it("prunes stale keys", () => { + const limiter = createFixedWindowRateLimiter({ + windowMs: 10, + maxRequests: 10, + maxTrackedKeys: 100, + pruneIntervalMs: 10, + }); + + for (let i = 0; i < 20; i += 1) { + limiter.isRateLimited(`key-${i}`, 100); + } + expect(limiter.size()).toBe(20); + + limiter.isRateLimited("fresh", 120); + expect(limiter.size()).toBe(1); + }); +}); + +describe("createBoundedCounter", () => { + it("increments and returns per-key counts", () => { + const counter = createBoundedCounter({ maxTrackedKeys: 100 }); + + expect(counter.increment("k", 1_000)).toBe(1); + expect(counter.increment("k", 1_001)).toBe(2); + expect(counter.increment("k", 1_002)).toBe(3); + }); + + it("caps tracked keys", () => { + const counter = createBoundedCounter({ maxTrackedKeys: 3 }); + + for (let i = 0; i < 10; i += 1) { + counter.increment(`k-${i}`, 1_000 + i); + } + + expect(counter.size()).toBeLessThanOrEqual(3); + }); + + it("expires stale keys when ttl is set", () => { + const counter = createBoundedCounter({ + maxTrackedKeys: 100, + ttlMs: 10, + pruneIntervalMs: 10, + }); + + counter.increment("old-1", 100); + counter.increment("old-2", 100); + expect(counter.size()).toBe(2); + + counter.increment("fresh", 120); + expect(counter.size()).toBe(1); + }); +}); diff --git a/src/plugin-sdk/webhook-memory-guards.ts b/src/plugin-sdk/webhook-memory-guards.ts new file mode 100644 index 000000000..5f9768951 --- /dev/null +++ b/src/plugin-sdk/webhook-memory-guards.ts @@ -0,0 +1,136 @@ +import { pruneMapToMaxSize } from "../infra/map-size.js"; + +type FixedWindowState = { + count: number; + windowStartMs: number; +}; + +type CounterState = { + count: number; + updatedAtMs: number; +}; + +export type FixedWindowRateLimiter = { + isRateLimited: (key: string, nowMs?: number) => boolean; + size: () => number; + clear: () => void; +}; + +export type BoundedCounter = { + increment: (key: string, nowMs?: number) => number; + size: () => number; + clear: () => void; +}; + +export function createFixedWindowRateLimiter(options: { + windowMs: number; + maxRequests: number; + maxTrackedKeys: number; + pruneIntervalMs?: number; +}): FixedWindowRateLimiter { + const windowMs = Math.max(1, Math.floor(options.windowMs)); + const maxRequests = Math.max(1, Math.floor(options.maxRequests)); + const maxTrackedKeys = Math.max(1, Math.floor(options.maxTrackedKeys)); + const pruneIntervalMs = Math.max(1, Math.floor(options.pruneIntervalMs ?? windowMs)); + const state = new Map(); + let lastPruneMs = 0; + + const touch = (key: string, value: FixedWindowState) => { + state.delete(key); + state.set(key, value); + }; + + const prune = (nowMs: number) => { + for (const [key, entry] of state) { + if (nowMs - entry.windowStartMs >= windowMs) { + state.delete(key); + } + } + }; + + return { + isRateLimited: (key: string, nowMs = Date.now()) => { + if (!key) { + return false; + } + if (nowMs - lastPruneMs >= pruneIntervalMs) { + prune(nowMs); + lastPruneMs = nowMs; + } + + const existing = state.get(key); + if (!existing || nowMs - existing.windowStartMs >= windowMs) { + touch(key, { count: 1, windowStartMs: nowMs }); + pruneMapToMaxSize(state, maxTrackedKeys); + return false; + } + + const nextCount = existing.count + 1; + touch(key, { count: nextCount, windowStartMs: existing.windowStartMs }); + pruneMapToMaxSize(state, maxTrackedKeys); + return nextCount > maxRequests; + }, + size: () => state.size, + clear: () => { + state.clear(); + lastPruneMs = 0; + }, + }; +} + +export function createBoundedCounter(options: { + maxTrackedKeys: number; + ttlMs?: number; + pruneIntervalMs?: number; +}): BoundedCounter { + const maxTrackedKeys = Math.max(1, Math.floor(options.maxTrackedKeys)); + const ttlMs = Math.max(0, Math.floor(options.ttlMs ?? 0)); + const pruneIntervalMs = Math.max( + 1, + Math.floor(options.pruneIntervalMs ?? (ttlMs > 0 ? ttlMs : 60_000)), + ); + const counters = new Map(); + let lastPruneMs = 0; + + const touch = (key: string, value: CounterState) => { + counters.delete(key); + counters.set(key, value); + }; + + const isExpired = (entry: CounterState, nowMs: number) => + ttlMs > 0 && nowMs - entry.updatedAtMs >= ttlMs; + + const prune = (nowMs: number) => { + if (ttlMs > 0) { + for (const [key, entry] of counters) { + if (isExpired(entry, nowMs)) { + counters.delete(key); + } + } + } + }; + + return { + increment: (key: string, nowMs = Date.now()) => { + if (!key) { + return 0; + } + if (nowMs - lastPruneMs >= pruneIntervalMs) { + prune(nowMs); + lastPruneMs = nowMs; + } + + const existing = counters.get(key); + const baseCount = existing && !isExpired(existing, nowMs) ? existing.count : 0; + const nextCount = baseCount + 1; + touch(key, { count: nextCount, updatedAtMs: nowMs }); + pruneMapToMaxSize(counters, maxTrackedKeys); + return nextCount; + }, + size: () => counters.size, + clear: () => { + counters.clear(); + lastPruneMs = 0; + }, + }; +}