HttpClient: Try decode Location header using UTF-8 - #37852
Conversation
| { | ||
| if (input[i] > 127) | ||
| { | ||
| possibleUtf8 = true; |
There was a problem hiding this comment.
Doesn't ISO-8859-1 use the full 256 range? How likely is it going to be then that this detects "possibleUtf8" but it's really just ISO-8859-1? It'll then fail to decode as UTF-8, throwing and catching an exception, with is relatively very expensive.
There was a problem hiding this comment.
This particular piece of code was taken from desktop where I have not heard of any related issues so I added it here for compat.
I've read various comments on the internet saying that in many languages you cannot write anything useful using ISO-8859-1 because range of characters is too small in which cases they would url/mime encode their address.
I think this will be a trade-off regardless what we do.
Recommendation I've seen in various places is that clients should not try to interpret the header values and just use the raw bytes instead.
Other places mention that only US-ASCII is guaranteed to work.
There is also https://stackoverflow.com/questions/4400678/what-character-encoding-should-i-use-for-a-http-header
and @davidsh comment saying that various implementations support UTF-8.
I think given above it makes sense to leave the desktop behavior and possibly adjust this in the future (perhaps by adding new API such as HeaderEncoding or something similar).
There was a problem hiding this comment.
Also spec says it should be US-ASCII so we are just making a favor https://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2 😄 (although to be completely fair I've seen some comments that it used to say ISO-8859-1 in the past)
There was a problem hiding this comment.
The TEXT rule is only used for descriptive field contents and values that are not intended to be interpreted by the message parser. Words of *TEXT MAY contain characters from character sets other than ISO- 8859-1 [22] only when encoded according to the rules of RFC 2047 [14].
There was a problem hiding this comment.
Main goal of this was compatibility with framework.
The ISO-8859-1 simply assigns characters to byte values. We can %encode byte values if we fail to decode them as utf-8. Since that does not conform to spec I would think the behavior may not be well defined but it should be safe IMHO from code prospective. It would be interesting IMHO to know how browsers handle that but that could also be done in separate effort.
There was a problem hiding this comment.
It seems kind of weird to me to special-case this for the Location header. It looks to me like the referenced desktop code is trying UTF8 for all header values (though I'm not sure about that).
Right now we always interpret header values as ISO-8859-1. It's not clear to me how valuable this is. Possibly, we should try decoding all header values as UTF8, and only fall back to ISO-8859-1 if that fails.
There was a problem hiding this comment.
actually per https://referencesource.microsoft.com/#System/net/System/Net/WebHeaderCollection.cs,55ef10d3082a9232,references only Location is special.
I do not have enough info to pull the trigger on all of the header values 😄
There was a problem hiding this comment.
Huh, interesting. Thanks.
I still think it's weird to treat Location differently than other headers, but at least there is precedent...
There was a problem hiding this comment.
I agree @geoffkizer. I think Location is only one which matters in practice as we use it in redirects. I suspect that doing only that is for performance reasons.
davidsh
left a comment
davidsh
left a comment
There was a problem hiding this comment.
Left some comments but otherwise LGTM.
Also, please run all Outerloop tests before final merge.
|
@ViktorHofer the failures/timeout seem to be unrelated to my change - could you verify if this is some known issue? |
|
Yes that's a known one: #38343 |
* Use desktop logic for parsing Location (try decode using UTF-8) * Tests and feedback * fix netfx * Convert UTF-8 in non-allocating way without try .. catch Commit migrated from dotnet/corefx@0202690
9 participants
Fixes: https://github.com/dotnet/corefx/issues/35103
Some websites such as i.e. https://www.ilna.ir/fa/tiny/news-724227 (link from repro)
when asking for redirect put utf-8 characters in the Location header - currently all browsers and desktop implementation of HttpClient can handle such case.
This change is moving the desktop logic which is checking if Location contains any characters higher than 127 and if it does it tries to decode using UTF-8 (if that fails it falls back to the old behavior which is ISO-8859-1)
Desktop code:
https://referencesource.microsoft.com/#System/net/System/Net/WebHeaderCollection.cs,1126
You can note that there is only one usage for Location header (every other header uses ISO-8859-1)
https://referencesource.microsoft.com/#System/net/System/Net/WebHeaderCollection.cs,175