fix(clerk-js): Pass dev_browser to AP via query param, fix AP origin …#1567
Conversation
yourtallness
requested review from
SokratisVidros and
nikosdouvlis
as code owners
🦋 Changeset detectedLatest commit: 9d54fe7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
yourtallness
force-pushed
the
yourtallness/dev_browser_query_params_for_ap
branch
from
0144a11 to
b9aa85d
Compare
![jit-ci[bot]](https://avatars.githubusercontent.com/in/142441?s=60&v=4){kind=small}
yourtallness
force-pushed
the
yourtallness/dev_browser_query_params_for_ap
branch
from
b9aa85d to
e313934
Compare
panteliselef
left a comment
panteliselef
left a comment
There was a problem hiding this comment.
Looks good to me 💯 but maybe wait for Haris as requested
| return res; | ||
| } | ||
|
|
||
| // Returns true for hosts such as: |
There was a problem hiding this comment.
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
| }); | ||
| } | ||
|
|
||
| // Returns true for hosts such as: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 (?!).
There was a problem hiding this comment.
@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?
yourtallness
force-pushed
the
yourtallness/dev_browser_query_params_for_ap
branch
2 times, most recently
from
ba28b18 to
4f92750
Compare
|
|
||
| export const LEGACY_DEV_SUFFIXES = ['.lcl.dev', '.lclstage.dev', '.lclclerk.com']; | ||
|
|
||
| export const KIMA_DEV_SUFFIXES = ['.accounts.dev', '.accountstage.dev', '.accounts.lclclerk.com']; |
There was a problem hiding this comment.
| export const KIMA_DEV_SUFFIXES = ['.accounts.dev', '.accountstage.dev', '.accounts.lclclerk.com']; | |
| export const DEV_SUFFIXES = ['.accounts.dev', '.accountstage.dev', '.accounts.lclclerk.com']; |
Kima was an internal project naming that shouldn't leak in the code.
| // * foo-bar-13.accounts.lclclerk.com | ||
| // But false for: | ||
| // * foo-bar-13.clerk.accounts.lclclerk.com | ||
| function isKimaDevAccountPortalOrigin(host: string): boolean { |
| 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); |
There was a problem hiding this comment.
❓ 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.
Yes, but it might be in the hash & we need to move it to search or vice versa.
yourtallness
force-pushed
the
yourtallness/dev_browser_query_params_for_ap
branch
from
4f92750 to
9d54fe7
Compare
| 'accounts.dev', | ||
| ]; | ||
|
|
||
| export const LEGACY_DEV_SUFFIXES = ['.lcl.dev', '.lclstage.dev', '.lclclerk.com']; |
There was a problem hiding this comment.
Adding a comment for posterity/ future reference mostly, this change needs to be made to all similar helpers until we revamp clerk/shared.
|
This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
6 participants
…detection util
Type of change
Packages affected
@clerk/clerk-js@clerk/clerk-react@clerk/nextjs@clerk/remix@clerk/types@clerk/themes@clerk/localizations@clerk/clerk-expo@clerk/backend@clerk/clerk-sdk-node@clerk/shared@clerk/fastify@clerk/chrome-extensiongatsby-plugin-clerkbuild/tooling/choreDescription
To make the dev_browser JWT available to the SSR context on Account Portal, we need to pass the JWT via the query, not the hash.
The current PR detects whether the URL we are navigating to is an AP URL & passes the db JWT as a query param.
Also fixed the AP origin detection util, which did not cover kima instances (ported logic from our backend).
Adding @chanioxaris to help verify if the AP origin detection logic is accurate.