fix(auth-guard): keep anonymous routes working when getSession fails (e.g. DB down) - #160
Open
ropdias wants to merge 2 commits into
Open
fix(auth-guard): keep anonymous routes working when getSession fails (e.g. DB down)#160ropdias wants to merge 2 commits into
ropdias wants to merge 2 commits into
Conversation
getSession() was called before the route metadata was evaluated, so an infrastructure failure (e.g. an unreachable database) rejected and bubbled up as a 500 on every route — including @AllowAnonymous() liveness/readiness probes whose whole purpose is to keep responding while the database is down. Wrap the getSession() call in a try/catch and evaluate the metadata afterwards: on failure fall back to a null session and log it (mirroring the try/catch + console.error pattern already used by the role and permission checks in this guard). @AllowAnonymous() and @OptionalAuth() routes then proceed, while routes that require a session re-throw the original error instead of masking a DB outage as a 401. Closes ThallesP#159 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
commit: |
… route The db-failure regression test asserted only authenticated: false, which covers req.user. Also assert session: null so the test fully pins the anonymous state the fix guarantees — otherwise a stray req.session leaking through would still pass. Mirrors the authenticated-with-auth test, which already asserts the session shape. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #159.
Problem
AuthGuard.canActivatecallsgetSession()before it reads the route metadata (@AllowAnonymous()/@OptionalAuth()). Because Better Auth'sgetSession()goes through the same database adapter, an infrastructure failure (e.g. an unreachable database) makes it reject. With notry/catcharound it, that rejection bubbles up and NestJS turns it into a generic500on every route — including@AllowAnonymous()liveness/readiness probes, whose entire job is to keep responding while the database is down. TheisPublicshort-circuit is never reached.Approach
Wrap the
getSession()call in atry/catchand evaluate the route metadata afterwards:nullsession and log it — mirroring thetry/catch+console.errorpattern already used by the role/permission checks in this same guard.@AllowAnonymous()routes returntrueregardless of the failure. The session is still hydrated when the DB is up, so@Session()on public routes keeps working.@OptionalAuth()routes proceed as anonymous.5xxinstead of a misleading401(which would push clients into a logout / redirect-to-login loop they can't escape while the DB is down). Protected-route behavior is otherwise unchanged.No public API changes — the only observable difference is that anonymous/optional routes no longer
500when the session lookup fails. It's apatch.Why not the alternatives raised in the issue thread
isPublicfirst and skippinggetSession()entirely would break session hydration on public/optional routes: an authenticated user hitting a public route would losereq.user/req.session(the@Session()/ optional-auth pattern).injectSessionInPublicRoutesoption adds configuration surface for a false dichotomy — thetry/catchalready gives both behaviors (session hydrated when the DB is up, route still served when it's down) without a new flag to document, test and misconfigure.Tests
Added
tests/e2e/auth-guard-db-failure.e2e.test.ts(runs on both Express and Fastify), simulating a rejectinggetSession():@AllowAnonymous()→200@OptionalAuth()→200(anonymous)500(error surfaced, not masked)@ThallesP would appreciate your review on this one 🙏
Checklist
bun run checkpasses (lint + format)bun run testpasses (Express + Fastify)