-
Notifications
You must be signed in to change notification settings - Fork 278
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,26 +27,52 @@ export const DEV_OR_STAGING_SUFFIXES = [ | |
'accounts.dev', | ||
]; | ||
|
||
export const LEGACY_DEV_SUFFIXES = ['.lcl.dev', '.lclstage.dev', '.lclclerk.com']; | ||
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. 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: | ||
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. Maybe we should be a bit more strict with our checks here. For example we will return true for hostname |
||
// * 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: | ||
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. Same here we can be more strict with our checks. For example we will return true for hostname 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. 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 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. @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.', ''); | ||
} | ||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.