From 9f97555b5e8a81ac7ed5c5b814556e2c2bae694f Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 22 Feb 2026 08:56:24 +0100 Subject: [PATCH] refactor(security): unify hook rate-limit and hook module loading --- src/gateway/auth-rate-limit.ts | 12 ++++-- src/gateway/hooks-mapping.ts | 15 ++++--- src/gateway/server-http.ts | 75 +++++++++------------------------ src/hooks/loader.ts | 38 +++++++++-------- src/hooks/module-loader.test.ts | 48 +++++++++++++++++++++ src/hooks/module-loader.ts | 46 ++++++++++++++++++++ 6 files changed, 152 insertions(+), 82 deletions(-) create mode 100644 src/hooks/module-loader.test.ts create mode 100644 src/hooks/module-loader.ts diff --git a/src/gateway/auth-rate-limit.ts b/src/gateway/auth-rate-limit.ts index 1516ce3dce8..166c215a5bb 100644 --- a/src/gateway/auth-rate-limit.ts +++ b/src/gateway/auth-rate-limit.ts @@ -31,11 +31,14 @@ export interface RateLimitConfig { lockoutMs?: number; /** Exempt loopback (localhost) addresses from rate limiting. @default true */ exemptLoopback?: boolean; + /** Background prune interval in milliseconds; set <= 0 to disable auto-prune. @default 60_000 */ + pruneIntervalMs?: number; } export const AUTH_RATE_LIMIT_SCOPE_DEFAULT = "default"; export const AUTH_RATE_LIMIT_SCOPE_SHARED_SECRET = "shared-secret"; export const AUTH_RATE_LIMIT_SCOPE_DEVICE_TOKEN = "device-token"; +export const AUTH_RATE_LIMIT_SCOPE_HOOK_AUTH = "hook-auth"; export interface RateLimitEntry { /** Timestamps (epoch ms) of recent failed attempts inside the window. */ @@ -94,13 +97,14 @@ export function createAuthRateLimiter(config?: RateLimitConfig): AuthRateLimiter const windowMs = config?.windowMs ?? DEFAULT_WINDOW_MS; const lockoutMs = config?.lockoutMs ?? DEFAULT_LOCKOUT_MS; const exemptLoopback = config?.exemptLoopback ?? true; + const pruneIntervalMs = config?.pruneIntervalMs ?? PRUNE_INTERVAL_MS; const entries = new Map(); // Periodic cleanup to avoid unbounded map growth. - const pruneTimer = setInterval(() => prune(), PRUNE_INTERVAL_MS); + const pruneTimer = pruneIntervalMs > 0 ? setInterval(() => prune(), pruneIntervalMs) : null; // Allow the Node.js process to exit even if the timer is still active. - if (pruneTimer.unref) { + if (pruneTimer?.unref) { pruneTimer.unref(); } @@ -218,7 +222,9 @@ export function createAuthRateLimiter(config?: RateLimitConfig): AuthRateLimiter } function dispose(): void { - clearInterval(pruneTimer); + if (pruneTimer) { + clearInterval(pruneTimer); + } entries.clear(); } diff --git a/src/gateway/hooks-mapping.ts b/src/gateway/hooks-mapping.ts index f9ede350456..20c3a76ccca 100644 --- a/src/gateway/hooks-mapping.ts +++ b/src/gateway/hooks-mapping.ts @@ -1,6 +1,6 @@ import path from "node:path"; -import { pathToFileURL } from "node:url"; import { CONFIG_PATH, type HookMappingConfig, type HooksConfig } from "../config/config.js"; +import { importFileModule, resolveFunctionModuleExport } from "../hooks/module-loader.js"; import type { HookMessageChannel } from "./hooks.js"; export type HookMappingResolved = { @@ -330,19 +330,22 @@ async function loadTransform(transform: HookMappingTransformResolved): Promise; + const mod = await importFileModule({ modulePath: transform.modulePath }); const fn = resolveTransformFn(mod, transform.exportName); transformCache.set(cacheKey, fn); return fn; } function resolveTransformFn(mod: Record, exportName?: string): HookTransformFn { - const candidate = exportName ? mod[exportName] : (mod.default ?? mod.transform); - if (typeof candidate !== "function") { + const candidate = resolveFunctionModuleExport({ + mod, + exportName, + fallbackExportNames: ["default", "transform"], + }); + if (!candidate) { throw new Error("hook transform module must export a function"); } - return candidate as HookTransformFn; + return candidate; } function resolvePath(baseDir: string, target: string): string { diff --git a/src/gateway/server-http.ts b/src/gateway/server-http.ts index d178fc31892..0bde2ea10b9 100644 --- a/src/gateway/server-http.ts +++ b/src/gateway/server-http.ts @@ -19,7 +19,12 @@ import { loadConfig } from "../config/config.js"; import type { createSubsystemLogger } from "../logging/subsystem.js"; import { safeEqualSecret } from "../security/secret-equal.js"; import { handleSlackHttpRequest } from "../slack/http/index.js"; -import { normalizeRateLimitClientIp, type AuthRateLimiter } from "./auth-rate-limit.js"; +import { + AUTH_RATE_LIMIT_SCOPE_HOOK_AUTH, + createAuthRateLimiter, + normalizeRateLimitClientIp, + type AuthRateLimiter, +} from "./auth-rate-limit.js"; import { authorizeHttpGatewayConnect, isLocalDirectRequest, @@ -58,11 +63,9 @@ import type { GatewayWsClient } from "./server/ws-types.js"; import { handleToolsInvokeHttpRequest } from "./tools-invoke-http.js"; type SubsystemLogger = ReturnType; -type HookAuthFailure = { count: number; windowStartedAtMs: number }; const HOOK_AUTH_FAILURE_LIMIT = 20; const HOOK_AUTH_FAILURE_WINDOW_MS = 60_000; -const HOOK_AUTH_FAILURE_TRACK_MAX = 2048; type HookDispatchers = { dispatchWakeHook: (value: { text: string; mode: "now" | "next-heartbeat" }) => void; @@ -219,60 +222,19 @@ export function createHooksRequestHandler( } & HookDispatchers, ): HooksRequestHandler { const { getHooksConfig, bindHost, port, logHooks, dispatchAgentHook, dispatchWakeHook } = opts; - const hookAuthFailures = new Map(); + const hookAuthLimiter = createAuthRateLimiter({ + maxAttempts: HOOK_AUTH_FAILURE_LIMIT, + windowMs: HOOK_AUTH_FAILURE_WINDOW_MS, + lockoutMs: HOOK_AUTH_FAILURE_WINDOW_MS, + exemptLoopback: false, + // Handler lifetimes are tied to gateway runtime/tests; skip background timer fanout. + pruneIntervalMs: 0, + }); const resolveHookClientKey = (req: IncomingMessage): string => { return normalizeRateLimitClientIp(req.socket?.remoteAddress); }; - const recordHookAuthFailure = ( - clientKey: string, - nowMs: number, - ): { throttled: boolean; retryAfterSeconds?: number } => { - if (!hookAuthFailures.has(clientKey) && hookAuthFailures.size >= HOOK_AUTH_FAILURE_TRACK_MAX) { - // Prune expired entries instead of clearing all state. - for (const [key, entry] of hookAuthFailures) { - if (nowMs - entry.windowStartedAtMs >= HOOK_AUTH_FAILURE_WINDOW_MS) { - hookAuthFailures.delete(key); - } - } - // If still at capacity after pruning, drop the oldest half. - if (hookAuthFailures.size >= HOOK_AUTH_FAILURE_TRACK_MAX) { - let toRemove = Math.floor(hookAuthFailures.size / 2); - for (const key of hookAuthFailures.keys()) { - if (toRemove <= 0) { - break; - } - hookAuthFailures.delete(key); - toRemove--; - } - } - } - const current = hookAuthFailures.get(clientKey); - const expired = !current || nowMs - current.windowStartedAtMs >= HOOK_AUTH_FAILURE_WINDOW_MS; - const next: HookAuthFailure = expired - ? { count: 1, windowStartedAtMs: nowMs } - : { count: current.count + 1, windowStartedAtMs: current.windowStartedAtMs }; - // Delete-before-set refreshes Map insertion order so recently-active - // clients are not evicted before dormant ones during oldest-half eviction. - if (hookAuthFailures.has(clientKey)) { - hookAuthFailures.delete(clientKey); - } - hookAuthFailures.set(clientKey, next); - if (next.count <= HOOK_AUTH_FAILURE_LIMIT) { - return { throttled: false }; - } - const retryAfterMs = Math.max(1, next.windowStartedAtMs + HOOK_AUTH_FAILURE_WINDOW_MS - nowMs); - return { - throttled: true, - retryAfterSeconds: Math.ceil(retryAfterMs / 1000), - }; - }; - - const clearHookAuthFailure = (clientKey: string) => { - hookAuthFailures.delete(clientKey); - }; - return async (req, res) => { const hooksConfig = getHooksConfig(); if (!hooksConfig) { @@ -296,9 +258,9 @@ export function createHooksRequestHandler( const token = extractHookToken(req); const clientKey = resolveHookClientKey(req); if (!safeEqualSecret(token, hooksConfig.token)) { - const throttle = recordHookAuthFailure(clientKey, Date.now()); - if (throttle.throttled) { - const retryAfter = throttle.retryAfterSeconds ?? 1; + const throttle = hookAuthLimiter.check(clientKey, AUTH_RATE_LIMIT_SCOPE_HOOK_AUTH); + if (!throttle.allowed) { + const retryAfter = throttle.retryAfterMs > 0 ? Math.ceil(throttle.retryAfterMs / 1000) : 1; res.statusCode = 429; res.setHeader("Retry-After", String(retryAfter)); res.setHeader("Content-Type", "text/plain; charset=utf-8"); @@ -306,12 +268,13 @@ export function createHooksRequestHandler( logHooks.warn(`hook auth throttled for ${clientKey}; retry-after=${retryAfter}s`); return true; } + hookAuthLimiter.recordFailure(clientKey, AUTH_RATE_LIMIT_SCOPE_HOOK_AUTH); res.statusCode = 401; res.setHeader("Content-Type", "text/plain; charset=utf-8"); res.end("Unauthorized"); return true; } - clearHookAuthFailure(clientKey); + hookAuthLimiter.reset(clientKey, AUTH_RATE_LIMIT_SCOPE_HOOK_AUTH); if (req.method !== "POST") { res.statusCode = 405; diff --git a/src/hooks/loader.ts b/src/hooks/loader.ts index 342e74ac9af..8c87375359d 100644 --- a/src/hooks/loader.ts +++ b/src/hooks/loader.ts @@ -6,7 +6,6 @@ */ import path from "node:path"; -import { pathToFileURL } from "node:url"; import type { OpenClawConfig } from "../config/config.js"; import { createSubsystemLogger } from "../logging/subsystem.js"; import { isPathInsideWithRealpath } from "../security/scan-paths.js"; @@ -14,6 +13,7 @@ import { resolveHookConfig } from "./config.js"; import { shouldIncludeHook } from "./config.js"; import type { InternalHookHandler } from "./internal-hooks.js"; import { registerInternalHook } from "./internal-hooks.js"; +import { importFileModule, resolveFunctionModuleExport } from "./module-loader.js"; import { loadWorkspaceHookEntries } from "./workspace.js"; const log = createSubsystemLogger("hooks:loader"); @@ -82,16 +82,18 @@ export async function loadInternalHooks( ); continue; } - // Import handler module with cache-busting - const url = pathToFileURL(entry.hook.handlerPath).href; - const cacheBustedUrl = `${url}?t=${Date.now()}`; - const mod = (await import(cacheBustedUrl)) as Record; - // Get handler function (default or named export) const exportName = entry.metadata?.export ?? "default"; - const handler = mod[exportName]; + const mod = await importFileModule({ + modulePath: entry.hook.handlerPath, + cacheBust: true, + }); + const handler = resolveFunctionModuleExport({ + mod, + exportName, + }); - if (typeof handler !== "function") { + if (!handler) { log.error(`Handler '${exportName}' from ${entry.hook.name} is not a function`); continue; } @@ -104,7 +106,7 @@ export async function loadInternalHooks( } for (const event of events) { - registerInternalHook(event, handler as InternalHookHandler); + registerInternalHook(event, handler); } log.info( @@ -157,21 +159,23 @@ export async function loadInternalHooks( continue; } - // Import the module with cache-busting to ensure fresh reload - const url = pathToFileURL(modulePath).href; - const cacheBustedUrl = `${url}?t=${Date.now()}`; - const mod = (await import(cacheBustedUrl)) as Record; - // Get the handler function const exportName = handlerConfig.export ?? "default"; - const handler = mod[exportName]; + const mod = await importFileModule({ + modulePath, + cacheBust: true, + }); + const handler = resolveFunctionModuleExport({ + mod, + exportName, + }); - if (typeof handler !== "function") { + if (!handler) { log.error(`Handler '${exportName}' from ${modulePath} is not a function`); continue; } - registerInternalHook(handlerConfig.event, handler as InternalHookHandler); + registerInternalHook(handlerConfig.event, handler); log.info( `Registered hook (legacy): ${handlerConfig.event} -> ${modulePath}${exportName !== "default" ? `#${exportName}` : ""}`, ); diff --git a/src/hooks/module-loader.test.ts b/src/hooks/module-loader.test.ts new file mode 100644 index 00000000000..efe345f96ff --- /dev/null +++ b/src/hooks/module-loader.test.ts @@ -0,0 +1,48 @@ +import path from "node:path"; +import { pathToFileURL } from "node:url"; +import { describe, expect, it } from "vitest"; +import { resolveFileModuleUrl, resolveFunctionModuleExport } from "./module-loader.js"; + +describe("hooks module loader helpers", () => { + it("builds a file URL without cache-busting by default", () => { + const modulePath = path.resolve("/tmp/hook-handler.js"); + expect(resolveFileModuleUrl({ modulePath })).toBe(pathToFileURL(modulePath).href); + }); + + it("adds a cache-busting query when requested", () => { + const modulePath = path.resolve("/tmp/hook-handler.js"); + expect( + resolveFileModuleUrl({ + modulePath, + cacheBust: true, + nowMs: 123, + }), + ).toBe(`${pathToFileURL(modulePath).href}?t=123`); + }); + + it("resolves explicit function exports", () => { + const fn = () => "ok"; + const resolved = resolveFunctionModuleExport({ + mod: { run: fn }, + exportName: "run", + }); + expect(resolved).toBe(fn); + }); + + it("falls back through named exports when no explicit export is provided", () => { + const fallback = () => "ok"; + const resolved = resolveFunctionModuleExport({ + mod: { transform: fallback }, + fallbackExportNames: ["default", "transform"], + }); + expect(resolved).toBe(fallback); + }); + + it("returns undefined when export exists but is not callable", () => { + const resolved = resolveFunctionModuleExport({ + mod: { run: "nope" }, + exportName: "run", + }); + expect(resolved).toBeUndefined(); + }); +}); diff --git a/src/hooks/module-loader.ts b/src/hooks/module-loader.ts new file mode 100644 index 00000000000..7ce275aea3e --- /dev/null +++ b/src/hooks/module-loader.ts @@ -0,0 +1,46 @@ +import { pathToFileURL } from "node:url"; + +type ModuleNamespace = Record; +type GenericFunction = (...args: never[]) => unknown; + +export function resolveFileModuleUrl(params: { + modulePath: string; + cacheBust?: boolean; + nowMs?: number; +}): string { + const url = pathToFileURL(params.modulePath).href; + if (!params.cacheBust) { + return url; + } + const ts = params.nowMs ?? Date.now(); + return `${url}?t=${ts}`; +} + +export async function importFileModule(params: { + modulePath: string; + cacheBust?: boolean; + nowMs?: number; +}): Promise { + const specifier = resolveFileModuleUrl(params); + return (await import(specifier)) as ModuleNamespace; +} + +export function resolveFunctionModuleExport(params: { + mod: ModuleNamespace; + exportName?: string; + fallbackExportNames?: string[]; +}): T | undefined { + const explicitExport = params.exportName?.trim(); + if (explicitExport) { + const candidate = params.mod[explicitExport]; + return typeof candidate === "function" ? (candidate as T) : undefined; + } + const fallbacks = params.fallbackExportNames ?? ["default"]; + for (const exportName of fallbacks) { + const candidate = params.mod[exportName]; + if (typeof candidate === "function") { + return candidate as T; + } + } + return undefined; +}