Skip to content

Commit

Permalink
chore: deprecate pathless cookie, warn on path: '' (#11237)
Browse files Browse the repository at this point in the history
This prints a warning if you call cookies.set(...) or cookies.delete(...) without specifying a path, as part of #9299.
  • Loading branch information
Rich-Harris authored Dec 9, 2023
1 parent 24a16ce commit ded1630
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 16 deletions.
5 changes: 5 additions & 0 deletions .changeset/real-carrots-share.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

chore: deprecate cookies.set/delete without path option
26 changes: 26 additions & 0 deletions packages/kit/src/runtime/server/cookie.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { parse, serialize } from 'cookie';
import { normalize_path } from '../../utils/url.js';
import { warn_with_callsite } from './utils.js';

/**
* Tracks all cookies set during dev mode so we can emit warnings
Expand All @@ -14,6 +15,27 @@ const cookie_paths = {};
*/
const MAX_COOKIE_SIZE = 4129;

/**
*
* @param {import('cookie').CookieSerializeOptions} opts
* @param {'set' | 'delete'} method
*/
function deprecate_missing_path(opts, method) {
if (opts.path === undefined) {
warn_with_callsite(
`Calling \`cookies.${method}}(...)\` without specifying a \`path\` is deprecated, and will be disallowed in SvelteKit 2.0. Relative paths can be used`,
1
);
}

if (opts.path === '') {
warn_with_callsite(
`Calling \`cookies.${method}(...)\` with \`path: ''\` will behave differently in SvelteKit 2.0. Instead of using the browser default behaviour, it will set the cookie path to the current pathname`,
1
);
}
}

/**
* @param {Request} request
* @param {URL} url
Expand Down Expand Up @@ -107,6 +129,7 @@ export function get_cookies(request, url, trailing_slash) {
* @param {import('cookie').CookieSerializeOptions} opts
*/
set(name, value, opts = {}) {
deprecate_missing_path(opts, 'set');
set_internal(name, value, { ...defaults, ...opts });
},

Expand All @@ -115,7 +138,10 @@ export function get_cookies(request, url, trailing_slash) {
* @param {import('cookie').CookieSerializeOptions} opts
*/
delete(name, opts = {}) {
deprecate_missing_path(opts, 'delete');

cookies.set(name, '', {
path: default_path, // TODO 2.0 remove this
...opts,
maxAge: 0
});
Expand Down
32 changes: 16 additions & 16 deletions packages/kit/src/runtime/server/cookie.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,15 @@ const cookies_setup = ({ href, headers } = {}) => {

test('a cookie should not be present after it is deleted', () => {
const { cookies } = cookies_setup();
cookies.set('a', 'b');
cookies.set('a', 'b', { path: '/' });
expect(cookies.get('a')).toEqual('b');
cookies.delete('a');
cookies.delete('a', { path: '/' });
assert.isNotOk(cookies.get('a'));
});

test('default values when set is called', () => {
const { cookies, new_cookies } = cookies_setup();
cookies.set('a', 'b');
cookies.set('a', 'b', { path: '/' });
const opts = new_cookies['a']?.options;
assert.equal(opts?.secure, true);
assert.equal(opts?.httpOnly, true);
Expand All @@ -86,7 +86,7 @@ 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');
cookies.set('a', 'b', { path: '/' });
const opts = new_cookies['a']?.options;
assert.equal(opts?.secure, false);
});
Expand All @@ -103,7 +103,7 @@ test('overridden defaults when set is called', () => {

test('default values when delete is called', () => {
const { cookies, new_cookies } = cookies_setup();
cookies.delete('a');
cookies.delete('a', { path: '/' });
const opts = new_cookies['a']?.options;
assert.equal(opts?.secure, true);
assert.equal(opts?.httpOnly, true);
Expand All @@ -125,24 +125,24 @@ test('overridden defaults when delete is called', () => {

test('cannot override maxAge on delete', () => {
const { cookies, new_cookies } = cookies_setup();
cookies.delete('a', { maxAge: 1234 });
cookies.delete('a', { path: '/', maxAge: 1234 });
const opts = new_cookies['a']?.options;
assert.equal(opts?.maxAge, 0);
});

test('last cookie set with the same name wins', () => {
const { cookies, new_cookies } = cookies_setup();
cookies.set('a', 'foo');
cookies.set('a', 'bar');
cookies.set('a', 'foo', { path: '/' });
cookies.set('a', 'bar', { path: '/' });
const entry = new_cookies['a'];
assert.equal(entry?.value, 'bar');
});

test('cookie names are case sensitive', () => {
const { cookies, new_cookies } = cookies_setup();
// not that one should do this, but we follow the spec...
cookies.set('a', 'foo');
cookies.set('A', 'bar');
cookies.set('a', 'foo', { path: '/' });
cookies.set('A', 'bar', { path: '/' });
const entrya = new_cookies['a'];
const entryA = new_cookies['A'];
assert.equal(entrya?.value, 'foo');
Expand All @@ -157,8 +157,8 @@ test('serialized cookie header should be url-encoded', () => {
cookie: 'a=f%C3%BC; b=foo+bar' // a=fü
}
});
cookies.set('c', 'fö'); // should use default encoding
cookies.set('d', 'fö', { encode: () => 'öf' }); // should respect `encode`
cookies.set('c', 'fö', { path: '/' }); // should use default encoding
cookies.set('d', 'fö', { path: '/', encode: () => 'öf' }); // should respect `encode`
const header = get_cookie_header(new URL(href), 'e=f%C3%A4; f=foo+bar');
assert.equal(header, 'a=f%C3%BC; b=foo+bar; c=f%C3%B6; d=öf; e=f%C3%A4; f=foo+bar');
});
Expand All @@ -169,7 +169,7 @@ test('warns if cookie exceeds 4,129 bytes', () => {

try {
const { cookies } = cookies_setup();
cookies.set('a', 'a'.repeat(4097));
cookies.set('a', 'a'.repeat(4097), { path: '/' });
} catch (e) {
const error = /** @type {Error} */ (e);

Expand All @@ -184,9 +184,9 @@ test('get all cookies from header and set calls', () => {
const { cookies } = cookies_setup();
expect(cookies.getAll()).toEqual([{ name: 'a', value: 'b' }]);

cookies.set('a', 'foo');
cookies.set('a', 'bar');
cookies.set('b', 'baz');
cookies.set('a', 'foo', { path: '/' });
cookies.set('a', 'bar', { path: '/' });
cookies.set('b', 'baz', { path: '/' });

expect(cookies.getAll()).toEqual([
{ name: 'a', value: 'bar' },
Expand Down
14 changes: 14 additions & 0 deletions packages/kit/src/runtime/server/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,3 +159,17 @@ export function stringify_uses(node) {

return `"uses":{${uses.join(',')}}`;
}

/**
* @param {string} message
* @param {number} offset
*/
export function warn_with_callsite(message, offset = 0) {
if (DEV) {
const stack = fix_stack_trace(new Error()).split('\n');
const line = stack.at(3 + offset);
message += `\n${line}`;
}

console.warn(message);
}

0 comments on commit ded1630

Please sign in to comment.