From 1fb8f45705ed2764df7748a4096e7b51c89be253 Mon Sep 17 00:00:00 2001 From: Michael Freno Date: Sun, 11 Jan 2026 11:23:32 -0500 Subject: [PATCH] fix: session validation fixes --- src/routes/account.tsx | 2 +- src/server/api/routers/user.ts | 10 +-- src/server/security.ts | 101 +++++++++++++++++++------ src/server/security/rate-limit.test.ts | 73 ++++++++++++++++++ src/server/session-helpers.ts | 17 +++-- 5 files changed, 166 insertions(+), 37 deletions(-) diff --git a/src/routes/account.tsx b/src/routes/account.tsx index 0ca4843..d0fb90b 100644 --- a/src/routes/account.tsx +++ b/src/routes/account.tsx @@ -1261,7 +1261,7 @@ function ActiveSessions(props: { userId: string }) {
Last active:{" "} - {formatDate(session.lastRotatedAt || session.createdAt)} + {formatDate(session.lastActiveAt || session.createdAt)}
diff --git a/src/server/api/routers/user.ts b/src/server/api/routers/user.ts index 28b26bd..08de0cb 100644 --- a/src/server/api/routers/user.ts +++ b/src/server/api/routers/user.ts @@ -419,11 +419,11 @@ export const userRouter = createTRPCRouter({ const conn = ConnectionFactory(); const res = await conn.execute({ - sql: `SELECT session_id, token_family, created_at, expires_at, last_rotated_at, - rotation_count, client_ip, user_agent + sql: `SELECT session_id, token_family, created_at, expires_at, last_active_at, + rotation_count, ip_address, user_agent FROM Session WHERE user_id = ? AND revoked = 0 AND expires_at > datetime('now') - ORDER BY last_rotated_at DESC`, + ORDER BY last_active_at DESC`, args: [userId] }); @@ -435,9 +435,9 @@ export const userRouter = createTRPCRouter({ tokenFamily: row.token_family, createdAt: row.created_at, expiresAt: row.expires_at, - lastRotatedAt: row.last_rotated_at, + lastActiveAt: row.last_active_at, rotationCount: row.rotation_count, - clientIp: row.client_ip, + clientIp: row.ip_address, userAgent: row.user_agent, isCurrent: currentSession?.sessionId === row.session_id })); diff --git a/src/server/security.ts b/src/server/security.ts index 45adb66..06c3727 100644 --- a/src/server/security.ts +++ b/src/server/security.ts @@ -349,19 +349,33 @@ export const RATE_LIMITS = CONFIG_RATE_LIMITS; /** * Rate limiting middleware for login operations + * In development, skips IP rate limiting to avoid self-DoS + * For unknown IPs in production, uses stricter shared limits */ export async function rateLimitLogin( email: string, clientIP: string, event?: H3Event ): Promise { - await checkRateLimit( - `login:ip:${clientIP}`, - RATE_LIMITS.LOGIN_IP.maxAttempts, - RATE_LIMITS.LOGIN_IP.windowMs, - event - ); + // In development, skip IP rate limiting to avoid self-DoS + if (env.NODE_ENV !== "development") { + const isUnknownIP = clientIP === "unknown"; + const ipIdentifier = isUnknownIP + ? `login:unknown-ip` + : `login:ip:${clientIP}`; + const ipLimit = isUnknownIP + ? { maxAttempts: 3, windowMs: RATE_LIMITS.LOGIN_IP.windowMs } // Stricter for unknown IPs + : RATE_LIMITS.LOGIN_IP; + await checkRateLimit( + ipIdentifier, + ipLimit.maxAttempts, + ipLimit.windowMs, + event + ); + } + + // Always rate limit by email in all environments await checkRateLimit( `login:email:${email}`, RATE_LIMITS.LOGIN_EMAIL.maxAttempts, @@ -372,47 +386,86 @@ export async function rateLimitLogin( /** * Rate limiting middleware for password reset + * In development, skips IP rate limiting to avoid self-DoS + * For unknown IPs in production, uses stricter shared limits */ export async function rateLimitPasswordReset( clientIP: string, event?: H3Event ): Promise { - await checkRateLimit( - `password-reset:ip:${clientIP}`, - RATE_LIMITS.PASSWORD_RESET_IP.maxAttempts, - RATE_LIMITS.PASSWORD_RESET_IP.windowMs, - event - ); + // In development, skip IP rate limiting to avoid self-DoS + if (env.NODE_ENV !== "development") { + const isUnknownIP = clientIP === "unknown"; + const ipIdentifier = isUnknownIP + ? `password-reset:unknown-ip` + : `password-reset:ip:${clientIP}`; + const ipLimit = isUnknownIP + ? { maxAttempts: 2, windowMs: RATE_LIMITS.PASSWORD_RESET_IP.windowMs } // Stricter for unknown IPs + : RATE_LIMITS.PASSWORD_RESET_IP; + + await checkRateLimit( + ipIdentifier, + ipLimit.maxAttempts, + ipLimit.windowMs, + event + ); + } } /** * Rate limiting middleware for registration + * In development, skips IP rate limiting to avoid self-DoS + * For unknown IPs in production, uses stricter shared limits */ export async function rateLimitRegistration( clientIP: string, event?: H3Event ): Promise { - await checkRateLimit( - `registration:ip:${clientIP}`, - RATE_LIMITS.REGISTRATION_IP.maxAttempts, - RATE_LIMITS.REGISTRATION_IP.windowMs, - event - ); + // In development, skip IP rate limiting to avoid self-DoS + if (env.NODE_ENV !== "development") { + const isUnknownIP = clientIP === "unknown"; + const ipIdentifier = isUnknownIP + ? `registration:unknown-ip` + : `registration:ip:${clientIP}`; + const ipLimit = isUnknownIP + ? { maxAttempts: 2, windowMs: RATE_LIMITS.REGISTRATION_IP.windowMs } // Stricter for unknown IPs + : RATE_LIMITS.REGISTRATION_IP; + + await checkRateLimit( + ipIdentifier, + ipLimit.maxAttempts, + ipLimit.windowMs, + event + ); + } } /** * Rate limiting middleware for email verification + * In development, skips IP rate limiting to avoid self-DoS + * For unknown IPs in production, uses stricter shared limits */ export async function rateLimitEmailVerification( clientIP: string, event?: H3Event ): Promise { - await checkRateLimit( - `email-verification:ip:${clientIP}`, - RATE_LIMITS.EMAIL_VERIFICATION_IP.maxAttempts, - RATE_LIMITS.EMAIL_VERIFICATION_IP.windowMs, - event - ); + // In development, skip IP rate limiting to avoid self-DoS + if (env.NODE_ENV !== "development") { + const isUnknownIP = clientIP === "unknown"; + const ipIdentifier = isUnknownIP + ? `email-verification:unknown-ip` + : `email-verification:ip:${clientIP}`; + const ipLimit = isUnknownIP + ? { maxAttempts: 3, windowMs: RATE_LIMITS.EMAIL_VERIFICATION_IP.windowMs } // Stricter for unknown IPs + : RATE_LIMITS.EMAIL_VERIFICATION_IP; + + await checkRateLimit( + ipIdentifier, + ipLimit.maxAttempts, + ipLimit.windowMs, + event + ); + } } export const ACCOUNT_LOCKOUT = CONFIG_ACCOUNT_LOCKOUT; diff --git a/src/server/security/rate-limit.test.ts b/src/server/security/rate-limit.test.ts index 8bc45de..4e28067 100644 --- a/src/server/security/rate-limit.test.ts +++ b/src/server/security/rate-limit.test.ts @@ -416,6 +416,79 @@ describe("Rate Limiting", () => { }); }); + describe("Unknown IP Handling", () => { + it("should handle 'unknown' IP in development without rate limiting", async () => { + // In development, IP rate limits should be skipped + // This test assumes NODE_ENV is 'development' or 'test' + const unknownIP = "unknown"; + const email = `test-${Date.now()}@example.com`; + + // Should allow many login attempts in development with unknown IP + // (only email rate limit applies) + for (let i = 0; i < RATE_LIMITS.LOGIN_EMAIL.maxAttempts; i++) { + const testEmail = `test-${Date.now()}-${i}@example.com`; + await rateLimitLogin(testEmail, unknownIP); + } + + // Should be able to continue with different emails (no IP limit in dev) + await rateLimitLogin(`final-${Date.now()}@example.com`, unknownIP); + }); + + it("should still enforce email rate limits with unknown IP", async () => { + const unknownIP = "unknown"; + const email = `test-${Date.now()}@example.com`; + + // Use up email rate limit + for (let i = 0; i < RATE_LIMITS.LOGIN_EMAIL.maxAttempts; i++) { + await rateLimitLogin(email, unknownIP); + } + + // Next attempt should fail due to email limit + try { + await rateLimitLogin(email, unknownIP); + expect.unreachable("Should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(TRPCError); + } + }); + + it("should handle unknown IP in password reset", async () => { + const unknownIP = "unknown"; + + // In development, should allow many attempts (no IP limit) + for (let i = 0; i < 10; i++) { + await rateLimitPasswordReset(unknownIP); + } + + // Should not throw in development + expect(true).toBe(true); + }); + + it("should handle unknown IP in registration", async () => { + const unknownIP = "unknown"; + + // In development, should allow many attempts (no IP limit) + for (let i = 0; i < 10; i++) { + await rateLimitRegistration(unknownIP); + } + + // Should not throw in development + expect(true).toBe(true); + }); + + it("should handle unknown IP in email verification", async () => { + const unknownIP = "unknown"; + + // In development, should allow many attempts (no IP limit) + for (let i = 0; i < 10; i++) { + await rateLimitEmailVerification(unknownIP); + } + + // Should not throw in development + expect(true).toBe(true); + }); + }); + describe("Rate Limit Configuration", () => { it("should have reasonable limits configured", () => { // Login should be more permissive than registration diff --git a/src/server/session-helpers.ts b/src/server/session-helpers.ts index 5069f08..c9dacca 100644 --- a/src/server/session-helpers.ts +++ b/src/server/session-helpers.ts @@ -185,19 +185,22 @@ export async function getAuthSession( // In SSR contexts where headers may already be sent, use unsealSession directly if (skipUpdate) { const { unsealSession } = await import("vinxi/http"); - const cookieValue = getCookie(event, sessionConfig.cookieName); + const cookieName = sessionConfig.name || "session"; + const cookieValue = getCookie(event, cookieName); if (!cookieValue) { return null; } try { - const data = await unsealSession( - event, - sessionConfig, - cookieValue - ); + // unsealSession returns Partial>, not T directly + const session = await unsealSession(event, sessionConfig, cookieValue); - if (!data || !data.userId || !data.sessionId) { + if (!session?.data || typeof session.data !== "object") { + return null; + } + + const data = session.data as SessionData; + if (!data.userId || !data.sessionId) { return null; }