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

yourtallness · 2023-08-09T12:23:27Z

…detection util

Type of change

Packages affected

Description

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.

changeset-bot · 2023-08-09T12:23:30Z

🦋 Changeset detected

Latest commit: 9d54fe7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch

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

jit-ci

✅ Great news! Jit hasn't found any security issues in your PR. Good Job! 🏆

panteliselef

Looks good to me 💯 but maybe wait for Haris as requested

chanioxaris · 2023-08-10T11:02:44Z

packages/clerk-js/src/utils/url.ts

  return res;
 }

+// Returns true for hosts such as:


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

chanioxaris · 2023-08-10T11:03:33Z

packages/clerk-js/src/utils/url.ts

+  });
+}
+
+// Returns true for hosts such as:


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?

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 (?!).

@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?

SokratisVidros · 2023-08-14T13:11:10Z

packages/clerk-js/src/utils/url.ts

@@ -27,26 +27,53 @@ export const DEV_OR_STAGING_SUFFIXES = [
  'accounts.dev',
 ];

+export const LEGACY_DEV_SUFFIXES = ['.lcl.dev', '.lclstage.dev', '.lclclerk.com'];
+
+export const KIMA_DEV_SUFFIXES = ['.accounts.dev', '.accountstage.dev', '.accounts.lclclerk.com'];


Suggested change

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.

Will rename

SokratisVidros · 2023-08-14T13:11:47Z

packages/clerk-js/src/utils/url.ts

+// * foo-bar-13.accounts.lclclerk.com
+// But false for:
+// * foo-bar-13.clerk.accounts.lclclerk.com
+function isKimaDevAccountPortalOrigin(host: string): boolean {


Ditto about Kima

SokratisVidros · 2023-08-14T13:13:14Z

packages/clerk-js/src/utils/devBrowser.ts

-    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);


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

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

…detection util

nikosdouvlis · 2023-08-24T07:56:01Z

packages/clerk-js/src/utils/url.ts

@@ -27,26 +27,52 @@ export const DEV_OR_STAGING_SUFFIXES = [
  'accounts.dev',
 ];

+export const LEGACY_DEV_SUFFIXES = ['.lcl.dev', '.lclstage.dev', '.lclclerk.com'];


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

clerk-cookie · 2024-08-24T00:01:31Z

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.

yourtallness requested review from SokratisVidros and nikosdouvlis as code owners August 9, 2023 12:23

yourtallness requested a review from chanioxaris August 9, 2023 12:23

clerk-cookie added the clerk-js label Aug 9, 2023

yourtallness force-pushed the yourtallness/dev_browser_query_params_for_ap branch from 0144a11 to b9aa85d Compare August 9, 2023 12:25

jit-ci bot reviewed Aug 9, 2023

View reviewed changes

yourtallness force-pushed the yourtallness/dev_browser_query_params_for_ap branch from b9aa85d to e313934 Compare August 9, 2023 15:02

panteliselef approved these changes Aug 9, 2023

View reviewed changes

chanioxaris reviewed Aug 10, 2023

View reviewed changes

yourtallness force-pushed the yourtallness/dev_browser_query_params_for_ap branch 2 times, most recently from ba28b18 to 4f92750 Compare August 11, 2023 14:18

clerk-cookie added the nextjs label Aug 11, 2023

SokratisVidros suggested changes Aug 14, 2023

View reviewed changes

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

9d54fe7

…detection util

yourtallness force-pushed the yourtallness/dev_browser_query_params_for_ap branch from 4f92750 to 9d54fe7 Compare August 14, 2023 16:25

SokratisVidros approved these changes Aug 14, 2023

View reviewed changes

yourtallness merged commit 854aa38 into main Aug 14, 2023
8 checks passed

yourtallness deleted the yourtallness/dev_browser_query_params_for_ap branch August 14, 2023 16:53

clerk-cookie mentioned this pull request Aug 14, 2023

Version Packages #1546

Merged

nikosdouvlis reviewed Aug 24, 2023

View reviewed changes

clerk locked as resolved and limited conversation to collaborators Aug 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yourtallness commented Aug 9, 2023 •

edited

Loading

changeset-bot bot commented Aug 9, 2023 •

edited

Loading

jit-ci bot left a comment

panteliselef left a comment

chanioxaris Aug 10, 2023

chanioxaris Aug 10, 2023

yourtallness Aug 14, 2023

nikosdouvlis Aug 24, 2023

SokratisVidros Aug 14, 2023

yourtallness Aug 14, 2023

SokratisVidros Aug 14, 2023

yourtallness Aug 14, 2023

SokratisVidros Aug 14, 2023

yourtallness Aug 14, 2023

nikosdouvlis Aug 24, 2023

clerk-cookie commented Aug 24, 2024

	export const KIMA_DEV_SUFFIXES = ['.accounts.dev', '.accountstage.dev', '.accounts.lclclerk.com'];
	export const DEV_SUFFIXES = ['.accounts.dev', '.accountstage.dev', '.accounts.lclclerk.com'];

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

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

Conversation

yourtallness commented Aug 9, 2023 • edited Loading

Type of change

Packages affected

Description

changeset-bot bot commented Aug 9, 2023 • edited Loading

🦋 Changeset detected

jit-ci bot left a comment

Choose a reason for hiding this comment

panteliselef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clerk-cookie commented Aug 24, 2024

yourtallness commented Aug 9, 2023 •

edited

Loading

changeset-bot bot commented Aug 9, 2023 •

edited

Loading