Skip to content

Commit

Permalink
fix: Don't set defaults when parsing cookies from headers (#9908)
Browse files Browse the repository at this point in the history
Closes #9901

Bypasses setting Kit defaults on cookies parsed from fetch response headers. To illustrate what the problem was:

- Set a cookie in +server.ts: cookies.set('foo', 'bar', { httpOnly: false })
- fetch this endpoint from +page.server.ts
- As part of fetch, parse the cookies from the headers
- httpOnly isn't set (because false === unset)
- Adding this cookie to the cookies causes the default httpOnly: true to be added

The better solution here is not to apply defaults to cookies we parse from fetch response headers. If these cookies were set through cookies.set in a +server.ts endpoint, we've already set the defaults, and if they're not, we shouldn't touch them anyway.
  • Loading branch information
1 parent bc70b4e commit 26d2b7f
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 31 deletions.
5 changes: 5 additions & 0 deletions .changeset/olive-dogs-bathe.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

fix: stop setting Kit cookie defaults on cookies parsed from headers
62 changes: 35 additions & 27 deletions packages/kit/src/runtime/server/cookie.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,32 +107,7 @@ export function get_cookies(request, url, trailing_slash) {
* @param {import('cookie').CookieSerializeOptions} opts
*/
set(name, value, opts = {}) {
let path = opts.path ?? default_path;

new_cookies[name] = {
name,
value,
options: {
...defaults,
...opts,
path
}
};

if (__SVELTEKIT_DEV__) {
const serialized = serialize(name, value, new_cookies[name].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`);
}

cookie_paths[name] ??= new Set();

if (!value) {
cookie_paths[name].delete(path);
} else {
cookie_paths[name].add(path);
}
}
set_internal(name, value, { ...defaults, ...opts });
},

/**
Expand Down Expand Up @@ -193,7 +168,40 @@ export function get_cookies(request, url, trailing_slash) {
.join('; ');
}

return { cookies, new_cookies, get_cookie_header };
/**
* @param {string} name
* @param {string} value
* @param {import('cookie').CookieSerializeOptions} opts
*/
function set_internal(name, value, opts) {
let path = opts.path ?? default_path;

new_cookies[name] = {
name,
value,
options: {
...opts,
path
}
};

if (__SVELTEKIT_DEV__) {
const serialized = serialize(name, value, new_cookies[name].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`);
}

cookie_paths[name] ??= new Set();

if (!value) {
cookie_paths[name].delete(path);
} else {
cookie_paths[name].add(path);
}
}
}

return { cookies, new_cookies, get_cookie_header, set_internal };
}

/**
Expand Down
15 changes: 15 additions & 0 deletions packages/kit/src/runtime/server/cookie.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,4 +195,19 @@ test('get all cookies from header and set calls', () => {
]);
});

test("set_internal isn't affected by defaults", () => {
const { cookies, new_cookies, set_internal } = cookies_setup({
href: 'https://example.com/a/b/c'
});
const options = /** @type {const} */ ({
secure: false,
httpOnly: false,
sameSite: 'none',
path: '/a/b/c'
});
set_internal('test', 'foo', options);
assert.equal(cookies.get('test'), 'foo');
assert.equal(new_cookies['test']?.options, options);
});

test.run();
5 changes: 3 additions & 2 deletions packages/kit/src/runtime/server/fetch.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@ import * as paths from '__sveltekit/paths';
* manifest: import('types').SSRManifest;
* state: import('types').SSRState;
* get_cookie_header: (url: URL, header: string | null) => string;
* set_internal: (name: string, value: string, opts: import('cookie').CookieSerializeOptions) => void;
* }} opts
* @returns {typeof fetch}
*/
export function create_fetch({ event, options, manifest, state, get_cookie_header }) {
export function create_fetch({ event, options, manifest, state, get_cookie_header, set_internal }) {
return async (info, init) => {
const original_request = normalize_fetch_input(info, init, event.url);

Expand Down Expand Up @@ -131,7 +132,7 @@ export function create_fetch({ event, options, manifest, state, get_cookie_heade
const { name, value, ...options } = set_cookie_parser.parseString(str);

// options.sameSite is string, something more specific is required - type cast is safe
event.cookies.set(
set_internal(
name,
value,
/** @type {import('cookie').CookieSerializeOptions} */ (options)
Expand Down
11 changes: 9 additions & 2 deletions packages/kit/src/runtime/server/respond.js
Original file line number Diff line number Diff line change
Expand Up @@ -244,15 +244,22 @@ export async function respond(request, options, manifest, state) {
}
}

const { cookies, new_cookies, get_cookie_header } = get_cookies(
const { cookies, new_cookies, get_cookie_header, set_internal } = get_cookies(
request,
url,
trailing_slash ?? 'never'
);

cookies_to_add = new_cookies;
event.cookies = cookies;
event.fetch = create_fetch({ event, options, manifest, state, get_cookie_header });
event.fetch = create_fetch({
event,
options,
manifest,
state,
get_cookie_header,
set_internal
});

if (state.prerendering && !state.prerendering.fallback) disable_search(url);

Expand Down

0 comments on commit 26d2b7f

Please sign in to comment.