Skip to content

fix(clerk-js): Set localhost SameSite=None cookies as Secure#3604

Merged
anagstef merged 11 commits intomainfrom
stefanos/core-2541-remove-the-cypress-specific-fix-from-clerkjs
Jul 10, 2024
Merged

fix(clerk-js): Set localhost SameSite=None cookies as Secure#3604
anagstef merged 11 commits intomainfrom
stefanos/core-2541-remove-the-cypress-specific-fix-from-clerkjs

Conversation

@anagstef
Copy link
Copy Markdown
Member

@anagstef anagstef commented Jun 20, 2024

Description

This PR removes the specific fix we did a while ago for Cypress.
The real reason our cookies where getting deleted in Cypress Chrome is because Chrome requires Secure when setting SameSite=None cookies.

Safari needs a bit of a different handling because it does not consider localhost to be a secure context, so in Safari localhost cookies cannot have the Secure attribute.

So the new logic about the Secure attribute on cookies goes as follows:

  • If you are in a https location then add Secure on cookies.
  • If you are not in https:
    • If the browser is Safari, then do not add Secure (safari considers secure only https origins).
    • If the window.isSecureContext property exists, then respect it (safari does not work correctly with this, that's why we added a condition before this)
    • Lastly, if the window location is localhost and SameSite is set to None, add the Secure attribute (that's required by Chrome)

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

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

@anagstef anagstef self-assigned this Jun 20, 2024
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jun 20, 2024

🦋 Changeset detected

Latest commit: 802fd8f

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

This PR includes changesets to release 4 packages
Name Type
@clerk/clerk-js Patch
@clerk/astro 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

@anagstef anagstef marked this pull request as ready for review June 20, 2024 17:25
@anagstef
Copy link
Copy Markdown
Member Author

!snapshot

@clerk-cookie
Copy link
Copy Markdown
Collaborator

Hey @anagstef - the snapshot version command generated the following package versions:

Package Version
@clerk/chrome-extension 1.1.0-snapshot.v1f5b5e4
@clerk/clerk-js 5.7.2-snapshot.v1f5b5e4
@clerk/elements 0.9.0-snapshot.v1f5b5e4
@clerk/clerk-expo 1.2.3-snapshot.v1f5b5e4
gatsby-plugin-clerk 5.0.0-beta.45
@clerk/localizations 2.4.6-snapshot.v1f5b5e4
@clerk/ui 0.1.3-snapshot.v1f5b5e4

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

npm i @clerk/chrome-extension@1.1.0-snapshot.v1f5b5e4 --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@5.7.2-snapshot.v1f5b5e4 --save-exact

@clerk/elements

npm i @clerk/elements@0.9.0-snapshot.v1f5b5e4 --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@1.2.3-snapshot.v1f5b5e4 --save-exact

gatsby-plugin-clerk

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

@clerk/localizations

npm i @clerk/localizations@2.4.6-snapshot.v1f5b5e4 --save-exact

@clerk/ui

npm i @clerk/ui@0.1.3-snapshot.v1f5b5e4 --save-exact

@panteliselef
Copy link
Copy Markdown
Contributor

So it seems this PR reverts #3245 and proposes another solution

@anagstef
Copy link
Copy Markdown
Member Author

anagstef commented Jun 26, 2024

@panteliselef Exactly! This PR removes the hacky Cypress fix and proposes a more holistic solution for Secure when on localhost, which also fixes the Cypress issues, even if the cookies are set with SameSite=None.

@anagstef anagstef changed the title fix(clerkjs): Set localhost SameSite=None cookies as Secure fix(clerk-js): Set localhost SameSite=None cookies as Secure Jul 1, 2024
@anagstef anagstef requested review from octoper and panteliselef July 1, 2024 15:02
@anagstef anagstef enabled auto-merge (squash) July 10, 2024 15:21
@anagstef anagstef merged commit 6a98c08 into main Jul 10, 2024
@anagstef anagstef deleted the stefanos/core-2541-remove-the-cypress-specific-fix-from-clerkjs branch July 10, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants