Cookies set on server are improperly escaped

[howard36]

Describe the bug

This load function in +page.server.ts sets a cookie with value "a/b":

export function load({cookies}: any) {
    cookies.set("test", `"a/b"`);
    return {};
}

But the value that's actually returned in the Set-Cookie header and saved in the browser is %22a%2Fb%22. This bug only happens when load is run on the server, not in the browser.

Reproduction

Code: https://codesandbox.io/p/devbox/sveltekit-cookie-issue-l3c2zc?file=%2Fsrc%2Froutes%2F%2Bpage.server.ts%3A4%2C2

Website: https://l3c2zc-5173.csb.app/

(The code is deployed on the website above, and visiting the website should set the incorrect cookie %22a%2Fb%22 in your browser)

Logs

No response

System Info

System:
    OS: Linux 5.13 Ubuntu 20.04.4 LTS (Focal Fossa)
    CPU: (4) x64 Intel(R) Xeon(R) Platinum 8375C CPU @ 2.90GHz
    Memory: 26.26 GB / 30.85 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 18.14.0 - ~/.nvm/versions/node/v18.14.0/bin/node
    npm: 9.3.1 - ~/.nvm/versions/node/v18.14.0/bin/npm
  Browsers:
    Chrome: 120.0.6099.234

Severity

serious, but I can work around it

Additional Information

No response

[Conduitry]

The server behavior sounds correct to me. The default behavior in the underlying cookie library is to URL-encode cookie values, but this can be overridden - https://github.com./jshttp/cookie#encode

What are you doing on the browser so that it's set differently? And what value does it end up being?

[howard36]

When I force it to run in the browser (by moving load from +layout.server.ts to +layout.ts and adding if (!browser) return {}), it sets the cookie to "a/b", which is inconsistent with the server-side behavior.

[Conduitry]

cookies isn't passed to universal load() functions, neither on the browser nor on the server. Could you show a complete reproduction of what you're doing?

[howard36]

Ah, my bad. The code I ran with the universal load() function was more complicated - calling an external API that sets a cookie (which I believe SvelteKit forwards to the browser?). I'll try to set up a minimal reproduction of this.

[Conduitry]

If it's an external API, that's going to have its own decisions that have been made about how/whether to encode cookie values.

In any case, I suspect the solution for you is to pass an identity encode callback to the cookies library when setting the cookie, which is essentially 'I know what I'm doing, use this literal cookie value'. I believe that library will still throw an error if your encoded cookie contains any characters disallowed by the spec (which presumably it won't, if this other external API is successfully setting it to that).

[howard36]

Here's a better reproduction. I have an API endpoint that sets a cookie:

/routes/api/set-cookie/+server.ts

import type { RequestHandler } from './$types';

export const POST: RequestHandler = () => {
    const headers = new Headers();
    headers.append('Set-Cookie', 'test="a/b"; Path=/; HttpOnly');

    return new Response("Cookie has been set", { headers });
}

and a universal load() function that calls this API:

/routes/bad/+page.ts

import { browser } from "$app/environment";
import type { PageLoad } from "./$types";

export const load: PageLoad = ({fetch}) => {
    if (!browser) {
        fetch('/api/set-cookie', { method: 'POST' });
    }
    return {};
}

When I use if (!browser) to force it to run on the server, the cookie is set to %22a%2Fb%22, which you can see for yourself by going to https://h9trhh-5173.csb.app/bad and checking your cookies.

But when I use if (browser) to run load() in the browser, it sets the cookie to "a/b", which you can see at https://h9trhh-5173.csb.app/good

You can find the full code here

Also see #1198 for a similar bug (except in that issue, the "bad" case wouldn't set a cookie at all)

[gterras]

Use the cookies arg of the load function or if you really want to set it manually use encodeURIComponent as stated.

[howard36]

@gterras the cookies arg is only available on +layout.server.ts, not +layout.ts. I have a workaround for this, but it seems like a bug in SvelteKit if load() acts differently when run server-side vs client-side.

[gterras]

From the link above :

You cannot add a set-cookie header with setHeaders — use cookies.set(name, value, options) instead.

Just use the cookies args inside your request handler (api/set-cookie). As for why it happens in addition to being explicitly not supported I suppose sveltekit or even your browser does it for you at some point. Ensuring cookies are encoded/decoded isn't mandatory but it really helps avoiding surprises.

[howard36]

This issue happens when fetching any URL returning a Set-Cookie response header, including external APIs that are independent of Svelte. My toy example uses /api/set-cookie because it was the simplest way to demonstrate the bug, but in my real use case, the external API is implemented in Python, where cookies.set() won't work.

encodeURIComponent also won't work: if the API returns an encoded cookie value (such as %2F instead of /), then the server-side load() function will encode it a second time (turning %2F into %252F) when passing on the Set-Cookie response header.

[Conduitry]

Sorry for the delay - yep, this indeed looks like a bug in how cookies are (re-)serialized when server-side rendering is collecting together the cookies getting sent by the various API calls made with fetch. The bug is presumably somewhere around

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

Lines 131 to 132 in c4efc26

	const set_cookie = response.headers.get('set-cookie');

or around

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

Line 186 in a2fe7ab

function set_internal(name, value, options) {

.

As an aside, in your repro you should be awaiting the result of the fetch in your load functions, otherwise it's not guaranteed to finish in time, and you may get errors about not being able to set cookies on the outgoing response.

[Conduitry]

My (untested) guess for a fix would be something like this:

diff --git a/packages/kit/src/runtime/server/fetch.js b/packages/kit/src/runtime/server/fetch.js
index e59112092..dcd1c240b 100644
--- a/packages/kit/src/runtime/server/fetch.js
+++ b/packages/kit/src/runtime/server/fetch.js
@@ -132,14 +132,17 @@ export function create_fetch({ event, options, manifest, state, get_cookie_heade
 				const set_cookie = response.headers.get('set-cookie');
 				if (set_cookie) {
 					for (const str of set_cookie_parser.splitCookiesString(set_cookie)) {
-						const { name, value, ...options } = set_cookie_parser.parseString(str);
+						const { name, value, ...options } = set_cookie_parser.parseString(str, {
+							decodeValues: false
+						});
 
 						const path = options.path ?? (url.pathname.split('/').slice(0, -1).join('/') || '/');
 
 						// options.sameSite is string, something more specific is required - type cast is safe
 						set_internal(name, value, {
 							path,
-							.../** @type {import('cookie').CookieSerializeOptions} */ (options)
+							.../** @type {import('cookie').CookieSerializeOptions} */ (options),
+							encode: (value) => value
 						});
 					}
 				}

We need to tell the set-cookie-parser to not try to URL-decode the cookie values, and then we need to tell the cookie library to not try to URL-encode them.