[fix] better cookie warning checks (#7528)

* [fix] better cookie warning checks

Fixes #7510

* tweak tests to not pollute logs with (correct) cookie warnings

* Apply suggestions from code review

Co-authored-by: Kaeso <24925519+ptlthg@users.noreply.github.com>

Co-authored-by: Rich Harris <richard.a.harris@gmail.com>
Co-authored-by: Kaeso <24925519+ptlthg@users.noreply.github.com>

.changeset/violet-candles-compare.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+[fix] better cookie warning checks

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

@@ -15,9 +15,35 @@ const cookie_paths = {};
  */
 export function get_cookies(request, url, options) {
 	const header = request.headers.get('cookie') ?? '';
-
 	const initial_cookies = parse(header);
 
+	const normalized_url = normalize_path(
+		// Remove suffix: 'foo/__data.json' would mean the cookie path is '/foo',
+		// whereas a direct hit of /foo would mean the cookie path is '/'
+		has_data_suffix(url.pathname) ? strip_data_suffix(url.pathname) : url.pathname,
+		options.trailing_slash
+	);
+	// Emulate browser-behavior: if the cookie is set at '/foo/bar', its path is '/foo'
+	const default_path = normalized_url.split('/').slice(0, -1).join('/') || '/';
+
+	if (options.dev) {
+		// Remove all cookies that no longer exist according to the request
+		for (const name of Object.keys(cookie_paths)) {
+			cookie_paths[name] = new Set(
+				[...cookie_paths[name]].filter(
+					(path) => !path_matches(normalized_url, path) || name in initial_cookies
+				)
+			);
+		}
+		// Add all new cookies we might not have seen before
+		for (const name in initial_cookies) {
+			cookie_paths[name] = cookie_paths[name] ?? new Set();
+			if (![...cookie_paths[name]].some((path) => path_matches(normalized_url, path))) {
+				cookie_paths[name].add(default_path);
+			}
+		}
+	}
+
 	/** @type {Record<string, import('./page/types').Cookie>} */
 	const new_cookies = {};
 
@@ -57,9 +83,14 @@ export function get_cookies(request, url, options) {
 				return cookie;
 			}
 
-			if (c || cookie_paths[name]?.size > 0) {
+			const paths = new Set([...(cookie_paths[name] ?? [])]);
+			if (c) {
+				paths.add(c.options.path ?? default_path);
+			}
+			if (paths.size > 0) {
 				console.warn(
-					`Cookie with name '${name}' was not found, but a cookie with that name exists at a sub path. Did you mean to set its 'path' to '/'?`
+					// prettier-ignore
+					`Cookie with name '${name}' was not found at path '${url.pathname}', but a cookie with that name exists at these paths: '${[...paths].join("', '")}'. Did you mean to set its 'path' to '/' instead?`
 				);
 			}
 		},
@@ -70,17 +101,7 @@ export function get_cookies(request, url, options) {
 		 * @param {import('cookie').CookieSerializeOptions} opts
 		 */
 		set(name, value, opts = {}) {
-			let path = opts.path;
-			if (!path) {
-				const normalized = normalize_path(
-					// Remove suffix: 'foo/__data.json' would mean the cookie path is '/foo',
-					// whereas a direct hit of /foo would mean the cookie path is '/'
-					has_data_suffix(url.pathname) ? strip_data_suffix(url.pathname) : url.pathname,
-					options.trailing_slash
-				);
-				// Emulate browser-behavior: if the cookie is set at '/foo/bar', its path is '/foo'
-				path = normalized.split('/').slice(0, -1).join('/') || '/';
-			}
+			let path = opts.path ?? default_path;
 
 			new_cookies[name] = {
 				name,
@@ -93,11 +114,12 @@ export function get_cookies(request, url, options) {
 			};
 
 			if (options.dev) {
-				cookie_paths[name] = cookie_paths[name] || new Set();
+				cookie_paths[name] = cookie_paths[name] ?? new Set();
 				if (!value) {
 					if (!cookie_paths[name].has(path) && cookie_paths[name].size > 0) {
+						const paths = `'${Array.from(cookie_paths[name]).join("', '")}'`;
 						console.warn(
-							`Trying to delete cookie '${name}' at path '${path}', but a cookie with that name only exists at a different path.`
+							`Trying to delete cookie '${name}' at path '${path}', but a cookie with that name only exists at these paths: ${paths}.`
 						);
 					}
 					cookie_paths[name].delete(path);

packages/kit/test/apps/basics/src/app.d.ts

@@ -1,7 +1,7 @@
 declare namespace App {
 	interface Locals {
 		answer: number;
-		name: string;
+		name?: string;
 		key: string;
 		params: Record<string, string>;
 	}

packages/kit/test/apps/basics/src/hooks.server.js

@@ -44,7 +44,10 @@ export const handle = sequence(
 		return resolve(event);
 	},
 	({ event, resolve }) => {
-		event.locals.name = /** @type {string} */ (event.cookies.get('name'));
+		if (event.url.pathname.includes('fetch-credentialed')) {
+			// Only get the cookie at the test where we know it's set to avoid polluting our logs with (correct) warnings
+			event.locals.name = /** @type {string} */ (event.cookies.get('name'));
+		}
 		return resolve(event);
 	},
 	async ({ event, resolve }) => {

packages/kit/test/apps/basics/src/routes/cookies/encoding/+page.server.js

@@ -1,6 +1,6 @@
 /** @type {import('./$types').PageServerLoad} */
 export function load(event) {
 	return {
-		a: event.cookies.get('a')
+		encoding: event.cookies.get('encoding')
 	};
 }

packages/kit/test/apps/basics/src/routes/cookies/encoding/+page.svelte

@@ -4,5 +4,5 @@
 </script>
 
 Cookie: <span id="cookie-value">
-	{data.a}
+	{data.encoding}
 </span>

packages/kit/test/apps/basics/src/routes/cookies/encoding/not-decoded-twice/+server.js

@@ -3,6 +3,6 @@ import { redirect } from '@sveltejs/kit';
 /** @type {import('@sveltejs/kit').RequestHandler} */
 export const GET = (event) => {
 	const sneaky = 'teapot%2C%20jane%20austen';
-	event.cookies.set('a', sneaky);
+	event.cookies.set('encoding', sneaky);
 	throw redirect(303, '/cookies/encoding');
 };

packages/kit/test/apps/basics/src/routes/cookies/encoding/set/+server.js

@@ -3,6 +3,6 @@ import { redirect } from '@sveltejs/kit';
 /** @type {import('@sveltejs/kit').RequestHandler} */
 export const GET = (event) => {
 	const needsEncoding = 'teapot, jane austen';
-	event.cookies.set('a', needsEncoding);
+	event.cookies.set('encoding', needsEncoding);
 	throw redirect(303, '/cookies/encoding');
 };

packages/kit/test/apps/basics/src/routes/load/fetch-credentialed.json/+server.js

@@ -3,6 +3,6 @@ import { json } from '@sveltejs/kit';
 /** @type {import('./$types').RequestHandler} */
 export function GET(event) {
 	return json({
-		name: event.locals.name
+		name: event.locals.name ?? 'Fail'
 	});
 }

packages/kit/test/apps/basics/test/test.js

@@ -1967,7 +1967,8 @@ test.describe('Actions', () => {
 	});
 });
 
-test.describe('Cookies API', () => {
+// Run in serial to not pollute the log with (correct) cookie warnings
+test.describe.serial('Cookies API', () => {
 	// there's a problem running these tests in the CI with webkit,
 	// since AFAICT the browser is using http://localhost and webkit won't
 	// set a `Secure` cookie on that. So we bail...
@@ -1994,18 +1995,6 @@ test.describe('Cookies API', () => {
 		expect(await span.innerText()).toContain('undefined');
 	});
 
-	test('cookies can be set with a path', async ({ page }) => {
-		await page.goto('/cookies/nested/a');
-		let span = page.locator('#cookie-value');
-		expect(await span.innerText()).toContain('teapot');
-		await page.goto('/cookies/nested/b');
-		span = page.locator('#cookie-value');
-		expect(await span.innerText()).toContain('undefined');
-		await page.goto('/cookies');
-		span = page.locator('#cookie-value');
-		expect(await span.innerText()).toContain('undefined');
-	});
-
 	test('more than one cookie can be set in one request', async ({ page }) => {
 		await page.goto('/cookies/set-more-than-one');
 		const span = page.locator('#cookie-value');
@@ -2043,4 +2032,16 @@ test.describe('Cookies API', () => {
 		await page.click('button#janeAusten');
 		await expect(page.locator('#cookie-value')).toHaveText('Jane Austen');
 	});
+
+	test('cookies can be set with a path', async ({ page }) => {
+		await page.goto('/cookies/nested/a');
+		let span = page.locator('#cookie-value');
+		expect(await span.innerText()).toContain('teapot');
+		await page.goto('/cookies/nested/b');
+		span = page.locator('#cookie-value');
+		expect(await span.innerText()).toContain('undefined');
+		await page.goto('/cookies');
+		span = page.locator('#cookie-value');
+		expect(await span.innerText()).toContain('undefined');
+	});
 });