fix(backend): Use proxy url if available for handshake URL #6119
Conversation
🦋 Changeset detectedLatest commit: 4d04f87 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 ↗︎
|
📝 WalkthroughWalkthroughThe update modifies the handshake URL construction logic in the backend service to use a proxy URL from the authentication context when available, instead of always using the default API hostname. Corresponding test cases are added to verify the correct handling of proxy URLs with and without trailing slashes. Changes
Suggested labels
🪧 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 (
|
|
@brkalow Agreed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/backend/src/tokens/handshake.ts (1)
139-143: Trim all trailing slashes for stronger normalisation
replace(/\/$/, '')removes only a single trailing slash.
If someone accidentally configuresproxyUrlashttps://foo.bar//the double-slash survives and the resulting redirect becomeshttps://foo.bar//v1/client/handshake, which some servers treat as a different path.A slightly broader (and still cheap) normalisation is to drop all trailing slashes:
-const baseUrl = this.authenticateContext.proxyUrl - ? this.authenticateContext.proxyUrl.replace(/\/$/, '') - : `https://${frontendApiNoProtocol}`; +const baseUrl = this.authenticateContext.proxyUrl + ? this.authenticateContext.proxyUrl.replace(/\/+$/, '') // strip *all* trailing slashes + : `https://${frontendApiNoProtocol}`;No functional change for the common case, but it prevents edge-case 404s.
packages/backend/src/tokens/__tests__/handshake.test.ts (1)
180-208: Minor test duplication – consider table-driven testThe two new specs differ only in the input proxyUrl and expectations around the trailing slash. Using
it.each(ortest.each) keeps the intent clear and trims ~20 lines:- it('should use proxy URL when available', () => { … }); - … - it('should handle proxy URL with trailing slash', () => { … }); +it.each([ + ['https://my-proxy.example.com', 'my-proxy.example.com'], + ['https://my-proxy.example.com/', 'my-proxy.example.com'], +])('buildRedirectToHandshake respects proxyUrl %s', (proxyUrl, expectedHost) => { + mockAuthenticateContext.proxyUrl = proxyUrl; + const headers = handshakeService.buildRedirectToHandshake('test-reason'); + const location = headers.get(constants.Headers.Location)!; + const url = new URL(location); + + expect(url.hostname).toBe(expectedHost); + expect(url.pathname).toBe('/v1/client/handshake'); + expect(url.searchParams.get('redirect_url')).toBe('https://example.com/'); + expect(url.searchParams.get(constants.QueryParameters.SuffixedCookies)).toBe('true'); + expect(url.searchParams.get(constants.QueryParameters.HandshakeReason)).toBe('test-reason'); +});Purely a readability / maintenance nicety – feel free to keep as-is if you prefer explicit tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/backend/src/tokens/__tests__/handshake.test.ts(1 hunks)packages/backend/src/tokens/handshake.ts(1 hunks)
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/happy-tools-cry.md (1)
5-5: Consider tightening the summary phrasing.The summary is clear, but you might rephrase to an imperative form and generalize slightly, e.g.:
“Fix handshake URL calculation when a proxy URL is configured.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/happy-tools-cry.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
.changeset/happy-tools-cry.md (1)
1-3: Changeset header is accurate.The patch is correctly targeted at
@clerk/backendand follows the standard changeset format.
@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: |
Description
When Proxy URL is defined, we must use it to determine the handshake URL.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Bug Fixes
Tests