-
Notifications
You must be signed in to change notification settings - Fork 146
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
Allow cookies without key or value #159
Comments
Discussed at Berlin IETF; Julian points out that we should check previous discussion in cookie WG. |
For clarity, are we saying Chrome, Firefox, and Edge treat such a cookie as key: "" and value "foo", and Safari treats it like key: "foo" and value: ""? |
Per the table linked in OP Safari serializes input Firefox attempted to be more strict on cookie setting lacking |
https://wpt.fyi/results/cookies/http-state/general-tests.html?label=experimental&label=master&aligned is the relevant test, in particular the following subtests:
I'll put up a PR against the spec if folks are on board with the suggestions above. |
Putting together a fix for Chromium's 0020 behavior, I ran into more excitement: 0024 ( I'd suggest that this behavior doesn't make much sense, and that it would be better for browsers to align on requiring cookies to have either a name or a value, and to ignore That would change the expectations for these four tests to expect a serialization of "Cookie: foo". WDYT, @annevk, @johnwilander? |
For clarity: https://chromium-review.googlesource.com/1982551 shows the test expectations I'd suggest we end up with. |
As discussed in [1], cookies with empty names and empty values should be rejected. This patch removes the carveout made in https://crbug.com/601786, and adjusts unittests accordingly. This patch does not change the WPT expectations; we'll do that at the same time we change the spec. In the meantime, we'll check in local expectations matching the behavior we believe is correct. [1]: httpwg/http-extensions#159 (comment) Bug: 1037996, 601786 Change-Id: I53319cee385efff019b313479184236c53b1d783
Tl;dr: I think changing the spec to match current implementations for In the 0004 case: We've been parsing In the 0020 ( Judging by Chrome metrics for Cookie.HeaderLength, there is definitely non-zero usage of empty cookies, so I'm concerned that rejecting empty cookies would break things... (In the 0020 case, one could also argue that explicitly sending a Set-Cookie header of I agree it's a bit silly to allow cookies with both empty name and empty value, though. I think that's a reasonable change to make to the spec, and maybe we don't care about potentially breaking things. |
Thanks, @chlily1! I agree with you for the 0004 case. @johnwilander, would Apple be willing to change CFNetwork's behavior to match the suggestions above? For 0020 and 0023:
Looking at the stable channel on for the 28 days ending on Dec 31st, 0.0025% of
It would signify an intent to create a cookie without a name and without a value. What would that intent mean? I'd like it to mean "Don't create a cookie." :) |
This patch alters the cookie parsing algorithm to treat `Set-Cookie: token` as creating a cookie with an empty name and a value of "token". It also rejects cookies with neither names nor values (e.g. `Set-Cookie:` and `Set-Cookie: =`. Closes #159.
#1018 should cover the proposed changes above. (I kinda want to rewrite the whole parsing algorithm in terms of Infra to be a bit more precise, but that's a task for another patch. :) ) |
I’ve pinged the relevant folks to take a look at this thread. Like me, many won’t be back until Monday.
|
Not sure if it's the same thing, but I've seen facebook send an empty For example visiting https://www.theguardian.com/international loads a 1 pixel gif with the following URL: https://www.facebook.com/tr?id=XXXXXXXXXXX&ev=PageView&noscript=1 (replacing XXXX with your unique tracking reference) with the following response headers (note the blank
This then gets set in the Chrome Cookie jar with the following: I can't seem to find this showing in Firefox's cookie jar (even when turning off the default Enhance Tracking Protection) nor in Safari. Anyway, I'm sure a good few of us wouldn't care if this "cookie" died a death, or if Chrome changed to ignore this "cookie", and not even sure if it DOES anything (it doesn't seem to be sent in follow up HTTP requests, but that's not to say it's not looked at by some local JavaScript). However, given the prevalence of Facebook tracking I'm really surprised these sorts of cookies are only seen by 0.12% of users so thought I'd comment to note this in case those estimates are not accurate? Perhaps the stats are not picking up these cases? I've seen this on many sites and only picked the Guardian as a well-known example and guessed first time it would be on there (news sites are good ones to pick when looking for tracking pixel examples!). https://www.wsj.com/ sets the same if you prefer something US based and https://www.theaustralian.com.au/ sets the same (but not on the main domain) for those of you in that side of the world. |
The number of 0-byte Cookie headers is only a lower bound on how many no-name-no-value cookies are in use. If such a cookie were sent along with some other cookies, it would not be logged as a 0-byte header. I don't think we have any metrics on per-cookie length, just the total length of the Cookie header. So it might be worth collecting more data? Also, that metric doesn't include cookies accessed via |
Regarding @bazzadp's comments on Facebook; I pinged someone there who will forward this to the appropriate team. It sounds like a bug to me, and not something that I think should stop us from making a decision here on the underlying question of whether an empty cookie is a thing we should support. @chlily1: I read your comments above as generally approving of the change suggested in #1018, but cautious about how to ship it in Chrome? That feels like an important part of an eventual "Intent to Ship", rather than a discussion of what we'd like the spec to say. If we decide to accept the suggestions above, then I'm pretty confident that we can work out how and when to ship it (especially given that there's no real agreement among browsers today, so the status quo is already trifurcated as per #159 (comment) above).
Indeed! You're exactly right, and I should have been more clear. The 0.0025% is the number of requests (not page views, FWIW) for which we'd shift from sending an empty
I think this is the easier case, actually, as the serialization of an empty cookie is indistinguishable from a empty cookie jar. |
Thanks for sending on @mikewest . I more meant it as a concern that the 0.12% of users figure sounds way too low to me given Facebooks prevalence. Note you don’t need to be a Facebook user or visit a Facebook site to get this cookie - just a site that uses Facebook advertising pixels (of which there are many!). So I still have that concern that this figure sounds way too low to me. Which then begs the question as to whether there are more cases than we think there are? Now I agree that particular example sounds like a bug and I doubt this empty cookie is being used and so don’t think blocking it off will cause a breakage, but still, I’m uneasy that the stats seem wrong and that we are using this (potentially incorrect) stat to justify the low chance of breakage with this change. There might be other cookies on other sites that will cause breakage. To be clear, I don’t actually think it will cause a lot of breakage, but would rather than was based on a stat we are comfortable to stand behind than one that we have potential concerns with. Is there any way to double check that 0.12% stat given this example? Maybe a lot less users than I think are getting this cookie but given I’ve spotted it on 3 major news paper sites in 3 distinct countries, without having to look too far for these examples, makes me suspect that is not the case. Other analysis says Facebook assets are being used on 2.38% of the most used 5.7 million sites and I’d guess they all come with some cookies for free. And that stat treats each of those 5.7 million sites equally when actual usage will be heavily skewed to the sites at the top end than the tail so usage will likely be more than that as top end sites are more likely to use Facebook advertising in my opinion. And again, I’m not concerned with this Facebook example - but more that it seems to suggest the 0.12% stat feels wrong. Or maybe it’s not being included since this empty cookie doesn’t seem to be sent in follow up requests, and that’s what that stat is measuring? If that’s the reason, and we’re comfortable with that explanation and the risks that entails (I am for what it’s worth), then that’s fine, but thought it worth asking the question. |
As @chlily1 notes above, we don't have statistics on cookie size generally; just aggregate numbers on |
OK makes sense. Thanks for answering. |
The CFNetwork team approves of the change and is tracking the work in rdar://problem/58358759. |
Given that, I'd suggest that we land something like #1018 along with the tests from https://chromium-review.googlesource.com/c/chromium/src/+/1982551. Thanks! |
As discussed in [1], cookies with empty names and empty values should be rejected. This patch removes the carveout made in https://crbug.com/601786, and adjusts unittests accordingly. This patch does not change the WPT expectations; we'll do that at the same time we change the spec. In the meantime, we'll check in local expectations matching the behavior we believe is correct. [1]: httpwg/http-extensions#159 (comment) Bug: 1037996, 601786 Change-Id: I53319cee385efff019b313479184236c53b1d783
As discussed in [1], cookies with empty names and empty values should be rejected. This patch removes the carveout made in https://crbug.com/601786, and adjusts unittests accordingly. This patch does not change the WPT expectations; we'll do that at the same time we change the spec. In the meantime, we'll check in local expectations matching the behavior we believe is correct. [1]: httpwg/http-extensions#159 (comment) Bug: 1037996, 601786 Change-Id: I53319cee385efff019b313479184236c53b1d783 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982549 Reviewed-by: Lily Chen <chlily@chromium.org> Commit-Queue: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/master@{#729403}
As discussed in [1], cookies with empty names and empty values should be rejected. This patch removes the carveout made in https://crbug.com/601786, and adjusts unittests accordingly. This patch does not change the WPT expectations; we'll do that at the same time we change the spec. In the meantime, we'll check in local expectations matching the behavior we believe is correct. [1]: httpwg/http-extensions#159 (comment) Bug: 1037996, 601786 Change-Id: I53319cee385efff019b313479184236c53b1d783 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982549 Reviewed-by: Lily Chen <chlily@chromium.org> Commit-Queue: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/master@{#729403}
Based on the discussion in [1], we should change the spec to support nameless cookies parsed from headers like `Set-Cookie: foo`. This patch updates the WPT expectations accordingly. [1]: httpwg/http-extensions#159 Bug: 1037996 Change-Id: Iefabea524377857d524c74457153347a107401ee
As discussed in [1], cookies with empty names and empty values should be rejected. This patch removes the carveout made in https://crbug.com/601786, and adjusts unittests accordingly. This patch does not change the WPT expectations; we'll do that at the same time we change the spec. In the meantime, we'll check in local expectations matching the behavior we believe is correct. [1]: httpwg/http-extensions#159 (comment) Bug: 1037996, 601786 Change-Id: I53319cee385efff019b313479184236c53b1d783 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982549 Reviewed-by: Lily Chen <chlily@chromium.org> Commit-Queue: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/master@{#729403}
Based on the discussion in [1], we should change the spec to support nameless cookies parsed from headers like `Set-Cookie: foo`. This patch updates the WPT expectations accordingly. [1]: httpwg/http-extensions#159 Bug: 1037996 Change-Id: Iefabea524377857d524c74457153347a107401ee
Based on the discussion in [1], we should change the spec to support nameless cookies parsed from headers like `Set-Cookie: foo`. This patch updates the WPT expectations accordingly. [1]: httpwg/http-extensions#159 Bug: 1037996 Change-Id: Iefabea524377857d524c74457153347a107401ee Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982551 Reviewed-by: Lily Chen <chlily@chromium.org> Commit-Queue: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/master@{#729807}
Based on the discussion in [1], we should change the spec to support nameless cookies parsed from headers like `Set-Cookie: foo`. This patch updates the WPT expectations accordingly. [1]: httpwg/http-extensions#159 Bug: 1037996 Change-Id: Iefabea524377857d524c74457153347a107401ee Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982551 Reviewed-by: Lily Chen <chlily@chromium.org> Commit-Queue: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/master@{#729807}
Based on the discussion in [1], we should change the spec to support nameless cookies parsed from headers like `Set-Cookie: foo`. This patch updates the WPT expectations accordingly. [1]: httpwg/http-extensions#159 Bug: 1037996 Change-Id: Iefabea524377857d524c74457153347a107401ee Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982551 Reviewed-by: Lily Chen <chlily@chromium.org> Commit-Queue: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/master@{#729807}
This patch alters the cookie parsing algorithm to treat `Set-Cookie: token` as creating a cookie with an empty name and a value of "token". It also rejects cookies with neither names nor values (e.g. `Set-Cookie:` and `Set-Cookie: =`. Closes #159.
…es., a=testonly Automatic update from web-platform-tests Reject cookies with empty names and values. As discussed in [1], cookies with empty names and empty values should be rejected. This patch removes the carveout made in https://crbug.com/601786, and adjusts unittests accordingly. This patch does not change the WPT expectations; we'll do that at the same time we change the spec. In the meantime, we'll check in local expectations matching the behavior we believe is correct. [1]: httpwg/http-extensions#159 (comment) Bug: 1037996, 601786 Change-Id: I53319cee385efff019b313479184236c53b1d783 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982549 Reviewed-by: Lily Chen <chlily@chromium.org> Commit-Queue: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/master@{#729403} -- wpt-commits: 3adc15d3ba2e20e9ae79daf019cb784934f0fa7f wpt-pr: 20923
…es., a=testonly Automatic update from web-platform-tests Reject cookies with empty names and values. As discussed in [1], cookies with empty names and empty values should be rejected. This patch removes the carveout made in https://crbug.com/601786, and adjusts unittests accordingly. This patch does not change the WPT expectations; we'll do that at the same time we change the spec. In the meantime, we'll check in local expectations matching the behavior we believe is correct. [1]: httpwg/http-extensions#159 (comment) Bug: 1037996, 601786 Change-Id: I53319cee385efff019b313479184236c53b1d783 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982549 Reviewed-by: Lily Chen <chlily@chromium.org> Commit-Queue: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/master@{#729403} -- wpt-commits: 3adc15d3ba2e20e9ae79daf019cb784934f0fa7f wpt-pr: 20923
…es., a=testonly Automatic update from web-platform-tests Reject cookies with empty names and values. As discussed in [1], cookies with empty names and empty values should be rejected. This patch removes the carveout made in https://crbug.com/601786, and adjusts unittests accordingly. This patch does not change the WPT expectations; we'll do that at the same time we change the spec. In the meantime, we'll check in local expectations matching the behavior we believe is correct. [1]: httpwg/http-extensions#159 (comment) Bug: 1037996, 601786 Change-Id: I53319cee385efff019b313479184236c53b1d783 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982549 Reviewed-by: Lily Chen <chlilychromium.org> Commit-Queue: Mike West <mkwstchromium.org> Cr-Commit-Position: refs/heads/master{#729403} -- wpt-commits: 3adc15d3ba2e20e9ae79daf019cb784934f0fa7f wpt-pr: 20923 UltraBlame original commit: 36b1685e6888bdd720505b9a1df2db3d15f7691f
…es., a=testonly Automatic update from web-platform-tests Reject cookies with empty names and values. As discussed in [1], cookies with empty names and empty values should be rejected. This patch removes the carveout made in https://crbug.com/601786, and adjusts unittests accordingly. This patch does not change the WPT expectations; we'll do that at the same time we change the spec. In the meantime, we'll check in local expectations matching the behavior we believe is correct. [1]: httpwg/http-extensions#159 (comment) Bug: 1037996, 601786 Change-Id: I53319cee385efff019b313479184236c53b1d783 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982549 Reviewed-by: Lily Chen <chlilychromium.org> Commit-Queue: Mike West <mkwstchromium.org> Cr-Commit-Position: refs/heads/master{#729403} -- wpt-commits: 3adc15d3ba2e20e9ae79daf019cb784934f0fa7f wpt-pr: 20923 UltraBlame original commit: 36b1685e6888bdd720505b9a1df2db3d15f7691f
…t nameless cookies., a=testonly Automatic update from web-platform-tests Update cookie WPT expectations to support nameless cookies. Based on the discussion in [1], we should change the spec to support nameless cookies parsed from headers like `Set-Cookie: foo`. This patch updates the WPT expectations accordingly. [1]: httpwg/http-extensions#159 Bug: 1037996 Change-Id: Iefabea524377857d524c74457153347a107401ee Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982551 Reviewed-by: Lily Chen <chlily@chromium.org> Commit-Queue: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/master@{#729807} -- wpt-commits: ceb4b92d4fea4a2559ecb02ab3c6c8c06134dd75 wpt-pr: 21093
…t nameless cookies., a=testonly Automatic update from web-platform-tests Update cookie WPT expectations to support nameless cookies. Based on the discussion in [1], we should change the spec to support nameless cookies parsed from headers like `Set-Cookie: foo`. This patch updates the WPT expectations accordingly. [1]: httpwg/http-extensions#159 Bug: 1037996 Change-Id: Iefabea524377857d524c74457153347a107401ee Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1982551 Reviewed-by: Lily Chen <chlily@chromium.org> Commit-Queue: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/master@{#729807} -- wpt-commits: ceb4b92d4fea4a2559ecb02ab3c6c8c06134dd75 wpt-pr: 21093
This is based on the recent changes in httpwg/http-extensions#159 and httpwg/http-extensions#1018. Differential Revision: https://phabricator.services.mozilla.com/D60465 --HG-- extra : moz-landing-system : lando
This is based on the recent changes in httpwg/http-extensions#159 and httpwg/http-extensions#1018. Differential Revision: https://phabricator.services.mozilla.com/D60465
As a part of discussion in whatwg/html#804 I've made a research of the modern browsers compatibility with the RFC 6265. It appears that all browsers nowadays allows cookies without key or (in case of Safari) without value, thus making it de facto standard. However, it's debatable how cookies like
foo;
should be treated: as the cookie without value or without key. Thinking of cookie jar as some kind of key/value store makes it more logical to treat such cookie as cookies without value, but on the other hand, currently most implementers treats them as the cookies with the special empty key.The text was updated successfully, but these errors were encountered: