chore(js,ui,shared): Correctly display OAuth consent redirect root domains#8700
chore(js,ui,shared): Correctly display OAuth consent redirect root domains#8700wobsoriano wants to merge 11 commits into
Conversation
…omains for known multi-label public suffixes
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 5254d8c 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 |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@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: |
…-suffix-list-to-extract-root
…-suffix-list-to-extract-root
wobsoriano
changed the title
API Changes Report
Summary
@clerk/sharedCurrent version: 4.14.0 Subpath
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR threads an optional redirectUri through the OAuth consent flow and surfaces a backend-resolved redirectDomain. Changes include type updates (adding redirectUri/redirectDomain), OAuthApplication.getConsentInfo accepting and sending redirect_uri and returning redirectDomain, propagating redirectUri into useOAuthConsent cache keys and fetch logic, deriving/passing redirectUri from the OAuthConsent component and preferring data.redirectDomain for display, and test updates plus a changeset releasing the fix. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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 (1)
packages/ui/src/components/OAuthConsent/__tests__/OAuthConsent.test.tsx (1)
185-210: ⚡ Quick winAdd a regression test for
redirectUrloverride precedence.Please add one public-path override test that sets
redirectUrlvia context props and assertsgetConsentInforeceives that value asredirectUri. This protects the redirect-domain trust text path from source mismatch regressions.As per coding guidelines:
**/*.{test,spec}.{ts,tsx}: "Unit tests are required for all new functionality" and "Verify proper error handling and edge cases."🤖 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/ui/src/components/OAuthConsent/__tests__/OAuthConsent.test.tsx` around lines 185 - 210, Add a regression test in the existing OAuthConsent.test.tsx case that verifies public-path override precedence for redirectUrl: reuse createFixtures/props.setProps (the same pattern used for oauthClientId and scope overrides), set props.setProps({ componentName: 'OAuthConsent', redirectUrl: 'https://override.example/callback', oauthClientId: 'override_id', scope: 'openid email' }) and then mock getConsentInfo via mockOAuthApplication(fixtures.clerk, { getConsentInfo }) and assert that getConsentInfo was called with redirectUri: 'https://override.example/callback' (i.e., expect(getConsentInfo).toHaveBeenCalledWith({ ..., redirectUri: 'https://override.example/callback' })), ensuring the test mirrors the existing override test structure.
🤖 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.
Inline comments:
In `@packages/ui/src/components/OAuthConsent/OAuthConsent.tsx`:
- Around line 53-57: The code uses getRedirectUriFromSearch() for the consent
fetch (passed into useOAuthConsent as redirectUri) but allows a different
redirectUrl to be shown in the UI, causing a mismatch; compute a single
resolvedRedirectUri once (e.g., const resolvedRedirectUri =
contextProvidedRedirectUrl ?? getRedirectUriFromSearch() ?? undefined) and use
that same resolvedRedirectUri when calling useOAuthConsent and when
rendering/displaying the redirect URL so both backend fetch and UI display use
the exact same value; update references to redirectUri/redirectUrl in
OAuthConsent (including the call site of useOAuthConsent and the display logic)
to use resolvedRedirectUri.
---
Nitpick comments:
In `@packages/ui/src/components/OAuthConsent/__tests__/OAuthConsent.test.tsx`:
- Around line 185-210: Add a regression test in the existing
OAuthConsent.test.tsx case that verifies public-path override precedence for
redirectUrl: reuse createFixtures/props.setProps (the same pattern used for
oauthClientId and scope overrides), set props.setProps({ componentName:
'OAuthConsent', redirectUrl: 'https://override.example/callback', oauthClientId:
'override_id', scope: 'openid email' }) and then mock getConsentInfo via
mockOAuthApplication(fixtures.clerk, { getConsentInfo }) and assert that
getConsentInfo was called with redirectUri: 'https://override.example/callback'
(i.e., expect(getConsentInfo).toHaveBeenCalledWith({ ..., redirectUri:
'https://override.example/callback' })), ensuring the test mirrors the existing
override test structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 61fb867e-80a7-4bfa-907a-c08e6c73b422
📒 Files selected for processing (8)
.changeset/quiet-terms-drum.mdpackages/clerk-js/src/core/modules/oauthApplication/index.tspackages/shared/src/react/hooks/useOAuthConsent.shared.tspackages/shared/src/react/hooks/useOAuthConsent.tsxpackages/shared/src/react/hooks/useOAuthConsent.types.tspackages/shared/src/types/oauthApplication.tspackages/ui/src/components/OAuthConsent/OAuthConsent.tsxpackages/ui/src/components/OAuthConsent/__tests__/OAuthConsent.test.tsx
| const redirectUri = getRedirectUriFromSearch(); | ||
| const { data, isLoading, error } = useOAuthConsent({ | ||
| oauthClientId, | ||
| scope, | ||
| redirectUri: redirectUri || undefined, |
There was a problem hiding this comment.
Use one redirect URI source for both consent fetch and display.
Line 57 always sends URL-derived redirectUri, while Line 74 allows context to override displayed redirectUrl. If those differ, domainAction can reflect backend resolution for a different URI than the one intended by context.
Suggested fix
- const redirectUri = getRedirectUriFromSearch();
+ const redirectUri = ctx.redirectUrl ?? getRedirectUriFromSearch();
const { data, isLoading, error } = useOAuthConsent({
oauthClientId,
scope,
redirectUri: redirectUri || undefined,
// TODO: Remove this once account portal is refactored to use this component
enabled: !hasContextCallbacks,
});
@@
- const redirectUrl = ctx.redirectUrl ?? redirectUri;
+ const redirectUrl = redirectUri;Also applies to: 74-74, 90-90
🤖 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/ui/src/components/OAuthConsent/OAuthConsent.tsx` around lines 53 -
57, The code uses getRedirectUriFromSearch() for the consent fetch (passed into
useOAuthConsent as redirectUri) but allows a different redirectUrl to be shown
in the UI, causing a mismatch; compute a single resolvedRedirectUri once (e.g.,
const resolvedRedirectUri = contextProvidedRedirectUrl ??
getRedirectUriFromSearch() ?? undefined) and use that same resolvedRedirectUri
when calling useOAuthConsent and when rendering/displaying the redirect URL so
both backend fetch and UI display use the exact same value; update references to
redirectUri/redirectUrl in OAuthConsent (including the call site of
useOAuthConsent and the display logic) to use resolvedRedirectUri.
There was a problem hiding this comment.
Is ctx just used for testing?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where useOAuthConsentContext is defined and how ctx is populated
rg -n "useOAuthConsentContext|OAuthConsentContext|OAuthConsentCtx" --type=ts --type=tsx -A 5Length of output: 447
🏁 Script executed:
#!/bin/bash
rg -rn "useOAuthConsentContext\|OAuthConsentContext" -A 5Length of output: 312
🏁 Script executed:
#!/bin/bash
rg -rn "useOAuthConsentContext" -A 5
rg -rn "OAuthConsentContext" -A 5Length of output: 5573
🏁 Script executed:
#!/bin/bash
# Get the full provider value and OAuthConsentCtx type
cat packages/ui/src/contexts/components/OAuthConsent.ts
rg -n "oauthClientId\|onAllow\|onDeny\|redirectUrl\|OAuthConsentCtx" packages/ui/src/contexts/ClerkUIComponentsContext.tsx -A 3Length of output: 937
🏁 Script executed:
#!/bin/bash
rg -rn "OAuthConsentCtx" -A 15
rg -n "oauthClientId\|onAllow\|onDeny\|redirectUrl" packages/ui/src/contexts/ClerkUIComponentsContext.tsx -B 2 -A 3Length of output: 2748
🏁 Script executed:
#!/bin/bash
# Get the full OAuthConsentCtx type from types.ts
rg -n "componentName: 'OAuthConsent'" packages/ui/src/types.ts -A 50 | head -60Length of output: 1934
@jfoshee No — ctx is a production integration point, not a test mechanism. According to the type definitions in packages/ui/src/types.ts, it represents the accounts portal path: a separate accounts portal service pre-populates all data (scopes, oauthApplicationName, redirectUrl, etc.) and provides onAllow/onDeny callbacks via context.
This also invalidates my earlier review comment. The mismatch I described can't occur in practice because:
ctx.redirectUrlis only populated on the accounts portal path, which always setsonAllow/onDeny.- The
useOAuthConsenthook hasenabled: !hasContextCallbacks, so it is disabled on the accounts portal path entirely. - Therefore, in the only path where the hook actually runs (the public path),
ctx.redirectUrlis alwaysundefined, and bothredirectUriandredirectUrlconsistently resolve togetRedirectUriFromSearch().
My original concern was a false positive — the current code is correct as-is. Sorry for the noise.
🧠 Learnings used
Learnt from: alexcarpenter
Repo: clerk/javascript PR: 8593
File: packages/ui/src/elements/ClipboardInput.tsx:0-0
Timestamp: 2026-05-19T17:58:25.594Z
Learning: When styling native HTML `<input type="text">` (or other single-line text inputs), applying `textOverflow: 'ellipsis'` by itself is sufficient—native single-line input behavior provides implicit single-line/overflow handling, so you generally do not need to also set `overflow: 'hidden'` or `whiteSpace: 'nowrap'`. Conversely, if you’re applying ellipsis to non-input/block-level elements (e.g., `<div>`, `<span>`, or other elements), ensure `whiteSpace: 'nowrap'` and `overflow: 'hidden'` are included alongside `textOverflow: 'ellipsis'` so the ellipsis can render.
Co-authored-by: Jacob Foshee <jacobf@gmail.com>
jfoshee
left a comment
jfoshee
left a comment
There was a problem hiding this comment.
Did you test w/ AP locally? You should be able to test it locally now using make up/local-js
…-suffix-list-to-extract-root
jacekradko
left a comment
jacekradko
left a comment
There was a problem hiding this comment.
Need to update the changeset
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@clerk/ui': patch | |||
There was a problem hiding this comment.
Looks like this also touches shared and clerk-js
There was a problem hiding this comment.
thanks! will update after some more local testing later
…-suffix-list-to-extract-root
API Changes Report
Summary
@clerk/sharedCurrent version: 4.14.0 Subpath
|
I tested via 
|
|
@jacekradko thoughts on the API changes report here #8700 (comment) ? The |
…-suffix-list-to-extract-root
3 participants
Description
The OAuth consent page was computing the display domain client-side by splitting the hostname on dots, which breaks for ccTLDs like
.co.ukand hosting platforms likegithub.io. This is a security concern since users make trust decisions based on that domain.The backend already has full PSL support, so this PR threads
redirect_urithrough toGET /v1/me/oauth/consent/{clientID}and uses the resolvedredirectDomainfrom the response instead. The client-side heuristic is kept as a fallback for IPs, localhost, and older FAPI versions.Part of USER-5440
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change