`cookies.delete` not working when using form `enhance`

[eltigerchino]

Describe the bug

Using enhance on a <form> and throwing redirect in the form action causes cookies.delete to not work in the page server load function.

Reproduction

https://github.com/s3812497/sveltekit-enhance-bug

Reproduction:

1. Click the enhanced form button.
2. User is redirected.
3. Cookie is not deleted.

Expected:

1. Click the enhanced form button.
2. User is redirected.
3. Cookie is deleted in +page.server

Logs

No response

System Info

System:
    OS: macOS 12.5.1
    CPU: (8) arm64 Apple M1 Pro
    Memory: 1.56 GB / 32.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.17.0 - ~/.nvm/versions/node/v16.17.0/bin/node
    npm: 8.15.0 - ~/.nvm/versions/node/v16.17.0/bin/npm
  Browsers:
    Chrome: 104.0.5112.79
    Firefox: 104.0.1
    Firefox Developer Edition: 105.0
    Safari: 15.6.1
    Safari Technology Preview: 16.0
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.71 
    @sveltejs/kit: next => 1.0.0-next.472 
    svelte: ^3.44.0 => 3.50.0 
    vite: ^3.1.0 => 3.1.0

Severity

serious, but I can work around it

Additional Information

No response

[dummdidumm]

I'm not sure if there's something we can do here besides documenting it. The enhance function can't know that a cookie was deleted, so this needs to be done manually on the client through the browser APIs.

[eltigerchino]

I'm not sure if there's something we can do here besides documenting it. The enhance function can't know that a cookie was deleted, so this needs to be done manually on the client through the browser APIs.

For additional context, in my app before the migration:

1. A user could login and a cookie would be set by the POST action (the form uses the previous enhance).
2. The user gets redirected to the dashboard, which has a logout button (a simple <a href="/auth?logout=true">)
3. Navigating to /auth?logout=true would delete the cookie by the load function in +page.server.ts checking the params for logout and setting an expired cookie.

However, the new cookies.delete API does not seem to delete the cookie compared to the previous method of using setHeaders and an expired cookie (which is no longer allowed because of the cookies API). Or could it be the new enhance preventing an expired cookie from being set?

[acoyfellow]

@s3812497 i haven't used the new cookies.delete function, but i am able to use cookies.set to remove a cookie:

cookies.set("cookieName", "", {
    httpOnly: true,
    path: '/',
    maxAge: 0
  })

[eltigerchino]

@s3812497 i haven't used the new cookies.delete function, but i am able to use cookies.set to remove a cookie:

cookies.set("cookieName", "", {
    httpOnly: true,
    path: '/',
    maxAge: 0
  })

Thanks for the solution! The issue probably lies with cookies.delete if anything. However, really strange that it only happens to cookies set as a result of an enhanced form.

[dummdidumm]

Mhm then maybe this is a bug with deleting cookies after all..

[eltigerchino]

Hi @Rich-Harris and @dummdidumm . Is it possible to re-open this issue?

I have tested the new update with the same reproduction and the issue still persists.

However, it does delete the cookie after a full page reload, where previously it wouldn't delete no matter the number of page reloads.

[cdcarson]

I have tested the new update with the same reproduction and the issue still persists.

@s3812497 Yep. #6622 fixed something relatively obvious, but there's something else going on here. This issue should be reopened.

[eltigerchino]

Hi @Rich-Harris @dummdidumm @benmccann , This cookies.delete issue still persists after I've updated SvelteKit to the latest version (1.0.0-next.483) to include the cookies fix from #6811

Currently, the most reliable method of deleting cookies is still:

  cookies.set('cookie_name', '', {
    httpOnly: true,
    path: '/',
    maxAge: 0
  })

[cdcarson]

@s3812497 Not solving for now, just trying to diagnose. Can you try this, and see whether it works as you intend? In src/routes/somewhere/+page.server.ts add the options parameter to cookies.delete with the path set...

import type { PageServerLoad } from "./$types";

export const load: PageServerLoad = ({ cookies }) => {
  // added options with path to both lines
  cookies.delete("cookie1", {path: '/'}); 
  cookies.delete("cookie2", {path: '/'});
};

This works for me (but I may be missing the point. )

@Rich-Harris If the solution above works for @s3812497, this is related to what we were just talking about re setting a default path. The difference @s3812497 points out seems to stem from the cookies being set/deleted on different paths for the normal vs. enhanced cases

[eltigerchino]

@s3812497 Not solving for now, just trying to diagnose. Can you try this, and see whether it works as you intend? In src/routes/somewhere/+page.server.ts add the options parameter to cookies.delete with the path set...

import type { PageServerLoad } from "./$types";

export const load: PageServerLoad = ({ cookies }) => {
  // added options with path to both lines
  cookies.delete("cookie1", {path: '/'}); 
  cookies.delete("cookie2", {path: '/'});
};

This works for me (but I may be missing the point. )

It works as expected.

[dummdidumm]

We talked about issuing a warning in cases like this. One possibility would be to issue a warning here something along the lines of "it seems you want to delete cookie X which is at path Y, but you actually delete cookie X for path Z. Set the path option accordingly"

[cdcarson]

We talked about issuing a warning in cases like this. One possibility would be to issue a warning here something along the lines of "it seems you want to delete cookie X which is at path Y, but you actually delete cookie X for path Z. Set the path option accordingly"

@dummdidumm I'm pretty sure it's not possible to give that level of detail when deleting a cookie set during a previous request. (During the same request it would be.) The cookie request header would have to give us the path on which the cookie was originally set. AFAICT that doesn't happen -- it's just serialized key value pairs.

So I think the message would have to be triggered when the path is not provided, both on cookies.set and cookies.delete:

Hey, you are modifying a cookie without specifying a path. This means the cookie will only be changed for requests matching ${event.url.pathname}. Are you sure this is what you want?

The other option is to set the default path to '/'. I understand this is less secure, but (a) it takes care of the most common case and (b) if the developer is setting the path to /secret-dashboard/abc we may be justified in assuming they know what they are doing.

[dummdidumm]

To safe future readers from going through this: This issue is now about the gotchas of cookies. In this example, the cookie wasn't removed because the path wasn't set, which means the path is different when adding/removing the cookie, which the browser treats as different cookies.

The question is if we add some kind of warning when omitting the path, and whether or not we should add more docs on this (we already have some in the method docs for set/delete).

[cdcarson]

add some kind of warning when omitting the path

Yes, in dev.

whether or not we should add more docs on this

The cookies API deserves its own section. It looks like folks are using it in handle as well as load.

Here's an incomplete and probably outdated version in a PR comment. I didn't know where to put it in the docs.

[dummdidumm]

I dug into this again and am seeing a difference between client and server where I don't know how to best handle it:

• when you go to /page directly, setting a cookie in its load function, that cookie is scoped to the / path
• when you go to /page/ directly (note the trailing slash; set through trailingSlash: 'always'), setting a cookie in its load function, that cookie is scoped to the /page path
• when you go to the /page through a client-side navigation, setting a cookie in its load function, that cookie is scope to the /page path

Reason for the difference in behavior: it seems that the browser goes back to the last slash and uses that as the path. This means

• direct hit goes to the /page path -> /
• direct hit goes to the /page/ path -> /path
• the client-side navigation calls the page through /page/__data.json -> /path

I'm not sure about the best way forward here.

• add logic to cookie calls to set the path to something specific (basically replicating the browser behavior to avoid the __data.json mismatch)?
• warnings? how exactly?
• set the default path to basePath || '/' after all? FWIW some frameworks also do this, .NET for example https://learn.microsoft.com/en-us/dotnet/api/system.net.cookie.path?view=net-6.0#remarks . PHP / Nuxt3 behave as we do currently

I think I lean towards a combinations of option 1 and 2