-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: support multiple cookies with the same name across different paths #14056
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
🦋 Changeset detectedLatest commit: 86f0bfb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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.
Pull Request Overview
This PR fixes the issue where multiple cookies with the same name but different paths would overwrite each other in the internal cookie storage. The fix changes the internal cookie key structure from using just the cookie name to a unique key combining domain, path, and name.
- Modified cookie key generation to use
domain/path?name
format instead of justname
- Added a third parameter to
cookies.get()
to retrieve cookies by specific domain and path - Updated all tests to use the new cookie key format
Reviewed Changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
packages/kit/src/runtime/server/cookie.js | Implements the new cookie key generation logic and updates the get/set methods |
packages/kit/src/runtime/server/cookie.spec.js | Updates test assertions to use new cookie key format and adds new test cases |
.changeset/shiny-spoons-stick.md | Adds changeset entry for the bug fix |
3c8d117
to
52a674e
Compare
packages/kit/src/exports/public.d.ts
Outdated
get: ( | ||
name: string, | ||
opts?: import('cookie').CookieParseOptions, | ||
target?: { domain?: string; path?: string } |
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.
I'm not sure if target
is appropriate name.
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.
I would suggest to put the domain
&path
options into the opts
object.
Having to specify the decode
parse option is probably even less likely than having to specify the domain
or path
option. Having a third argument would mean you have to do cookies.get('my-cookie', undefined, { domain: 'xyz.com' })
.
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.
Thank you so much!!
That looks much better.
I changed it!!
Thank you. This isn't quite right though — it only affects cookies that were created during the current request; the options are ignored for existing cookies. This leads to chaotic outcomes. Consider a case like this: // src/routes/+layout.server.ts
export function load({ cookies }) {
cookies.set('foo', 'bar', {
path: '/'
});
} // src/routes/x/y/z/+page.server.ts
export function load({ cookies }) {
return {
foo: cookies.get('foo')
};
} When you visit cookies.get('foo', { path: '/x/y/z' }); ...which returns My understanding (double-check me on this) is that the most specific cookie wins, which is to say that if you set all these (order doesn't matter)... n=1; Path=/; HttpOnly; SameSite=Lax
n=2; Path=/x; HttpOnly; SameSite=Lax
n=3; Path=/x/y; HttpOnly; SameSite=Lax
n=4 Path=/x/y/z; HttpOnly; SameSite=Lax ...then on So this PR is correct to ensure that |
- Add generate_cookie_key function to create unique keys from domain/path/name - Extend cookies.get() to accept optional domain and path options in opts parameter - Store cookies with unique keys to prevent overwrites - Maintain backward compatibility with existing API usage - Add comprehensive tests for new functionality - Address review feedback from PR #14056 Co-authored-by: Rich-Harris <[email protected]>
- Fix formatting issues with prettier - Verify all tests pass including edge cases - Confirm TypeScript type checking passes - Complete implementation addresses all review feedback from PR #14056 Co-authored-by: Rich-Harris <[email protected]>
@Rich-Harris And seen #14131, will you close this PR? |
I will, yes, since it also fixes |
Thank you so much!! |
fix #13947
Support handling multiple cookies with the same name across different paths.
Previously key of
new_cookies
object isname
. It causes the issue when set same cookie name.Use unique key from combination of domain, path and name.
Adding a third parameter to
cookies.get
function to get specific cookie by domain and path name.New format of a unique key for
new_cookies
is based on its domain, path, and name lieknew_cookies["<domain>/<path>?<name>"]
For example,
cookies["/?name"]
,cookies["example.com/foo?name"]
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits