rfc6265bis UA requirements and WPT mismatch

[essen]

I have implemented most of rfc6265bis from scratch for an HTTP client library, using the algorithms described in Section 5. I have found a few cases in WPT's http-state suite that do not pass and seem to go against the current editor draft of rfc6265bis (or vice versa). I figured this would be useful to share:

• attribute0023: Test Expected The current spec takes the last path while the test expects the first path to be used?
• chromium0009: Test Expected The current spec accepts this with the cookie name being empty.
• chromium0010: Test Expected The current spec accepts this with the cookie name being empty.
• chromium0012: Test Expected The current spec accepts this with the cookie name being empty.
• disabled-chromium0020: Test Expected The current spec accepts this with the cookie name being empty. Maybe this is a test of Chromium's limit on cookie names, but the current spec doesn't parse it as a name.
• disabled-chromium0022: Test Expected I'm not sure what should be expected to be honest.
• mozilla0012: Test Expected I expect the second Set-Cookie to also be sent back to the server, with an empty name.
• mozilla0014: Test Expected The current spec accepts this with the cookie name being empty.
• mozilla0015: Test Expected The current spec accepts this with the cookie names being empty.
• mozilla0016: Test Expected The current spec accepts this with the cookie names being empty.
• mozilla0017: Test Expected The current spec accepts this with the cookie names being empty, except for the last Set-Cookie which has a name provided.
• name0017: Test Expected The current spec accepts this with the cookie name being empty and the value everything after the first =.
• name0023: Test Expected The current spec accepts this with the cookie name being empty.
• name0025: Test Expected The current spec accepts this with the cookie name being empty and the value everything after the first =.
• name0028: Test Expected The current spec accepts this with the cookie name being empty and the value everything after the first =.
• name0031: Test Expected The current spec accepts this with the cookie name being "foo;bar" and the value everything after the first =.
• name0032: Test Expected The current spec accepts this with the cookie name being "foo\"bar;baz" and the value everything after the first =.
• name0033: Test Expected The current spec accepts this with the first cookie name being empty and the value everything after the first =, and the second cookie name being empty and the value being aaa.
• optional-domain0042: Test Expected The current spec takes the last domain while the test expects the first domain to be used?

Hope it's useful.

Everything else works, although note that I have not implemented SameSite checks at this point so I cannot comment on those.

[mikewest]

Thanks for this. That WPT directory is a straight port from the original test repository at https://github.com/abarth/http-state, and I still need to do a pass through them to sanity check them against the spec (and vice-versa). I appreciate you going through the tests and spec, and I'll try to honor that work by investing some time in cleaning up the repository this week.

[mikewest]

A few of these seem easy:

• The change in Allow cookies without key or value #159 is responsible for:

  • chromium0009
  • chromium0010
  • chromium0012
  • mozilla0012
  • mozilla0014
  • mozilla0015
  • mozilla0016
  • mozilla0017
  • name0017
  • name0023
  • name0025
  • name0033

  Addressing these in Adjusting //cookie/http-state to accept nameless cookies. web-platform-tests/wpt#22560 (https://chromium-review.googlesource.com/c/chromium/src/+/2129528).

• After the change above, name0031 and name0032 show Chrome and Firefox aligned on treating "foo;bar"=baz as a nameless cookie with a value of "foo, and "foo\"bar;baz"=qux as a nameless cookie with a value of "foo\"bar. That is, they both stop parsing the name/value pair when they hit a ;. This seems aligned with step 1 of https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-05#section-5.3, which defines the name-value-pair as everything up to but not including the first ;.

  Addressing these in Adjusting //cookie/http-state tests name0031 and name0032. web-platform-tests/wpt#22559 (https://chromium-review.googlesource.com/c/chromium/src/+/2128135).

I'll look at the remainder after lunch.

[essen]

About name0031 and name0032 you are of course right, my analysis above was incorrect. My implementation does the same as Chromium/Firefox and the draft.

[essen]

For name0017 the updated test shows an unfortunate effect of the parsing algorithms, where the server may send a cookie without a name and a value containing equal signs, and then when it parses the cookie header it gets a cookie with name a and the value after the equal sign. name0025 is a similar scenario.

Maybe this issue with empty names and equal signs in the value should be mentioned specifically in the Implementation Considerations section?

Edit: I've updated the description to better show which tests have been fixed in WPT, at the time of writing this comment.

[miketaylr]

@essen @mikewest - I had a look at the remaining tests with open questions (I'm assuming that the tests with checkmarks from the OP are no longer problematic). It seems like of the 4, disabled-chromium0022 is the most confusing and could possibly use some spec language tweaks.

• attribute0023: Test Expected The current spec takes the last path while the test expects the first path to be used?

This test seems like it was just incorrect -- it was fixed downstream by Ehsan in https://bugzilla.mozilla.org/show_bug.cgi?id=1549389.

---

• disabled-chromium0020: Test Expected The current spec accepts this with the cookie name being empty. Maybe this is a test of Chromium's limit on cookie names, but the current spec doesn't parse it as a name.

So this test has 4097 as. Section 6.1 mentions a limit of 4096 bytes ("as measured by the sum of the length of the cookie's name, value, and attributes")

Chromium: https://source.chromium.org/chromium/chromium/src/+/master:net/cookies/parsed_cookie.h;l=25?q=4096%20cookie&ss=chromium
Gecko: https://searchfox.org/mozilla-central/source/netwerk/cookie/CookieCommons.h#58
(I'm not aware of a nice code-search tool for WebKit, but it passes the test, so presumably it does the same thing).

This test is fine, it's just kinda confusing to figure out what's happening at a glance.

---

• disabled-chromium0022: Test Expected I'm not sure what should be expected to be honest.

Blink and WebKit agree here, and Gecko fails this test. Looking at the extended commit message in web-platform-tests/wpt@89637ce (which added this test), that links to whatwg/xhr#165, which likely explains why Gecko fails this test -- it doesn't (yet?) reject null bytes in the header. But... the test case shows that Chrome and Safari are just ignoring the content after \n, while Firefox is parsing as "AAA=BB; ZYX". AFAICT, this test doesn't even have \0 (NULL) in it, but \n. 🤔

And whatwg/xhr#165 (comment) suggests that LF can be legal (in obs-fold) as a value.

Also, cookie-octet contains US-ASCII characters excluding CTLs, and CTL is defined as %x00-1F / %x7F. A reasonable way to read that is to expect a parser to stop consuming US-ASCII once it hit CTL (at least that's how I interpreted it).

I see in https://source.chromium.org/chromium/chromium/src/+/master:net/cookies/parsed_cookie.cc;l=362-372;drc=1f5bcd95363404f6c80dcf24730feb78cb4b1187?q=FindFirstTerminator&ss=chromium - that's pretty much what Chromium does.

But in Step 1 of https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-06#section-5.3, in the Otherwise clause (which is where this test ends up because it lacks ";"), it says:

1. The name-value-pair string consists of all the characters
   contained in the set-cookie-string, and the unparsed-
   attributes is the empty string.

Maybe that language could use some tightening to make it more explicit?

---

• optional-domain0042: Test Expected The current spec takes the last domain while the test expects the first domain to be used?

Currently the test looks like Set-Cookie: foo=bar; domain=foo.example.org; domain= -- so this seems like expected behavior?

[essen]

• attribute0023: Test Expected The current spec takes the last path while the test expects the first path to be used?

This test seems like it was just incorrect -- it was fixed downstream by Ehsan in https://bugzilla.mozilla.org/show_bug.cgi?id=1549389.

Right but I had this fix when I tested. I may be confused as to why it's not supposed to set or return the cookie, what the test paths are supposed to be, or just because I'm not familiar enough with Javascript. Could you clarify whether the cookie should be set or not, and explain what goes on in that particular test case?

From my reading of the spec, the set-cookie will lead to the second Path attribute to be accepted, and later on when doing a Path-Match it will use / (because there is only one slash) which matches and therefore send the cookie.

• disabled-chromium0020: Test Expected The current spec accepts this with the cookie name being empty. Maybe this is a test of Chromium's limit on cookie names, but the current spec doesn't parse it as a name.

So this test has 4097 as. Section 6.1 mentions a limit of 4096 bytes ("as measured by the sum of the length of the cookie's name, value, and attributes")

OK so failure depends on configuration. Thanks for figuring it out!

• disabled-chromium0022: Test Expected I'm not sure what should be expected to be honest.

Blink and WebKit agree here, and Gecko fails this test. Looking at the extended commit message in web-platform-tests/wpt@89637ce (which added this test), that links to whatwg/xhr#165, which likely explains why Gecko fails this test -- it doesn't (yet?) reject null bytes in the header. But... the test case shows that Chrome and Safari are just ignoring the content after \n, while Firefox is parsing as "AAA=BB; ZYX". AFAICT, this test doesn't even have \0 (NULL) in it, but \n.

And whatwg/xhr#165 (comment) suggests that LF can be legal (in obs-fold) as a value.

obs-fold is CRLF 1*( SP / HTAB ) so that's probably not it.

I think it might just be a second header that doesn't have a colon or value. I don't think this belongs in a cookie test suite.

• optional-domain0042: Test Expected The current spec takes the last domain while the test expects the first domain to be used?

Currently the test looks like Set-Cookie: foo=bar; domain=foo.example.org; domain= -- so this seems like expected behavior?

OK then it takes the last Domain attribute and I think we fall under this rule:

If the attribute-value is empty, the behavior is undefined. However, the user agent SHOULD ignore the cookie-av entirely.

https://httpwg.org/http-extensions/draft-ietf-httpbis-rfc6265bis.html#name-the-domain-attribute-2

And in this case this would be my code that doesn't respect this rule. It currently treats missing Domain and empty Domain the same.

Thanks for the help!

[miketaylr]

Right but I had this fix when I tested. I may be confused as to why it's not supposed to set or return the cookie, what the test paths are supposed to be, or just because I'm not familiar enough with Javascript. Could you clarify whether the cookie should be set or not, and explain what goes on in that particular test case?

From my reading of the spec, the set-cookie will lead to the second Path attribute to be accepted, and later on when doing a Path-Match it will use / (because there is only one slash) which matches and therefore send the cookie.

As written today, I think the expectation of not setting the cookie is correct. But I think it's largely an accident of how the original tests were ported over to wpt, and due to the fact that it's not being sent to the path / set on the path it wants. I believe the intent of this test was to test that an invalid path, followed by a valid path should set the cookie (basically the inverse of attribute0024).

So for the test header,Set-Cookie: foo=bar; Path=/qux; Path=/cookie-parser-result:

If you look at the original test server code, you can see there is a /cookie-parser-result HTTP endpoint:

https://github.com/abarth/http-state/blob/master/tools/testserver/testserver.py#L105-L134

But the iframe that the current WPT serves the cookie from is /cookies/http-state/resources/cookie-setter.py?file=attribute0023. So it'll fail to set the cookie.

I plan to fix this, thanks for flagging!

[miketaylr]

(we can probably close this issue once I land the fix for that test, I think?)

[essen]

Right I get that WPT doesn't have the right path but I'm running tests that do (I reimplemented them in my test suite).

As written today, I think the expectation of not setting the cookie is correct. But I think it's largely an accident of how the original tests were ported over to wpt, and due to the fact that it's not being sent to the path / set on the path it wants. I believe the intent of this test was to test that an invalid path, followed by a valid path should set the cookie (basically the inverse of attribute0024).

That's what I mean, if the second path is the one accepted and assuming the paths used to do the requests are correct then the attribute0023-expected file should not be empty. Perhaps browsers used to use the first path before and then this was changed to the last path in the spec?

(we can probably close this issue once I land the fix for that test, I think?)

disabled-chromium0022 is still weird. The exact contents of the file are Set-Cookie: AAA=BB\rZYX\n.

(I have updated the top post to reflect the current state.)

[essen]

I think I have figured out exactly what is expected for optional-domain0042.

The test case is as follow and the cookie ends up not being sent:

Set-Cookie: foo=bar; domain=foo.example.org; domain=
Location: http://home.example.org:8888/cookie-parser-result?optional-domain0042

The spec says:

   If the attribute-name case-insensitively matches the string "Domain",
   the user agent MUST process the cookie-av as follows.

   1.  If the attribute-value is empty, the behavior is undefined.
       However, the user agent SHOULD ignore the cookie-av entirely.

This leads to the empty domain attribute to be removed. If the spec is followed strictly (it's a SHOULD), then the set-cookie header is ultimately equivalent to:

Set-Cookie: foo=bar; domain=foo.example.org

This leads to the cookie not being stored because we are on the home.example.org host.

It would be good to add tests such as:

Set-Cookie: foo=bar; domain=home.example.org; domain=

In this case the cookie should be stored and sent in the next request to home.example.org because the second domain attribute is dropped according to the spec.

It would also be good to remove this part about the behavior being undefined from the spec. This is the only place in the spec that has uncertainty. It's also mildly confusing to only remove the cookie-av, perhaps the cookie should be invalidated entirely. (cc @mikewest about this undefined behavior)

[miketaylr]

It would also be good to remove this part about the behavior being undefined from the spec. This is the only place in the spec that has uncertainty. It's also mildly confusing to only remove the cookie-av, perhaps the cookie should be invalidated entirely. (cc @mikewest about this undefined behavior)

Could you file a new issue for this (with the recommended test)? It's possible this suggestion gets lost in this issue about WPTs. Thanks!

[miketaylr]

FWIW, I'm in the middle of re-writing all the http-state tests (meta bug over at https://bugs.chromium.org/p/chromium/issues/detail?id=1152796, which will have more bugs as I progress).

As for https://github.com/web-platform-tests/wpt/blob/master/cookies/http-state/resources/test-files/attribute0023-test (which is 404 now, because I deleted it, but it used to look like this), it evolved into "No cookie returned for multiple mismatched paths", because even our best guesses of what attribute0023 was intending to capture don't reflect current browser reality.

I'll be tackling the "chromium" tests next, so I guess I'll be forced to reckon with disabled-chromium0022 and then we can close this issue.

[essen]

Submitted as #1332.

[miketaylr]

OK, I totally forgot to circle back to disabled-chromium0022, this got re-written as follows:

https://github.com/web-platform-tests/wpt/blob/489679e4fccedcb37ae4d3e028b5d1a56adbc769/cookies/value/value.html#L84-L88

  {
    cookie: `test=13\nZYX`,
    expected: "test=13",
    name: "Set cookie but ignore value after LF",
  },

It took me a minute to page back into memory, but my understanding is as follows:

A valid cookie-value is defined as *cookie-octet / ( DQUOTE *cookie-octet DQUOTE ). cookie-octet is basically all the ASCII octets, with a few exceptions carved out, namely CTLs.

https://tools.ietf.org/html/rfc5234#appendix-B.1 defines CTL as %x00-1F / %x7F. And LF (\n) is %x0a, so it's not a valid value.

At least Firefox and Chrome agree with that... but I don't really see in the current draft where it says to consume up until an invalid value, or line break. @chlily1, am I missing something obvious?

Maybe it's less complicated than that -- is it just looking for the first CRLF in the header to know where to stop parsing? Should the draft say something to that effect?

[chlily1]

Hm, I don't think you're missing anything. As far as I can tell, in section 5.3, it says to use all the characters after the first '=' as the value string.

[miketaylr]

Different rationales, but same result: Chromium and Gecko are treating CRLF as basically "the end of the cookie".

https://source.chromium.org/chromium/chromium/src/+/master:net/cookies/parsed_cookie.cc;l=252-261;drc=b35da1f38c91cbc8a3ec0f934b747360fa9a0f73;bpv=1;bpt=1

https://searchfox.org/mozilla-central/source/netwerk/cookie/CookieService.cpp#1205-1259

[essen]

The ABNF is much stricter than the parsing algorithm described in the draft. The parsing algorithm in the draft does not disallow CR, LF, NUL or any other invalid character. Perhaps it should, to reflect what browsers are doing. In that case I would suggest adding tests with CR, LF and NUL separately, at least, to detect and possibly correct differences.

[chlily1]

I have just filed issue #1399 regarding this and related issues.

[chlily1]

Since all the other issues have been addressed, I'm going to close this and we can follow #1399 for the final thing.