fix(backend): Consider proxyUrl in determining frontendApi URL #6120
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@jacekradko has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 Walkthrough## Walkthrough
This change refactors the handshake URL construction logic to use the `URL` constructor with normalized base URLs, updates token issuer validation to reference the original frontend API, and significantly expands test coverage for handshake URL edge cases and query parameter handling. No exported API signatures are altered.
## Changes
| Files/Groups | Change Summary |
|-------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------|
| packages/backend/src/tokens/handshake.ts | Refactored handshake URL construction to use the `URL` constructor with normalized base URLs. |
| packages/backend/src/tokens/authenticateContext.ts | Added `originalFrontendApi` property; updated token validation to use original frontend API; reordered interface fields. |
| packages/backend/src/tokens/__tests__/handshake.test.ts | Added extensive new tests for handshake URL construction edge cases, query parameters, and environment-specific logic. |
## Possibly related PRs
- clerk/javascript#6119: Introduced initial proxy URL usage in `buildRedirectToHandshake` and added basic proxy URL handling tests, which are directly expanded upon in this PR.
## Suggested reviewers
- wobsoriano
- aeliox
- brkalow✨ Finishing Touches
🪧 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 (
|
@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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/backend/src/tokens/handshake.ts (1)
138-146: Nice defensive URL normalisation – tiny readability nitRe-assigning
baseUrlin-place works, but creating a newconst trimmedBaseUrlwould keep the variable immutable and make the intent clearer:- let baseUrl = this.authenticateContext.frontendApi.startsWith('http') - ? this.authenticateContext.frontendApi - : `https://${this.authenticateContext.frontendApi}`; - - baseUrl = baseUrl.replace(/\/+$/, '') + '/'; + const trimmedBaseUrl = + (this.authenticateContext.frontendApi.startsWith('http') + ? this.authenticateContext.frontendApi + : `https://${this.authenticateContext.frontendApi}`) + .replace(/\/+$/, '') + '/';No functional difference, just marginally easier to scan.
packages/backend/src/tokens/authenticateContext.ts (1)
49-54: Empty-string default fororiginalFrontendApihides bad configurationIf parsing the publishable key were ever to fail (e.g. malformed env var in CI),
originalFrontendApiwould remain the default empty string and silently make every issuer comparison fail.
Consider initialising it asundefinedand asserting shortly afterparsePublishableKeysucceeds, so mis-configurations surface immediately.packages/backend/src/tokens/__tests__/handshake.test.ts (2)
405-408: Remove non-null assertions to satisfy ESLint and keep tests strict-mode safe- expect(() => new URL(location!)).not.toThrow(); - const url = new URL(location!); + expect(location).not.toBeNull(); + const url = new URL(location as string);Same pattern applies further down (lines 587-588).
The explicit runtime check removes the need for the!operator and silences@typescript-eslint/no-non-null-assertion.Also applies to: 582-588
🧰 Tools
🪛 ESLint
[error] 406-406: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
[error] 408-408: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
423-427: Hard-coding API version couples tests to implementation detail
expect(url.searchParams.get('__clerk_api_version')).toBe('2025-04-10');When the backend bumps
SUPPORTED_BAPI_VERSION, this test will fail noisily. Import the constant instead:-import { constants } from '../../constants'; +import { constants, SUPPORTED_BAPI_VERSION } from '../../constants'; … -expect(url.searchParams.get('__clerk_api_version')).toBe('2025-04-10'); +expect(url.searchParams.get('__clerk_api_version')).toBe(SUPPORTED_BAPI_VERSION);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/backend/src/tokens/__tests__/handshake.test.ts(4 hunks)packages/backend/src/tokens/authenticateContext.ts(4 hunks)packages/backend/src/tokens/handshake.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/backend/src/tokens/authenticateContext.ts (1)
packages/shared/src/keys.ts (1)
parsePublishableKey(36-71)
🪛 ESLint
packages/backend/src/tokens/__tests__/handshake.test.ts
[error] 406-406: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
[error] 408-408: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
[error] 587-587: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/backend/src/tokens/authenticateContext.ts (1)
170-180:parsePublishableKeymay still returnnull; guard before dereference
fatal: truecurrently throws on an invalid key, but future refactors could relax that. A minimal guard keeps this method future-proof:- const originalPk = parsePublishableKey(this.publishableKey, { … }); - this.originalFrontendApi = originalPk.frontendApi; + const originalPk = parsePublishableKey(this.publishableKey, { … }); + if (!originalPk) { + throw new Error('Failed to parse publishable key'); + } + this.originalFrontendApi = originalPk.frontendApi;
| const tokenIssuer = data.payload.iss.replace(/https?:\/\//gi, ''); | ||
| return this.frontendApi === tokenIssuer; | ||
| // Use original frontend API for token validation since tokens are issued by the actual Clerk API, not proxy | ||
| return this.originalFrontendApi === tokenIssuer; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Issuer check ignores trailing slashes / “/v1” – could produce false negatives
iss values coming from Clerk tokens usually end with /v1/ (or at least /v1).
Comparing the raw host against originalFrontendApi therefore fails for perfectly valid tokens behind a proxy.
- const tokenIssuer = data.payload.iss.replace(/https?:\/\//gi, '');
- return this.originalFrontendApi === tokenIssuer;
+ const tokenIssuer = data.payload.iss
+ .replace(/https?:\/\//gi, '')
+ .replace(/\/+v1\/?$/, '') // strip “/v1” if present
+ .replace(/\/+$/, ''); // strip trailing slash(es)
+
+ return this.originalFrontendApi.replace(/\/+$/, '') === tokenIssuer;This keeps proxy handling intact while tolerating the canonical Clerk issuer suffix.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const tokenIssuer = data.payload.iss.replace(/https?:\/\//gi, ''); | |
| return this.frontendApi === tokenIssuer; | |
| // Use original frontend API for token validation since tokens are issued by the actual Clerk API, not proxy | |
| return this.originalFrontendApi === tokenIssuer; | |
| } | |
| const tokenIssuer = data.payload.iss | |
| .replace(/https?:\/\//gi, '') | |
| .replace(/\/+v1\/?$/, '') // strip “/v1” if present | |
| .replace(/\/+$/, ''); // strip trailing slash(es) | |
| // Use original frontend API for token validation since tokens are issued by the actual Clerk API, not proxy | |
| return this.originalFrontendApi | |
| .replace(/\/+$/, '') === tokenIssuer; | |
| } |
🤖 Prompt for AI Agents
In packages/backend/src/tokens/authenticateContext.ts around lines 283 to 286,
the issuer check removes the protocol but does not account for trailing slashes
or path segments like "/v1", causing valid tokens to fail validation. Modify the
code to normalize the issuer by removing the protocol and any trailing slashes
or path suffixes such as "/v1" before comparing it to originalFrontendApi,
ensuring the comparison tolerates the canonical Clerk issuer suffix while
preserving proxy handling.
🦋 Changeset detectedLatest commit: d144b7b 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/backend/src/tokens/__tests__/handshake.test.ts (1)
180-210: 🛠️ Refactor suggestionMutating the shared
handshakeServicerisks hidden state leakageSeveral tests (
should use proxy URL when available,handle proxy URL …) mutatemockAuthenticateContextafterhandshakeServicehas been instantiated.
IfHandshakeServicecaches any derived values (e.g. a pre-sanitised base URL) in its constructor, later mutations will not be reflected and these tests may give false positives.A safer pattern is to rebuild the service inside each test that tweaks context-level URL fields:
- mockAuthenticateContext.proxyUrl = 'https://my-proxy.example.com'; - mockAuthenticateContext.frontendApi = 'https://my-proxy.example.com'; - const headers = handshakeService.buildRedirectToHandshake('test-reason'); + const ctx = { ...mockAuthenticateContext, + proxyUrl: 'https://my-proxy.example.com', + frontendApi: 'https://my-proxy.example.com' } as AuthenticateContext; + const service = new HandshakeService(ctx, mockOptions, mockOrganizationMatcher); + const headers = service.buildRedirectToHandshake('test-reason');Repeat for the other proxy-URL variations.
Prevents brittle tests and documents the real construction path.Also applies to: 212-240, 242-268
🧹 Nitpick comments (3)
.changeset/common-beers-read.md (1)
5-5: Clarify changelog wordingConsider switching to the imperative mood and matching the actual casing used in the codebase:
-Add logic to ensure that we consider the proxy_url when creating the frontendApi url. +Consider the `proxyUrl` when constructing the `frontendApi` URL.Keeps the changeset short, action-oriented, and consistent with variable names.
packages/backend/src/tokens/__tests__/handshake.test.ts (2)
548-566: Unit test duplicates production regex rather than exercising public behaviour
trailingSlashTestCasesvalidatesinput.replace(/\/+$/, ''), which is a literal copy of the implementation detail you’re trying to guard. If the production code ever changes (e.g. switches toURLparsing), this test will still pass while the real behaviour breaks.Prefer exercising the public API (
buildRedirectToHandshake) with those inputs, or extract the slash-trimming helper and test that helper directly.
389-416: Reduce duplication with a helper to assert handshake URL invariantsThe
frontendApiFormatsloop re-implements the samelocationretrieval and assertion sequence found in many earlier tests. Extracting a small helper, e.g.assertValidHandshake(headers, reason?), would:
- Eliminate copy-paste.
- Make future changes (e.g. new required query param) a one-liner.
- Shorten the test file by ~40 lines.
Pure readability win, no change in coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/common-beers-read.md(1 hunks)packages/backend/src/tokens/__tests__/handshake.test.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
Description
Adding more comprehensive logic to ensure that we consider the proxy_url when creating the frontendApi url.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Bug Fixes
Tests
Refactor