Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 0024932 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds exported functions 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
…omains Add automatic FAPI proxying detection for Vercel preview deployments across all SDKs. When an app is deployed to a .vercel.app subdomain without explicit proxy/domain configuration, the SDK automatically routes FAPI requests through the app's own domain via /__clerk proxy path. - Add isVercelPreviewDeploy() helper in @clerk/shared/proxy - Auto-detect in clerk-js proxyUrl getter for client-side SDK initialization - Auto-detect in @clerk/backend authenticateContext for server-side auth - Enable proxy interception in Next.js middleware for /__clerk/* requests on .vercel.app - Add comprehensive tests for all three layers (shared, backend, clerk-js, nextjs) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
brkalow
force-pushed
the
brkalow/auto-proxy-vercel-subdomains
branch
from
f73bc85 to
dbc580e
Compare
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@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: |
brkalow
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/tiny-badgers-smile.md:
- Line 8: Update the changeset description sentence that currently reads "Add
auto-proxy detection for eligible hosts and generalize the internal helper
naming for future providers." to be specific about which hosts are considered
eligible (e.g., Vercel preview deployments) and to clarify the helper change;
for example, change the description to: "Add auto-proxy detection for Vercel
preview deployments (.vercel.app subdomains) and generalize the internal helper
for future hosting providers." Locate and edit the line containing that
description in the changeset (the sentence beginning "Add auto-proxy detection
for eligible hosts...") and replace it with the clearer, user-facing wording.
- Around line 1-6: The changeset incorrectly marks package bumps as 'patch' but
a new public API (shouldAutoProxy) was added; update the entries so any package
that exports the new function (at least '@clerk/shared', and any packages that
re-export it like '@clerk/backend', '@clerk/clerk-js', '@clerk/nextjs' if
applicable) use 'minor' instead of 'patch' in the
.changeset/tiny-badgers-smile.md file; ensure the header lines for those package
entries read 'minor' to reflect the new public API bump while keeping other
metadata unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0cecde51-9659-43c2-8677-37774a033a71
📒 Files selected for processing (1)
.changeset/tiny-badgers-smile.md
| --- | ||
| '@clerk/backend': patch | ||
| '@clerk/clerk-js': patch | ||
| '@clerk/nextjs': patch | ||
| '@clerk/shared': patch | ||
| --- |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if shouldAutoProxy is a public export
# Search for shouldAutoProxy exports in the shared package
rg -n "export.*shouldAutoProxy" packages/shared/Repository: clerk/javascript
Length of output: 152
Update version bumps from patch to minor to reflect the new public API export.
The changeset marks all packages for patch bumps, but the PR introduces a new public API function shouldAutoProxy exported from @clerk/shared. Per semantic versioning standards, new public APIs require minor version bumps, not patch (which is reserved for bug fixes). Update the changeset accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/tiny-badgers-smile.md around lines 1 - 6, The changeset
incorrectly marks package bumps as 'patch' but a new public API
(shouldAutoProxy) was added; update the entries so any package that exports the
new function (at least '@clerk/shared', and any packages that re-export it like
'@clerk/backend', '@clerk/clerk-js', '@clerk/nextjs' if applicable) use 'minor'
instead of 'patch' in the .changeset/tiny-badgers-smile.md file; ensure the
header lines for those package entries read 'minor' to reflect the new public
API bump while keeping other metadata unchanged.
|
!snapshot |
This comment has been minimized.
This comment has been minimized.
|
!snapshot |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared/src/proxy.ts`:
- Around line 45-47: The normalizeHostname function can throw when called with a
malformed URL (e.g., VERCEL_PROJECT_PRODUCTION_URL); wrap the URL parsing in a
try-catch inside normalizeHostname (the function shown using new
URL(...).hostname) and return an empty string on error so config resolution
doesn't crash; ensure callers that rely on normalizeHostname (where
VERCEL_PROJECT_PRODUCTION_URL is passed) can handle an empty string result
gracefully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ad9a329-ea13-4ffe-86f9-f7dec3b163e7
📒 Files selected for processing (5)
.changeset/tiny-badgers-smile.mdpackages/nextjs/src/utils/__tests__/mergeNextClerkPropsWithEnv.test.tspackages/nextjs/src/utils/mergeNextClerkPropsWithEnv.tspackages/shared/src/__tests__/proxy.spec.tspackages/shared/src/proxy.ts
| function normalizeHostname(hostnameOrUrl: string): string { | ||
| if (hostnameOrUrl.startsWith('http://') || hostnameOrUrl.startsWith('https://')) { | ||
| return new URL(hostnameOrUrl).hostname; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify call path and URL constructor throw behavior
rg -n "normalizeHostname|getAutoProxyUrlFromEnvironment|VERCEL_PROJECT_PRODUCTION_URL" \
packages/shared/src/proxy.ts packages/nextjs/src/utils/mergeNextClerkPropsWithEnv.ts
node - <<'NODE'
const samples = ['https://', 'https://?x=1', 'https:///foo', 'https://myapp.vercel.app'];
for (const s of samples) {
try {
console.log(`${s} => hostname="${new URL(s).hostname}"`);
} catch (e) {
console.log(`${s} => THROWS (${e.name})`);
}
}
NODERepository: clerk/javascript
Length of output: 853
🏁 Script executed:
#!/bin/bash
# Get full function context at line 45-47 and surrounding lines
echo "=== normalizeHostname function context (lines 41-55) ==="
sed -n '41,55p' packages/shared/src/proxy.ts | cat -n
echo -e "\n=== getAutoProxyUrlFromEnvironment call context (lines 70-80) ==="
sed -n '70,80p' packages/shared/src/proxy.ts | cat -n
# Search for tests related to proxy.ts or normalizeHostname
echo -e "\n=== Tests for proxy functions ==="
find . -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "normalizeHostname\|getAutoProxyUrlFromEnvironment\|proxy" 2>/dev/null | head -10Repository: clerk/javascript
Length of output: 1707
🏁 Script executed:
#!/bin/bash
# Check if there are tests covering error cases for these functions
echo "=== Searching for test files related to proxy ==="
fd -e "test.ts" -e "spec.ts" | xargs grep -l "normalizeHostname\|getAutoProxyUrlFromEnvironment" 2>/dev/null
# Also check the test directory structure
echo -e "\n=== Checking __tests__ and test directory structure ==="
find packages/shared -type d \( -name "__tests__" -o -name "tests" -o -name "test" \) | head -5Repository: clerk/javascript
Length of output: 410
🏁 Script executed:
#!/bin/bash
# Read the existing proxy test file to see current coverage
echo "=== Existing proxy tests ==="
cat packages/shared/src/__tests__/proxy.spec.tsRepository: clerk/javascript
Length of output: 5211
Handle malformed VERCEL_PROJECT_PRODUCTION_URL in hostname normalization to prevent runtime crashes.
At line 76, normalizeHostname() is called with an environment variable without error handling. If VERCEL_PROJECT_PRODUCTION_URL is malformed (e.g., https:// or https://?x=1), new URL() throws a TypeError, crashing config resolution instead of gracefully returning an empty string. Add try-catch to safely handle this:
Proposed fix
function normalizeHostname(hostnameOrUrl: string): string {
if (hostnameOrUrl.startsWith('http://') || hostnameOrUrl.startsWith('https://')) {
- return new URL(hostnameOrUrl).hostname;
+ try {
+ return new URL(hostnameOrUrl).hostname;
+ } catch {
+ return '';
+ }
}
return hostnameOrUrl.split('/')[0] || '';
}Also applies to: 76-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/src/proxy.ts` around lines 45 - 47, The normalizeHostname
function can throw when called with a malformed URL (e.g.,
VERCEL_PROJECT_PRODUCTION_URL); wrap the URL parsing in a try-catch inside
normalizeHostname (the function shown using new URL(...).hostname) and return an
empty string on error so config resolution doesn't crash; ensure callers that
rely on normalizeHostname (where VERCEL_PROJECT_PRODUCTION_URL is passed) can
handle an empty string result gracefully.
|
!snapshot |
This comment has been minimized.
This comment has been minimized.
# Conflicts: # packages/backend/src/__tests__/proxy.test.ts # packages/backend/src/proxy.ts
Railly
force-pushed
the
brkalow/auto-proxy-vercel-subdomains
branch
from
dc2ff50 to
b89156d
Compare
There was a problem hiding this comment.
NIT: A possible extra test here is to check that it doesn't break when window is not defined (for native envs).
The if (inBrowser()) { already takes care of it, but tests are always nice.
There was a problem hiding this comment.
Addressed in 0859708, also updated existing tests to use production key since auto-proxy is now production-only
| const requestUrl = new URL(request.nextUrl.href); | ||
| const frontendApiProxyConfig = | ||
| resolvedParams.frontendApiProxy ?? | ||
| (resolvedParams.proxyUrl || PROXY_URL || resolvedParams.domain || DOMAIN | ||
| ? undefined | ||
| : getAutoDetectedProxyConfig(requestUrl)); |
There was a problem hiding this comment.
NIT: Works as is but is not the easiest to follow imo. I'd suggest either moving everything to an external resolveFrontendApiProxyConfig(resolvedParams) function, or inlining it all something like this:
| const requestUrl = new URL(request.nextUrl.href); | |
| const frontendApiProxyConfig = | |
| resolvedParams.frontendApiProxy ?? | |
| (resolvedParams.proxyUrl || PROXY_URL || resolvedParams.domain || DOMAIN | |
| ? undefined | |
| : getAutoDetectedProxyConfig(requestUrl)); | |
| // Handle Frontend API proxy requests early, before authentication | |
| let frontendApiProxyConfig = resolvedParams.frontendApiProxy; | |
| // Auto-detect when no explicit proxy or domain is configured | |
| const hasExplicitProxyOrDomain = resolvedParams.proxyUrl || PROXY_URL || resolvedParams.domain || DOMAIN; | |
| if (!frontendApiProxyConfig && !hasExplicitProxyOrDomain) { | |
| const requestUrl = new URL(request.nextUrl.href); | |
| if (shouldAutoProxy(requestUrl.hostname)) { | |
| frontendApiProxyConfig = { enabled: true }; | |
| } | |
| } |
There was a problem hiding this comment.
Adopted your suggestion, also removed the now-unused getAutoDetectedProxyConfig function. Addressed in 635c9b7.
| } | ||
|
|
||
| const AUTO_PROXY_HOST_SUFFIXES = ['.vercel.app']; | ||
| const AUTO_PROXY_PATH = '/__clerk'; |
There was a problem hiding this comment.
I think we should export this and use it in any place we call shouldAutoProxy and currently hardcode /__clerk.
There was a problem hiding this comment.
Exported and replaced the two hardcoded occurrences. Addressed in 96842db
| export function shouldAutoProxy(hostname: string): boolean { | ||
| return AUTO_PROXY_HOST_SUFFIXES.some(hostSuffix => hostname?.endsWith(hostSuffix)) ?? false; | ||
| } |
There was a problem hiding this comment.
The biggest unknown for me with this PR is when should we be auto-proxying exactly? If I understand the PR and the context I've gathered elsewhere, what we are trying to achieve is being able to use production Clerk instances on .vercel.app domains without setting up a custom domain for it right?
People might have already have working development instances set up on .vercel.app domains today (I think?), and it feels like we wouldn't want to proxy those?
One reason is, if we deploy clerk-js with auto-proxying when people have not updated their other SDK packages will likely break things? Since dev instances already work, changing from not proxying to proxying in a minor/patch also just seems off to me.
However, this function is not guarding for only auto-proxying production Clerk instances, and it is used in a few places that adds proxy config when this returns true. I'm having a hard time judging if that's intentional or not?
If it is intentional, I'm also not sure if the possible build time (getAutoProxyUrlFromEnvironment) / run time (shouldAutoProxy) mismatch can turn out to be a problem? 🤔 (The proxyUrl that could get passed to ClerkProvider at build time might mismatch with what clerk-js decides at run time as conditions differ.)
There was a problem hiding this comment.
Great catch 😄, we should auto-proxy for production-only apps that have the *.vercel.app associated.
Fixed by guarding with instanceType === 'production' in clerk-js and pk_live_ prefix check in the backend authenticateContext. This aligns with getAutoProxyUrlFromEnvironment which already had the production guard.
Here's the full integration overview for context: v0 + Clerk Integration Status
…function-based frontendApiProxy
| it('auto-derives proxyUrl for eligible hostnames', async () => { | ||
| const clerkRequest = createClerkRequest(new Request('https://myapp-abc123.vercel.app/dashboard')); | ||
| const context = await createAuthenticateContext(clerkRequest, { | ||
| publishableKey: pkTest, |
There was a problem hiding this comment.
The auto-proxy tests here still use pkTest but the guard on line 75 checks for pk_live_. This positive case cant pass at runtime, it only shows green because vitest runs in typecheck mode. Should swap to pkLive and add a negative case for dev keys.
| ) { | ||
| // Auto-detect proxy for supported platform deployments (production only) | ||
| if (!options.proxyUrl && !options.domain && options.publishableKey?.startsWith('pk_live_')) { | ||
| const hostname = clerkRequest.clerkUrl.hostname; |
There was a problem hiding this comment.
Just double checking - the hostname here comes from clerkUrl which trusts x-forwarded-host. On Vercel thats fine since the edge sets it, but outside Vercel a misconfigured reverse proxy could let someone spoof X-Forwarded-Host: anything.vercel.app and flip auto-proxy on. Im just being paranoid, but could this be exploitable outside Vercel? I don't think it matters, just raising for the sake of completeness :)
There was a problem hiding this comment.
Good callout. The hostname is derived from X-Forwarded-Host which is authoritative on Vercel's edge but spoofable behind misconfigured proxies. In practice, auto-proxy also requires a pk_live_* key and no explicit proxy/domain config. The spoofed value only affects frontendApi derivation (cookie suffix, cross-origin checks), JWKS fetches use apiUrl (BAPI) not frontendApi, so no outbound requests to the spoofed host. Worst case is a self-DoS where cookie suffix mismatches. Added a comment documenting the trust assumption.
|
|
||
| function normalizeHostname(hostnameOrUrl: string): string { | ||
| if (hostnameOrUrl.startsWith('http://') || hostnameOrUrl.startsWith('https://')) { | ||
| return new URL(hostnameOrUrl).hostname; |
There was a problem hiding this comment.
normalizeHostname can throw on malformed VERCEL_PROJECT_PRODUCTION_URL. To be extra safe, wrap in try/catch and return ''
| options: AuthenticateRequestOptions, | ||
| ) { | ||
| // Auto-detect proxy for supported platform deployments (production only) | ||
| if (!options.proxyUrl && !options.domain && options.publishableKey?.startsWith('pk_live_')) { |
There was a problem hiding this comment.
Could use isProductionFromPublishableKey from @clerk/shared/keys here instead of raw startsWith('pk_live_') for consistency
…rcel-subdomains # Conflicts: # packages/nextjs/src/server/clerkMiddleware.ts
…re keyless-provider formatting
5 participants
Description
This PR implements automatic FAPI proxying for Vercel preview deployments. When an app is deployed to a
.vercel.appsubdomain without explicit proxy or domain configuration, the SDK automatically routes FAPI requests through the app's own domain via the/__clerkproxy path.The auto-detection is centralized across all SDKs:
@clerk/shared/proxy): NewisVercelPreviewDeploy()helper detects.vercel.apphostnames@clerk/clerk-js): Auto-detect inproxyUrlgetter whenwindow.location.hostnameis.vercel.app@clerk/backend): Auto-detect inauthenticateContextconstructor when request hostname is.vercel.app@clerk/nextjs): Auto-enable proxy interception for/__clerk/*requests on.vercel.apphostnamesGuard conditions prevent auto-detection when explicit configuration is provided:
proxyUrl,domain, or environment variablesNEXT_PUBLIC_CLERK_PROXY_URL/NEXT_PUBLIC_CLERK_DOMAIN.Test Results
@clerk/shared: 981/981 tests passed@clerk/backend: 1114/1114 tests passed@clerk/clerk-js: 624/624 tests passed@clerk/nextjs: 362 tests passed (49 pre-existing failures unrelated to this change)Build verification: All packages build cleanly with no type errors.
Checklist
pnpm testruns as expectedpnpm buildruns as expectedType of change
Summary by CodeRabbit
New Features
Bug Fixes
Tests