Cookies: Test IDN/non-ASCII in domain attributes #32620
Conversation
This is for httpwg/http-extensions#1707 I took some inspiration from web-platform-tests#26170 and used a python server instead of a header file in order to better control which bytes are served as part of the header. The test runs exclusively on the élève. subdomain through opening a new window that then runs all test and reports back to the main test window.
|
@annevk @sbingler @chlily1 @martinthomson any thoughts/suggestions on this? :) |
There was a problem hiding this comment.
This seems like a fine way to test.
I'm less sure about the answers though. I'd want to be really, really sure that putting UTF-8 into HTTP headers is OK, though I'm guessing that this is a case where our hand is being forced by the robustness principle. Is it?
|
@martinthomson if you're happy with the test itself maybe we should continue discussing the current state of things and the merits of UTF-8 in the header in httpwg/http-extensions#1707, I'll follow up there :) |
| let response = await fetch("idn.py?set-punycode&host={{host}}"); | ||
| assert_equals(await response.text(), "set"); | ||
| response = await fetch("idn.py?get&host={{host}}"); | ||
| await assert_cookie(response); |
There was a problem hiding this comment.
I'm not seeing this request going out on the subdomain. I think that it would be good to ensure that the request is being sent to the IDN host (perhaps even feeding fetch élève.${window.location.host}).
There was a problem hiding this comment.
So, I don't mind doing that but I'm reasonably certain there's no other way for this request to be sent than on the subdomain. This code will only run on élève.${window.location.host}. But yeah, it won't hurt either way.
There was a problem hiding this comment.
Actually, having tried this now computing the correct prefix is a bit more annoying and I'm not sure if there's a concrete benefit when the popup is guaranteed to always have the IDN as it's top-level URL. It's certainly adding complexity to the test. So unless there's some evidence that we're really not sending these on the IDN subdomain I'll leave this unresolved, if that's okay with you.
|
I've updated the test to check that IDN domains are only accepted when they're punycode and that UTF-8 is explicitly not accepted as we agreed on in httpwg/http-extensions#1707 |
|
@annevk I can't change reviewers on this repo, would you mind reviewing this and merging if you're happy with it? I think we have consensus on this as per httpwg/http-extensions#1707 |
|
There's for lowercasing and a couple of tests in https://github.com/web-platform-tests/wpt/tree/master/cookies/domain for leading dot, I think.Probably a separate issue from this, but if you notice anything missing I'm happy to add more. |
|
LGTM too (after the fact). Thanks! |
With this change, cookies with a "Domain" attribute that contains a non-ASCII character will be rejected (e.g. Domain=éxample.com). This makes Chromium adhere to the recently agreed-upon change to RFC 6265 (httpwg/http-extensions#1969) and reflects what Firefox has been shipping already. The web platform tests for this change were landed in web-platform-tests/wpt#32620 and are now passing. Bug: 1296537 Change-Id: I8fc29fcc57d7a8218e7fb257d56272adc86ffe46 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3583727 Reviewed-by: Steven Bingler <bingler@chromium.org> Commit-Queue: Johann Hofmann <johannhof@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/main@{#1010491}
This is for httpwg/http-extensions#1707
I took some inspiration from #26170 and used a python server instead of
a header file in order to better control which bytes are served as part
of the header.
The test runs exclusively on the élève. subdomain through opening a new
window that then runs all test and reports back to the main test window.