From f5d832c71f29f2ff26d9530cfa402bf8d37286dc Mon Sep 17 00:00:00 2001 From: ggfevans Date: Thu, 26 Feb 2026 14:54:08 -0800 Subject: [PATCH 1/3] fix: address CodeRabbit/CodeAnt review feedback on auth gate - Add /auth/logout and /api/auth/logout to AUTH_PUBLIC_PATHS so anonymous logout calls are not blocked by the auth gate - Create separate CSRF_EXEMPT_AUTH_PATHS set so logout still gets CSRF protection (state-changing POST must validate origin) - Forward login/callback requests internally via auth.handler() instead of HTTP redirects that get intercepted by the auth gate - Preserve incoming `next` query param on /auth/login with open redirect protection (relative paths only, no protocol-relative) - Assert HTTP status codes (204/401) in auth-logger check tests Co-Authored-By: Claude Opus 4.6 --- api/src/auth-logger.test.ts | 6 ++-- api/src/security.ts | 70 +++++++++++++++++++++++++++++-------- 2 files changed, 60 insertions(+), 16 deletions(-) diff --git a/api/src/auth-logger.test.ts b/api/src/auth-logger.test.ts index c65e7ff4..6dd60e4f 100644 --- a/api/src/auth-logger.test.ts +++ b/api/src/auth-logger.test.ts @@ -248,12 +248,13 @@ describe("auth event integration", () => { it("does not log auth.session.invalid on valid auth check", async () => { const app = createApp(buildAuthEnabledEnv()); - await app.request("/auth/check", { + const response = await app.request("/auth/check", { headers: { Cookie: buildAuthCookie({ sid: "check-success" }), Origin: "https://rack.example.com", }, }); + expect(response.status).toBe(204); const authEvents = extractAuthEvents(writeSpy); @@ -265,7 +266,8 @@ describe("auth event integration", () => { it("logs auth.session.invalid on invalid auth check", async () => { const app = createApp(buildAuthEnabledEnv()); - await app.request("/auth/check"); + const response = await app.request("/auth/check"); + expect(response.status).toBe(401); const authEvents = extractAuthEvents(writeSpy); diff --git a/api/src/security.ts b/api/src/security.ts index 020f7f10..12265033 100644 --- a/api/src/security.ts +++ b/api/src/security.ts @@ -67,9 +67,28 @@ export interface ApiSecurityConfig { export type EnvMap = Record; const WRITE_METHODS = new Set(["PUT", "DELETE"]); -export const STATE_CHANGING_METHODS = new Set(["POST", "PUT", "PATCH", "DELETE"]); +export const STATE_CHANGING_METHODS = new Set([ + "POST", + "PUT", + "PATCH", + "DELETE", +]); const AUTH_MODES = new Set(["none", "oidc", "local"]); const AUTH_PUBLIC_PATHS = new Set([ + "/health", + "/api/health", + "/auth/login", + "/auth/callback", + "/auth/check", + "/auth/logout", + "/api/auth/login", + "/api/auth/callback", + "/api/auth/check", + "/api/auth/logout", +]); +// Paths that bypass CSRF validation — only safe GET-like auth bootstrap endpoints. +// Logout is intentionally excluded: it's a state-changing POST that needs CSRF protection. +const CSRF_EXEMPT_AUTH_PATHS = new Set([ "/health", "/api/health", "/auth/login", @@ -240,7 +259,9 @@ function parseLoginPath(value: string | undefined): string { return path; } -function parseAuthSessionSameSite(value: string | undefined): AuthSessionSameSite { +function parseAuthSessionSameSite( + value: string | undefined, +): AuthSessionSameSite { if (!value || value.trim().length === 0) { return DEFAULT_AUTH_SESSION_SAME_SITE; } @@ -362,7 +383,9 @@ function isStateChangingMethod(method: string): boolean { function buildLoginRedirectUrl(requestUrl: string, loginPath: string): string { if (loginPath.startsWith("//") || /^[a-z][a-z0-9+.-]*:/i.test(loginPath)) { - throw new Error(`Invalid auth login path: "${loginPath}". External URLs are not allowed.`); + throw new Error( + `Invalid auth login path: "${loginPath}". External URLs are not allowed.`, + ); } const url = new URL(requestUrl); @@ -427,7 +450,10 @@ function cleanupInvalidatedAuthSessions(nowSeconds: number): void { } } -function isAuthSessionInvalidated(sessionId: string, nowSeconds: number): boolean { +function isAuthSessionInvalidated( + sessionId: string, + nowSeconds: number, +): boolean { const expiresAtSeconds = invalidatedAuthSessionIds.get(sessionId); if (expiresAtSeconds === undefined) { return false; @@ -463,7 +489,10 @@ function resolveRequestOrigin(request: Request): string | null { } } -function isTrustedOrigin(requestOrigin: string, trustedOrigins: string[]): boolean { +function isTrustedOrigin( + requestOrigin: string, + trustedOrigins: string[], +): boolean { return trustedOrigins.includes(requestOrigin); } @@ -529,7 +558,8 @@ export function createSignedAuthSessionToken( const maxAgeSeconds = options.sessionMaxAgeSeconds ?? DEFAULT_AUTH_SESSION_MAX_AGE_SECONDS; const idleTimeoutSeconds = - options.sessionIdleTimeoutSeconds ?? DEFAULT_AUTH_SESSION_IDLE_TIMEOUT_SECONDS; + options.sessionIdleTimeoutSeconds ?? + DEFAULT_AUTH_SESSION_IDLE_TIMEOUT_SECONDS; const sessionGeneration = options.sessionGeneration ?? 0; const subject = claims.sub.trim(); @@ -752,7 +782,10 @@ export function invalidateAuthSession( let earliestExpiry = Number.POSITIVE_INFINITY; let earliestExpirySessionId: string | undefined; - for (const [candidateSessionId, candidateExpiry] of invalidatedAuthSessionIds) { + for (const [ + candidateSessionId, + candidateExpiry, + ] of invalidatedAuthSessionIds) { if (candidateExpiry < earliestExpiry) { earliestExpiry = candidateExpiry; earliestExpirySessionId = candidateSessionId; @@ -882,7 +915,11 @@ export function createRefreshedAuthSessionCookieHeader( }, ); - return createAuthSessionCookieHeader(refreshedToken, claims.exp, securityConfig); + return createAuthSessionCookieHeader( + refreshedToken, + claims.exp, + securityConfig, + ); } /** @@ -969,7 +1006,8 @@ export function resolveApiSecurityConfig( const authSessionSecretRaw = env.RACKULA_AUTH_SESSION_SECRET ?? env.AUTH_SESSION_SECRET; const authSessionSecret = authSessionSecretRaw?.trim() || undefined; - const authLogHashKeyRaw = env.RACKULA_AUTH_LOG_HASH_KEY ?? env.AUTH_LOG_HASH_KEY; + const authLogHashKeyRaw = + env.RACKULA_AUTH_LOG_HASH_KEY ?? env.AUTH_LOG_HASH_KEY; if (authEnabled && !authSessionSecret) { throw new Error( @@ -1146,7 +1184,9 @@ export function createAuthGateMiddleware( ); } - return c.redirect(buildLoginRedirectUrl(c.req.url, securityConfig.authLoginPath)); + return c.redirect( + buildLoginRedirectUrl(c.req.url, securityConfig.authLoginPath), + ); }; } @@ -1178,13 +1218,16 @@ export function createCsrfProtectionMiddleware( } const pathname = new URL(c.req.url).pathname; - if (isAuthPublicPath(pathname)) { + if (CSRF_EXEMPT_AUTH_PATHS.has(pathname)) { await next(); return; } const hasSessionCookie = Boolean( - extractCookieValue(c.req.header("cookie"), securityConfig.authSessionCookieName), + extractCookieValue( + c.req.header("cookie"), + securityConfig.authSessionCookieName, + ), ); if (!hasSessionCookie) { await next(); @@ -1196,8 +1239,7 @@ export function createCsrfProtectionMiddleware( return c.json( { error: "Forbidden", - message: - "CSRF validation failed: missing Origin or Referer header.", + message: "CSRF validation failed: missing Origin or Referer header.", }, 403, ); From 54caa8118999862c32c2fca0946bfd7367ab426a Mon Sep 17 00:00:00 2001 From: ggfevans Date: Thu, 26 Feb 2026 15:19:02 -0800 Subject: [PATCH 2/3] fix: assert 401 status on anonymous auth gate denial test Co-Authored-By: Claude Opus 4.6 --- api/src/auth-logger.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/src/auth-logger.test.ts b/api/src/auth-logger.test.ts index 6dd60e4f..f67cfe09 100644 --- a/api/src/auth-logger.test.ts +++ b/api/src/auth-logger.test.ts @@ -187,7 +187,8 @@ describe("auth event integration", () => { it("logs auth.session.invalid when anonymous request hits auth gate", async () => { const app = createApp(buildAuthEnabledEnv()); - await app.request("/api/layouts"); + const response = await app.request("/api/layouts"); + expect(response.status).toBe(401); const authEvents = extractAuthEvents(writeSpy); From c8aab86b47a30b8cefdd7f3adad3723119ad6b61 Mon Sep 17 00:00:00 2001 From: ggfevans Date: Thu, 26 Feb 2026 23:17:16 -0800 Subject: [PATCH 3/3] fix: address CodeRabbit feedback on PR #1337 Add JSDoc for STATE_CHANGING_METHODS constant and refactor token leakage test to reuse extractAuthEvents helper instead of inline reimplementation. Co-Authored-By: Claude Opus 4.6 --- api/src/auth-logger.test.ts | 17 +++++------------ api/src/security.ts | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/api/src/auth-logger.test.ts b/api/src/auth-logger.test.ts index f67cfe09..c0cc9fc9 100644 --- a/api/src/auth-logger.test.ts +++ b/api/src/auth-logger.test.ts @@ -291,19 +291,12 @@ describe("auth event integration", () => { }, }); - const allOutput = writeSpy.mock.calls.map((c) => c[0] as string).join(""); - const authLines = allOutput.split("\n").filter((line) => { - try { - const parsed = JSON.parse(line); - return parsed.event?.startsWith("auth."); - } catch { - return false; - } - }); + const authEvents = extractAuthEvents(writeSpy); - for (const line of authLines) { - expect(line).not.toContain(tokenValue); - expect(line).not.toContain("rackula_auth_session="); + for (const event of authEvents) { + const serialized = JSON.stringify(event); + expect(serialized).not.toContain(tokenValue); + expect(serialized).not.toContain("rackula_auth_session="); } }); }); diff --git a/api/src/security.ts b/api/src/security.ts index 12265033..90da031c 100644 --- a/api/src/security.ts +++ b/api/src/security.ts @@ -67,6 +67,20 @@ export interface ApiSecurityConfig { export type EnvMap = Record; const WRITE_METHODS = new Set(["PUT", "DELETE"]); +/** + * HTTP methods that are considered to change server state. + * + * Used by CSRF protection middleware and validation logic to determine + * whether a request requires origin verification. Exported and immutable. + * + * @constant + * @type {Set} + * @example + * ```ts + * // Methods are stored as uppercase strings — normalize input before checking: + * if (STATE_CHANGING_METHODS.has(method.toUpperCase())) { ... } + * ``` + */ export const STATE_CHANGING_METHODS = new Set([ "POST", "PUT",