chore: upgrade to cookie 1.0.2

[Conduitry]

We've regularly had comments/issues from people about a security vulnerability reported in [email protected], but we've been unable to upgrade because of later versions of the library having stricter validation. It sounds like these restrictions get loosened in 1.0.2 - jshttp/cookie#210 - and so I'm hopeful we can do this upgrade after all. If it turns out that there are still people who are affected by this, we can decide then whether we want to report this upstream to the cookie library or whether we want to deal with this in some other way.

---

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

• Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

[changeset-bot]

🦋 Changeset detected

Latest commit: ef525b3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Conduitry]

Ugh. There are type changes. I haven't looked yet whether we can deal with them internally or whether some of them will affect end-users.

[Conduitry]

After an embarrassing number of commits, this is green now in CI, but I'm a bit worried about the effect this might have on end users. Any type changes here were something that users were already being exposed to by specifying an override of the cookie dependency whether they knew it or not and whether TypeScript knew it or not - but this could still pose an issue.

I'm the most worried about cookie APIs that previously returned string and now return string | undefined and what effects that might have. It's definitely possible that we should be doing some filtering in code now so that getAll() will only return cookies whose value isn't undefined. I haven't dug into the cookie library to see why cookies can start having value=undefined or what that actually means.

[Conduitry]

I've pushed another change to remove @types/cookie, as cookie now provides its own types.

I'm still not sure what to do about the string | undefined stuff.

[Conduitry]

Ben has opened jshttp/cookie#216 asking about this type change. It looks like a user-specified decode callback is now allowed to return undefined, and this is used as-is in the parse() response, which seems peculiar.

[Conduitry]

If we just want to filter out cookies with a decoded value of undefined from the array returned in getAll(), we can do something like this:

diff --git a/packages/kit/src/exports/public.d.ts b/packages/kit/src/exports/public.d.ts
index 7f42a1318..c54ff9f98 100644
--- a/packages/kit/src/exports/public.d.ts
+++ b/packages/kit/src/exports/public.d.ts
@@ -221,9 +221,7 @@ export interface Cookies {
 	 * Gets all cookies that were previously set with `cookies.set`, or from the request headers.
 	 * @param opts the options, passed directly to `cookie.parse`. See documentation [here](https://github.com/jshttp/cookie#cookieparsestr-options)
 	 */
-	getAll: (
-		opts?: import('cookie').ParseOptions
-	) => Array<{ name: string; value: string | undefined }>;
+	getAll: (opts?: import('cookie').ParseOptions) => Array<{ name: string; value: string }>;
 
 	/**
 	 * Sets a cookie. This will add a `set-cookie` header to the response, but also make the cookie available via `cookies.get` or `cookies.getAll` during the current request.
diff --git a/packages/kit/src/runtime/server/cookie.js b/packages/kit/src/runtime/server/cookie.js
index a5e4737f5..966a86046 100644
--- a/packages/kit/src/runtime/server/cookie.js
+++ b/packages/kit/src/runtime/server/cookie.js
@@ -105,7 +105,11 @@ export function get_cookies(request, url, trailing_slash) {
 				}
 			}
 
-			return Object.entries(cookies).map(([name, value]) => ({ name, value }));
+			return /** @type {Array<{ name: string; value: string }>} */ (
+				Object.entries(cookies)
+					.filter(([, value]) => value != null)
+					.map(([name, value]) => ({ name, value }))
+			);
 		},
 
 		/**
diff --git a/packages/kit/types/index.d.ts b/packages/kit/types/index.d.ts
index 7ab61a872..4dcdc32c5 100644
--- a/packages/kit/types/index.d.ts
+++ b/packages/kit/types/index.d.ts
@@ -203,9 +203,7 @@ declare module '@sveltejs/kit' {
 		 * Gets all cookies that were previously set with `cookies.set`, or from the request headers.
 		 * @param opts the options, passed directly to `cookie.parse`. See documentation [here](https://github.com/jshttp/cookie#cookieparsestr-options)
 		 */
-		getAll: (
-			opts?: import('cookie').ParseOptions
-		) => Array<{ name: string; value: string | undefined }>;
+		getAll: (opts?: import('cookie').ParseOptions) => Array<{ name: string; value: string }>;
 
 		/**
 		 * Sets a cookie. This will add a `set-cookie` header to the response, but also make the cookie available via `cookies.get` or `cookies.getAll` during the current request.

If we go this way, I'd love suggestions about how to convince TS that this array really won't have any objects with value=undefined without needing this force cast.

[blakeembrey]

we've been unable to upgrade because of later versions of the library having stricter validation

If this is the blocker for 0.7 I'm happy to back port the validation change in 1.x to 0.x. 1.0 mostly contains performance improvements and cleanup of responsibilities in the code. It won't allow everything it used to, but should be much closer than today.

[benmccann]

Not sure if we need to do anything or can accept the risk of breaking, but I wanted to leave a comment for reference at least. I've been comparing the validation and it does appear to tighten it still compared to 0.6
https://github.com/jshttp/cookie/blob/38323bad3aa04bce840103ff6075bc05cc0bf884/index.js#L33
https://github.com/jshttp/cookie/blob/e739f419e56442b754e4fea6dbcf98c1c8d00dda/src/index.ts#L15

If I've read the code correctly, things like the tab character, line feed, semi-colon, and equals are no longer allowed with this upgrade along with everything under "Latin-1 Supplement" on https://en.wikipedia.org/wiki/List_of_Unicode_characters. I don't know how many users might have used something like á in their cookie name if they're non-English speaking

[benmccann]

I've confirmed that cookie.serialize("bár", "foo"); works on cookie 0.6, but not 1.0

@blakeembrey thanks for the offer to loosen the validation in 0.7. Do you think that we could change it to accept characters like á that are not supported by 1.0 but which don't have any special meaning or pose any security risk? I hate to ask, but we had to roll back the last time we tried upgrading to 0.7 and I'm hoping to avoid needing to do that again

[blakeembrey]

are not supported by 1.0 but which don't have any special meaning or pose any security risk?

Honestly, without a better expert opinion on this I wouldn't want to. That said, I'm 90% sure there's no support for sending non-ASCII characters in HTTP headers so it should be a non-issue.

Related: RFC 2047 on encoding, RFC encoding the Link header, RFC on encoding in auth. TL;DR is they always convert into ASCII. Cookie doesn't encode names so relies on the application having done it.

[benmccann]

That said, I'm 90% sure there's no support for sending non-ASCII characters in HTTP headers so it should be a non-issue.

I'd expect cookie.serialize to encode characters like á. However, it does validation before encoding and rejects á, but I'm not sure it needs to. I don't think the application or user should encode anything because then we'll end up with double encoding since cookie.serialize is going to do a round of encoding as well.

[blakeembrey]

Cookie names are not altered. Values are, and the validation occurs after encoding. I didn’t alter any behavior in the 1.0 release that may have unexpectedly changed how cookies are written or read to ensure a smoother upgrade.

Update: Took a quick look into whether it’s worth adding cookie names encoding and saw dotnet/aspnetcore#23578, so it would need more thought.

[elikoga]

I've confirmed that cookie.serialize("bár", "foo"); works on cookie 0.6, but not 1.0

Can you confirm that this flows via the network from a server to the browser and back from a browser to a server correctly? As far as I can see, this is the entire concern with non-ASCII chars in cookie names.

[benmccann]

Thanks again for all the help here. I tested to see if á could be sent in as part of a cookie name in an application today using cookie 0.6 and it seems to work. So unfortunately, I'm still a bit concerned that folks may be doing that and that we could break them by upgrading the cookie library to a version which disallows accented characters.

I created a new SvelteKit app with npx sv create myapp and then added a src/hooks.server.js file to write a cookie and it appears to work without error:

/** @type {import('@sveltejs/kit').Handle} */
export async function handle({ event, resolve }) {
	event.cookies.set('bár', 'foo', { path: '/' });
	return await resolve(event);
}

When loading the page in the browser I see it come through:

And then when reloading the page I see the browser sending it back to the server (along with some others I have leftover from testing previous apps):

[Conduitry]

@benmccann Have you confirmed whether it's the dev tools URL-decoding the values for you when displaying them for convenience? If you 'copy as curl', what are the actual header values?

[elikoga]

https://stackoverflow.com/a/1969339 tells us that this is browser specific behaviour:

What isn't mentioned and browsers are totally inconsistent about, is non-ASCII (Unicode) characters:

in Opera and Google Chrome, they are encoded to Cookie headers with UTF-8;
in IE, the machine's default code page is used (locale-specific and never UTF-8);
Firefox (and other Mozilla-based browsers) use the low byte of each UTF-16 code point on its own (so ISO-8859-1 is OK but anything else is mangled);
Safari simply refuses to send any cookie containing non-ASCII characters.

[paoloricciuti]

Thanks again for all the help here. I tested to see if á could be sent in as part of a cookie name in an application today using cookie 0.6 and it seems to work. So unfortunately, I'm still a bit concerned that folks may be doing that and that we could break them by upgrading the cookie library to a version which disallows accented characters.

I created a new SvelteKit app with npx sv create myapp and then added a src/hooks.server.js file to write a cookie and it appears to work without error:

/** @type {import('@sveltejs/kit').Handle} */
export async function handle({ event, resolve }) {
	event.cookies.set('bár', 'foo', { path: '/' });
	return await resolve(event);
}

When loading the page in the browser I see it come through:

And then when reloading the page I see the browser sending it back to the server (along with some others I have leftover from testing previous apps):

Unfortunately it seems to work in safari

[elikoga]

https://bugzilla.redhat.com/show_bug.cgi?id=2316549

This is the bug everyone seems to be complaining about which is exactly "Bug 2316549 (CVE-2024-47764) - CVE-2024-47764 cookie: cookie accepts cookie name, path, and domain with out of bounds characters:"

[elikoga]

Sure. Coincidentally we later had a production incident that very definitely did involve this bug so I'm happy to contribute

Originally posted by @bewinsnw in #165

@bewinsnw Was what you experienced a security incident and is somehow related to the CVE?