feat(api): harden auth surface (#68) - #109
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a global fail-closed ChangesAuth Surface Hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ThrottlerGuard
participant SessionAuthGuard
participant Reflector
participant AuthService
participant SessionsRepository
Client->>ThrottlerGuard: HTTP request
ThrottlerGuard->>SessionAuthGuard: within rate limit
SessionAuthGuard->>Reflector: getAllAndOverride(IS_PUBLIC_KEY)
alt public route
SessionAuthGuard-->>Client: allow request
else protected route
SessionAuthGuard->>SessionAuthGuard: extract bearer token or cookie
SessionAuthGuard->>AuthService: validateToken(token)
AuthService->>SessionsRepository: findActiveAndTouch(tokenHash, idleTtl, debounce)
SessionsRepository-->>AuthService: session or undefined
alt session missing userId
AuthService->>SessionsRepository: deleteStaleByTokenHash(tokenHash)
AuthService-->>SessionAuthGuard: undefined
SessionAuthGuard-->>Client: 401 Unauthorized
else valid session
AuthService-->>SessionAuthGuard: userId, sessionId
SessionAuthGuard-->>Client: allow request
end
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements robust authentication hardening, including a fail-closed global session guard, rate limiting on credential endpoints, atomic session validation with a read-only debounce, and an automated hourly session cleanup cron job. Feedback on these changes highlights a bug in getTrustProxySetting where string representations of booleans are not correctly cast for Express's proxy configuration, a potential database performance issue where the session cleanup query will trigger full table scans due to missing indexes on expires and lastSeenAt, and a recommendation to explicitly catch and log errors in the background cleanup service.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 26 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
8a1a6e8 to
675ecca
Compare
5c9967f to
20be567
Compare
d386ae7 to
97d1bce
Compare
…, TRUST_PROXY (#68) First tranche of the #60 auth-surface hardening list: - Fail-closed by default: SessionAuthGuard registers as a global APP_GUARD; @public() is the explicit, reviewable opt-out (login, register, the liveness root). Per-route @UseGuards were REMOVED so the global guard is load-bearing — every existing 401 e2e assertion now proves the default-deny path, not a per-controller annotation. This kills the bug class behind the original /users exposure. - Atomic session validation: findActiveAndTouch validates and stamps last_seen_at in one UPDATE … RETURNING (no SELECT→check→UPDATE TOCTOU), with a read-only fast path inside a 60s debounce window that takes the per-request write off the hot path entirely. Stale rows are deleted as housekeeping on the miss path. - Session listing returns active (unexpired) sessions only, backed by a new (user_id, expires) index (migration 0015); the current-session lookup is a single WHERE user_id AND id query instead of list-then-find. - TRUST_PROXY (off by default — trusting proxy headers with no proxy lets clients spoof their IP): hop count or Express subnet spec, so session.ip records the real client behind a reverse proxy (SPEC §22.0 prerequisite for IP/subnet policies). Still open in #68: login/register rate limiting (@nestjs/throttler), the cross-site SameSite=None + CSRF posture, token-free responses for cookie clients, session rotation (vacuous until a change-password endpoint exists), and periodic expired-session cleanup. Refs #68 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
… cron (#68) - Rate limiting (@nestjs/throttler): instance-wide 300/min default with strict 10/min per-IP overrides on POST /auth/v1/login and /register — each attempt burns a bcrypt compare, an unbounded brute-force + DoS amplification surface. Guard order is deliberate: ThrottlerGuard registers BEFORE SessionAuthGuard, so a flood is rejected without paying the session lookup. Uses req.ip, so the TRUST_PROXY setting from tranche 1 feeds directly into per-client fairness. Proven by an e2e that rapid-fires logins into a 429 (placed last in the suite — it spends the instance's login budget). - Expired-session housekeeping on a pg-boss cron: SessionCleanupService schedules 'sessions.cleanup' hourly (off the :00 mark) and purges expired/idle rows — the #47 scheduler's first production consumer. Cross-user by design (sessions carry no RLS; they are consulted pre-authentication and expiry is a global fact, not tenant data); idempotent across concurrent instances. The purge SQL is proven against real Postgres in the integration suite: expired rows go, live rows stay. Remaining in #68: cross-site SameSite=None + CSRF posture, token-free responses for cookie clients, and session rotation (vacuous until a change-password endpoint exists). Refs #68 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BMxH1qw3rCdjXAywDrJU9
20be567 to
9f39e2b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/auth/auth.service.ts (1)
96-122: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winAvoid the extra DELETE on failed auth checks.
findActiveAndTouch()already does the lookup/update work; when it returnsundefined,deleteStaleByTokenHash()adds another blocking DB round-trip on every invalid token, including tokens that never matched a session row. Since this is best-effort housekeeping, move it off the request path or remove it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/auth/auth.service.ts` around lines 96 - 122, The failed-auth path in validateToken is doing an extra blocking delete after findActiveAndTouch returns no usable session, which adds unnecessary DB work for every invalid token. Remove the inline deleteStaleByTokenHash call from AuthService.validateToken, or move that housekeeping to a background/periodic cleanup path so the request path only performs the existing findActiveAndTouch lookup and returns undefined on failure.
🧹 Nitpick comments (3)
apps/api/src/app.module.ts (1)
36-38: 🩺 Stability & Availability | 🔵 TrivialUse shared throttler storage before scaling replicas.
ThrottlerModule.forRoot()falls back to per-process in-memory counters, so the limit is only enforced per instance. If this API can run with multiple pods/replicas, back it with a shared store (for example Redis) so the login/register ceiling stays global.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/app.module.ts` around lines 36 - 38, The throttling setup in ThrottlerModule.forRoot currently uses the default in-memory counters, so limits are enforced per process rather than across replicas. Update the AppModule throttler configuration to use a shared storage backend such as Redis, wiring it into the throttler options so the default login/register rate limit is global across all instances.apps/api/src/auth/session-cleanup.service.ts (1)
26-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider guarding bootstrap wiring and adding shutdown handling.
onApplicationBootstraphas no error handling aroundensureQueue/schedule/consume, so a transient queue-backend hiccup at startup could crash the whole API rather than just disabling cleanup. Also, there's noOnModuleDestroyto stop consuming on shutdown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/auth/session-cleanup.service.ts` around lines 26 - 49, Wrap the startup wiring in SessionCleanupService.onApplicationBootstrap with error handling so failures in queue.ensureQueue, queue.schedule, or queue.consume don’t take down the API if the queue backend is temporarily unavailable. Add a fallback path that logs the problem and skips session cleanup setup rather than crashing, and introduce OnModuleDestroy on SessionCleanupService to stop the SESSIONS_CLEANUP_QUEUE consumer during shutdown. Use the existing queue and logger fields, and keep the cleanup logic in cleanup() unchanged.apps/api/src/auth/sessions.repository.ts (1)
167-186: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider an index to support
deleteExpired's cross-user scan.The new
sessions_user_expires_idxis composite on(userId, expires)and is intended forlistForUser(per its inline comment); it won't be efficiently usable by this query sincedeleteExpiredfilters globally onexpires/lastSeenAtwithout a leadinguserIdpredicate. As the sessions table grows, this periodic cross-user cleanup could degrade into a sequential scan. Consider a standalone index onexpiresand/orlastSeenAtif cleanup runs frequently against a large table.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/auth/sessions.repository.ts` around lines 167 - 186, The `deleteExpired` housekeeping query scans sessions globally by `expires` and `lastSeenAt`, so the existing `sessions_user_expires_idx` on `(userId, expires)` won’t help this path. Add an index that matches the cross-user predicates used in `deleteExpired`—for example a standalone index on `sessions.expires` and/or `sessions.lastSeenAt`—so the periodic cleanup stays efficient as the table grows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/app.setup.ts`:
- Around line 34-43: The getTrustProxySetting helper currently forwards any
non-numeric TRUST_PROXY value back to Express, which can cause proxy-addr to
fail startup on unsupported strings. Update this function to validate and
whitelist only the supported trust-proxy formats before returning, and make
invalid values fail with a clear configuration error instead of passing raw
strings through to app.set('trust proxy', ...). Keep the fix centered in
getTrustProxySetting so all callers inherit the stricter parsing.
In `@apps/api/src/auth/sessions.repository.ts`:
- Around line 106-113: `listForUser` currently treats sessions as active based
only on `expires`, but `findActiveAndTouch` also requires `lastSeenAt` to be
within `idleTtlMs`. Update `SessionsRepository.listForUser` to accept and use
`idleTtlMs` so it filters out idle sessions too, mirroring the same usability
rule as `findActiveAndTouch`; use the existing `sessions`, `and`, `eq`, `gt`,
and `desc` query pattern to add the `lastSeenAt`/idle constraint.
---
Outside diff comments:
In `@apps/api/src/auth/auth.service.ts`:
- Around line 96-122: The failed-auth path in validateToken is doing an extra
blocking delete after findActiveAndTouch returns no usable session, which adds
unnecessary DB work for every invalid token. Remove the inline
deleteStaleByTokenHash call from AuthService.validateToken, or move that
housekeeping to a background/periodic cleanup path so the request path only
performs the existing findActiveAndTouch lookup and returns undefined on
failure.
---
Nitpick comments:
In `@apps/api/src/app.module.ts`:
- Around line 36-38: The throttling setup in ThrottlerModule.forRoot currently
uses the default in-memory counters, so limits are enforced per process rather
than across replicas. Update the AppModule throttler configuration to use a
shared storage backend such as Redis, wiring it into the throttler options so
the default login/register rate limit is global across all instances.
In `@apps/api/src/auth/session-cleanup.service.ts`:
- Around line 26-49: Wrap the startup wiring in
SessionCleanupService.onApplicationBootstrap with error handling so failures in
queue.ensureQueue, queue.schedule, or queue.consume don’t take down the API if
the queue backend is temporarily unavailable. Add a fallback path that logs the
problem and skips session cleanup setup rather than crashing, and introduce
OnModuleDestroy on SessionCleanupService to stop the SESSIONS_CLEANUP_QUEUE
consumer during shutdown. Use the existing queue and logger fields, and keep the
cleanup logic in cleanup() unchanged.
In `@apps/api/src/auth/sessions.repository.ts`:
- Around line 167-186: The `deleteExpired` housekeeping query scans sessions
globally by `expires` and `lastSeenAt`, so the existing
`sessions_user_expires_idx` on `(userId, expires)` won’t help this path. Add an
index that matches the cross-user predicates used in `deleteExpired`—for example
a standalone index on `sessions.expires` and/or `sessions.lastSeenAt`—so the
periodic cleanup stays efficient as the table grows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1da6ad6f-7f6c-4f31-ab66-fc3afff84db1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
CHANGELOG.mdapps/api/.env.exampleapps/api/openapi.jsonapps/api/package.jsonapps/api/src/app.controller.tsapps/api/src/app.module.tsapps/api/src/app.setup.tsapps/api/src/auth/auth.controller.tsapps/api/src/auth/auth.module.tsapps/api/src/auth/auth.service.spec.tsapps/api/src/auth/auth.service.tsapps/api/src/auth/constants.tsapps/api/src/auth/public.decorator.tsapps/api/src/auth/session-auth.guard.spec.tsapps/api/src/auth/session-auth.guard.tsapps/api/src/auth/session-cleanup.service.tsapps/api/src/auth/sessions.repository.tsapps/api/src/chats/chats-rls.integration.spec.tsapps/api/src/chats/chats.controller.tsapps/api/src/db/migrations/0014_greedy_maddog.sqlapps/api/src/db/migrations/meta/0014_snapshot.jsonapps/api/src/db/migrations/meta/_journal.jsonapps/api/src/db/schema/auth.tsapps/api/src/runs/runs.controller.tsapps/api/test/auth.e2e-spec.ts
💤 Files with no reviewable changes (2)
- apps/api/src/runs/runs.controller.ts
- apps/api/src/chats/chats.controller.ts
All eight distinct bot findings verified and fixed: - TRUST_PROXY: 'true'/'false' now become real booleans (as strings, Express would treat 'true' as a subnet list, fail to parse, and silently trust nothing — breaking req.ip, per-IP throttling, and session IP attribution); a non-integer or negative hop count now fails startup loudly instead of skewing req.ip silently - sessions cleanup: indexes on expires and last_seen_at (the hourly delete filters on each alone; neither was reachable via the (user_id, expires) composite) — migration 0014 regenerated with all three indexes and a journal timestamp strictly after 0013's (the existing-database upgrade lesson from #108) - cleanup sweep failures are logged locally, then rethrown — pg-boss retry/dead-letter semantics unchanged, but the failure is visible in app logs, not just the queue tables - listForUser and findByIdForUser now apply the SAME validity filter as findActiveAndTouch (expiry AND idle TTL) — a session the list shows is a session that would authenticate, and an expired-but-not- purged row can never surface as the "current" session - the stale-session housekeeping delete on the null-auth path is best-effort: a DB error there logs a warning instead of turning an unauthenticated request into a 500 (the cleanup cron covers misses) - the purge integration test asserts PHYSICAL deletion via the raw table (listForUser filters expired rows, so it would have passed even if the delete no-opped) Verified: tsgo + oxlint (type-aware) + prettier clean, unit 130 (exit-code checked, maxWorkers capped), full rls-test.sh green (RLS 13, queue 8, e2e 35). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
|
Rebased onto current master (post #108): only the two auth commits replay; migration renumbered 0015 → 0014 and regenerated from the schema (journal timestamp strictly after 0013's — the existing-database upgrade lesson from #108), session-cleanup ported to the typed queue API, lockfile regenerated against the pnpm catalog. All eight distinct review findings fixed and each thread answered individually — highlights: TRUST_PROXY now handles boolean strings and rejects malformed hop counts at startup (as a string, 'true' would have made Express silently trust nothing); listForUser/findByIdForUser share findActiveAndTouch's exact validity semantics; cleanup-path failures can no longer 500 an unauthenticated request; the purge test asserts physical deletion. With this, all umbrella #101 threads are also resolved (fixed here, fixed by earlier slices, tracked in #125, or deferred to the browser-resume slice). Verified per push: unit 130, RLS 13, queue 8, e2e 35. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/db/migrations/0014_glamorous_speed_demon.sql (1)
1-3: 🚀 Performance & Scalability | 🔵 TrivialOperational note: index build briefly blocks session writes.
These plain
CREATE INDEXstatements take a lock that blocks concurrent writes (new logins,last_seen_attouches) onsessionswhile they build. For small self-hosted tables this is negligible, but on a largesessionstable on a live deployment consider building theseCONCURRENTLYout-of-band (drizzle runs migrations in a transaction, soCONCURRENTLYcan't be emitted here). Otherwise this is fine.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/db/migrations/0014_glamorous_speed_demon.sql` around lines 1 - 3, The session index migration currently uses plain CREATE INDEX statements, which can block writes on the sessions table during deployment. Update the migration for the sessions indexes to avoid doing these builds inside the transactional Drizzle migration path; either leave them as-is only if blocking is acceptable, or move the index creation out-of-band and use CONCURRENTLY for the sessions_user_expires_idx, sessions_expires_idx, and sessions_last_seen_at_idx indexes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/api/src/db/migrations/0014_glamorous_speed_demon.sql`:
- Around line 1-3: The session index migration currently uses plain CREATE INDEX
statements, which can block writes on the sessions table during deployment.
Update the migration for the sessions indexes to avoid doing these builds inside
the transactional Drizzle migration path; either leave them as-is only if
blocking is acceptable, or move the index creation out-of-band and use
CONCURRENTLY for the sessions_user_expires_idx, sessions_expires_idx, and
sessions_last_seen_at_idx indexes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d756901d-374c-4506-a441-daa44e35520b
📒 Files selected for processing (9)
apps/api/src/app.setup.tsapps/api/src/auth/auth.service.tsapps/api/src/auth/session-cleanup.service.tsapps/api/src/auth/sessions.repository.tsapps/api/src/chats/chats-rls.integration.spec.tsapps/api/src/db/migrations/0014_glamorous_speed_demon.sqlapps/api/src/db/migrations/meta/0014_snapshot.jsonapps/api/src/db/migrations/meta/_journal.jsonapps/api/src/db/schema/auth.ts
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/db/migrations/meta/0014_snapshot.json
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api/src/db/schema/auth.ts
- apps/api/src/auth/session-cleanup.service.ts
- apps/api/src/auth/auth.service.ts
- apps/api/src/chats/chats-rls.integration.spec.ts
The rate-limit counters are in-memory per process: api × N replicas multiply the effective ceiling by N and restarts reset it. Fine single-node; a shared ThrottlerStorage becomes necessary with #116 — noted at the wiring site and in docs/scaling.md so the limit is never mistaken for a fleet-wide guarantee. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014peA4x8efDtokaggcbyokP
1 participant
Summary
Issues
Deferred in later stack PRs
Summary by CodeRabbit
New Features
Bug Fixes