Skip to content
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

fix(clerk-js): Pass dev_browser to AP via query param, fix AP origin … #1567

Merged
merged 1 commit into from
Aug 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/afraid-countries-smash.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@clerk/clerk-js': patch
---

Pass dev_browser to AP via query param, fix AP origin detection util
36 changes: 36 additions & 0 deletions packages/clerk-js/src/core/clerk.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1494,5 +1494,41 @@ describe('Clerk singleton', () => {
const url = sut.buildUrlWithAuth('foo');
expect(url).toBe('foo');
});

it('uses the hash to propagate the dev_browser JWT by default on dev', async () => {
mockUsesUrlBasedSessionSync.mockReturnValue(true);
const sut = new Clerk(devFrontendApi);
await sut.load();

const url = sut.buildUrlWithAuth('https://example.com/some-path');
expect(url).toBe('https://example.com/some-path#__clerk_db_jwt[deadbeef]');
});

it('uses the query param to propagate the dev_browser JWT if specified by option on dev', async () => {
mockUsesUrlBasedSessionSync.mockReturnValue(true);
const sut = new Clerk(devFrontendApi);
await sut.load();

const url = sut.buildUrlWithAuth('https://example.com/some-path', { useQueryParam: true });
expect(url).toBe('https://example.com/some-path?__dev_session=deadbeef');
});

it('uses the query param to propagate the dev_browser JWT to Account Portal pages on dev - non-kima', async () => {
mockUsesUrlBasedSessionSync.mockReturnValue(true);
const sut = new Clerk(devFrontendApi);
await sut.load();

const url = sut.buildUrlWithAuth('https://accounts.abcef.12345.dev.lclclerk.com');
expect(url).toBe('https://accounts.abcef.12345.dev.lclclerk.com/?__dev_session=deadbeef');
});

it('uses the query param to propagate the dev_browser JWT to Account Portal pages on dev - kima', async () => {
mockUsesUrlBasedSessionSync.mockReturnValue(true);
const sut = new Clerk(devFrontendApi);
await sut.load();

const url = sut.buildUrlWithAuth('https://rested-anemone-14.accounts.dev');
expect(url).toBe('https://rested-anemone-14.accounts.dev/?__dev_session=deadbeef');
});
});
});
9 changes: 5 additions & 4 deletions packages/clerk-js/src/core/clerk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ import {
ignoreEventValue,
inActiveBrowserTab,
inBrowser,
isAccountsHostedPages,
isDevAccountPortalOrigin,
isDevOrStagingUrl,
isError,
isRedirectForFAPIInitiatedFlow,
Expand Down Expand Up @@ -668,9 +668,10 @@ export default class Clerk implements ClerkInterface {
return clerkMissingDevBrowserJwt();
}

const asQueryParam = !!options?.useQueryParam;
// Use query param for Account Portal pages so that SSR can access the dev_browser JWT
const asQueryParam = !!options?.useQueryParam || isDevAccountPortalOrigin(toURL.hostname);

return setDevBrowserJWTInURL(toURL.href, devBrowserJwt, asQueryParam);
return setDevBrowserJWTInURL(toURL, devBrowserJwt, asQueryParam).href;
}

public buildSignInUrl(options?: RedirectOptions): string {
Expand Down Expand Up @@ -1231,7 +1232,7 @@ export default class Clerk implements ClerkInterface {
this.#authService = new SessionCookieService(this);
this.#pageLifecycle = createPageLifecycle();

const isInAccountsHostedPages = isAccountsHostedPages(window?.location.hostname);
const isInAccountsHostedPages = isDevAccountPortalOrigin(window?.location.hostname);

this.#setupListeners();

Expand Down
2 changes: 1 addition & 1 deletion packages/clerk-js/src/core/devBrowserHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ export default function createDevBrowserHandler({

async function setUrlBasedSessionSyncBrowser(): Promise<void> {
// 1. Get the JWT from hash search parameters when the redirection comes from Clerk Hosted Pages
const devBrowserToken = getDevBrowserJWTFromURL(window.location.href);
const devBrowserToken = getDevBrowserJWTFromURL(new URL(window.location.href));
if (devBrowserToken) {
setDevBrowserJWT(devBrowserToken);
return;
Expand Down
30 changes: 19 additions & 11 deletions packages/clerk-js/src/utils/__tests__/devbrowser.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { getDevBrowserJWTFromURL, setDevBrowserJWTInURL } from '../devBrowser';

const DUMMY_URL_BASE = 'http://clerk-dummy';

describe('setDevBrowserJWTInURL(url, jwt)', () => {
const testCases: Array<[string, string, boolean, string]> = [
['', 'deadbeef', false, '#__clerk_db_jwt[deadbeef]'],
Expand All @@ -15,8 +17,10 @@ describe('setDevBrowserJWTInURL(url, jwt)', () => {

test.each(testCases)(
'sets the dev browser JWT at the end of the provided url. Params: url=(%s), jwt=(%s), expected url=(%s)',
(hash, paramName, asQueryParam, expectedUrl) => {
expect(setDevBrowserJWTInURL(hash, paramName, asQueryParam)).toEqual(expectedUrl);
(input, paramName, asQueryParam, expected) => {
expect(setDevBrowserJWTInURL(new URL(input, DUMMY_URL_BASE), paramName, asQueryParam).href).toEqual(
new URL(expected, DUMMY_URL_BASE).href,
);
},
);
});
Expand All @@ -42,7 +46,7 @@ describe('getDevBrowserJWTFromURL(url,)', () => {
});

it('does not replaceState if the url does not contain a dev browser JWT', () => {
expect(getDevBrowserJWTFromURL('/foo')).toEqual('');
expect(getDevBrowserJWTFromURL(new URL('/foo', DUMMY_URL_BASE))).toEqual('');
expect(replaceStateMock).not.toHaveBeenCalled();
});

Expand All @@ -56,12 +60,16 @@ describe('getDevBrowserJWTFromURL(url,)', () => {
['/foo?bar=42#qux__clerk_db_jwt[deadbeef]', 'deadbeef', '/foo?bar=42#qux'],
];

test.each(testCases)('returns the dev browser JWT from a url. Params: url=(%s), jwt=(%s)', (url, jwt, calledWith) => {
expect(getDevBrowserJWTFromURL(url)).toEqual(jwt);
if (calledWith === null) {
expect(replaceStateMock).not.toHaveBeenCalled();
} else {
expect(replaceStateMock).toHaveBeenCalledWith(null, '', calledWith);
}
});
test.each(testCases)(
'returns the dev browser JWT from a url. Params: url=(%s), jwt=(%s)',
(input, jwt, calledWith) => {
expect(getDevBrowserJWTFromURL(new URL(input, DUMMY_URL_BASE))).toEqual(jwt);

if (calledWith === null) {
expect(replaceStateMock).not.toHaveBeenCalled();
} else {
expect(replaceStateMock).toHaveBeenCalledWith(null, '', new URL(calledWith, DUMMY_URL_BASE).href);
}
},
);
});
16 changes: 8 additions & 8 deletions packages/clerk-js/src/utils/__tests__/url.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,32 +8,32 @@ import {
getSearchParameterFromHash,
hasBannedProtocol,
hasExternalAccountSignUpError,
isAccountsHostedPages,
isAllowedRedirectOrigin,
isDataUri,
isDevAccountPortalOrigin,
isRedirectForFAPIInitiatedFlow,
isValidUrl,
mergeFragmentIntoUrl,
requiresUserInput,
trimTrailingSlash,
} from '../url';

describe('isAccountsHostedPages(url)', () => {
describe('isDevAccountPortalOrigin(url)', () => {
const goodUrls: Array<[string | URL, boolean]> = [
['clerk.dev.lclclerk.com', false],
['clerk.prod.lclclerk.com', false],
['clerk.abc.efg.lclstage.dev', false],
['clerk.abc.efg.stgstage.dev', false],
['accounts.abc.efg.dev.lclclerk.com', true],
['https://accounts.abc.efg.stg.lclclerk.com', true],
[new URL('https://clerk.abc.efg.lcl.dev'), false],
[new URL('https://accounts.abc.efg.lcl.dev'), true],
[new URL('https://accounts.abc.efg.stg.dev'), true],
['rested-anemone-14.accounts.dev', true],
['rested-anemone-14.accounts.dev.accountsstage.dev', true],
['rested-anemone-14.accounts.dev.accounts.lclclerk.com', true],
['rested-anemone-14.clerk.accounts.dev', false],
];

test.each(goodUrls)('.isAccountsHostedPages(%s)', (a, expected) => {
test.each(goodUrls)('.isDevAccountPortalOrigin(%s)', (a, expected) => {
// @ts-ignore
expect(isAccountsHostedPages(a)).toBe(expected);
expect(isDevAccountPortalOrigin(a)).toBe(expected);
});
});

Expand Down
69 changes: 45 additions & 24 deletions packages/clerk-js/src/utils/devBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,41 +3,62 @@ import { DEV_BROWSER_SSO_JWT_PARAMETER } from '../core/constants';
export const DEV_BROWSER_JWT_MARKER = '__clerk_db_jwt';
const DEV_BROWSER_JWT_MARKER_REGEXP = /__clerk_db_jwt\[(.*)\]/;

function extractDevBrowserJWT(url: string): string {
const matches = url.match(DEV_BROWSER_JWT_MARKER_REGEXP);
return matches ? matches[1] : '';
}
// Sets the dev_browser JWT in the hash or the search
export function setDevBrowserJWTInURL(url: URL, jwt: string, asQueryParam: boolean): URL {
const resultURL = new URL(url);

export function setDevBrowserJWTInURL(url: string, jwt: string, asQueryParam: boolean): string {
if (asQueryParam) {
const hasQueryParam = (url || '').includes('?');
return `${url}${hasQueryParam ? '&' : '?'}${DEV_BROWSER_SSO_JWT_PARAMETER}=${(jwt || '').trim()}`;
// extract & strip existing jwt from hash
const jwtFromHash = extractDevBrowserJWTFromHash(resultURL.hash);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Since this is the setDevBrowserJWTInURL method are we using extractDevBrowserJWTFromHash to make sure it's only set once?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it might be in the hash & we need to move it to search or vice versa.

resultURL.hash = resultURL.hash.replace(DEV_BROWSER_JWT_MARKER_REGEXP, '');
if (resultURL.href.endsWith('#')) {
resultURL.hash = '';
}

const dbJwt = extractDevBrowserJWT(url);
if (dbJwt) {
url.replace(`${DEV_BROWSER_JWT_MARKER}[${dbJwt}]`, jwt);
return url;
// extract & strip existing jwt from search
const jwtFromSearch = resultURL.searchParams.get(DEV_BROWSER_SSO_JWT_PARAMETER);
resultURL.searchParams.delete(DEV_BROWSER_SSO_JWT_PARAMETER);

// Existing jwt takes precedence
const jwtToSet = jwtFromHash || jwtFromSearch || jwt;

if (jwtToSet) {
if (asQueryParam) {
resultURL.searchParams.append(DEV_BROWSER_SSO_JWT_PARAMETER, jwtToSet);
} else {
resultURL.hash = resultURL.hash + `${DEV_BROWSER_JWT_MARKER}[${jwtToSet}]`;
}
}
const hasHash = (url || '').includes('#');
return `${url}${hasHash ? '' : '#'}${DEV_BROWSER_JWT_MARKER}[${(jwt || '').trim()}]`;

return resultURL;
}

export function getDevBrowserJWTFromURL(url: string): string {
const jwt = extractDevBrowserJWT(url);
if (!jwt) {
return '';
// Gets the dev_browser JWT from either the hash or the search
// Side effect:
// Removes dev_browser JWT from the URL as a side effect and updates the browser history
export function getDevBrowserJWTFromURL(url: URL): string {
const resultURL = new URL(url);

// extract & strip existing jwt from hash
const jwtFromHash = extractDevBrowserJWTFromHash(resultURL.hash);
resultURL.hash = resultURL.hash.replace(DEV_BROWSER_JWT_MARKER_REGEXP, '');
if (resultURL.href.endsWith('#')) {
resultURL.hash = '';
}

let newUrl = url.replace(DEV_BROWSER_JWT_MARKER_REGEXP, '');
// extract & strip existing jwt from search
const jwtFromSearch = resultURL.searchParams.get(DEV_BROWSER_SSO_JWT_PARAMETER) || '';
resultURL.searchParams.delete(DEV_BROWSER_SSO_JWT_PARAMETER);

if (newUrl.endsWith('#')) {
newUrl = newUrl.slice(0, -1);
}
const jwt = jwtFromHash || jwtFromSearch;

if (typeof globalThis.history !== undefined) {
globalThis.history.replaceState(null, '', newUrl);
if (jwt && typeof globalThis.history !== undefined) {
globalThis.history.replaceState(null, '', resultURL.href);
}

return jwt;
}

function extractDevBrowserJWTFromHash(hash: string): string {
const matches = hash.match(DEV_BROWSER_JWT_MARKER_REGEXP);
return matches ? matches[1] : '';
}
40 changes: 33 additions & 7 deletions packages/clerk-js/src/utils/url.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,26 +27,52 @@ export const DEV_OR_STAGING_SUFFIXES = [
'accounts.dev',
];

export const LEGACY_DEV_SUFFIXES = ['.lcl.dev', '.lclstage.dev', '.lclclerk.com'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a comment for posterity/ future reference mostly, this change needs to be made to all similar helpers until we revamp clerk/shared.

export const CURRENT_DEV_SUFFIXES = ['.accounts.dev', '.accountsstage.dev', '.accounts.lclclerk.com'];

const BANNED_URI_PROTOCOLS = ['javascript:'] as const;

const { isDevOrStagingUrl } = createDevOrStagingUrlCache();
export { isDevOrStagingUrl };
const accountsCache = new Map<string, boolean>();
const accountPortalCache = new Map<string, boolean>();

export function isAccountsHostedPages(url: string | URL = window.location.hostname): boolean {
if (!url) {
export function isDevAccountPortalOrigin(hostname: string = window.location.hostname): boolean {
if (!hostname) {
return false;
}

const hostname = typeof url === 'string' ? url : url.hostname;
let res = accountsCache.get(hostname);
let res = accountPortalCache.get(hostname);

if (res === undefined) {
res = DEV_OR_STAGING_SUFFIXES.some(s => /^(https?:\/\/)?accounts\./.test(hostname) && hostname.endsWith(s));
accountsCache.set(hostname, res);
res = isLegacyDevAccountPortalOrigin(hostname) || isCurrentDevAccountPortalOrigin(hostname);
accountPortalCache.set(hostname, res);
}

return res;
}

// Returns true for hosts such as:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should be a bit more strict with our checks here. For example we will return true for hostname accounts.foo.13.bar-1.lcl.dev which is wrong

// * accounts.foo.bar-13.lcl.dev
// * accounts.foo.bar-13.lclstage.dev
// * accounts.foo.bar-13.dev.lclclerk.com
function isLegacyDevAccountPortalOrigin(host: string): boolean {
return LEGACY_DEV_SUFFIXES.some(legacyDevSuffix => {
return host.startsWith('accounts.') && host.endsWith(legacyDevSuffix);
});
}

// Returns true for hosts such as:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here we can be more strict with our checks. For example we will return true for hostname foo.bar.13.whatever.accounts.dev which is wrong. Maybe use a regex instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of a regex, but preferred to use the same logic as the BE has for this.

Note that this regex would also require that .clerk is not contained be contained and I'm a bit worried about using a negative lookahead (?!).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chanioxaris do you have any planned changes in mind that could break this check? I totally agree that we could make this stricter to be on the safe side... the example domain you provided is unlikely to be used by someone though - could we be missing any cases here?

// * foo-bar-13.accounts.dev
// * foo-bar-13.accountsstage.dev
// * foo-bar-13.accounts.lclclerk.com
// But false for:
// * foo-bar-13.clerk.accounts.lclclerk.com
function isCurrentDevAccountPortalOrigin(host: string): boolean {
return CURRENT_DEV_SUFFIXES.some(currentDevSuffix => {
return host.endsWith(currentDevSuffix) && !host.endsWith('.clerk' + currentDevSuffix);
});
}

export function getETLDPlusOneFromFrontendApi(frontendApi: string): string {
return frontendApi.replace('clerk.', '');
}
Expand Down
4 changes: 2 additions & 2 deletions packages/nextjs/src/server/authMiddleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -456,14 +456,14 @@ describe('Dev Browser JWT when redirecting to cross origin', function () {
expect(authenticateRequest).toBeCalled();
});

it('appends the Dev Browser JWT on the URL when cookie __clerk_db_jwt exists', async () => {
it('appends the Dev Browser JWT to the search when cookie __clerk_db_jwt exists and location is an Account Portal URL', async () => {
const resp = await authMiddleware({
beforeAuth: () => NextResponse.next(),
})(mockRequest({ url: '/protected', appendDevBrowserCookie: true }), {} as NextFetchEvent);

expect(resp?.status).toEqual(307);
expect(resp?.headers.get('location')).toEqual(
'https://accounts.included.katydid-92.lcl.dev/sign-in?redirect_url=https%3A%2F%2Fwww.clerk.com%2Fprotected#__clerk_db_jwt[test_jwt]',
'https://accounts.included.katydid-92.lcl.dev/sign-in?redirect_url=https%3A%2F%2Fwww.clerk.com%2Fprotected&__dev_session=test_jwt',
);
expect(resp?.headers.get('x-clerk-auth-reason')).toEqual('redirect');
expect(authenticateRequest).toBeCalled();
Expand Down
17 changes: 14 additions & 3 deletions packages/nextjs/src/server/authMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { DEV_BROWSER_JWT_MARKER, setDevBrowserJWTInURL } from './devBrowser';
import { infiniteRedirectLoopDetected, informAboutProtectedRouteInfo, receivedRequestForIgnoredRoute } from './errors';
import { redirectToSignIn } from './redirect';
import type { NextMiddlewareResult, WithAuthOptions } from './types';
import { isDevAccountPortalOrigin } from './url';
import {
apiEndpointUnauthorizedNextResponse,
decorateRequest,
Expand Down Expand Up @@ -282,16 +283,26 @@ const withDefaultPublicRoutes = (publicRoutes: RouteMatcherParam | undefined) =>
// Middleware runs on the server side, before clerk-js is loaded, that's why we need Cookies.
const appendDevBrowserOnCrossOrigin = (req: WithClerkUrl<NextRequest>, res: Response, opts: AuthMiddlewareParams) => {
const location = res.headers.get('location');

const shouldAppendDevBrowser = res.headers.get(constants.Headers.ClerkRedirectTo) === 'true';

if (
shouldAppendDevBrowser &&
!!location &&
isDevelopmentFromApiKey(opts.secretKey || SECRET_KEY) &&
isCrossOrigin(req.experimental_clerkUrl, location)
) {
const dbJwt = req.cookies.get(DEV_BROWSER_JWT_MARKER)?.value;
const urlWithDevBrowser = setDevBrowserJWTInURL(location, dbJwt);
return NextResponse.redirect(urlWithDevBrowser, res);
const dbJwt = req.cookies.get(DEV_BROWSER_JWT_MARKER)?.value || '';

// Next.js 12.1+ allows redirects only to absolute URLs
const url = new URL(location);

// Use query param for Account Portal pages so that SSR can access the dev_browser JWT
const asQueryParam = isDevAccountPortalOrigin(url.hostname);

const urlWithDevBrowser = setDevBrowserJWTInURL(url, dbJwt, asQueryParam);

return NextResponse.redirect(urlWithDevBrowser.href, res);
}
return res;
};
Expand Down
Loading