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 7 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: 7bb2c0b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds support for displaying accurately resolved OAuth consent redirect domains using Public Suffix List resolution. The changes introduce a 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
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