Skip to content
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

Ambiguity on setting a cookie with "" (empty string) as the path. #242

Open
RupinMittal opened this issue Jan 9, 2025 · 10 comments
Open

Comments

@RupinMittal
Copy link

The spec (https://wicg.github.io/cookie-store/#set-a-cookie) says that if set is called with a path passed in, and the path does not start with a "/", then we should return a failure. This implies that passing in "" should cause a failure.

However, Chrome does not return failure here, but rather fills in the path itself and sets the cookie.

Which is correct? Returning a failure or filling in the path automatically if an empty string is passed in? Thank you!

@RupinMittal
Copy link
Author

CC @annevk @szewai @cdumez

@johannhof
Copy link
Member

I don't think I have a strong opinion on this, though of course changing this might lead to backwards incompat. @DCtheTall @inexorabletash @ayuishii?

@inexorabletash
Copy link
Member

Agreed - no strong feelings either way. Logically, feels like a Chromium bug but also seems harmless to support if needed for compat. Does the FF impl treat this as an error? If so that could inform compat risk.

@annevk
Copy link
Collaborator

annevk commented Jan 10, 2025

cc @bakulf

@annevk
Copy link
Collaborator

annevk commented Jan 10, 2025

@RupinMittal what does "fills in the path itself" mean? Does it take the path from the url argument?

@cdumez
Copy link

cdumez commented Jan 10, 2025

Agreed - no strong feelings either way. Logically, feels like a Chromium bug but also seems harmless to support if needed for compat. Does the FF impl treat this as an error? If so that could inform compat risk.

My understanding is that Firefox is not shipping this API yet, though they are actively working on it. We could maybe check their behavior using a nightly build but I don't think it would inform compat risk?

@inexorabletash
Copy link
Member

Since Moz has already done some amount of compat consideration about shipping only a subset of the API that Chrome shipped, I was hoping they might have noticed any issues here as well. A bit of a stretch, though.

@RupinMittal
Copy link
Author

RupinMittal commented Jan 10, 2025

Firefox:
I was not able to confirm its behavior. I used a nightly build (Jan 6 build), but was not able to set any cookies using the Cookie Store API even though the feature flag was turned on. Is this API functional on FF currently? It seems from wpt.fyi that layout tests are indeed passing. Maybe I'm doing something wrong?

Chrome:
Yes, if you pass in an empty string for the path, It takes the path from the url. But ignoring the empty string feels odd. The logic seems to be:

  • If no path passed in, use the default "/"
  • If empty string passed in, ignore it and take the path from the url
  • If non-empty string passed in, check it and set cookie if valid

As opposed to:

  • If no path passed in, use the default "/"
  • If path passed in, check it and set cookie if valid

@asutherland
Copy link

In Firefox, I was able to set cookies on Nightly at https://worker-playground.glitch.me/ specifically pasting in code like the following based on https://developer.mozilla.org/en-US/docs/Web/API/CookieStore#examples:

const day = 24 * 60 * 60 * 1000;

cookieStore
  .set({
    name: "cookie1",
    value: "cookie1-value",
    expires: Date.now() + day,
    domain: "worker-playground.glitch.me",
    path: "",
  })
  .then(
    () => {
      console.log("It worked!");
    },
    (reason) => {
      console.error("It failed: ", reason);
    },
  );

The code currently likes to normalize an empty path to be / and that's what I observe in our devtools storage tab afterwards:

nsString path(aPath);
if (path.IsEmpty() || path[path.Length() - 1] != '/') {
  path.Append('/');
}

Firefox currently use a gating pref that is only defaulted to true in our nightly builds. Bug 1937477 tracks enabling it to ride the trains to release.

@annevk
Copy link
Collaborator

annevk commented Jan 11, 2025

I guess I don't have a strong preference, but it does seem like a bit of a footgun that the empty string gives you the path of the URL. Equivalent to not passing it (thus /) or failure seem both much more appropriate.

(I could see an argument for the current behavior if the restriction on starting with / wasn't there. In which case you could parse it as a URL and use the resulting path. But it seems weird for only the empty string to be treated as such and not foo and the like.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants