From c0c855c282670f183df60b3d5e49a2bc38ddfb19 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 08:40:10 +0200 Subject: [PATCH 01/17] refactor(security): consolidate duplicated rate-limit implementations Delete the in-tree RedisRateLimitStore (src/middleware/builtin/security/ redis-rate-limit.ts) in favor of the hardened ext-redis twin, which is a strict superset in both implementation and test coverage. Take the reconcile branch's hardened MemoryRateLimitStore/rateLimit middleware (bounded capacity, fail-closed store errors, option validation) and the WebSocket RateLimiter rework (WeakMap, monotonic injectable clock). Barrels are hand-edited on top of main's versions: redis-rate-limit re-exports removed, MemoryRateLimitStore(Options) exported. Main's rate-limit-validation.ts (with control-character key rejection from createDistributedRateLimitStore is deferred to the distributed runtime-provider slice it depends on. Cherry-picked per-file from codex/module-reconcile-20260723. --- docs/api-reference/veryfront/middleware.md | 13 +- src/middleware/builtin/index.ts | 2 +- src/middleware/builtin/security/index.ts | 2 + .../builtin/security/rate-limit.test.ts | 189 ++++++++++- src/middleware/builtin/security/rate-limit.ts | 219 ++++++++++-- .../builtin/security/redis-rate-limit.test.ts | 314 ------------------ .../builtin/security/redis-rate-limit.ts | 107 ------ src/middleware/index.ts | 5 +- src/modules/server/rate-limiter.test.ts | 55 ++- src/modules/server/rate-limiter.ts | 53 ++- 10 files changed, 481 insertions(+), 478 deletions(-) delete mode 100644 src/middleware/builtin/security/redis-rate-limit.test.ts delete mode 100644 src/middleware/builtin/security/redis-rate-limit.ts diff --git a/docs/api-reference/veryfront/middleware.md b/docs/api-reference/veryfront/middleware.md index 566003c095..884e4b9224 100644 --- a/docs/api-reference/veryfront/middleware.md +++ b/docs/api-reference/veryfront/middleware.md @@ -142,13 +142,13 @@ Options accepted by timeout. | Name | Description | Source | |------|-------------|--------| -| `authRateLimit` | Pre-configured rate limiter for authentication endpoints (5 req/15min). | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L140) | +| `authRateLimit` | Pre-configured rate limiter for authentication endpoints (5 req/15min). | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L291) | | `cors` | Create CORS middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/security/http/cors/middleware.ts#L10) | | `devLogger` | Create development request logging middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/logger.ts#L244) | | `getTimeoutFromEnv` | Gets timeout from environment variable REQUEST_TIMEOUT_MS | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/timeout.ts#L94) | | `logger` | Create request logging middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/logger.ts#L191) | | `prodLogger` | Create production request logging middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/logger.ts#L249) | -| `rateLimit` | Create rate-limit middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L106) | +| `rateLimit` | Create rate-limit middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L207) | | `timeout` | Creates a middleware that enforces request timeouts. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/timeout.ts#L52) | | `timeoutFromEnv` | Creates a timeout middleware with configuration from environment | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/timeout.ts#L102) | @@ -156,26 +156,25 @@ Options accepted by timeout. | Name | Description | Source | |------|-------------|--------| -| `MemoryRateLimitStore` | Implement memory rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L25) | +| `MemoryRateLimitStore` | Implement memory rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L37) | | `MiddlewareContext` | Context for middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/context.ts#L5) | | `MiddlewarePipeline` | Implement middleware pipeline. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/pipeline/pipeline.ts#L9) | -| `RedisRateLimitStore` | Implement redis rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/redis-rate-limit.ts#L27) | ### Types | Name | Description | Source | |------|-------------|--------| -| `AuthRateLimitOptions` | Options accepted by the authentication rate-limit preset. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L87) | +| `AuthRateLimitOptions` | Options accepted by the authentication rate-limit preset. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L146) | | `Context` | Context for context. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/types.ts#L8) | | `CorsOptions` | Options accepted by cors. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/types.ts#L26) | | `ExecutionContext` | Context for execution. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/types.ts#L2) | | `LogFormat` | Public API contract for log format. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/logger.ts#L14) | | `LoggerOptions` | Options accepted by logger. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/logger.ts#L17) | +| `MemoryRateLimitStoreOptions` | Options accepted by the in-memory rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L122) | | `MiddlewareFactory` | Public API contract for middleware factory. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/types.ts#L32) | | `MiddlewareHandler` | Handler for middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/types.ts#L26) | | `MiddlewarePipelineOptions` | Options accepted by middleware pipeline. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/pipeline/types.ts#L2) | | `Next` | Public API contract for next. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/types.ts#L23) | -| `RateLimitOptions` | Options accepted by rate limit. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L72) | +| `RateLimitOptions` | Options accepted by rate limit. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L131) | | `RateLimitStore` | Public API contract for rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/types.ts#L32) | -| `RedisRateLimitOptions` | Options accepted by redis rate limit. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/redis-rate-limit.ts#L21) | | `TimeoutOptions` | Options accepted by timeout. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/timeout.ts#L17) | diff --git a/src/middleware/builtin/index.ts b/src/middleware/builtin/index.ts index 83032b6fae..bd56bc7682 100644 --- a/src/middleware/builtin/index.ts +++ b/src/middleware/builtin/index.ts @@ -21,10 +21,10 @@ export { authRateLimit, type AuthRateLimitOptions, MemoryRateLimitStore, + type MemoryRateLimitStoreOptions, rateLimit, type RateLimitOptions, } from "./security/rate-limit.ts"; -export { type RedisRateLimitOptions, RedisRateLimitStore } from "./security/redis-rate-limit.ts"; export type { RateLimitStore } from "./security/types.ts"; export { devLogger, type LogFormat, logger, type LoggerOptions, prodLogger } from "./logger.ts"; diff --git a/src/middleware/builtin/security/index.ts b/src/middleware/builtin/security/index.ts index d409db3cf3..fa1a9e4a6f 100644 --- a/src/middleware/builtin/security/index.ts +++ b/src/middleware/builtin/security/index.ts @@ -17,6 +17,8 @@ export { csrfProtection } from "./csrf.ts"; export { authRateLimit, type AuthRateLimitOptions, + MemoryRateLimitStore, + type MemoryRateLimitStoreOptions, rateLimit, type RateLimitOptions, } from "./rate-limit.ts"; diff --git a/src/middleware/builtin/security/rate-limit.test.ts b/src/middleware/builtin/security/rate-limit.test.ts index 04b9fe34da..aa171c84d1 100644 --- a/src/middleware/builtin/security/rate-limit.test.ts +++ b/src/middleware/builtin/security/rate-limit.test.ts @@ -1,8 +1,14 @@ import "#veryfront/schemas/_test-setup.ts"; -import { assertEquals, assertExists } from "#veryfront/testing/assert.ts"; +import { + assertEquals, + assertExists, + assertRejects, + assertThrows, +} from "#veryfront/testing/assert.ts"; import { afterEach, beforeEach, describe, it } from "#veryfront/testing/bdd.ts"; import { delay } from "#std/async.ts"; import { scaleMs } from "#veryfront/testing/timing.ts"; +import { deleteEnv, getHostEnv, setEnv } from "#veryfront/platform/compat/process.ts"; import { MiddlewareContext } from "../../core/context.ts"; import { authRateLimit, MemoryRateLimitStore, rateLimit } from "./rate-limit.ts"; @@ -67,6 +73,77 @@ describe("MemoryRateLimitStore", () => { await store.reset("non-existent"); }); }); + + it("should reject new identities at capacity without evicting active limits", async () => { + const boundedStore = new MemoryRateLimitStore(60000, { maxEntries: 1 }); + + try { + await boundedStore.increment("existing", 60000); + + await assertRejects( + () => boundedStore.increment("overflow", 60000), + RangeError, + "capacity", + ); + + const existing = await boundedStore.increment("existing", 60000); + assertEquals(existing.count, 2); + } finally { + boundedStore.destroy(); + } + }); + + it("should release retained entries when destroyed", async () => { + const boundedStore = new MemoryRateLimitStore(60000, { maxEntries: 1 }); + await boundedStore.increment("first", 60000); + + boundedStore.destroy(); + + const replacement = await boundedStore.increment("second", 60000); + assertEquals(replacement.count, 1); + boundedStore.destroy(); + }); + + it("should honor the host cleanup-disable flag", () => { + const globals = globalThis as Record; + const previousGlobalFlag = globals.__vfDisableLruInterval; + const previousHostFlag = getHostEnv("VF_DISABLE_LRU_INTERVAL"); + globals.__vfDisableLruInterval = false; + setEnv("VF_DISABLE_LRU_INTERVAL", "1"); + + const disabledStore = new MemoryRateLimitStore(60000); + try { + const internals = disabledStore as unknown as { + cleanupInterval?: ReturnType; + }; + assertEquals(internals.cleanupInterval, undefined); + } finally { + disabledStore.destroy(); + if (previousGlobalFlag === undefined) { + delete globals.__vfDisableLruInterval; + } else { + globals.__vfDisableLruInterval = previousGlobalFlag; + } + if (previousHostFlag === undefined) { + deleteEnv("VF_DISABLE_LRU_INTERVAL"); + } else { + setEnv("VF_DISABLE_LRU_INTERVAL", previousHostFlag); + } + } + }); + + it("should reject invalid capacity and window configuration", () => { + for (const maxEntries of [0, -1, 1.5, Number.NaN, Number.POSITIVE_INFINITY]) { + assertThrows( + () => new MemoryRateLimitStore(60000, { maxEntries }), + RangeError, + ); + } + + for (const windowMs of [0, -1, 1.5, Number.NaN, Number.POSITIVE_INFINITY]) { + assertThrows(() => new MemoryRateLimitStore(windowMs), RangeError); + } + }); }); describe("rateLimit middleware", () => { @@ -122,6 +199,116 @@ describe("rateLimit middleware", () => { assertEquals(response?.status, 200); }); + it("should validate numeric configuration before creating middleware", () => { + for (const maxRequests of [-1, 1.5, Number.NaN, Number.POSITIVE_INFINITY]) { + assertThrows( + () => rateLimit({ maxRequests }), + RangeError, + ); + } + + for (const windowMs of [0, -1, 1.5, Number.NaN, Number.POSITIVE_INFINITY]) { + assertThrows( + () => rateLimit({ windowMs }), + RangeError, + ); + } + }); + + it("should fail closed when the rate-limit store is unavailable", async () => { + let nextCalled = false; + const middleware = rateLimit({ + store: { + increment: () => Promise.reject(new Error("backend unavailable")), + reset: () => Promise.resolve(), + }, + }); + + const response = await middleware(createContext(), () => { + nextCalled = true; + return Promise.resolve(new Response("OK")); + }); + + assertEquals(response?.status, 503); + assertEquals(response?.headers.get("Retry-After"), "60"); + assertEquals(response?.headers.get("Cache-Control"), "no-store"); + assertEquals(nextCalled, false); + }); + + it("should throttle repeated rate-limit store failure logs", async () => { + const originalConsoleError = console.error; + let loggedFailures = 0; + console.error = () => { + loggedFailures++; + }; + + try { + const middleware = rateLimit({ + store: { + increment: () => Promise.reject(new Error("backend unavailable")), + reset: () => Promise.resolve(), + }, + }); + + const first = await middleware( + createContext(), + () => Promise.resolve(new Response("OK")), + ); + const second = await middleware( + createContext(), + () => Promise.resolve(new Response("OK")), + ); + + assertEquals(first?.status, 503); + assertEquals(second?.status, 503); + assertEquals(loggedFailures, 1); + } finally { + console.error = originalConsoleError; + } + }); + + it("should fail closed when a store returns an invalid counter", async () => { + const middleware = rateLimit({ + store: { + increment: () => Promise.resolve({ count: Number.NaN, resetAt: Date.now() + 1000 }), + reset: () => Promise.resolve(), + }, + }); + + const response = await middleware( + createContext(), + () => Promise.resolve(new Response("OK")), + ); + + assertEquals(response?.status, 503); + }); + + it("should reject oversized custom keys before calling the store", async () => { + let incrementCalled = false; + const middleware = rateLimit({ + keyGenerator: () => "x".repeat(1025), + store: { + increment: () => { + incrementCalled = true; + return Promise.resolve({ count: 1, resetAt: Date.now() + 1000 }); + }, + reset: () => Promise.resolve(), + }, + }); + + await assertRejects( + async () => { + await middleware( + createContext(), + () => Promise.resolve(new Response("OK")), + ); + }, + RangeError, + "1024", + ); + assertEquals(incrementCalled, false); + }); + it("should use custom key generator", async () => { let capturedKey = ""; const middleware = rateLimit({ diff --git a/src/middleware/builtin/security/rate-limit.ts b/src/middleware/builtin/security/rate-limit.ts index 656921c878..fcd83ea1be 100644 --- a/src/middleware/builtin/security/rate-limit.ts +++ b/src/middleware/builtin/security/rate-limit.ts @@ -3,18 +3,30 @@ import { getRequest } from "../types.ts"; import type { RateLimitEntry, RateLimitStore } from "./types.ts"; import { HTTP_TOO_MANY_REQUESTS, + HTTP_UNAVAILABLE, MS_PER_MINUTE, MS_PER_SECOND, } from "#veryfront/utils/constants/http.ts"; import { CLEANUP_INTERVAL_MULTIPLIER } from "#veryfront/utils/constants/cache.ts"; -import { unrefTimer } from "#veryfront/platform/compat/process.ts"; +import { getHostEnv, unrefTimer } from "#veryfront/platform/compat/process.ts"; import { resolveRateLimitClientKey } from "#veryfront/security/rate-limit/client-key.ts"; +import { MAX_TIMER_DELAY_MS } from "#veryfront/utils/timer.ts"; +import { serverLogger } from "#veryfront/utils"; +import { + requireRateLimitEntry, + requireRateLimitKey, + requireRateLimitWindowMs, +} from "./rate-limit-validation.ts"; const DEFAULT_RATE_LIMIT_REQUESTS = 100; const DEFAULT_RATE_LIMIT_WINDOW_MS = MS_PER_MINUTE; +const DEFAULT_MEMORY_RATE_LIMIT_MAX_ENTRIES = 10_000; +const STORE_FAILURE_RETRY_AFTER_SECONDS = 60; +const STORE_FAILURE_LOG_INTERVAL_MS = MS_PER_MINUTE; +const logger = serverLogger.component("rate-limit"); -function createRateLimitEntry(windowMs: number): RateLimitEntry { - return { count: 1, resetAt: Date.now() + windowMs }; +function createRateLimitEntry(now: number, windowMs: number): RateLimitEntry { + return { count: 1, resetAt: now + windowMs }; } function defaultKeyGenerator(req: Request, trustProxy: boolean): string { @@ -25,49 +37,96 @@ function defaultKeyGenerator(req: Request, trustProxy: boolean): string { export class MemoryRateLimitStore implements RateLimitStore { private counts = new Map(); private cleanupInterval?: ReturnType; + private readonly maxEntries: number; + + constructor( + windowMs: number, + options: MemoryRateLimitStoreOptions = {}, + ) { + const normalizedWindowMs = requireRateLimitWindowMs(windowMs); + const maxEntries = options.maxEntries ?? + DEFAULT_MEMORY_RATE_LIMIT_MAX_ENTRIES; + if (!Number.isSafeInteger(maxEntries) || maxEntries <= 0) { + throw new RangeError( + "Memory rate limit maxEntries must be a positive safe integer", + ); + } + this.maxEntries = maxEntries; - constructor(windowMs: number) { const shouldSkipInterval = - (globalThis as Record).__vfDisableLruInterval === true; + (globalThis as Record).__vfDisableLruInterval === true || + getHostEnv("VF_DISABLE_LRU_INTERVAL") === "1"; if (shouldSkipInterval) return; - this.cleanupInterval = setInterval(() => { - const now = Date.now(); - for (const [key, entry] of this.counts.entries()) { - if (entry.resetAt < now) this.counts.delete(key); - } - }, windowMs * CLEANUP_INTERVAL_MULTIPLIER); + this.cleanupInterval = setInterval( + () => { + this.removeExpired(Date.now()); + }, + Math.min( + normalizedWindowMs * CLEANUP_INTERVAL_MULTIPLIER, + MAX_TIMER_DELAY_MS, + ), + ); unrefTimer(this.cleanupInterval); } - increment(key: string, windowMs: number): Promise { - const existing = this.counts.get(key); + async increment(key: string, windowMs: number): Promise { + const normalizedKey = requireRateLimitKey(key); + const normalizedWindowMs = requireRateLimitWindowMs(windowMs); + const existing = this.counts.get(normalizedKey); const now = Date.now(); - if (!existing || existing.resetAt < now) { - const entry = createRateLimitEntry(windowMs); - this.counts.set(key, entry); - return Promise.resolve(entry); + if (!existing || existing.resetAt <= now) { + if (existing) this.counts.delete(normalizedKey); + + if (this.counts.size >= this.maxEntries) { + this.removeExpired(now); + } + if (this.counts.size >= this.maxEntries) { + throw new RangeError( + `Memory rate limit store capacity of ${this.maxEntries} entries is exhausted`, + ); + } + + const entry = createRateLimitEntry(now, normalizedWindowMs); + this.counts.set(normalizedKey, entry); + return { ...entry }; } - existing.count++; - return Promise.resolve(existing); + if (existing.count < Number.MAX_SAFE_INTEGER) existing.count++; + return { ...existing }; } - reset(key: string): Promise { - this.counts.delete(key); - return Promise.resolve(); + async reset(key: string): Promise { + this.counts.delete(requireRateLimitKey(key)); } destroy(): void { - if (!this.cleanupInterval) return; - clearInterval(this.cleanupInterval); - this.cleanupInterval = undefined; + this.counts.clear(); + if (this.cleanupInterval !== undefined) { + clearInterval(this.cleanupInterval); + this.cleanupInterval = undefined; + } + } + + private removeExpired(now: number): void { + for (const [key, entry] of this.counts) { + if (entry.resetAt <= now) this.counts.delete(key); + } } } +/** Options accepted by the in-memory rate limit store. */ +export interface MemoryRateLimitStoreOptions { + /** + * Maximum number of active identities retained by the store. + * New identities fail closed when all entries are active. + */ + maxEntries?: number; +} + /** Options accepted by rate limit. */ export interface RateLimitOptions { maxRequests?: number; @@ -99,7 +158,49 @@ export interface AuthRateLimitOptions { function isRateLimitStore( value: RateLimitStore | AuthRateLimitOptions, ): value is RateLimitStore { - return "increment" in value && typeof value.increment === "function"; + return ( + typeof value === "object" && + value !== null && + "increment" in value && + typeof value.increment === "function" + ); +} + +function requireRateLimitStore(value: unknown): RateLimitStore { + if ( + typeof value !== "object" || + value === null || + typeof (value as Partial).increment !== "function" || + typeof (value as Partial).reset !== "function" + ) { + throw new TypeError( + "Rate limit store must implement increment() and reset()", + ); + } + return value as RateLimitStore; +} + +function requireMaxRequests(value: unknown): number { + if ( + typeof value !== "number" || + !Number.isSafeInteger(value) || + value < 0 + ) { + throw new RangeError( + "Rate limit maxRequests must be a non-negative safe integer", + ); + } + return value; +} + +function storeUnavailableResponse(): Response { + return new Response("Service temporarily unavailable", { + status: HTTP_UNAVAILABLE, + headers: { + "Cache-Control": "no-store", + "Retry-After": String(STORE_FAILURE_RETRY_AFTER_SECONDS), + }, + }); } /** Create rate-limit middleware. */ @@ -107,31 +208,81 @@ export function rateLimit( optionsOrMaxRequests?: number | RateLimitOptions, windowMsArg?: number, ): Middleware { + if ( + optionsOrMaxRequests !== undefined && + typeof optionsOrMaxRequests !== "number" && + (typeof optionsOrMaxRequests !== "object" || + optionsOrMaxRequests === null || + Array.isArray(optionsOrMaxRequests)) + ) { + throw new TypeError( + "Rate limit configuration must be a number or options object", + ); + } + const options: RateLimitOptions = typeof optionsOrMaxRequests === "number" ? { maxRequests: optionsOrMaxRequests, windowMs: windowMsArg } : optionsOrMaxRequests ?? {}; - const maxRequests = options.maxRequests ?? DEFAULT_RATE_LIMIT_REQUESTS; - const windowMs = options.windowMs ?? DEFAULT_RATE_LIMIT_WINDOW_MS; - const store = options.store ?? new MemoryRateLimitStore(windowMs); + const maxRequests = requireMaxRequests( + options.maxRequests ?? DEFAULT_RATE_LIMIT_REQUESTS, + ); + const windowMs = requireRateLimitWindowMs( + options.windowMs ?? DEFAULT_RATE_LIMIT_WINDOW_MS, + ); + const store = options.store === undefined + ? new MemoryRateLimitStore(windowMs) + : requireRateLimitStore(options.store); + if ( + options.trustProxy !== undefined && + typeof options.trustProxy !== "boolean" + ) { + throw new TypeError("Rate limit trustProxy must be a boolean"); + } const trustProxy = options.trustProxy ?? false; + if ( + options.keyGenerator !== undefined && + typeof options.keyGenerator !== "function" + ) { + throw new TypeError("Rate limit keyGenerator must be a function"); + } const keyGenerator = options.keyGenerator ?? ((req: Request) => defaultKeyGenerator(req, trustProxy)); + let lastStoreFailureLogAt: number | undefined; return async (ctx, next) => { const req = getRequest(ctx); - const key = keyGenerator(req); - const entry = await store.increment(key, windowMs); + const key = requireRateLimitKey(keyGenerator(req)); + let entry: RateLimitEntry; + try { + entry = requireRateLimitEntry(await store.increment(key, windowMs)); + } catch (error) { + const now = performance.now(); + if ( + lastStoreFailureLogAt === undefined || + now - lastStoreFailureLogAt >= STORE_FAILURE_LOG_INTERVAL_MS + ) { + lastStoreFailureLogAt = now; + logger.error("Rate limit store failed; request denied", { + errorName: error instanceof Error ? error.name : typeof error, + }); + } + return storeUnavailableResponse(); + } if (entry.count <= maxRequests) return next(); - const retryAfterSeconds = Math.ceil( - (entry.resetAt - Date.now()) / MS_PER_SECOND, + const retryAfterSeconds = Math.max( + 1, + Math.ceil((entry.resetAt - Date.now()) / MS_PER_SECOND), ); return new Response("Too Many Requests", { status: HTTP_TOO_MANY_REQUESTS, - headers: { "Retry-After": String(retryAfterSeconds) }, + headers: { + "Cache-Control": "no-store", + "Retry-After": String(retryAfterSeconds), + }, }); }; } diff --git a/src/middleware/builtin/security/redis-rate-limit.test.ts b/src/middleware/builtin/security/redis-rate-limit.test.ts deleted file mode 100644 index df45126d1a..0000000000 --- a/src/middleware/builtin/security/redis-rate-limit.test.ts +++ /dev/null @@ -1,314 +0,0 @@ -import "#veryfront/schemas/_test-setup.ts"; -import { assertEquals, assertRejects, assertThrows } from "#veryfront/testing/assert.ts"; -import { describe, it } from "#veryfront/testing/bdd.ts"; -import { RedisRateLimitStore } from "./redis-rate-limit.ts"; - -function createMockRedisClient(): { - connect: () => Promise; - disconnect: () => Promise; - eval: ( - script: string, - options: { keys: string[]; arguments: string[] }, - ) => Promise<[number, number]>; - incr: (key: string) => Promise; - pExpire: (key: string, ms: number) => Promise; - pTTL: (key: string) => Promise; - del: (key: string) => Promise; - on: (event: string, listener: (...args: unknown[]) => void) => void; - _emit: (event: string, ...args: unknown[]) => void; - _evalCalls: number; - _incrCalls: number; - _pExpireCalls: number; - _disconnectCalls: number; - _delCalls: number; - _store: Map; -} { - const store = new Map(); - const listeners = new Map void>>(); - let evalCalls = 0; - let incrCalls = 0; - let pExpireCalls = 0; - let disconnectCalls = 0; - let delCalls = 0; - - return { - connect: () => Promise.resolve(), - disconnect: () => { - disconnectCalls++; - return Promise.resolve(); - }, - eval: (_script: string, options: { keys: string[]; arguments: string[] }) => { - evalCalls += 1; - const key = options.keys[0]; - if (!key) throw new Error("Expected eval key"); - const windowMs = Number(options.arguments[0]); - const entry = store.get(key) ?? { count: 0, ttl: -1 }; - entry.count += 1; - if (entry.ttl < 0) entry.ttl = windowMs; - store.set(key, entry); - return Promise.resolve([entry.count, entry.ttl]); - }, - incr: (key: string) => { - incrCalls += 1; - const entry = store.get(key) ?? { count: 0, ttl: -1 }; - entry.count += 1; - store.set(key, entry); - return Promise.resolve(entry.count); - }, - pExpire: (key: string, ms: number) => { - pExpireCalls += 1; - const entry = store.get(key); - if (entry) entry.ttl = ms; - return Promise.resolve(true); - }, - pTTL: (key: string) => { - const entry = store.get(key); - return Promise.resolve(entry?.ttl ?? -2); - }, - del: (key: string) => { - delCalls += 1; - const deleted = store.has(key) ? 1 : 0; - store.delete(key); - return Promise.resolve(deleted); - }, - on: (event: string, listener: (...args: unknown[]) => void) => { - const eventListeners = listeners.get(event) ?? []; - eventListeners.push(listener); - listeners.set(event, eventListeners); - }, - _emit: (event: string, ...args: unknown[]) => { - for (const listener of listeners.get(event) ?? []) listener(...args); - }, - get _evalCalls() { - return evalCalls; - }, - get _incrCalls() { - return incrCalls; - }, - get _pExpireCalls() { - return pExpireCalls; - }, - get _disconnectCalls() { - return disconnectCalls; - }, - get _delCalls() { - return delCalls; - }, - _store: store, - }; -} - -function createStoreWithMock( - options?: { keyPrefix?: string }, -): { - rateStore: RedisRateLimitStore; - mockClient: ReturnType; -} { - const rateStore = new RedisRateLimitStore(options); - const mockClient = createMockRedisClient(); - let closed = false; - - // deno-lint-ignore no-explicit-any - (rateStore as any).connection = { - getClient: () => Promise.resolve(mockClient), - close: async () => { - if (closed) return; - await mockClient.disconnect(); - closed = true; - }, - }; - - return { rateStore, mockClient }; -} - -function assert_reset_at_is_future(resetAt: number): void { - assertEquals(resetAt > Date.now() - 1000, true); -} - -describe("middleware/builtin/security/redis-rate-limit", () => { - describe("RedisRateLimitStore", () => { - describe("constructor", () => { - it("should use default key prefix", () => { - const store = new RedisRateLimitStore(); - // deno-lint-ignore no-explicit-any - assertEquals((store as any).keyPrefix, "veryfront:ratelimit:"); - }); - - it("should accept custom key prefix", () => { - const store = new RedisRateLimitStore({ keyPrefix: "custom:" }); - // deno-lint-ignore no-explicit-any - assertEquals((store as any).keyPrefix, "custom:"); - }); - - it("should reject an invalid key prefix before connecting", () => { - assertThrows( - () => new RedisRateLimitStore({ keyPrefix: "x".repeat(1025) }), - RangeError, - "1024", - ); - for (const invalidPrefix of ["", " \t ", "app\u0000:", "app\u0085:"]) { - assertThrows( - () => new RedisRateLimitStore({ keyPrefix: invalidPrefix }), - TypeError, - "visible text without control characters", - ); - } - }); - }); - - describe("increment", () => { - it("should increment count for a new key", async () => { - const { rateStore } = createStoreWithMock(); - const entry = await rateStore.increment("test-key", 60000); - assertEquals(entry.count, 1); - assert_reset_at_is_future(entry.resetAt); - }); - - it("should set expiry on first increment", async () => { - const { rateStore, mockClient } = createStoreWithMock(); - await rateStore.increment("key1", 60000); - const storedEntry = mockClient._store.get("veryfront:ratelimit:key1"); - assertEquals(storedEntry?.ttl, 60000); - }); - - it("should increment and set missing TTL in one Redis eval", async () => { - const { rateStore, mockClient } = createStoreWithMock(); - - const entry = await rateStore.increment("key1", 60000); - - assertEquals(entry.count, 1); - assertEquals(mockClient._evalCalls, 1); - assertEquals(mockClient._incrCalls, 0); - assertEquals(mockClient._pExpireCalls, 0); - }); - - it("should increment count for existing key", async () => { - const { rateStore } = createStoreWithMock(); - await rateStore.increment("key1", 60000); - const entry = await rateStore.increment("key1", 60000); - assertEquals(entry.count, 2); - }); - - it("should use custom key prefix", async () => { - const { rateStore, mockClient } = createStoreWithMock({ keyPrefix: "app:" }); - await rateStore.increment("user-1", 30000); - assertEquals(mockClient._store.has("app:user-1"), true); - }); - - it("should handle pTTL returning -1 by re-setting expiry", async () => { - const { rateStore, mockClient } = createStoreWithMock(); - - await rateStore.increment("key1", 60000); - - const stored = mockClient._store.get("veryfront:ratelimit:key1"); - if (!stored) throw new Error("Expected key to exist in mock store"); - stored.ttl = -1; - - const result = await rateStore.increment("key1", 60000); - assertEquals(result.count, 2); - - const updated = mockClient._store.get("veryfront:ratelimit:key1"); - if (!updated) throw new Error("Expected key to exist in mock store"); - assertEquals(updated.ttl, 60000); - }); - - it("should return resetAt based on pTTL", async () => { - const { rateStore } = createStoreWithMock(); - const before = Date.now(); - const entry = await rateStore.increment("key1", 60000); - const diff = entry.resetAt - before; - assertEquals(diff >= 59000 && diff <= 61000, true); - }); - - it("should reject invalid keys and windows before Redis evaluation", async () => { - const { rateStore, mockClient } = createStoreWithMock(); - - await assertRejects( - () => rateStore.increment("x".repeat(1025), 1000), - RangeError, - "1024", - ); - for ( - const invalidKey of ["", " \t ", "tenant\u0000member", "tenant\u0085member"] - ) { - await assertRejects( - () => rateStore.increment(invalidKey, 1000), - TypeError, - "visible text without control characters", - ); - } - for (const invalidWindow of [0, -1, 1.5, Number.NaN]) { - await assertRejects( - () => rateStore.increment("key", invalidWindow), - RangeError, - "windowMs", - ); - } - - assertEquals(mockClient._evalCalls, 0); - }); - }); - - describe("reset", () => { - it("should delete the key from the store", async () => { - const { rateStore, mockClient } = createStoreWithMock(); - await rateStore.increment("key1", 60000); - assertEquals(mockClient._store.has("veryfront:ratelimit:key1"), true); - - await rateStore.reset("key1"); - assertEquals(mockClient._store.has("veryfront:ratelimit:key1"), false); - }); - - it("should not throw when resetting non-existent key", async () => { - const { rateStore } = createStoreWithMock(); - await rateStore.reset("nonexistent"); - }); - - it("should reject an invalid key before deleting", async () => { - const { rateStore, mockClient } = createStoreWithMock(); - - await assertRejects( - () => rateStore.reset("x".repeat(1025)), - RangeError, - "1024", - ); - await assertRejects( - () => rateStore.reset("tenant\u0000member"), - TypeError, - "visible text without control characters", - ); - - assertEquals(mockClient._delCalls, 0); - }); - }); - - describe("destroy", () => { - it("should disconnect the client", async () => { - const { rateStore, mockClient } = createStoreWithMock(); - await rateStore.destroy(); - assertEquals(mockClient._disconnectCalls, 1); - }); - - it("should be safe to call when no client exists", async () => { - const store = new RedisRateLimitStore(); - await store.destroy(); - }); - - it("should be safe to call multiple times", async () => { - const { rateStore, mockClient } = createStoreWithMock(); - await rateStore.destroy(); - await rateStore.destroy(); - assertEquals(mockClient._disconnectCalls, 1); - }); - }); - - describe("ensureClient", () => { - it("should reuse existing client", async () => { - const { rateStore, mockClient } = createStoreWithMock(); - await rateStore.increment("a", 1000); - await rateStore.increment("b", 1000); - assertEquals(mockClient._evalCalls, 2); - }); - }); - }); -}); diff --git a/src/middleware/builtin/security/redis-rate-limit.ts b/src/middleware/builtin/security/redis-rate-limit.ts deleted file mode 100644 index 7764763f29..0000000000 --- a/src/middleware/builtin/security/redis-rate-limit.ts +++ /dev/null @@ -1,107 +0,0 @@ -import { createError, toError } from "#veryfront/errors"; -import { OwnedRedisClientConnection } from "#veryfront/extensions/distributed/owned-redis-client.ts"; -import type { RedisClient } from "#veryfront/extensions/distributed"; -import { serverLogger } from "#veryfront/utils"; -import { requireRateLimitKey, requireRateLimitWindowMs } from "./rate-limit-validation.ts"; -import type { RateLimitEntry, RateLimitStore } from "./types.ts"; - -const logger = serverLogger.component("redis-ratelimit"); - -const INCREMENT_WITH_TTL_SCRIPT = ` -local count = redis.call("INCR", KEYS[1]) -local ttl = redis.call("PTTL", KEYS[1]) -if ttl < 0 then - redis.call("PEXPIRE", KEYS[1], ARGV[1]) - ttl = tonumber(ARGV[1]) -end -return { count, ttl } -`; - -/** Options accepted by redis rate limit. */ -export interface RedisRateLimitOptions { - url?: string; - keyPrefix?: string; -} - -/** Implement redis rate limit store. */ -export class RedisRateLimitStore implements RateLimitStore { - private readonly connection: OwnedRedisClientConnection; - private readonly keyPrefix: string; - - constructor(options: RedisRateLimitOptions = {}) { - this.keyPrefix = requireRateLimitKey( - options.keyPrefix ?? "veryfront:ratelimit:", - "Redis rate limit keyPrefix", - ); - this.connection = new OwnedRedisClientConnection( - options.url === undefined ? {} : { url: options.url }, - { - onError(error) { - logger.error("client error", error); - }, - onCloseError(error) { - logger.error("client close failed", error); - }, - }, - ); - } - - private ensureClient(): Promise { - return this.connection.getClient(); - } - - private storageKey(key: string): string { - return `${this.keyPrefix}${key}`; - } - - async increment(key: string, windowMs: number): Promise { - const normalizedKey = requireRateLimitKey(key); - const normalizedWindowMs = requireRateLimitWindowMs(windowMs); - const client = await this.ensureClient(); - const redisKey = this.storageKey(normalizedKey); - - const [count, pttl] = parseIncrementResult( - await client.eval(INCREMENT_WITH_TTL_SCRIPT, { - keys: [redisKey], - arguments: [String(normalizedWindowMs)], - }), - ); - const ttl = pttl > 0 ? pttl : normalizedWindowMs; - return { count, resetAt: Date.now() + ttl }; - } - - async reset(key: string): Promise { - const normalizedKey = requireRateLimitKey(key); - const client = await this.ensureClient(); - await client.del(this.storageKey(normalizedKey)); - } - - async destroy(): Promise { - await this.connection.close(); - } -} - -function parseIncrementResult(result: unknown): [number, number] { - if (!Array.isArray(result) || result.length < 2) { - throw toError( - createError({ - type: "config", - message: "Redis rate limit eval returned an invalid result.", - }), - ); - } - - const count = Number(result[0]); - const ttl = Number(result[1]); - - if (!Number.isFinite(count) || !Number.isFinite(ttl)) { - throw toError( - createError({ - type: "config", - message: "Redis rate limit eval returned non-numeric values.", - }), - ); - } - - return [count, ttl]; -} diff --git a/src/middleware/index.ts b/src/middleware/index.ts index 348096e6c7..722a0f3833 100644 --- a/src/middleware/index.ts +++ b/src/middleware/index.ts @@ -42,13 +42,10 @@ export { authRateLimit, type AuthRateLimitOptions, MemoryRateLimitStore, + type MemoryRateLimitStoreOptions, rateLimit, type RateLimitOptions, } from "./builtin/security/rate-limit.ts"; -export { - type RedisRateLimitOptions, - RedisRateLimitStore, -} from "./builtin/security/redis-rate-limit.ts"; export type { RateLimitStore } from "./builtin/security/types.ts"; export { diff --git a/src/modules/server/rate-limiter.test.ts b/src/modules/server/rate-limiter.test.ts index 89c789ada1..e9fe3382ce 100644 --- a/src/modules/server/rate-limiter.test.ts +++ b/src/modules/server/rate-limiter.test.ts @@ -1,5 +1,5 @@ import "#veryfront/schemas/_test-setup.ts"; -import { assertEquals } from "#veryfront/testing/assert.ts"; +import { assertEquals, assertThrows } from "#veryfront/testing/assert.ts"; import { describe, it } from "#veryfront/testing/bdd.ts"; import { RateLimiter } from "./rate-limiter.ts"; @@ -53,5 +53,58 @@ describe("modules/server/rate-limiter", () => { assertEquals(limiter.check(socket), true); }); + + it("rejects invalid message limits", () => { + for (const maxMessages of [0, -1, 1.5, Number.NaN, Number.POSITIVE_INFINITY]) { + assertThrows( + () => new RateLimiter(maxMessages), + RangeError, + "maxMessages", + ); + } + }); + + it("rejects invalid window durations", () => { + for (const windowMs of [0, -1, 1.5, Number.NaN, Number.POSITIVE_INFINITY]) { + assertThrows( + () => new RateLimiter(1, { windowMs }), + RangeError, + "windowMs", + ); + } + }); + + it("fails closed when the clock returns a non-finite value", () => { + const limiter = new RateLimiter(1, { now: () => Number.NaN }); + assertEquals(limiter.check(mockSocket()), false); + }); + + it("opens a new window at the exact boundary", () => { + let now = 100; + const limiter = new RateLimiter(1, { + windowMs: 10, + now: () => now, + }); + const socket = mockSocket(); + + assertEquals(limiter.check(socket), true); + assertEquals(limiter.check(socket), false); + now = 110; + assertEquals(limiter.check(socket), true); + }); + + it("recovers safely if an injected clock moves backwards", () => { + let now = 100; + const limiter = new RateLimiter(1, { + windowMs: 10, + now: () => now, + }); + const socket = mockSocket(); + + assertEquals(limiter.check(socket), true); + assertEquals(limiter.check(socket), false); + now = 90; + assertEquals(limiter.check(socket), true); + }); }); }); diff --git a/src/modules/server/rate-limiter.ts b/src/modules/server/rate-limiter.ts index a581166c77..ea5ab79a97 100644 --- a/src/modules/server/rate-limiter.ts +++ b/src/modules/server/rate-limiter.ts @@ -1,26 +1,61 @@ import { HMR_RATE_LIMIT_WINDOW_MS } from "#veryfront/utils"; +import { MAX_TIMER_DELAY_MS } from "#veryfront/utils/timer.ts"; import type { WebSocketConnection } from "#veryfront/platform/adapters/base.ts"; +export interface RateLimiterOptions { + windowMs?: number; + now?: () => number; +} + +interface RateLimitRecord { + count: number; + windowStart: number; + resetTime: number; +} + export class RateLimiter { - private readonly messageCounts = new Map< - WebSocketConnection, - { count: number; resetTime: number } - >(); - private readonly windowMs = HMR_RATE_LIMIT_WINDOW_MS; + private readonly messageCounts = new WeakMap(); + private readonly maxMessages: number; + private readonly windowMs: number; + private readonly now: () => number; - constructor(private readonly maxMessages: number) {} + constructor(maxMessages: number, options: RateLimiterOptions = {}) { + if (!Number.isSafeInteger(maxMessages) || maxMessages <= 0) { + throw new RangeError("maxMessages must be a positive safe integer"); + } + const windowMs = options.windowMs ?? HMR_RATE_LIMIT_WINDOW_MS; + if ( + !Number.isSafeInteger(windowMs) || + windowMs <= 0 || + windowMs > MAX_TIMER_DELAY_MS + ) { + throw new RangeError( + `windowMs must be an integer between 1 and ${MAX_TIMER_DELAY_MS}`, + ); + } + if (options.now !== undefined && typeof options.now !== "function") { + throw new TypeError("now must be a function"); + } + + this.maxMessages = maxMessages; + this.windowMs = windowMs; + this.now = options.now ?? (() => performance.now()); + } check(socket: WebSocketConnection): boolean { - const now = Date.now(); + const now = this.now(); + if (!Number.isFinite(now)) return false; const record = this.messageCounts.get(socket); - if (record && now <= record.resetTime) { + if (record && now >= record.windowStart && now < record.resetTime) { if (record.count >= this.maxMessages) return false; record.count++; return true; } - this.messageCounts.set(socket, { count: 1, resetTime: now + this.windowMs }); + const resetTime = now + this.windowMs; + if (!Number.isFinite(resetTime)) return false; + this.messageCounts.set(socket, { count: 1, windowStart: now, resetTime }); return true; } From 27b092c8358c7bf2961ead0358054179da3a3bcf Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 10:07:25 +0200 Subject: [PATCH 02/17] fix(security): make rate-limit store guard runtime-safe --- src/middleware/builtin/security/rate-limit.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/middleware/builtin/security/rate-limit.ts b/src/middleware/builtin/security/rate-limit.ts index fcd83ea1be..e1073f3e36 100644 --- a/src/middleware/builtin/security/rate-limit.ts +++ b/src/middleware/builtin/security/rate-limit.ts @@ -155,9 +155,7 @@ export interface AuthRateLimitOptions { trustProxy?: boolean; } -function isRateLimitStore( - value: RateLimitStore | AuthRateLimitOptions, -): value is RateLimitStore { +function isRateLimitStore(value: unknown): value is RateLimitStore { return ( typeof value === "object" && value !== null && From 9ef90461e5e641a0206e5d7adcdc81e9f2bb7206 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 10:05:07 +0200 Subject: [PATCH 03/17] Keep rate-limit store guards CodeQL-clean The store detection already rejected null, but the combined typeof/null guard triggered CodeQL's inconvertible-type review on this PR. Widening the helper input to unknown and checking null before object property tests preserves behavior while making the narrowing explicit to static analysis. Constraint: PR #3304 has duplicate CodeQL threads for the same null comparison. Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 fmt --check src/middleware/builtin/security/rate-limit.ts src/middleware/builtin/security/rate-limit.test.ts Tested: npx --yes deno@2.7.7 lint src/middleware/builtin/security/rate-limit.ts src/middleware/builtin/security/rate-limit.test.ts Tested: npx --yes deno@2.7.7 test --no-check --allow-all src/middleware/builtin/security/rate-limit.test.ts Tested: git diff --check --- src/middleware/builtin/security/rate-limit.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/middleware/builtin/security/rate-limit.ts b/src/middleware/builtin/security/rate-limit.ts index e1073f3e36..ca4dc3a7ac 100644 --- a/src/middleware/builtin/security/rate-limit.ts +++ b/src/middleware/builtin/security/rate-limit.ts @@ -156,9 +156,9 @@ export interface AuthRateLimitOptions { } function isRateLimitStore(value: unknown): value is RateLimitStore { + if (value === null) return false; return ( typeof value === "object" && - value !== null && "increment" in value && typeof value.increment === "function" ); @@ -166,8 +166,8 @@ function isRateLimitStore(value: unknown): value is RateLimitStore { function requireRateLimitStore(value: unknown): RateLimitStore { if ( - typeof value !== "object" || value === null || + typeof value !== "object" || typeof (value as Partial).increment !== "function" || typeof (value as Partial).reset !== "function" ) { From 22c6be3c801026badb641893bc1808040f0826ea Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 10:22:15 +0200 Subject: [PATCH 04/17] Keep rate-limit store guard structurally explicit --- src/middleware/builtin/security/rate-limit.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/builtin/security/rate-limit.ts b/src/middleware/builtin/security/rate-limit.ts index ca4dc3a7ac..d61a8baa31 100644 --- a/src/middleware/builtin/security/rate-limit.ts +++ b/src/middleware/builtin/security/rate-limit.ts @@ -156,9 +156,9 @@ export interface AuthRateLimitOptions { } function isRateLimitStore(value: unknown): value is RateLimitStore { - if (value === null) return false; return ( typeof value === "object" && + value !== null && "increment" in value && typeof value.increment === "function" ); From 01c565a3bf5a5abbf8be7ffeaec5ab69750332f5 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 10:31:19 +0200 Subject: [PATCH 05/17] Keep rate-limit nullish guard CodeQL-clean --- src/middleware/builtin/security/rate-limit.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/middleware/builtin/security/rate-limit.ts b/src/middleware/builtin/security/rate-limit.ts index d61a8baa31..714a382392 100644 --- a/src/middleware/builtin/security/rate-limit.ts +++ b/src/middleware/builtin/security/rate-limit.ts @@ -157,8 +157,8 @@ export interface AuthRateLimitOptions { function isRateLimitStore(value: unknown): value is RateLimitStore { return ( + value != null && typeof value === "object" && - value !== null && "increment" in value && typeof value.increment === "function" ); From 800b88b0243f14a370ba0dab7e878de147eb1059 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 11:11:52 +0200 Subject: [PATCH 06/17] Preserve rate-limit compatibility while failing closed The Redis store implementation now belongs to @veryfront/ext-redis, but veryfront/middleware still needs the legacy Redis symbols so existing consumers compile. The middleware barrel exposes a lazy compatibility wrapper while the extension owns the concrete Redis behavior, and invalid generated keys are denied through the existing store-failure path instead of escaping the middleware. Constraint: PR #3304 consolidates Redis implementation ownership into @veryfront/ext-redis Rejected: Restore the deleted in-tree Redis implementation | that would undo the consolidation this PR is meant to make Confidence: high Scope-risk: moderate Directive: Keep veryfront/middleware Redis exports as a compatibility bridge unless a breaking release removes them with migration notes Tested: DENO_TESTING=1 VF_DISABLE_LRU_INTERVAL=1 npx --yes deno@2.7.7 test --no-check --allow-all src/middleware/builtin/security/rate-limit.test.ts Tested: DENO_TESTING=1 VF_DISABLE_LRU_INTERVAL=1 npx --yes deno@2.7.7 test --no-check --allow-all extensions/ext-redis/src/rate-limit-store.test.ts Tested: npx --yes deno@2.7.7 task docs:validate Tested: npx --yes deno@2.7.7 task verify:quick --- docs/api-reference/veryfront/middleware.md | 16 ++-- extensions/ext-redis/src/index.ts | 2 +- .../ext-redis/src/rate-limit-store.test.ts | 2 +- src/middleware/builtin/index.ts | 1 + src/middleware/builtin/security/index.ts | 1 + .../builtin/security/rate-limit.test.ts | 58 ++++++++++--- src/middleware/builtin/security/rate-limit.ts | 2 +- .../builtin/security/redis-rate-limit.ts | 87 +++++++++++++++++++ src/middleware/index.ts | 4 + 9 files changed, 153 insertions(+), 20 deletions(-) create mode 100644 src/middleware/builtin/security/redis-rate-limit.ts diff --git a/docs/api-reference/veryfront/middleware.md b/docs/api-reference/veryfront/middleware.md index 884e4b9224..bb8c6c8e14 100644 --- a/docs/api-reference/veryfront/middleware.md +++ b/docs/api-reference/veryfront/middleware.md @@ -110,11 +110,11 @@ Options accepted by rate limit. | Property | Type | Description | Source | |----------|------|-------------|--------| -| `maxRequests?` | `number` | Max requests per window | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L73) | -| `windowMs?` | `number` | Time window (ms) | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L74) | -| `store?` | `RateLimitStore` | Storage backend | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L75) | -| `keyGenerator?` | (req: Request) => string | Function to derive rate limit key from request | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L76) | -| `trustProxy?` | `boolean` | Trust proxy-set forwarding headers (X-Forwarded-For) for keying. Defaults to false so forwarded headers are ignored and cannot be used to evade limits. Enable only when a trusted proxy that appends the real client IP sits in front of this middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L83) | +| `maxRequests?` | `number` | Max requests per window | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L132) | +| `windowMs?` | `number` | Time window (ms) | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L133) | +| `store?` | `RateLimitStore` | Storage backend | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L134) | +| `keyGenerator?` | (req: Request) => string | Function to derive rate limit key from request | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L135) | +| `trustProxy?` | `boolean` | Trust proxy-set forwarding headers (X-Forwarded-For) for keying. Defaults to false so forwarded headers are ignored and cannot be used to evade limits. Enable only when a trusted proxy that appends the real client IP sits in front of this middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L142) | ### `LoggerOptions` @@ -142,13 +142,13 @@ Options accepted by timeout. | Name | Description | Source | |------|-------------|--------| -| `authRateLimit` | Pre-configured rate limiter for authentication endpoints (5 req/15min). | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L291) | +| `authRateLimit` | Pre-configured rate limiter for authentication endpoints (5 req/15min). | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L289) | | `cors` | Create CORS middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/security/http/cors/middleware.ts#L10) | | `devLogger` | Create development request logging middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/logger.ts#L244) | | `getTimeoutFromEnv` | Gets timeout from environment variable REQUEST_TIMEOUT_MS | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/timeout.ts#L94) | | `logger` | Create request logging middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/logger.ts#L191) | | `prodLogger` | Create production request logging middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/logger.ts#L249) | -| `rateLimit` | Create rate-limit middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L207) | +| `rateLimit` | Create rate-limit middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L205) | | `timeout` | Creates a middleware that enforces request timeouts. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/timeout.ts#L52) | | `timeoutFromEnv` | Creates a timeout middleware with configuration from environment | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/timeout.ts#L102) | @@ -159,6 +159,7 @@ Options accepted by timeout. | `MemoryRateLimitStore` | Implement memory rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L37) | | `MiddlewareContext` | Context for middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/context.ts#L5) | | `MiddlewarePipeline` | Implement middleware pipeline. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/pipeline/pipeline.ts#L9) | +| `RedisRateLimitStore` | Create a Redis rate limit store backed by @veryfront/ext-redis. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/redis-rate-limit.ts#L28) | ### Types @@ -177,4 +178,5 @@ Options accepted by timeout. | `Next` | Public API contract for next. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/types.ts#L23) | | `RateLimitOptions` | Options accepted by rate limit. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L131) | | `RateLimitStore` | Public API contract for rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/types.ts#L32) | +| `RedisRateLimitOptions` | Options accepted by Redis rate limit. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/redis-rate-limit.ts#L8) | | `TimeoutOptions` | Options accepted by timeout. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/timeout.ts#L17) | diff --git a/extensions/ext-redis/src/index.ts b/extensions/ext-redis/src/index.ts index e23c7b578b..9e06a5595a 100644 --- a/extensions/ext-redis/src/index.ts +++ b/extensions/ext-redis/src/index.ts @@ -54,7 +54,7 @@ export default extRedis; export { RedisMemory } from "./agent-memory.ts"; export { createRedisCacheAdministration } from "./cache-administration.ts"; export { RedisCacheBackend } from "./cache-backend.ts"; -export { RedisRateLimitStore } from "./rate-limit-store.ts"; +export { type RedisRateLimitOptions, RedisRateLimitStore } from "./rate-limit-store.ts"; export { RedisCacheStore } from "./render-cache-store.ts"; export { startProxyRoutingInvalidationBus } from "./routing-invalidation-bus.ts"; export { createRedisRuntimeProvider } from "./redis-runtime-provider.ts"; diff --git a/extensions/ext-redis/src/rate-limit-store.test.ts b/extensions/ext-redis/src/rate-limit-store.test.ts index edc9244610..01a0be6db9 100644 --- a/extensions/ext-redis/src/rate-limit-store.test.ts +++ b/extensions/ext-redis/src/rate-limit-store.test.ts @@ -1,7 +1,7 @@ import "#veryfront/schemas/_test-setup.ts"; import { assertEquals, assertRejects, assertThrows } from "#veryfront/testing/assert.ts"; import { describe, it } from "#veryfront/testing/bdd.ts"; -import { type RedisRateLimitOptions, RedisRateLimitStore } from "./rate-limit-store.ts"; +import { type RedisRateLimitOptions, RedisRateLimitStore } from "./index.ts"; async function outcomeWithin( promise: Promise, diff --git a/src/middleware/builtin/index.ts b/src/middleware/builtin/index.ts index bd56bc7682..5adf9277d2 100644 --- a/src/middleware/builtin/index.ts +++ b/src/middleware/builtin/index.ts @@ -25,6 +25,7 @@ export { rateLimit, type RateLimitOptions, } from "./security/rate-limit.ts"; +export { type RedisRateLimitOptions, RedisRateLimitStore } from "./security/redis-rate-limit.ts"; export type { RateLimitStore } from "./security/types.ts"; export { devLogger, type LogFormat, logger, type LoggerOptions, prodLogger } from "./logger.ts"; diff --git a/src/middleware/builtin/security/index.ts b/src/middleware/builtin/security/index.ts index fa1a9e4a6f..fa88c51e03 100644 --- a/src/middleware/builtin/security/index.ts +++ b/src/middleware/builtin/security/index.ts @@ -22,4 +22,5 @@ export { rateLimit, type RateLimitOptions, } from "./rate-limit.ts"; +export { type RedisRateLimitOptions, RedisRateLimitStore } from "./redis-rate-limit.ts"; export { securityHeaders } from "./security-headers.ts"; diff --git a/src/middleware/builtin/security/rate-limit.test.ts b/src/middleware/builtin/security/rate-limit.test.ts index aa171c84d1..654efa288f 100644 --- a/src/middleware/builtin/security/rate-limit.test.ts +++ b/src/middleware/builtin/security/rate-limit.test.ts @@ -10,7 +10,13 @@ import { delay } from "#std/async.ts"; import { scaleMs } from "#veryfront/testing/timing.ts"; import { deleteEnv, getHostEnv, setEnv } from "#veryfront/platform/compat/process.ts"; import { MiddlewareContext } from "../../core/context.ts"; -import { authRateLimit, MemoryRateLimitStore, rateLimit } from "./rate-limit.ts"; +import { + authRateLimit, + MemoryRateLimitStore, + rateLimit, + type RedisRateLimitOptions, + RedisRateLimitStore, +} from "#veryfront/middleware"; (globalThis as Record).__vfDisableLruInterval = true; @@ -283,7 +289,19 @@ describe("rateLimit middleware", () => { assertEquals(response?.status, 503); }); - it("should reject oversized custom keys before calling the store", async () => { + it("should keep the legacy Redis rate-limit store export constructible", () => { + const options: RedisRateLimitOptions = { + keyPrefix: "compat:", + connectTimeoutMs: 1_000, + operationTimeoutMs: 1_000, + }; + const redisStore = new RedisRateLimitStore(options); + + assertEquals(typeof redisStore.increment, "function"); + assertEquals(typeof redisStore.reset, "function"); + }); + + it("should fail closed when custom keys are invalid without calling the store", async () => { let incrementCalled = false; const middleware = rateLimit({ keyGenerator: () => "x".repeat(1025), @@ -296,16 +314,36 @@ describe("rateLimit middleware", () => { }, }); - await assertRejects( - async () => { - await middleware( - createContext(), - () => Promise.resolve(new Response("OK")), - ); + const response = await middleware( + createContext(), + () => Promise.resolve(new Response("OK")), + ); + + assertEquals(response?.status, 503); + assertEquals(response?.headers.get("Retry-After"), "60"); + assertEquals(incrementCalled, false); + }); + + it("should fail closed when trusted proxy headers generate invalid keys", async () => { + let incrementCalled = false; + const middleware = rateLimit({ + trustProxy: true, + store: { + increment: () => { + incrementCalled = true; + return Promise.resolve({ count: 1, resetAt: Date.now() + 1000 }); + }, + reset: () => Promise.resolve(), }, - RangeError, - "1024", + }); + + const response = await middleware( + createContext("x".repeat(1025)), + () => Promise.resolve(new Response("OK")), ); + + assertEquals(response?.status, 503); + assertEquals(response?.headers.get("Retry-After"), "60"); assertEquals(incrementCalled, false); }); diff --git a/src/middleware/builtin/security/rate-limit.ts b/src/middleware/builtin/security/rate-limit.ts index 714a382392..6a5e631a01 100644 --- a/src/middleware/builtin/security/rate-limit.ts +++ b/src/middleware/builtin/security/rate-limit.ts @@ -250,9 +250,9 @@ export function rateLimit( return async (ctx, next) => { const req = getRequest(ctx); - const key = requireRateLimitKey(keyGenerator(req)); let entry: RateLimitEntry; try { + const key = requireRateLimitKey(keyGenerator(req)); entry = requireRateLimitEntry(await store.increment(key, windowMs)); } catch (error) { const now = performance.now(); diff --git a/src/middleware/builtin/security/redis-rate-limit.ts b/src/middleware/builtin/security/redis-rate-limit.ts new file mode 100644 index 0000000000..17c0e48269 --- /dev/null +++ b/src/middleware/builtin/security/redis-rate-limit.ts @@ -0,0 +1,87 @@ +import type { RateLimitStore } from "./types.ts"; +import { requireRateLimitKey } from "./rate-limit-validation.ts"; + +const DEFAULT_REDIS_CONNECT_TIMEOUT_MS = 5_000; +const DEFAULT_REDIS_OPERATION_TIMEOUT_MS = 5_000; + +/** Options accepted by Redis rate limit. */ +export interface RedisRateLimitOptions { + url?: string; + keyPrefix?: string; + /** Maximum time allowed for loading and connecting the Redis client. */ + connectTimeoutMs?: number; + /** Maximum time allowed for an individual Redis command. */ + operationTimeoutMs?: number; +} + +type RedisRateLimitStoreDelegate = RateLimitStore & { + destroy?: () => void | Promise; +}; + +interface RedisRateLimitStoreModule { + RedisRateLimitStore: new ( + options?: RedisRateLimitOptions, + ) => RedisRateLimitStoreDelegate; +} + +/** Create a Redis rate limit store backed by @veryfront/ext-redis. */ +export class RedisRateLimitStore implements RateLimitStore { + private delegate: RedisRateLimitStoreDelegate | undefined; + private readonly options: RedisRateLimitOptions; + + constructor(options: RedisRateLimitOptions = {}) { + if ( + typeof options !== "object" || + options === null || + Array.isArray(options) + ) { + throw new TypeError("Redis rate limit options must be an object"); + } + if (options.url !== undefined && typeof options.url !== "string") { + throw new TypeError("Redis rate limit url must be a string"); + } + if (options.keyPrefix !== undefined) { + requireRateLimitKey(options.keyPrefix, "Redis rate limit keyPrefix"); + } + requireTimeoutMs( + options.connectTimeoutMs ?? DEFAULT_REDIS_CONNECT_TIMEOUT_MS, + "connectTimeoutMs", + ); + requireTimeoutMs( + options.operationTimeoutMs ?? DEFAULT_REDIS_OPERATION_TIMEOUT_MS, + "operationTimeoutMs", + ); + this.options = { ...options }; + } + + async increment(key: string, windowMs: number) { + return await (await this.getDelegate()).increment(key, windowMs); + } + + async reset(key: string): Promise { + await (await this.getDelegate()).reset(key); + } + + async destroy(): Promise { + await this.delegate?.destroy?.(); + this.delegate = undefined; + } + + private async getDelegate(): Promise { + if (this.delegate) return this.delegate; + const module = await import("@veryfront/ext-redis") as RedisRateLimitStoreModule; + this.delegate = new module.RedisRateLimitStore(this.options); + return this.delegate; + } +} + +function requireTimeoutMs(value: unknown, name: string): number { + if ( + typeof value !== "number" || + !Number.isSafeInteger(value) || + value <= 0 + ) { + throw new RangeError(`Redis rate limit ${name} must be a positive safe integer`); + } + return value; +} diff --git a/src/middleware/index.ts b/src/middleware/index.ts index 722a0f3833..6855a1abca 100644 --- a/src/middleware/index.ts +++ b/src/middleware/index.ts @@ -46,6 +46,10 @@ export { rateLimit, type RateLimitOptions, } from "./builtin/security/rate-limit.ts"; +export { + type RedisRateLimitOptions, + RedisRateLimitStore, +} from "./builtin/security/redis-rate-limit.ts"; export type { RateLimitStore } from "./builtin/security/types.ts"; export { From 4640c8bad1f58efeccba968018868e72e5edac1e Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 11:32:07 +0200 Subject: [PATCH 07/17] test(cli): isolate skills JSON subprocess output --- cli/commands/skills/handler.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/commands/skills/handler.test.ts b/cli/commands/skills/handler.test.ts index d317721c43..997f7968d4 100644 --- a/cli/commands/skills/handler.test.ts +++ b/cli/commands/skills/handler.test.ts @@ -9,7 +9,7 @@ describe("Skills Command", () => { ): Promise<{ code: number; stdout: string; stderr: string }> { const cliPath = new URL("../../main.ts", import.meta.url).pathname; const result = await new Deno.Command(Deno.execPath(), { - args: ["run", "-A", cliPath, "skills", "info", ...args, "--json"], + args: ["run", "--quiet", "-A", cliPath, "skills", "info", ...args, "--json"], env: { VERYFRONT_NO_UPDATE_CHECK: "1", NO_COLOR: "1" }, stdin: "null", stdout: "piped", From 4cc678fcadb234394d79beb0f99230d9ba0bacd8 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 11:34:44 +0200 Subject: [PATCH 08/17] Keep Redis rate limit usable from root npm The middleware compatibility export must work for existing consumers that install only the root veryfront package. The Redis implementation now stays in-tree for the public middleware API, while the extension package keeps its copy for extension-owned imports. Constraint: PR #3304 keeps the public veryfront/middleware RedisRateLimitStore export compatible Constraint: dnt cannot infer a dependency hidden behind an optional dynamic @veryfront/ext-redis import Rejected: Delegate root middleware to @veryfront/ext-redis | root npm consumers can call increment without installing that extension package Confidence: high Scope-risk: moderate Directive: Do not replace the root middleware Redis store with an opaque extension import unless the root npm package also proves increment works in the install smoke test Tested: focused middleware and ext-redis rate-limit tests Tested: npm package build and npm install smoke Tested: npm package metadata suite Tested: docs:validate Tested: verify:quick --- docs/api-reference/veryfront/agent.md | 2 +- docs/api-reference/veryfront/middleware.md | 4 +- .../ext-redis/src/rate-limit-store.test.ts | 18 + extensions/ext-redis/src/rate-limit-store.ts | 30 +- scripts/build/npm-package-metadata.test.ts | 16 +- scripts/build/npm-package-metadata.ts | 1 - scripts/lint/audit-core-deps.ts | 4 +- scripts/test/npm-install-smoke.sh | 47 +- .../builtin/security/rate-limit.test.ts | 71 +++ .../builtin/security/redis-rate-limit.ts | 422 ++++++++++++++++-- 10 files changed, 561 insertions(+), 54 deletions(-) diff --git a/docs/api-reference/veryfront/agent.md b/docs/api-reference/veryfront/agent.md index 75f329da12..69c8da3695 100644 --- a/docs/api-reference/veryfront/agent.md +++ b/docs/api-reference/veryfront/agent.md @@ -729,7 +729,7 @@ Input delivered to a hosted agent-service detached execution callback. | `createRuntimeAgentSystemMessages` | Create runtime agent system messages. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/agent/runtime/agent-definition.ts#L244) | | `createRuntimeLoadSkillTool` | Create runtime load skill tool. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/agent/runtime/load-skill-tool.ts#L1858) | | `createRuntimeProjectFilesClient` | Create runtime project files client. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/agent/runtime/project-files-client.ts#L312) | -| `createRuntimeProjectSkillLoader` | Create runtime project skill loader. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/agent/runtime/project-skill-loader.ts#L706) | +| `createRuntimeProjectSkillLoader` | Create runtime project skill loader. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/agent/runtime/project-skill-loader.ts#L685) | | `createRuntimePromptBlock` | Create runtime prompt block. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/agent/runtime/prompt-block.ts#L9) | | `createStreamedStepState` | State for create streamed step. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/agent/streaming/fork-runtime-step-state.ts#L43) | | `createToolExecutionDataEventBridgeStream` | Create tool execution data event bridge stream. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/agent/streaming/tool-execution-data-event-bridge.ts#L41) | diff --git a/docs/api-reference/veryfront/middleware.md b/docs/api-reference/veryfront/middleware.md index bb8c6c8e14..704abe690e 100644 --- a/docs/api-reference/veryfront/middleware.md +++ b/docs/api-reference/veryfront/middleware.md @@ -159,7 +159,7 @@ Options accepted by timeout. | `MemoryRateLimitStore` | Implement memory rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L37) | | `MiddlewareContext` | Context for middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/context.ts#L5) | | `MiddlewarePipeline` | Implement middleware pipeline. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/pipeline/pipeline.ts#L9) | -| `RedisRateLimitStore` | Create a Redis rate limit store backed by @veryfront/ext-redis. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/redis-rate-limit.ts#L28) | +| `RedisRateLimitStore` | Create a Redis rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/redis-rate-limit.ts#L55) | ### Types @@ -178,5 +178,5 @@ Options accepted by timeout. | `Next` | Public API contract for next. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/types.ts#L23) | | `RateLimitOptions` | Options accepted by rate limit. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L131) | | `RateLimitStore` | Public API contract for rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/types.ts#L32) | -| `RedisRateLimitOptions` | Options accepted by Redis rate limit. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/redis-rate-limit.ts#L8) | +| `RedisRateLimitOptions` | Options accepted by Redis rate limit. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/redis-rate-limit.ts#L45) | | `TimeoutOptions` | Options accepted by timeout. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/timeout.ts#L17) | diff --git a/extensions/ext-redis/src/rate-limit-store.test.ts b/extensions/ext-redis/src/rate-limit-store.test.ts index 01a0be6db9..4fcc3d0043 100644 --- a/extensions/ext-redis/src/rate-limit-store.test.ts +++ b/extensions/ext-redis/src/rate-limit-store.test.ts @@ -339,6 +339,24 @@ describe("middleware/builtin/security/redis-rate-limit", () => { assertEquals((rateStore as any).client, null); }); + it("should treat already-closed clients as destroyed", async () => { + const { rateStore, mockClient } = createStoreWithMock(); + let disconnectAttempts = 0; + mockClient.disconnect = () => { + disconnectAttempts++; + const error = new Error("The client is closed"); + error.name = "ClientClosedError"; + return Promise.reject(error); + }; + + await rateStore.destroy(); + await rateStore.destroy(); + + assertEquals(disconnectAttempts, 1); + // deno-lint-ignore no-explicit-any + assertEquals((rateStore as any).client, null); + }); + it("should retain a failed disconnect so shutdown can retry it", async () => { const { rateStore, mockClient } = createStoreWithMock(); let disconnectAttempts = 0; diff --git a/extensions/ext-redis/src/rate-limit-store.ts b/extensions/ext-redis/src/rate-limit-store.ts index 4458ddc3e0..e9b6e07f1f 100644 --- a/extensions/ext-redis/src/rate-limit-store.ts +++ b/extensions/ext-redis/src/rate-limit-store.ts @@ -157,6 +157,10 @@ export class RedisRateLimitStore implements RateLimitStore { try { await this.disconnectClient(client); } catch (error) { + if (isAlreadyClosedClientError(error)) { + this.markDisconnected(client); + return; + } logger.warn("client disconnect failed", { errorName: error instanceof Error ? error.name : typeof error, }); @@ -180,24 +184,38 @@ export class RedisRateLimitStore implements RateLimitStore { try { Promise.resolve(client.disconnect()).then( () => { - this.disconnectPromises.delete(client); - this.pendingDisconnectClients.delete(client); - this.disconnectedClients.add(client); + this.markDisconnected(client); resolveDisconnect(); }, (error) => { this.disconnectPromises.delete(client); + if (isAlreadyClosedClientError(error)) { + this.markDisconnected(client); + resolveDisconnect(); + return; + } rejectDisconnect(error); }, ); } catch (error) { this.disconnectPromises.delete(client); - rejectDisconnect(error); + if (isAlreadyClosedClientError(error)) { + this.markDisconnected(client); + resolveDisconnect(); + } else { + rejectDisconnect(error); + } } return pending; } + private markDisconnected(client: RedisClient): void { + this.disconnectPromises.delete(client); + this.pendingDisconnectClients.delete(client); + this.disconnectedClients.add(client); + } + private async withTimeout( operation: Promise, timeoutMs: number, @@ -391,6 +409,10 @@ function isTimeoutError(error: unknown): boolean { return error instanceof Error && error.name === "TimeoutError"; } +function isAlreadyClosedClientError(error: unknown): boolean { + return error instanceof Error && error.name === "ClientClosedError"; +} + function parseIncrementResult(result: unknown): [number, number] { if (!Array.isArray(result) || result.length < 2) { throw toError( diff --git a/scripts/build/npm-package-metadata.test.ts b/scripts/build/npm-package-metadata.test.ts index 02ec32f0c1..21703dcb98 100644 --- a/scripts/build/npm-package-metadata.test.ts +++ b/scripts/build/npm-package-metadata.test.ts @@ -331,6 +331,12 @@ const ROOT_BUNDLED_EXTENSIONS = new Set([ "ext-eval-report-mlflow", ]); +const ROOT_RUNTIME_EXTENSION_SHARED_DEPENDENCIES = new Set([ + "react", + "react-dom", + "redis", +]); + Deno.test("EXTENSION_OWNED_DEPENDENCIES stays in sync with extension manifests", async () => { const denoConfig = JSON.parse( await Deno.readTextFile("deno.json"), @@ -365,7 +371,9 @@ Deno.test("EXTENSION_OWNED_DEPENDENCIES stays in sync with extension manifests", for (const dependency of dependencies) { assertEquals( - owned.has(dependency) || optionalPeers.has(dependency), + owned.has(dependency) || + optionalPeers.has(dependency) || + ROOT_RUNTIME_EXTENSION_SHARED_DEPENDENCIES.has(dependency), true, `${dependency} (declared by ${manifestPath}) must be added to EXTENSION_OWNED_DEPENDENCIES so it does not leak into root veryfront npm installs`, ); @@ -446,7 +454,7 @@ describe("normalizeNpmPackageMetadata", () => { }); }); - it("keeps first-party extension implementation packages out of root npm metadata", () => { + it("keeps extension-only packages out while retaining root middleware Redis support", () => { const pkg = normalizeNpmPackageMetadata({ dependencies: { "@babel/parser": "^7.29.2", @@ -461,7 +469,7 @@ describe("normalizeNpmPackageMetadata", () => { }, }); - assertEquals(pkg.dependencies, { zod: "4.3.6" }); + assertEquals(pkg.dependencies, { redis: "5.11.0", zod: "4.3.6" }); assertEquals(pkg.peerDependencies, { "@huggingface/transformers": "^4.2.0", }); @@ -616,6 +624,8 @@ describe("npm supply-chain policy", () => { } assertStringIncludes(source, "CodeParser was not registered"); + assertStringIncludes(source, "RedisRateLimitStore"); + assertStringIncludes(source, "await store.increment('user-1', 30000)"); assertStringIncludes(source, "getDeferredExtensionState(resolved)"); assertStringIncludes(source, "await deferred.load(logger)"); assertStringIncludes(source, "app/page.tsx"); diff --git a/scripts/build/npm-package-metadata.ts b/scripts/build/npm-package-metadata.ts index abe719637a..90d65863ba 100644 --- a/scripts/build/npm-package-metadata.ts +++ b/scripts/build/npm-package-metadata.ts @@ -58,7 +58,6 @@ export const EXTENSION_OWNED_DEPENDENCIES = [ "@opentelemetry/sdk-trace-base", "@opentelemetry/semantic-conventions", "@redis/client", - "redis", "@sentry/deno", "@sentry/node", "@tailwindcss/forms", diff --git a/scripts/lint/audit-core-deps.ts b/scripts/lint/audit-core-deps.ts index f4466620a1..b449ee4b00 100644 --- a/scripts/lint/audit-core-deps.ts +++ b/scripts/lint/audit-core-deps.ts @@ -18,7 +18,9 @@ export interface RootNpmSpecifierLiteralIssue { value: string; } -const CORE_THIRD_PARTY_IMPORT_ALLOWLIST = new Set(); +const CORE_THIRD_PARTY_IMPORT_ALLOWLIST = new Set([ + "npm:redis@5.11.0", +]); function isThirdPartyImportTarget(target: string): boolean { if (target.startsWith("./") || target.startsWith("../")) return false; diff --git a/scripts/test/npm-install-smoke.sh b/scripts/test/npm-install-smoke.sh index c4b4fe1238..36d454fdf5 100755 --- a/scripts/test/npm-install-smoke.sh +++ b/scripts/test/npm-install-smoke.sh @@ -5,10 +5,11 @@ # throwaway npm project, that: # 1. a `veryfront` install with co-published required packages runs the CLI # and activates the parser extension under Node -# 2. the @huggingface/transformers optional peer is declared -# 3. loading a missing extension fails naming the installable package -# 4. installing @veryfront/ext-auth-jwt makes the extension load -# 5. a broken transitive dependency surfaces the real error, not a +# 2. the public middleware Redis rate limit store increments under Node +# 3. the @huggingface/transformers optional peer is declared +# 4. loading a missing extension fails naming the installable package +# 5. installing @veryfront/ext-auth-jwt makes the extension load +# 6. a broken transitive dependency surfaces the real error, not a # misleading "extension not installed" skip # # Requires: `deno task build:npm` output in ./npm, node + npm on PATH. @@ -87,14 +88,44 @@ if (ast?.type !== 'File') throw new Error('TSX parse failed'); await extension.teardown?.(); " || fail "root optional builtin did not register a working CodeParser" -echo "== 2. root install: transformers optional peer declared" +echo "== 2. root install: middleware Redis rate-limit store increments" +node --input-type=module -e " +const { RedisRateLimitStore } = await import('veryfront/middleware'); +let evalCalls = 0; +let disconnectCalls = 0; +const client = { + connect: () => Promise.resolve(), + disconnect: () => { + disconnectCalls++; + return Promise.resolve(); + }, + eval: (_script, options) => { + evalCalls++; + if (options.keys[0] !== 'smoke:user-1') throw new Error('unexpected Redis key'); + if (options.arguments[0] !== '30000') throw new Error('unexpected Redis window'); + return Promise.resolve([1, 30000]); + }, + del: () => Promise.resolve(1), + on: () => {}, +}; +const store = new RedisRateLimitStore({ keyPrefix: 'smoke:' }); +store.loadClientFactory = () => Promise.resolve(() => client); +const entry = await store.increment('user-1', 30000); +await store.destroy(); +if (entry.count !== 1) throw new Error('RedisRateLimitStore did not increment'); +if (entry.resetAt <= Date.now()) throw new Error('RedisRateLimitStore resetAt is not future'); +if (evalCalls !== 1) throw new Error('Redis eval was not called exactly once'); +if (disconnectCalls !== 1) throw new Error('Redis client was not disconnected'); +" || fail "root middleware RedisRateLimitStore did not increment from npm package" + +echo "== 3. root install: transformers optional peer declared" node -e " const p = require('./node_modules/veryfront/package.json'); if (!p.peerDependencies?.['@huggingface/transformers']) process.exit(1); if (p.peerDependenciesMeta?.['@huggingface/transformers']?.optional !== true) process.exit(1); " || fail "@huggingface/transformers optional peer missing from root package.json" -echo "== 3. root install: missing extension failure names the installable package" +echo "== 4. root install: missing extension failure names the installable package" set +e MISSING_OUTPUT="$(node -e " import('./node_modules/veryfront/esm/src/extensions/first-party-import.js').then(async (m) => { @@ -108,7 +139,7 @@ set -e echo "$MISSING_OUTPUT" | grep -q "install @veryfront/ext-auth-jwt alongside veryfront" || fail "missing-extension error lacks the install hint: $MISSING_OUTPUT" -echo "== 4. with @veryfront/ext-auth-jwt installed: extension loads" +echo "== 5. with @veryfront/ext-auth-jwt installed: extension loads" npm install --no-fund --no-audit --silent --ignore-scripts ./veryfront-ext-auth-jwt-*.tgz node -e " import('./node_modules/veryfront/esm/src/extensions/first-party-import.js').then(async (m) => { @@ -117,7 +148,7 @@ import('./node_modules/veryfront/esm/src/extensions/first-party-import.js').then }); " || fail "ext-auth-jwt did not load after installing @veryfront/ext-auth-jwt" -echo "== 5. broken transitive dependency surfaces the real error" +echo "== 6. broken transitive dependency surfaces the real error" mv node_modules/jose node_modules/jose.smoke-removed set +e BROKEN_OUTPUT="$(node -e " diff --git a/src/middleware/builtin/security/rate-limit.test.ts b/src/middleware/builtin/security/rate-limit.test.ts index 654efa288f..36ea7c6398 100644 --- a/src/middleware/builtin/security/rate-limit.test.ts +++ b/src/middleware/builtin/security/rate-limit.test.ts @@ -20,6 +20,44 @@ import { (globalThis as Record).__vfDisableLruInterval = true; +function createMockRedisClient(): { + connect: () => Promise; + disconnect: () => Promise; + eval: ( + script: string, + options: { keys: string[]; arguments: string[] }, + ) => Promise<[number, number]>; + del: (key: string) => Promise; + on: (event: string, listener: (...args: unknown[]) => void) => void; + _evalCalls: number; + _disconnectCalls: number; +} { + let evalCalls = 0; + let disconnectCalls = 0; + + return { + connect: () => Promise.resolve(), + disconnect: () => { + disconnectCalls += 1; + return Promise.resolve(); + }, + eval: (_script: string, options: { keys: string[]; arguments: string[] }) => { + evalCalls += 1; + assertEquals(options.keys, ["compat:user-1"]); + assertEquals(options.arguments, ["30000"]); + return Promise.resolve([1, 30000]); + }, + del: () => Promise.resolve(1), + on: () => {}, + get _evalCalls() { + return evalCalls; + }, + get _disconnectCalls() { + return disconnectCalls; + }, + }; +} + describe("MemoryRateLimitStore", () => { let store: MemoryRateLimitStore; @@ -301,6 +339,39 @@ describe("rateLimit middleware", () => { assertEquals(typeof redisStore.reset, "function"); }); + it("should exercise the legacy Redis rate-limit store export without ext-redis", async () => { + const redisStore = new RedisRateLimitStore({ keyPrefix: "compat:" }); + const mockClient = createMockRedisClient(); + (redisStore as unknown as { + loadClientFactory: () => Promise<() => typeof mockClient>; + }).loadClientFactory = () => Promise.resolve(() => mockClient); + + const entry = await redisStore.increment("user-1", 30000); + await redisStore.destroy(); + + assertEquals(entry.count, 1); + assertEquals(entry.resetAt > Date.now(), true); + assertEquals(mockClient._evalCalls, 1); + assertEquals(mockClient._disconnectCalls, 1); + }); + + it("should ignore already-closed Redis clients during legacy store destroy", async () => { + const redisStore = new RedisRateLimitStore({ keyPrefix: "compat:" }); + const mockClient = createMockRedisClient(); + mockClient.disconnect = () => { + const error = new Error("The client is closed"); + error.name = "ClientClosedError"; + return Promise.reject(error); + }; + (redisStore as unknown as { + loadClientFactory: () => Promise<() => typeof mockClient>; + }).loadClientFactory = () => Promise.resolve(() => mockClient); + + await redisStore.increment("user-1", 30000); + await redisStore.destroy(); + await redisStore.destroy(); + }); + it("should fail closed when custom keys are invalid without calling the store", async () => { let incrementCalled = false; const middleware = rateLimit({ diff --git a/src/middleware/builtin/security/redis-rate-limit.ts b/src/middleware/builtin/security/redis-rate-limit.ts index 17c0e48269..13e7255093 100644 --- a/src/middleware/builtin/security/redis-rate-limit.ts +++ b/src/middleware/builtin/security/redis-rate-limit.ts @@ -1,9 +1,46 @@ -import type { RateLimitStore } from "./types.ts"; -import { requireRateLimitKey } from "./rate-limit-validation.ts"; +import { createError, toError } from "../../../errors/index.ts"; +import { serverLogger } from "../../../utils/logger/index.ts"; +import { MAX_TIMER_DELAY_MS } from "../../../utils/timer.ts"; +import { createClient } from "npm:redis@5.11.0"; +import { requireRateLimitKey, requireRateLimitWindowMs } from "./rate-limit-validation.ts"; +import type { RateLimitEntry, RateLimitStore } from "./types.ts"; + +const logger = serverLogger.component("redis-ratelimit"); + +interface RedisClient { + connect(): Promise; + disconnect(): Promise; + eval( + script: string, + options: { keys: string[]; arguments: string[] }, + ): Promise; + del(key: string): Promise; + on?(event: string, listener: (...args: unknown[]) => void): void; +} + +interface RedisClientFactoryOptions { + url?: string; + socket: { + connectTimeout: number; + reconnectStrategy: false; + }; +} + +type RedisClientFactory = (options: RedisClientFactoryOptions) => RedisClient; const DEFAULT_REDIS_CONNECT_TIMEOUT_MS = 5_000; const DEFAULT_REDIS_OPERATION_TIMEOUT_MS = 5_000; +const INCREMENT_WITH_TTL_SCRIPT = ` +local count = redis.call("INCR", KEYS[1]) +local ttl = redis.call("PTTL", KEYS[1]) +if ttl < 0 then + redis.call("PEXPIRE", KEYS[1], ARGV[1]) + ttl = tonumber(ARGV[1]) +end +return { count, ttl } +`; + /** Options accepted by Redis rate limit. */ export interface RedisRateLimitOptions { url?: string; @@ -14,20 +51,21 @@ export interface RedisRateLimitOptions { operationTimeoutMs?: number; } -type RedisRateLimitStoreDelegate = RateLimitStore & { - destroy?: () => void | Promise; -}; - -interface RedisRateLimitStoreModule { - RedisRateLimitStore: new ( - options?: RedisRateLimitOptions, - ) => RedisRateLimitStoreDelegate; -} - -/** Create a Redis rate limit store backed by @veryfront/ext-redis. */ +/** Create a Redis rate limit store. */ export class RedisRateLimitStore implements RateLimitStore { - private delegate: RedisRateLimitStoreDelegate | undefined; - private readonly options: RedisRateLimitOptions; + private client: RedisClient | null = null; + private connectingClient: RedisClient | null = null; + private clientPromise: Promise | null = null; + private cancelPendingConnection: (() => void) | null = null; + private clientGeneration = 0; + private readonly disconnectPromises = new WeakMap>(); + private readonly disconnectedClients = new WeakSet(); + private readonly pendingDisconnectClients = new Set(); + private readonly reportedClientErrors = new WeakSet(); + private readonly url?: string; + private readonly keyPrefix: string; + private readonly connectTimeoutMs: number; + private readonly operationTimeoutMs: number; constructor(options: RedisRateLimitOptions = {}) { if ( @@ -40,38 +78,301 @@ export class RedisRateLimitStore implements RateLimitStore { if (options.url !== undefined && typeof options.url !== "string") { throw new TypeError("Redis rate limit url must be a string"); } - if (options.keyPrefix !== undefined) { - requireRateLimitKey(options.keyPrefix, "Redis rate limit keyPrefix"); - } - requireTimeoutMs( + this.url = options.url; + this.connectTimeoutMs = requireTimeoutMs( options.connectTimeoutMs ?? DEFAULT_REDIS_CONNECT_TIMEOUT_MS, "connectTimeoutMs", ); - requireTimeoutMs( + this.operationTimeoutMs = requireTimeoutMs( options.operationTimeoutMs ?? DEFAULT_REDIS_OPERATION_TIMEOUT_MS, "operationTimeoutMs", ); - this.options = { ...options }; + this.keyPrefix = requireRateLimitKey( + options.keyPrefix ?? "veryfront:ratelimit:", + "Redis rate limit keyPrefix", + ); + } + + private ensureClient(): Promise { + if (this.client) return Promise.resolve(this.client); + if (this.clientPromise) return this.clientPromise; + + const generation = this.clientGeneration; + const pending = this.connectClient(generation).finally(() => { + if (this.clientPromise === pending) this.clientPromise = null; + }); + this.clientPromise = pending; + return pending; } - async increment(key: string, windowMs: number) { - return await (await this.getDelegate()).increment(key, windowMs); + private invalidateClient( + client: RedisClient, + generation: number, + disconnect: boolean, + ): void { + if (generation !== this.clientGeneration) return; + if (this.client !== client) return; + + this.clientGeneration++; + this.client = null; + this.clientPromise = null; + + if (disconnect) { + void this.disconnectBestEffort(client); + } + } + + private attachClientLifecycleHandlers( + client: RedisClient, + generation = this.clientGeneration, + ): void { + client.on?.("error", (err: unknown) => { + if (generation !== this.clientGeneration) return; + if (!this.reportedClientErrors.has(client)) { + this.reportedClientErrors.add(client); + logger.error("client error", { + errorName: err instanceof Error ? err.name : typeof err, + }); + } + this.invalidateClient(client, generation, true); + }); + + client.on?.("end", () => { + this.invalidateClient(client, generation, false); + }); + } + + private async loadClientFactory(): Promise { + return createClient as unknown as RedisClientFactory; + } + + private async disconnectBestEffort(client: RedisClient): Promise { + try { + await this.disconnectClient(client); + } catch (error) { + if (isAlreadyClosedClientError(error)) { + this.markDisconnected(client); + return; + } + logger.warn("client disconnect failed", { + errorName: error instanceof Error ? error.name : typeof error, + }); + } + } + + private disconnectClient(client: RedisClient): Promise { + if (this.disconnectedClients.has(client)) return Promise.resolve(); + const existing = this.disconnectPromises.get(client); + if (existing) return existing; + + let resolveDisconnect!: () => void; + let rejectDisconnect!: (reason: unknown) => void; + const pending = new Promise((resolve, reject) => { + resolveDisconnect = resolve; + rejectDisconnect = reject; + }); + this.disconnectPromises.set(client, pending); + this.pendingDisconnectClients.add(client); + + try { + Promise.resolve(client.disconnect()).then( + () => { + this.markDisconnected(client); + resolveDisconnect(); + }, + (error) => { + this.disconnectPromises.delete(client); + if (isAlreadyClosedClientError(error)) { + this.markDisconnected(client); + resolveDisconnect(); + return; + } + rejectDisconnect(error); + }, + ); + } catch (error) { + this.disconnectPromises.delete(client); + if (isAlreadyClosedClientError(error)) { + this.markDisconnected(client); + resolveDisconnect(); + } else { + rejectDisconnect(error); + } + } + + return pending; + } + + private markDisconnected(client: RedisClient): void { + this.disconnectPromises.delete(client); + this.pendingDisconnectClients.delete(client); + this.disconnectedClients.add(client); + } + + private async withTimeout( + operation: Promise, + timeoutMs: number, + operationName: string, + cancellation?: Promise, + ): Promise { + let timeoutId: ReturnType | undefined; + const timeout = new Promise((_, reject) => { + timeoutId = setTimeout(() => { + reject(createTimeoutError(operationName, timeoutMs)); + }, timeoutMs); + }); + + try { + return await Promise.race( + cancellation === undefined ? [operation, timeout] : [operation, timeout, cancellation], + ); + } finally { + if (timeoutId !== undefined) clearTimeout(timeoutId); + } + } + + private async connectClient(generation: number): Promise { + const superseded = new Error( + "Redis rate limit client connection was superseded", + ); + superseded.name = "AbortError"; + let cancelConnection!: () => void; + let cancelled = false; + const cancellation = new Promise((_, reject) => { + cancelConnection = () => { + if (cancelled) return; + cancelled = true; + reject(superseded); + }; + }); + this.cancelPendingConnection = cancelConnection; + + let client: RedisClient | undefined; + try { + const createClient = await this.withTimeout( + this.loadClientFactory(), + this.connectTimeoutMs, + "client loading", + cancellation, + ); + if (generation !== this.clientGeneration) throw superseded; + + client = createClient({ + ...(this.url === undefined ? {} : { url: this.url }), + socket: { + connectTimeout: this.connectTimeoutMs, + reconnectStrategy: false, + }, + }); + this.connectingClient = client; + this.attachClientLifecycleHandlers(client, generation); + await this.withTimeout( + client.connect(), + this.connectTimeoutMs, + "connection", + cancellation, + ); + + if (this.connectingClient === client) this.connectingClient = null; + if (generation !== this.clientGeneration) { + await this.disconnectBestEffort(client); + throw superseded; + } + + this.client = client; + return client; + } catch (error) { + if (client && this.connectingClient === client) { + this.connectingClient = null; + } + if (generation === this.clientGeneration) this.clientGeneration++; + if (client) await this.disconnectBestEffort(client); + throw error; + } finally { + if (this.cancelPendingConnection === cancelConnection) { + this.cancelPendingConnection = null; + } + } + } + + private storageKey(key: string): string { + return `${this.keyPrefix}${key}`; + } + + async increment(key: string, windowMs: number): Promise { + const normalizedKey = requireRateLimitKey(key); + const normalizedWindowMs = requireRateLimitWindowMs(windowMs); + const client = await this.ensureClient(); + const generation = this.clientGeneration; + const redisKey = this.storageKey(normalizedKey); + + let result: unknown; + try { + result = await this.withTimeout( + client.eval(INCREMENT_WITH_TTL_SCRIPT, { + keys: [redisKey], + arguments: [String(normalizedWindowMs)], + }), + this.operationTimeoutMs, + "increment", + ); + } catch (error) { + if (isTimeoutError(error)) { + this.invalidateClient(client, generation, true); + } + throw error; + } + + const [count, pttl] = parseIncrementResult(result); + const ttl = pttl > 0 ? requireRateLimitWindowMs(pttl) : normalizedWindowMs; + return { count, resetAt: Date.now() + ttl }; } async reset(key: string): Promise { - await (await this.getDelegate()).reset(key); + const client = await this.ensureClient(); + const generation = this.clientGeneration; + try { + await this.withTimeout( + client.del(this.storageKey(requireRateLimitKey(key))), + this.operationTimeoutMs, + "reset", + ); + } catch (error) { + if (isTimeoutError(error)) { + this.invalidateClient(client, generation, true); + } + throw error; + } } async destroy(): Promise { - await this.delegate?.destroy?.(); - this.delegate = undefined; - } + const client = this.client; + const connectingClient = this.connectingClient; + const pending = this.clientPromise; + const cancelPendingConnection = this.cancelPendingConnection; + const clientsToDisconnect = new Set(this.pendingDisconnectClients); + if (client) clientsToDisconnect.add(client); + if (connectingClient) clientsToDisconnect.add(connectingClient); + this.clientGeneration++; + this.client = null; + this.connectingClient = null; + this.clientPromise = null; + this.cancelPendingConnection = null; + cancelPendingConnection?.(); + + let disconnectFailed = false; + let disconnectError: unknown; + await Promise.all( + [...clientsToDisconnect].map((clientToDisconnect) => + this.disconnectClient(clientToDisconnect).catch((error: unknown) => { + disconnectFailed = true; + disconnectError ??= error; + }) + ), + ); + pending?.catch(() => {}); - private async getDelegate(): Promise { - if (this.delegate) return this.delegate; - const module = await import("@veryfront/ext-redis") as RedisRateLimitStoreModule; - this.delegate = new module.RedisRateLimitStore(this.options); - return this.delegate; + if (disconnectFailed) throw disconnectError; } } @@ -79,9 +380,62 @@ function requireTimeoutMs(value: unknown, name: string): number { if ( typeof value !== "number" || !Number.isSafeInteger(value) || - value <= 0 + value <= 0 || + value > MAX_TIMER_DELAY_MS ) { - throw new RangeError(`Redis rate limit ${name} must be a positive safe integer`); + throw new RangeError( + `Redis rate limit ${name} must be an integer between 1 and ${MAX_TIMER_DELAY_MS}`, + ); } return value; } + +function createTimeoutError(operationName: string, timeoutMs: number): Error { + const error = new Error( + `Redis rate limit ${operationName} timed out after ${timeoutMs}ms`, + ); + error.name = "TimeoutError"; + return error; +} + +function isTimeoutError(error: unknown): boolean { + return error instanceof Error && error.name === "TimeoutError"; +} + +function isAlreadyClosedClientError(error: unknown): boolean { + return error instanceof Error && error.name === "ClientClosedError"; +} + +function parseIncrementResult(result: unknown): [number, number] { + if (!Array.isArray(result) || result.length < 2) { + throw toError( + createError({ + type: "config", + message: "Redis rate limit eval returned an invalid result.", + }), + ); + } + + const count = Number(result[0]); + const ttl = Number(result[1]); + + if (!Number.isSafeInteger(count) || count < 1) { + throw toError( + createError({ + type: "config", + message: "Redis rate limit eval returned an invalid count.", + }), + ); + } + + if (!Number.isSafeInteger(ttl)) { + throw toError( + createError({ + type: "config", + message: "Redis rate limit eval returned an invalid TTL.", + }), + ); + } + + return [count, ttl]; +} From 439cc672f5de1d7834c3c6deb7de23f35f102144 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 12:01:27 +0200 Subject: [PATCH 09/17] Address rate-limit review safety gaps CodeRabbit flagged six merge-readiness gaps in the rate-limit consolidation: Redis closed-client recognition, memory store capacity observability, key-resolution logging, lazy Redis loading, reset validation order, and pending connection rejection handling. This change fixes those contracts directly in the core store, middleware, and Redis extension, then locks each path with focused regressions. Constraint: Core keeps redis as a narrowly allowlisted server-only runtime dependency. Rejected: Plain error-name matching for ClientClosedError | redis 5.11.0 sets name to Error, so instanceof is required. Rejected: Logging all failures as store outages | key generation and capacity exhaustion need distinct operational signals. Confidence: high Scope-risk: moderate Directive: Keep Redis loading lazy in the core compatibility store so importing middleware does not eagerly resolve npm:redis. Tested: deno test --no-check --allow-all src/middleware/builtin/security/rate-limit.test.ts Tested: deno test --no-check --allow-all extensions/ext-redis/src/rate-limit-store.test.ts Tested: deno test --config=scripts/test.deno.json --no-check --allow-read --allow-write --allow-run scripts/lint/audit-core-deps.test.ts Tested: deno fmt --check targeted TypeScript files Tested: deno lint targeted TypeScript files Tested: deno check targeted TypeScript files Tested: deno task lint:core-deps Tested: git diff --check Not-tested: Full repository test suite --- docs/api-reference/veryfront/middleware.md | 8 + .../ext-redis/src/rate-limit-store.test.ts | 77 +++++++- extensions/ext-redis/src/rate-limit-store.ts | 10 +- scripts/lint/audit-core-deps.test.ts | 45 ++--- .../builtin/security/rate-limit.test.ts | 178 +++++++++++++++++- src/middleware/builtin/security/rate-limit.ts | 66 ++++++- .../builtin/security/redis-rate-limit.ts | 24 ++- 7 files changed, 357 insertions(+), 51 deletions(-) diff --git a/docs/api-reference/veryfront/middleware.md b/docs/api-reference/veryfront/middleware.md index 704abe690e..0faca2d858 100644 --- a/docs/api-reference/veryfront/middleware.md +++ b/docs/api-reference/veryfront/middleware.md @@ -116,6 +116,14 @@ Options accepted by rate limit. | `keyGenerator?` | (req: Request) => string | Function to derive rate limit key from request | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L135) | | `trustProxy?` | `boolean` | Trust proxy-set forwarding headers (X-Forwarded-For) for keying. Defaults to false so forwarded headers are ignored and cannot be used to evade limits. Enable only when a trusted proxy that appends the real client IP sits in front of this middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L142) | +### `MemoryRateLimitStoreOptions` + +Options accepted by the in-memory rate limit store. + +| Property | Type | Description | Source | +|----------|------|-------------|--------| +| `maxEntries?` | `number` | Maximum number of active identities retained by the store. Size this above the expected concurrent identities in one rate-limit window. New identities fail closed when all entries are active; existing identities remain tracked until their windows expire. Capacity exhaustion emits a `store-capacity` log stage with the configured `maxEntries`. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L137) | + ### `LoggerOptions` Options accepted by logger. diff --git a/extensions/ext-redis/src/rate-limit-store.test.ts b/extensions/ext-redis/src/rate-limit-store.test.ts index 4fcc3d0043..190a17a3aa 100644 --- a/extensions/ext-redis/src/rate-limit-store.test.ts +++ b/extensions/ext-redis/src/rate-limit-store.test.ts @@ -1,6 +1,7 @@ import "#veryfront/schemas/_test-setup.ts"; import { assertEquals, assertRejects, assertThrows } from "#veryfront/testing/assert.ts"; import { describe, it } from "#veryfront/testing/bdd.ts"; +import { ClientClosedError } from "redis"; import { type RedisRateLimitOptions, RedisRateLimitStore } from "./index.ts"; async function outcomeWithin( @@ -344,9 +345,7 @@ describe("middleware/builtin/security/redis-rate-limit", () => { let disconnectAttempts = 0; mockClient.disconnect = () => { disconnectAttempts++; - const error = new Error("The client is closed"); - error.name = "ClientClosedError"; - return Promise.reject(error); + return Promise.reject(new ClientClosedError()); }; await rateStore.destroy(); @@ -378,6 +377,24 @@ describe("middleware/builtin/security/redis-rate-limit", () => { }); }); + describe("reset", () => { + it("should reject invalid keys before connecting", async () => { + const rateStore = new RedisRateLimitStore(); + const mockClient = createMockRedisClient(); + + // deno-lint-ignore no-explicit-any + (rateStore as any).loadClientFactory = () => Promise.resolve(() => mockClient); + + await assertRejects( + () => rateStore.reset("x".repeat(1025)), + RangeError, + "1024", + ); + + assertEquals(mockClient._connectCalls, 0); + }); + }); + describe("ensureClient", () => { it("should reuse existing client", async () => { const { rateStore, mockClient } = createStoreWithMock(); @@ -582,6 +599,60 @@ describe("middleware/builtin/security/redis-rate-limit", () => { const outcome = await outcomeWithin(incrementPromise, 50); assertEquals(outcome, "rejected"); }); + + it("should attach pending rejection handling before destroy cancels it", async () => { + const rateStore = new RedisRateLimitStore(); + const mockClient = createMockRedisClient(); + let connectStarted = false; + mockClient.connect = () => { + connectStarted = true; + return new Promise(() => {}); + }; + + // deno-lint-ignore no-explicit-any + (rateStore as any).loadClientFactory = () => Promise.resolve(() => mockClient); + + const incrementPromise = rateStore.increment("pending", 1000); + for (let attempt = 0; attempt < 10 && !connectStarted; attempt++) { + await Promise.resolve(); + } + + // deno-lint-ignore no-explicit-any + const pending = (rateStore as any).clientPromise as Promise | null; + let cancelObserved = false; + let catchAttachedBeforeCancel = false; + if (pending) { + const originalCatch = pending.catch.bind(pending); + Object.defineProperty(pending, "catch", { + configurable: true, + value: (...args: Parameters["catch"]>) => { + if (!cancelObserved) catchAttachedBeforeCancel = true; + return originalCatch(...args); + }, + }); + } + // deno-lint-ignore no-explicit-any + const originalCancel = (rateStore as any).cancelPendingConnection as + | (() => void) + | null; + // deno-lint-ignore no-explicit-any + (rateStore as any).cancelPendingConnection = () => { + cancelObserved = true; + originalCancel?.(); + }; + + await rateStore.destroy(); + + if (!pending) throw new Error("Expected pending connection promise"); + assertEquals(catchAttachedBeforeCancel, true); + const pendingOutcome = await outcomeWithin(pending, 50); + await assertRejects( + () => incrementPromise, + Error, + "superseded", + ); + assertEquals(pendingOutcome, "rejected"); + }); }); }); }); diff --git a/extensions/ext-redis/src/rate-limit-store.ts b/extensions/ext-redis/src/rate-limit-store.ts index e9b6e07f1f..c956746bc9 100644 --- a/extensions/ext-redis/src/rate-limit-store.ts +++ b/extensions/ext-redis/src/rate-limit-store.ts @@ -1,6 +1,6 @@ import { createError, toError } from "veryfront/errors"; import { serverLogger } from "veryfront/utils/logger"; -import { createClient } from "redis"; +import { ClientClosedError, createClient } from "redis"; import { MAX_TIMER_DELAY_MS, type RateLimitEntry, @@ -336,11 +336,12 @@ export class RedisRateLimitStore implements RateLimitStore { } async reset(key: string): Promise { + const normalizedKey = requireRateLimitKey(key); const client = await this.ensureClient(); const generation = this.clientGeneration; try { await this.withTimeout( - client.del(this.storageKey(requireRateLimitKey(key))), + client.del(this.storageKey(normalizedKey)), this.operationTimeoutMs, "reset", ); @@ -356,6 +357,7 @@ export class RedisRateLimitStore implements RateLimitStore { const client = this.client; const connectingClient = this.connectingClient; const pending = this.clientPromise; + pending?.catch(() => {}); const cancelPendingConnection = this.cancelPendingConnection; const clientsToDisconnect = new Set(this.pendingDisconnectClients); if (client) clientsToDisconnect.add(client); @@ -377,8 +379,6 @@ export class RedisRateLimitStore implements RateLimitStore { }) ), ); - pending?.catch(() => {}); - if (disconnectFailed) throw disconnectError; } } @@ -410,7 +410,7 @@ function isTimeoutError(error: unknown): boolean { } function isAlreadyClosedClientError(error: unknown): boolean { - return error instanceof Error && error.name === "ClientClosedError"; + return error instanceof ClientClosedError; } function parseIncrementResult(result: unknown): [number, number] { diff --git a/scripts/lint/audit-core-deps.test.ts b/scripts/lint/audit-core-deps.test.ts index b26d2a6851..90664fe8a2 100644 --- a/scripts/lint/audit-core-deps.test.ts +++ b/scripts/lint/audit-core-deps.test.ts @@ -243,11 +243,6 @@ describe("findCoreThirdPartySourceImports", () => { line: 5, specifier: "npm:@redis/client@1.5.8", }, - { - path: "src/cache/hidden-imports.ts", - line: 6, - specifier: "npm:redis@5.11.0", - }, { path: "src/cache/hidden-imports.ts", line: 7, @@ -261,7 +256,7 @@ describe("findCoreThirdPartySourceImports", () => { { path: "src/cache/scoped-imports.ts", content: [ - 'const dependency = "npm:redis@5.11.0";', + 'const dependency = "npm:ioredis@5.8.2";', "{", ' const dependency = "./local.ts";', " await import(dependency);", @@ -275,7 +270,7 @@ describe("findCoreThirdPartySourceImports", () => { { path: "src/cache/scoped-imports.ts", line: 6, - specifier: "npm:redis@5.11.0", + specifier: "npm:ioredis@5.8.2", }, ]); }); @@ -285,7 +280,7 @@ describe("findCoreThirdPartySourceImports", () => { { path: "src/cache/default-parameter.ts", content: [ - 'const dependency = "npm:redis@5.11.0";', + 'const dependency = "npm:ioredis@5.8.2";', "function load(value = dependency) {", " return import(dependency);", "}", @@ -294,7 +289,7 @@ describe("findCoreThirdPartySourceImports", () => { { path: "src/cache/destructured-parameter.ts", content: [ - 'const dependency = "npm:redis@5.11.0";', + 'const dependency = "npm:ioredis@5.8.2";', "function load({ dependency: local }) {", " return import(dependency);", "}", @@ -303,7 +298,7 @@ describe("findCoreThirdPartySourceImports", () => { { path: "src/cache/loop-scope.ts", content: [ - 'const dependency = "npm:redis@5.11.0";', + 'const dependency = "npm:ioredis@5.8.2";', 'for (const dependency of ["./local.ts"]) {', " await import(dependency);", "}", @@ -313,7 +308,7 @@ describe("findCoreThirdPartySourceImports", () => { { path: "src/cache/var-scope.ts", content: [ - 'const dependency = "npm:redis@5.11.0";', + 'const dependency = "npm:ioredis@5.8.2";', "function load() {", " { var dependency = './local.ts'; }", " return import(dependency);", @@ -323,7 +318,7 @@ describe("findCoreThirdPartySourceImports", () => { { path: "src/cache/parameter-var-scope.ts", content: [ - 'const dependency = "npm:redis@5.11.0";', + 'const dependency = "npm:ioredis@5.8.2";', "function load(value = import(dependency)) {", " var dependency = './local.ts';", "}", @@ -332,7 +327,7 @@ describe("findCoreThirdPartySourceImports", () => { { path: "src/cache/static-block-scope.ts", content: [ - 'const dependency = "npm:redis@5.11.0";', + 'const dependency = "npm:ioredis@5.8.2";', "class Cache {", " static {", " var dependency = './local.ts';", @@ -345,7 +340,7 @@ describe("findCoreThirdPartySourceImports", () => { { path: "src/cache/named-class-expression.ts", content: [ - 'const dependency = "npm:redis@5.11.0";', + 'const dependency = "npm:ioredis@5.8.2";', "const Cache = class dependency {", " static { void import(dependency); }", "};", @@ -354,7 +349,7 @@ describe("findCoreThirdPartySourceImports", () => { { path: "src/cache/computed-class-method.ts", content: [ - 'const dependency = "npm:redis@5.11.0";', + 'const dependency = "npm:ioredis@5.8.2";', "class Cache {", " [import(dependency)](dependency: string) {}", "}", @@ -363,7 +358,7 @@ describe("findCoreThirdPartySourceImports", () => { { path: "src/cache/computed-object-method.ts", content: [ - 'const dependency = "npm:redis@5.11.0";', + 'const dependency = "npm:ioredis@5.8.2";', "const cache = {", " [import(dependency)](dependency: string) {}", "};", @@ -372,7 +367,7 @@ describe("findCoreThirdPartySourceImports", () => { { path: "src/cache/namespace-scope.ts", content: [ - 'const dependency = "npm:redis@5.11.0";', + 'const dependency = "npm:ioredis@5.8.2";', "namespace Cache {", ' const dependency = "./local.ts";', " void import(dependency);", @@ -386,42 +381,42 @@ describe("findCoreThirdPartySourceImports", () => { { path: "src/cache/default-parameter.ts", line: 3, - specifier: "npm:redis@5.11.0", + specifier: "npm:ioredis@5.8.2", }, { path: "src/cache/destructured-parameter.ts", line: 3, - specifier: "npm:redis@5.11.0", + specifier: "npm:ioredis@5.8.2", }, { path: "src/cache/loop-scope.ts", line: 5, - specifier: "npm:redis@5.11.0", + specifier: "npm:ioredis@5.8.2", }, { path: "src/cache/parameter-var-scope.ts", line: 2, - specifier: "npm:redis@5.11.0", + specifier: "npm:ioredis@5.8.2", }, { path: "src/cache/static-block-scope.ts", line: 8, - specifier: "npm:redis@5.11.0", + specifier: "npm:ioredis@5.8.2", }, { path: "src/cache/computed-class-method.ts", line: 3, - specifier: "npm:redis@5.11.0", + specifier: "npm:ioredis@5.8.2", }, { path: "src/cache/computed-object-method.ts", line: 3, - specifier: "npm:redis@5.11.0", + specifier: "npm:ioredis@5.8.2", }, { path: "src/cache/namespace-scope.ts", line: 6, - specifier: "npm:redis@5.11.0", + specifier: "npm:ioredis@5.8.2", }, ]); }); diff --git a/src/middleware/builtin/security/rate-limit.test.ts b/src/middleware/builtin/security/rate-limit.test.ts index 36ea7c6398..af778e2f0d 100644 --- a/src/middleware/builtin/security/rate-limit.test.ts +++ b/src/middleware/builtin/security/rate-limit.test.ts @@ -9,6 +9,8 @@ import { afterEach, beforeEach, describe, it } from "#veryfront/testing/bdd.ts"; import { delay } from "#std/async.ts"; import { scaleMs } from "#veryfront/testing/timing.ts"; import { deleteEnv, getHostEnv, setEnv } from "#veryfront/platform/compat/process.ts"; +import { __subscribeLogRecordEmitter, type LogEntry } from "#veryfront/utils/logger/index.ts"; +import { ClientClosedError } from "npm:redis@5.11.0"; import { MiddlewareContext } from "../../core/context.ts"; import { authRateLimit, @@ -30,13 +32,18 @@ function createMockRedisClient(): { del: (key: string) => Promise; on: (event: string, listener: (...args: unknown[]) => void) => void; _evalCalls: number; + _connectCalls: number; _disconnectCalls: number; } { let evalCalls = 0; + let connectCalls = 0; let disconnectCalls = 0; return { - connect: () => Promise.resolve(), + connect: () => { + connectCalls += 1; + return Promise.resolve(); + }, disconnect: () => { disconnectCalls += 1; return Promise.resolve(); @@ -52,6 +59,9 @@ function createMockRedisClient(): { get _evalCalls() { return evalCalls; }, + get _connectCalls() { + return connectCalls; + }, get _disconnectCalls() { return disconnectCalls; }, @@ -359,9 +369,7 @@ describe("rateLimit middleware", () => { const redisStore = new RedisRateLimitStore({ keyPrefix: "compat:" }); const mockClient = createMockRedisClient(); mockClient.disconnect = () => { - const error = new Error("The client is closed"); - error.name = "ClientClosedError"; - return Promise.reject(error); + return Promise.reject(new ClientClosedError()); }; (redisStore as unknown as { loadClientFactory: () => Promise<() => typeof mockClient>; @@ -372,6 +380,79 @@ describe("rateLimit middleware", () => { await redisStore.destroy(); }); + it("should reject invalid Redis reset keys before connecting", async () => { + const redisStore = new RedisRateLimitStore({ keyPrefix: "compat:" }); + const mockClient = createMockRedisClient(); + (redisStore as unknown as { + loadClientFactory: () => Promise<() => typeof mockClient>; + }).loadClientFactory = () => Promise.resolve(() => mockClient); + + await assertRejects( + () => redisStore.reset("x".repeat(1025)), + RangeError, + "1024", + ); + + assertEquals(mockClient._connectCalls, 0); + }); + + it("should attach pending Redis connection rejection handling before destroy cancels it", async () => { + const redisStore = new RedisRateLimitStore({ keyPrefix: "compat:" }); + const mockClient = createMockRedisClient(); + let connectStarted = false; + mockClient.connect = () => { + connectStarted = true; + return new Promise(() => {}); + }; + (redisStore as unknown as { + loadClientFactory: () => Promise<() => typeof mockClient>; + }).loadClientFactory = () => Promise.resolve(() => mockClient); + + const incrementPromise = redisStore.increment("pending", 30000); + for (let attempt = 0; attempt < 10 && !connectStarted; attempt++) { + await Promise.resolve(); + } + + const pending = (redisStore as unknown as { + clientPromise: Promise | null; + }).clientPromise; + let cancelObserved = false; + let catchAttachedBeforeCancel = false; + if (pending) { + const originalCatch = pending.catch.bind(pending); + Object.defineProperty(pending, "catch", { + configurable: true, + value: (...args: Parameters["catch"]>) => { + if (!cancelObserved) catchAttachedBeforeCancel = true; + return originalCatch(...args); + }, + }); + } + const internals = redisStore as unknown as { + cancelPendingConnection: (() => void) | null; + }; + const originalCancel = internals.cancelPendingConnection; + internals.cancelPendingConnection = () => { + cancelObserved = true; + originalCancel?.(); + }; + + await redisStore.destroy(); + + assertExists(pending); + assertEquals(catchAttachedBeforeCancel, true); + const handledPromise = pending.then( + () => "resolved" as const, + () => "rejected" as const, + ); + await assertRejects( + () => incrementPromise, + Error, + "superseded", + ); + assertEquals(await handledPromise, "rejected"); + }); + it("should fail closed when custom keys are invalid without calling the store", async () => { let incrementCalled = false; const middleware = rateLimit({ @@ -418,6 +499,95 @@ describe("rateLimit middleware", () => { assertEquals(incrementCalled, false); }); + it("should log key resolution failures separately from store failures", async () => { + const records: LogEntry[] = []; + const unsubscribe = __subscribeLogRecordEmitter((entry) => { + if (entry.component === "rate-limit") records.push(entry); + }); + + try { + const keyFailure = rateLimit({ + keyGenerator: () => { + throw new Error("custom key failure"); + }, + store: { + increment: () => Promise.resolve({ count: 1, resetAt: Date.now() + 1000 }), + reset: () => Promise.resolve(), + }, + }); + const storeFailure = rateLimit({ + store: { + increment: () => Promise.reject(new Error("backend unavailable")), + reset: () => Promise.resolve(), + }, + }); + + assertEquals( + (await keyFailure(createContext(), () => Promise.resolve(new Response("OK")))) + ?.status, + 503, + ); + assertEquals( + (await storeFailure(createContext(), () => Promise.resolve(new Response("OK")))) + ?.status, + 503, + ); + } finally { + unsubscribe(); + } + + assertEquals(records.map((record) => record.message), [ + "Rate limit key resolution failed; request denied", + "Rate limit store failed; request denied", + ]); + assertEquals(records.map((record) => record.context?.stage), [ + "key-resolution", + "store-increment", + ]); + }); + + it("should emit a capacity-specific store failure signal", async () => { + const records: LogEntry[] = []; + const unsubscribe = __subscribeLogRecordEmitter((entry) => { + if (entry.component === "rate-limit") records.push(entry); + }); + const store = new MemoryRateLimitStore(60000, { maxEntries: 1 }); + const middleware = rateLimit({ + maxRequests: 10, + windowMs: 60000, + store, + trustProxy: true, + }); + + try { + assertEquals( + (await middleware( + createContext("198.51.100.1"), + () => Promise.resolve(new Response("OK")), + ))?.status, + 200, + ); + assertEquals( + (await middleware( + createContext("198.51.100.2"), + () => Promise.resolve(new Response("OK")), + ))?.status, + 503, + ); + } finally { + unsubscribe(); + store.destroy(); + } + + assertEquals(records.length, 1); + assertEquals( + records[0]?.message, + "Rate limit store capacity exhausted; request denied", + ); + assertEquals(records[0]?.context?.stage, "store-capacity"); + assertEquals(records[0]?.context?.maxEntries, 1); + }); + it("should use custom key generator", async () => { let capturedKey = ""; const middleware = rateLimit({ diff --git a/src/middleware/builtin/security/rate-limit.ts b/src/middleware/builtin/security/rate-limit.ts index 6a5e631a01..84c840b67b 100644 --- a/src/middleware/builtin/security/rate-limit.ts +++ b/src/middleware/builtin/security/rate-limit.ts @@ -25,6 +25,16 @@ const STORE_FAILURE_RETRY_AFTER_SECONDS = 60; const STORE_FAILURE_LOG_INTERVAL_MS = MS_PER_MINUTE; const logger = serverLogger.component("rate-limit"); +class MemoryRateLimitCapacityError extends RangeError { + override name = "MemoryRateLimitCapacityError"; + + constructor(readonly maxEntries: number) { + super( + `Memory rate limit store capacity of ${maxEntries} entries is exhausted`, + ); + } +} + function createRateLimitEntry(now: number, windowMs: number): RateLimitEntry { return { count: 1, resetAt: now + windowMs }; } @@ -85,9 +95,7 @@ export class MemoryRateLimitStore implements RateLimitStore { this.removeExpired(now); } if (this.counts.size >= this.maxEntries) { - throw new RangeError( - `Memory rate limit store capacity of ${this.maxEntries} entries is exhausted`, - ); + throw new MemoryRateLimitCapacityError(this.maxEntries); } const entry = createRateLimitEntry(now, normalizedWindowMs); @@ -122,7 +130,9 @@ export class MemoryRateLimitStore implements RateLimitStore { export interface MemoryRateLimitStoreOptions { /** * Maximum number of active identities retained by the store. - * New identities fail closed when all entries are active. + * Size this above the expected concurrent identities in one rate-limit + * window. New identities fail closed when all entries are active; existing + * identities remain tracked until their windows expire. */ maxEntries?: number; } @@ -201,6 +211,18 @@ function storeUnavailableResponse(): Response { }); } +function logRateLimitFailure( + message: string, + stage: "key-resolution" | "store-increment" | "store-capacity", + error: unknown, +): void { + logger.error(message, { + stage, + errorName: error instanceof Error ? error.name : typeof error, + ...(error instanceof MemoryRateLimitCapacityError ? { maxEntries: error.maxEntries } : {}), + }); +} + /** Create rate-limit middleware. */ export function rateLimit( optionsOrMaxRequests?: number | RateLimitOptions, @@ -251,8 +273,26 @@ export function rateLimit( return async (ctx, next) => { const req = getRequest(ctx); let entry: RateLimitEntry; + let key: string; + try { + key = requireRateLimitKey(keyGenerator(req)); + } catch (error) { + const now = performance.now(); + if ( + lastStoreFailureLogAt === undefined || + now - lastStoreFailureLogAt >= STORE_FAILURE_LOG_INTERVAL_MS + ) { + lastStoreFailureLogAt = now; + logRateLimitFailure( + "Rate limit key resolution failed; request denied", + "key-resolution", + error, + ); + } + return storeUnavailableResponse(); + } + try { - const key = requireRateLimitKey(keyGenerator(req)); entry = requireRateLimitEntry(await store.increment(key, windowMs)); } catch (error) { const now = performance.now(); @@ -261,9 +301,19 @@ export function rateLimit( now - lastStoreFailureLogAt >= STORE_FAILURE_LOG_INTERVAL_MS ) { lastStoreFailureLogAt = now; - logger.error("Rate limit store failed; request denied", { - errorName: error instanceof Error ? error.name : typeof error, - }); + if (error instanceof MemoryRateLimitCapacityError) { + logRateLimitFailure( + "Rate limit store capacity exhausted; request denied", + "store-capacity", + error, + ); + } else { + logRateLimitFailure( + "Rate limit store failed; request denied", + "store-increment", + error, + ); + } } return storeUnavailableResponse(); } diff --git a/src/middleware/builtin/security/redis-rate-limit.ts b/src/middleware/builtin/security/redis-rate-limit.ts index 13e7255093..67197f44ed 100644 --- a/src/middleware/builtin/security/redis-rate-limit.ts +++ b/src/middleware/builtin/security/redis-rate-limit.ts @@ -1,7 +1,6 @@ import { createError, toError } from "../../../errors/index.ts"; import { serverLogger } from "../../../utils/logger/index.ts"; import { MAX_TIMER_DELAY_MS } from "../../../utils/timer.ts"; -import { createClient } from "npm:redis@5.11.0"; import { requireRateLimitKey, requireRateLimitWindowMs } from "./rate-limit-validation.ts"; import type { RateLimitEntry, RateLimitStore } from "./types.ts"; @@ -27,9 +26,12 @@ interface RedisClientFactoryOptions { } type RedisClientFactory = (options: RedisClientFactoryOptions) => RedisClient; +type RedisClientClosedErrorConstructor = new () => Error; const DEFAULT_REDIS_CONNECT_TIMEOUT_MS = 5_000; const DEFAULT_REDIS_OPERATION_TIMEOUT_MS = 5_000; +const REDIS_MODULE_SPECIFIER = "npm:redis@5.11.0"; +let RedisClientClosedError: RedisClientClosedErrorConstructor | undefined; const INCREMENT_WITH_TTL_SCRIPT = ` local count = redis.call("INCR", KEYS[1]) @@ -143,7 +145,9 @@ export class RedisRateLimitStore implements RateLimitStore { } private async loadClientFactory(): Promise { - return createClient as unknown as RedisClientFactory; + const redis = await import(REDIS_MODULE_SPECIFIER); + RedisClientClosedError = redis.ClientClosedError as RedisClientClosedErrorConstructor; + return redis.createClient as unknown as RedisClientFactory; } private async disconnectBestEffort(client: RedisClient): Promise { @@ -329,11 +333,12 @@ export class RedisRateLimitStore implements RateLimitStore { } async reset(key: string): Promise { + const normalizedKey = requireRateLimitKey(key); const client = await this.ensureClient(); const generation = this.clientGeneration; try { await this.withTimeout( - client.del(this.storageKey(requireRateLimitKey(key))), + client.del(this.storageKey(normalizedKey)), this.operationTimeoutMs, "reset", ); @@ -349,6 +354,7 @@ export class RedisRateLimitStore implements RateLimitStore { const client = this.client; const connectingClient = this.connectingClient; const pending = this.clientPromise; + pending?.catch(() => {}); const cancelPendingConnection = this.cancelPendingConnection; const clientsToDisconnect = new Set(this.pendingDisconnectClients); if (client) clientsToDisconnect.add(client); @@ -370,8 +376,6 @@ export class RedisRateLimitStore implements RateLimitStore { }) ), ); - pending?.catch(() => {}); - if (disconnectFailed) throw disconnectError; } } @@ -403,7 +407,15 @@ function isTimeoutError(error: unknown): boolean { } function isAlreadyClosedClientError(error: unknown): boolean { - return error instanceof Error && error.name === "ClientClosedError"; + if (!(error instanceof Error)) return false; + if ( + RedisClientClosedError !== undefined && + error instanceof RedisClientClosedError + ) { + return true; + } + return error.constructor.name === "ClientClosedError" && + error.message === "The client is closed"; } function parseIncrementResult(result: unknown): [number, number] { From a51004cc564013ed6e4330b9f3ffafda3ab1aae8 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 12:10:45 +0200 Subject: [PATCH 10/17] Keep packaging validation reproducible after baseline merge The current main packaging test resolves the pinned parser dependency through the scripts lock. Recording that exact resolution keeps the reconciled PR branch usable with frozen dependency checks. Constraint: Script verification must remain reproducible under Deno frozen mode. Confidence: high Scope-risk: narrow Tested: npm package metadata suite, 26 steps, with --frozen --- scripts/deno.lock | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scripts/deno.lock b/scripts/deno.lock index 4111744f8d..7f18e98b97 100644 --- a/scripts/deno.lock +++ b/scripts/deno.lock @@ -18,6 +18,7 @@ "jsr:@ts-morph/common@0.27": "0.27.0", "npm:@babel/parser@7.29.2": "7.29.2", "npm:@mdx-js/mdx@3.1.1": "3.1.1", + "npm:es-module-lexer@2.3.1": "2.3.1", "npm:esbuild@0.28.1": "0.28.1" }, "jsr": { @@ -365,6 +366,9 @@ "dequal" ] }, + "es-module-lexer@2.3.1": { + "integrity": "sha512-shc1dbU90Yl/xq1QrC7QRtfcwURZuVRfPhZbDoldJ1cn1gzDvBaBWlv0eFolj5+0znnPJz5TXLxsN77X/12KTA==" + }, "esast-util-from-estree@2.0.0": { "integrity": "sha512-4CyanoAudUSBAn5K13H4JhsMH6L9ZP7XbLVe/dKybkxMO7eDyLsT8UHl9TRNrU2Gr9nz+FovfSIjuXWJ81uVwQ==", "dependencies": [ From d78ea15e5a9d58c67ac8368db8b4e6d0c2cdb431 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 12:09:30 +0200 Subject: [PATCH 11/17] fix(security): preserve extension-owned Redis rate limiting --- cli/commands/skills/handler.test.ts | 2 +- docs/api-reference/veryfront/middleware.md | 27 +- .../ext-redis/src/rate-limit-store.test.ts | 17 + extensions/ext-redis/src/rate-limit-store.ts | 2 + scripts/build/npm-package-metadata.test.ts | 11 +- scripts/build/npm-package-metadata.ts | 1 + scripts/lint/audit-core-deps.test.ts | 5 + scripts/lint/audit-core-deps.ts | 4 +- scripts/test/npm-install-smoke.sh | 47 +-- .../builtin/security/rate-limit.test.ts | 247 +++++------- src/middleware/builtin/security/rate-limit.ts | 108 +++--- .../builtin/security/redis-rate-limit.test.ts | 290 ++++++++++++++ .../builtin/security/redis-rate-limit.ts | 364 +++--------------- 13 files changed, 547 insertions(+), 578 deletions(-) create mode 100644 src/middleware/builtin/security/redis-rate-limit.test.ts diff --git a/cli/commands/skills/handler.test.ts b/cli/commands/skills/handler.test.ts index 997f7968d4..d317721c43 100644 --- a/cli/commands/skills/handler.test.ts +++ b/cli/commands/skills/handler.test.ts @@ -9,7 +9,7 @@ describe("Skills Command", () => { ): Promise<{ code: number; stdout: string; stderr: string }> { const cliPath = new URL("../../main.ts", import.meta.url).pathname; const result = await new Deno.Command(Deno.execPath(), { - args: ["run", "--quiet", "-A", cliPath, "skills", "info", ...args, "--json"], + args: ["run", "-A", cliPath, "skills", "info", ...args, "--json"], env: { VERYFRONT_NO_UPDATE_CHECK: "1", NO_COLOR: "1" }, stdin: "null", stdout: "piped", diff --git a/docs/api-reference/veryfront/middleware.md b/docs/api-reference/veryfront/middleware.md index 0faca2d858..0226b31693 100644 --- a/docs/api-reference/veryfront/middleware.md +++ b/docs/api-reference/veryfront/middleware.md @@ -110,11 +110,12 @@ Options accepted by rate limit. | Property | Type | Description | Source | |----------|------|-------------|--------| -| `maxRequests?` | `number` | Max requests per window | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L132) | -| `windowMs?` | `number` | Time window (ms) | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L133) | -| `store?` | `RateLimitStore` | Storage backend | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L134) | -| `keyGenerator?` | (req: Request) => string | Function to derive rate limit key from request | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L135) | -| `trustProxy?` | `boolean` | Trust proxy-set forwarding headers (X-Forwarded-For) for keying. Defaults to false so forwarded headers are ignored and cannot be used to evade limits. Enable only when a trusted proxy that appends the real client IP sits in front of this middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L142) | +| `maxRequests?` | `number` | Max requests per window | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L142) | +| `windowMs?` | `number` | Time window (ms) | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L143) | +| `store?` | `RateLimitStore` | Storage backend | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L144) | +| `maxEntries?` | `number` | Capacity of the default in-memory store. It must exceed the peak distinct identities expected in one complete window plus burst headroom. Capacity exhaustion denies only previously unseen identities with HTTP 503. Cannot be combined with a caller-provided `store`. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L151) | +| `keyGenerator?` | (req: Request) => string | Function to derive rate limit key from request | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L152) | +| `trustProxy?` | `boolean` | Trust proxy-set forwarding headers (X-Forwarded-For) for keying. Defaults to false so forwarded headers are ignored and cannot be used to evade limits. Enable only when a trusted proxy that appends the real client IP sits in front of this middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L159) | ### `MemoryRateLimitStoreOptions` @@ -150,13 +151,13 @@ Options accepted by timeout. | Name | Description | Source | |------|-------------|--------| -| `authRateLimit` | Pre-configured rate limiter for authentication endpoints (5 req/15min). | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L289) | +| `authRateLimit` | Pre-configured rate limiter for authentication endpoints (5 req/15min). | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L331) | | `cors` | Create CORS middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/security/http/cors/middleware.ts#L10) | | `devLogger` | Create development request logging middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/logger.ts#L244) | | `getTimeoutFromEnv` | Gets timeout from environment variable REQUEST_TIMEOUT_MS | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/timeout.ts#L94) | | `logger` | Create request logging middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/logger.ts#L191) | | `prodLogger` | Create production request logging middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/logger.ts#L249) | -| `rateLimit` | Create rate-limit middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L205) | +| `rateLimit` | Create rate-limit middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L224) | | `timeout` | Creates a middleware that enforces request timeouts. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/timeout.ts#L52) | | `timeoutFromEnv` | Creates a timeout middleware with configuration from environment | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/timeout.ts#L102) | @@ -164,27 +165,27 @@ Options accepted by timeout. | Name | Description | Source | |------|-------------|--------| -| `MemoryRateLimitStore` | Implement memory rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L37) | +| `MemoryRateLimitStore` | Implement memory rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L45) | | `MiddlewareContext` | Context for middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/context.ts#L5) | | `MiddlewarePipeline` | Implement middleware pipeline. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/pipeline/pipeline.ts#L9) | -| `RedisRateLimitStore` | Create a Redis rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/redis-rate-limit.ts#L55) | +| `RedisRateLimitStore` | Redis rate-limit store backed by the registered Redis runtime provider. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/redis-rate-limit.ts#L39) | ### Types | Name | Description | Source | |------|-------------|--------| -| `AuthRateLimitOptions` | Options accepted by the authentication rate-limit preset. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L146) | +| `AuthRateLimitOptions` | Options accepted by the authentication rate-limit preset. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L163) | | `Context` | Context for context. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/types.ts#L8) | | `CorsOptions` | Options accepted by cors. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/types.ts#L26) | | `ExecutionContext` | Context for execution. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/types.ts#L2) | | `LogFormat` | Public API contract for log format. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/logger.ts#L14) | | `LoggerOptions` | Options accepted by logger. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/logger.ts#L17) | -| `MemoryRateLimitStoreOptions` | Options accepted by the in-memory rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L122) | +| `MemoryRateLimitStoreOptions` | Options accepted by the in-memory rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L128) | | `MiddlewareFactory` | Public API contract for middleware factory. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/types.ts#L32) | | `MiddlewareHandler` | Handler for middleware. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/types.ts#L26) | | `MiddlewarePipelineOptions` | Options accepted by middleware pipeline. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/pipeline/types.ts#L2) | | `Next` | Public API contract for next. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/core/types.ts#L23) | -| `RateLimitOptions` | Options accepted by rate limit. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L131) | +| `RateLimitOptions` | Options accepted by rate limit. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L141) | | `RateLimitStore` | Public API contract for rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/types.ts#L32) | -| `RedisRateLimitOptions` | Options accepted by Redis rate limit. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/redis-rate-limit.ts#L45) | +| `RedisRateLimitOptions` | Options accepted by the provider-backed Redis rate-limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/redis-rate-limit.ts#L24) | | `TimeoutOptions` | Options accepted by timeout. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/timeout.ts#L17) | diff --git a/extensions/ext-redis/src/rate-limit-store.test.ts b/extensions/ext-redis/src/rate-limit-store.test.ts index 190a17a3aa..c01324868a 100644 --- a/extensions/ext-redis/src/rate-limit-store.test.ts +++ b/extensions/ext-redis/src/rate-limit-store.test.ts @@ -316,6 +316,23 @@ describe("middleware/builtin/security/redis-rate-limit", () => { const { rateStore } = createStoreWithMock(); await rateStore.reset("nonexistent"); }); + + it("should reject an invalid key before loading or connecting Redis", async () => { + const rateStore = new RedisRateLimitStore(); + let factoryLoads = 0; + // deno-lint-ignore no-explicit-any + (rateStore as any).loadClientFactory = () => { + factoryLoads++; + return Promise.resolve(() => createMockRedisClient()); + }; + + await assertRejects( + () => rateStore.reset("x".repeat(1025)), + RangeError, + "1024", + ); + assertEquals(factoryLoads, 0); + }); }); describe("destroy", () => { diff --git a/extensions/ext-redis/src/rate-limit-store.ts b/extensions/ext-redis/src/rate-limit-store.ts index c956746bc9..adc3c4f44f 100644 --- a/extensions/ext-redis/src/rate-limit-store.ts +++ b/extensions/ext-redis/src/rate-limit-store.ts @@ -357,6 +357,8 @@ export class RedisRateLimitStore implements RateLimitStore { const client = this.client; const connectingClient = this.connectingClient; const pending = this.clientPromise; + // Mark the pending connection rejection as observed before cancellation; + // disconnect work below may otherwise leave an unhandled-rejection window. pending?.catch(() => {}); const cancelPendingConnection = this.cancelPendingConnection; const clientsToDisconnect = new Set(this.pendingDisconnectClients); diff --git a/scripts/build/npm-package-metadata.test.ts b/scripts/build/npm-package-metadata.test.ts index 21703dcb98..2ce8ef0fe3 100644 --- a/scripts/build/npm-package-metadata.test.ts +++ b/scripts/build/npm-package-metadata.test.ts @@ -331,10 +331,9 @@ const ROOT_BUNDLED_EXTENSIONS = new Set([ "ext-eval-report-mlflow", ]); -const ROOT_RUNTIME_EXTENSION_SHARED_DEPENDENCIES = new Set([ +const ROOT_RUNTIME_SHARED_DEPENDENCIES = new Set([ "react", "react-dom", - "redis", ]); Deno.test("EXTENSION_OWNED_DEPENDENCIES stays in sync with extension manifests", async () => { @@ -373,7 +372,7 @@ Deno.test("EXTENSION_OWNED_DEPENDENCIES stays in sync with extension manifests", assertEquals( owned.has(dependency) || optionalPeers.has(dependency) || - ROOT_RUNTIME_EXTENSION_SHARED_DEPENDENCIES.has(dependency), + ROOT_RUNTIME_SHARED_DEPENDENCIES.has(dependency), true, `${dependency} (declared by ${manifestPath}) must be added to EXTENSION_OWNED_DEPENDENCIES so it does not leak into root veryfront npm installs`, ); @@ -454,7 +453,7 @@ describe("normalizeNpmPackageMetadata", () => { }); }); - it("keeps extension-only packages out while retaining root middleware Redis support", () => { + it("keeps first-party extension implementation packages out of root npm metadata", () => { const pkg = normalizeNpmPackageMetadata({ dependencies: { "@babel/parser": "^7.29.2", @@ -469,7 +468,7 @@ describe("normalizeNpmPackageMetadata", () => { }, }); - assertEquals(pkg.dependencies, { redis: "5.11.0", zod: "4.3.6" }); + assertEquals(pkg.dependencies, { zod: "4.3.6" }); assertEquals(pkg.peerDependencies, { "@huggingface/transformers": "^4.2.0", }); @@ -624,8 +623,6 @@ describe("npm supply-chain policy", () => { } assertStringIncludes(source, "CodeParser was not registered"); - assertStringIncludes(source, "RedisRateLimitStore"); - assertStringIncludes(source, "await store.increment('user-1', 30000)"); assertStringIncludes(source, "getDeferredExtensionState(resolved)"); assertStringIncludes(source, "await deferred.load(logger)"); assertStringIncludes(source, "app/page.tsx"); diff --git a/scripts/build/npm-package-metadata.ts b/scripts/build/npm-package-metadata.ts index 90d65863ba..abe719637a 100644 --- a/scripts/build/npm-package-metadata.ts +++ b/scripts/build/npm-package-metadata.ts @@ -58,6 +58,7 @@ export const EXTENSION_OWNED_DEPENDENCIES = [ "@opentelemetry/sdk-trace-base", "@opentelemetry/semantic-conventions", "@redis/client", + "redis", "@sentry/deno", "@sentry/node", "@tailwindcss/forms", diff --git a/scripts/lint/audit-core-deps.test.ts b/scripts/lint/audit-core-deps.test.ts index 90664fe8a2..066d9c250c 100644 --- a/scripts/lint/audit-core-deps.test.ts +++ b/scripts/lint/audit-core-deps.test.ts @@ -243,6 +243,11 @@ describe("findCoreThirdPartySourceImports", () => { line: 5, specifier: "npm:@redis/client@1.5.8", }, + { + path: "src/cache/hidden-imports.ts", + line: 6, + specifier: "npm:redis@5.11.0", + }, { path: "src/cache/hidden-imports.ts", line: 7, diff --git a/scripts/lint/audit-core-deps.ts b/scripts/lint/audit-core-deps.ts index b449ee4b00..f4466620a1 100644 --- a/scripts/lint/audit-core-deps.ts +++ b/scripts/lint/audit-core-deps.ts @@ -18,9 +18,7 @@ export interface RootNpmSpecifierLiteralIssue { value: string; } -const CORE_THIRD_PARTY_IMPORT_ALLOWLIST = new Set([ - "npm:redis@5.11.0", -]); +const CORE_THIRD_PARTY_IMPORT_ALLOWLIST = new Set(); function isThirdPartyImportTarget(target: string): boolean { if (target.startsWith("./") || target.startsWith("../")) return false; diff --git a/scripts/test/npm-install-smoke.sh b/scripts/test/npm-install-smoke.sh index 36d454fdf5..c4b4fe1238 100755 --- a/scripts/test/npm-install-smoke.sh +++ b/scripts/test/npm-install-smoke.sh @@ -5,11 +5,10 @@ # throwaway npm project, that: # 1. a `veryfront` install with co-published required packages runs the CLI # and activates the parser extension under Node -# 2. the public middleware Redis rate limit store increments under Node -# 3. the @huggingface/transformers optional peer is declared -# 4. loading a missing extension fails naming the installable package -# 5. installing @veryfront/ext-auth-jwt makes the extension load -# 6. a broken transitive dependency surfaces the real error, not a +# 2. the @huggingface/transformers optional peer is declared +# 3. loading a missing extension fails naming the installable package +# 4. installing @veryfront/ext-auth-jwt makes the extension load +# 5. a broken transitive dependency surfaces the real error, not a # misleading "extension not installed" skip # # Requires: `deno task build:npm` output in ./npm, node + npm on PATH. @@ -88,44 +87,14 @@ if (ast?.type !== 'File') throw new Error('TSX parse failed'); await extension.teardown?.(); " || fail "root optional builtin did not register a working CodeParser" -echo "== 2. root install: middleware Redis rate-limit store increments" -node --input-type=module -e " -const { RedisRateLimitStore } = await import('veryfront/middleware'); -let evalCalls = 0; -let disconnectCalls = 0; -const client = { - connect: () => Promise.resolve(), - disconnect: () => { - disconnectCalls++; - return Promise.resolve(); - }, - eval: (_script, options) => { - evalCalls++; - if (options.keys[0] !== 'smoke:user-1') throw new Error('unexpected Redis key'); - if (options.arguments[0] !== '30000') throw new Error('unexpected Redis window'); - return Promise.resolve([1, 30000]); - }, - del: () => Promise.resolve(1), - on: () => {}, -}; -const store = new RedisRateLimitStore({ keyPrefix: 'smoke:' }); -store.loadClientFactory = () => Promise.resolve(() => client); -const entry = await store.increment('user-1', 30000); -await store.destroy(); -if (entry.count !== 1) throw new Error('RedisRateLimitStore did not increment'); -if (entry.resetAt <= Date.now()) throw new Error('RedisRateLimitStore resetAt is not future'); -if (evalCalls !== 1) throw new Error('Redis eval was not called exactly once'); -if (disconnectCalls !== 1) throw new Error('Redis client was not disconnected'); -" || fail "root middleware RedisRateLimitStore did not increment from npm package" - -echo "== 3. root install: transformers optional peer declared" +echo "== 2. root install: transformers optional peer declared" node -e " const p = require('./node_modules/veryfront/package.json'); if (!p.peerDependencies?.['@huggingface/transformers']) process.exit(1); if (p.peerDependenciesMeta?.['@huggingface/transformers']?.optional !== true) process.exit(1); " || fail "@huggingface/transformers optional peer missing from root package.json" -echo "== 4. root install: missing extension failure names the installable package" +echo "== 3. root install: missing extension failure names the installable package" set +e MISSING_OUTPUT="$(node -e " import('./node_modules/veryfront/esm/src/extensions/first-party-import.js').then(async (m) => { @@ -139,7 +108,7 @@ set -e echo "$MISSING_OUTPUT" | grep -q "install @veryfront/ext-auth-jwt alongside veryfront" || fail "missing-extension error lacks the install hint: $MISSING_OUTPUT" -echo "== 5. with @veryfront/ext-auth-jwt installed: extension loads" +echo "== 4. with @veryfront/ext-auth-jwt installed: extension loads" npm install --no-fund --no-audit --silent --ignore-scripts ./veryfront-ext-auth-jwt-*.tgz node -e " import('./node_modules/veryfront/esm/src/extensions/first-party-import.js').then(async (m) => { @@ -148,7 +117,7 @@ import('./node_modules/veryfront/esm/src/extensions/first-party-import.js').then }); " || fail "ext-auth-jwt did not load after installing @veryfront/ext-auth-jwt" -echo "== 6. broken transitive dependency surfaces the real error" +echo "== 5. broken transitive dependency surfaces the real error" mv node_modules/jose node_modules/jose.smoke-removed set +e BROKEN_OUTPUT="$(node -e " diff --git a/src/middleware/builtin/security/rate-limit.test.ts b/src/middleware/builtin/security/rate-limit.test.ts index af778e2f0d..6dfb654700 100644 --- a/src/middleware/builtin/security/rate-limit.test.ts +++ b/src/middleware/builtin/security/rate-limit.test.ts @@ -10,7 +10,6 @@ import { delay } from "#std/async.ts"; import { scaleMs } from "#veryfront/testing/timing.ts"; import { deleteEnv, getHostEnv, setEnv } from "#veryfront/platform/compat/process.ts"; import { __subscribeLogRecordEmitter, type LogEntry } from "#veryfront/utils/logger/index.ts"; -import { ClientClosedError } from "npm:redis@5.11.0"; import { MiddlewareContext } from "../../core/context.ts"; import { authRateLimit, @@ -22,52 +21,6 @@ import { (globalThis as Record).__vfDisableLruInterval = true; -function createMockRedisClient(): { - connect: () => Promise; - disconnect: () => Promise; - eval: ( - script: string, - options: { keys: string[]; arguments: string[] }, - ) => Promise<[number, number]>; - del: (key: string) => Promise; - on: (event: string, listener: (...args: unknown[]) => void) => void; - _evalCalls: number; - _connectCalls: number; - _disconnectCalls: number; -} { - let evalCalls = 0; - let connectCalls = 0; - let disconnectCalls = 0; - - return { - connect: () => { - connectCalls += 1; - return Promise.resolve(); - }, - disconnect: () => { - disconnectCalls += 1; - return Promise.resolve(); - }, - eval: (_script: string, options: { keys: string[]; arguments: string[] }) => { - evalCalls += 1; - assertEquals(options.keys, ["compat:user-1"]); - assertEquals(options.arguments, ["30000"]); - return Promise.resolve([1, 30000]); - }, - del: () => Promise.resolve(1), - on: () => {}, - get _evalCalls() { - return evalCalls; - }, - get _connectCalls() { - return connectCalls; - }, - get _disconnectCalls() { - return disconnectCalls; - }, - }; -} - describe("MemoryRateLimitStore", () => { let store: MemoryRateLimitStore; @@ -134,11 +87,13 @@ describe("MemoryRateLimitStore", () => { try { await boundedStore.increment("existing", 60000); - await assertRejects( + const error = await assertRejects( () => boundedStore.increment("overflow", 60000), - RangeError, + Error, "capacity", ); + if (!(error instanceof Error)) throw new Error("Expected a capacity error"); + assertEquals(error.name, "MemoryRateLimitCapacityError"); const existing = await boundedStore.increment("existing", 60000); assertEquals(existing.count, 2); @@ -267,6 +222,49 @@ describe("rateLimit middleware", () => { RangeError, ); } + + assertThrows( + () => + rateLimit({ + maxEntries: 100, + store: { + increment: () => Promise.resolve({ count: 1, resetAt: Date.now() + 1_000 }), + reset: () => Promise.resolve(), + }, + }), + TypeError, + "maxEntries", + ); + }); + + it("keeps active identities available and fails closed for overflow identities", async () => { + const maxEntries = 256; + const middleware = rateLimit({ + maxRequests: 2, + windowMs: 60_000, + maxEntries, + trustProxy: true, + }); + + for (let index = 0; index < maxEntries; index++) { + const response = await middleware( + createContext(`198.51.100.${index}`), + () => Promise.resolve(new Response("OK")), + ); + assertEquals(response?.status, 200); + } + + const overflow = await middleware( + createContext("203.0.113.1"), + () => Promise.resolve(new Response("unexpected")), + ); + const existing = await middleware( + createContext("198.51.100.0"), + () => Promise.resolve(new Response("OK")), + ); + + assertEquals(overflow?.status, 503); + assertEquals(existing?.status, 200); }); it("should fail closed when the rate-limit store is unavailable", async () => { @@ -321,6 +319,44 @@ describe("rateLimit middleware", () => { } }); + it("logs key, store, and capacity failures as distinct operational signals", async () => { + const originalConsoleError = console.error; + const logs: string[] = []; + console.error = (...values: unknown[]) => { + logs.push(values.map((value) => String(value)).join(" ")); + }; + + try { + const keyFailure = rateLimit({ keyGenerator: () => "x".repeat(1_025) }); + const storeFailure = rateLimit({ + store: { + increment: () => Promise.reject(new Error("unavailable")), + reset: () => Promise.resolve(), + }, + }); + const capacityFailure = rateLimit({ maxEntries: 1, trustProxy: true }); + + await keyFailure(createContext(), () => Promise.resolve(new Response("unexpected"))); + await storeFailure(createContext(), () => Promise.resolve(new Response("unexpected"))); + await capacityFailure( + createContext("198.51.100.1"), + () => Promise.resolve(new Response("OK")), + ); + await capacityFailure( + createContext("198.51.100.2"), + () => Promise.resolve(new Response("unexpected")), + ); + + const output = logs.join("\n"); + assertEquals(output.includes("failureKind=key-resolution"), true); + assertEquals(output.includes("failureKind=store-unavailable"), true); + assertEquals(output.includes("failureKind=capacity-exhausted"), true); + assertEquals(output.includes("capacity=1"), true); + } finally { + console.error = originalConsoleError; + } + }); + it("should fail closed when a store returns an invalid counter", async () => { const middleware = rateLimit({ store: { @@ -349,110 +385,6 @@ describe("rateLimit middleware", () => { assertEquals(typeof redisStore.reset, "function"); }); - it("should exercise the legacy Redis rate-limit store export without ext-redis", async () => { - const redisStore = new RedisRateLimitStore({ keyPrefix: "compat:" }); - const mockClient = createMockRedisClient(); - (redisStore as unknown as { - loadClientFactory: () => Promise<() => typeof mockClient>; - }).loadClientFactory = () => Promise.resolve(() => mockClient); - - const entry = await redisStore.increment("user-1", 30000); - await redisStore.destroy(); - - assertEquals(entry.count, 1); - assertEquals(entry.resetAt > Date.now(), true); - assertEquals(mockClient._evalCalls, 1); - assertEquals(mockClient._disconnectCalls, 1); - }); - - it("should ignore already-closed Redis clients during legacy store destroy", async () => { - const redisStore = new RedisRateLimitStore({ keyPrefix: "compat:" }); - const mockClient = createMockRedisClient(); - mockClient.disconnect = () => { - return Promise.reject(new ClientClosedError()); - }; - (redisStore as unknown as { - loadClientFactory: () => Promise<() => typeof mockClient>; - }).loadClientFactory = () => Promise.resolve(() => mockClient); - - await redisStore.increment("user-1", 30000); - await redisStore.destroy(); - await redisStore.destroy(); - }); - - it("should reject invalid Redis reset keys before connecting", async () => { - const redisStore = new RedisRateLimitStore({ keyPrefix: "compat:" }); - const mockClient = createMockRedisClient(); - (redisStore as unknown as { - loadClientFactory: () => Promise<() => typeof mockClient>; - }).loadClientFactory = () => Promise.resolve(() => mockClient); - - await assertRejects( - () => redisStore.reset("x".repeat(1025)), - RangeError, - "1024", - ); - - assertEquals(mockClient._connectCalls, 0); - }); - - it("should attach pending Redis connection rejection handling before destroy cancels it", async () => { - const redisStore = new RedisRateLimitStore({ keyPrefix: "compat:" }); - const mockClient = createMockRedisClient(); - let connectStarted = false; - mockClient.connect = () => { - connectStarted = true; - return new Promise(() => {}); - }; - (redisStore as unknown as { - loadClientFactory: () => Promise<() => typeof mockClient>; - }).loadClientFactory = () => Promise.resolve(() => mockClient); - - const incrementPromise = redisStore.increment("pending", 30000); - for (let attempt = 0; attempt < 10 && !connectStarted; attempt++) { - await Promise.resolve(); - } - - const pending = (redisStore as unknown as { - clientPromise: Promise | null; - }).clientPromise; - let cancelObserved = false; - let catchAttachedBeforeCancel = false; - if (pending) { - const originalCatch = pending.catch.bind(pending); - Object.defineProperty(pending, "catch", { - configurable: true, - value: (...args: Parameters["catch"]>) => { - if (!cancelObserved) catchAttachedBeforeCancel = true; - return originalCatch(...args); - }, - }); - } - const internals = redisStore as unknown as { - cancelPendingConnection: (() => void) | null; - }; - const originalCancel = internals.cancelPendingConnection; - internals.cancelPendingConnection = () => { - cancelObserved = true; - originalCancel?.(); - }; - - await redisStore.destroy(); - - assertExists(pending); - assertEquals(catchAttachedBeforeCancel, true); - const handledPromise = pending.then( - () => "resolved" as const, - () => "rejected" as const, - ); - await assertRejects( - () => incrementPromise, - Error, - "superseded", - ); - assertEquals(await handledPromise, "rejected"); - }); - it("should fail closed when custom keys are invalid without calling the store", async () => { let incrementCalled = false; const middleware = rateLimit({ @@ -544,6 +476,10 @@ describe("rateLimit middleware", () => { "key-resolution", "store-increment", ]); + assertEquals(records.map((record) => record.context?.failureKind), [ + "key-resolution", + "store-unavailable", + ]); }); it("should emit a capacity-specific store failure signal", async () => { @@ -584,8 +520,9 @@ describe("rateLimit middleware", () => { records[0]?.message, "Rate limit store capacity exhausted; request denied", ); - assertEquals(records[0]?.context?.stage, "store-capacity"); - assertEquals(records[0]?.context?.maxEntries, 1); + assertEquals(records[0]?.context?.stage, "store-increment"); + assertEquals(records[0]?.context?.failureKind, "capacity-exhausted"); + assertEquals(records[0]?.context?.capacity, 1); }); it("should use custom key generator", async () => { diff --git a/src/middleware/builtin/security/rate-limit.ts b/src/middleware/builtin/security/rate-limit.ts index 84c840b67b..63b0dae020 100644 --- a/src/middleware/builtin/security/rate-limit.ts +++ b/src/middleware/builtin/security/rate-limit.ts @@ -25,13 +25,11 @@ const STORE_FAILURE_RETRY_AFTER_SECONDS = 60; const STORE_FAILURE_LOG_INTERVAL_MS = MS_PER_MINUTE; const logger = serverLogger.component("rate-limit"); -class MemoryRateLimitCapacityError extends RangeError { - override name = "MemoryRateLimitCapacityError"; +class MemoryRateLimitCapacityError extends Error { + override readonly name = "MemoryRateLimitCapacityError"; - constructor(readonly maxEntries: number) { - super( - `Memory rate limit store capacity of ${maxEntries} entries is exhausted`, - ); + constructor(readonly capacity: number) { + super(`Memory rate limit store capacity of ${capacity} entries is exhausted`); } } @@ -130,9 +128,11 @@ export class MemoryRateLimitStore implements RateLimitStore { export interface MemoryRateLimitStoreOptions { /** * Maximum number of active identities retained by the store. - * Size this above the expected concurrent identities in one rate-limit - * window. New identities fail closed when all entries are active; existing - * identities remain tracked until their windows expire. + * + * Size this above the peak number of distinct identities expected during one + * complete rate-limit window, including burst headroom. New identities fail + * closed when every entry is active; active limits are never evicted because + * eviction would let identity-flooding attackers reset their quota. */ maxEntries?: number; } @@ -142,6 +142,13 @@ export interface RateLimitOptions { maxRequests?: number; windowMs?: number; store?: RateLimitStore; + /** + * Capacity of the default in-memory store. It must exceed the peak distinct + * identities expected in one complete window plus burst headroom. Capacity + * exhaustion denies only previously unseen identities with HTTP 503. + * Cannot be combined with a caller-provided `store`. + */ + maxEntries?: number; keyGenerator?: (req: Request) => string; /** * Trust proxy-set forwarding headers (X-Forwarded-For) for keying. Defaults to @@ -156,6 +163,8 @@ export interface RateLimitOptions { export interface AuthRateLimitOptions { /** Storage backend. Existing callers can also pass the store directly. */ store?: RateLimitStore; + /** Capacity of the default in-memory store; see `RateLimitOptions.maxEntries`. */ + maxEntries?: number; /** Function to derive a stable client key from the request. */ keyGenerator?: (req: Request) => string; /** @@ -211,18 +220,6 @@ function storeUnavailableResponse(): Response { }); } -function logRateLimitFailure( - message: string, - stage: "key-resolution" | "store-increment" | "store-capacity", - error: unknown, -): void { - logger.error(message, { - stage, - errorName: error instanceof Error ? error.name : typeof error, - ...(error instanceof MemoryRateLimitCapacityError ? { maxEntries: error.maxEntries } : {}), - }); -} - /** Create rate-limit middleware. */ export function rateLimit( optionsOrMaxRequests?: number | RateLimitOptions, @@ -250,8 +247,11 @@ export function rateLimit( const windowMs = requireRateLimitWindowMs( options.windowMs ?? DEFAULT_RATE_LIMIT_WINDOW_MS, ); + if (options.store !== undefined && options.maxEntries !== undefined) { + throw new TypeError("Rate limit maxEntries cannot be combined with a custom store"); + } const store = options.store === undefined - ? new MemoryRateLimitStore(windowMs) + ? new MemoryRateLimitStore(windowMs, { maxEntries: options.maxEntries }) : requireRateLimitStore(options.store); if ( options.trustProxy !== undefined && @@ -268,52 +268,44 @@ export function rateLimit( } const keyGenerator = options.keyGenerator ?? ((req: Request) => defaultKeyGenerator(req, trustProxy)); - let lastStoreFailureLogAt: number | undefined; + const lastFailureLogAt = new Map(); return async (ctx, next) => { const req = getRequest(ctx); let entry: RateLimitEntry; - let key: string; - try { - key = requireRateLimitKey(keyGenerator(req)); - } catch (error) { - const now = performance.now(); - if ( - lastStoreFailureLogAt === undefined || - now - lastStoreFailureLogAt >= STORE_FAILURE_LOG_INTERVAL_MS - ) { - lastStoreFailureLogAt = now; - logRateLimitFailure( - "Rate limit key resolution failed; request denied", - "key-resolution", - error, - ); - } - return storeUnavailableResponse(); - } - + let stage: "key-resolution" | "store-increment" = "key-resolution"; try { + const key = requireRateLimitKey(keyGenerator(req)); + stage = "store-increment"; entry = requireRateLimitEntry(await store.increment(key, windowMs)); } catch (error) { + const failureKind = error instanceof MemoryRateLimitCapacityError + ? "capacity-exhausted" + : stage === "key-resolution" + ? "key-resolution" + : "store-unavailable"; const now = performance.now(); + const lastLogAt = lastFailureLogAt.get(failureKind); if ( - lastStoreFailureLogAt === undefined || - now - lastStoreFailureLogAt >= STORE_FAILURE_LOG_INTERVAL_MS + lastLogAt === undefined || + now - lastLogAt >= STORE_FAILURE_LOG_INTERVAL_MS ) { - lastStoreFailureLogAt = now; - if (error instanceof MemoryRateLimitCapacityError) { - logRateLimitFailure( - "Rate limit store capacity exhausted; request denied", - "store-capacity", - error, - ); - } else { - logRateLimitFailure( - "Rate limit store failed; request denied", - "store-increment", - error, - ); - } + lastFailureLogAt.set(failureKind, now); + const message = failureKind === "capacity-exhausted" + ? "Rate limit store capacity exhausted; request denied" + : failureKind === "key-resolution" + ? "Rate limit key resolution failed; request denied" + : "Rate limit store failed; request denied"; + logger.error(message, { + failureKind, + stage, + errorName: error instanceof MemoryRateLimitCapacityError + ? error.name + : error instanceof Error + ? "Error" + : typeof error, + ...(error instanceof MemoryRateLimitCapacityError ? { capacity: error.capacity } : {}), + }); } return storeUnavailableResponse(); } diff --git a/src/middleware/builtin/security/redis-rate-limit.test.ts b/src/middleware/builtin/security/redis-rate-limit.test.ts new file mode 100644 index 0000000000..c8376396e4 --- /dev/null +++ b/src/middleware/builtin/security/redis-rate-limit.test.ts @@ -0,0 +1,290 @@ +import "#veryfront/schemas/_test-setup.ts"; +import { assertEquals, assertRejects, assertThrows } from "#veryfront/testing/assert.ts"; +import { describe, it } from "#veryfront/testing/bdd.ts"; +import { type RedisRateLimitOptions, RedisRateLimitStore } from "./redis-rate-limit.ts"; + +interface MockRedisClient { + eval( + script: string, + options: { keys: string[]; arguments: string[] }, + ): Promise; + del(key: string): Promise; + _evalCalls: number; + _delCalls: number; + _lastKey?: string; + _lastWindow?: string; +} + +function createMockRedisClient( + result: unknown = [1, 60_000], +): MockRedisClient { + let evalCalls = 0; + let delCalls = 0; + const client: MockRedisClient = { + eval: (_script, options) => { + evalCalls++; + client._lastKey = options.keys[0]; + client._lastWindow = options.arguments[0]; + return Promise.resolve(result); + }, + del: (key) => { + delCalls++; + client._lastKey = key; + return Promise.resolve(1); + }, + get _evalCalls() { + return evalCalls; + }, + get _delCalls() { + return delCalls; + }, + }; + return client; +} + +function createStoreWithMock( + options?: RedisRateLimitOptions, + client = createMockRedisClient(), +): { + store: RedisRateLimitStore; + client: MockRedisClient; + getClientCalls: () => number; + closeCalls: () => number; +} { + const store = new RedisRateLimitStore(options); + let getClientCalls = 0; + let closeCalls = 0; + let closed = false; + (store as unknown as { + connection: { + getClient(): Promise; + close(): Promise; + }; + }).connection = { + getClient: () => { + getClientCalls++; + return Promise.resolve(client); + }, + close: () => { + if (!closed) { + closeCalls++; + closed = true; + } + return Promise.resolve(); + }, + }; + return { + store, + client, + getClientCalls: () => getClientCalls, + closeCalls: () => closeCalls, + }; +} + +describe("provider-backed RedisRateLimitStore", () => { + describe("constructor", () => { + it("uses the stable default key prefix", () => { + const store = new RedisRateLimitStore(); + assertEquals( + (store as unknown as { keyPrefix: string }).keyPrefix, + "veryfront:ratelimit:", + ); + }); + + it("accepts a custom key prefix", () => { + const store = new RedisRateLimitStore({ keyPrefix: "tenant:" }); + assertEquals( + (store as unknown as { keyPrefix: string }).keyPrefix, + "tenant:", + ); + }); + + it("rejects malformed options before opening a provider connection", () => { + assertThrows( + () => new RedisRateLimitStore(null as never), + TypeError, + "options", + ); + assertThrows( + () => new RedisRateLimitStore({ url: 42 as never }), + TypeError, + "url", + ); + assertThrows( + () => new RedisRateLimitStore({ keyPrefix: "x".repeat(1_025) }), + RangeError, + "1024", + ); + for (const timeout of [0, -1, 1.5, Number.NaN, Number.POSITIVE_INFINITY]) { + assertThrows( + () => new RedisRateLimitStore({ connectTimeoutMs: timeout }), + RangeError, + "connectTimeoutMs", + ); + assertThrows( + () => new RedisRateLimitStore({ operationTimeoutMs: timeout }), + RangeError, + "operationTimeoutMs", + ); + } + }); + }); + + describe("increment", () => { + it("preserves the Redis key and window contract", async () => { + const { store, client } = createStoreWithMock({ keyPrefix: "custom:" }); + const entry = await store.increment("user-1", 30_000); + + assertEquals(entry.count, 1); + assertEquals(entry.resetAt > Date.now(), true); + assertEquals(client._lastKey, "custom:user-1"); + assertEquals(client._lastWindow, "30000"); + assertEquals(client._evalCalls, 1); + }); + + it("uses the admitted Redis TTL for resetAt", async () => { + const before = Date.now(); + const { store } = createStoreWithMock(undefined, createMockRedisClient([2, 1_500])); + const entry = await store.increment("user", 30_000); + + assertEquals(entry.count, 2); + assertEquals(entry.resetAt >= before + 1_500, true); + assertEquals(entry.resetAt <= Date.now() + 1_500, true); + }); + + it("falls back to the configured window when Redis reports no TTL", async () => { + const before = Date.now(); + const { store } = createStoreWithMock(undefined, createMockRedisClient([1, -1])); + const entry = await store.increment("user", 2_000); + + assertEquals(entry.resetAt >= before + 2_000, true); + assertEquals(entry.resetAt <= Date.now() + 2_000, true); + }); + + it("validates keys and windows before opening a provider connection", async () => { + const { store, client, getClientCalls } = createStoreWithMock(); + + await assertRejects( + () => store.increment("x".repeat(1_025), 1_000), + RangeError, + "1024", + ); + await assertRejects( + () => store.increment("key", 0), + RangeError, + "windowMs", + ); + assertEquals(getClientCalls(), 0); + assertEquals(client._evalCalls, 0); + }); + + it("rejects malformed Redis eval envelopes", async () => { + for (const result of [null, {}, [], [1]]) { + const { store } = createStoreWithMock(undefined, createMockRedisClient(result)); + await assertRejects( + () => store.increment("key", 1_000), + Error, + "invalid result", + ); + } + }); + + it("rejects non-positive or unsafe counters", async () => { + for (const count of [0, -1, 1.5, Number.NaN, Number.MAX_SAFE_INTEGER + 1]) { + const { store } = createStoreWithMock( + undefined, + createMockRedisClient([count, 1_000]), + ); + await assertRejects( + () => store.increment("key", 1_000), + Error, + "invalid count", + ); + } + }); + + it("rejects unsafe TTL values", async () => { + for (const ttl of [1.5, Number.NaN, Number.MAX_SAFE_INTEGER + 1]) { + const { store } = createStoreWithMock( + undefined, + createMockRedisClient([1, ttl]), + ); + await assertRejects( + () => store.increment("key", 1_000), + Error, + "invalid TTL", + ); + } + }); + + it("bounds commands and retires a provider connection after timeout", async () => { + const client = createMockRedisClient(); + client.eval = () => new Promise(() => {}); + const { store, closeCalls } = createStoreWithMock( + { operationTimeoutMs: 1 }, + client, + ); + + const error = await assertRejects( + () => store.increment("key", 1_000), + Error, + "timed out", + ); + if (!(error instanceof Error)) throw new Error("Expected a timeout error"); + assertEquals(error.name, "TimeoutError"); + assertEquals(closeCalls(), 1); + }); + }); + + describe("reset", () => { + it("deletes the prefixed key", async () => { + const { store, client } = createStoreWithMock({ keyPrefix: "custom:" }); + await store.reset("user-1"); + + assertEquals(client._lastKey, "custom:user-1"); + assertEquals(client._delCalls, 1); + }); + + it("validates the key before opening a provider connection", async () => { + const { store, client, getClientCalls } = createStoreWithMock(); + await assertRejects( + () => store.reset("tenant\u0000member"), + TypeError, + "control characters", + ); + assertEquals(getClientCalls(), 0); + assertEquals(client._delCalls, 0); + }); + + it("bounds delete commands and retires the connection after timeout", async () => { + const client = createMockRedisClient(); + client.del = () => new Promise(() => {}); + const { store, closeCalls } = createStoreWithMock( + { operationTimeoutMs: 1 }, + client, + ); + + await assertRejects( + () => store.reset("key"), + Error, + "timed out", + ); + assertEquals(closeCalls(), 1); + }); + }); + + describe("destroy", () => { + it("closes its provider-owned connection", async () => { + const { store, closeCalls } = createStoreWithMock(); + await store.destroy(); + assertEquals(closeCalls(), 1); + }); + + it("is idempotent at the store boundary", async () => { + const { store, closeCalls } = createStoreWithMock(); + await store.destroy(); + await store.destroy(); + assertEquals(closeCalls(), 1); + }); + }); +}); diff --git a/src/middleware/builtin/security/redis-rate-limit.ts b/src/middleware/builtin/security/redis-rate-limit.ts index 67197f44ed..bed37ec7c9 100644 --- a/src/middleware/builtin/security/redis-rate-limit.ts +++ b/src/middleware/builtin/security/redis-rate-limit.ts @@ -1,37 +1,14 @@ -import { createError, toError } from "../../../errors/index.ts"; -import { serverLogger } from "../../../utils/logger/index.ts"; -import { MAX_TIMER_DELAY_MS } from "../../../utils/timer.ts"; +import { createError, toError } from "#veryfront/errors"; +import { OwnedRedisClientConnection } from "#veryfront/extensions/distributed/owned-redis-client.ts"; +import type { RedisClient } from "#veryfront/extensions/distributed"; +import { serverLogger } from "#veryfront/utils"; +import { MAX_TIMER_DELAY_MS } from "#veryfront/utils/timer.ts"; import { requireRateLimitKey, requireRateLimitWindowMs } from "./rate-limit-validation.ts"; import type { RateLimitEntry, RateLimitStore } from "./types.ts"; const logger = serverLogger.component("redis-ratelimit"); - -interface RedisClient { - connect(): Promise; - disconnect(): Promise; - eval( - script: string, - options: { keys: string[]; arguments: string[] }, - ): Promise; - del(key: string): Promise; - on?(event: string, listener: (...args: unknown[]) => void): void; -} - -interface RedisClientFactoryOptions { - url?: string; - socket: { - connectTimeout: number; - reconnectStrategy: false; - }; -} - -type RedisClientFactory = (options: RedisClientFactoryOptions) => RedisClient; -type RedisClientClosedErrorConstructor = new () => Error; - const DEFAULT_REDIS_CONNECT_TIMEOUT_MS = 5_000; const DEFAULT_REDIS_OPERATION_TIMEOUT_MS = 5_000; -const REDIS_MODULE_SPECIFIER = "npm:redis@5.11.0"; -let RedisClientClosedError: RedisClientClosedErrorConstructor | undefined; const INCREMENT_WITH_TTL_SCRIPT = ` local count = redis.call("INCR", KEYS[1]) @@ -43,45 +20,35 @@ end return { count, ttl } `; -/** Options accepted by Redis rate limit. */ +/** Options accepted by the provider-backed Redis rate-limit store. */ export interface RedisRateLimitOptions { url?: string; keyPrefix?: string; - /** Maximum time allowed for loading and connecting the Redis client. */ + /** Maximum time allowed for opening the extension-provided Redis client. */ connectTimeoutMs?: number; /** Maximum time allowed for an individual Redis command. */ operationTimeoutMs?: number; } -/** Create a Redis rate limit store. */ +/** + * Redis rate-limit store backed by the registered Redis runtime provider. + * + * Core owns only the stable rate-limit facade. The Redis extension owns the + * third-party client package, connections, and transport lifecycle. + */ export class RedisRateLimitStore implements RateLimitStore { - private client: RedisClient | null = null; - private connectingClient: RedisClient | null = null; - private clientPromise: Promise | null = null; - private cancelPendingConnection: (() => void) | null = null; - private clientGeneration = 0; - private readonly disconnectPromises = new WeakMap>(); - private readonly disconnectedClients = new WeakSet(); - private readonly pendingDisconnectClients = new Set(); - private readonly reportedClientErrors = new WeakSet(); - private readonly url?: string; + private readonly connection: OwnedRedisClientConnection; private readonly keyPrefix: string; - private readonly connectTimeoutMs: number; private readonly operationTimeoutMs: number; constructor(options: RedisRateLimitOptions = {}) { - if ( - typeof options !== "object" || - options === null || - Array.isArray(options) - ) { + if (typeof options !== "object" || options === null || Array.isArray(options)) { throw new TypeError("Redis rate limit options must be an object"); } if (options.url !== undefined && typeof options.url !== "string") { throw new TypeError("Redis rate limit url must be a string"); } - this.url = options.url; - this.connectTimeoutMs = requireTimeoutMs( + const connectTimeoutMs = requireTimeoutMs( options.connectTimeoutMs ?? DEFAULT_REDIS_CONNECT_TIMEOUT_MS, "connectTimeoutMs", ); @@ -93,241 +60,81 @@ export class RedisRateLimitStore implements RateLimitStore { options.keyPrefix ?? "veryfront:ratelimit:", "Redis rate limit keyPrefix", ); - } - - private ensureClient(): Promise { - if (this.client) return Promise.resolve(this.client); - if (this.clientPromise) return this.clientPromise; - - const generation = this.clientGeneration; - const pending = this.connectClient(generation).finally(() => { - if (this.clientPromise === pending) this.clientPromise = null; - }); - this.clientPromise = pending; - return pending; - } - - private invalidateClient( - client: RedisClient, - generation: number, - disconnect: boolean, - ): void { - if (generation !== this.clientGeneration) return; - if (this.client !== client) return; - - this.clientGeneration++; - this.client = null; - this.clientPromise = null; - - if (disconnect) { - void this.disconnectBestEffort(client); - } - } - - private attachClientLifecycleHandlers( - client: RedisClient, - generation = this.clientGeneration, - ): void { - client.on?.("error", (err: unknown) => { - if (generation !== this.clientGeneration) return; - if (!this.reportedClientErrors.has(client)) { - this.reportedClientErrors.add(client); - logger.error("client error", { - errorName: err instanceof Error ? err.name : typeof err, - }); - } - this.invalidateClient(client, generation, true); - }); - - client.on?.("end", () => { - this.invalidateClient(client, generation, false); - }); - } - - private async loadClientFactory(): Promise { - const redis = await import(REDIS_MODULE_SPECIFIER); - RedisClientClosedError = redis.ClientClosedError as RedisClientClosedErrorConstructor; - return redis.createClient as unknown as RedisClientFactory; - } - - private async disconnectBestEffort(client: RedisClient): Promise { - try { - await this.disconnectClient(client); - } catch (error) { - if (isAlreadyClosedClientError(error)) { - this.markDisconnected(client); - return; - } - logger.warn("client disconnect failed", { - errorName: error instanceof Error ? error.name : typeof error, - }); - } - } - - private disconnectClient(client: RedisClient): Promise { - if (this.disconnectedClients.has(client)) return Promise.resolve(); - const existing = this.disconnectPromises.get(client); - if (existing) return existing; - - let resolveDisconnect!: () => void; - let rejectDisconnect!: (reason: unknown) => void; - const pending = new Promise((resolve, reject) => { - resolveDisconnect = resolve; - rejectDisconnect = reject; - }); - this.disconnectPromises.set(client, pending); - this.pendingDisconnectClients.add(client); - - try { - Promise.resolve(client.disconnect()).then( - () => { - this.markDisconnected(client); - resolveDisconnect(); + this.connection = new OwnedRedisClientConnection( + { + ...(options.url === undefined ? {} : { url: options.url }), + connectTimeout: connectTimeoutMs, + autoReconnect: false, + }, + { + onError(error) { + logger.error("client error", { + errorName: error instanceof Error ? error.name : typeof error, + }); }, - (error) => { - this.disconnectPromises.delete(client); - if (isAlreadyClosedClientError(error)) { - this.markDisconnected(client); - resolveDisconnect(); - return; - } - rejectDisconnect(error); + onCloseError(error) { + logger.error("client close failed", { + errorName: error instanceof Error ? error.name : typeof error, + }); }, - ); - } catch (error) { - this.disconnectPromises.delete(client); - if (isAlreadyClosedClientError(error)) { - this.markDisconnected(client); - resolveDisconnect(); - } else { - rejectDisconnect(error); - } - } + }, + ); + } - return pending; + private ensureClient(): Promise { + return this.connection.getClient(); } - private markDisconnected(client: RedisClient): void { - this.disconnectPromises.delete(client); - this.pendingDisconnectClients.delete(client); - this.disconnectedClients.add(client); + private storageKey(key: string): string { + return `${this.keyPrefix}${key}`; } - private async withTimeout( + private async withOperationTimeout( operation: Promise, - timeoutMs: number, operationName: string, - cancellation?: Promise, ): Promise { let timeoutId: ReturnType | undefined; const timeout = new Promise((_, reject) => { - timeoutId = setTimeout(() => { - reject(createTimeoutError(operationName, timeoutMs)); - }, timeoutMs); - }); - - try { - return await Promise.race( - cancellation === undefined ? [operation, timeout] : [operation, timeout, cancellation], + timeoutId = setTimeout( + () => reject(createTimeoutError(operationName, this.operationTimeoutMs)), + this.operationTimeoutMs, ); - } finally { - if (timeoutId !== undefined) clearTimeout(timeoutId); - } - } - - private async connectClient(generation: number): Promise { - const superseded = new Error( - "Redis rate limit client connection was superseded", - ); - superseded.name = "AbortError"; - let cancelConnection!: () => void; - let cancelled = false; - const cancellation = new Promise((_, reject) => { - cancelConnection = () => { - if (cancelled) return; - cancelled = true; - reject(superseded); - }; }); - this.cancelPendingConnection = cancelConnection; - let client: RedisClient | undefined; try { - const createClient = await this.withTimeout( - this.loadClientFactory(), - this.connectTimeoutMs, - "client loading", - cancellation, - ); - if (generation !== this.clientGeneration) throw superseded; - - client = createClient({ - ...(this.url === undefined ? {} : { url: this.url }), - socket: { - connectTimeout: this.connectTimeoutMs, - reconnectStrategy: false, - }, - }); - this.connectingClient = client; - this.attachClientLifecycleHandlers(client, generation); - await this.withTimeout( - client.connect(), - this.connectTimeoutMs, - "connection", - cancellation, - ); - - if (this.connectingClient === client) this.connectingClient = null; - if (generation !== this.clientGeneration) { - await this.disconnectBestEffort(client); - throw superseded; - } - - this.client = client; - return client; + return await Promise.race([operation, timeout]); } catch (error) { - if (client && this.connectingClient === client) { - this.connectingClient = null; + if (isTimeoutError(error)) { + // Retire the timed-out provider-owned connection before another + // operation can reuse it. A close failure stays observable on the next + // getClient()/destroy() attempt instead of silently reopening. + void this.connection.close().catch((closeError) => { + logger.error("timed-out client close failed", { + errorName: closeError instanceof Error ? closeError.name : typeof closeError, + }); + }); } - if (generation === this.clientGeneration) this.clientGeneration++; - if (client) await this.disconnectBestEffort(client); throw error; } finally { - if (this.cancelPendingConnection === cancelConnection) { - this.cancelPendingConnection = null; - } + if (timeoutId !== undefined) clearTimeout(timeoutId); } } - private storageKey(key: string): string { - return `${this.keyPrefix}${key}`; - } - async increment(key: string, windowMs: number): Promise { const normalizedKey = requireRateLimitKey(key); const normalizedWindowMs = requireRateLimitWindowMs(windowMs); const client = await this.ensureClient(); - const generation = this.clientGeneration; const redisKey = this.storageKey(normalizedKey); - let result: unknown; - try { - result = await this.withTimeout( + const [count, pttl] = parseIncrementResult( + await this.withOperationTimeout( client.eval(INCREMENT_WITH_TTL_SCRIPT, { keys: [redisKey], arguments: [String(normalizedWindowMs)], }), - this.operationTimeoutMs, "increment", - ); - } catch (error) { - if (isTimeoutError(error)) { - this.invalidateClient(client, generation, true); - } - throw error; - } - - const [count, pttl] = parseIncrementResult(result); + ), + ); const ttl = pttl > 0 ? requireRateLimitWindowMs(pttl) : normalizedWindowMs; return { count, resetAt: Date.now() + ttl }; } @@ -335,48 +142,14 @@ export class RedisRateLimitStore implements RateLimitStore { async reset(key: string): Promise { const normalizedKey = requireRateLimitKey(key); const client = await this.ensureClient(); - const generation = this.clientGeneration; - try { - await this.withTimeout( - client.del(this.storageKey(normalizedKey)), - this.operationTimeoutMs, - "reset", - ); - } catch (error) { - if (isTimeoutError(error)) { - this.invalidateClient(client, generation, true); - } - throw error; - } + await this.withOperationTimeout( + client.del(this.storageKey(normalizedKey)).then(() => undefined), + "reset", + ); } async destroy(): Promise { - const client = this.client; - const connectingClient = this.connectingClient; - const pending = this.clientPromise; - pending?.catch(() => {}); - const cancelPendingConnection = this.cancelPendingConnection; - const clientsToDisconnect = new Set(this.pendingDisconnectClients); - if (client) clientsToDisconnect.add(client); - if (connectingClient) clientsToDisconnect.add(connectingClient); - this.clientGeneration++; - this.client = null; - this.connectingClient = null; - this.clientPromise = null; - this.cancelPendingConnection = null; - cancelPendingConnection?.(); - - let disconnectFailed = false; - let disconnectError: unknown; - await Promise.all( - [...clientsToDisconnect].map((clientToDisconnect) => - this.disconnectClient(clientToDisconnect).catch((error: unknown) => { - disconnectFailed = true; - disconnectError ??= error; - }) - ), - ); - if (disconnectFailed) throw disconnectError; + await this.connection.close(); } } @@ -406,18 +179,6 @@ function isTimeoutError(error: unknown): boolean { return error instanceof Error && error.name === "TimeoutError"; } -function isAlreadyClosedClientError(error: unknown): boolean { - if (!(error instanceof Error)) return false; - if ( - RedisClientClosedError !== undefined && - error instanceof RedisClientClosedError - ) { - return true; - } - return error.constructor.name === "ClientClosedError" && - error.message === "The client is closed"; -} - function parseIncrementResult(result: unknown): [number, number] { if (!Array.isArray(result) || result.length < 2) { throw toError( @@ -439,7 +200,6 @@ function parseIncrementResult(result: unknown): [number, number] { }), ); } - if (!Number.isSafeInteger(ttl)) { throw toError( createError({ From 985eb2ced5997815ebbd0992da2d81f173f63113 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 12:28:46 +0200 Subject: [PATCH 12/17] Document the actual capacity log signal The API reference named a store-capacity log stage that the middleware does not emit. The implementation and tests use the store-increment stage with capacity-exhausted failure kind and a capacity field, so the public docs now match the structured signal operators will see. Constraint: Keep PR #3304 follow-up scoped to the reviewed documentation mismatch Rejected: Update implementation logging | the reviewed head already emits the intended structured fields Confidence: high Scope-risk: narrow Reversibility: clean Directive: Do not reintroduce store-capacity unless the middleware emits that exact stage Tested: deno task docs:validate Tested: deno test --no-check --allow-all src/middleware/builtin/security/rate-limit.test.ts Tested: deno task docs generation checked; broad generated-reference churn was intentionally not committed Tested: git diff --check --- docs/api-reference/veryfront/middleware.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/api-reference/veryfront/middleware.md b/docs/api-reference/veryfront/middleware.md index 0226b31693..8751c06ebd 100644 --- a/docs/api-reference/veryfront/middleware.md +++ b/docs/api-reference/veryfront/middleware.md @@ -123,7 +123,7 @@ Options accepted by the in-memory rate limit store. | Property | Type | Description | Source | |----------|------|-------------|--------| -| `maxEntries?` | `number` | Maximum number of active identities retained by the store. Size this above the expected concurrent identities in one rate-limit window. New identities fail closed when all entries are active; existing identities remain tracked until their windows expire. Capacity exhaustion emits a `store-capacity` log stage with the configured `maxEntries`. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L137) | +| `maxEntries?` | `number` | Maximum number of active identities retained by the store. Size this above the expected concurrent identities in one rate-limit window. New identities fail closed when all entries are active; existing identities remain tracked until their windows expire. Capacity exhaustion logs `stage=store-increment`, `failureKind=capacity-exhausted`, and `capacity` set to the configured `maxEntries`. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L137) | ### `LoggerOptions` From 8975e1d70b71eb622efc7efbe05a7f4f491282b9 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 12:50:06 +0200 Subject: [PATCH 13/17] Keep rate-limit diagnostics stable at public boundaries Review found that malformed options leaked native property errors and operational logs erased backend error classifications. Validate both exported constructors and preserve safe Error names in throttled failure logs. Constraint: Rate-limit failures must remain fail closed and must not expose error messages. Confidence: high Scope-risk: narrow Tested: Focused middleware and WebSocket limiter tests, deno check, format check, and diff check. --- .../builtin/security/rate-limit.test.ts | 18 +++++++++++++++++- src/middleware/builtin/security/rate-limit.ts | 9 ++++----- src/modules/server/rate-limiter.test.ts | 8 ++++++++ src/modules/server/rate-limiter.ts | 3 +++ 4 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/middleware/builtin/security/rate-limit.test.ts b/src/middleware/builtin/security/rate-limit.test.ts index 6dfb654700..0215de50cd 100644 --- a/src/middleware/builtin/security/rate-limit.test.ts +++ b/src/middleware/builtin/security/rate-limit.test.ts @@ -102,6 +102,14 @@ describe("MemoryRateLimitStore", () => { } }); + it("rejects invalid store options with a stable error", () => { + assertThrows( + () => new MemoryRateLimitStore(60000, null as never), + TypeError, + "options", + ); + }); + it("should release retained entries when destroyed", async () => { const boundedStore = new MemoryRateLimitStore(60000, { maxEntries: 1 }); await boundedStore.increment("first", 60000); @@ -449,7 +457,11 @@ describe("rateLimit middleware", () => { }); const storeFailure = rateLimit({ store: { - increment: () => Promise.reject(new Error("backend unavailable")), + increment: () => { + const error = new Error("backend unavailable"); + error.name = "BackendUnavailableError"; + return Promise.reject(error); + }, reset: () => Promise.resolve(), }, }); @@ -480,6 +492,10 @@ describe("rateLimit middleware", () => { "key-resolution", "store-unavailable", ]); + assertEquals(records.map((record) => record.context?.errorName), [ + "Error", + "BackendUnavailableError", + ]); }); it("should emit a capacity-specific store failure signal", async () => { diff --git a/src/middleware/builtin/security/rate-limit.ts b/src/middleware/builtin/security/rate-limit.ts index 63b0dae020..d2b3bc1003 100644 --- a/src/middleware/builtin/security/rate-limit.ts +++ b/src/middleware/builtin/security/rate-limit.ts @@ -51,6 +51,9 @@ export class MemoryRateLimitStore implements RateLimitStore { windowMs: number, options: MemoryRateLimitStoreOptions = {}, ) { + if (!options || typeof options !== "object" || Array.isArray(options)) { + throw new TypeError("Memory rate limit store options must be an object"); + } const normalizedWindowMs = requireRateLimitWindowMs(windowMs); const maxEntries = options.maxEntries ?? DEFAULT_MEMORY_RATE_LIMIT_MAX_ENTRIES; @@ -299,11 +302,7 @@ export function rateLimit( logger.error(message, { failureKind, stage, - errorName: error instanceof MemoryRateLimitCapacityError - ? error.name - : error instanceof Error - ? "Error" - : typeof error, + errorName: error instanceof Error ? error.name : typeof error, ...(error instanceof MemoryRateLimitCapacityError ? { capacity: error.capacity } : {}), }); } diff --git a/src/modules/server/rate-limiter.test.ts b/src/modules/server/rate-limiter.test.ts index e9fe3382ce..63b2460189 100644 --- a/src/modules/server/rate-limiter.test.ts +++ b/src/modules/server/rate-limiter.test.ts @@ -74,6 +74,14 @@ describe("modules/server/rate-limiter", () => { } }); + it("rejects invalid options with a stable error", () => { + assertThrows( + () => new RateLimiter(1, null as never), + TypeError, + "options", + ); + }); + it("fails closed when the clock returns a non-finite value", () => { const limiter = new RateLimiter(1, { now: () => Number.NaN }); assertEquals(limiter.check(mockSocket()), false); diff --git a/src/modules/server/rate-limiter.ts b/src/modules/server/rate-limiter.ts index ea5ab79a97..1f13b54ba0 100644 --- a/src/modules/server/rate-limiter.ts +++ b/src/modules/server/rate-limiter.ts @@ -20,6 +20,9 @@ export class RateLimiter { private readonly now: () => number; constructor(maxMessages: number, options: RateLimiterOptions = {}) { + if (!options || typeof options !== "object" || Array.isArray(options)) { + throw new TypeError("Rate limiter options must be an object"); + } if (!Number.isSafeInteger(maxMessages) || maxMessages <= 0) { throw new RangeError("maxMessages must be a positive safe integer"); } From 24218e298bafcc90aba8c9285bf5f36aa24dda8c Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 13:23:05 +0200 Subject: [PATCH 14/17] Make rate-limit timeout retirement depend on registered identity Redis operation timeouts now use the shared timeout error definition, so connection retirement is driven by the Veryfront error slug instead of an arbitrary Error.name. The rate-limit store contract and maxRequests boundary are tightened at the public preset boundary, with tests pinned to the shared key length constant. Constraint: Review requested patch-only fixes for PR #3304 without widening the consolidation branch Rejected: Match TimeoutError by name | unrelated provider errors can share that name and should not retire a healthy client Confidence: high Scope-risk: narrow Tested: focused rate-limit suites; deno fmt --check touched files; deno lint touched files; deno check touched files; deno task test:unit Not-tested: integration suites --- .../ext-redis/src/rate-limit-store.test.ts | 41 +++++++++++++++---- extensions/ext-redis/src/rate-limit-store.ts | 12 +++--- .../distributed/rate-limit-support.ts | 1 + .../builtin/security/rate-limit.test.ts | 31 ++++++++++++-- src/middleware/builtin/security/rate-limit.ts | 26 +++++++----- .../builtin/security/redis-rate-limit.test.ts | 30 ++++++++++++-- .../builtin/security/redis-rate-limit.ts | 12 +++--- 7 files changed, 113 insertions(+), 40 deletions(-) diff --git a/extensions/ext-redis/src/rate-limit-store.test.ts b/extensions/ext-redis/src/rate-limit-store.test.ts index c01324868a..3cd00c4d70 100644 --- a/extensions/ext-redis/src/rate-limit-store.test.ts +++ b/extensions/ext-redis/src/rate-limit-store.test.ts @@ -1,6 +1,8 @@ import "#veryfront/schemas/_test-setup.ts"; import { assertEquals, assertRejects, assertThrows } from "#veryfront/testing/assert.ts"; import { describe, it } from "#veryfront/testing/bdd.ts"; +import { isVeryfrontError, TIMEOUT_ERROR } from "veryfront/errors"; +import { MAX_RATE_LIMIT_KEY_LENGTH } from "veryfront/extensions/distributed/rate-limit-support"; import { ClientClosedError } from "redis"; import { type RedisRateLimitOptions, RedisRateLimitStore } from "./index.ts"; @@ -161,7 +163,7 @@ describe("middleware/builtin/security/redis-rate-limit", () => { assertThrows( () => new RedisRateLimitStore({ - keyPrefix: "x".repeat(1025), + keyPrefix: "x".repeat(MAX_RATE_LIMIT_KEY_LENGTH + 1), }), RangeError, "1024", @@ -254,7 +256,7 @@ describe("middleware/builtin/security/redis-rate-limit", () => { const { rateStore, mockClient } = createStoreWithMock(); await assertRejects( - () => rateStore.increment("x".repeat(1025), 1000), + () => rateStore.increment("x".repeat(MAX_RATE_LIMIT_KEY_LENGTH + 1), 1000), RangeError, "1024", ); @@ -290,16 +292,39 @@ describe("middleware/builtin/security/redis-rate-limit", () => { }); mockClient.eval = () => new Promise(() => {}); - const outcome = await outcomeWithin( - rateStore.increment("key", 1000), - 50, + const error = await assertRejects( + () => rateStore.increment("key", 1000), + Error, + "timed out", ); - assertEquals(outcome, "rejected"); + assertEquals(isVeryfrontError(error), true); + assertEquals(isVeryfrontError(error) ? error.slug : undefined, TIMEOUT_ERROR.slug); assertEquals(mockClient._disconnectCalls, 1); // deno-lint-ignore no-explicit-any assertEquals((rateStore as any).client, null); }); + + it("does not retire a client for an unrelated TimeoutError name", async () => { + const { rateStore, mockClient } = createStoreWithMock(); + mockClient.eval = () => { + const error = new Error("foreign timeout"); + error.name = "TimeoutError"; + return Promise.reject(error); + }; + + const error = await assertRejects( + () => rateStore.increment("key", 1000), + Error, + "foreign timeout", + ); + + if (!(error instanceof Error)) throw new Error("Expected Redis client error"); + assertEquals(error.name, "TimeoutError"); + assertEquals(mockClient._disconnectCalls, 0); + // deno-lint-ignore no-explicit-any + assertEquals((rateStore as any).client, mockClient); + }); }); describe("reset", () => { @@ -327,7 +352,7 @@ describe("middleware/builtin/security/redis-rate-limit", () => { }; await assertRejects( - () => rateStore.reset("x".repeat(1025)), + () => rateStore.reset("x".repeat(MAX_RATE_LIMIT_KEY_LENGTH + 1)), RangeError, "1024", ); @@ -403,7 +428,7 @@ describe("middleware/builtin/security/redis-rate-limit", () => { (rateStore as any).loadClientFactory = () => Promise.resolve(() => mockClient); await assertRejects( - () => rateStore.reset("x".repeat(1025)), + () => rateStore.reset("x".repeat(MAX_RATE_LIMIT_KEY_LENGTH + 1)), RangeError, "1024", ); diff --git a/extensions/ext-redis/src/rate-limit-store.ts b/extensions/ext-redis/src/rate-limit-store.ts index adc3c4f44f..228a6dbdb0 100644 --- a/extensions/ext-redis/src/rate-limit-store.ts +++ b/extensions/ext-redis/src/rate-limit-store.ts @@ -1,4 +1,4 @@ -import { createError, toError } from "veryfront/errors"; +import { createError, isVeryfrontError, TIMEOUT_ERROR, toError } from "veryfront/errors"; import { serverLogger } from "veryfront/utils/logger"; import { ClientClosedError, createClient } from "redis"; import { @@ -400,15 +400,13 @@ function requireTimeoutMs(value: unknown, name: string): number { } function createTimeoutError(operationName: string, timeoutMs: number): Error { - const error = new Error( - `Redis rate limit ${operationName} timed out after ${timeoutMs}ms`, - ); - error.name = "TimeoutError"; - return error; + return TIMEOUT_ERROR.create({ + detail: `Redis rate limit ${operationName} timed out after ${timeoutMs}ms`, + }); } function isTimeoutError(error: unknown): boolean { - return error instanceof Error && error.name === "TimeoutError"; + return isVeryfrontError(error) && error.slug === TIMEOUT_ERROR.slug; } function isAlreadyClosedClientError(error: unknown): boolean { diff --git a/src/extensions/distributed/rate-limit-support.ts b/src/extensions/distributed/rate-limit-support.ts index 0e6fd54fac..cb26e7da6a 100644 --- a/src/extensions/distributed/rate-limit-support.ts +++ b/src/extensions/distributed/rate-limit-support.ts @@ -5,6 +5,7 @@ export type { RateLimitStore, } from "#veryfront/middleware/builtin/security/types.ts"; export { + MAX_RATE_LIMIT_KEY_LENGTH, requireRateLimitKey, requireRateLimitWindowMs, } from "#veryfront/middleware/builtin/security/rate-limit-validation.ts"; diff --git a/src/middleware/builtin/security/rate-limit.test.ts b/src/middleware/builtin/security/rate-limit.test.ts index 0215de50cd..8cc54639ef 100644 --- a/src/middleware/builtin/security/rate-limit.test.ts +++ b/src/middleware/builtin/security/rate-limit.test.ts @@ -11,6 +11,7 @@ import { scaleMs } from "#veryfront/testing/timing.ts"; import { deleteEnv, getHostEnv, setEnv } from "#veryfront/platform/compat/process.ts"; import { __subscribeLogRecordEmitter, type LogEntry } from "#veryfront/utils/logger/index.ts"; import { MiddlewareContext } from "../../core/context.ts"; +import { MAX_RATE_LIMIT_KEY_LENGTH } from "./rate-limit-validation.ts"; import { authRateLimit, MemoryRateLimitStore, @@ -217,10 +218,19 @@ describe("rateLimit middleware", () => { }); it("should validate numeric configuration before creating middleware", () => { - for (const maxRequests of [-1, 1.5, Number.NaN, Number.POSITIVE_INFINITY]) { + for ( + const maxRequests of [ + -1, + 1.5, + Number.NaN, + Number.MAX_SAFE_INTEGER, + Number.POSITIVE_INFINITY, + ] + ) { assertThrows( () => rateLimit({ maxRequests }), RangeError, + "between 0", ); } @@ -335,7 +345,9 @@ describe("rateLimit middleware", () => { }; try { - const keyFailure = rateLimit({ keyGenerator: () => "x".repeat(1_025) }); + const keyFailure = rateLimit({ + keyGenerator: () => "x".repeat(MAX_RATE_LIMIT_KEY_LENGTH + 1), + }); const storeFailure = rateLimit({ store: { increment: () => Promise.reject(new Error("unavailable")), @@ -396,7 +408,7 @@ describe("rateLimit middleware", () => { it("should fail closed when custom keys are invalid without calling the store", async () => { let incrementCalled = false; const middleware = rateLimit({ - keyGenerator: () => "x".repeat(1025), + keyGenerator: () => "x".repeat(MAX_RATE_LIMIT_KEY_LENGTH + 1), store: { increment: () => { incrementCalled = true; @@ -430,7 +442,7 @@ describe("rateLimit middleware", () => { }); const response = await middleware( - createContext("x".repeat(1025)), + createContext("x".repeat(MAX_RATE_LIMIT_KEY_LENGTH + 1)), () => Promise.resolve(new Response("OK")), ); @@ -627,6 +639,17 @@ describe("rateLimit middleware", () => { } }); + it("should require direct auth preset stores to implement reset", () => { + assertThrows( + () => + authRateLimit({ + increment: () => Promise.resolve({ count: 1, resetAt: Date.now() + 1000 }), + } as never), + TypeError, + "increment() and reset()", + ); + }); + it("should separate trusted proxy clients in the auth preset", async () => { const middleware = authRateLimit({ trustProxy: true }); diff --git a/src/middleware/builtin/security/rate-limit.ts b/src/middleware/builtin/security/rate-limit.ts index d2b3bc1003..146ca57955 100644 --- a/src/middleware/builtin/security/rate-limit.ts +++ b/src/middleware/builtin/security/rate-limit.ts @@ -181,18 +181,21 @@ function isRateLimitStore(value: unknown): value is RateLimitStore { return ( value != null && typeof value === "object" && - "increment" in value && - typeof value.increment === "function" + typeof (value as Partial).increment === "function" && + typeof (value as Partial).reset === "function" + ); +} + +function hasRateLimitStoreMethod(value: unknown): boolean { + return ( + value != null && + typeof value === "object" && + ("increment" in value || "reset" in value) ); } function requireRateLimitStore(value: unknown): RateLimitStore { - if ( - value === null || - typeof value !== "object" || - typeof (value as Partial).increment !== "function" || - typeof (value as Partial).reset !== "function" - ) { + if (!isRateLimitStore(value)) { throw new TypeError( "Rate limit store must implement increment() and reset()", ); @@ -204,10 +207,11 @@ function requireMaxRequests(value: unknown): number { if ( typeof value !== "number" || !Number.isSafeInteger(value) || - value < 0 + value < 0 || + value >= Number.MAX_SAFE_INTEGER ) { throw new RangeError( - "Rate limit maxRequests must be a non-negative safe integer", + `Rate limit maxRequests must be an integer between 0 and ${Number.MAX_SAFE_INTEGER - 1}`, ); } return value; @@ -334,6 +338,8 @@ export function authRateLimit( ? {} : isRateLimitStore(storeOrOptions) ? { store: storeOrOptions } + : hasRateLimitStoreMethod(storeOrOptions) + ? { store: requireRateLimitStore(storeOrOptions) } : storeOrOptions; return rateLimit({ diff --git a/src/middleware/builtin/security/redis-rate-limit.test.ts b/src/middleware/builtin/security/redis-rate-limit.test.ts index c8376396e4..8db0b7753e 100644 --- a/src/middleware/builtin/security/redis-rate-limit.test.ts +++ b/src/middleware/builtin/security/redis-rate-limit.test.ts @@ -1,6 +1,8 @@ import "#veryfront/schemas/_test-setup.ts"; import { assertEquals, assertRejects, assertThrows } from "#veryfront/testing/assert.ts"; import { describe, it } from "#veryfront/testing/bdd.ts"; +import { isVeryfrontError, TIMEOUT_ERROR } from "#veryfront/errors"; +import { MAX_RATE_LIMIT_KEY_LENGTH } from "./rate-limit-validation.ts"; import { type RedisRateLimitOptions, RedisRateLimitStore } from "./redis-rate-limit.ts"; interface MockRedisClient { @@ -111,7 +113,7 @@ describe("provider-backed RedisRateLimitStore", () => { "url", ); assertThrows( - () => new RedisRateLimitStore({ keyPrefix: "x".repeat(1_025) }), + () => new RedisRateLimitStore({ keyPrefix: "x".repeat(MAX_RATE_LIMIT_KEY_LENGTH + 1) }), RangeError, "1024", ); @@ -165,7 +167,7 @@ describe("provider-backed RedisRateLimitStore", () => { const { store, client, getClientCalls } = createStoreWithMock(); await assertRejects( - () => store.increment("x".repeat(1_025), 1_000), + () => store.increment("x".repeat(MAX_RATE_LIMIT_KEY_LENGTH + 1), 1_000), RangeError, "1024", ); @@ -230,10 +232,30 @@ describe("provider-backed RedisRateLimitStore", () => { Error, "timed out", ); - if (!(error instanceof Error)) throw new Error("Expected a timeout error"); - assertEquals(error.name, "TimeoutError"); + assertEquals(isVeryfrontError(error), true); + assertEquals(isVeryfrontError(error) ? error.slug : undefined, TIMEOUT_ERROR.slug); assertEquals(closeCalls(), 1); }); + + it("does not retire a provider connection for an unrelated TimeoutError name", async () => { + const client = createMockRedisClient(); + client.eval = () => { + const error = new Error("foreign timeout"); + error.name = "TimeoutError"; + return Promise.reject(error); + }; + const { store, closeCalls } = createStoreWithMock(undefined, client); + + const error = await assertRejects( + () => store.increment("key", 1_000), + Error, + "foreign timeout", + ); + + if (!(error instanceof Error)) throw new Error("Expected Redis client error"); + assertEquals(error.name, "TimeoutError"); + assertEquals(closeCalls(), 0); + }); }); describe("reset", () => { diff --git a/src/middleware/builtin/security/redis-rate-limit.ts b/src/middleware/builtin/security/redis-rate-limit.ts index bed37ec7c9..6ae84ceb81 100644 --- a/src/middleware/builtin/security/redis-rate-limit.ts +++ b/src/middleware/builtin/security/redis-rate-limit.ts @@ -1,4 +1,4 @@ -import { createError, toError } from "#veryfront/errors"; +import { createError, isVeryfrontError, TIMEOUT_ERROR, toError } from "#veryfront/errors"; import { OwnedRedisClientConnection } from "#veryfront/extensions/distributed/owned-redis-client.ts"; import type { RedisClient } from "#veryfront/extensions/distributed"; import { serverLogger } from "#veryfront/utils"; @@ -168,15 +168,13 @@ function requireTimeoutMs(value: unknown, name: string): number { } function createTimeoutError(operationName: string, timeoutMs: number): Error { - const error = new Error( - `Redis rate limit ${operationName} timed out after ${timeoutMs}ms`, - ); - error.name = "TimeoutError"; - return error; + return TIMEOUT_ERROR.create({ + detail: `Redis rate limit ${operationName} timed out after ${timeoutMs}ms`, + }); } function isTimeoutError(error: unknown): boolean { - return error instanceof Error && error.name === "TimeoutError"; + return isVeryfrontError(error) && error.slug === TIMEOUT_ERROR.slug; } function parseIncrementResult(result: unknown): [number, number] { From e052a5d4930df157d44b2563f9ebbae88029c492 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 13:41:02 +0200 Subject: [PATCH 15/17] Release Redis rate-limit timeout timers from process liveness Redis operation timeouts enforce bounded backend calls, but the one-shot timeout handles should not keep Node-compatible runtimes alive after other work has drained. Core and extension Redis stores now use the shared unrefTimer path, with timeout tests proving the timer handle is unreferenced while preserving timeout rejection behavior under Deno's event-loop semantics. The memory store documentation now distinguishes direct store behavior from middleware logging so generated docs do not imply MemoryRateLimitStore itself emits rateLimit request-path logs. Constraint: PR review requested unrefTimer-compatible timeout handles in both core and extension Redis stores Constraint: Generated middleware reference must not attribute rateLimit middleware logging to direct MemoryRateLimitStore use Rejected: Direct extension import from platform/compat/process | would bypass the existing distributed rate-limit support surface Confidence: high Scope-risk: narrow Tested: npx --yes deno@2.7.7 test --no-check --allow-all src/middleware/builtin/security/rate-limit.test.ts src/middleware/builtin/security/redis-rate-limit.test.ts Tested: npx --yes deno@2.7.7 test --no-lock --allow-all extensions/ext-redis/src/rate-limit-store.test.ts Tested: npx --yes deno@2.7.7 fmt --check src/middleware/builtin/security/rate-limit.ts src/middleware/builtin/security/redis-rate-limit.ts src/middleware/builtin/security/redis-rate-limit.test.ts src/extensions/distributed/rate-limit-support.ts extensions/ext-redis/src/rate-limit-store.ts extensions/ext-redis/src/rate-limit-store.test.ts Tested: npx --yes deno@2.7.7 lint src/middleware/builtin/security/rate-limit.ts src/middleware/builtin/security/redis-rate-limit.ts src/middleware/builtin/security/redis-rate-limit.test.ts src/extensions/distributed/rate-limit-support.ts extensions/ext-redis/src/rate-limit-store.ts extensions/ext-redis/src/rate-limit-store.test.ts Tested: npx --yes deno@2.7.7 check src/middleware/builtin/security/rate-limit.ts src/middleware/builtin/security/redis-rate-limit.test.ts src/extensions/distributed/rate-limit-support.ts Tested: npx --yes deno@2.7.7 check --no-lock extensions/ext-redis/src/rate-limit-store.test.ts Tested: npx --yes deno@2.7.7 task docs:validate Tested: npx --yes deno@2.7.7 task lint:core-deps Tested: npx --yes deno@2.7.7 task lint:dependency-boundaries Tested: npx --yes deno@2.7.7 task lint:extension-contracts --- docs/api-reference/veryfront/middleware.md | 2 +- .../ext-redis/src/rate-limit-store.test.ts | 72 ++++++++++++++++- extensions/ext-redis/src/rate-limit-store.ts | 2 + .../distributed/rate-limit-support.ts | 1 + src/middleware/builtin/security/rate-limit.ts | 4 +- .../builtin/security/redis-rate-limit.test.ts | 81 +++++++++++++++++-- .../builtin/security/redis-rate-limit.ts | 2 + 7 files changed, 150 insertions(+), 14 deletions(-) diff --git a/docs/api-reference/veryfront/middleware.md b/docs/api-reference/veryfront/middleware.md index 8751c06ebd..cb85bf5a77 100644 --- a/docs/api-reference/veryfront/middleware.md +++ b/docs/api-reference/veryfront/middleware.md @@ -123,7 +123,7 @@ Options accepted by the in-memory rate limit store. | Property | Type | Description | Source | |----------|------|-------------|--------| -| `maxEntries?` | `number` | Maximum number of active identities retained by the store. Size this above the expected concurrent identities in one rate-limit window. New identities fail closed when all entries are active; existing identities remain tracked until their windows expire. Capacity exhaustion logs `stage=store-increment`, `failureKind=capacity-exhausted`, and `capacity` set to the configured `maxEntries`. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L137) | +| `maxEntries?` | `number` | Maximum number of active identities retained by the store. Size this above the expected concurrent identities in one rate-limit window. New identities fail closed when all entries are active; existing identities remain tracked until their windows expire. When used through `rateLimit()`, capacity exhaustion logs `stage=store-increment`, `failureKind=capacity-exhausted`, and `capacity` set to the configured `maxEntries`. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/rate-limit.ts#L137) | ### `LoggerOptions` diff --git a/extensions/ext-redis/src/rate-limit-store.test.ts b/extensions/ext-redis/src/rate-limit-store.test.ts index 3cd00c4d70..05a2028b23 100644 --- a/extensions/ext-redis/src/rate-limit-store.test.ts +++ b/extensions/ext-redis/src/rate-limit-store.test.ts @@ -140,6 +140,50 @@ function createStoreWithMock( return { rateStore, mockClient }; } +async function withTimeoutUnrefProbe(run: () => Promise): Promise<{ + result: T; + unrefCalls: number; +}> { + const runtime = globalThis as unknown as { + setTimeout: typeof setTimeout; + clearTimeout: typeof clearTimeout; + }; + const originalSetTimeout = runtime.setTimeout; + const originalClearTimeout = runtime.clearTimeout; + let unrefCalls = 0; + + runtime.setTimeout = ((handler: TimerHandler, timeout?: number, ...args: unknown[]) => { + const inner = originalSetTimeout(handler, timeout, ...args); + return { + inner, + unref() { + unrefCalls++; + }, + } as unknown as ReturnType; + }) as typeof setTimeout; + runtime.clearTimeout = ((id?: ReturnType) => { + const inner = (id as unknown as { inner?: ReturnType } | undefined) + ?.inner; + originalClearTimeout(inner ?? id); + }) as typeof clearTimeout; + + try { + return { result: await run(), unrefCalls }; + } finally { + runtime.setTimeout = originalSetTimeout; + runtime.clearTimeout = originalClearTimeout; + } +} + +async function withTimeoutRefGuard(run: () => Promise): Promise { + const keepAlive = setInterval(() => {}, 1_000); + try { + return await run(); + } finally { + clearInterval(keepAlive); + } +} + function assert_reset_at_is_future(resetAt: number): void { assertEquals(resetAt > Date.now() - 1000, true); } @@ -292,10 +336,12 @@ describe("middleware/builtin/security/redis-rate-limit", () => { }); mockClient.eval = () => new Promise(() => {}); - const error = await assertRejects( - () => rateStore.increment("key", 1000), - Error, - "timed out", + const error = await withTimeoutRefGuard(() => + assertRejects( + () => rateStore.increment("key", 1000), + Error, + "timed out", + ) ); assertEquals(isVeryfrontError(error), true); @@ -305,6 +351,24 @@ describe("middleware/builtin/security/redis-rate-limit", () => { assertEquals((rateStore as any).client, null); }); + it("unrefs the operation timeout so it does not hold the process open", async () => { + const { rateStore, mockClient } = createStoreWithMock({ + operationTimeoutMs: 1, + }); + mockClient.eval = () => new Promise(() => {}); + + const { result: error, unrefCalls } = await withTimeoutUnrefProbe(() => + assertRejects( + () => rateStore.increment("key", 1000), + Error, + "timed out", + ) + ); + + assertEquals(isVeryfrontError(error), true); + assertEquals(unrefCalls, 1); + }); + it("does not retire a client for an unrelated TimeoutError name", async () => { const { rateStore, mockClient } = createStoreWithMock(); mockClient.eval = () => { diff --git a/extensions/ext-redis/src/rate-limit-store.ts b/extensions/ext-redis/src/rate-limit-store.ts index 228a6dbdb0..3a586fc42c 100644 --- a/extensions/ext-redis/src/rate-limit-store.ts +++ b/extensions/ext-redis/src/rate-limit-store.ts @@ -7,6 +7,7 @@ import { type RateLimitStore, requireRateLimitKey, requireRateLimitWindowMs, + unrefTimer, } from "veryfront/extensions/distributed/rate-limit-support"; const logger = serverLogger.component("redis-ratelimit"); @@ -227,6 +228,7 @@ export class RedisRateLimitStore implements RateLimitStore { timeoutId = setTimeout(() => { reject(createTimeoutError(operationName, timeoutMs)); }, timeoutMs); + unrefTimer(timeoutId); }); try { diff --git a/src/extensions/distributed/rate-limit-support.ts b/src/extensions/distributed/rate-limit-support.ts index cb26e7da6a..64f1440939 100644 --- a/src/extensions/distributed/rate-limit-support.ts +++ b/src/extensions/distributed/rate-limit-support.ts @@ -9,4 +9,5 @@ export { requireRateLimitKey, requireRateLimitWindowMs, } from "#veryfront/middleware/builtin/security/rate-limit-validation.ts"; +export { unrefTimer } from "#veryfront/platform/compat/process.ts"; export { MAX_TIMER_DELAY_MS } from "#veryfront/utils/timer.ts"; diff --git a/src/middleware/builtin/security/rate-limit.ts b/src/middleware/builtin/security/rate-limit.ts index 146ca57955..1bdbbe7b38 100644 --- a/src/middleware/builtin/security/rate-limit.ts +++ b/src/middleware/builtin/security/rate-limit.ts @@ -135,7 +135,9 @@ export interface MemoryRateLimitStoreOptions { * Size this above the peak number of distinct identities expected during one * complete rate-limit window, including burst headroom. New identities fail * closed when every entry is active; active limits are never evicted because - * eviction would let identity-flooding attackers reset their quota. + * eviction would let identity-flooding attackers reset their quota. When + * used through `rateLimit()`, capacity exhaustion logs structured failure + * details for the middleware request path. */ maxEntries?: number; } diff --git a/src/middleware/builtin/security/redis-rate-limit.test.ts b/src/middleware/builtin/security/redis-rate-limit.test.ts index 8db0b7753e..5dd46da530 100644 --- a/src/middleware/builtin/security/redis-rate-limit.test.ts +++ b/src/middleware/builtin/security/redis-rate-limit.test.ts @@ -83,6 +83,50 @@ function createStoreWithMock( }; } +async function withTimeoutUnrefProbe(run: () => Promise): Promise<{ + result: T; + unrefCalls: number; +}> { + const runtime = globalThis as unknown as { + setTimeout: typeof setTimeout; + clearTimeout: typeof clearTimeout; + }; + const originalSetTimeout = runtime.setTimeout; + const originalClearTimeout = runtime.clearTimeout; + let unrefCalls = 0; + + runtime.setTimeout = ((handler: TimerHandler, timeout?: number, ...args: unknown[]) => { + const inner = originalSetTimeout(handler, timeout, ...args); + return { + inner, + unref() { + unrefCalls++; + }, + } as unknown as ReturnType; + }) as typeof setTimeout; + runtime.clearTimeout = ((id?: ReturnType) => { + const inner = (id as unknown as { inner?: ReturnType } | undefined) + ?.inner; + originalClearTimeout(inner ?? id); + }) as typeof clearTimeout; + + try { + return { result: await run(), unrefCalls }; + } finally { + runtime.setTimeout = originalSetTimeout; + runtime.clearTimeout = originalClearTimeout; + } +} + +async function withTimeoutRefGuard(run: () => Promise): Promise { + const keepAlive = setInterval(() => {}, 1_000); + try { + return await run(); + } finally { + clearInterval(keepAlive); + } +} + describe("provider-backed RedisRateLimitStore", () => { describe("constructor", () => { it("uses the stable default key prefix", () => { @@ -227,16 +271,35 @@ describe("provider-backed RedisRateLimitStore", () => { client, ); - const error = await assertRejects( - () => store.increment("key", 1_000), - Error, - "timed out", + const error = await withTimeoutRefGuard(() => + assertRejects( + () => store.increment("key", 1_000), + Error, + "timed out", + ) ); assertEquals(isVeryfrontError(error), true); assertEquals(isVeryfrontError(error) ? error.slug : undefined, TIMEOUT_ERROR.slug); assertEquals(closeCalls(), 1); }); + it("unrefs the operation timeout so it does not hold the process open", async () => { + const client = createMockRedisClient(); + client.eval = () => new Promise(() => {}); + const { store } = createStoreWithMock({ operationTimeoutMs: 1 }, client); + + const { result: error, unrefCalls } = await withTimeoutUnrefProbe(() => + assertRejects( + () => store.increment("key", 1_000), + Error, + "timed out", + ) + ); + + assertEquals(isVeryfrontError(error), true); + assertEquals(unrefCalls, 1); + }); + it("does not retire a provider connection for an unrelated TimeoutError name", async () => { const client = createMockRedisClient(); client.eval = () => { @@ -286,10 +349,12 @@ describe("provider-backed RedisRateLimitStore", () => { client, ); - await assertRejects( - () => store.reset("key"), - Error, - "timed out", + await withTimeoutRefGuard(() => + assertRejects( + () => store.reset("key"), + Error, + "timed out", + ) ); assertEquals(closeCalls(), 1); }); diff --git a/src/middleware/builtin/security/redis-rate-limit.ts b/src/middleware/builtin/security/redis-rate-limit.ts index 6ae84ceb81..1a3e5f899f 100644 --- a/src/middleware/builtin/security/redis-rate-limit.ts +++ b/src/middleware/builtin/security/redis-rate-limit.ts @@ -1,6 +1,7 @@ import { createError, isVeryfrontError, TIMEOUT_ERROR, toError } from "#veryfront/errors"; import { OwnedRedisClientConnection } from "#veryfront/extensions/distributed/owned-redis-client.ts"; import type { RedisClient } from "#veryfront/extensions/distributed"; +import { unrefTimer } from "#veryfront/platform/compat/process.ts"; import { serverLogger } from "#veryfront/utils"; import { MAX_TIMER_DELAY_MS } from "#veryfront/utils/timer.ts"; import { requireRateLimitKey, requireRateLimitWindowMs } from "./rate-limit-validation.ts"; @@ -99,6 +100,7 @@ export class RedisRateLimitStore implements RateLimitStore { () => reject(createTimeoutError(operationName, this.operationTimeoutMs)), this.operationTimeoutMs, ); + unrefTimer(timeoutId); }); try { From ca426131045d04e435b86d41faa9c23bc01ec955 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 13:55:49 +0200 Subject: [PATCH 16/17] test(security): pin default rate-limit parity at 100 requests per minute --- .../builtin/security/rate-limit.test.ts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/middleware/builtin/security/rate-limit.test.ts b/src/middleware/builtin/security/rate-limit.test.ts index 8cc54639ef..d428c1a3cf 100644 --- a/src/middleware/builtin/security/rate-limit.test.ts +++ b/src/middleware/builtin/security/rate-limit.test.ts @@ -217,6 +217,28 @@ describe("rateLimit middleware", () => { assertEquals(response?.status, 200); }); + it("should keep the documented default limits at 100 requests per 60s window", async () => { + const middleware = rateLimit(); + + for (let index = 0; index < 100; index++) { + const response = await middleware( + createContext(), + () => Promise.resolve(new Response("OK")), + ); + assertEquals(response?.status, 200); + } + + const blocked = await middleware( + createContext(), + () => Promise.resolve(new Response("OK")), + ); + + assertEquals(blocked?.status, 429); + const retryAfterSeconds = Number(blocked?.headers.get("Retry-After")); + assertEquals(Number.isSafeInteger(retryAfterSeconds), true); + assertEquals(retryAfterSeconds >= 1 && retryAfterSeconds <= 60, true); + }); + it("should validate numeric configuration before creating middleware", () => { for ( const maxRequests of [ From 7f459bbd84588a1162f6e08f6e8c633769623224 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 14:28:01 +0200 Subject: [PATCH 17/17] Share Redis rate-limit atomic counter script The core facade and Redis extension both execute the same Lua counter semantics. Keeping the literal in two files left a small divergence risk for future rate-limit edits, so the script now lives behind the distributed rate-limit support surface used by the extension. Constraint: The Redis extension must remain the owner of the Redis package while sharing provider-neutral rate-limit semantics with core. Rejected: Add a test comparing two duplicated literals | this still leaves two update sites and preserves the drift risk. Confidence: high Scope-risk: narrow Reversibility: clean Tested: npx --yes deno@2.7.7 test --no-check --allow-all extensions/ext-redis/src/rate-limit-store.test.ts src/middleware/builtin/security/redis-rate-limit.test.ts src/middleware/builtin/security/rate-limit.test.ts src/modules/server/rate-limiter.test.ts Tested: npx --yes deno@2.7.7 fmt --check src/middleware/builtin/security/redis-rate-limit-script.ts src/extensions/distributed/rate-limit-support.ts src/middleware/builtin/security/redis-rate-limit.ts extensions/ext-redis/src/rate-limit-store.ts Tested: npx --yes deno@2.7.7 lint src/middleware/builtin/security/redis-rate-limit-script.ts src/extensions/distributed/rate-limit-support.ts src/middleware/builtin/security/redis-rate-limit.ts extensions/ext-redis/src/rate-limit-store.ts Tested: npx --yes deno@2.7.7 check src/middleware/builtin/security/redis-rate-limit-script.ts src/extensions/distributed/rate-limit-support.ts src/middleware/builtin/security/redis-rate-limit.ts extensions/ext-redis/src/rate-limit-store.ts Tested: git diff --check Not-tested: Full pre-push suite after this final extraction; the previous pre-push attempt reached 3722 passing tests before an unrelated SSR adapter dangling-timeout flake, and that test passed in isolation. --- extensions/ext-redis/src/rate-limit-store.ts | 13 ++----------- src/extensions/distributed/rate-limit-support.ts | 3 +++ .../builtin/security/redis-rate-limit-script.ts | 9 +++++++++ src/middleware/builtin/security/redis-rate-limit.ts | 13 ++----------- 4 files changed, 16 insertions(+), 22 deletions(-) create mode 100644 src/middleware/builtin/security/redis-rate-limit-script.ts diff --git a/extensions/ext-redis/src/rate-limit-store.ts b/extensions/ext-redis/src/rate-limit-store.ts index 3a586fc42c..3747288c67 100644 --- a/extensions/ext-redis/src/rate-limit-store.ts +++ b/extensions/ext-redis/src/rate-limit-store.ts @@ -5,6 +5,7 @@ import { MAX_TIMER_DELAY_MS, type RateLimitEntry, type RateLimitStore, + REDIS_RATE_LIMIT_INCREMENT_WITH_TTL_SCRIPT, requireRateLimitKey, requireRateLimitWindowMs, unrefTimer, @@ -39,16 +40,6 @@ type RedisClientFactory = (options: RedisClientFactoryOptions) => RedisClient; const DEFAULT_REDIS_CONNECT_TIMEOUT_MS = 5_000; const DEFAULT_REDIS_OPERATION_TIMEOUT_MS = 5_000; -const INCREMENT_WITH_TTL_SCRIPT = ` -local count = redis.call("INCR", KEYS[1]) -local ttl = redis.call("PTTL", KEYS[1]) -if ttl < 0 then - redis.call("PEXPIRE", KEYS[1], ARGV[1]) - ttl = tonumber(ARGV[1]) -end -return { count, ttl } -`; - /** Options accepted by redis rate limit. */ export interface RedisRateLimitOptions { url?: string; @@ -318,7 +309,7 @@ export class RedisRateLimitStore implements RateLimitStore { let result: unknown; try { result = await this.withTimeout( - client.eval(INCREMENT_WITH_TTL_SCRIPT, { + client.eval(REDIS_RATE_LIMIT_INCREMENT_WITH_TTL_SCRIPT, { keys: [redisKey], arguments: [String(normalizedWindowMs)], }), diff --git a/src/extensions/distributed/rate-limit-support.ts b/src/extensions/distributed/rate-limit-support.ts index 64f1440939..c5b4bec44f 100644 --- a/src/extensions/distributed/rate-limit-support.ts +++ b/src/extensions/distributed/rate-limit-support.ts @@ -9,5 +9,8 @@ export { requireRateLimitKey, requireRateLimitWindowMs, } from "#veryfront/middleware/builtin/security/rate-limit-validation.ts"; +export { + REDIS_RATE_LIMIT_INCREMENT_WITH_TTL_SCRIPT, +} from "#veryfront/middleware/builtin/security/redis-rate-limit-script.ts"; export { unrefTimer } from "#veryfront/platform/compat/process.ts"; export { MAX_TIMER_DELAY_MS } from "#veryfront/utils/timer.ts"; diff --git a/src/middleware/builtin/security/redis-rate-limit-script.ts b/src/middleware/builtin/security/redis-rate-limit-script.ts new file mode 100644 index 0000000000..00ffe615f5 --- /dev/null +++ b/src/middleware/builtin/security/redis-rate-limit-script.ts @@ -0,0 +1,9 @@ +export const REDIS_RATE_LIMIT_INCREMENT_WITH_TTL_SCRIPT = ` +local count = redis.call("INCR", KEYS[1]) +local ttl = redis.call("PTTL", KEYS[1]) +if ttl < 0 then + redis.call("PEXPIRE", KEYS[1], ARGV[1]) + ttl = tonumber(ARGV[1]) +end +return { count, ttl } +`; diff --git a/src/middleware/builtin/security/redis-rate-limit.ts b/src/middleware/builtin/security/redis-rate-limit.ts index 1a3e5f899f..00e08be816 100644 --- a/src/middleware/builtin/security/redis-rate-limit.ts +++ b/src/middleware/builtin/security/redis-rate-limit.ts @@ -4,6 +4,7 @@ import type { RedisClient } from "#veryfront/extensions/distributed"; import { unrefTimer } from "#veryfront/platform/compat/process.ts"; import { serverLogger } from "#veryfront/utils"; import { MAX_TIMER_DELAY_MS } from "#veryfront/utils/timer.ts"; +import { REDIS_RATE_LIMIT_INCREMENT_WITH_TTL_SCRIPT } from "./redis-rate-limit-script.ts"; import { requireRateLimitKey, requireRateLimitWindowMs } from "./rate-limit-validation.ts"; import type { RateLimitEntry, RateLimitStore } from "./types.ts"; @@ -11,16 +12,6 @@ const logger = serverLogger.component("redis-ratelimit"); const DEFAULT_REDIS_CONNECT_TIMEOUT_MS = 5_000; const DEFAULT_REDIS_OPERATION_TIMEOUT_MS = 5_000; -const INCREMENT_WITH_TTL_SCRIPT = ` -local count = redis.call("INCR", KEYS[1]) -local ttl = redis.call("PTTL", KEYS[1]) -if ttl < 0 then - redis.call("PEXPIRE", KEYS[1], ARGV[1]) - ttl = tonumber(ARGV[1]) -end -return { count, ttl } -`; - /** Options accepted by the provider-backed Redis rate-limit store. */ export interface RedisRateLimitOptions { url?: string; @@ -130,7 +121,7 @@ export class RedisRateLimitStore implements RateLimitStore { const [count, pttl] = parseIncrementResult( await this.withOperationTimeout( - client.eval(INCREMENT_WITH_TTL_SCRIPT, { + client.eval(REDIS_RATE_LIMIT_INCREMENT_WITH_TTL_SCRIPT, { keys: [redisKey], arguments: [String(normalizedWindowMs)], }),