Skip to content

Commit b2c5d02

Browse files
CopilotRich-Harris
Copilot
and
Rich-Harris
authored
fix: support multiple cookies with the same name across different paths and domains (#14131)
* Initial plan * Fix multiple cookies with same name but different paths/domains - 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]> * Format code and finalize cookie fix implementation - 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]> * Remove domain/path options from cookies.get() and implement 'most specific path wins' logic Co-authored-by: Rich-Harris <[email protected]> * Fix path specificity calculation in cookies.get() method Co-authored-by: Rich-Harris <[email protected]> * prettier * update changeset * fix * return undefined, not the empty string, when getting a deleted cookie * add changeset * tweak * add failing test for getAll * fix `getAll` to match updated `get` behaviour * copilot doesnt know about snake_case * use a Map for new_cookies, it's better * encode name * Update packages/kit/src/runtime/server/cookie.js * oops --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: Rich-Harris <[email protected]> Co-authored-by: Rich Harris <[email protected]> Co-authored-by: Rich Harris <[email protected]>
1 parent 0bf6185 commit b2c5d02

File tree

5 files changed

+147
-30
lines changed

5 files changed

+147
-30
lines changed

‎.changeset/fix-multiple-cookies-same-name.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@sveltejs/kit': patch
3+
---
4+
5+
fix: support multiple cookies with the same name across different paths and domains

‎.changeset/seven-cougars-train.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@sveltejs/kit': patch
3+
---
4+
5+
fix: `cookies.get(...)` returns `undefined` for a just-deleted cookie

‎packages/kit/src/runtime/server/cookie.js

Lines changed: 52 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,21 @@ function validate_options(options) {
2727
}
2828
}
2929

30+
/**
31+
* Generates a unique key for a cookie based on its domain, path, and name in
32+
* the format: `<domain>/<path>?<name>`.
33+
* If domain is undefined, it will be omitted.
34+
* For example: `/?name`, `example.com/foo?name`.
35+
*
36+
* @param {string | undefined} domain
37+
* @param {string} path
38+
* @param {string} name
39+
* @returns {string}
40+
*/
41+
function generate_cookie_key(domain, path, name) {
42+
return `${domain || ''}${path}?${encodeURIComponent(name)}`;
43+
}
44+
3045
/**
3146
* @param {Request} request
3247
* @param {URL} url
@@ -38,8 +53,8 @@ export function get_cookies(request, url) {
3853
/** @type {string | undefined} */
3954
let normalized_url;
4055

41-
/** @type {Record<string, import('./page/types.js').Cookie>} */
42-
const new_cookies = {};
56+
/** @type {Map<string, import('./page/types.js').Cookie>} */
57+
const new_cookies = new Map();
4358

4459
/** @type {import('cookie').CookieSerializeOptions} */
4560
const defaults = {
@@ -60,13 +75,19 @@ export function get_cookies(request, url) {
6075
* @param {import('cookie').CookieParseOptions} [opts]
6176
*/
6277
get(name, opts) {
63-
const c = new_cookies[name];
64-
if (
65-
c &&
66-
domain_matches(url.hostname, c.options.domain) &&
67-
path_matches(url.pathname, c.options.path)
68-
) {
69-
return c.value;
78+
// Look for the most specific matching cookie from new_cookies
79+
const best_match = Array.from(new_cookies.values())
80+
.filter((c) => {
81+
return (
82+
c.name === name &&
83+
domain_matches(url.hostname, c.options.domain) &&
84+
path_matches(url.pathname, c.options.path)
85+
);
86+
})
87+
.sort((a, b) => b.options.path.length - a.options.path.length)[0];
88+
89+
if (best_match) {
90+
return best_match.options.maxAge === 0 ? undefined : best_match.value;
7091
}
7192

7293
const req_cookies = parse(header, { decode: opts?.decode });
@@ -98,15 +119,28 @@ export function get_cookies(request, url) {
98119
getAll(opts) {
99120
const cookies = parse(header, { decode: opts?.decode });
100121

101-
for (const c of Object.values(new_cookies)) {
122+
// Group cookies by name and find the most specific one for each name
123+
const lookup = new Map();
124+
125+
for (const c of new_cookies.values()) {
102126
if (
103127
domain_matches(url.hostname, c.options.domain) &&
104128
path_matches(url.pathname, c.options.path)
105129
) {
106-
cookies[c.name] = c.value;
130+
const existing = lookup.get(c.name);
131+
132+
// If no existing cookie or this one has a more specific (longer) path, use this one
133+
if (!existing || c.options.path.length > existing.options.path.length) {
134+
lookup.set(c.name, c);
135+
}
107136
}
108137
}
109138

139+
// Add the most specific cookies to the result
140+
for (const c of lookup.values()) {
141+
cookies[c.name] = c.value;
142+
}
143+
110144
return Object.entries(cookies).map(([name, value]) => ({ name, value }));
111145
},
112146

@@ -172,8 +206,7 @@ export function get_cookies(request, url) {
172206
};
173207

174208
// cookies previous set during this event with cookies.set have higher precedence
175-
for (const key in new_cookies) {
176-
const cookie = new_cookies[key];
209+
for (const cookie of new_cookies.values()) {
177210
if (!domain_matches(destination.hostname, cookie.options.domain)) continue;
178211
if (!path_matches(destination.pathname, cookie.options.path)) continue;
179212

@@ -214,10 +247,13 @@ export function get_cookies(request, url) {
214247
path = resolve(normalized_url, path);
215248
}
216249

217-
new_cookies[name] = { name, value, options: { ...options, path } };
250+
// Generate unique key for cookie storage
251+
const cookie_key = generate_cookie_key(options.domain, path, name);
252+
const cookie = { name, value, options: { ...options, path } };
253+
new_cookies.set(cookie_key, cookie);
218254

219255
if (__SVELTEKIT_DEV__) {
220-
const serialized = serialize(name, value, new_cookies[name].options);
256+
const serialized = serialize(name, value, cookie.options);
221257
if (text_encoder.encode(serialized).byteLength > MAX_COOKIE_SIZE) {
222258
throw new Error(`Cookie "${name}" is too large, and will be discarded by the browser`);
223259
}
@@ -271,7 +307,7 @@ export function path_matches(path, constraint) {
271307

272308
/**
273309
* @param {Headers} headers
274-
* @param {import('./page/types.js').Cookie[]} cookies
310+
* @param {MapIterator<import('./page/types.js').Cookie>} cookies
275311
*/
276312
export function add_cookies_to_headers(headers, cookies) {
277313
for (const new_cookie of cookies) {

‎packages/kit/src/runtime/server/cookie.spec.js

Lines changed: 83 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -63,13 +63,13 @@ test('a cookie should not be present after it is deleted', () => {
6363
cookies.set('a', 'b', { path: '/' });
6464
expect(cookies.get('a')).toEqual('b');
6565
cookies.delete('a', { path: '/' });
66-
assert.isNotOk(cookies.get('a'));
66+
assert.isUndefined(cookies.get('a'));
6767
});
6868

6969
test('default values when set is called', () => {
7070
const { cookies, new_cookies } = cookies_setup();
7171
cookies.set('a', 'b', { path: '/' });
72-
const opts = new_cookies['a']?.options;
72+
const opts = new_cookies.get('/?a')?.options;
7373
assert.equal(opts?.secure, true);
7474
assert.equal(opts?.httpOnly, true);
7575
assert.equal(opts?.path, '/');
@@ -79,7 +79,7 @@ test('default values when set is called', () => {
7979
test('default values when set is called on sub path', () => {
8080
const { cookies, new_cookies } = cookies_setup({ href: 'https://example.com/foo/bar' });
8181
cookies.set('a', 'b', { path: '' });
82-
const opts = new_cookies['a']?.options;
82+
const opts = new_cookies.get('/foo/bar?a')?.options;
8383
assert.equal(opts?.secure, true);
8484
assert.equal(opts?.httpOnly, true);
8585
assert.equal(opts?.path, '/foo/bar');
@@ -89,14 +89,14 @@ test('default values when set is called on sub path', () => {
8989
test('default values when on localhost', () => {
9090
const { cookies, new_cookies } = cookies_setup({ href: 'http://localhost:1234' });
9191
cookies.set('a', 'b', { path: '/' });
92-
const opts = new_cookies['a']?.options;
92+
const opts = new_cookies.get('/?a')?.options;
9393
assert.equal(opts?.secure, false);
9494
});
9595

9696
test('overridden defaults when set is called', () => {
9797
const { cookies, new_cookies } = cookies_setup();
9898
cookies.set('a', 'b', { secure: false, httpOnly: false, sameSite: 'strict', path: '/a/b/c' });
99-
const opts = new_cookies['a']?.options;
99+
const opts = new_cookies.get('/a/b/c?a')?.options;
100100
assert.equal(opts?.secure, false);
101101
assert.equal(opts?.httpOnly, false);
102102
assert.equal(opts?.path, '/a/b/c');
@@ -106,7 +106,7 @@ test('overridden defaults when set is called', () => {
106106
test('default values when delete is called', () => {
107107
const { cookies, new_cookies } = cookies_setup();
108108
cookies.delete('a', { path: '/' });
109-
const opts = new_cookies['a']?.options;
109+
const opts = new_cookies.get('/?a')?.options;
110110
assert.equal(opts?.secure, true);
111111
assert.equal(opts?.httpOnly, true);
112112
assert.equal(opts?.path, '/');
@@ -117,7 +117,7 @@ test('default values when delete is called', () => {
117117
test('overridden defaults when delete is called', () => {
118118
const { cookies, new_cookies } = cookies_setup();
119119
cookies.delete('a', { secure: false, httpOnly: false, sameSite: 'strict', path: '/a/b/c' });
120-
const opts = new_cookies['a']?.options;
120+
const opts = new_cookies.get('/a/b/c?a')?.options;
121121
assert.equal(opts?.secure, false);
122122
assert.equal(opts?.httpOnly, false);
123123
assert.equal(opts?.path, '/a/b/c');
@@ -128,15 +128,15 @@ test('overridden defaults when delete is called', () => {
128128
test('cannot override maxAge on delete', () => {
129129
const { cookies, new_cookies } = cookies_setup();
130130
cookies.delete('a', { path: '/', maxAge: 1234 });
131-
const opts = new_cookies['a']?.options;
131+
const opts = new_cookies.get('/?a')?.options;
132132
assert.equal(opts?.maxAge, 0);
133133
});
134134

135135
test('last cookie set with the same name wins', () => {
136136
const { cookies, new_cookies } = cookies_setup();
137137
cookies.set('a', 'foo', { path: '/' });
138138
cookies.set('a', 'bar', { path: '/' });
139-
const entry = new_cookies['a'];
139+
const entry = new_cookies.get('/?a');
140140
assert.equal(entry?.value, 'bar');
141141
});
142142

@@ -145,8 +145,8 @@ test('cookie names are case sensitive', () => {
145145
// not that one should do this, but we follow the spec...
146146
cookies.set('a', 'foo', { path: '/' });
147147
cookies.set('A', 'bar', { path: '/' });
148-
const entrya = new_cookies['a'];
149-
const entryA = new_cookies['A'];
148+
const entrya = new_cookies.get('/?a');
149+
const entryA = new_cookies.get('/?A');
150150
assert.equal(entrya?.value, 'foo');
151151
assert.equal(entryA?.value, 'bar');
152152
});
@@ -211,5 +211,76 @@ test("set_internal isn't affected by defaults", () => {
211211
set_internal('test', 'foo', options);
212212

213213
expect(cookies.get('test')).toEqual('foo');
214-
expect(new_cookies['test']?.options).toEqual(options);
214+
expect(new_cookies.get('/a/b/c?test')?.options).toEqual(options);
215+
});
216+
217+
test('reproduce issue #13947: multiple cookies with same name but different paths', () => {
218+
// Test on root path to see if most specific cookie wins
219+
const { cookies: root_cookies } = cookies_setup({ href: 'https://example.com/' });
220+
root_cookies.set('key', 'value_root', { path: '/' });
221+
root_cookies.set('key', 'value_foo', { path: '/foo' });
222+
223+
// When on root path, should get the root cookie
224+
expect(root_cookies.get('key')).toEqual('value_root');
225+
226+
// Test on /foo path to see if more specific cookie wins
227+
const { cookies: foo_cookies } = cookies_setup({ href: 'https://example.com/foo' });
228+
foo_cookies.set('key', 'value_root', { path: '/' });
229+
foo_cookies.set('key', 'value_foo', { path: '/foo' });
230+
231+
// When on /foo path, should get the more specific /foo cookie
232+
expect(foo_cookies.get('key')).toEqual('value_foo');
233+
});
234+
235+
test('cookies with same name but different domains work correctly', () => {
236+
// Test setting cookies with different domains (unique key storage should work)
237+
const { cookies, new_cookies } = cookies_setup({ href: 'https://example.com/' });
238+
239+
cookies.set('key', 'value1', { path: '/', domain: 'example.com' });
240+
cookies.set('key', 'value2', { path: '/', domain: 'sub.example.com' });
241+
242+
// Both cookies should be stored with unique keys
243+
expect(new_cookies.get('example.com/?key')).toBeDefined();
244+
expect(new_cookies.get('sub.example.com/?key')).toBeDefined();
245+
expect(new_cookies.get('example.com/?key')?.value).toEqual('value1');
246+
expect(new_cookies.get('sub.example.com/?key')?.value).toEqual('value2');
247+
});
248+
249+
test('cookie path specificity: more specific paths win', () => {
250+
const { cookies } = cookies_setup({ href: 'https://example.com/x/y/z' });
251+
252+
// Set cookies with increasing path specificity
253+
cookies.set('n', '1', { path: '/' });
254+
cookies.set('n', '2', { path: '/x' });
255+
cookies.set('n', '3', { path: '/x/y' });
256+
cookies.set('n', '4', { path: '/x/y/z' });
257+
258+
// Most specific path should win
259+
expect(cookies.get('n')).toEqual('4');
260+
});
261+
262+
test('backward compatibility: get without domain/path options still works', () => {
263+
const { cookies } = cookies_setup();
264+
265+
// Set a cookie the old way
266+
cookies.set('old-style', 'value', { path: '/' });
267+
268+
// Should be retrievable without specifying path
269+
expect(cookies.get('old-style')).toEqual('value');
270+
});
271+
272+
test('getAll should return most specific cookie when multiple cookies have same name', () => {
273+
// This test demonstrates the bug: getAll doesn't respect path specificity like get() does
274+
const { cookies } = cookies_setup({ href: 'https://example.com/foo/bar' });
275+
276+
// Set cookies with the same name but different path specificity
277+
// Setting most specific first, then less specific ones to expose the bug
278+
cookies.set('duplicate', 'foobar_value', { path: '/foo/bar' }); // Most specific
279+
cookies.set('duplicate', 'root_value', { path: '/' }); // Least specific
280+
cookies.set('duplicate', 'foo_value', { path: '/foo' }); // Middle specificity
281+
282+
const all = cookies.getAll();
283+
const duplicate = all.find((c) => c.name === 'duplicate');
284+
285+
expect(duplicate?.value).toEqual('foobar_value');
215286
});

‎packages/kit/src/runtime/server/respond.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -432,7 +432,7 @@ export async function internal_respond(request, options, manifest, state) {
432432
response.headers.set(key, /** @type {string} */ (value));
433433
}
434434

435-
add_cookies_to_headers(response.headers, Object.values(new_cookies));
435+
add_cookies_to_headers(response.headers, new_cookies.values());
436436

437437
if (state.prerendering && event.route.id !== null) {
438438
response.headers.set('x-sveltekit-routeid', encodeURI(event.route.id));
@@ -507,7 +507,7 @@ export async function internal_respond(request, options, manifest, state) {
507507
: route?.page && is_action_json_request(event)
508508
? action_json_redirect(e)
509509
: redirect_response(e.status, e.location);
510-
add_cookies_to_headers(response.headers, Object.values(new_cookies));
510+
add_cookies_to_headers(response.headers, new_cookies.values());
511511
return response;
512512
}
513513
return await handle_fatal_error(event, event_state, options, e);

0 commit comments

Comments
 (0)