fix(clerk-js): apply the Safari ITP cookie refresh on sign-out - #9254
Open
dmoerner wants to merge 1 commit into
Open
fix(clerk-js): apply the Safari ITP cookie refresh on sign-out#9254dmoerner wants to merge 1 commit into
dmoerner wants to merge 1 commit into
Conversation
Signing out re-issues the client cookie from a fetch, which Safari's ITP caps at 7 days. The client outlives sign-out — persistClient defaults to true, so signOut() removes the sessions and keeps the client — and the client is what Client Trust uses to recognise a known device. Leaving the cap in place meant a user who signed out and returned more than a week later had lost the cookie, arrived as a new client, and was challenged for a second factor. Route the sign-out redirect through /v1/client/touch when the cookie is close to expiring, which is the workaround setActive() already applies after sign-in: touch is reached by a top-level navigation, which ITP does not cap, so it restores the full lifetime. The redirect is returned unchanged when no refresh is needed, so this only adds a hop on Safari — every other browser gets a 400 day cookie and is never eligible. This covers the signOut() paths that navigate to a redirect URL, including <UserButton /> and <SignOutButton />. signOut(callback) navigates on the caller's behalf and is left alone. The existing signOut() test clients gain the isEligibleForTouch and buildTouchUrl members that the real ClientResource always exposes. USER-5769
🦋 Changeset detectedLatest commit: 0141915 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.
|
@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: |
Contributor
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
Contributor
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/clerk-js/src/core/clerk.ts (1)
659-668: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the private-method comment terse and implementation-accurate.
This block restates the implementation and asserts Safari-only behavior, while
isEligibleForTouch()is a browser-agnostic, time-based client contract. Reduce it to a single-line rationale to avoid stale guidance.Suggested simplification
- /** - * Routes the post-sign-out navigation through `/v1/client/touch` when the client - * cookie is close to expiring, and returns `redirectUrl` unchanged otherwise. - * - * In practice this only adds a redirect on Safari. Signing out re-issues the client - * cookie from a fetch, which ITP caps at 7 days; every other browser gets 400 days - * and is never eligible. The cap matters because the client outlives sign-out and is - * what Client Trust uses to recognize a known device, so a user returning a week - * later would be challenged for a second factor. Touch is a top-level navigation, - * which ITP does not cap, so it restores the full lifetime. - */ + // Use top-level touch navigation when the client cookie needs refreshing.As per coding guidelines, comments should explain only non-obvious rationale and remain minimal.
🤖 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/clerk.ts` around lines 659 - 668, Shorten the comment above the private method associated with isEligibleForTouch() to a single-line rationale explaining only why post-sign-out navigation may use the touch endpoint. Remove the detailed cookie, browser-specific, and lifetime claims, while leaving the implementation unchanged.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.
Inline comments:
In `@packages/clerk-js/src/core/__tests__/clerk.test.ts`:
- Around line 1324-1341: The buildTouchUrl mock in the Clerk sign-out test must
URL-encode the nested redirect URL, matching Client.buildTouchUrl(). Update the
mock around buildTouchUrl to encode redirectUrl.toString() before placing it in
the redirect_url query parameter, while preserving the existing navigation
assertions and destination value.
---
Nitpick comments:
In `@packages/clerk-js/src/core/clerk.ts`:
- Around line 659-668: Shorten the comment above the private method associated
with isEligibleForTouch() to a single-line rationale explaining only why
post-sign-out navigation may use the touch endpoint. Remove the detailed cookie,
browser-specific, and lifetime claims, while leaving the implementation
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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fccafcd-212c-473f-9c71-e394d1ded0c0
📒 Files selected for processing (3)
.changeset/safari-itp-sign-out-cookie-refresh.mdpackages/clerk-js/src/core/__tests__/clerk.test.tspackages/clerk-js/src/core/clerk.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual) → reviewed against open PR#20738daniel/user-5769-client-cookie-expires-at-stale-on-sign-outinstead of the default branchclerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)clerk/cli(auto-detected)
Comment on lines
+1324
to
+1341
| buildTouchUrl: ({ redirectUrl }: { redirectUrl: URL }) => | ||
| `https://clerk.example.com/v1/client/touch?redirect_url=${redirectUrl.toString()}`, | ||
| }), | ||
| ); | ||
|
|
||
| const sut = new Clerk(productionPublishableKey); | ||
| sut.navigate = vi.fn(); | ||
| await sut.load(); | ||
| await sut.signOut({ redirectUrl: '/after-sign-out' }); | ||
|
|
||
| await waitFor(() => { | ||
| expect(mockClientRemoveSessions).toHaveBeenCalled(); | ||
| const navigatedTo = new URL((sut.navigate as ReturnType<typeof vi.fn>).mock.calls[0][0]); | ||
| expect(navigatedTo.pathname).toEqual('/v1/client/touch'); | ||
| // The user still lands on the original destination, via the touch redirect. | ||
| expect(navigatedTo.searchParams.get('redirect_url')).toEqual( | ||
| new URL('/after-sign-out', mockWindowLocation.href).toString(), | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Encode nested redirect URLs in the test mock.
The mock interpolates redirectUrl.toString() directly into the query, unlike Client.buildTouchUrl(). A destination containing ? or & would be parsed incorrectly, so this test does not cover the redirect-preservation contract.
Suggested test adjustment
- buildTouchUrl: ({ redirectUrl }: { redirectUrl: URL }) =>
- `https://clerk.example.com/v1/client/touch?redirect_url=${redirectUrl.toString()}`,
+ buildTouchUrl: ({ redirectUrl }: { redirectUrl: URL }) => {
+ const touchUrl = new URL('https://clerk.example.com/v1/client/touch');
+ touchUrl.searchParams.set('redirect_url', redirectUrl.toString());
+ return touchUrl.toString();
+ },
...
- await sut.signOut({ redirectUrl: '/after-sign-out' });
+ await sut.signOut({ redirectUrl: '/after-sign-out?tab=security#settings' });🤖 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 1324 - 1341,
The buildTouchUrl mock in the Clerk sign-out test must URL-encode the nested
redirect URL, matching Client.buildTouchUrl(). Update the mock around
buildTouchUrl to encode redirectUrl.toString() before placing it in the
redirect_url query parameter, while preserving the existing navigation
assertions and destination value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signing out re-issues the client cookie from a fetch, which Safari's ITP caps at 7 days. The client outlives sign-out — persistClient defaults to true, so
signOut()removes the sessions and keeps the client — and the client is what Client Trust uses to recognise a known device. Leaving the cap in place meant a user who signed out and returned more than a week later had lost the cookie, arrived as a new client, and was challenged for a second factor.Route the sign-out redirect through
/v1/client/touchwhen the cookie is close to expiring, which is the workaroundsetActive()already applies after sign-in: touch is reached by a top-level navigation, which ITP does not cap, so it restores the full lifetime. The redirect is returned unchanged when no refresh is needed, so this only adds a hop on Safari — every other browser gets a 400 day cookie and is never eligible.This covers the
signOut()paths that navigate to a redirect URL, including and .signOut(callback)navigates on the caller's behalf and is left alone.Fixes USER-5769.
Depends on https://github.com/clerk/clerk_go/pull/20738.
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change