feat: validate session cookie #2406
Conversation
wilsonrivera
requested review from
JivusAyrus,
StarpTech and
thisisnithin
as code owners
WalkthroughAdds a centralized session expiration check, enforces DB-backed session validation during JWT authentication, updates WebSessionAuthenticator constructor to accept the DB, and applies stronger typing plus consolidated transaction logic in auth route handlers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Tip ✨ Issue Enrichment is now available for GitHub issues!CodeRabbit can now help you manage issues more effectively:
Disable automatic issue enrichmentTo disable automatic issue enrichment, add the following to your issue_enrichment:
auto_enrich:
enabled: falseComment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
controlplane/src/core/auth-utils.ts (1)
302-312: Clarify the dual expiration check semantics.The method implements two expiration checks:
- Absolute:
expiresAt <= now- hard session end-of-life- Relative:
(updatedAt ?? createdAt) + DEFAULT_SESSION_MAX_AGE_SEC <= now- sliding windowSince both
expiresAtand the sliding window useDEFAULT_SESSION_MAX_AGE_SEC, they will typically expire at the same time when the session is renewed. Consider adding a brief doc comment explaining the intent of the dual check (e.g., defense in depth, or handling edge cases where expiresAt might be set differently).+ /** + * Determines if a session is expired using both absolute and sliding-window checks. + * - Absolute: session.expiresAt has passed + * - Sliding: no activity within DEFAULT_SESSION_MAX_AGE_SEC from last update + */ public static isSessionExpired(session: { createdAt: Date; updatedAt: Date | null; expiresAt: Date }): boolean {controlplane/src/core/controllers/auth.ts (3)
223-232: No-op update on conflict could be simplified.The
onConflictDoUpdatesetsuserIdandorganizationIdto the same values being inserted. This works but is effectively a no-op update just to enablereturning(). Consider usingonConflictDoNothing()with a separate select, or document that this pattern is intentional.
371-375: Verify 429 response is appropriate for lock contention.Returning HTTP 429 (Too Many Requests) when failing to acquire the advisory lock is reasonable, but typically 429 implies rate limiting. Consider if 409 (Conflict) or 503 (Service Unavailable with Retry-After) might be more semantically accurate for lock contention scenarios.
414-424: Consider more specific error handling for the callback route.The catch block redirects all errors to the base URL, which is user-friendly but may mask issues during debugging. The differentiation between
AuthenticationError(debug log) and other errors (error log) is good, but consider if certain errors should redirect to a specific error page with a user-friendly message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
controlplane/src/core/auth-utils.ts(6 hunks)controlplane/src/core/build-server.ts(1 hunks)controlplane/src/core/controllers/auth.ts(5 hunks)controlplane/src/core/services/WebSessionAuthenticator.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T10:28:04.846Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1749-1751
Timestamp: 2025-08-29T10:28:04.846Z
Learning: In the controlplane codebase, authentication and authorization checks (including organization scoping) are handled at the service layer in files like unlinkSubgraph.ts before calling repository methods. Repository methods like unlinkSubgraph() in SubgraphRepository.ts can focus purely on data operations without redundant security checks.
Applied to files:
controlplane/src/core/controllers/auth.ts
🧬 Code graph analysis (3)
controlplane/src/core/build-server.ts (1)
controlplane/src/core/services/WebSessionAuthenticator.ts (1)
WebSessionAuthenticator(19-85)
controlplane/src/core/auth-utils.ts (3)
controlplane/src/core/crypto/jwt.ts (1)
DEFAULT_SESSION_MAX_AGE_SEC(9-9)controlplane/src/core/errors/errors.ts (1)
AuthenticationError(19-19)controlplane/src/db/schema.ts (1)
sessions(1147-1169)
controlplane/src/core/controllers/auth.ts (3)
controlplane/src/types/index.ts (2)
UserInfoEndpointResponse(464-473)CustomAccessTokenClaims(456-462)controlplane/src/core/crypto/jwt.ts (4)
decodeJWT(104-106)cosmoIdpHintCookieName(17-17)DEFAULT_SESSION_MAX_AGE_SEC(9-9)encrypt(60-69)controlplane/src/core/errors/errors.ts (1)
AuthenticationError(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
controlplane/src/core/services/WebSessionAuthenticator.ts (2)
2-3: LGTM - New imports for database-backed session validation.The imports properly support the new session validation functionality using drizzle-orm.
Also applies to: 7-8
19-24: LGTM - Constructor updated to accept database instance.The constructor now properly accepts the database dependency needed for session validation.
controlplane/src/core/build-server.ts (1)
249-249: LGTM - Correctly updated to pass database instance.The instantiation now correctly passes
fastify.dbas the first argument, aligning with the updatedWebSessionAuthenticatorconstructor signature.controlplane/src/core/auth-utils.ts (3)
7-7: LGTM - Import added for date manipulation.The
addSecondsimport from date-fns is appropriately added to support the session expiration calculations.
343-347: LGTM - Session expiration check integrated into renewal flow.The renewal flow now properly validates session expiration before allowing token refresh, preventing renewal of already-expired sessions.
351-386: LGTM - Session renewal logic correctly updates all relevant fields.The renewal flow properly:
- Computes new
expiresAtbased on current time- Updates both
expiresAtandupdatedAtin the database- Uses
DEFAULT_SESSION_MAX_AGE_SECconsistently for JWT and cookie expirationcontrolplane/src/core/controllers/auth.ts (4)
42-72: LGTM - Type aliases improve code clarity.The explicit type aliases for request/response pairs enhance readability and provide clear documentation of expected query parameters for each route.
77-77: LGTM - Route handlers now use typed parameters.The
/sessionand/logoutroutes now use strongly-typed request/response parameters.Also applies to: 126-126
169-286: LGTM - Transaction consolidates user, membership, and session operations.The transaction properly:
- Upserts the user record
- Synchronizes Keycloak group memberships to organization memberships
- Upserts the session with proper conflict handling on
userId- Includes
updatedAtin the conflict update clause
288-369: LGTM - Advisory lock prevents race conditions on organization creation.The use of
pg_try_advisory_xact_lockwith a hashed user ID is a good pattern to prevent duplicate organization creation when multiple concurrent requests occur for the same user.
…-not-invalidatedestroy-the-user
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/core/services/WebSessionAuthenticator.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
controlplane/src/core/services/WebSessionAuthenticator.ts (2)
2-3: Constructor signature change is properly implemented at the only call site.The
WebSessionAuthenticatorinstantiation atcontrolplane/src/core/build-server.ts:249correctly passesfastify.db(aPostgresJsDatabase<typeof schema>instance) as the first parameter, matching the updated constructor signature. No issues found.
48-66: The review comment's concern is invalid;sessions.userIdis non-nullable in the schema.The schema defines
userIdasuuid('user_id').notNull()(line 1147 incontrolplane/src/db/schema.ts), guaranteeing it is always a non-null UUID string. The.toLowerCase()call on line 62 is safe and will not throw. The existing code already validates the critical conditions: line 44 checks thatdecryptedJwt.issis present, and line 60 checks thatexistingSessions.length === 1. The suggested refactoring to add atypeofcheck is unnecessary.Likely an incorrect or invalid review comment.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (4.70%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2406 +/- ##
===========================================
+ Coverage 42.59% 62.28% +19.68%
===========================================
Files 1013 295 -718
Lines 141063 41153 -99910
Branches 8660 4194 -4466
===========================================
- Hits 60086 25632 -34454
+ Misses 79365 15501 -63864
+ Partials 1612 20 -1592 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…-not-invalidatedestroy-the-user
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist
Enforce session validation when a request comes in from the web and addresses an issue that causes the session to appear to be closed even when the session is still valid, causing the user to have to press "Log in" just to be redirected back to the app