Skip to content
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): Set SameSite=Lax for db cookie, so that it can be read… #1638

Merged

Conversation

yourtallness
Copy link
Contributor

… from SSR after redirects

Type of change

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

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-extension
  • gatsby-plugin-clerk
  • build/tooling/chore

Description

  • npm test runs as expected.
  • npm run build runs as expected.

WIP, works for new flows

Need to figure out the following:

  • Best point to change already set Strict db cookies to Lax
  • See if there is potential for duplicate cookies to be set

@changeset-bot
Copy link

changeset-bot bot commented Aug 27, 2023

🦋 Changeset detected

Latest commit: 8ebb192

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

@yourtallness yourtallness marked this pull request as draft August 27, 2023 16:22
Copy link

@jit-ci jit-ci bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -67,7 +67,7 @@ export const createCookieHandler = () => {

const setDevBrowserCookie = (jwt: string) => {
const expires = addYears(Date.now(), 1);
const sameSite = 'Strict';
const sameSite = 'Lax';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dimkl do you foresee any issues with e2e testing? My question is related to the changes we made about a month ago

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are making it more loose, it will probably not cause any issue. We could test it against the clerkjs cypress starter to verify that we don't miss something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yourtallness yourtallness force-pushed the mark/dsh-474-make-__clerk_db_jwt-cookie-samesitelax branch from c586cad to 70061e6 Compare August 28, 2023 14:02
@yourtallness yourtallness marked this pull request as ready for review August 29, 2023 18:37
@yourtallness
Copy link
Contributor Author

!snapshot

@clerk-cookie
Copy link
Collaborator

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

Package Version
@clerk/backend 0.28.0-snapshot.70061e6
@clerk/chrome-extension 0.3.27-snapshot.70061e6
@clerk/clerk-js 4.56.0-snapshot.70061e6
eslint-config-custom 0.3.0
@clerk/clerk-expo 0.18.18-snapshot.70061e6
@clerk/fastify 0.6.4-snapshot.70061e6
gatsby-plugin-clerk 4.4.5-snapshot.70061e6
@clerk/localizations 1.25.0-snapshot.70061e6
@clerk/nextjs 4.23.3-snapshot.70061e6
@clerk/clerk-react 4.24.0-snapshot.70061e6
@clerk/remix 2.10.0-snapshot.70061e6
@clerk/clerk-sdk-node 4.12.3-snapshot.70061e6
@clerk/shared 0.22.0-snapshot.70061e6
@clerk/themes 1.7.5
@clerk/types 3.50.0-snapshot.70061e6

Tip: use the snippet copy button below to quickly install the required packages.

# @clerk/backend
npm i @clerk/backend@0.28.0-snapshot.70061e6
# @clerk/chrome-extension
npm i @clerk/chrome-extension@0.3.27-snapshot.70061e6
# @clerk/clerk-js
npm i @clerk/clerk-js@4.56.0-snapshot.70061e6
# eslint-config-custom
npm i eslint-config-custom@0.3.0
# @clerk/clerk-expo
npm i @clerk/clerk-expo@0.18.18-snapshot.70061e6
# @clerk/fastify
npm i @clerk/fastify@0.6.4-snapshot.70061e6
# gatsby-plugin-clerk
npm i gatsby-plugin-clerk@4.4.5-snapshot.70061e6
# @clerk/localizations
npm i @clerk/localizations@1.25.0-snapshot.70061e6
# @clerk/nextjs
npm i @clerk/nextjs@4.23.3-snapshot.70061e6
# @clerk/clerk-react
npm i @clerk/clerk-react@4.24.0-snapshot.70061e6
# @clerk/remix
npm i @clerk/remix@2.10.0-snapshot.70061e6
# @clerk/clerk-sdk-node
npm i @clerk/clerk-sdk-node@4.12.3-snapshot.70061e6
# @clerk/shared
npm i @clerk/shared@0.22.0-snapshot.70061e6
# @clerk/themes
npm i @clerk/themes@1.7.5
# @clerk/types
npm i @clerk/types@3.50.0-snapshot.70061e6

@yourtallness yourtallness force-pushed the mark/dsh-474-make-__clerk_db_jwt-cookie-samesitelax branch from 70061e6 to 8ebb192 Compare August 30, 2023 12:45
@yourtallness yourtallness merged commit 3882e91 into main Aug 30, 2023
8 checks passed
@yourtallness yourtallness deleted the mark/dsh-474-make-__clerk_db_jwt-cookie-samesitelax branch August 30, 2023 13:02
@clerk-cookie clerk-cookie mentioned this pull request Aug 30, 2023
@clerk-cookie
Copy link
Collaborator

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.

@clerk clerk locked as resolved and limited conversation to collaborators Aug 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants