Skip to content

Cookie with name 'fail-type' was not found at path '...' #9130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Rich-Harris opened this issue Feb 20, 2023 · 7 comments · Fixed by #9298
Closed

Cookie with name 'fail-type' was not found at path '...' #9130

Rich-Harris opened this issue Feb 20, 2023 · 7 comments · Fixed by #9298
Labels
bug Something isn't working
Milestone

Comments

@Rich-Harris
Copy link
Member

Describe the bug

I keep seeing this error when I run the tests:

Cookie with name 'fail-type' was not found at path '...', but a cookie with that name exists at these paths: '/errors/error-html'. Did you mean to set its 'path' to '/' instead?

It doesn't happen every time, but it happens often. The fail-type cookie is set here...

export function GET({ cookies, url }) {
cookies.set('fail-type', url.searchParams.get('type'), { path: '/' });
return new Response();
}

...so that it can be read here:

export async function load({ cookies }) {
const should_fail = cookies.get('fail-type');
if (should_fail) {
cookies.delete('fail-type', { path: '/' });

As you can see, the path is set, so it's unclear why this message appears. But it feels indicative of a real bug, just one I haven't wrapped my head round.

Reproduction

Run pnpm test enough times, you'll see it.

Logs

No response

System Info

workspace

Severity

annoyance

Additional Information

No response

@Rich-Harris Rich-Harris added the bug Something isn't working label Feb 20, 2023
@Smirow
Copy link

Smirow commented Feb 21, 2023

I am also seeing these warnings in my projet. It rapidly appeared when I implemented auth in a near blank project, so should be easily reproducible.

@mustofa-id
Copy link

It also happens to me when using the --host flag when I want to show my project on another computer using the host computer's IP address, and can't log in without any error message, just redirected to login page. I was using adapter-node.

@cdcarson
Copy link
Contributor

This was introduced because folks were inadvertently setting cookies on __data paths. For the same reason and more worrisome, IMO, this line got added...

// 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('/') || '/';

  • This theoretically fixes the special _data case. Only theoretically, because it's unlikely that a request to, say, /sign-in/__data.js should result in a cookie being set to /sign-in.
  • For other cases it changes /foo/123 to /foo, which most likely would be the opposite of what's intended.
  • Note this behavior, unlike the error message, isn't limited to dev.

I think it's reasonable to require path when setting a cookie. It's a good thing to make me think about where I want to set a cookie, even if the usual case is just /.

But that would be a breaking change, so how about this:

  • For now, get rid of the notion of maintaining cookie state across requests in dev, which is causing the flakiness mentioned above.
  • Get rid of the current dev error. Instead warn that cookies.set will eventually require path.
  • Later, actually require path and get rid of the line of code above.

@dummdidumm
Copy link
Member

dummdidumm commented Feb 22, 2023

As stated in the comment, this emulates the native browser behavior, so it's as if the path isn't set from a browser perspective, only that it also handles the /__data.json case. If you would set a cookie without the path attribute (and without the logic from the snippet) on /foo/123, the cookie is registered at /foo.
I don't think that the linked snippet is wrong, it's more likely that something in the dev time detection heuristics is wrong.

@cdcarson
Copy link
Contributor

Hi @dummdidumm: I understand where you're coming from. I still disagree. Sure browsers and the underlying cookie api we use allow setting cookies without the path, but they don't have special concerns, like __data urls.

I guess I'd come around if someone could show me a case where requiring path to be set prevents doing something useful. I can't think of any, but who knows? In the meantime I'll agree to disagree.

@csellis
Copy link

csellis commented Feb 26, 2023

It also happens to me when using the --host flag when I want to show my project on another computer using the host computer's IP address, and can't log in without any error message, just redirected to login page. I was using adapter-node.

Had this same issue trying to log out on a remotely connected device. It would not set the cookie preventing the user from logging out.

@Rich-Harris
Copy link
Member Author

Ok, looking at the code again this makes sense. We're populating cookie_paths from the request headers, even though by that time we have no idea what the original path was.

I think it's worth keeping the error, just without the false positives. Requiring path in the next major version seems reasonable to me, will open a follow-up issue for that.

@Rich-Harris Rich-Harris added this to the soon milestone Mar 3, 2023
dummdidumm added a commit that referenced this issue Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants