fix(shared): allow satellite redirect URLs with non-default ports#9059
fix(shared): allow satellite redirect URLs with non-default ports#9059FrancoKaddour wants to merge 4 commits into
Conversation
isAllowedRedirect() compares url.origin against allowedRedirectOrigins patterns. When the redirect URL includes a non-default port (e.g. :5173 from a Vite dev server), url.origin includes the port suffix, while patterns like 'https://*.example.net' have none — causing the match to fail and the redirect to fall back to the home URL silently. Fix: when url.port is non-empty, also test the port-stripped origin (protocol + hostname) against each pattern. Domain validation is preserved — only the port suffix is relaxed. Fixes clerk#8263
🦋 Changeset detectedLatest commit: e25be4d The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 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 |
|
@FrancoKaddour is attempting to deploy a commit to the Clerk Production Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesPort-aware redirect origin matching
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant RedirectURL
participant isAllowedRedirect
participant allowedRedirectOrigins
RedirectURL->>isAllowedRedirect: url with explicit port
isAllowedRedirect->>isAllowedRedirect: reject problematic URLs
isAllowedRedirect->>allowedRedirectOrigins: test url.origin against patterns
allowedRedirectOrigins-->>isAllowedRedirect: no match
isAllowedRedirect->>allowedRedirectOrigins: test portless origin for wildcard strings
allowedRedirectOrigins-->>isAllowedRedirect: match found
isAllowedRedirect-->>RedirectURL: allow redirect
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
|
Good catch. The portless fallback now only applies when the original entry is a wildcard glob (contains *). Exact string entries like https://www.clerk.com remain port-sensitive — the portless test is skipped entirely for them. Updated the tests to reflect this: ['https://www.clerk.com:3000', ['https://www.clerk.com'], false]. |
Exact string entries in allowedRedirectOrigins (e.g. https://www.clerk.com) were also matching ports they never declared because the portless-origin test ran against all patterns. Port is part of the browser origin, so an exact entry must remain port-sensitive. The portless fallback now only applies when the original entry is a glob (contains '*'), which covers the satellite-app use case where the pattern is https://*.example.net and the dev server runs on :5173.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/shared/src/internal/clerk-js/url.ts (1)
426-454: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit return type to this exported curried API.
isAllowedRedirectis exported and not a test helper, so the touched function should not rely on inferred return types.♻️ Proposed typing update
export const isAllowedRedirect = - (allowedRedirectOrigins: Array<string | RegExp> | undefined, currentOrigin: string) => (_url: URL | string) => { + ( + allowedRedirectOrigins: Array<string | RegExp> | undefined, + currentOrigin: string, + ): ((_url: URL | string) => boolean) => + (_url: URL | string): boolean => {As per coding guidelines, “Always define explicit return types for functions, especially public APIs.” Based on learnings, exported functions and public APIs in TypeScript files should have explicit return type annotations.
🤖 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/shared/src/internal/clerk-js/url.ts` around lines 426 - 454, Add an explicit return type to the exported curried API isAllowedRedirect so it does not rely on inference. Update the function signature in the url.ts module to annotate the outer function and, if needed, the returned predicate type, keeping the existing behavior of relativeToAbsoluteUrl, isProblematicUrl, and the origin matching logic unchanged.Sources: Coding guidelines, Learnings
🤖 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.
Nitpick comments:
In `@packages/shared/src/internal/clerk-js/url.ts`:
- Around line 426-454: Add an explicit return type to the exported curried API
isAllowedRedirect so it does not rely on inference. Update the function
signature in the url.ts module to annotate the outer function and, if needed,
the returned predicate type, keeping the existing behavior of
relativeToAbsoluteUrl, isProblematicUrl, and the origin matching logic
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d8f91786-f36b-44f3-be9b-0cb9441f7c45
📒 Files selected for processing (2)
packages/shared/src/internal/clerk-js/__tests__/url.test.tspackages/shared/src/internal/clerk-js/url.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/shared/src/internal/clerk-js/tests/url.test.ts
|
Done. Added explicit return types to both the outer and inner functions of isAllowedRedirect. |
1 participant
Fixes #8263
In local development, satellite apps often run on non-default ports (e.g. Vite on :5173, :5174). When isAllowedRedirect() evaluates the redirect URL, it compares url.origin — which includes the port suffix https://app.example.net:5173) — against patterns like https://*.example.net generated by createAllowedRedirectOrigins. The glob-to-regexp conversion anchors the pattern at the domain end, so the trailing :5173 breaks the match, causing the Account Portal to silently fall back to the home URL instead of honoring the redirect_url.
Fix: when url.port is non-empty, also test the port-stripped origin protocol + hostname) against each pattern. Domain validation is preserved — the fix only relaxes the port suffix comparison, not the domain check.
Summary by CodeRabbit
https://app.example.net:5173) are no longer incorrectly rejected when they match the allowed origin patterns.