diff --git a/.changeset/sad-turkeys-rhyme.md b/.changeset/sad-turkeys-rhyme.md new file mode 100644 index 00000000000..34b19fa4825 --- /dev/null +++ b/.changeset/sad-turkeys-rhyme.md @@ -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. diff --git a/.vscode/tasks.json b/.vscode/tasks.json new file mode 100644 index 00000000000..71f66cc4083 --- /dev/null +++ b/.vscode/tasks.json @@ -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" + } + ] +} \ No newline at end of file diff --git a/packages/clerk-js/sandbox/app.ts b/packages/clerk-js/sandbox/app.ts index db52e1ac505..15d45afc722 100644 --- a/packages/clerk-js/sandbox/app.ts +++ b/packages/clerk-js/sandbox/app.ts @@ -334,6 +334,8 @@ void (async () => { scopes, oAuthApplicationName: searchParams.get('oauth-application-name'), redirectUrl: searchParams.get('redirect_uri'), + oAuthApplicationLogoUrl: searchParams.get('logo-url'), + oAuthApplicationUrl: searchParams.get('app-url'), }, ); }, diff --git a/packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx b/packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx index dc7a55fac4c..d40ed41d158 100644 --- a/packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx +++ b/packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx @@ -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() { @@ -65,6 +67,8 @@ export function OAuthConsentInternal() { & { * The URL to navigate to when the logo is clicked. */ href?: string; + /** + * Whether the href should be treated as an external link. + * When true, uses a Link component with target="_blank" and proper security attributes. + * When false or undefined, uses RouterLink for internal navigation. + */ + isExternal?: boolean; }; export const ApplicationLogo: React.FC = (props: ApplicationLogoProps): JSX.Element | null => { - const { src, alt, href, sx, ...rest } = props; + const { src, alt, href, isExternal, sx, ...rest } = props; const imageRef = React.useRef(null); const [loaded, setLoaded] = React.useState(false); const { logoImageUrl, applicationName, homeUrl } = useEnvironment().displayConfig; @@ -80,12 +87,22 @@ export const ApplicationLogo: React.FC = (props: Applicati ]} > {logoUrl ? ( - - {image} - + isExternal ? ( + + {image} + + ) : ( + + {image} + + ) ) : ( image )} diff --git a/packages/clerk-js/src/ui/primitives/Link.tsx b/packages/clerk-js/src/ui/primitives/Link.tsx index b57e89caef2..c724e659779 100644 --- a/packages/clerk-js/src/ui/primitives/Link.tsx +++ b/packages/clerk-js/src/ui/primitives/Link.tsx @@ -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 { const { isExternal, children, href, onClick, ...rest } = props; + // Sanitize href to prevent dangerous protocols + const sanitizedHref = sanitizeHref(href); + const onClickHandler = onClick ? (e: React.MouseEvent) => { - if (!href) { + if (!sanitizedHref) { e.preventDefault(); } onClick(e); @@ -70,9 +74,9 @@ export const Link = (props: LinkProps): JSX.Element => { {children} diff --git a/packages/clerk-js/src/utils/__tests__/url.spec.ts b/packages/clerk-js/src/utils/__tests__/url.spec.ts index 9e37b01cc08..3c368500718 100644 --- a/packages/clerk-js/src/utils/__tests__/url.spec.ts +++ b/packages/clerk-js/src/utils/__tests__/url.spec.ts @@ -7,6 +7,7 @@ import { createAllowedRedirectOrigins, getETLDPlusOneFromFrontendApi, getSearchParameterFromHash, + hasBannedHrefProtocol, hasBannedProtocol, hasExternalAccountSignUpError, isAllowedRedirect, @@ -18,6 +19,7 @@ import { mergeFragmentIntoUrl, relativeToAbsoluteUrl, requiresUserInput, + sanitizeHref, trimLeadingSlash, trimTrailingSlash, } from '../url'; @@ -36,7 +38,7 @@ describe('isDevAccountPortalOrigin(url)', () => { ]; test.each(goodUrls)('.isDevAccountPortalOrigin(%s)', (a, expected) => { - // @ts-ignore + // @ts-ignore - Type assertion for test parameter expect(isDevAccountPortalOrigin(a)).toBe(expected); }); }); @@ -147,6 +149,79 @@ describe('hasBannedProtocol(url)', () => { }); }); +describe('hasBannedHrefProtocol(url)', () => { + const cases: Array<[string, boolean]> = [ + ['https://www.clerk.com/', false], + ['http://www.clerk.com/', false], + ['/sign-in', false], + ['/sign-in?test=1', false], + ['/?test', false], + ['javascript:console.log(document.cookies)', true], + ['data:image/png;base64,iVBORw0KGgoAAA5ErkJggg==', true], + ['vbscript:alert("xss")', true], + ['blob:https://example.com/12345678-1234-1234-1234-123456789012', true], + ['ftp://files.example.com/file.txt', false], + ['mailto:user@example.com', false], + ]; + + test.each(cases)('.hasBannedHrefProtocol(%s)', (a, expected) => { + expect(hasBannedHrefProtocol(a)).toBe(expected); + }); +}); + +describe('sanitizeHref(href)', () => { + const cases: Array<[string | undefined | null, string | null]> = [ + // Null/undefined/empty cases + [null, null], + [undefined, null], + ['', null], + [' ', null], + + // Safe relative URLs + ['/path/to/page', '/path/to/page'], + ['#anchor', '#anchor'], + ['?query=param', '?query=param'], + ['../relative/path', '../relative/path'], + ['relative/path', 'relative/path'], + ['path/page#anchor', 'path/page#anchor'], + + // Safe absolute URLs + ['https://www.clerk.com/', 'https://www.clerk.com/'], + ['http://localhost:3000/path', 'http://localhost:3000/path'], + ['ftp://files.example.com/file.txt', 'ftp://files.example.com/file.txt'], + ['mailto:user@example.com', 'mailto:user@example.com'], + + // Dangerous protocols - should return null + ['javascript:alert("xss")', null], + ['javascript:console.log(document.cookies)', null], + ['data:text/html,', null], + ['data:text/html;base64,PHNjcmlwdD5hbGVydCgnWFNTIGZyb20gZGF0YSBVUkknKTwvc2NyaXB0Pg==', null], + ['data:image/png;base64,iVBORw0KGgoAAA5ErkJggg==', null], + ['vbscript:alert("xss")', null], + ['blob:https://example.com/12345678-1234-1234-1234-123456789012', null], + + // Sneaky cases with dangerous protocols + ['JAVASCRIPT:alert("xss")', null], // All caps protocol + ['JavaScript:alert("xss")', null], // Mixed case + [' javascript:alert("xss") ', null], // Whitespace + ['javascript: alert("xss") ', null], // Whitespace + + // Malformed URLs that might be relative paths + ['not-a-url', 'not-a-url'], + ['path:with:colons', 'path:with:colons'], + ]; + + test.each(cases)('.sanitizeHref(%s)', (href, expected) => { + expect(sanitizeHref(href)).toBe(expected); + }); + + it('handles malformed URLs gracefully', () => { + // These should not throw errors and should be allowed as potential relative URLs + expect(sanitizeHref(':::invalid:::')).toBe(':::invalid:::'); + expect(sanitizeHref('malformed:url:here')).toBe('malformed:url:here'); + }); +}); + describe('buildURL(options: URLParams, skipOrigin)', () => { it('builds a URL()', () => { expect(buildURL({}, { stringify: true })).toBe('http://localhost:3000/'); diff --git a/packages/clerk-js/src/utils/url.ts b/packages/clerk-js/src/utils/url.ts index d5b69503538..9f511e82e6b 100644 --- a/packages/clerk-js/src/utils/url.ts +++ b/packages/clerk-js/src/utils/url.ts @@ -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; + const { isDevOrStagingUrl } = createDevOrStagingUrlCache(); export { isDevOrStagingUrl }; const accountPortalCache = new Map(); @@ -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); +} + +/** + * 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('#/'); }; diff --git a/packages/types/src/clerk.ts b/packages/types/src/clerk.ts index bb5ab633869..c60738237a9 100644 --- a/packages/types/src/clerk.ts +++ b/packages/types/src/clerk.ts @@ -2002,6 +2002,10 @@ export type __internal_OAuthConsentProps = { * Logo URL of the OAuth application. */ oAuthApplicationLogoUrl?: string; + /** + * URL of the OAuth application. + */ + oAuthApplicationUrl?: string; /** * Scopes requested by the OAuth application. */