From 4f31690281142f5ddccc9a778e40ab62281fc28b Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Mon, 16 Feb 2026 13:15:41 -0600 Subject: [PATCH] =?UTF-8?q?fix(#411):=20QA-002=20=E2=80=94=20invert=20veri?= =?UTF-8?q?fySession=20error=20classification=20+=20health=20check=20escal?= =?UTF-8?q?ation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit verifySession now allowlists known auth errors (return null) and re-throws everything else as infrastructure errors. OIDC health check escalates to error level after 3 consecutive failures. Co-Authored-By: Claude Opus 4.6 --- apps/api/src/auth/auth.service.spec.ts | 152 ++++++++++++++++++++++++- apps/api/src/auth/auth.service.ts | 85 +++++++++----- 2 files changed, 205 insertions(+), 32 deletions(-) diff --git a/apps/api/src/auth/auth.service.spec.ts b/apps/api/src/auth/auth.service.spec.ts index 4811b33..24e6d3d 100644 --- a/apps/api/src/auth/auth.service.spec.ts +++ b/apps/api/src/auth/auth.service.spec.ts @@ -161,6 +161,8 @@ describe("AuthService", () => { (service as any).lastHealthCheck = 0; // eslint-disable-next-line @typescript-eslint/no-explicit-any (service as any).lastHealthResult = false; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (service as any).consecutiveHealthFailures = 0; }); it("should return true when discovery URL returns 200", async () => { @@ -252,6 +254,90 @@ describe("AuthService", () => { expect(result2).toBe(false); expect(mockFetch).toHaveBeenCalledTimes(1); }); + + it("should escalate to error level after 3 consecutive failures", async () => { + const mockFetch = vi.fn().mockRejectedValue(new Error("ECONNREFUSED")); + vi.stubGlobal("fetch", mockFetch); + + const loggerWarn = vi.spyOn(service["logger"], "warn"); + const loggerError = vi.spyOn(service["logger"], "error"); + + // Failures 1 and 2 should log at warn level + await service.isOidcProviderReachable(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (service as any).lastHealthCheck = 0; // Reset cache + await service.isOidcProviderReachable(); + + expect(loggerWarn).toHaveBeenCalledTimes(2); + expect(loggerError).not.toHaveBeenCalled(); + + // Failure 3 should escalate to error level + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (service as any).lastHealthCheck = 0; + await service.isOidcProviderReachable(); + + expect(loggerError).toHaveBeenCalledTimes(1); + expect(loggerError).toHaveBeenCalledWith( + expect.stringContaining("OIDC provider unreachable"), + ); + }); + + it("should escalate to error level after 3 consecutive non-OK responses", async () => { + const mockFetch = vi.fn().mockResolvedValue({ ok: false, status: 503 }); + vi.stubGlobal("fetch", mockFetch); + + const loggerWarn = vi.spyOn(service["logger"], "warn"); + const loggerError = vi.spyOn(service["logger"], "error"); + + // Failures 1 and 2 at warn level + await service.isOidcProviderReachable(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (service as any).lastHealthCheck = 0; + await service.isOidcProviderReachable(); + + expect(loggerWarn).toHaveBeenCalledTimes(2); + expect(loggerError).not.toHaveBeenCalled(); + + // Failure 3 at error level + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (service as any).lastHealthCheck = 0; + await service.isOidcProviderReachable(); + + expect(loggerError).toHaveBeenCalledTimes(1); + expect(loggerError).toHaveBeenCalledWith( + expect.stringContaining("OIDC provider returned non-OK status"), + ); + }); + + it("should reset failure counter and log recovery on success after failures", async () => { + const mockFetch = vi + .fn() + .mockRejectedValueOnce(new Error("ECONNREFUSED")) + .mockRejectedValueOnce(new Error("ECONNREFUSED")) + .mockResolvedValueOnce({ ok: true, status: 200 }); + vi.stubGlobal("fetch", mockFetch); + + const loggerLog = vi.spyOn(service["logger"], "log"); + + // Two failures + await service.isOidcProviderReachable(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (service as any).lastHealthCheck = 0; + await service.isOidcProviderReachable(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (service as any).lastHealthCheck = 0; + + // Recovery + const result = await service.isOidcProviderReachable(); + + expect(result).toBe(true); + expect(loggerLog).toHaveBeenCalledWith( + expect.stringContaining("OIDC provider recovered after 2 consecutive failure(s)"), + ); + // Verify counter reset + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((service as any).consecutiveHealthFailures).toBe(0); + }); }); describe("getAuthConfig", () => { @@ -349,14 +435,72 @@ describe("AuthService", () => { expect(result).toBeNull(); }); - it("should return null and log warning on auth verification failure", async () => { + it("should return null for 'invalid token' auth error", async () => { + const auth = service.getAuth(); + const mockGetSession = vi.fn().mockRejectedValue(new Error("Invalid token provided")); + auth.api = { getSession: mockGetSession } as any; + + const result = await service.verifySession("bad-token"); + + expect(result).toBeNull(); + }); + + it("should return null for 'expired' auth error", async () => { + const auth = service.getAuth(); + const mockGetSession = vi.fn().mockRejectedValue(new Error("Token expired")); + auth.api = { getSession: mockGetSession } as any; + + const result = await service.verifySession("expired-token"); + + expect(result).toBeNull(); + }); + + it("should return null for 'session not found' auth error", async () => { + const auth = service.getAuth(); + const mockGetSession = vi.fn().mockRejectedValue(new Error("Session not found")); + auth.api = { getSession: mockGetSession } as any; + + const result = await service.verifySession("missing-session"); + + expect(result).toBeNull(); + }); + + it("should return null for 'unauthorized' auth error", async () => { + const auth = service.getAuth(); + const mockGetSession = vi.fn().mockRejectedValue(new Error("Unauthorized")); + auth.api = { getSession: mockGetSession } as any; + + const result = await service.verifySession("unauth-token"); + + expect(result).toBeNull(); + }); + + it("should return null for 'invalid session' auth error", async () => { + const auth = service.getAuth(); + const mockGetSession = vi.fn().mockRejectedValue(new Error("Invalid session")); + auth.api = { getSession: mockGetSession } as any; + + const result = await service.verifySession("invalid-session"); + + expect(result).toBeNull(); + }); + + it("should return null when a non-Error value is thrown", async () => { + const auth = service.getAuth(); + const mockGetSession = vi.fn().mockRejectedValue("string-error"); + auth.api = { getSession: mockGetSession } as any; + + const result = await service.verifySession("any-token"); + + expect(result).toBeNull(); + }); + + it("should re-throw unexpected errors that are not known auth errors", async () => { const auth = service.getAuth(); const mockGetSession = vi.fn().mockRejectedValue(new Error("Verification failed")); auth.api = { getSession: mockGetSession } as any; - const result = await service.verifySession("error-token"); - - expect(result).toBeNull(); + await expect(service.verifySession("error-token")).rejects.toThrow("Verification failed"); }); it("should re-throw Prisma infrastructure errors", async () => { diff --git a/apps/api/src/auth/auth.service.ts b/apps/api/src/auth/auth.service.ts index 4bfd489..e5d521f 100644 --- a/apps/api/src/auth/auth.service.ts +++ b/apps/api/src/auth/auth.service.ts @@ -12,6 +12,15 @@ const OIDC_HEALTH_CACHE_TTL_MS = 30_000; /** Timeout in milliseconds for the OIDC discovery URL fetch */ const OIDC_HEALTH_TIMEOUT_MS = 2_000; +/** Number of consecutive health-check failures before escalating to error level */ +const HEALTH_ESCALATION_THRESHOLD = 3; + +/** Verified session shape returned by BetterAuth's getSession */ +interface VerifiedSession { + user: Record; + session: Record; +} + @Injectable() export class AuthService { private readonly logger = new Logger(AuthService.name); @@ -22,11 +31,16 @@ export class AuthService { private lastHealthCheck = 0; /** Cached result of the last OIDC health check */ private lastHealthResult = false; + /** Consecutive OIDC health check failure count for log-level escalation */ + private consecutiveHealthFailures = 0; constructor(private readonly prisma: PrismaService) { // PrismaService extends PrismaClient and is compatible with BetterAuth's adapter // Cast is safe as PrismaService provides all required PrismaClient methods + // TODO(#411): BetterAuth returns opaque types — replace when upstream exports typed interfaces + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment this.auth = createAuth(this.prisma as unknown as PrismaClient); + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-call this.nodeHandler = toNodeHandler(this.auth); } @@ -87,12 +101,13 @@ export class AuthService { /** * Verify session token - * Returns session data if valid, null if invalid or expired + * Returns session data if valid, null if invalid or expired. + * Only known-safe auth errors return null; everything else propagates as 500. */ - async verifySession( - token: string - ): Promise<{ user: Record; session: Record } | null> { + async verifySession(token: string): Promise { try { + // TODO(#411): BetterAuth getSession returns opaque types — replace when upstream exports typed interfaces + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access const session = await this.auth.api.getSession({ headers: { authorization: `Bearer ${token}`, @@ -104,31 +119,32 @@ export class AuthService { } return { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access user: session.user as Record, + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access session: session.session as Record, }; - } catch (error) { - // Infrastructure errors (database down, connection failures) should propagate - // so the global exception filter returns 500/503, not 401 - if ( - error instanceof Error && - (error.constructor.name.startsWith("Prisma") || - error.message.includes("connect") || - error.message.includes("ECONNREFUSED") || - error.message.includes("timeout")) - ) { - this.logger.error( - "Session verification failed due to infrastructure error", - error.stack, - ); - throw error; - } + } catch (error: unknown) { + // Only known-safe auth errors return null + if (error instanceof Error) { + const msg = error.message.toLowerCase(); + const isExpectedAuthError = + msg.includes("invalid token") || + msg.includes("expired") || + msg.includes("session not found") || + msg.includes("unauthorized") || + msg.includes("invalid session"); - // Expected auth errors (invalid/expired token) return null - this.logger.warn( - "Session verification failed", - error instanceof Error ? error.message : "Unknown error", - ); + if (!isExpectedAuthError) { + // Infrastructure or unexpected — propagate as 500 + this.logger.error( + "Session verification failed due to unexpected error", + error.stack ?? error.message + ); + throw error; + } + } + // Non-Error thrown values or expected auth errors return null; } } @@ -159,8 +175,18 @@ export class AuthService { this.lastHealthCheck = Date.now(); this.lastHealthResult = response.ok; - if (!response.ok) { - this.logger.warn( + if (response.ok) { + if (this.consecutiveHealthFailures > 0) { + this.logger.log( + `OIDC provider recovered after ${String(this.consecutiveHealthFailures)} consecutive failure(s)` + ); + } + this.consecutiveHealthFailures = 0; + } else { + this.consecutiveHealthFailures++; + const logLevel = + this.consecutiveHealthFailures >= HEALTH_ESCALATION_THRESHOLD ? "error" : "warn"; + this.logger[logLevel]( `OIDC provider returned non-OK status: ${String(response.status)} from ${discoveryUrl}` ); } @@ -169,9 +195,12 @@ export class AuthService { } catch (error: unknown) { this.lastHealthCheck = Date.now(); this.lastHealthResult = false; + this.consecutiveHealthFailures++; const message = error instanceof Error ? error.message : String(error); - this.logger.warn(`OIDC provider unreachable at ${discoveryUrl}: ${message}`); + const logLevel = + this.consecutiveHealthFailures >= HEALTH_ESCALATION_THRESHOLD ? "error" : "warn"; + this.logger[logLevel](`OIDC provider unreachable at ${discoveryUrl}: ${message}`); return false; }