fix(clerk-js): apply session tokens monotonically on a single tab#9030
fix(clerk-js): apply session tokens monotonically on a single tab#9030nikosdouvlis wants to merge 14 commits into
Conversation
… tab A stale edge-minted session token (older oiat header) could overwrite a fresher one on the same tab. The only freshness guard ran solely in the cross-tab broadcast receiver, so the __session cookie, Session.lastActiveToken, and the token cache each accepted whatever token arrived last, letting a slow or out-of-order edge read revert a newer token. Reuse pickFreshestJwt across all three stores: - tokenCache.setInternal keeps the live entry's resolvedToken monotonic regardless of which resolver owns the slot, carries the prior token and its expiry forward, and derives expiry/refresh timers from the winner. - Session.#fetchToken and #refreshTokenInBackground resolve to the freshest of the fetched token and the live cache entry, so coalesced waiters and the dispatch receive the winner; lastActiveToken never regresses to a strictly-staler token. - Session.fromJSON reconciles an incoming last_active_token against the active cache slot, never adopting a strictly-staler or wrong-org token nor polluting the slot getToken reads. - AuthCookieService.updateSessionCookie drops tokens for a different active session/org or strictly staler than the same-context cookie, failing open for tokens without oiat. Freshness is decided by oiat then iat; drops happen only when both tokens carry oiat and the incoming is strictly older, so a missing oiat or a different context always lets the token through.
🦋 Changeset detectedLatest commit: 8aaf690 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds monotonic staleness checks across shared token helpers, session token caching, session hydration and refresh flows, and session cookie updates. ChangesMonotonic Session Token Guard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/clerk-js/src/core/resources/Session.ts (1)
212-242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the explicit
voidreturn type.This new helper only mutates session/cache state; annotate it to satisfy the TypeScript return-type rule. As per coding guidelines, "
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs".Proposed fix
- `#applyIncomingLastActiveToken`(raw: SessionJSON['last_active_token'] | null) { + `#applyIncomingLastActiveToken`(raw: SessionJSON['last_active_token'] | null): void {🤖 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 `@packages/clerk-js/src/core/resources/Session.ts` around lines 212 - 242, The new helper `#applyIncomingLastActiveToken` currently relies on type inference, but it should have an explicit void return type to satisfy the TypeScript return-type rule. Update the method signature on Session to declare the return type explicitly, keeping the existing cache/token mutation logic unchanged.Source: Coding guidelines
packages/clerk-js/src/core/resources/__tests__/Session.test.ts (1)
2074-2082: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid defaulting the helper generic to
any.Use
unknownas the default so callers either narrow or provide a concrete type. Based on learnings, internal test helpers may omit explicit return types, but the guideline still disallows unjustifiedany.Proposed fix
- function deferred<T = any>() { + function deferred<T = unknown>() {🤖 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 `@packages/clerk-js/src/core/resources/__tests__/Session.test.ts` around lines 2074 - 2082, The deferred helper in Session.test.ts defaults its generic to any, which violates the typing guideline. Update deferred<T = any>() to use unknown as the default generic instead, and keep the resolve/reject/promise shape unchanged so callers of deferred() in the Session.test suite must either narrow the value or supply a concrete type via the deferred symbol.Sources: Coding guidelines, Learnings
🤖 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 `@packages/clerk-js/src/core/__tests__/clerk.test.ts`:
- Around line 908-922: Add a positive test for an organization-scoped token
whose org_id matches the active organization. In clerk.test.ts рядом with the
existing loadClerkWithSession/emitToken cases, create a matching org JWT and
assert document.cookie contains it, so the org-context guard in the token
handling flow still accepts valid active-organization tokens while rejecting
wrong-org ones.
In `@packages/clerk-js/src/core/auth/AuthCookieService.ts`:
- Around line 221-227: The token filtering in AuthCookieService should not
reject a target-session `TokenUpdate` during `Clerk.setActive` just because
`this.clerk.session?.id` has not been committed yet. Update the session check in
the logic around `tokenSid(incoming)` so it can accept the token for the session
being activated, and make sure the `setActive` flow that calls
`newSession.getToken()` before `#updateAccessors(newSession)` still allows the
new `__session` cookie to be written. Use the existing `tokenSid`, `tokenOrgId`,
and `normalizeOrgId` comparisons in `AuthCookieService` to adjust the condition
without breaking org filtering.
In `@packages/clerk-js/src/core/tokenCache.ts`:
- Around line 360-372: The current TokenCache merge path in tokenCache.ts
carries forward an existing resolved token and TTL, but the rejection handling
still removes that live entry when a replacement resolver fails. Update the
rejection path associated with the refresh/skipCache resolver so it does not
delete or overwrite the carried-forward entry when
`existing.entry.resolvedToken` and `existing.expiresIn` were preserved; instead,
keep the prior token/TTL intact and only clear the new pending resolution state.
Use the `resolvedToken` merge logic and the cache entry write/remove flow in
`TokenCache` to locate and adjust the failure path.
---
Nitpick comments:
In `@packages/clerk-js/src/core/resources/__tests__/Session.test.ts`:
- Around line 2074-2082: The deferred helper in Session.test.ts defaults its
generic to any, which violates the typing guideline. Update deferred<T = any>()
to use unknown as the default generic instead, and keep the
resolve/reject/promise shape unchanged so callers of deferred() in the
Session.test suite must either narrow the value or supply a concrete type via
the deferred symbol.
In `@packages/clerk-js/src/core/resources/Session.ts`:
- Around line 212-242: The new helper `#applyIncomingLastActiveToken` currently
relies on type inference, but it should have an explicit void return type to
satisfy the TypeScript return-type rule. Update the method signature on Session
to declare the return type explicitly, keeping the existing cache/token mutation
logic unchanged.
🪄 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: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 101fbb04-b962-438e-aded-5ec66a2d39c0
📒 Files selected for processing (9)
.changeset/monotonic-session-token-guard.mdpackages/clerk-js/src/core/__tests__/clerk.test.tspackages/clerk-js/src/core/__tests__/tokenCache.test.tspackages/clerk-js/src/core/__tests__/tokenFreshness.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/resources/__tests__/Session.test.tspackages/clerk-js/src/core/tokenCache.tspackages/clerk-js/src/core/tokenFreshness.ts
| it('drops a token for a different organization', async () => { | ||
| await loadClerkWithSession(); | ||
|
|
||
| const wrongOrg = createJwtWithOiat(1000, 200, { org: 'org_other' }); | ||
| emitToken(wrongOrg); | ||
| expect(document.cookie).not.toContain(wrongOrg); | ||
| }); | ||
|
|
||
| it('applies a personal-workspace token (no org) for the active personal workspace', async () => { | ||
| await loadClerkWithSession(); | ||
|
|
||
| const personal = createJwtWithOiat(1000, 200); | ||
| emitToken(personal); | ||
| expect(document.cookie).toContain(personal); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a positive active-organization token case.
The suite covers wrong-org rejection and personal-workspace acceptance, but not a token whose org_id matches the active organization. Add that case so the new org-context guard cannot regress into rejecting valid organization-scoped tokens. As per coding guidelines, “Unit tests are required for all new functionality” and should “Verify proper error handling and edge cases.”
🤖 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 `@packages/clerk-js/src/core/__tests__/clerk.test.ts` around lines 908 - 922,
Add a positive test for an organization-scoped token whose org_id matches the
active organization. In clerk.test.ts рядом with the existing
loadClerkWithSession/emitToken cases, create a matching org JWT and assert
document.cookie contains it, so the org-context guard in the token handling flow
still accepts valid active-organization tokens while rejecting wrong-org ones.
Source: Coding guidelines
#applyIncomingLastActiveToken only used the active-org cache slot as the freshness baseline. When that slot was empty (evicted, or an in-flight fetch leaves resolvedToken null) a strictly-staler or wrong-org last_active_token from fromJSON was adopted, regressing lastActiveToken. Fall back to the token already held as the freshness baseline when the cache slot is empty, but only when that held token is same-context, so a token left over from the previous org cannot veto a valid token for the active org. When no same-context baseline exists, adopt the incoming token instead of clearing lastActiveToken: a wrong-org or staler token is still caught downstream by the cookie guard, whereas an empty lastActiveToken is treated as sign-out and drops the session cookie.
a3082a3 to
570b749
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/clerk-js/src/core/resources/__tests__/Session.test.ts (1)
2441-2447: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid widening the session to
anyfor hydration calls.Use a narrow test-only interface/helper for
fromJSONinstead of repeated(session as any)casts.♻️ Suggested refactor
+ const hydrateSession = (target: unknown, json: SessionJSON): void => { + (target as { fromJSON(json: SessionJSON): void }).fromJSON(json); + }; + - (session as any).fromJSON(sessionJsonWith(high)); + hydrateSession(session, sessionJsonWith(high));As per coding guidelines, “No
anytypes without justification in code review.”Also applies to: 2470-2476, 2486-2486, 2502-2508
🤖 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 `@packages/clerk-js/src/core/resources/__tests__/Session.test.ts` around lines 2441 - 2447, The test is repeatedly widening Session to any just to call fromJSON, which should be replaced with a narrow test-only helper or interface for hydration. Update Session.test.ts to introduce a typed helper or local interface that exposes fromJSON, then use that instead of repeated (session as any) casts in the affected hydration cases. Keep the change limited to the test code and reference the Session.fromJSON call sites so the casts are removed consistently.Source: Coding guidelines
🤖 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 `@packages/clerk-js/src/core/resources/__tests__/Session.test.ts`:
- Around line 2441-2447: The test is repeatedly widening Session to any just to
call fromJSON, which should be replaced with a narrow test-only helper or
interface for hydration. Update Session.test.ts to introduce a typed helper or
local interface that exposes fromJSON, then use that instead of repeated
(session as any) casts in the affected hydration cases. Keep the change limited
to the test code and reference the Session.fromJSON call sites so the casts are
removed consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b3dc1eb-50c8-45d3-a022-b71890480952
📒 Files selected for processing (2)
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/resources/__tests__/Session.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/src/core/resources/Session.ts
…c-session-token # Conflicts: # packages/clerk-js/src/core/tokenCache.ts
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/clerk-js/src/core/tokenCache.ts (2)
250-253: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon’t block broadcast updates on a pending local resolver.
Line 253 can wait indefinitely on a slow/stuck local
tokenResolver, preventing a fresher cross-tab token from being applied. If there’s noresolvedTokenyet, proceed withsetInternal; the resolver path already reconciles against the live cache before publishing.Proposed fix
- const existingToken = result.entry.resolvedToken ?? (await result.entry.tokenResolver); - if (pickFreshestJwt(existingToken, token) === existingToken) { + const existingToken = result.entry.resolvedToken; + if (existingToken && pickFreshestJwt(existingToken, token) === existingToken) {🤖 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 `@packages/clerk-js/src/core/tokenCache.ts` around lines 250 - 253, The local token lookup in token cache handling is waiting on result.entry.tokenResolver when resolvedToken is missing, which can block broadcast updates; update the flow around get(...) and the existingToken resolution so it does not await a pending local resolver before applying setInternal. Prefer using result.entry.resolvedToken only, and let the tokenResolver path reconcile with the live cache later before publishing.
382-414: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSchedule timers from remaining TTL, not full token lifetime.
expiresInisexp - iat, butsetTimeoutand proactive refresh run relative to now. For an already-aged winner token, this delays cleanup/refresh by the token’s age and can keep the cache from refreshing before actual expiry.Proposed fix
const issuedAt = claims.iat; const expiresIn: Seconds = claims.exp - issuedAt; + const remainingTtl: Seconds = claims.exp - Math.floor(Date.now() / 1000); + + if (remainingTtl <= 0) { + if (store.get(key) === live) { + clearTimeout(live.timeoutId); + clearTimeout(live.refreshTimeoutId); + store.delete(key); + } + return; + } live.createdAt = issuedAt; live.expiresIn = expiresIn; @@ - const timeoutId = setTimeout(liveDeleteKey, expiresIn * 1000); + const timeoutId = setTimeout(liveDeleteKey, remainingTtl * 1000); @@ - const refreshFireTime = expiresIn - leeway - refreshLeadTime; + const refreshFireTime = remainingTtl - leeway - refreshLeadTime;🤖 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 `@packages/clerk-js/src/core/tokenCache.ts` around lines 382 - 414, The timer scheduling in tokenCache.ts uses the token’s full lifetime (`expiresIn = claims.exp - claims.iat`) instead of the remaining TTL from now, so cleanup and proactive refresh can run too late for an already-aged token. In the logic around `liveDeleteKey`, `timeoutId`, and `refreshFireTime`, compute the remaining time until expiry from the current time and schedule both timers off that remaining TTL rather than `exp - iat`. Keep the proactive refresh buffer (`refreshLeadTime`, `BACKGROUND_REFRESH_THRESHOLD_IN_SECONDS`, `POLLER_INTERVAL_IN_MS`) applied to the remaining time so refresh still fires before the actual expiry.
🤖 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.
Outside diff comments:
In `@packages/clerk-js/src/core/tokenCache.ts`:
- Around line 250-253: The local token lookup in token cache handling is waiting
on result.entry.tokenResolver when resolvedToken is missing, which can block
broadcast updates; update the flow around get(...) and the existingToken
resolution so it does not await a pending local resolver before applying
setInternal. Prefer using result.entry.resolvedToken only, and let the
tokenResolver path reconcile with the live cache later before publishing.
- Around line 382-414: The timer scheduling in tokenCache.ts uses the token’s
full lifetime (`expiresIn = claims.exp - claims.iat`) instead of the remaining
TTL from now, so cleanup and proactive refresh can run too late for an
already-aged token. In the logic around `liveDeleteKey`, `timeoutId`, and
`refreshFireTime`, compute the remaining time until expiry from the current time
and schedule both timers off that remaining TTL rather than `exp - iat`. Keep
the proactive refresh buffer (`refreshLeadTime`,
`BACKGROUND_REFRESH_THRESHOLD_IN_SECONDS`, `POLLER_INTERVAL_IN_MS`) applied to
the remaining time so refresh still fires before the actual expiry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4feb6c1e-fcaf-4f22-998f-4273468c2874
📒 Files selected for processing (4)
packages/clerk-js/src/core/__tests__/clerk.test.tspackages/clerk-js/src/core/__tests__/tokenCache.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/tokenCache.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/clerk-js/src/core/tests/clerk.test.ts
- packages/clerk-js/src/core/auth/AuthCookieService.ts
- packages/clerk-js/src/core/tests/tokenCache.test.ts
oiat is stamped at origin (origin-issued-at, set when claims are assembled from the DB), not by the edge token minter, so a token that lacks oiat is a pre-feature token and is genuinely staler than any token that has one. That is exactly how pickFreshestJwt already ranks tokens. isStrictlyStalerJwt reimplemented the same oiat-then-iat comparison only to fail open on a missing oiat, guarding a fresh-token-without-oiat case that cannot arise from edge rollout. Drop it and route the cookie, lastActiveToken hydrate, and token-dispatch guards through pickFreshestJwt, removing the duplicated comparison.
setInternal repeated the same teardown (clear both timers, delete the slot if it still holds this value) in three places: the reject path, the malformed-claims branch, and the expiry timer. Collapse them into one dropIfCurrent helper, which also drops the redundant undefined-guards before clearTimeout (clearTimeout ignores undefined). #dispatchTokenEvents had a dead lastActiveToken null-check: pickFreshestOrIncoming already returns the incoming token when the existing one is null, so the extra branch could never change the result.
pickFreshestOrIncoming only existed to return the incoming token when there was no baseline. Teach pickFreshestJwt to tolerate a null/undefined existing token directly and drop the wrapper, so the optional-baseline call sites and the ones with a guaranteed baseline share one function.
…+org
The cookie guard dropped any token whose sid/org did not match the active
session/org. During a multi-session setActive({ session }) switch, clerk.session
lags behind the dispatched token (it is committed after getToken), so the new
session's token was dropped and the cookie held the old session's token until the
next poll.
Scope the guard to a same session+org comparison: it now only drops a token that
is strictly staler than a same-context cookie, and writes through (fails open) on
any cross-context token, matching pre-feature behavior across a session or org
switch. Cross-context cookie protection is deferred to a follow-up.
…kie guard The cookie guard no longer drops a cross-context token downstream (it now fails open on a session/org mismatch), so #applyIncomingLastActiveToken's note that a bad token 'is still dropped downstream by the cookie guard' was false. Adopting the incoming token with no same-context baseline is justified on its own: an empty lastActiveToken reads as a sign-out, and the adopted token is not cached under the active-org key.
Why: The inline org-context freshness guard in the session token hydration path was dense and mixed the wrong-org veto with the monotonic freshness pick. What changed: Moved the same-context selection into pickSameContextFreshestJwt and isSameOrgContext in tokenFreshness.ts and restored the #hydrateCache name. No behavior change; the piggyback and monotonic guard suites still pass.
…e hydrateCache guard
Why: The guard added monotonic and org-context handling when applying a piggybacked last_active_token, but its necessity on that path was not established: the reproduced regression was the fetch path, and its unit tests only restated the guard's own behavior. Falling back to the simpler hydration until piggyback staleness is confirmed. What changed: Restored main's #hydrateCache (plain cache seed) and the direct fromJSON assignment, and removed the fromJSON piggyback guard test suite. Fetch-path monotonic guards are unchanged.
Why: The Token import went unused after the piggyback last_active_token guard tests were dropped; eslint's unused-imports rule flagged it as an error and failed the Static analysis CI job.
Why
A stale edge-minted session token (older
oiatheader) could overwrite a fresher one on the same tab. The freshness guard only ran in the cross-tab broadcast receiver, so the__sessioncookie,Session.lastActiveToken, and the token cache each just took whatever token landed last. A slow or out-of-order edge read could revert a newer token.What changed
Reuse the existing
pickFreshestJwtcomparison across all three stores so a token only moves forward in freshness. The token cache keeps its live entry monotonic and carries the prior token and its expiry forward.SessionresolvesgetTokenandlastActiveTokento the freshest of the fetched and cached token, andfromJSONwon't adopt a staler or wrong-org token during hydration. The cookie service drops tokens for a different session/org or strictly staler than the same-context cookie.Freshness is decided by
oiattheniat, and a drop only happens when both tokens carryoiatand the incoming one is strictly older. A missingoiator a different context always lets the token through, so this fails open.Summary by CodeRabbit
Bug Fixes
lastActiveTokenand token cache consistency, with fail-open behavior when freshness info is missing.New Features
isStrictlyStalerJwt,pickFreshestOrIncoming, tokenoiat/sid/orgextraction).Tests