fix(backend): reject machine JWTs presented as session tokens#9168
Open
dominic-clerk wants to merge 1 commit into
Open
fix(backend): reject machine JWTs presented as session tokens#9168dominic-clerk wants to merge 1 commit into
dominic-clerk wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: b66f063 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughMachine-token JWTs are rejected when supplied as session cookies or verified as session tokens. Cookie authentication returns a signed-out state, token verification fails before key resolution, and tests plus a patch changeset document the behavior. ChangesSession token security
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Request
participant CookieAuthentication
participant TokenVerification
Request->>CookieAuthentication: Supply machine JWT in __session
CookieAuthentication->>CookieAuthentication: Detect machine JWT
CookieAuthentication-->>Request: Return signedOut with TokenTypeMismatch
TokenVerification->>TokenVerification: Reject M2M category before key resolution
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
The cookie path (authenticateRequestWithTokenInCookie) lacked the isMachineJwt() guard the header path has, so a same-instance M2M JWT in the __session cookie passed verifyToken() and produced a signed-in state with userId = "mch_...". - Guard the cookie path with isMachineJwt(), mirroring the header path - verifyToken() now also rejects any JWT tagged with a non-session token category (cat), closing the confusion regardless of transport - Add regression tests for both paths
dominic-clerk
force-pushed
the
dc-machine-jwt
branch
from
c2c897f to
b66f063
Compare
dominic-clerk
changed the title
@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: |
![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/backend/src/tokens/__tests__/request.test.ts (1)
1283-1307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the OAuth cookie branch as well.
This regression test covers only M2M JWTs, while the production guard handles both OAuth and M2M tokens. Add or confirm a cookie case for an OAuth JWT asserting the same signed-out
TokenTypeMismatchresult.As per coding guidelines, unit tests are required for all new functionality and should cover 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/backend/src/tokens/__tests__/request.test.ts` around lines 1283 - 1307, The cookieToken regression coverage only exercises the M2M JWT path; extend the request tests around the existing cookieToken test to present an OAuth JWT in the __session cookie and assert the same signed-out result with AuthErrorReason.TokenTypeMismatch, including the existing toAuth() expectation. Reuse the established OAuth JWT fixture or construction helpers and preserve the current M2M coverage.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/backend/src/tokens/request.ts`:
- Around line 634-643: Move the isMachineJwt check in the cookie authentication
flow before the handleMaybeHandshakeStatus branches, so machine JWTs in
__session return the TokenTypeMismatch signedOut result even when __client_uat
is absent. Add a regression case covering a machine JWT without __client_uat and
verify the expected signed-out response.
---
Nitpick comments:
In `@packages/backend/src/tokens/__tests__/request.test.ts`:
- Around line 1283-1307: The cookieToken regression coverage only exercises the
M2M JWT path; extend the request tests around the existing cookieToken test to
present an OAuth JWT in the __session cookie and assert the same signed-out
result with AuthErrorReason.TokenTypeMismatch, including the existing toAuth()
expectation. Reuse the established OAuth JWT fixture or construction helpers and
preserve the current M2M coverage.
🪄 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: 063de93e-c94c-496a-9c69-5ab6fabc5524
📒 Files selected for processing (5)
.changeset/shiny-words-lay.mdpackages/backend/src/tokens/__tests__/request.test.tspackages/backend/src/tokens/__tests__/verify.test.tspackages/backend/src/tokens/request.tspackages/backend/src/tokens/verify.ts
Comment on lines
+634
to
+643
| // Machine JWTs pass verifyToken() but must not be accepted as session tokens (mirrors header path). | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| if (isMachineJwt(authenticateContext.sessionTokenInCookie!)) { | ||
| return signedOut({ | ||
| tokenType: TokenType.SessionToken, | ||
| authenticateContext, | ||
| reason: AuthErrorReason.TokenTypeMismatch, | ||
| message: '', | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and surrounding symbols first.
ast-grep outline packages/backend/src/tokens/request.ts --view expanded || true
echo
echo "---- relevant line ranges ----"
nl -ba packages/backend/src/tokens/request.ts | sed -n '560,700p'
echo
echo "---- search for handshake helpers / machine jwt checks ----"
rg -n "isMachineJwt|handleMaybeHandshakeStatus|TokenTypeMismatch|__client_uat|sessionTokenInCookie|signedOut" packages/backend/src/tokens/request.tsRepository: clerk/javascript
Length of output: 1189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- packages/backend/src/tokens/request.ts (lines 600-660) ----"
sed -n '600,660p' packages/backend/src/tokens/request.ts
echo
echo "---- surrounding references ----"
rg -n "handleMaybeHandshakeStatus|isMachineJwt|sessionTokenInCookie|__client_uat|TokenTypeMismatch" packages/backend/src/tokens/request.tsRepository: clerk/javascript
Length of output: 4888
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- state derivation around hasActiveClient / client UAT / session token ----"
sed -n '440,625p' packages/backend/src/tokens/request.ts
echo
echo "---- machine helpers ----"
sed -n '1,220p' packages/backend/src/tokens/machine.tsRepository: clerk/javascript
Length of output: 14777
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- handleMaybeHandshakeStatus definition ----"
sed -n '300,380p' packages/backend/src/tokens/request.ts
echo
echo "---- call sites of handleMaybeHandshakeStatus in request.ts ----"
rg -n "function handleMaybeHandshakeStatus|return handleMaybeHandshakeStatus|handleMaybeHandshakeStatus\(" packages/backend/src/tokens/request.tsRepository: clerk/javascript
Length of output: 4982
Move the machine-JWT check before the cookie handshake branches. A machine JWT in __session without __client_uat hits handleMaybeHandshakeStatus first, so it never returns the intended TokenTypeMismatch signed-out state. Add a regression case for the no-__client_uat path.
🤖 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/request.ts` around lines 634 - 643, Move the
isMachineJwt check in the cookie authentication flow before the
handleMaybeHandshakeStatus branches, so machine JWTs in __session return the
TokenTypeMismatch signedOut result even when __client_uat is absent. Add a
regression case covering a machine JWT without __client_uat and verify the
expected signed-out response.
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 |
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.
Description
The cookie path (authenticateRequestWithTokenInCookie) lacked the isMachineJwt() guard the header path has, so a same-instance M2M JWT in the __session cookie passed verifyToken() and produced a signed-in state with userId = "mch_...".
Fixes SDK-107
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit