fix(expo): Prevent stale native clients from replacing signed-in sess…#9222
fix(expo): Prevent stale native clients from replacing signed-in sess…#9222wobsoriano wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: c9a13a1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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.
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@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: |
There was a problem hiding this comment.
Really nice work tracking this down — I went through the FAPI side to sanity-check the assumptions and the foreign-sessionless rejection holds up well against what the server actually does (a stale token on GET /v1/client re-mints a lazy client reusing the same id, so the validation fetch is deterministic). I found one interaction with the 401 recovery path that I think can still reproduce the original bug, plus two smaller things — details inline.
| }); | ||
| } | ||
|
|
||
| refreshedClient = await fetchRefreshedJsClient(clerkInstance); |
There was a problem hiding this comment.
I think there's a window here worth closing before we ship: the native token lands in the shared cache before the fetch resolves, and if a session token refresh 401s in that window, the intercepted handleUnauthenticated recovery
javascript/packages/expo/src/provider/nativeClientSync.tsx
Lines 778 to 788 in c9a13a1
refreshJsClientFromNativeState without the new guard — so it adopts the foreign sessionless client via updateClient. The outer validation then restores the token but not the client/session state, which leaves a good token in the cache with session null. I managed to reproduce it with a regression test, so it seems worth handling.
A couple of ideas, curious what you think:
- Share the in-flight validation promise and have the recovery path await it — once the good token is restored the 401 should mostly resolve itself.
- Or simply thread
rejectForeignSessionlessClient+ previous client/token into the recovery call too, so every path gets the same protection.
The fully airtight version (validating the candidate token without publishing it first) would need client.fetch to take an explicit token, which is clerk-js plumbing — totally fine as a follow-up.
| return; | ||
| } | ||
|
|
||
| const previousDeviceToken = didChangeDeviceToken ? await getCachedDeviceToken(tokenCache) : undefined; |
There was a problem hiding this comment.
Small edge case: getCachedDeviceToken's 1s timeout resolves to null, same as "confirmed no token". If the cache read is just slow and we later hit the rejection/error path, the restore ends up calling clearToken on a token that actually exists. Maybe return undefined on timeout so the restore stays a no-op? Could also argue for skipping native adoption entirely in that case since we don't have a reliable snapshot — but I don't feel strongly about that part.
| } | ||
| } | ||
|
|
||
| function getDeviceTokenIssuedAt(deviceToken: string | null): number | null { |
There was a problem hiding this comment.
One thing I noticed while checking the server side: client cookie JWTs are signed with exactly {id, rotating_token}
(https://github.com/clerk/clerk_go/blob/01fd589c0a2a283317ddfb89674bcd5d62b71e4f/model/client_cookie.go#L22-L25) and nothing injects iat, so I believe this branch can't fire with real tokens.
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change