diff --git a/.changeset/fix-multiple-cookies-same-name.md b/.changeset/fix-multiple-cookies-same-name.md new file mode 100644 index 000000000000..7293fdb77c4c --- /dev/null +++ b/.changeset/fix-multiple-cookies-same-name.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: support multiple cookies with the same name across different paths and domains diff --git a/.changeset/seven-cougars-train.md b/.changeset/seven-cougars-train.md new file mode 100644 index 000000000000..9ab470828f9e --- /dev/null +++ b/.changeset/seven-cougars-train.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: `cookies.get(...)` returns `undefined` for a just-deleted cookie diff --git a/packages/kit/src/runtime/server/cookie.js b/packages/kit/src/runtime/server/cookie.js index 2e683543a534..b1585c13aa46 100644 --- a/packages/kit/src/runtime/server/cookie.js +++ b/packages/kit/src/runtime/server/cookie.js @@ -26,6 +26,21 @@ function validate_options(options) { } } +/** + * Generates a unique key for a cookie based on its domain, path, and name in + * the format: `/?`. + * If domain is undefined, it will be omitted. + * For example: `/?name`, `example.com/foo?name`. + * + * @param {string | undefined} domain + * @param {string} path + * @param {string} name + * @returns {string} + */ +function generate_cookie_key(domain, path, name) { + return `${domain || ''}${path}?${name}`; +} + /** * @param {Request} request * @param {URL} url @@ -37,8 +52,8 @@ export function get_cookies(request, url) { /** @type {string | undefined} */ let normalized_url; - /** @type {Record} */ - const new_cookies = {}; + /** @type {Map} */ + const new_cookies = new Map(); /** @type {import('cookie').CookieSerializeOptions} */ const defaults = { @@ -59,13 +74,19 @@ export function get_cookies(request, url) { * @param {import('cookie').CookieParseOptions} [opts] */ get(name, opts) { - const c = new_cookies[name]; - if ( - c && - domain_matches(url.hostname, c.options.domain) && - path_matches(url.pathname, c.options.path) - ) { - return c.value; + // Look for the most specific matching cookie from new_cookies + const best_match = Array.from(new_cookies.values()) + .filter((c) => { + return ( + c.name === name && + domain_matches(url.hostname, c.options.domain) && + path_matches(url.pathname, c.options.path) + ); + }) + .sort((a, b) => b.options.path.length - a.options.path.length)[0]; + + if (best_match) { + return best_match.value || undefined; } const req_cookies = parse(header, { decode: opts?.decode }); @@ -97,15 +118,28 @@ export function get_cookies(request, url) { getAll(opts) { const cookies = parse(header, { decode: opts?.decode }); - for (const c of Object.values(new_cookies)) { + // Group cookies by name and find the most specific one for each name + const lookup = new Map(); + + for (const c of new_cookies.values()) { if ( domain_matches(url.hostname, c.options.domain) && path_matches(url.pathname, c.options.path) ) { - cookies[c.name] = c.value; + const existing = lookup.get(c.name); + + // If no existing cookie or this one has a more specific (longer) path, use this one + if (!existing || c.options.path.length > existing.options.path.length) { + lookup.set(c.name, c); + } } } + // Add the most specific cookies to the result + for (const c of lookup.values()) { + cookies[c.name] = c.value; + } + return Object.entries(cookies).map(([name, value]) => ({ name, value })); }, @@ -171,8 +205,7 @@ export function get_cookies(request, url) { }; // cookies previous set during this event with cookies.set have higher precedence - for (const key in new_cookies) { - const cookie = new_cookies[key]; + for (const cookie of new_cookies.values()) { if (!domain_matches(destination.hostname, cookie.options.domain)) continue; if (!path_matches(destination.pathname, cookie.options.path)) continue; @@ -213,10 +246,13 @@ export function get_cookies(request, url) { path = resolve(normalized_url, path); } - new_cookies[name] = { name, value, options: { ...options, path } }; + // Generate unique key for cookie storage + const cookie_key = generate_cookie_key(options.domain, path, name); + const cookie = { name, value, options: { ...options, path } }; + new_cookies.set(cookie_key, cookie); if (__SVELTEKIT_DEV__) { - const serialized = serialize(name, value, new_cookies[name].options); + const serialized = serialize(name, value, cookie.options); if (new TextEncoder().encode(serialized).byteLength > MAX_COOKIE_SIZE) { throw new Error(`Cookie "${name}" is too large, and will be discarded by the browser`); } @@ -270,7 +306,7 @@ export function path_matches(path, constraint) { /** * @param {Headers} headers - * @param {import('./page/types.js').Cookie[]} cookies + * @param {MapIterator} cookies */ export function add_cookies_to_headers(headers, cookies) { for (const new_cookie of cookies) { diff --git a/packages/kit/src/runtime/server/cookie.spec.js b/packages/kit/src/runtime/server/cookie.spec.js index 8d98bcc3c283..c5cd90814a4c 100644 --- a/packages/kit/src/runtime/server/cookie.spec.js +++ b/packages/kit/src/runtime/server/cookie.spec.js @@ -63,13 +63,13 @@ test('a cookie should not be present after it is deleted', () => { cookies.set('a', 'b', { path: '/' }); expect(cookies.get('a')).toEqual('b'); cookies.delete('a', { path: '/' }); - assert.isNotOk(cookies.get('a')); + assert.isUndefined(cookies.get('a')); }); test('default values when set is called', () => { const { cookies, new_cookies } = cookies_setup(); cookies.set('a', 'b', { path: '/' }); - const opts = new_cookies['a']?.options; + const opts = new_cookies.get('/?a')?.options; assert.equal(opts?.secure, true); assert.equal(opts?.httpOnly, true); assert.equal(opts?.path, '/'); @@ -79,7 +79,7 @@ test('default values when set is called', () => { test('default values when set is called on sub path', () => { const { cookies, new_cookies } = cookies_setup({ href: 'https://example.com/foo/bar' }); cookies.set('a', 'b', { path: '' }); - const opts = new_cookies['a']?.options; + const opts = new_cookies.get('/foo/bar?a')?.options; assert.equal(opts?.secure, true); assert.equal(opts?.httpOnly, true); assert.equal(opts?.path, '/foo/bar'); @@ -89,14 +89,14 @@ test('default values when set is called on sub path', () => { test('default values when on localhost', () => { const { cookies, new_cookies } = cookies_setup({ href: 'http://localhost:1234' }); cookies.set('a', 'b', { path: '/' }); - const opts = new_cookies['a']?.options; + const opts = new_cookies.get('/?a')?.options; assert.equal(opts?.secure, false); }); test('overridden defaults when set is called', () => { const { cookies, new_cookies } = cookies_setup(); cookies.set('a', 'b', { secure: false, httpOnly: false, sameSite: 'strict', path: '/a/b/c' }); - const opts = new_cookies['a']?.options; + const opts = new_cookies.get('/a/b/c?a')?.options; assert.equal(opts?.secure, false); assert.equal(opts?.httpOnly, false); assert.equal(opts?.path, '/a/b/c'); @@ -106,7 +106,7 @@ test('overridden defaults when set is called', () => { test('default values when delete is called', () => { const { cookies, new_cookies } = cookies_setup(); cookies.delete('a', { path: '/' }); - const opts = new_cookies['a']?.options; + const opts = new_cookies.get('/?a')?.options; assert.equal(opts?.secure, true); assert.equal(opts?.httpOnly, true); assert.equal(opts?.path, '/'); @@ -117,7 +117,7 @@ test('default values when delete is called', () => { test('overridden defaults when delete is called', () => { const { cookies, new_cookies } = cookies_setup(); cookies.delete('a', { secure: false, httpOnly: false, sameSite: 'strict', path: '/a/b/c' }); - const opts = new_cookies['a']?.options; + const opts = new_cookies.get('/a/b/c?a')?.options; assert.equal(opts?.secure, false); assert.equal(opts?.httpOnly, false); assert.equal(opts?.path, '/a/b/c'); @@ -128,7 +128,7 @@ test('overridden defaults when delete is called', () => { test('cannot override maxAge on delete', () => { const { cookies, new_cookies } = cookies_setup(); cookies.delete('a', { path: '/', maxAge: 1234 }); - const opts = new_cookies['a']?.options; + const opts = new_cookies.get('/?a')?.options; assert.equal(opts?.maxAge, 0); }); @@ -136,7 +136,7 @@ test('last cookie set with the same name wins', () => { const { cookies, new_cookies } = cookies_setup(); cookies.set('a', 'foo', { path: '/' }); cookies.set('a', 'bar', { path: '/' }); - const entry = new_cookies['a']; + const entry = new_cookies.get('/?a'); assert.equal(entry?.value, 'bar'); }); @@ -145,8 +145,8 @@ test('cookie names are case sensitive', () => { // not that one should do this, but we follow the spec... cookies.set('a', 'foo', { path: '/' }); cookies.set('A', 'bar', { path: '/' }); - const entrya = new_cookies['a']; - const entryA = new_cookies['A']; + const entrya = new_cookies.get('/?a'); + const entryA = new_cookies.get('/?A'); assert.equal(entrya?.value, 'foo'); assert.equal(entryA?.value, 'bar'); }); @@ -211,5 +211,76 @@ test("set_internal isn't affected by defaults", () => { set_internal('test', 'foo', options); expect(cookies.get('test')).toEqual('foo'); - expect(new_cookies['test']?.options).toEqual(options); + expect(new_cookies.get('/a/b/c?test')?.options).toEqual(options); +}); + +test('reproduce issue #13947: multiple cookies with same name but different paths', () => { + // Test on root path to see if most specific cookie wins + const { cookies: root_cookies } = cookies_setup({ href: 'https://example.com/' }); + root_cookies.set('key', 'value_root', { path: '/' }); + root_cookies.set('key', 'value_foo', { path: '/foo' }); + + // When on root path, should get the root cookie + expect(root_cookies.get('key')).toEqual('value_root'); + + // Test on /foo path to see if more specific cookie wins + const { cookies: foo_cookies } = cookies_setup({ href: 'https://example.com/foo' }); + foo_cookies.set('key', 'value_root', { path: '/' }); + foo_cookies.set('key', 'value_foo', { path: '/foo' }); + + // When on /foo path, should get the more specific /foo cookie + expect(foo_cookies.get('key')).toEqual('value_foo'); +}); + +test('cookies with same name but different domains work correctly', () => { + // Test setting cookies with different domains (unique key storage should work) + const { cookies, new_cookies } = cookies_setup({ href: 'https://example.com/' }); + + cookies.set('key', 'value1', { path: '/', domain: 'example.com' }); + cookies.set('key', 'value2', { path: '/', domain: 'sub.example.com' }); + + // Both cookies should be stored with unique keys + expect(new_cookies.get('example.com/?key')).toBeDefined(); + expect(new_cookies.get('sub.example.com/?key')).toBeDefined(); + expect(new_cookies.get('example.com/?key')?.value).toEqual('value1'); + expect(new_cookies.get('sub.example.com/?key')?.value).toEqual('value2'); +}); + +test('cookie path specificity: more specific paths win', () => { + const { cookies } = cookies_setup({ href: 'https://example.com/x/y/z' }); + + // Set cookies with increasing path specificity + cookies.set('n', '1', { path: '/' }); + cookies.set('n', '2', { path: '/x' }); + cookies.set('n', '3', { path: '/x/y' }); + cookies.set('n', '4', { path: '/x/y/z' }); + + // Most specific path should win + expect(cookies.get('n')).toEqual('4'); +}); + +test('backward compatibility: get without domain/path options still works', () => { + const { cookies } = cookies_setup(); + + // Set a cookie the old way + cookies.set('old-style', 'value', { path: '/' }); + + // Should be retrievable without specifying path + expect(cookies.get('old-style')).toEqual('value'); +}); + +test('getAll should return most specific cookie when multiple cookies have same name', () => { + // This test demonstrates the bug: getAll doesn't respect path specificity like get() does + const { cookies } = cookies_setup({ href: 'https://example.com/foo/bar' }); + + // Set cookies with the same name but different path specificity + // Setting most specific first, then less specific ones to expose the bug + cookies.set('duplicate', 'foobar_value', { path: '/foo/bar' }); // Most specific + cookies.set('duplicate', 'root_value', { path: '/' }); // Least specific + cookies.set('duplicate', 'foo_value', { path: '/foo' }); // Middle specificity + + const all = cookies.getAll(); + const duplicate = all.find((c) => c.name === 'duplicate'); + + expect(duplicate?.value).toEqual('foobar_value'); }); diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js index c14f37ab6406..bea8469ef853 100644 --- a/packages/kit/src/runtime/server/respond.js +++ b/packages/kit/src/runtime/server/respond.js @@ -394,7 +394,7 @@ export async function respond(request, options, manifest, state) { response.headers.set(key, /** @type {string} */ (value)); } - add_cookies_to_headers(response.headers, Object.values(new_cookies)); + add_cookies_to_headers(response.headers, new_cookies.values()); if (state.prerendering && event.route.id !== null) { response.headers.set('x-sveltekit-routeid', encodeURI(event.route.id)); @@ -457,7 +457,7 @@ export async function respond(request, options, manifest, state) { : route?.page && is_action_json_request(event) ? action_json_redirect(e) : redirect_response(e.status, e.location); - add_cookies_to_headers(response.headers, Object.values(new_cookies)); + add_cookies_to_headers(response.headers, new_cookies.values()); return response; } return await handle_fatal_error(event, options, e);