-
Notifications
You must be signed in to change notification settings - Fork 346
feat(clerk-js): __session cookie Partitioned attribute #5514
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 0c62cd0 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
packages/clerk-js/src/core/auth/cookies/session.ts:34
- [nitpick] Consider renaming 'partitioned' to 'isPartitioned' to make the boolean nature of the variable more explicit.
const partitioned = secure;
packages/clerk-js/src/core/auth/cookies/session.ts:38
- [nitpick] Review the removal logic in the 'if (partitioned)' block; consider reusing the common cookie removal function to ensure consistency and reduce duplicated logic.
if (partitioned) {
!snapshot |
Hey @jacekradko - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new feature that sets the __session cookie with the Partitioned attribute in secure contexts while ensuring any existing non-partitioned cookies are removed to avoid conflicts. Key changes include:
- Exporting a new entry point (index.chips.ts) for the CHIP variant.
- Updating the session cookie handler to support the Partitioned attribute and remove existing cookies before setting the new ones.
- Adding a new variant (clerkCHIPS) in the rspack and jest configurations, along with corresponding build flag definitions.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
packages/clerk-js/src/index.chips.ts | Adds a new entry export for the CHIP variant. |
packages/clerk-js/src/core/auth/cookies/session.ts | Implements the Partitioned attribute support and cookie removal. |
packages/clerk-js/rspack.config.js | Introduces the new variant clerkCHIPS and related build flag. |
packages/clerk-js/jest.config.js | Updates globals to include the new variant flag. |
.changeset/plain-crabs-move.md | Documents the minor version bump and change description. |
Comments suppressed due to low confidence (1)
packages/clerk-js/src/core/auth/cookies/session.ts:27
- The removal order of cookies has been reversed compared to the original implementation. Please verify that calling sessionCookie.remove() before suffixedSessionCookie.remove() does not introduce any unintended behavior in secure contexts.
suffixedSessionCookie.remove();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look good to me, I added a question about the new variant
@@ -25,6 +26,7 @@ const variantToSourceFile = { | |||
[variants.clerkHeadless]: './src/index.headless.ts', | |||
[variants.clerkHeadlessBrowser]: './src/index.headless.browser.ts', | |||
[variants.clerkLegacyBrowser]: './src/index.legacy.browser.ts', | |||
[variants.clerkCHIPS]: './src/index.chips.ts', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we actually shipping this variant or it is a temporary thing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would ship it and pin specific instances to it
Description
Updating the way we set the
__session
cookie by adding the Partitioned attribute in secure contexts. Previously, our session cookie was set without this attribute, which could lead to potential cross-site issues. Now, we ensure that before setting the new cookie with the Partitioned attribute, any existing__session
cookies are removed to prevent conflicts with cookies without the Partitioned attribute as there is no way to distinguish them once they are set.Fixes: SDKI-898
Checklist
pnpm test
runs as expected.pnpm build
runs as expected.Type of change