fix(backend): Fix cross-origin handshake bypass#9145
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Fix a cross-origin handshake bypass where `isKnownClerkReferrer()` trusted overly broad referrer hosts as Clerk-owned: any `accounts.*` host (e.g. `accounts.attacker.com`), plus dev account-portal domains (`*.accounts.dev` and legacy suffixes) on production instances. These let unrelated origins skip the handshake and its session-freshness check. The referrer is now trusted only for the accounts portal derived from the instance's frontend API, plus dev account-portal domains on non-production instances.
db0893d to
67c3fdc
Compare
dominic-clerk
changed the title
🦋 Changeset detectedLatest commit: 0cd1824 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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 |
@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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe backend now restricts trusted Clerk referrers to instance-derived accounts portals, permits development portals only for non-production instances, and expands cross-origin handshake tests for unrelated and production referrers. Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
| }); | ||
| }); | ||
|
|
||
| test('does not trigger handshake when referer is from production accounts portal', async () => { |
There was a problem hiding this comment.
We revert the logic of this test because it tested that example.com's account portal wouldn't trigger a handshake on another (primary.com's) instance.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/backend/src/tokens/__tests__/request.test.ts (1)
2091-2139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWeak assertion in the new "does not trigger handshake" tests.
Both new tests only assert
requestState.reasonis notPrimaryDomainCrossOriginSync. This doesn't confirm the request actually reached a signed-in state — a regression producing some other error/reason would still pass. Consider asserting the actual expected signed-in shape (e.g.status/isSignedIn), similar to how the sibling "triggers" tests use the fulltoMatchHandshakematcher.As per coding guidelines: "Unit tests are required for all new functionality... Verify proper error handling and edge cases."
✅ Example strengthened assertion
- expect(requestState.reason).not.toBe(AuthErrorReason.PrimaryDomainCrossOriginSync); + expect(requestState.reason).not.toBe(AuthErrorReason.PrimaryDomainCrossOriginSync); + expect(requestState.status).toBe(AuthStatus.SignedIn);🤖 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/backend/src/tokens/__tests__/request.test.ts` around lines 2091 - 2139, Strengthen both dev accounts portal tests around authenticateRequest by asserting the complete expected signed-in request state, using the existing toMatchHandshake matcher or the established status/isSignedIn assertions from sibling tests. Keep the checks for both current and legacy referer formats, and ensure each test verifies successful authentication rather than only excluding PrimaryDomainCrossOriginSync.Source: Coding guidelines
packages/backend/src/tokens/authenticateContext.ts (1)
218-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winJSDoc no longer matches the tightened trust behavior.
The doc comment still says referrer trust "includes both development and production account portal domains" unconditionally, but the implementation below now only trusts dev account-portal domains when
instanceType !== 'production', and no longer trusts anyaccounts.*host. If this method's JSDoc is surfaced in generated reference docs, it should be updated to reflect the new production/non-production distinction; ifAuthenticateContextis purely internal, this may not apply — could you confirm which is the case?As per path instructions: "If a PR adds or changes public/reference-facing API surface area, check whether the corresponding JSDoc is present, accurate, and aligned with the implementation... When unsure whether a symbol is public/reference-facing, ask for clarification instead of asserting that documentation is required."
🤖 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/backend/src/tokens/authenticateContext.ts` around lines 218 - 224, Confirm whether the referrer-validation method on AuthenticateContext is public/reference-facing; if so, update its JSDoc to state that only non-production instances trust development account-portal domains, production instances do not trust them, and FAPI domains remain supported. If AuthenticateContext is internal, leave the documentation unchanged.Source: Path instructions
🤖 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/backend/src/tokens/__tests__/request.test.ts`:
- Around line 2091-2139: Strengthen both dev accounts portal tests around
authenticateRequest by asserting the complete expected signed-in request state,
using the existing toMatchHandshake matcher or the established status/isSignedIn
assertions from sibling tests. Keep the checks for both current and legacy
referer formats, and ensure each test verifies successful authentication rather
than only excluding PrimaryDomainCrossOriginSync.
In `@packages/backend/src/tokens/authenticateContext.ts`:
- Around line 218-224: Confirm whether the referrer-validation method on
AuthenticateContext is public/reference-facing; if so, update its JSDoc to state
that only non-production instances trust development account-portal domains,
production instances do not trust them, and FAPI domains remain supported. If
AuthenticateContext is internal, leave the documentation unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 3656d6fa-398a-4d3b-9669-6c3884df8c9c
📒 Files selected for processing (3)
.changeset/khaki-chairs-kiss.mdpackages/backend/src/tokens/__tests__/request.test.tspackages/backend/src/tokens/authenticateContext.ts
…s-via-overly-broad-accounts
wobsoriano
left a comment
wobsoriano
left a comment
There was a problem hiding this comment.
Had to rebase because break check somehow reports a breaking change, even when there's none
👍🏼
2 participants
Description
Fix a cross-origin handshake bypass where
isKnownClerkReferrer()trusted overly broad referrer hosts as Clerk-owned: anyaccounts.*host (e.g.accounts.attacker.com), plus dev account-portal domains (*.accounts.devand legacy suffixes) on production instances. These let unrelated origins skip the handshake and its session-freshness check. The referrer is now trusted only for the accounts portal derived from the instance's frontend API, plus dev account-portal domains on non-production instances.Fixes SDK-143
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change