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

IDN names for domain attributes #1707

Closed
martinthomson opened this issue Sep 30, 2021 · 11 comments
Closed

IDN names for domain attributes #1707

martinthomson opened this issue Sep 30, 2021 · 11 comments
Labels

Comments

@martinthomson
Copy link
Contributor

We should specify what to do if the domain attribute in a cookie is:

  1. punycode (A-labels, xn--etc)
  2. unicode (U-labels)

I think that the right behaviour here is to only accept punycode, but we should ensure that this is what people do.

(This is a small piece of #1073 that might just be tractable.)

@miketaylr
Copy link
Collaborator

https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis#section-5.1.2 step 2 refers to punycode, but I don't see anywhere that actually refers to a "a canonicalized host name" to do that work. However, there is a reference to "canonicalized request-host"s... that just magically exist I guess (in steps 8 and 9 of https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis#section-5.5).

@annevk
Copy link

annevk commented Dec 8, 2021

https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis#section-5.4 defines the parser and doesn't reject non-ASCII bytes outright. https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis#section-5.4.3 doesn't appear to deal with them either.

And then in https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis#section-5.5 it assumes the value given is a domain...

So yeah, that does raise the question what ought to happen.

And not just with non-ASCII bytes, but with regards to IP addresses as well.

@johannhof
Copy link
Collaborator

Trying to write a WPT test for this right now and it looks like Firefox is the only browser not accepting unicode domains (and only accepts punycode). The other two seem to be able to deal with the unicode well enough.

@annevk
Copy link

annevk commented Jan 20, 2022

Are you testing document.cookie or Set-Cookie? If the latter, UTF-8 bytes then? Anything special happening for malformed UTF-8 sequences?

@johannhof
Copy link
Collaborator

Did you mean document.cookie? And yeah, I'm going to try and add additional tests for malformed or control characters (I noticed there's some history with that), to the extent possible with wpt. Feel free to ping me if you have specific recommendations.

@annevk
Copy link

annevk commented Jan 20, 2022

Yeah I did.

johannhof added a commit to johannhof/wpt that referenced this issue Jan 31, 2022
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.
@johannhof
Copy link
Collaborator

I added a test in web-platform-tests/wpt#32620 which assumes that UTF-8 would be supported and it looks like the current behavior is:

  • Chrome parses UTF-8 in the domain attribute and correctly compares it to a punycode version of the site host, thus passing all tests (though we think that this wasn't an intentional choice)
  • Firefox will reject any non-ASCII characters, but I suspect this also happens rather accidentally by comparing the raw cookie domain with punycode a URL host here
  • Safari will ignore the domain attribute for non-ASCII characters, leading to situations where cookies with a "valid" cross-site domain attribute will be set.

Personally I'm not a fan of what Safari does here, but I do think either the Chrome or Firefox behaviors (if changed to be explicit rejection on non-ASCII) could be agreed upon. Talking to @sbingler on Chrome side we have a weak preference for keeping UTF-8 support but would be very interested in hearing concerns about it. We're also going to try and add measurements for the existence of non-ASCII chars in domain attributes, but getting results from that will take several months, so I don't want to block the discussion on it.

@martinthomson wrote in web-platform-tests/wpt#32620 (review)

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?

Can you elaborate more on what your concerns are with supporting UTF-8 overall and specifically in the domain attribute?

@martinthomson
Copy link
Contributor Author

My concern was rooted in the fact that HTTP was (at some time in the past) defined as using ISO-8859-1 encoding. Interpreting content as UTF-8 really is taking liberties. We've gradually moved to avoiding saying anything about anything other than a very narrow set of octet values. In particular, octets with the high bit set are basically regarded as dangerous, if not totally unusable. Many implementations pass arbitrary octets, but how they are interpreted varies.

Your test results really confirm that handling is inconsistent.

It's not merely sufficient to fix/align three implementations. There are more clients than these three that might deal with cookies.

Also, I fear that security (or policy) checks would be inconsistently applied if there are multiple ways to spell a domain name: UTF-8 or punycode.

@johnwilander
Copy link
Contributor

  • Safari will ignore the domain attribute for non-ASCII characters, leading to situations where cookies with a "valid" cross-site domain attribute will be set.

Personally I'm not a fan of what Safari does here …

I've filed this bug as rdar://88349235 (Apple-internal link).

@martinthomson
Copy link
Contributor Author

Discussed today: proposal is to reject a cookie (name-value pair) if it contains non-ASCII characters in the domain attribute and instead mandate the use of A-labels (punycode encoded names) for domain.

Rationale being the above comments.

@johannhof
Copy link
Collaborator

Thanks Martin! Given that the above seemed to be consensus on the call yesterday I'll update the tests to check for rejection of non-ASCII characters instead.

annevk pushed a commit to web-platform-tests/wpt that referenced this issue Feb 7, 2022
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.
mattwoodrow pushed a commit to mattwoodrow/wpt that referenced this issue Feb 15, 2022
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.
johannhof added a commit to johannhof/http-extensions that referenced this issue Feb 16, 2022
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 26, 2022
…tributes, a=testonly

Automatic update from web-platform-tests
Cookies: Test IDN/non-ASCII in domain attributes

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.
--

wpt-commits: 48d28a82aa28f3491e525b7c32b8bb1cdfdaf6f6
wpt-pr: 32620
DanielRyanSmith pushed a commit to DanielRyanSmith/wpt that referenced this issue Feb 28, 2022
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.
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Mar 1, 2022
…tributes, a=testonly

Automatic update from web-platform-tests
Cookies: Test IDN/non-ASCII in domain attributes

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.
--

wpt-commits: 48d28a82aa28f3491e525b7c32b8bb1cdfdaf6f6
wpt-pr: 32620
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 8, 2022
…tributes, a=testonly

Automatic update from web-platform-tests
Cookies: Test IDN/non-ASCII in domain attributes

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.
--

wpt-commits: 48d28a82aa28f3491e525b7c32b8bb1cdfdaf6f6
wpt-pr: 32620
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Mar 8, 2022
…tributes, a=testonly

Automatic update from web-platform-tests
Cookies: Test IDN/non-ASCII in domain attributes

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.
--

wpt-commits: 48d28a82aa28f3491e525b7c32b8bb1cdfdaf6f6
wpt-pr: 32620
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

5 participants