fix cookie issues, add unit and e2e tests for cookies api

[cdcarson]

There are some issues with cookie deletion unresolved in this issue: #6609

I believe the issue was that we were not setting the path on delete (and maybe on set.) Some tests are needed to confirm.

Marking this as draft until then.

• unit tests
• e2e tests (probably good to have more eventually, fix and unskip the flaky one)
• expose cookie serialize options from the SvelteKit cookies API (this should be a separate typescript issue) this is fixed by directly annotating the get, set and delete with JSDoc params rather than relying on the imported type of cookies
• document api changes, if any (e.g. setting path to other than '/') See this comment for suggested copy.

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. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: b576d02

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

[cdcarson]

This is as far as I can take it, for now. I'd liked to have written more Playwright tests, but wasn't able to figure out fancier stuff, like testing enhanced forms. So the unresolved issue in #6609 I was only able to check by running the repro against this pr. But the perfect is the enemy of the good, and I'm confident that this is on the right track.

Net: The main issue folks were having stemmed from Kit not setting path and sameSite by default, for both set and delete. See also #6650. The defaults in this pr are exactly what I had been using to set/delete before the API, with no problems, so I'm pretty confident they are the right ones.

[benmccann]

It looks to me like you added the same tests to client.test.js and server.test.js. If you want to run a test on both the client and server you can add it to test.js in the same directory, so I'd suggest you remove the tests from client.test.js/server.test.js and move them to test.js

[benmccann]

I just glanced quickly, but the Playwright tests look to me like they could cause flakiness. They run in parallel and are checking the same cookie values

[Rich-Harris]

Thanks @cdcarson — re the parallel-tests-causing-flakiness thing @benmccann referred to, you can set a group of tests to run serially like so:

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

Line 648 in e20425f

test.describe.serial('Invalidation', () => {

[cdcarson]

I'm happy that this now fixes the issues we know about, knock on wood.

Remaining todos, not necessarily blocking.

• Decide whether to keep encode and decode as options, or just go with the cookie library defaults (de/encodeURIComponent.) Currently we are keeping them. The API surface is only minimally larger, since they are optional and most folks will ignore them. On the other hand, the advantages of disabling bespoke encode and decode are (1) we would only have to read the cookies once and (2) there's very little case for having a bespoke solution for de/encoding 3 disallowed characters.
• Documentation. IMO, basically the following. (Wasn't sure where this would go in the docs.)

Cookies

Unlike all other response headers, which you set with setHeaders, set-cookie headers should be set and deleted with cookies, which is available as part of RequestEvent.

// example tktk

The cookies API is built on the cookie library, which you may already be familiar with. SvelteKit provides opinionated, sensible defaults for cookie serialization:

{
	httpOnly: true,
	secure: true,
	path: '/',
	sameSite: 'lax'
}

Also, implicitly, cookies are set to the session, so they will disappear after the user closes the browser. If you want a long-lasting cookie, set maxAge to the number of seconds you want it to last. Other defaults can be changed as necessary by passing your overrides to cookie.set and cookie.delete.

// example TKTK, including maxAge

Important: If you do use your own values for httpOnly, Secure, SameSite or path to set a cookie, you should use the same values to delete it. Some browsers are picky about this.

Safari and dev mode: Safari does not send Secure cookies to localhost over http://. To get around this... (TKTK not sure how far down the rabbit hole we want to go here.)

[DoisKoh]

I'm having issues setting cookies when they're being passed between server to server requests. Also when setting them in the load function (in +layout.server.ts). My code was working fine before switching to the new cookies API. Would you happen to know if this might fix those issues as well?

Thanks...

[cdcarson]

@DoisKoh Probably. There are some bugs in the current implementation that this PR fixes.

@benmccann Do I have to take care of the failing mac tests in the CI? These pass on my mac, and I'm not sure what's going on. Perfectly willing to try to figure it out, but want to check if this is something that happens often, and if there's a trick. Thanks.

[cdcarson]

@benmccann Ignore my last qusetion about mac ci tests. I figured it out.

[Rich-Harris]

Thank you @cdcarson, this is awesome. I tried to make some changes locally and push them but for some reason it's not letting me, so I opened a new PR with those changes — we can either continue working in #6811 or you could merge that branch into this one.

The only real question I had was around the path default — it feels like this might be an opinion too far. Is it necessary?

[cdcarson]

The only real question I had was around the path default — it feels like this might be an opinion too far. Is it necessary?

@Rich-Harris Pretty sure it is necessary. See #6650. Basically a cookie set in a request to /sign-in will have that path. A subsequent request to /profile won't send that cookie.

The only downside of having the default is if the site base path is /foo. We can probably address this in the docs.

[Rich-Harris]

Merged #6811, so I'll close this. Thanks @cdcarson!