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 566003c095..cb85bf5a77 100644 --- a/docs/api-reference/veryfront/middleware.md +++ b/docs/api-reference/veryfront/middleware.md @@ -110,11 +110,20 @@ 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#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` + +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. 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` @@ -142,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#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#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#L106) | +| `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) | @@ -156,26 +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#L25) | +| `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` | Implement redis rate limit store. | [source](https://github.com/veryfront/veryfront-code/blob/main/src/middleware/builtin/security/redis-rate-limit.ts#L27) | +| `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#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#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#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#L72) | +| `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#L21) | +| `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/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..05a2028b23 100644 --- a/extensions/ext-redis/src/rate-limit-store.test.ts +++ b/extensions/ext-redis/src/rate-limit-store.test.ts @@ -1,7 +1,10 @@ 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 { 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"; async function outcomeWithin( promise: Promise, @@ -137,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); } @@ -160,7 +207,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", @@ -253,7 +300,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", ); @@ -289,16 +336,59 @@ describe("middleware/builtin/security/redis-rate-limit", () => { }); mockClient.eval = () => new Promise(() => {}); - const outcome = await outcomeWithin( - rateStore.increment("key", 1000), - 50, + const error = await withTimeoutRefGuard(() => + 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("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 = () => { + 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", () => { @@ -315,6 +405,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(MAX_RATE_LIMIT_KEY_LENGTH + 1)), + RangeError, + "1024", + ); + assertEquals(factoryLoads, 0); + }); }); describe("destroy", () => { @@ -339,6 +446,22 @@ 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++; + return Promise.reject(new ClientClosedError()); + }; + + 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; @@ -360,6 +483,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(MAX_RATE_LIMIT_KEY_LENGTH + 1)), + RangeError, + "1024", + ); + + assertEquals(mockClient._connectCalls, 0); + }); + }); + describe("ensureClient", () => { it("should reuse existing client", async () => { const { rateStore, mockClient } = createStoreWithMock(); @@ -564,6 +705,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 4458ddc3e0..3747288c67 100644 --- a/extensions/ext-redis/src/rate-limit-store.ts +++ b/extensions/ext-redis/src/rate-limit-store.ts @@ -1,12 +1,14 @@ -import { createError, toError } from "veryfront/errors"; +import { createError, isVeryfrontError, TIMEOUT_ERROR, 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, type RateLimitStore, + REDIS_RATE_LIMIT_INCREMENT_WITH_TTL_SCRIPT, requireRateLimitKey, requireRateLimitWindowMs, + unrefTimer, } from "veryfront/extensions/distributed/rate-limit-support"; const logger = serverLogger.component("redis-ratelimit"); @@ -38,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; @@ -157,6 +149,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 +176,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, @@ -209,6 +219,7 @@ export class RedisRateLimitStore implements RateLimitStore { timeoutId = setTimeout(() => { reject(createTimeoutError(operationName, timeoutMs)); }, timeoutMs); + unrefTimer(timeoutId); }); try { @@ -298,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)], }), @@ -318,11 +329,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", ); @@ -338,6 +350,9 @@ 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); if (client) clientsToDisconnect.add(client); @@ -359,8 +374,6 @@ export class RedisRateLimitStore implements RateLimitStore { }) ), ); - pending?.catch(() => {}); - if (disconnectFailed) throw disconnectError; } } @@ -380,15 +393,17 @@ 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 { + return error instanceof ClientClosedError; } function parseIncrementResult(result: unknown): [number, number] { 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": [ diff --git a/scripts/lint/audit-core-deps.test.ts b/scripts/lint/audit-core-deps.test.ts index b26d2a6851..066d9c250c 100644 --- a/scripts/lint/audit-core-deps.test.ts +++ b/scripts/lint/audit-core-deps.test.ts @@ -261,7 +261,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 +275,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 +285,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 +294,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 +303,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 +313,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 +323,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 +332,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 +345,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 +354,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 +363,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 +372,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 +386,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/extensions/distributed/rate-limit-support.ts b/src/extensions/distributed/rate-limit-support.ts index 0e6fd54fac..c5b4bec44f 100644 --- a/src/extensions/distributed/rate-limit-support.ts +++ b/src/extensions/distributed/rate-limit-support.ts @@ -5,7 +5,12 @@ 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"; +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/index.ts b/src/middleware/builtin/index.ts index 83032b6fae..5adf9277d2 100644 --- a/src/middleware/builtin/index.ts +++ b/src/middleware/builtin/index.ts @@ -21,6 +21,7 @@ export { authRateLimit, type AuthRateLimitOptions, MemoryRateLimitStore, + type MemoryRateLimitStoreOptions, rateLimit, type RateLimitOptions, } from "./security/rate-limit.ts"; diff --git a/src/middleware/builtin/security/index.ts b/src/middleware/builtin/security/index.ts index d409db3cf3..fa88c51e03 100644 --- a/src/middleware/builtin/security/index.ts +++ b/src/middleware/builtin/security/index.ts @@ -17,7 +17,10 @@ export { csrfProtection } from "./csrf.ts"; export { authRateLimit, type AuthRateLimitOptions, + MemoryRateLimitStore, + type MemoryRateLimitStoreOptions, 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 04b9fe34da..d428c1a3cf 100644 --- a/src/middleware/builtin/security/rate-limit.test.ts +++ b/src/middleware/builtin/security/rate-limit.test.ts @@ -1,10 +1,24 @@ 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 { __subscribeLogRecordEmitter, type LogEntry } from "#veryfront/utils/logger/index.ts"; import { MiddlewareContext } from "../../core/context.ts"; -import { authRateLimit, MemoryRateLimitStore, rateLimit } from "./rate-limit.ts"; +import { MAX_RATE_LIMIT_KEY_LENGTH } from "./rate-limit-validation.ts"; +import { + authRateLimit, + MemoryRateLimitStore, + rateLimit, + type RedisRateLimitOptions, + RedisRateLimitStore, +} from "#veryfront/middleware"; (globalThis as Record).__vfDisableLruInterval = true; @@ -67,6 +81,87 @@ 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); + + const error = await assertRejects( + () => boundedStore.increment("overflow", 60000), + 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); + } finally { + boundedStore.destroy(); + } + }); + + 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); + + 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 +217,364 @@ 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 [ + -1, + 1.5, + Number.NaN, + Number.MAX_SAFE_INTEGER, + Number.POSITIVE_INFINITY, + ] + ) { + assertThrows( + () => rateLimit({ maxRequests }), + RangeError, + "between 0", + ); + } + + for (const windowMs of [0, -1, 1.5, Number.NaN, Number.POSITIVE_INFINITY]) { + assertThrows( + () => rateLimit({ windowMs }), + 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 () => { + 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("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(MAX_RATE_LIMIT_KEY_LENGTH + 1), + }); + 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: { + 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 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(MAX_RATE_LIMIT_KEY_LENGTH + 1), + store: { + increment: () => { + incrementCalled = true; + return Promise.resolve({ count: 1, resetAt: Date.now() + 1000 }); + }, + reset: () => Promise.resolve(), + }, + }); + + 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(), + }, + }); + + const response = await middleware( + createContext("x".repeat(MAX_RATE_LIMIT_KEY_LENGTH + 1)), + () => Promise.resolve(new Response("OK")), + ); + + assertEquals(response?.status, 503); + assertEquals(response?.headers.get("Retry-After"), "60"); + 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: () => { + const error = new Error("backend unavailable"); + error.name = "BackendUnavailableError"; + return Promise.reject(error); + }, + 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", + ]); + assertEquals(records.map((record) => record.context?.failureKind), [ + "key-resolution", + "store-unavailable", + ]); + assertEquals(records.map((record) => record.context?.errorName), [ + "Error", + "BackendUnavailableError", + ]); + }); + + 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-increment"); + assertEquals(records[0]?.context?.failureKind, "capacity-exhausted"); + assertEquals(records[0]?.context?.capacity, 1); + }); + it("should use custom key generator", async () => { let capturedKey = ""; const middleware = rateLimit({ @@ -208,6 +661,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 656921c878..1bdbbe7b38 100644 --- a/src/middleware/builtin/security/rate-limit.ts +++ b/src/middleware/builtin/security/rate-limit.ts @@ -3,18 +3,38 @@ 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"); + +class MemoryRateLimitCapacityError extends Error { + override readonly name = "MemoryRateLimitCapacityError"; + + constructor(readonly capacity: number) { + super(`Memory rate limit store capacity of ${capacity} entries is exhausted`); + } +} -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,54 +45,115 @@ 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 = {}, + ) { + 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; + 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 MemoryRateLimitCapacityError(this.maxEntries); + } + + 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. + * + * 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. When + * used through `rateLimit()`, capacity exhaustion logs structured failure + * details for the middleware request path. + */ + maxEntries?: number; +} + /** Options accepted by rate limit. */ 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 @@ -87,6 +168,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; /** @@ -96,10 +179,54 @@ export interface AuthRateLimitOptions { trustProxy?: boolean; } -function isRateLimitStore( - value: RateLimitStore | AuthRateLimitOptions, -): value is RateLimitStore { - return "increment" in value && typeof value.increment === "function"; +function isRateLimitStore(value: unknown): value is RateLimitStore { + return ( + value != null && + typeof value === "object" && + 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 (!isRateLimitStore(value)) { + 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 || + value >= Number.MAX_SAFE_INTEGER + ) { + throw new RangeError( + `Rate limit maxRequests must be an integer between 0 and ${Number.MAX_SAFE_INTEGER - 1}`, + ); + } + 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 +234,100 @@ 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, + ); + 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, { maxEntries: options.maxEntries }) + : 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)); + const lastFailureLogAt = new Map(); return async (ctx, next) => { const req = getRequest(ctx); - const key = keyGenerator(req); - const entry = await store.increment(key, windowMs); + let entry: RateLimitEntry; + 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 ( + lastLogAt === undefined || + now - lastLogAt >= STORE_FAILURE_LOG_INTERVAL_MS + ) { + 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 Error ? error.name : typeof error, + ...(error instanceof MemoryRateLimitCapacityError ? { capacity: error.capacity } : {}), + }); + } + 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), + }, }); }; } @@ -144,6 +340,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-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.test.ts b/src/middleware/builtin/security/redis-rate-limit.test.ts index df45126d1a..5dd46da530 100644 --- a/src/middleware/builtin/security/redis-rate-limit.test.ts +++ b/src/middleware/builtin/security/redis-rate-limit.test.ts @@ -1,314 +1,377 @@ 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"; +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"; -function createMockRedisClient(): { - connect: () => Promise; - disconnect: () => Promise; - eval: ( +interface MockRedisClient { + 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; + ): Promise; + del(key: string): Promise; _evalCalls: number; - _incrCalls: number; - _pExpireCalls: number; - _disconnectCalls: number; _delCalls: number; - _store: Map; -} { - const store = new Map(); - const listeners = new Map void>>(); + _lastKey?: string; + _lastWindow?: string; +} + +function createMockRedisClient( + result: unknown = [1, 60_000], +): MockRedisClient { 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); + const client: MockRedisClient = { + eval: (_script, options) => { + evalCalls++; + client._lastKey = options.keys[0]; + client._lastWindow = options.arguments[0]; + return Promise.resolve(result); }, - 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); + del: (key) => { + delCalls++; + client._lastKey = key; + return Promise.resolve(1); }, get _evalCalls() { return evalCalls; }, - get _incrCalls() { - return incrCalls; - }, - get _pExpireCalls() { - return pExpireCalls; - }, - get _disconnectCalls() { - return disconnectCalls; - }, get _delCalls() { return delCalls; }, - _store: store, }; + return client; } function createStoreWithMock( - options?: { keyPrefix?: string }, + options?: RedisRateLimitOptions, + client = createMockRedisClient(), ): { - rateStore: RedisRateLimitStore; - mockClient: ReturnType; + store: RedisRateLimitStore; + client: MockRedisClient; + getClientCalls: () => number; + closeCalls: () => number; } { - const rateStore = new RedisRateLimitStore(options); - const mockClient = createMockRedisClient(); + const store = new RedisRateLimitStore(options); + let getClientCalls = 0; + let closeCalls = 0; 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; + (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, + }; +} - 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; + } } -function assert_reset_at_is_future(resetAt: number): void { - assertEquals(resetAt > Date.now() - 1000, true); +async function withTimeoutRefGuard(run: () => Promise): Promise { + const keepAlive = setInterval(() => {}, 1_000); + try { + return await run(); + } finally { + clearInterval(keepAlive); + } } -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", () => { +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(MAX_RATE_LIMIT_KEY_LENGTH + 1) }), + RangeError, + "1024", + ); + for (const timeout of [0, -1, 1.5, Number.NaN, Number.POSITIVE_INFINITY]) { assertThrows( - () => new RedisRateLimitStore({ keyPrefix: "x".repeat(1025) }), + () => new RedisRateLimitStore({ connectTimeoutMs: timeout }), RangeError, - "1024", + "connectTimeoutMs", ); - for (const invalidPrefix of ["", " \t ", "app\u0000:", "app\u0085:"]) { - assertThrows( - () => new RedisRateLimitStore({ keyPrefix: invalidPrefix }), - TypeError, - "visible text without control characters", - ); - } - }); + 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); }); - 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(); + 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); - 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); - }); + assertEquals(entry.count, 2); + assertEquals(entry.resetAt >= before + 1_500, true); + assertEquals(entry.resetAt <= Date.now() + 1_500, true); }); - 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); + 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); - await rateStore.reset("key1"); - assertEquals(mockClient._store.has("veryfront:ratelimit:key1"), false); - }); + assertEquals(entry.resetAt >= before + 2_000, true); + assertEquals(entry.resetAt <= Date.now() + 2_000, true); + }); - it("should not throw when resetting non-existent key", async () => { - const { rateStore } = createStoreWithMock(); - await rateStore.reset("nonexistent"); - }); + it("validates keys and windows before opening a provider connection", async () => { + const { store, client, getClientCalls } = createStoreWithMock(); + + await assertRejects( + () => store.increment("x".repeat(MAX_RATE_LIMIT_KEY_LENGTH + 1), 1_000), + RangeError, + "1024", + ); + await assertRejects( + () => store.increment("key", 0), + RangeError, + "windowMs", + ); + assertEquals(getClientCalls(), 0); + assertEquals(client._evalCalls, 0); + }); - it("should reject an invalid key before deleting", async () => { - const { rateStore, mockClient } = createStoreWithMock(); + 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( - () => rateStore.reset("x".repeat(1025)), - RangeError, - "1024", + () => 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( - () => rateStore.reset("tenant\u0000member"), - TypeError, - "visible text without control characters", + () => 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 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); + }); - assertEquals(mockClient._delCalls, 0); - }); + 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); }); - 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); - }); + 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", () => { + 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 withTimeoutRefGuard(() => + 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); }); - 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); - }); + 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 7764763f29..00e08be816 100644 --- a/src/middleware/builtin/security/redis-rate-limit.ts +++ b/src/middleware/builtin/security/redis-rate-limit.ts @@ -1,46 +1,73 @@ -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 { 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"; 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 redis rate limit. */ +/** Options accepted by the provider-backed Redis rate-limit store. */ export interface RedisRateLimitOptions { url?: string; keyPrefix?: string; + /** Maximum time allowed for opening the extension-provided Redis client. */ + connectTimeoutMs?: number; + /** Maximum time allowed for an individual Redis command. */ + operationTimeoutMs?: number; } -/** Implement 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 readonly connection: OwnedRedisClientConnection; private readonly keyPrefix: string; + private readonly operationTimeoutMs: number; 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"); + } + const connectTimeoutMs = requireTimeoutMs( + options.connectTimeoutMs ?? DEFAULT_REDIS_CONNECT_TIMEOUT_MS, + "connectTimeoutMs", + ); + this.operationTimeoutMs = requireTimeoutMs( + options.operationTimeoutMs ?? DEFAULT_REDIS_OPERATION_TIMEOUT_MS, + "operationTimeoutMs", + ); this.keyPrefix = requireRateLimitKey( options.keyPrefix ?? "veryfront:ratelimit:", "Redis rate limit keyPrefix", ); this.connection = new OwnedRedisClientConnection( - options.url === undefined ? {} : { url: options.url }, + { + ...(options.url === undefined ? {} : { url: options.url }), + connectTimeout: connectTimeoutMs, + autoReconnect: false, + }, { onError(error) { - logger.error("client error", error); + logger.error("client error", { + errorName: error instanceof Error ? error.name : typeof error, + }); }, onCloseError(error) { - logger.error("client close failed", error); + logger.error("client close failed", { + errorName: error instanceof Error ? error.name : typeof error, + }); }, }, ); @@ -54,6 +81,38 @@ export class RedisRateLimitStore implements RateLimitStore { return `${this.keyPrefix}${key}`; } + private async withOperationTimeout( + operation: Promise, + operationName: string, + ): Promise { + let timeoutId: ReturnType | undefined; + const timeout = new Promise((_, reject) => { + timeoutId = setTimeout( + () => reject(createTimeoutError(operationName, this.operationTimeoutMs)), + this.operationTimeoutMs, + ); + unrefTimer(timeoutId); + }); + + try { + return await Promise.race([operation, timeout]); + } catch (error) { + 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, + }); + }); + } + throw error; + } finally { + if (timeoutId !== undefined) clearTimeout(timeoutId); + } + } + async increment(key: string, windowMs: number): Promise { const normalizedKey = requireRateLimitKey(key); const normalizedWindowMs = requireRateLimitWindowMs(windowMs); @@ -61,19 +120,25 @@ export class RedisRateLimitStore implements RateLimitStore { const redisKey = this.storageKey(normalizedKey); const [count, pttl] = parseIncrementResult( - await client.eval(INCREMENT_WITH_TTL_SCRIPT, { - keys: [redisKey], - arguments: [String(normalizedWindowMs)], - }), + await this.withOperationTimeout( + client.eval(REDIS_RATE_LIMIT_INCREMENT_WITH_TTL_SCRIPT, { + keys: [redisKey], + arguments: [String(normalizedWindowMs)], + }), + "increment", + ), ); - const ttl = pttl > 0 ? pttl : normalizedWindowMs; + const ttl = pttl > 0 ? requireRateLimitWindowMs(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)); + await this.withOperationTimeout( + client.del(this.storageKey(normalizedKey)).then(() => undefined), + "reset", + ); } async destroy(): Promise { @@ -81,6 +146,30 @@ export class RedisRateLimitStore implements RateLimitStore { } } +function requireTimeoutMs(value: unknown, name: string): number { + if ( + typeof value !== "number" || + !Number.isSafeInteger(value) || + value <= 0 || + value > MAX_TIMER_DELAY_MS + ) { + 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 { + return TIMEOUT_ERROR.create({ + detail: `Redis rate limit ${operationName} timed out after ${timeoutMs}ms`, + }); +} + +function isTimeoutError(error: unknown): boolean { + return isVeryfrontError(error) && error.slug === TIMEOUT_ERROR.slug; +} + function parseIncrementResult(result: unknown): [number, number] { if (!Array.isArray(result) || result.length < 2) { throw toError( @@ -94,11 +183,19 @@ function parseIncrementResult(result: unknown): [number, number] { const count = Number(result[0]); const ttl = Number(result[1]); - if (!Number.isFinite(count) || !Number.isFinite(ttl)) { + 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 non-numeric values.", + message: "Redis rate limit eval returned an invalid TTL.", }), ); } diff --git a/src/middleware/index.ts b/src/middleware/index.ts index 348096e6c7..6855a1abca 100644 --- a/src/middleware/index.ts +++ b/src/middleware/index.ts @@ -42,6 +42,7 @@ export { authRateLimit, type AuthRateLimitOptions, MemoryRateLimitStore, + type MemoryRateLimitStoreOptions, rateLimit, type RateLimitOptions, } from "./builtin/security/rate-limit.ts"; diff --git a/src/modules/server/rate-limiter.test.ts b/src/modules/server/rate-limiter.test.ts index 89c789ada1..63b2460189 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,66 @@ 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("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); + }); + + 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..1f13b54ba0 100644 --- a/src/modules/server/rate-limiter.ts +++ b/src/modules/server/rate-limiter.ts @@ -1,26 +1,64 @@ 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(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"); + } + 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"); + } - constructor(private readonly maxMessages: number) {} + 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; }