chore(backend,nextjs): Improve token handling and optimize verification #6123
Conversation
🦋 Changeset detectedLatest commit: afbb6d9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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 Git ↗︎
|
📝 Walkthrough## Walkthrough
This change refines token type handling across authentication logic and tests. It introduces explicit handling for invalid or unaccepted token types, updates type signatures to allow `null` token types, and standardizes unauthenticated state representation. Related tests and helper functions are updated for consistent behavior and to improve test coverage for multiple token scenarios.
## Changes
| Files / Areas | Change Summary |
|--------------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `packages/backend/src/tokens/authObjects.ts`, `authStatus.ts`, `request.ts` | Refactored token acceptance logic, updated types to allow `null` token type, added explicit handling for invalid tokens, and updated exported/public entity signatures. Introduced early return for invalid tokens in array-acceptance cases. |
| `packages/backend/src/tokens/__tests__/request.test-d.ts`, `request.test.ts` | Updated test assertions and expectations to reflect new type signatures and unauthenticated state handling, including the addition of `null` as a possible token type. |
| `packages/nextjs/src/server/data/getAuthDataFromRequest.ts` | Refactored async authentication logic to streamline token type checks, removed helpers, and unified handling of accepted token types and invalid tokens. Used new utility for consistent auth object wrapping. |
| `packages/nextjs/src/server/protect.ts` | Simplified unauthorized access checks by relying on `isAuthenticated` instead of token ID or null token type. |
| `packages/nextjs/src/server/__tests__/clerkMiddleware.test.ts`, `getAuthDataFromRequest.test.ts` | Standardized token strings, expanded test coverage for multiple token types, and added explicit assertions for authentication state and error handling. |
## Possibly related PRs
- clerk/javascript#6112: Refactors `getAuthObjectForAcceptedToken`, introduces `InvalidTokenAuthObject`, and updates related types and tests for invalid token and unauthenticated state handling.
- clerk/javascript#6099: Enhances machine authentication object properties and token type handling, related by focus on machine auth structure and token type management.
## Suggested reviewers
- aeliox
- tmilewski📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
packages/backend/src/tokens/__tests__/request.test.ts (1)
1240-1245: RedundantisAuthenticatedin expectations
toBeMachineUnauthenticatedToAuthignores all properties excepttokenType.
AddingisAuthenticated: falseis harmless but misleading. Consider removing it to keep the test intent clear.Also applies to: 1290-1295, 1320-1325
packages/backend/src/tokens/authStatus.ts (1)
32-41: Conditional type is clever but hard to scan
ToAuth<T, Authenticated>now handles four cases in one expression.
For maintainability, consider splitting into smaller helper types (e.g.ToAuthNull,ToAuthSession,ToAuthMachine) or at least adding an inline comment that explains each branch.packages/nextjs/src/server/__tests__/clerkMiddleware.test.ts (1)
480-489: Nit: test token IDs are now tiny – consider clarifying commentsSwitching to the shorter IDs (
ak_123,oat_123, …) is fine, but it’s easy to lose track of which string represents which token type. A short inline comment next to each header would improve readability.Example:
[constants.Headers.Authorization]: 'Bearer ak_123', // API keyAlso applies to: 528-537, 556-566, 662-670, 685-694, 708-717
packages/backend/src/tokens/request.ts (1)
91-106: Duplicate logic & unnecessary fallback
isTokenTypeInAcceptedArrayre-implementsisTokenTypeAcceptedwith extra fallback tosession_token.
You can:
- Drop the helper altogether and call
isTokenTypeAccepted(parsedTokenType ?? TokenType.SessionToken, acceptsToken)directly.- Or keep the helper but at least rename it to indicate the fallback behaviour to avoid surprises for future maintainers.
This will reduce duplication and potential drift.
packages/nextjs/src/server/__tests__/getAuthDataFromRequest.test.ts (2)
42-49: RedundantbeforeEachThere’s a top-level
beforeEachat lines 42-45 and another one right below at 47-49 doing the exact samevi.clearAllMocks()call.
Remove one of them to avoid confusion.
67-67: Typo in test description
auth obkect→auth object.packages/nextjs/src/server/data/getAuthDataFromRequest.ts (1)
123-129: Potential double work for non-session bearer tokensIf a random bearer token is present and
acceptsTokenexcludessession_token, you correctly returninvalidTokenAuthObject.
However,isMachineTokenByPrefixalready filtered machine tokens above.
For other random tokens this branch still executes after the verification attempt.
Re-order the checks so that this inexpensive validation happens before any verification calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/backend/src/tokens/__tests__/request.test-d.ts(1 hunks)packages/backend/src/tokens/__tests__/request.test.ts(8 hunks)packages/backend/src/tokens/authObjects.ts(1 hunks)packages/backend/src/tokens/authStatus.ts(5 hunks)packages/backend/src/tokens/request.ts(5 hunks)packages/nextjs/src/server/__tests__/clerkMiddleware.test.ts(6 hunks)packages/nextjs/src/server/__tests__/getAuthDataFromRequest.test.ts(7 hunks)packages/nextjs/src/server/data/getAuthDataFromRequest.ts(3 hunks)packages/nextjs/src/server/protect.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/nextjs/src/server/__tests__/clerkMiddleware.test.ts (3)
packages/backend/src/internal.ts (3)
constants(1-1)AuthStatus(39-39)TokenType(18-18)packages/backend/src/tokens/authStatus.ts (2)
AuthStatus(24-28)AuthStatus(30-30)packages/backend/src/tokens/tokenTypes.ts (2)
TokenType(1-6)TokenType(11-11)
packages/backend/src/tokens/__tests__/request.test-d.ts (1)
packages/backend/src/tokens/authStatus.ts (1)
RequestState(127-130)
packages/backend/src/tokens/__tests__/request.test.ts (2)
packages/backend/src/tokens/tokenTypes.ts (2)
TokenType(1-6)TokenType(11-11)packages/backend/src/tokens/authStatus.ts (2)
AuthErrorReason(105-121)AuthErrorReason(123-123)
packages/nextjs/src/server/__tests__/getAuthDataFromRequest.test.ts (4)
packages/backend/src/internal.ts (3)
verifyMachineAuthToken(55-55)AuthenticatedMachineObject(25-25)constants(1-1)packages/backend/src/tokens/verify.ts (1)
verifyMachineAuthToken(248-260)packages/nextjs/src/server/data/getAuthDataFromRequest.ts (1)
getAuthDataFromRequestAsync(87-130)packages/backend/src/tokens/authObjects.ts (1)
AuthenticatedMachineObject(117-128)
packages/backend/src/tokens/authObjects.ts (3)
packages/backend/src/tokens/tokenTypes.ts (2)
TokenType(1-6)TokenType(11-11)packages/backend/src/tokens/types.ts (1)
AuthenticateRequestOptions(19-70)packages/backend/src/tokens/machine.ts (2)
isTokenTypeAccepted(59-73)isMachineTokenType(81-83)
packages/nextjs/src/server/data/getAuthDataFromRequest.ts (2)
packages/backend/src/internal.ts (8)
isMachineTokenByPrefix(57-57)getMachineTokenType(57-57)invalidTokenAuthObject(34-34)verifyMachineAuthToken(55-55)unauthenticatedMachineObject(33-33)authenticatedMachineObject(32-32)getAuthObjectForAcceptedToken(36-36)TokenType(18-18)packages/backend/src/tokens/tokenTypes.ts (2)
TokenType(1-6)TokenType(11-11)
🔇 Additional comments (2)
packages/backend/src/tokens/__tests__/request.test-d.ts (1)
27-30: Type assertion update is correctIncluding
nullin the expected union keeps the declaration test in sync with the newRequestStatedefinition. Looks good.packages/nextjs/src/server/protect.ts (1)
148-153: Double-check thatisAuthenticatedis always present on machine-token auth objectsThe old guard relied on
id; the new one relies onisAuthenticated.
If any code path creates anAuthenticatedMachineObjectthat omitsisAuthenticated, users will now be rejected. Please verify that:
authenticatedMachineObjectin@clerk/backendalways setsisAuthenticated: true.- All mocks in unit tests include that flag (most already updated).
- Third-party integrators that may construct custom
AuthObjects won’t break.If any of these aren’t guaranteed, add a fallback (
authObject.id ?? authObject.isAuthenticated) or ensure the object factory always populates the field.
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/twenty-beds-serve.md (1)
6-7: Fix typo and improve description clarity
Please correctauthenticateRequyest()toauthenticateRequest(). For readers’ context, you may also mention that this logic is implemented via thesignedOutInvalidToken()helper andgetAuthObjectForAcceptedTokenfunction.- Add handling for invalid token types when `acceptsToken` is an array in `authenticateRequyest()`: now returns a clear unauthenticated state (`tokenType: null`) if the token is not in the accepted list. + Add handling for invalid token types when `acceptsToken` is an array in `authenticateRequest()`: now returns a clear unauthenticated state (`tokenType: null`) if the token is not in the accepted list (via `signedOutInvalidToken()` / `getAuthObjectForAcceptedToken`).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/twenty-beds-serve.md(1 hunks)packages/nextjs/src/server/__tests__/getAuthDataFromRequest.test.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nextjs/src/server/tests/getAuthDataFromRequest.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (24)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 13)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (tanstack-react-router, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (quickstart, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Static analysis
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Unit Tests (18, --filter=@clerk/astro --filter=@clerk/backend --filter=@clerk/express --filter=@c...
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
.changeset/twenty-beds-serve.md (1)
1-4: Validate changeset header formatting
The YAML frontmatter correctly lists both packages with aminorversion bump, following the Changeset specification.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/nextjs/src/server/__tests__/getAuthDataFromRequest.test.ts (4)
23-30: PreferHeadersInitoveranyand simplify URL handling
headers?: anyonMockRequestParamsweakens type-safety. UseHeadersInitso callers can pass eitherHeadersor a plain record.
Also,new URL(url, 'https://www.clerk.com').toString()can be shortened:NextRequestaccepts aURL, so the extra.toString()isn’t needed.-type MockRequestParams = { +type MockRequestParams = { url: string; appendDevBrowserCookie?: boolean; method?: string; - headers?: any; + headers?: HeadersInit; }; … -return new NextRequest(new URL(url, 'https://www.clerk.com').toString(), { method, headers: headersWithCookie }); +return new NextRequest(new URL(url, 'https://www.clerk.com'), { method, headers: headersWithCookie });
32-40: Tighten the mock error’s typing
machineTokenErrorMockis currentlyany[]. Addas const(or an explicit type) so accidental mutation is prevented and IntelliSense retains literal values.-const machineTokenErrorMock = [ +const machineTokenErrorMock = [ { message: 'Token type mismatch', code: 'token-invalid', status: 401, name: 'MachineTokenVerificationError', getFullMessage: () => 'Token type mismatch', - }, -]; + }, +] as const;
94-96: Avoid hard-coding the generic in the castIf the implementation ever changes to return another machine-compatible token type, the cast will silently lie.
Usetypeof auth.tokenType(or keep the cast but reference the variable) to stay future-proof.-expect((auth as AuthenticatedMachineObject<'api_key'>).id).toBe('ak_id123'); +expect( + (auth as AuthenticatedMachineObject<typeof auth.tokenType>).id, +).toBe('ak_id123');
196-208: Assert that machine-token verification is skipped for session tokensThis test exercises the session-token path. Verifying that the machine-token verifier is not called clarifies intent and protects against accidental routing changes.
expect((auth as SignedOutAuthObject).userId).toBeNull(); expect(auth.isAuthenticated).toBe(false); +expect(verifyMachineAuthToken).not.toHaveBeenCalled();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/backend/src/tokens/request.ts(5 hunks)packages/nextjs/src/server/__tests__/getAuthDataFromRequest.test.ts(7 hunks)packages/nextjs/src/server/data/getAuthDataFromRequest.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/backend/src/tokens/request.ts
- packages/nextjs/src/server/data/getAuthDataFromRequest.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/nextjs/src/server/__tests__/getAuthDataFromRequest.test.ts (1)
47-61: Validate the short-circuit by assertingverifyMachineAuthTokenis untouchedThe essence of this test is the early exit when the incoming token type is not accepted.
Assert that no verification occurred; it both documents intent and guards against regressions.expect(auth.tokenType).toBeNull(); expect(auth.isAuthenticated).toBe(false); +expect(verifyMachineAuthToken).not.toHaveBeenCalled();
wobsoriano
changed the title
| bearerToken, | ||
| tokenType: tokenType as MachineTokenType, | ||
| acceptsToken, | ||
| options, |
There was a problem hiding this comment.
this whole part, is now baked into @clerk/backend, right ?
Description
This pull request builds upon the previous PR #6112 , where we introduced the new
nulltoken type in an auth object for tokens not included in theacceptsTokenarray. In that PR, the changes were limited to returning a new invalid token type inauth(),getAuth()function calls.In this follow-up PR, we:
authenticateRequest()call. We have atokenTypeproperty in theauthenticateRequest()call state as well, so it just makes sense to includenullresult in here as well.acceptsTokenarray.Example:
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit