fix(#411): QA-002 — invert verifySession error classification + health check escalation
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 <noreply@anthropic.com>
This commit is contained in:
@@ -161,6 +161,8 @@ describe("AuthService", () => {
|
|||||||
(service as any).lastHealthCheck = 0;
|
(service as any).lastHealthCheck = 0;
|
||||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||||
(service as any).lastHealthResult = false;
|
(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 () => {
|
it("should return true when discovery URL returns 200", async () => {
|
||||||
@@ -252,6 +254,90 @@ describe("AuthService", () => {
|
|||||||
expect(result2).toBe(false);
|
expect(result2).toBe(false);
|
||||||
expect(mockFetch).toHaveBeenCalledTimes(1);
|
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", () => {
|
describe("getAuthConfig", () => {
|
||||||
@@ -349,14 +435,72 @@ describe("AuthService", () => {
|
|||||||
expect(result).toBeNull();
|
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 auth = service.getAuth();
|
||||||
const mockGetSession = vi.fn().mockRejectedValue(new Error("Verification failed"));
|
const mockGetSession = vi.fn().mockRejectedValue(new Error("Verification failed"));
|
||||||
auth.api = { getSession: mockGetSession } as any;
|
auth.api = { getSession: mockGetSession } as any;
|
||||||
|
|
||||||
const result = await service.verifySession("error-token");
|
await expect(service.verifySession("error-token")).rejects.toThrow("Verification failed");
|
||||||
|
|
||||||
expect(result).toBeNull();
|
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should re-throw Prisma infrastructure errors", async () => {
|
it("should re-throw Prisma infrastructure errors", async () => {
|
||||||
|
|||||||
@@ -12,6 +12,15 @@ const OIDC_HEALTH_CACHE_TTL_MS = 30_000;
|
|||||||
/** Timeout in milliseconds for the OIDC discovery URL fetch */
|
/** Timeout in milliseconds for the OIDC discovery URL fetch */
|
||||||
const OIDC_HEALTH_TIMEOUT_MS = 2_000;
|
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<string, unknown>;
|
||||||
|
session: Record<string, unknown>;
|
||||||
|
}
|
||||||
|
|
||||||
@Injectable()
|
@Injectable()
|
||||||
export class AuthService {
|
export class AuthService {
|
||||||
private readonly logger = new Logger(AuthService.name);
|
private readonly logger = new Logger(AuthService.name);
|
||||||
@@ -22,11 +31,16 @@ export class AuthService {
|
|||||||
private lastHealthCheck = 0;
|
private lastHealthCheck = 0;
|
||||||
/** Cached result of the last OIDC health check */
|
/** Cached result of the last OIDC health check */
|
||||||
private lastHealthResult = false;
|
private lastHealthResult = false;
|
||||||
|
/** Consecutive OIDC health check failure count for log-level escalation */
|
||||||
|
private consecutiveHealthFailures = 0;
|
||||||
|
|
||||||
constructor(private readonly prisma: PrismaService) {
|
constructor(private readonly prisma: PrismaService) {
|
||||||
// PrismaService extends PrismaClient and is compatible with BetterAuth's adapter
|
// PrismaService extends PrismaClient and is compatible with BetterAuth's adapter
|
||||||
// Cast is safe as PrismaService provides all required PrismaClient methods
|
// 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);
|
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);
|
this.nodeHandler = toNodeHandler(this.auth);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -87,12 +101,13 @@ export class AuthService {
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Verify session token
|
* 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(
|
async verifySession(token: string): Promise<VerifiedSession | null> {
|
||||||
token: string
|
|
||||||
): Promise<{ user: Record<string, unknown>; session: Record<string, unknown> } | null> {
|
|
||||||
try {
|
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({
|
const session = await this.auth.api.getSession({
|
||||||
headers: {
|
headers: {
|
||||||
authorization: `Bearer ${token}`,
|
authorization: `Bearer ${token}`,
|
||||||
@@ -104,31 +119,32 @@ export class AuthService {
|
|||||||
}
|
}
|
||||||
|
|
||||||
return {
|
return {
|
||||||
|
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
|
||||||
user: session.user as Record<string, unknown>,
|
user: session.user as Record<string, unknown>,
|
||||||
|
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
|
||||||
session: session.session as Record<string, unknown>,
|
session: session.session as Record<string, unknown>,
|
||||||
};
|
};
|
||||||
} catch (error) {
|
} catch (error: unknown) {
|
||||||
// Infrastructure errors (database down, connection failures) should propagate
|
// Only known-safe auth errors return null
|
||||||
// so the global exception filter returns 500/503, not 401
|
if (error instanceof Error) {
|
||||||
if (
|
const msg = error.message.toLowerCase();
|
||||||
error instanceof Error &&
|
const isExpectedAuthError =
|
||||||
(error.constructor.name.startsWith("Prisma") ||
|
msg.includes("invalid token") ||
|
||||||
error.message.includes("connect") ||
|
msg.includes("expired") ||
|
||||||
error.message.includes("ECONNREFUSED") ||
|
msg.includes("session not found") ||
|
||||||
error.message.includes("timeout"))
|
msg.includes("unauthorized") ||
|
||||||
) {
|
msg.includes("invalid session");
|
||||||
this.logger.error(
|
|
||||||
"Session verification failed due to infrastructure error",
|
|
||||||
error.stack,
|
|
||||||
);
|
|
||||||
throw error;
|
|
||||||
}
|
|
||||||
|
|
||||||
// Expected auth errors (invalid/expired token) return null
|
if (!isExpectedAuthError) {
|
||||||
this.logger.warn(
|
// Infrastructure or unexpected — propagate as 500
|
||||||
"Session verification failed",
|
this.logger.error(
|
||||||
error instanceof Error ? error.message : "Unknown error",
|
"Session verification failed due to unexpected error",
|
||||||
);
|
error.stack ?? error.message
|
||||||
|
);
|
||||||
|
throw error;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
// Non-Error thrown values or expected auth errors
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -159,8 +175,18 @@ export class AuthService {
|
|||||||
this.lastHealthCheck = Date.now();
|
this.lastHealthCheck = Date.now();
|
||||||
this.lastHealthResult = response.ok;
|
this.lastHealthResult = response.ok;
|
||||||
|
|
||||||
if (!response.ok) {
|
if (response.ok) {
|
||||||
this.logger.warn(
|
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}`
|
`OIDC provider returned non-OK status: ${String(response.status)} from ${discoveryUrl}`
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
@@ -169,9 +195,12 @@ export class AuthService {
|
|||||||
} catch (error: unknown) {
|
} catch (error: unknown) {
|
||||||
this.lastHealthCheck = Date.now();
|
this.lastHealthCheck = Date.now();
|
||||||
this.lastHealthResult = false;
|
this.lastHealthResult = false;
|
||||||
|
this.consecutiveHealthFailures++;
|
||||||
|
|
||||||
const message = error instanceof Error ? error.message : String(error);
|
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;
|
return false;
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user