feat: expire session relative to creation #2144
Conversation
WalkthroughrenewSession now anchors session expiry to the session's creation time (uses userSession.createdAt.getTime()), computes absolute expiresAt, refuses renewal when that lifetime has elapsed, persists the adjusted expiresAt, and issues the JWT with maxAge equal to the remaining seconds until that absolute expiry. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
controlplane/src/core/auth-utils.ts (1)
349-356: Align JWT maxAge with remaining fixed session lifetimeCurrently the encrypted session token is minted with the full DEFAULT_SESSION_MAX_AGE_SEC, which reintroduces sliding validity for decryption even when the absolute session lifetime is shorter. Use the computed remainingSeconds instead.
- const jwt = await encrypt<UserSession>({ - maxAgeInSeconds: sessionExpiresIn, + const jwt = await encrypt<UserSession>({ + maxAgeInSeconds: remainingSeconds,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
controlplane/src/core/auth-utils.ts(1 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: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
- GitHub Check: build_test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
controlplane/src/core/auth-utils.ts (4)
320-326: Round up TTL and capture time once to avoid off-by-one and driftUsing floor and multiple Date.now() calls can produce a 0-second TTL when only a few ms remain and introduces minor drift. Prefer capturing now once and using ceil.
Apply this diff:
- const baseMs = userSession.createdAt?.getTime() ?? Date.now(); - const expiresAtMs = baseMs + DEFAULT_SESSION_MAX_AGE_SEC * 1000; - const sessionExpiresDate = new Date(expiresAtMs); - const remainingSeconds = Math.max(0, Math.floor((expiresAtMs - Date.now()) / 1000)); + const nowMs = Date.now(); + const baseMs = userSession.createdAt?.getTime() ?? nowMs; + const expiresAtMs = baseMs + DEFAULT_SESSION_MAX_AGE_SEC * 1000; + const sessionExpiresDate = new Date(expiresAtMs); + const remainingSeconds = Math.max(0, Math.ceil((expiresAtMs - nowMs) / 1000));
356-358: Avoid TTL drift at issuance: recompute remaining seconds for the JWTBetween computing remainingSeconds and issuing the JWT, some time elapses (network call + DB update). Recompute just-in-time so the JWT doesn’t outlive the cookie’s expiry. Minimal change inline below. If you prefer stricter semantics, re-check and abort when the recomputed value is 0.
Apply this diff:
- maxAgeInSeconds: remainingSeconds, + maxAgeInSeconds: Math.max(0, Math.ceil((sessionExpiresDate.getTime() - Date.now()) / 1000)),
291-294: Update the docstring to reflect fixed-lifetime enforcementThe behavior now also rejects renewal when the absolute session lifetime is exceeded. Reflect that to keep docs accurate.
/** - * renewSession renews the user session if the access token is expired. - * If the refresh token is expired, an error is thrown. + * renewSession renews the user session if the access token is (near) expired, + * provided the session has not exceeded its absolute lifetime (createdAt + DEFAULT_SESSION_MAX_AGE_SEC). + * If the refresh token is expired or the absolute lifetime has elapsed, an error is thrown. */
320-330: Add tests for absolute lifetime and rounding behaviorPlease add/adjust tests to cover:
- Renewal fails when createdAt + DEFAULT_SESSION_MAX_AGE_SEC is in the past.
- JWT maxAge aligns with the remaining absolute lifetime (rounding up from sub-second intervals).
- Fallback behavior when createdAt is missing (if applicable to legacy data).
I can draft these unit/integration tests with table/fixtures for sessions and clock control (using fake timers). Want me to open a PR with the test suite?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
controlplane/src/core/auth-utils.ts(2 hunks)
🔇 Additional comments (2)
controlplane/src/core/auth-utils.ts (2)
327-330: LGTM: refuse renewal once absolute lifetime has elapsedThe guard correctly enforces a fixed session lifetime relative to creation.
322-324: createdAt column enforces non-null with default now()
Thesessionstable schema definescreatedAtastimestamp('created_at', { withTimezone: true }).notNull().defaultNow()so every new session row will always have a valid timestamp. The
?? Date.now()fallback only applies to very old rows that predate this column.
…xpire-with-keycloaks-session' into wilson/eng-7923-session-should-expire-with-keycloaks-session
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
controlplane/src/core/auth-utils.ts (2)
324-324: Use Math.ceil for TTL to avoid sub-second early expiry flakinessUsing Math.floor can cause the JWT to expire up to ~1s before the cookie’s expires attribute, creating a brief mismatch. Ceil keeps them aligned (the cookie still enforces the absolute boundary).
- const remainingSeconds = Math.max(0, Math.floor((expiresAtMs - Date.now()) / 1000)); + const remainingSeconds = Math.max(0, Math.ceil((expiresAtMs - Date.now()) / 1000));
326-329: Clear the expired cookie before throwing to avoid client retry loopsWhen the absolute lifetime has elapsed, proactively expire the session cookie so clients don’t keep sending an already-dead cookie and hitting 401s repeatedly.
if (remainingSeconds <= 0) { - // Absolute session lifetime has elapsed; do not renew. + // Absolute session lifetime has elapsed; do not renew. + // Proactively clear the cookie to avoid client retry loops with a stale session cookie. + this.clearCookie(res, this.opts.session.cookieName); throw new AuthenticationError(EnumStatusCode.ERROR_NOT_AUTHENTICATED, 'Session expired'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
controlplane/src/core/auth-utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controlplane/src/core/auth-utils.ts (2)
controlplane/src/core/crypto/jwt.ts (1)
DEFAULT_SESSION_MAX_AGE_SEC(7-7)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: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (2)
controlplane/src/core/auth-utils.ts (2)
356-356: JWT TTL aligned with absolute session expiry — LGTMUsing remainingSeconds for maxAge keeps the encrypted cookie/JWT validity aligned with the absolute session end. Good.
320-325: HardencreatedAthandling and clamp future‐skewed timestampsBefore trusting
userSession.createdAt, make sure it’s always present and a real Date and then clamp it to “now” so clock skew can’t silently extend a session’s absolute lifetime:// The session expiration is relative to the creation time + // Ensure `createdAt` exists, is a Date, and not in the future. + const createdAtMs = userSession.createdAt?.getTime?.(); + if (createdAtMs == null || Number.isNaN(createdAtMs)) { + throw new AuthenticationError( + EnumStatusCode.ERROR_NOT_AUTHENTICATED, + 'Invalid session: missing or invalid creation timestamp', + ); + } + const baseMs = Math.min(createdAtMs, Date.now()); const expiresAtMs = baseMs + DEFAULT_SESSION_MAX_AGE_SEC * 1000; const sessionExpiresDate = new Date(expiresAtMs); const remainingSeconds = Math.max(0, Math.floor((expiresAtMs - Date.now()) / 1000));Before merging, please verify that:
- Your JWT‐payload type (
UserSession) actually declares acreatedAt: Datefield (either directly or viaJWTPayload).- When you call
encrypt<UserSession>({ token: { … } }), you includecreatedAt: new Date()so every session token carries a valid timestamp.
wilsonrivera
deleted the
wilson/eng-7923-session-should-expire-with-keycloaks-session
branch
Summary by CodeRabbit
Checklist