fix(chrome-extension): Ensure synced client cookie URLs resolve appropriately

clerk / javascript

Official Javascript repository for Clerk authentication

https://clerk.com

MIT License

1.09k stars 240 forks source link

fix(chrome-extension): Ensure synced client cookie URLs resolve appropriately #3658

Closed tmilewski closed 3 months ago

tmilewski commented 3 months ago

Description

Ensures that there's a valid URL as some don't resolve with a protocol

Fixes SDK-1867

Checklist

[ ] npm test runs as expected.
[ ] npm run build runs as expected.
[ ] (If applicable) JSDoc comments have been added or updated for any package exports
[ ] (If applicable) Documentation has been updated

Type of change

[x] 🐛 Bug fix
[ ] 🌟 New feature
[ ] 🔨 Breaking change
[ ] 📖 Refactoring / dependency upgrade / documentation
[ ] other:

changeset-bot[bot] commented 3 months ago

🦋 Changeset detected

Latest commit: 2bdca284ef0f547afc2401e1aba94a33fd984aa1

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

This PR includes changesets to release 1 package

| Name | Type | | ----------------------- | ----- | | @clerk/chrome-extension | 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

tmilewski commented 3 months ago

!snapshot

clerk-cookie commented 3 months ago

Hey @tmilewski - the snapshot version command generated the following package versions:	Package	Version
@clerk/chrome-extension	1.1.2-snapshot.vd49286e
gatsby-plugin-clerk	5.0.0-beta.45

Tip: Use the snippet copy button below to quickly install the required packages. @clerk/chrome-extension

npm i @clerk/chrome-extension@1.1.2-snapshot.vd49286e --save-exact

gatsby-plugin-clerk

npm i gatsby-plugin-clerk@5.0.0-beta.45 --save-exact

tmilewski commented 3 months ago

Instead of making this change, why don't we fix the code where it's being called to pass a valid URL with protocol? This issue is caused by incorrectly passing the clerk.frontendApi (which is not a full URL) as param in the JWTHandler.

ref:

https://github.com/clerk/javascript/blob/1a704ed9de3ad222ff17d10cb383639b2fc038b9/packages/chrome-extension/src/singleton.ts#L48

@dimkl Yup, I just prepended it. I'm going to leave the other method as there are other cases where that will be necessary and ultimately extended.

dimkl commented 3 months ago

@dimkl Yup, I just prepended it. I'm going to leave the other method as there are other cases where that will be necessary and ultimately extended.

@tmilewski Could you provide example of other cases? Based on the code, the only case where the url as string is used is for a production instance where we explicitly pass the frontendApiKey (prefixed with https after this PR). In the case of the development instance, an array of URLs is passed and this code is not executed. Could we revert the changes in the packages/chrome-extension/src/utils/cookies.ts as it seems unnecessary?