fix(security): harden webhook memory guards across channels
This commit is contained in:
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<string, number[]> = 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;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -14,13 +14,25 @@ const rateLimiters = new Map<string, RateLimiter>();
|
||||
|
||||
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<string> {
|
||||
return new Promise((resolve, reject) => {
|
||||
|
||||
Reference in New Issue
Block a user