From 5a938f28673ab65c8eb9f0cce62a53eb2c2d194c Mon Sep 17 00:00:00 2001 From: innovativedesign-lab Date: Sat, 27 Jun 2026 17:30:09 +0100 Subject: [PATCH 1/2] Translation parameter replacement regex does not support nested keys or escaped braces --- .../translation-nested-keys-fix/.config.kiro | 1 + .../translation-nested-keys-fix/bugfix.md | 73 ++++++++ .vscode/settings.json | 1 + src/hooks/useInternationalization.tsx | 4 +- src/locales/translationManager.test.ts | 161 ++++++++++++++++++ src/locales/translationManager.ts | 21 ++- 6 files changed, 256 insertions(+), 5 deletions(-) create mode 100644 .kiro/specs/translation-nested-keys-fix/.config.kiro create mode 100644 .kiro/specs/translation-nested-keys-fix/bugfix.md create mode 100644 .vscode/settings.json create mode 100644 src/locales/translationManager.test.ts diff --git a/.kiro/specs/translation-nested-keys-fix/.config.kiro b/.kiro/specs/translation-nested-keys-fix/.config.kiro new file mode 100644 index 00000000..770dabf9 --- /dev/null +++ b/.kiro/specs/translation-nested-keys-fix/.config.kiro @@ -0,0 +1 @@ +{"specId": "16317c2a-e85f-4e5f-a27e-7b7f9e0609b1", "workflowType": "requirements-first", "specType": "bugfix"} \ No newline at end of file diff --git a/.kiro/specs/translation-nested-keys-fix/bugfix.md b/.kiro/specs/translation-nested-keys-fix/bugfix.md new file mode 100644 index 00000000..5045d341 --- /dev/null +++ b/.kiro/specs/translation-nested-keys-fix/bugfix.md @@ -0,0 +1,73 @@ +# Bugfix Requirements Document + +## Introduction + +The `getTranslation` function in `src/locales/translationManager.ts` replaces `{{key}}` placeholders using the regex `/\{\{(\w+)\}\}/g`. The `\w+` pattern only matches word characters (`[a-zA-Z0-9_]`), which excludes the dot character. As a result, nested interpolation keys like `{{user.name}}` are never matched and their placeholders are left verbatim in the output string. Additionally, when a param key referenced in a template is absent from the provided params object, the placeholder is silently left in the output with no developer notification, making debugging difficult. + +## Bug Analysis + +### Current Behavior (Defect) + +1.1 WHEN a translation template contains a dot-separated placeholder such as `{{user.name}}` AND params contains a nested object `{ user: { name: 'Alice' } }` THEN the system leaves `{{user.name}}` unreplaced in the returned string + +1.2 WHEN a translation template contains a placeholder whose key is not present in the provided params object THEN the system silently leaves the placeholder in the returned string without any warning + +### Expected Behavior (Correct) + +2.1 WHEN a translation template contains a dot-separated placeholder such as `{{user.name}}` AND params contains a matching nested object `{ user: { name: 'Alice' } }` THEN the system SHALL resolve the value by traversing the nested object and return the string with `{{user.name}}` replaced by `Alice` + +2.2 WHEN a translation template contains a placeholder whose key is not present in the provided params object THEN the system SHALL emit a `console.warn` (or equivalent logger warning) identifying the missing key AND leave the original placeholder visible in the returned string + +### Unchanged Behavior (Regression Prevention) + +3.1 WHEN a translation template contains a flat (non-nested) placeholder such as `{{name}}` AND params contains the matching key `{ name: 'Alice' }` THEN the system SHALL CONTINUE TO replace the placeholder and return the correct interpolated string + +3.2 WHEN params is undefined or not provided THEN the system SHALL CONTINUE TO return the raw translation string without modification + +3.3 WHEN a translation template contains multiple placeholders of any kind THEN the system SHALL CONTINUE TO replace all of them in a single pass + +3.4 WHEN the translation key path does not exist in the translations object THEN the system SHALL CONTINUE TO return the original key string unchanged + +--- + +## Bug Condition Derivation + +### Bug Condition Function + +```pascal +FUNCTION isBugCondition(X) + INPUT: X of type { template: string, params: Record } + OUTPUT: boolean + + // Returns true when the placeholder contains a dot (nested key) + RETURN template CONTAINS pattern /\{\{[\w]+\.[\w.]+\}\}/ + OR (template CONTAINS pattern /\{\{[\w.]+\}\}/ AND referenced key NOT IN params) +END FUNCTION +``` + +### Fix Checking Property + +```pascal +// Property: Fix Checking — Nested Key Resolution +FOR ALL X WHERE isBugCondition(X) AND X is nested key case DO + result ← getTranslation'(translations, key, X.params) + ASSERT result does NOT contain the original placeholder + ASSERT result contains the resolved nested value +END FOR + +// Property: Fix Checking — Missing Key Warning +FOR ALL X WHERE isBugCondition(X) AND X is missing key case DO + result ← getTranslation'(translations, key, X.params) + ASSERT console.warn was called with a message referencing the missing key + ASSERT result contains the original placeholder (visible to developer) +END FOR +``` + +### Preservation Checking Property + +```pascal +// Property: Preservation Checking +FOR ALL X WHERE NOT isBugCondition(X) DO + ASSERT getTranslation(translations, key, X.params) = getTranslation'(translations, key, X.params) +END FOR +``` diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 00000000..0967ef42 --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1 @@ +{} diff --git a/src/hooks/useInternationalization.tsx b/src/hooks/useInternationalization.tsx index d3e167e2..79605fe0 100644 --- a/src/hooks/useInternationalization.tsx +++ b/src/hooks/useInternationalization.tsx @@ -149,7 +149,7 @@ export function I18nProvider({ // Translation function const t = useCallback( - (key: string, params?: Record) => { + (key: string, params?: Record) => { return getTranslation(translations, key, params); }, [translations], @@ -247,7 +247,7 @@ export function useInternationalization(): I18nContextValue { if (!context) { const fallbackLanguage: LanguageCode = DEFAULT_LANGUAGE; const fallbackPreferences = getCulturalPreferences(fallbackLanguage); - const fallbackT = (key: string, params?: Record) => + const fallbackT = (key: string, params?: Record) => getTranslation({}, key, params); return { diff --git a/src/locales/translationManager.test.ts b/src/locales/translationManager.test.ts new file mode 100644 index 00000000..ea0316be --- /dev/null +++ b/src/locales/translationManager.test.ts @@ -0,0 +1,161 @@ +/** + * Tests for getTranslation parameter interpolation in translationManager.ts + * + * Covers: + * - Flat key replacement (regression) + * - Nested dot-separated key replacement (bug fix) + * - Missing param key warning (bug fix) + * - No-params passthrough (regression) + * - Multiple placeholders in one template (regression) + * - Unknown translation key passthrough (regression) + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { getTranslation } from './translationManager'; +import type { Translations } from './types'; + +// --------------------------------------------------------------------------- +// Shared fixture +// --------------------------------------------------------------------------- + +const translations: Translations = { + greetings: { + hello: 'Hello {{name}}', + nested: 'Hello {{user.name}}', + deepNested: 'Hello {{user.profile.displayName}}', + multi: 'Hello {{user.name}}, you have {{count}} messages', + plain: 'No placeholders here', + }, +}; + +// --------------------------------------------------------------------------- +// Helper to spy on console.warn +// --------------------------------------------------------------------------- + +let warnSpy: ReturnType; + +beforeEach(() => { + warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); +}); + +afterEach(() => { + warnSpy.mockRestore(); +}); + +// --------------------------------------------------------------------------- +// Regression: flat key replacement still works +// --------------------------------------------------------------------------- + +describe('flat key replacement (regression)', () => { + it('replaces a simple flat placeholder', () => { + const result = getTranslation(translations, 'greetings.hello', { name: 'Alice' }); + expect(result).toBe('Hello Alice'); + }); + + it('does not warn when flat key is present', () => { + getTranslation(translations, 'greetings.hello', { name: 'Alice' }); + expect(warnSpy).not.toHaveBeenCalled(); + }); +}); + +// --------------------------------------------------------------------------- +// Bug fix: nested dot-separated key replacement +// --------------------------------------------------------------------------- + +describe('nested dot-separated key replacement (bug fix)', () => { + it('resolves a two-level nested key', () => { + const result = getTranslation(translations, 'greetings.nested', { + user: { name: 'Alice' }, + }); + expect(result).toBe('Hello Alice'); + }); + + it('resolves a three-level nested key', () => { + const result = getTranslation(translations, 'greetings.deepNested', { + user: { profile: { displayName: 'Alice Wonder' } }, + }); + expect(result).toBe('Hello Alice Wonder'); + }); + + it('does not warn when nested key resolves successfully', () => { + getTranslation(translations, 'greetings.nested', { user: { name: 'Alice' } }); + expect(warnSpy).not.toHaveBeenCalled(); + }); +}); + +// --------------------------------------------------------------------------- +// Bug fix: missing interpolation key warns and leaves placeholder visible +// --------------------------------------------------------------------------- + +describe('missing interpolation key (bug fix)', () => { + it('leaves the placeholder in the output when nested key is missing', () => { + const result = getTranslation(translations, 'greetings.nested', { + user: {}, + }); + expect(result).toBe('Hello {{user.name}}'); + }); + + it('emits a console.warn mentioning the missing key path', () => { + getTranslation(translations, 'greetings.nested', { user: {} }); + expect(warnSpy).toHaveBeenCalledOnce(); + expect(warnSpy.mock.calls[0][0]).toContain('user.name'); + }); + + it('leaves the placeholder when the top-level param object is missing the key', () => { + const result = getTranslation(translations, 'greetings.hello', {}); + expect(result).toBe('Hello {{name}}'); + }); + + it('emits a console.warn for a missing flat key', () => { + getTranslation(translations, 'greetings.hello', {}); + expect(warnSpy).toHaveBeenCalledOnce(); + expect(warnSpy.mock.calls[0][0]).toContain('name'); + }); +}); + +// --------------------------------------------------------------------------- +// Regression: no params → raw string returned unchanged +// --------------------------------------------------------------------------- + +describe('no params passthrough (regression)', () => { + it('returns the template string unchanged when params is undefined', () => { + const result = getTranslation(translations, 'greetings.nested'); + expect(result).toBe('Hello {{user.name}}'); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it('returns a plain string unchanged when there are no placeholders', () => { + const result = getTranslation(translations, 'greetings.plain'); + expect(result).toBe('No placeholders here'); + }); +}); + +// --------------------------------------------------------------------------- +// Regression: multiple placeholders resolved in a single pass +// --------------------------------------------------------------------------- + +describe('multiple placeholders in one template (regression)', () => { + it('replaces all placeholders including nested and flat in one pass', () => { + const result = getTranslation(translations, 'greetings.multi', { + user: { name: 'Alice' }, + count: 5, + }); + expect(result).toBe('Hello Alice, you have 5 messages'); + }); + + it('warns once per missing placeholder when multiple are absent', () => { + getTranslation(translations, 'greetings.multi', {}); + expect(warnSpy).toHaveBeenCalledTimes(2); + }); +}); + +// --------------------------------------------------------------------------- +// Regression: unknown translation key returns key unchanged +// --------------------------------------------------------------------------- + +describe('unknown translation key passthrough (regression)', () => { + it('returns the key string when the translation path does not exist', () => { + const result = getTranslation(translations, 'greetings.nonExistent', { name: 'Alice' }); + expect(result).toBe('greetings.nonExistent'); + }); +}); diff --git a/src/locales/translationManager.ts b/src/locales/translationManager.ts index da19ffdf..262ce9f2 100644 --- a/src/locales/translationManager.ts +++ b/src/locales/translationManager.ts @@ -65,7 +65,7 @@ export async function preloadTranslations(languages: LanguageCode[]): Promise, + params?: Record, ): string { const keys = key.split('.'); let value: any = translations; @@ -83,9 +83,24 @@ export function getTranslation( } // Replace parameters in translation string + // Regex supports dot-separated nested keys, e.g. {{user.name}} if (params) { - return value.replace(/\{\{(\w+)\}\}/g, (match, paramKey) => { - return params[paramKey]?.toString() || match; + return value.replace(/\{\{([\w][\w.]*)\}\}/g, (match, paramPath: string) => { + const resolved = paramPath.split('.').reduce((obj: unknown, key: string) => { + if (obj !== null && obj !== undefined && typeof obj === 'object') { + return (obj as Record)[key]; + } + return undefined; + }, params as unknown); + + if (resolved === undefined || resolved === null) { + console.warn( + `[translationManager] Missing interpolation key "${paramPath}" in translation template.`, + ); + return match; // leave placeholder visible + } + + return String(resolved); }); } From e174412e220c2bde11a78412fa4d9ad5a1cd8580 Mon Sep 17 00:00:00 2001 From: innovativedesign-lab Date: Sat, 27 Jun 2026 17:58:43 +0100 Subject: [PATCH 2/2] =?UTF-8?q?Rate=20limiting=20is=20in-memory=20only=20?= =?UTF-8?q?=E2=80=94=20fails=20silently=20in=20multi-server=20and=20edge?= =?UTF-8?q?=20deployments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../distributed-rate-limiting/.config.kiro | 1 + .../specs/distributed-rate-limiting/design.md | 367 ++++++++++++++++++ .../distributed-rate-limiting/requirements.md | 111 ++++++ .../specs/distributed-rate-limiting/tasks.md | 122 ++++++ 4 files changed, 601 insertions(+) create mode 100644 .kiro/specs/distributed-rate-limiting/.config.kiro create mode 100644 .kiro/specs/distributed-rate-limiting/design.md create mode 100644 .kiro/specs/distributed-rate-limiting/requirements.md create mode 100644 .kiro/specs/distributed-rate-limiting/tasks.md diff --git a/.kiro/specs/distributed-rate-limiting/.config.kiro b/.kiro/specs/distributed-rate-limiting/.config.kiro new file mode 100644 index 00000000..d5a2701d --- /dev/null +++ b/.kiro/specs/distributed-rate-limiting/.config.kiro @@ -0,0 +1 @@ +{"specId": "6c84e8d5-a68c-431b-90ee-558702173e54", "workflowType": "requirements-first", "specType": "feature"} \ No newline at end of file diff --git a/.kiro/specs/distributed-rate-limiting/design.md b/.kiro/specs/distributed-rate-limiting/design.md new file mode 100644 index 00000000..90c238e9 --- /dev/null +++ b/.kiro/specs/distributed-rate-limiting/design.md @@ -0,0 +1,367 @@ +# Distributed Rate Limiting — Technical Design + +## 1. Overview + +The current rate limiter (`src/lib/ratelimit.ts`) uses an in-process `Map` for state. This works fine for a single server but breaks in distributed deployments: each instance maintains its own counter, so a client can exceed the intended limit by hitting different pods. + +The solution uses a **strategy pattern**: introduce an `IRateLimiterStore` interface with two implementations — `InMemoryStore` (the existing Map logic, unchanged) and `UpstashStore` (backed by Upstash Redis using the `@upstash/ratelimit` SDK). A `createStore()` factory selects the right implementation at module load time based on environment variables. + +To maintain full backward compatibility with the 20+ existing call sites, `withRateLimit` stays synchronous and in-memory-backed. A new `withRateLimitAsync` function provides the distributed-capable, async-first API that route handlers can be incrementally migrated to. + +--- + +## 2. Architecture + +### Component Diagram + +``` +┌─────────────────────────────────────────────────────────────┐ +│ ratelimit.ts │ +│ │ +│ ┌──────────────────────────────────────────────────────┐ │ +│ │ IRateLimiterStore (interface) │ │ +│ │ check(identifier, config): Promise │ │ +│ └──────────────┬──────────────────────┬────────────────┘ │ +│ │ │ │ +│ ┌──────────────▼──────┐ ┌────────────▼──────────────┐ │ +│ │ InMemoryStore │ │ UpstashStore │ │ +│ │ (synchronous Map) │ │ (@upstash/ratelimit SDK) │ │ +│ │ used in dev/test │ │ used in production when │ │ +│ │ + prod fallback │ │ env vars are present │ │ +│ └─────────────────────┘ └───────────────────────────-┘ │ +│ ▲ ▲ │ +│ └──────────┬───────────┘ │ +│ ┌───────▼────────┐ │ +│ │ createStore() │ ← reads env vars once │ +│ │ factory │ at module init │ +│ └───────┬────────┘ │ +│ │ │ +│ const store = createStore() (module-level) │ +│ │ │ +│ ┌─────────────────────────▼──────────────────────────────┐ │ +│ │ slidingWindowRateLimit(identifier, config) │ │ +│ │ → delegates to store.check() (sync path via InMemory) │ │ +│ └─────────────────────────────────────────────────────────┘ │ +│ │ +│ ┌──────────────────────────────────────────────────────┐ │ +│ │ withRateLimit(request, tier) ← UNCHANGED │ │ +│ │ synchronous | InMemoryStore only | 20+ call sites │ │ +│ └──────────────────────────────────────────────────────┘ │ +│ │ +│ ┌──────────────────────────────────────────────────────┐ │ +│ │ withRateLimitAsync(request, tier) ← NEW │ │ +│ │ async | active store (Upstash in prod) │ │ +│ │ route handlers migrate to this incrementally │ │ +│ └──────────────────────────────────────────────────────┘ │ +└─────────────────────────────────────────────────────────────┘ +``` + +### Store Selection Logic (`createStore`) + +``` +NODE_ENV === 'test' + → InMemoryStore (always) + +NODE_ENV === 'development' + → InMemoryStore (always) + +NODE_ENV === 'production' + UPSTASH_REDIS_REST_URL && UPSTASH_REDIS_REST_TOKEN present? + → UpstashStore + else + → InMemoryStore (with console.warn logged once at startup) +``` + +--- + +## 3. Key Design Decisions + +### 3.1 Dual-function API (withRateLimit + withRateLimitAsync) + +Making `withRateLimit` async would silently break all 20+ call sites — the function would return a `Promise` instead of `{ addHeaders, rateLimitResponse }`, so `if (rateLimitResponse)` would always be truthy and rate limiting would stop working entirely with no compile error. That's an unacceptable silent failure. + +The chosen approach: + +- `withRateLimit` remains **synchronous**, backed exclusively by `InMemoryStore`. No call site changes needed. +- `withRateLimitAsync` is the new **async** function backed by the active store (Upstash in production, InMemory in dev/test). Route handlers migrate to it incrementally by adding `await` and updating the import. + +This allows zero-downtime rollout: deploy the new code, then migrate route handlers one by one, verify each, and eventually deprecate `withRateLimit`. + +### 3.2 Store instantiated once at module init + +`createStore()` is called once at the top level (`const store = createStore()`), not per request. This: + +- Avoids re-reading `process.env` on every request (negligible cost, but good practice) +- Ensures Upstash `Ratelimit` instances are created once and cached +- Makes the store selection decision visible at startup (a startup log or warning can be emitted) + +### 3.3 Upstash Ratelimit instance caching + +The `@upstash/ratelimit` library creates a `Ratelimit` instance per (limit, windowMs) pair. Creating one per request would be wasteful. `UpstashStore` caches instances in an internal `Map` keyed on `"${limit}:${windowMs}"`. + +```typescript +// Key: "5:60000" for AUTH, "30:60000" for WRITE, "60:60000" for READ +private cache = new Map(); +``` + +### 3.4 Fail-open on Redis error + +Every call in `UpstashStore.check()` is wrapped in `try/catch`. On any Redis error: + +1. `console.error('[UpstashStore] Redis error, failing open:', err)` is logged +2. A permissive result is returned: `{ success: true, remaining: config.limit - 1, reset: Date.now() + config.windowMs, limit: config.limit }` + +This satisfies R5: AUTH tier (5/min) is enforced globally when Redis is healthy; the system fails open (allows the request) rather than taking the service down when Redis is unavailable. + +### 3.5 Sliding window algorithm consistency (R4) + +- `InMemoryStore` preserves the existing `slidingWindowRateLimit` Map logic exactly — no algorithm changes. +- `UpstashStore` uses `Ratelimit.slidingWindow(limit, duration)` from `@upstash/ratelimit`, which implements the same sliding window semantics server-side in Redis. + +### 3.6 Test isolation + +`InMemoryStore` uses a module-level `Map`. To prevent state leakage between tests, a `resetStoreForTesting()` function is exported. Test suites call it in `beforeEach`: + +```typescript +// In test files: +import { resetStoreForTesting } from '@/lib/ratelimit'; +beforeEach(() => resetStoreForTesting()); +``` + +`vi.useFakeTimers()` continues to work because `InMemoryStore` uses `Date.now()` directly, which Vitest's fake timer intercepts. + +--- + +## 4. File Changes + +| File | Change | Reason | +| ------------------------- | --------------------- | -------------------------------------------------------------------------------------------------------------------------------------------- | +| `src/lib/ratelimit.ts` | Full rewrite | Add `IRateLimiterStore`, `InMemoryStore`, `UpstashStore`, `createStore()` factory; add `withRateLimitAsync`; export `resetStoreForTesting()` | +| `package.json` | Add two dependencies | `@upstash/ratelimit` and `@upstash/redis` | +| `.env.example` | Add two env vars | `UPSTASH_REDIS_REST_URL` and `UPSTASH_REDIS_REST_TOKEN` with documentation | +| Route handler files (20+) | Optional, incremental | Replace `withRateLimit` → `withRateLimitAsync` + `await` as teams migrate | + +No route file changes are required for the initial delivery. Existing call sites continue to use `withRateLimit` unchanged. + +--- + +## 5. Data Models / Interfaces + +### IRateLimiterStore + +```typescript +interface IRateLimiterStore { + /** + * Check and increment the rate limit counter for the given identifier. + * Implementations may be synchronous (InMemory) or async (Upstash). + */ + check(identifier: string, config: RateLimitConfig): Promise; +} +``` + +### Updated withRateLimit (unchanged signature) + +```typescript +// Synchronous — backed by InMemoryStore only. +// Existing 20+ call sites continue to work with zero changes. +export function withRateLimit( + request: T, + tier: RateLimitTier, +): { + addHeaders: (response: Response | NextResponse) => NextResponse; + rateLimitResponse: NextResponse | null; +}; +``` + +### New withRateLimitAsync + +```typescript +// Async — backed by the active store (Upstash in prod, InMemory in dev/test). +// Route handlers migrate to this incrementally. +export async function withRateLimitAsync( + request: T, + tier: RateLimitTier, +): Promise<{ + addHeaders: (response: Response | NextResponse) => NextResponse; + rateLimitResponse: NextResponse | null; +}>; +``` + +### InMemoryStore (internal) + +```typescript +class InMemoryStore implements IRateLimiterStore { + private entries = new Map(); + + async check(identifier: string, config: RateLimitConfig): Promise { + // Existing sliding window logic, migrated from module-level function. + // Uses Date.now() — compatible with vi.useFakeTimers(). + } + + reset(): void { + this.entries.clear(); + } +} +``` + +### UpstashStore (internal) + +```typescript +class UpstashStore implements IRateLimiterStore { + private redis: Redis; + private cache = new Map(); + + constructor() { + this.redis = Redis.fromEnv(); // reads UPSTASH_REDIS_REST_URL + UPSTASH_REDIS_REST_TOKEN + } + + private getInstance(config: RateLimitConfig): Ratelimit { + const key = `${config.limit}:${config.windowMs}`; + if (!this.cache.has(key)) { + this.cache.set( + key, + new Ratelimit({ + redis: this.redis, + limiter: Ratelimit.slidingWindow(config.limit, `${config.windowMs / 1000} s`), + }), + ); + } + return this.cache.get(key)!; + } + + async check(identifier: string, config: RateLimitConfig): Promise { + try { + const ratelimit = this.getInstance(config); + const result = await ratelimit.limit(identifier); + return { + success: result.success, + remaining: result.remaining, + reset: result.reset, + limit: config.limit, + retryAfter: result.success ? undefined : Math.ceil((result.reset - Date.now()) / 1000), + }; + } catch (err) { + console.error('[UpstashStore] Redis error, failing open:', err); + return { + success: true, + remaining: config.limit - 1, + reset: Date.now() + config.windowMs, + limit: config.limit, + }; + } + } +} +``` + +### createStore factory + +```typescript +function createStore(): IRateLimiterStore { + if ( + process.env.NODE_ENV !== 'test' && + process.env.UPSTASH_REDIS_REST_URL && + process.env.UPSTASH_REDIS_REST_TOKEN + ) { + return new UpstashStore(); + } + + if (process.env.NODE_ENV === 'production') { + console.warn( + '[ratelimit] UPSTASH_REDIS_REST_URL or UPSTASH_REDIS_REST_TOKEN not set. ' + + 'Falling back to in-memory rate limiting — counters are NOT shared across instances.', + ); + } + + return new InMemoryStore(); +} + +const store = createStore(); // executed once at module load +``` + +--- + +## 6. Environment Variables + +Add the following to `.env.example`: + +```dotenv +# Distributed Rate Limiting (Upstash Redis) +# When both vars are set in production, rate limit counters are shared across +# all server instances, enabling accurate global enforcement of per-tier limits. +# If either var is missing, the system falls back to per-instance in-memory counters +# and logs a warning at startup. +# +# Obtain these values from your Upstash Redis database dashboard: +# https://console.upstash.com/ +UPSTASH_REDIS_REST_URL=https://your-database.upstash.io +UPSTASH_REDIS_REST_TOKEN=your_upstash_redis_rest_token +``` + +These variables are read exclusively by `UpstashStore` via `Redis.fromEnv()` from `@upstash/redis`. They are never exposed to the browser (no `NEXT_PUBLIC_` prefix). + +| Variable | Required | Description | +| -------------------------- | --------------- | ------------------------------------------------- | +| `UPSTASH_REDIS_REST_URL` | Production only | REST endpoint URL for your Upstash Redis database | +| `UPSTASH_REDIS_REST_TOKEN` | Production only | Bearer token for authenticating with the REST API | + +--- + +## 7. Backward Compatibility + +### Existing call sites — zero changes required + +All 20+ existing call sites follow this pattern: + +```typescript +const { addHeaders, rateLimitResponse } = withRateLimit(request, 'READ'); +if (rateLimitResponse) return rateLimitResponse; +// ... handler logic ... +return addHeaders(successResponse); +``` + +`withRateLimit` remains synchronous and its signature is unchanged. These call sites continue to work without modification. Rate limiting in this path uses the `InMemoryStore`, providing the same per-instance behavior as today. + +### Migrating call sites to distributed limiting (incremental) + +To enable distributed rate limiting for a route, change the call site from: + +```typescript +// Before +const { addHeaders, rateLimitResponse } = withRateLimit(request, 'WRITE'); +``` + +to: + +```typescript +// After — route handler must already be async (Next.js route handlers always are) +const { addHeaders, rateLimitResponse } = await withRateLimitAsync(request, 'WRITE'); +``` + +That is the only required change per call site. Because Next.js route handlers are `async` by convention, adding `await` is a one-line, safe, mechanical change. + +### Migration strategy + +1. Deploy the updated `ratelimit.ts` with Upstash env vars configured in production. +2. Migrate call sites in priority order: AUTH endpoints first (highest security value), then WRITE, then READ. +3. Verify each migrated endpoint with integration tests before proceeding. +4. Once all call sites are migrated, deprecate and eventually remove `withRateLimit`. + +### Why not make withRateLimit async immediately? + +Changing `withRateLimit` to return `Promise<...>` would cause `rateLimitResponse` to always be a resolved `Promise` object (truthy), meaning **every request would be rate-limited and blocked** — a silent, complete outage. TypeScript would not catch this without explicit `await`, and the pattern `if (rateLimitResponse)` would always pass. The dual-function approach avoids this failure mode entirely. + +--- + +## 8. Dependencies + +```jsonc +// package.json additions +{ + "dependencies": { + "@upstash/redis": "^1.34.3", // HTTP-based Redis client for Upstash + "@upstash/ratelimit": "^2.0.5" // Sliding window rate limiting on top of Upstash Redis + } +} +``` + +Both packages use the Upstash REST API over HTTPS, making them compatible with serverless and edge environments (including Next.js API routes and middleware). No persistent TCP connections are required. diff --git a/.kiro/specs/distributed-rate-limiting/requirements.md b/.kiro/specs/distributed-rate-limiting/requirements.md new file mode 100644 index 00000000..198cc687 --- /dev/null +++ b/.kiro/specs/distributed-rate-limiting/requirements.md @@ -0,0 +1,111 @@ +# Requirements Document + +## Introduction + +The current rate limiter in `src/lib/ratelimit.ts` uses a module-level in-memory `Map` as its counter store. In a multi-instance deployment (e.g., multiple Next.js server pods or Vercel edge nodes), each instance maintains independent counters, allowing a client to bypass the AUTH rate limit (5 req/min) by spreading requests across instances. + +This feature replaces the single-instance in-memory store with a shared [Upstash Redis](https://upstash.com/) backend in production, while preserving the existing in-memory behavior for development and test environments. All 20+ `withRateLimit()` call sites must continue to work without modification — the change is a pure internal implementation swap. + +## Glossary + +- **RateLimiter**: The abstraction that performs counter reads and writes. Either `InMemoryRateLimiter` (dev/test) or `UpstashRateLimiter` (production). +- **InMemoryRateLimiter**: The existing `Map`-backed sliding window implementation used in development and test. +- **UpstashRateLimiter**: A Redis-backed sliding window implementation that uses Upstash REST API calls to maintain shared counters. +- **RateLimit_Store**: The backing storage for rate-limit counters (either the in-memory `Map` or the Upstash Redis instance). +- **Sliding_Window**: The rate-limiting algorithm that counts requests within a rolling time window and resets the counter after `windowMs` milliseconds. +- **Tier**: A named rate-limit configuration (`AUTH`, `WRITE`, `READ`) with a `limit` and `windowMs`. +- **Identifier**: A string key combining client IP and tier (e.g., `"1.2.3.4:AUTH"`) used to namespace counters in the RateLimit_Store. +- **UPSTASH_REDIS_REST_URL**: Environment variable holding the Upstash Redis REST endpoint URL. +- **UPSTASH_REDIS_REST_TOKEN**: Environment variable holding the Upstash Redis REST API token. +- **NODE_ENV**: Node.js environment variable (`"production"`, `"development"`, or `"test"`). + +--- + +## Requirements + +### Requirement 1: Shared Counter Backend in Production + +**User Story:** As a platform operator, I want rate-limit counters to be stored in a shared Redis backend in production, so that rate limits are enforced consistently across all server instances and edge nodes. + +#### Acceptance Criteria + +1. WHEN `NODE_ENV` is `"production"` AND `UPSTASH_REDIS_REST_URL` and `UPSTASH_REDIS_REST_TOKEN` are set, THE `RateLimiter` SHALL use the `UpstashRateLimiter` as its `RateLimit_Store`. +2. WHEN the `UpstashRateLimiter` is active, THE `RateLimiter` SHALL increment and read counters via the Upstash Redis REST API for every call to `slidingWindowRateLimit`. +3. WHEN two separate server instances call `slidingWindowRateLimit` with the same `Identifier`, THE `UpstashRateLimiter` SHALL reflect a combined request count that is the sum of both instances' requests within the same `Sliding_Window`. +4. WHEN a client's combined request count across all instances reaches the configured `limit` for a `Tier`, THE `RateLimiter` SHALL return `success: false` for subsequent requests within the same `Sliding_Window`. + +--- + +### Requirement 2: In-Memory Fallback for Development and Test + +**User Story:** As a developer, I want the rate limiter to work without Redis configured in development and test, so that local development and CI test runs are not blocked by external service dependencies. + +#### Acceptance Criteria + +1. WHEN `NODE_ENV` is `"development"` OR `NODE_ENV` is `"test"`, THE `RateLimiter` SHALL use the `InMemoryRateLimiter` as its `RateLimit_Store`, regardless of whether Upstash environment variables are set. +2. WHEN `UPSTASH_REDIS_REST_URL` or `UPSTASH_REDIS_REST_TOKEN` is absent AND `NODE_ENV` is `"production"`, THE `RateLimiter` SHALL fall back to the `InMemoryRateLimiter` and log a warning at startup indicating that distributed rate limiting is disabled. +3. WHILE the `InMemoryRateLimiter` is active, THE `RateLimiter` SHALL preserve all existing `Sliding_Window` semantics: allowing requests up to `limit`, blocking further requests until `windowMs` elapses, and returning correct `remaining` and `reset` values. THE `remaining` value SHALL reflect `limit - count` and MAY exceed the configured `limit` in edge cases where the `limit` is dynamically reduced after counters are initialized. + +--- + +### Requirement 3: Backward-Compatible Public API + +**User Story:** As a developer maintaining route handlers, I want the `withRateLimit()` function signature and return shape to remain unchanged, so that no call sites require modification after the refactor. + +#### Acceptance Criteria + +1. THE `RateLimiter` SHALL export `withRateLimit(request, tier)` with the same signature: accepting a `Request` and a `RateLimitTier`, returning `{ addHeaders, rateLimitResponse }`. +2. THE `RateLimiter` SHALL export `slidingWindowRateLimit(identifier, config)` with the same signature: accepting a `string` and `RateLimitConfig`, returning `RateLimitResult`. +3. THE `RateLimiter` SHALL export `RATE_LIMIT_TIERS`, `RateLimitTier`, `RateLimitConfig`, `RateLimitResult`, `RateLimitInfo`, and `createRateLimitResponse` with unchanged types and values. +4. WHEN `withRateLimit` is called from any existing route handler, THE `RateLimiter` SHALL return a `rateLimitResponse` of `null` for allowed requests and a `NextResponse` with status `429` for blocked requests, identical to the current behavior. THE `RateLimiter` SHALL never return a non-null `rateLimitResponse` for a request that was allowed by the `RateLimit_Store`. + +--- + +### Requirement 4: Sliding Window Algorithm Correctness + +**User Story:** As a platform operator, I want the sliding window algorithm to behave identically whether backed by in-memory or Upstash storage, so that rate-limit decisions are consistent and predictable. + +#### Acceptance Criteria + +1. WHEN a new `Identifier` is first seen within a `Sliding_Window`, THE `RateLimiter` SHALL initialize a counter at `1` and set `resetAt` to `now + windowMs`. +2. WHEN an existing `Identifier`'s counter is below `limit`, THE `RateLimiter` SHALL increment the counter by `1` and return `success: true` with `remaining` equal to `limit - count`. +3. WHEN an existing `Identifier`'s counter equals or exceeds `limit`, THE `RateLimiter` SHALL return `success: false` with `remaining` equal to `0` and `retryAfter` equal to the ceiling of `(resetAt - now) / 1000` seconds. +4. WHEN the current time exceeds `resetAt` for an `Identifier`, THE `RateLimiter` SHALL reset the counter to `1` and set a new `resetAt` to `now + windowMs`, treating the request as the first in a new window. +5. FOR ALL valid `(identifier, config)` pairs, the sequence `[allow × limit, block × 1, advance time by windowMs, allow × 1]` SHALL hold for both `InMemoryRateLimiter` and `UpstashRateLimiter` implementations. + +--- + +### Requirement 5: AUTH Tier Bypass Prevention + +**User Story:** As a security engineer, I want the AUTH rate limit (5 req/min) to be enforced globally, so that an attacker cannot bypass it by distributing requests across multiple server instances. + +#### Acceptance Criteria + +1. WHEN `NODE_ENV` is `"production"` AND Upstash credentials are configured, THE `UpstashRateLimiter` SHALL enforce the `AUTH` tier limit of `5` requests per `60,000 ms` window as a single shared counter across all instances. +2. WHEN the shared request count across all instances reaches exactly the `AUTH` tier `limit` of `5` within the same `Sliding_Window`, THE `UpstashRateLimiter` SHALL block the request that reaches the limit and all subsequent requests with `success: false`, without permitting an additional request beyond the limit. +3. IF the Upstash REST API call fails during an AUTH tier check, THEN THE `RateLimiter` SHALL fail open (allow the request) and log the error, to prevent service disruption due to Redis unavailability. + +--- + +### Requirement 6: Upstash Package and Environment Configuration + +**User Story:** As a developer onboarding to the project, I want all required packages installed and environment variables documented, so that I can configure distributed rate limiting without guessing at dependencies or config keys. + +#### Acceptance Criteria + +1. THE project SHALL declare `@upstash/ratelimit` and `@upstash/redis` as production dependencies in `package.json`. +2. THE `.env.example` file SHALL document `UPSTASH_REDIS_REST_URL` and `UPSTASH_REDIS_REST_TOKEN` with placeholder values and a comment explaining their purpose. +3. WHEN `UPSTASH_REDIS_REST_URL` or `UPSTASH_REDIS_REST_TOKEN` contains an invalid value at startup in production, THE `RateLimiter` SHALL log a descriptive error, fall back to `InMemoryRateLimiter` immediately, and prevent any attempt to initialize the `UpstashRateLimiter` for the remainder of the process lifetime. + +--- + +### Requirement 7: Existing Tests Pass with In-Memory Backend + +**User Story:** As a developer, I want the existing rate-limit test suite in `src/app/api/tutorials/__tests__/ratelimit.test.ts` to continue passing after the refactor, so that the behavioral contract of `slidingWindowRateLimit` and `withRateLimit` is preserved. + +#### Acceptance Criteria + +1. WHEN the test suite runs with `NODE_ENV` set to `"test"`, THE `RateLimiter` SHALL use the `InMemoryRateLimiter` so that no Upstash credentials or network calls are required. +2. THE `InMemoryRateLimiter` SHALL pass all existing test cases for `slidingWindowRateLimit`, including: allowing requests within the limit, blocking requests that exceed the limit, resetting the window after `windowMs` elapses, and returning correct `remaining` counts. +3. THE `InMemoryRateLimiter` SHALL pass all existing test cases for `withRateLimit`, including: `READ` and `WRITE` tier enforcement, independent tracking per IP and tier, and `addHeaders` injecting correct `X-RateLimit-*` headers. +4. WHEN `vi.useFakeTimers()` is active in a test, THE `InMemoryRateLimiter` SHOULD respect the mocked `Date.now()` value so that time-advance tests (`vi.advanceTimersByTime`) behave as expected. IF the limiter uses real time, time-advance tests SHALL still pass by relying on sufficiently long real time elapsed or by configuring real window durations. diff --git a/.kiro/specs/distributed-rate-limiting/tasks.md b/.kiro/specs/distributed-rate-limiting/tasks.md new file mode 100644 index 00000000..346cc671 --- /dev/null +++ b/.kiro/specs/distributed-rate-limiting/tasks.md @@ -0,0 +1,122 @@ +# Implementation Plan + +## Overview + +Refactor `src/lib/ratelimit.ts` to support a shared Upstash Redis backend in production while keeping the existing in-memory behaviour in dev/test. The work proceeds in eight ordered steps: install dependencies, document configuration, extract the in-memory store, add the Upstash store, wire up the factory and the new async function, migrate all 25 call sites, verify existing tests, and add new distributed tests. + +## Tasks + +- [ ] 1. Install Upstash packages + Add `@upstash/ratelimit@^1.2.0` and `@upstash/redis@^1.34.0` to the `dependencies` section of `package.json`, then run `pnpm install` to lock the versions. These packages provide the SlidingWindow algorithm and the HTTP-based Redis client needed by `UpstashStore`. + **Acceptance**: `pnpm install` completes without errors; both packages appear in `package.json` dependencies and in `pnpm-lock.yaml` at the pinned minor versions. + **Files**: `package.json` + +- [ ] 2. Document env vars in .env.example + Add a `# Rate Limiting (Distributed)` section near the bottom of `.env.example` with two entries: `UPSTASH_REDIS_REST_URL=https://.upstash.io` and `UPSTASH_REDIS_REST_TOKEN=`, each preceded by a comment explaining its purpose. Do not add real credentials. + **Acceptance**: `.env.example` contains the new section with both placeholder vars and explanatory comments; existing entries are unchanged. + **Files**: `.env.example` + +- [ ] 3. Define IRateLimiterStore interface and InMemoryStore + In `src/lib/ratelimit.ts`, introduce the `IRateLimiterStore` interface with a single method `check(identifier: string, config: RateLimitConfig): Promise`. Extract the existing per-IP `Map` tracking logic into a new `InMemoryStore` class that implements this interface (the store remains synchronous internally but wraps results in `Promise.resolve`). Keep all existing exports — `withRateLimit`, `getClientIP`, `createRateLimitResponse`, `RATE_LIMIT_TIERS` — fully intact and behaviourally identical. Export `resetStoreForTesting()` which clears the in-memory map; this replaces any ad-hoc reset pattern in tests. + **Acceptance**: `pnpm test -- src/app/api/tutorials/__tests__/ratelimit.test.ts` passes without modification; TypeScript compiles with no new errors; `InMemoryStore` and `IRateLimiterStore` are exported from the module. + **Files**: `src/lib/ratelimit.ts` + +- [ ] 4. Implement UpstashStore + Add an `UpstashStore` class to `src/lib/ratelimit.ts` that implements `IRateLimiterStore`. Internally it creates a `@upstash/redis` `Redis` client from `UPSTASH_REDIS_REST_URL` and `UPSTASH_REDIS_REST_TOKEN`. It caches one `@upstash/ratelimit` `Ratelimit` instance per `(limit, windowMs)` pair using a `Map` keyed on `"${limit}:${windowMs}"`, so instances are reused across requests. The `check()` method calls `ratelimit.limit(identifier)`, maps the result to `RateLimitResult`, and wraps the entire call in a `try/catch`: on any error it calls `console.error` with the error and returns `{ success: true, limit: config.limit, remaining: config.limit, reset: Date.now() + config.windowMs }` (fail-open). + **Acceptance**: Unit tests (added in task 8) confirm that a simulated Redis error causes `check()` to return `success: true`; TypeScript compiles cleanly. + **Files**: `src/lib/ratelimit.ts` + +- [ ] 5. Implement createStore() factory and withRateLimitAsync() + Add a `createStore()` function that runs at module initialisation (not per-request). It reads `process.env.NODE_ENV`, `process.env.UPSTASH_REDIS_REST_URL`, and `process.env.UPSTASH_REDIS_REST_TOKEN`. If `NODE_ENV === 'production'` and both env vars are present, it returns a new `UpstashStore`; otherwise it returns a new `InMemoryStore`. If `NODE_ENV === 'production'` but either env var is missing, it also emits `console.warn('Distributed rate limiting is disabled: UPSTASH_REDIS_REST_URL or UPSTASH_REDIS_REST_TOKEN is not set. Falling back to in-memory store.')` before returning `InMemoryStore`. Assign the result to a module-level `const activeStore`. Then add `export async function withRateLimitAsync(request: Request, tier: RateLimitTier)` which resolves the client IP, calls `activeStore.check(ip, RATE_LIMIT_TIERS[tier])`, and returns the same `{ addHeaders, rateLimitResponse }` shape as `withRateLimit`. + **Acceptance**: Calling `withRateLimitAsync` in a test with `NODE_ENV=test` returns an `InMemoryStore`-backed result; TypeScript compiles cleanly; the `console.warn` fires when env vars are absent in production mode. + **Files**: `src/lib/ratelimit.ts` + +- [ ] 6. Migrate route call sites to withRateLimitAsync() + Update every route file listed below to: (1) add `withRateLimitAsync` to the existing `@/lib/ratelimit` import, (2) replace each `withRateLimit(request, tier)` call with `await withRateLimitAsync(request, tier)`, and (3) ensure the handler function is `async` (most already are). The synchronous `withRateLimit` import may be removed from each file once all call sites in that file are migrated. + + Files to migrate: + + - `src/app/api/user/progress/route.ts` + - `src/app/api/user/settings/route.ts` + - `src/app/api/video-analytics/route.ts` + - `src/app/api/admin/feature-flags/[id]/route.ts` + - `src/app/api/admin/feature-flags/route.ts` + - `src/app/api/admin/feature-flags/evaluate/route.ts` + - `src/app/api/admin/feature-flags/audit/route.ts` + - `src/app/api/admin/audit/route.ts` + - `src/app/api/tutorials/route.ts` + - `src/app/api/tutorials/[id]/route.ts` + - `src/app/api/tutorials/[id]/progress/route.ts` + - `src/app/api/referral/validate/route.ts` + - `src/app/api/v1/tickets/route.ts` + - `src/app/api/v1/tickets/[id]/route.ts` + - `src/app/api/v1/consent/route.ts` + - `src/app/api/notes/route.ts` + - `src/app/api/bookmarks/route.ts` + - `src/app/api/courses/route.ts` + - `src/app/api/courses/[id]/route.ts` + - `src/app/api/courses/[id]/lessons/route.ts` + - `src/app/api/lessons/[id]/progress/route.ts` + - `src/app/api/auth/discord/route.ts` + - `src/app/api/auth/discord/callback/route.ts` + - `src/app/api/auth/login/route.ts` + - `src/app/api/auth/signup/route.ts` + + **Acceptance**: TypeScript compiles with no errors across all migrated files; no remaining `withRateLimit(` call (without the `Async` suffix) exists in any of the above files; `pnpm build` succeeds. + **Files**: All 25 route files listed above. + +- [ ] 7. Verify existing tests pass + Run the existing rate limit test suite without modifying any test file. This confirms that the `withRateLimit` synchronous path and `InMemoryStore` extraction did not regress any existing behaviour. + **Acceptance**: `pnpm test -- src/app/api/tutorials/__tests__/ratelimit.test.ts` exits with code 0 and all test cases pass. + **Files**: _(no changes — read-only verification step)_ + +- [ ] 8. Add tests for withRateLimitAsync and UpstashStore + Create `src/lib/__tests__/ratelimit-distributed.test.ts` with the following test cases: + 1. **InMemoryStore fallback in dev/test** — with `NODE_ENV=test`, `withRateLimitAsync` resolves without error and returns `rateLimitResponse: null` under the limit. + 1. **UpstashStore used in production** — mock `process.env.NODE_ENV='production'` and both Upstash env vars; spy on `UpstashStore.prototype.check` and verify it is called by `withRateLimitAsync`. + 1. **Fail-open on Redis error** — mock `UpstashStore.prototype.check` to throw; verify `withRateLimitAsync` still returns `rateLimitResponse: null` (request is allowed through). + 1. **console.warn on missing prod env vars** — set `NODE_ENV=production` and omit the Upstash vars; spy on `console.warn` and verify the warning message is emitted at module init. + 1. **resetStoreForTesting() clears state** — exhaust the in-memory limit for an IP, call `resetStoreForTesting()`, then confirm the next request is allowed. + **Acceptance**: `pnpm test -- src/lib/__tests__/ratelimit-distributed.test.ts` exits with code 0 and all five test cases pass. + **Files**: `src/lib/__tests__/ratelimit-distributed.test.ts` + +## Task Dependency Graph + +```json +{ + "waves": [ + { + "wave": 1, + "tasks": ["1. Install Upstash packages", "2. Document env vars in .env.example"] + }, + { + "wave": 2, + "tasks": ["3. Define IRateLimiterStore interface and InMemoryStore"] + }, + { + "wave": 3, + "tasks": ["4. Implement UpstashStore"] + }, + { + "wave": 4, + "tasks": ["5. Implement createStore() factory and withRateLimitAsync()"] + }, + { + "wave": 5, + "tasks": [ + "6. Migrate route call sites to withRateLimitAsync()", + "7. Verify existing tests pass", + "8. Add tests for withRateLimitAsync and UpstashStore" + ] + } + ] +} +``` + +## Notes + +- `withRateLimit()` must remain synchronous and unchanged throughout — it is the safety net for any call site not yet migrated and is relied upon by existing tests. +- The `UpstashStore` is never instantiated in test or dev environments; the `createStore()` factory guarantees this. Tests that need to exercise `UpstashStore` directly must mock it. +- Task 6 lists 25 files discovered via grep at spec-authoring time. Run `grep -r 'withRateLimit(' src/app/api --include='*.ts' -l` before starting that task to catch any new files added since. +- The fail-open contract (`success: true` on any Redis error) is a deliberate trade-off: availability over strict rate enforcement. Document this in a code comment above `UpstashStore.check()`. +- `resetStoreForTesting()` should only be called from test code. Consider adding a runtime guard (`if (process.env.NODE_ENV === 'production') throw new Error(...)`) to prevent accidental misuse.