Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 469
feat(clerk-js): Link to external App page in OAuth Consent#6447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
7e24a6a6b70d4f72e5409373ded00cc8f608c20482d66f708b455d8549283eaefe0820dcfde0ee87b64092ede1e632d77976c8af403576fdd10fb1557bff23ecaab41d437ac9b23e967038a74a96dc277File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- | ||
| '@clerk/clerk-js': patch | ||
| '@clerk/types': patch | ||
| --- | ||
| Add optional `isExternal` to `ApplicationLogo` | ||
| Add optional `oAuthApplicationUrl` parameter to OAuth Consent mounting (which is used to provide a link to the OAuth App homepage). | ||
| Harden `Link` component so it sanitizes the given `href` to avoid dangerous protocols. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| { | ||
| "version": "2.0.0", | ||
| "tasks": [ | ||
| { | ||
| "type": "npm", | ||
| "script": "dev:sandbox", | ||
| "path": "packages/clerk-js", | ||
| "problemMatcher": [], | ||
| "label": "Dev: Sandbox", | ||
| "detail": "npm: dev:sandbox - packages/clerk-js" | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -17,7 +17,7 @@ import { common } from '@/ui/styledSystem'; | ||
| import { colors } from '@/ui/utils/colors'; | ||
| export function OAuthConsentInternal() { | ||
| const { scopes, oAuthApplicationName, oAuthApplicationLogoUrl, redirectUrl, onDeny, onAllow } = | ||
| const { scopes, oAuthApplicationName, oAuthApplicationLogoUrl, oAuthApplicationUrl, redirectUrl, onDeny, onAllow } = | ||
| useOAuthConsentContext(); | ||
| const { user } = useUser(); | ||
| const { applicationName, logoImageUrl } = useEnvironment().displayConfig; | ||
| @@ -46,6 +46,8 @@ export function OAuthConsentInternal() { | ||
| <ApplicationLogo | ||
| src={oAuthApplicationLogoUrl} | ||
| alt={oAuthApplicationName} | ||
| href={oAuthApplicationUrl} | ||
| isExternal | ||
jfoshee marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| /> | ||
| </ConnectionItem> | ||
| <ConnectionSeparator /> | ||
| @@ -65,6 +67,8 @@ export function OAuthConsentInternal() { | ||
| <ApplicationLogo | ||
| src={oAuthApplicationLogoUrl} | ||
| alt={oAuthApplicationName} | ||
| href={oAuthApplicationUrl} | ||
| isExternal | ||
| /> | ||
| <ConnectionIcon | ||
| size='sm' | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import React from 'react'; | ||
| import { sanitizeHref } from '../../utils/url'; | ||
| import type { PrimitiveProps, StyleVariants } from '../styledSystem'; | ||
| import { common, createVariants } from '../styledSystem'; | ||
| import { applyDataStateProps } from './applyDataStateProps'; | ||
| @@ -57,9 +58,12 @@ export type LinkProps = PrimitiveProps<'a'> & OwnProps & StyleVariants<typeof ap | ||
| export const Link = (props: LinkProps): JSX.Element => { | ||
| const { isExternal, children, href, onClick, ...rest } = props; | ||
| // Sanitize href to prevent dangerous protocols | ||
| const sanitizedHref = sanitizeHref(href); | ||
| const onClickHandler = onClick | ||
| ? (e: React.MouseEvent<HTMLAnchorElement, MouseEvent>) => { | ||
| if (!href) { | ||
| if (!sanitizedHref) { | ||
| e.preventDefault(); | ||
| } | ||
| onClick(e); | ||
| @@ -70,9 +74,9 @@ export const Link = (props: LinkProps): JSX.Element => { | ||
| <a | ||
| {...applyDataStateProps(filterProps(rest))} | ||
| onClick={onClickHandler} | ||
| href={href || ''} | ||
| target={href && isExternal ? '_blank' : undefined} | ||
| rel={href && isExternal ? 'noopener' : undefined} | ||
| href={sanitizedHref || ''} | ||
| target={sanitizedHref && isExternal ? '_blank' : undefined} | ||
| rel={sanitizedHref && isExternal ? 'noopener noreferrer' : undefined} | ||
Comment on lines
+77
to
+79
| ||
| css={applyVariants(props) as any} | ||
| > | ||
| {children} | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -21,6 +21,9 @@ const DUMMY_URL_BASE = 'http://clerk-dummy'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const BANNED_URI_PROTOCOLS = ['javascript:'] as const; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Protocols that are dangerous specifically for href attributes in links | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const BANNED_HREF_PROTOCOLS = ['javascript:', 'data:', 'vbscript:', 'blob:'] as const; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+24
to
+26
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainReassess banning blob: (and vbscript:) in hrefs; verify repo-wide impact Blocking blob: at the sanitizer level will disable legitimate download links (URL.createObjectURL) everywhere Link is used. vbscript: is fine to ban; blob: is commonly used and origin-bound. If the intent is to harden OAuth consent only, consider scoping or allowing blob:. Run this script to detect potential breakage from banning blob: URLs: If you decide to keep downloads working, drop blob: from the banned list: -const BANNED_HREF_PROTOCOLS = ['javascript:', 'data:', 'vbscript:', 'blob:'] as const;+const BANNED_HREF_PROTOCOLS = ['javascript:', 'data:', 'vbscript:'] as const;🏁 Script executed: #!/bin/bash# Find usages that generate or expect blob: URLs
rg -n --no-heading -S "URL\.createObjectURL|createObjectURL\(|\bblob:" -A 2 -B 2
# Look for anchors/Link components likely used for downloads
rg -n --no-heading -S "<a [^>]*download|download\s*=" -A 2 -B 2
rg -n --no-heading -S "<Link[^>]*download|download\s*=" -A 2 -B 2
# Find all sanitizeHref call sites to assess scope
rg -n --no-heading -S "sanitizeHref\(" -A 2 -B 2Length of output: 11819 🏁 Script executed: #!/bin/bash# Display Link.tsx around sanitizeHref usage for context
sed -n '1,200p' packages/clerk-js/src/ui/primitives/Link.tsxLength of output: 2398 Allow blob: URLs for legitimate download links Drop • packages/clerk-js/src/utils/url.ts • packages/clerk-js/src/utils/tests/url.spec.ts Example diffs: --- a/packages/clerk-js/src/utils/url.ts+++ b/packages/clerk-js/src/utils/url.ts
@@
// Protocols that are dangerous specifically for href attributes in links
-const BANNED_HREF_PROTOCOLS = ['javascript:', 'data:', 'vbscript:', 'blob:'] as const;+const BANNED_HREF_PROTOCOLS = ['javascript:', 'data:', 'vbscript:'] as const;
@@
- * This prevents some XSS attacks through javascript:, data:, vbscript:, and blob: URLs.+ * This prevents some XSS attacks through javascript:, data:, and vbscript: URLs.--- a/packages/clerk-js/src/utils/__tests__/url.spec.ts+++ b/packages/clerk-js/src/utils/__tests__/url.spec.ts
@@ describe('isDangerousProtocol(val)', () => {
- ['blob:https://example.com/123…', true],+ ['blob:https://example.com/123…', false],
@@ describe('sanitizeHref(href)', () => {
- ['blob:https://example.com/123…', null],+ ['blob:https://example.com/123…', 'blob:https://example.com/123…'],📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { isDevOrStagingUrl } = createDevOrStagingUrlCache(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export { isDevOrStagingUrl }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const accountPortalCache = new Map<string, boolean>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -276,6 +279,16 @@ export function isDataUri(val?: string): val is string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return new URL(val).protocol === 'data:'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Checks if a URL uses javascript: protocol. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * This prevents some XSS attacks through javascript: URLs. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * IMPORTANT: This does not check for `data:` or other protocols which | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * are dangerous if used for links or setting the window location. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param val - The URL to check | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @returns True if the URL contains a banned protocol, false otherwise | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function hasBannedProtocol(val: string | URL) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!isValidUrl(val)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -284,6 +297,58 @@ export function hasBannedProtocol(val: string | URL) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return BANNED_URI_PROTOCOLS.some(bp => bp === protocol); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Checks if a URL contains a banned protocol for href attributes in links. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * This prevents some XSS attacks through javascript:, data:, vbscript:, and blob: URLs. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param val - The URL to check | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @returns True if the URL contains a banned protocol, false otherwise | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function hasBannedHrefProtocol(val: string | URL): boolean { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!isValidUrl(val)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const protocol = new URL(val).protocol; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return BANNED_HREF_PROTOCOLS.some(bp => bp === protocol); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+300
to
+314
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correctness: handle URL instances without reparsing and ensure consistent boolean return Same issue as hasBannedProtocol. Avoid reparsing when val is already a URL; current isValidUrl(val) returns false for URL objects, weakening the check. Apply: -export function hasBannedHrefProtocol(val: string | URL): boolean {- if (!isValidUrl(val)) {- return false;- }- const protocol = new URL(val).protocol;- return BANNED_HREF_PROTOCOLS.some(bp => bp === protocol);-}+export function hasBannedHrefProtocol(val: string | URL): boolean {+ if (val instanceof URL) {+ return BANNED_HREF_PROTOCOLS.some(bp => bp === val.protocol);+ }+ if (!isValidUrl(val)) {+ return false;+ }+ const protocol = new URL(val).protocol;+ return BANNED_HREF_PROTOCOLS.some(bp => bp === protocol);+}Add tests for URL inputs and mixed-case schemes (scheme is normalized, but test guards regressions). 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Sanitizes an href value by checking for dangerous protocols. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Returns null if the href contains a dangerous protocol, otherwise returns the original href. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * This prevents some XSS attacks through javascript:, data:, vbscript:, and blob: URLs. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param href - The href value to sanitize | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @returns The sanitized href or null if dangerous | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function sanitizeHref(href: string | undefined | null): string | null { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!href || href.trim() === '') { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // For relative URLs (starting with / or # or ?), allow them through | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (href.startsWith('/') || href.startsWith('#') || href.startsWith('?')) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return href; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // For relative URLs without leading slash, allow them through | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!href.includes(':')) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return href; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Check if it's a valid URL with a dangerous protocol | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const url = new URL(href); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (hasBannedHrefProtocol(url)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return href; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If URL parsing fails, it's likely a relative URL or malformed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Allow relative URLs through, but be cautious with malformed ones | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return href; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export const hasUrlInFragment = (_url: URL | string) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return new URL(_url, DUMMY_URL_BASE).hash.startsWith('#/'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -2002,6 +2002,10 @@ export type __internal_OAuthConsentProps = { | ||
| * Logo URL of the OAuth application. | ||
| */ | ||
| oAuthApplicationLogoUrl?: string; | ||
| /** | ||
| * URL of the OAuth application. | ||
| */ | ||
| oAuthApplicationUrl?: string; | ||
jfoshee marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| /** | ||
| * Scopes requested by the OAuth application. | ||
| */ | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.