From 110e4ff1aaa82a70388c9a579349e0673daa8b25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hartmut=20N=C3=B6renberg?= Date: Sat, 11 Apr 2026 08:03:42 +0200 Subject: [PATCH] fix(security): harden auth reset, rate limiter fallback, and CI secrets - Move CI_AUTH_SECRET from plaintext to ${{ secrets.CI_AUTH_SECRET }} - Wrap password reset (update + session kill + token mark) in $transaction to prevent stale sessions on partial failure (CWE-613) - Rate limiter Redis fallback now uses stricter degraded limits (maxRequests/10) and logs at error level instead of warn Co-Authored-By: Claude Opus 4.6 --- .github/workflows/ci.yml | 2 +- .../src/__tests__/auth-password-reset.test.ts | 4 +- packages/api/src/__tests__/rate-limit.test.ts | 32 +++++----- packages/api/src/middleware/rate-limit.ts | 63 +++++++++++-------- packages/api/src/router/auth.ts | 30 +++++---- 5 files changed, 78 insertions(+), 53 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 873f228..ce4acba 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -14,7 +14,7 @@ env: NODE_VERSION: "20" PNPM_VERSION: "9.14.2" CI_AUTH_URL: http://localhost:3100 - CI_AUTH_SECRET: capakraken-ci-build-secret-rotate-if-shared + CI_AUTH_SECRET: ${{ secrets.CI_AUTH_SECRET }} jobs: guardrails: diff --git a/packages/api/src/__tests__/auth-password-reset.test.ts b/packages/api/src/__tests__/auth-password-reset.test.ts index 080acd8..c1b4e5b 100644 --- a/packages/api/src/__tests__/auth-password-reset.test.ts +++ b/packages/api/src/__tests__/auth-password-reset.test.ts @@ -26,7 +26,7 @@ function makeDb( resetToken?: Partial>; } = {}, ) { - return { + const db = { user: { findUnique: vi.fn().mockResolvedValue({ id: "user_1", email: "user@example.com" }), update: vi.fn().mockResolvedValue({ id: "user_1" }), @@ -42,7 +42,9 @@ function makeDb( update: vi.fn().mockResolvedValue({}), ...overrides.resetToken, }, + $transaction: vi.fn(async (fn: (tx: unknown) => Promise) => fn(db)), } as never; + return db; } function makeCtx(db = makeDb()) { diff --git a/packages/api/src/__tests__/rate-limit.test.ts b/packages/api/src/__tests__/rate-limit.test.ts index 09a1f62..e10eb93 100644 --- a/packages/api/src/__tests__/rate-limit.test.ts +++ b/packages/api/src/__tests__/rate-limit.test.ts @@ -47,18 +47,20 @@ describe("rate limiter", () => { vi.doMock("ioredis", () => ({ Redis: vi.fn().mockImplementation(() => ({ on: vi.fn(), - eval: vi.fn(async (_script: string, _numKeys: number, key: string, windowMsValue: string) => { - const now = Date.now(); - const windowMs = Number(windowMsValue); - const existing = store.get(key); - if (!existing || existing.resetAt <= now) { - store.set(key, { count: 1, resetAt: now + windowMs }); - } else { - existing.count += 1; - } - const current = store.get(key)!; - return [current.count, current.resetAt - now]; - }), + eval: vi.fn( + async (_script: string, _numKeys: number, key: string, windowMsValue: string) => { + const now = Date.now(); + const windowMs = Number(windowMsValue); + const existing = store.get(key); + if (!existing || existing.resetAt <= now) { + store.set(key, { count: 1, resetAt: now + windowMs }); + } else { + existing.count += 1; + } + const current = store.get(key)!; + return [current.count, current.resetAt - now]; + }, + ), scan: vi.fn(async () => ["0", [...store.keys()]]), del: delMock, })), @@ -88,7 +90,7 @@ describe("rate limiter", () => { expect(afterReset.remaining).toBe(1); }); - it("falls back to in-memory counters when Redis is unavailable", async () => { + it("falls back to degraded in-memory counters when Redis is unavailable", async () => { vi.doMock("ioredis", () => ({ Redis: vi.fn().mockImplementation(() => ({ on: vi.fn(), @@ -101,7 +103,9 @@ describe("rate limiter", () => { })); const { createRateLimiter } = await import("../middleware/rate-limit.js"); - const limiter = createRateLimiter(60_000, 2, { + // Degraded fallback uses max(1, floor(maxRequests/10)), so with + // maxRequests=20 the degraded limit is 2. + const limiter = createRateLimiter(60_000, 20, { backend: "redis", redisUrl: "redis://test", name: "redis-fallback-test", diff --git a/packages/api/src/middleware/rate-limit.ts b/packages/api/src/middleware/rate-limit.ts index 5e3eb98..75d2438 100644 --- a/packages/api/src/middleware/rate-limit.ts +++ b/packages/api/src/middleware/rate-limit.ts @@ -37,13 +37,19 @@ const DEFAULT_REDIS_URL = process.env["REDIS_URL"]?.trim(); const warnedRedisFailures = new Set(); let sharedRedisClient: Redis | null = null; -function getBackendMode( - requestedBackend: RateLimitBackendMode | undefined, -): RateLimitBackendMode { - if (requestedBackend === "memory" || requestedBackend === "redis" || requestedBackend === "auto") { +function getBackendMode(requestedBackend: RateLimitBackendMode | undefined): RateLimitBackendMode { + if ( + requestedBackend === "memory" || + requestedBackend === "redis" || + requestedBackend === "auto" + ) { return requestedBackend; } - if (DEFAULT_REDIS_BACKEND === "memory" || DEFAULT_REDIS_BACKEND === "redis" || DEFAULT_REDIS_BACKEND === "auto") { + if ( + DEFAULT_REDIS_BACKEND === "memory" || + DEFAULT_REDIS_BACKEND === "redis" || + DEFAULT_REDIS_BACKEND === "auto" + ) { return DEFAULT_REDIS_BACKEND; } return "auto"; @@ -69,10 +75,7 @@ function getRedisClient(redisUrl: string): Redis { return sharedRedisClient; } -function createMemoryBackend( - windowMs: number, - maxRequests: number, -): RateLimiterBackend { +function createMemoryBackend(windowMs: number, maxRequests: number): RateLimiterBackend { const store = new Map(); const cleanupInterval = setInterval(() => { const now = Date.now(); @@ -127,7 +130,7 @@ function createRedisBackend( async function runRedisCheck(key: string): Promise { const client = getRedisClient(options.redisUrl); const redisKey = `${redisKeyPrefix}:${key}`; - const result = await client.eval( + const result = (await client.eval( ` local current = redis.call("INCR", KEYS[1]) local ttl = redis.call("PTTL", KEYS[1]) @@ -140,7 +143,7 @@ function createRedisBackend( 1, redisKey, String(windowMs), - ) as [number | string, number | string]; + )) as [number | string, number | string]; const count = Number(result[0]); const ttlMs = Math.max(0, Number(result[1])); @@ -156,13 +159,7 @@ function createRedisBackend( const matchPattern = `${redisKeyPrefix}:*`; let cursor = "0"; do { - const [nextCursor, keys] = await client.scan( - cursor, - "MATCH", - matchPattern, - "COUNT", - 100, - ); + const [nextCursor, keys] = await client.scan(cursor, "MATCH", matchPattern, "COUNT", 100); cursor = nextCursor; if (keys.length > 0) { await client.del(...keys); @@ -177,9 +174,9 @@ function createRedisBackend( } catch (error) { if (!warnedRedisFailures.has(warningKey)) { warnedRedisFailures.add(warningKey); - logger.warn( + logger.error( { err: error, redisUrl: options.redisUrl, limiter: options.name }, - "Rate limiter Redis backend unavailable, falling back to in-memory counters", + "Rate limiter Redis backend unavailable, falling back to degraded in-memory counters", ); } throw error; @@ -208,9 +205,19 @@ export function createRateLimiter( const redisUrl = options.redisUrl?.trim() || DEFAULT_REDIS_URL; const keyPrefix = options.keyPrefix ?? DEFAULT_REDIS_KEY_PREFIX; const shouldUseRedis = backendMode === "redis" || (backendMode === "auto" && Boolean(redisUrl)); - const redisBackend = shouldUseRedis && redisUrl - ? createRedisBackend(windowMs, maxRequests, { name, redisUrl, keyPrefix }) - : null; + const redisBackend = + shouldUseRedis && redisUrl + ? createRedisBackend(windowMs, maxRequests, { name, redisUrl, keyPrefix }) + : null; + + // When Redis is unavailable, apply a stricter limit to compensate for + // per-node isolation (each process keeps independent in-memory counters, + // so the effective cluster-wide limit is maxRequests × nodeCount). + const degradedMemoryBackend = createMemoryBackend( + windowMs, + Math.max(1, Math.floor(maxRequests / 10)), + ); + let redisDegraded = false; const check = (async (key: string) => { const normalizedKey = key.trim().toLowerCase(); @@ -227,9 +234,15 @@ export function createRateLimiter( } try { - return await redisBackend.check(normalizedKey); + const result = await redisBackend.check(normalizedKey); + if (redisDegraded) { + redisDegraded = false; + logger.info({ limiter: name }, "Rate limiter Redis backend recovered"); + } + return result; } catch { - return memoryBackend.check(normalizedKey); + redisDegraded = true; + return degradedMemoryBackend.check(normalizedKey); } }) as RateLimiter; diff --git a/packages/api/src/router/auth.ts b/packages/api/src/router/auth.ts index efec654..32104f4 100644 --- a/packages/api/src/router/auth.ts +++ b/packages/api/src/router/auth.ts @@ -94,7 +94,10 @@ export const authRouter = createTRPCRouter({ throw new TRPCError({ code: "NOT_FOUND", message: "Reset link not found." }); } if (record.usedAt) { - throw new TRPCError({ code: "BAD_REQUEST", message: "This reset link has already been used." }); + throw new TRPCError({ + code: "BAD_REQUEST", + message: "This reset link has already been used.", + }); } if (record.expiresAt < new Date()) { throw new TRPCError({ code: "BAD_REQUEST", message: "This reset link has expired." }); @@ -103,19 +106,22 @@ export const authRouter = createTRPCRouter({ const { hash } = await import("@node-rs/argon2"); const passwordHash = await hash(input.password); - const updatedUser = await ctx.db.user.update({ - where: { email: record.email }, - data: { passwordHash }, - select: { id: true }, - }); + // All three operations must succeed atomically: if session deletion + // fails after the password is already changed, old sessions could + // persist with the new password (CWE-613). + await ctx.db.$transaction(async (tx) => { + const updatedUser = await tx.user.update({ + where: { email: record.email }, + data: { passwordHash }, + select: { id: true }, + }); - // Invalidate all active sessions so any session obtained before the - // password reset cannot be reused (CWE-613). - await ctx.db.activeSession.deleteMany({ where: { userId: updatedUser.id } }); + await tx.activeSession.deleteMany({ where: { userId: updatedUser.id } }); - await ctx.db.passwordResetToken.update({ - where: { token: input.token }, - data: { usedAt: new Date() }, + await tx.passwordResetToken.update({ + where: { token: input.token }, + data: { usedAt: new Date() }, + }); }); return { success: true };