Sdk: Invalid HTTP header field values

[bnxm]

On Flutter channel beta and dev, requests to certain APIs are failing because of invalid http response header field values:

~
E/flutter (17842): [ERROR:flutter/lib/ui/ui_dart_state.cc(157)] Unhandled Exception: HttpException: Invalid HTTP header field value: "246.792µs"
E/flutter (17842): #0 _HttpHeaders._validateValue (dart:_http/http_headers.dart:626:9)
E/flutter (17842): #1 _HttpHeaders._addAll (dart:_http/http_headers.dart:75:18)
E/flutter (17842): #2 _HttpHeaders.add (dart:_http/http_headers.dart:66:5)
E/flutter (17842): #3 _HttpParser._doParse (dart:_http/http_parser.dart:719:24)
E/flutter (17842): #4 _HttpParser._parse (dart:_http/http_parser.dart:328:7)
E/flutter (17842): #5 _HttpParser._onData (dart:_http/http_parser.dart:850:5)
~

This is not the case on channel stable. Every http library I know of is affected by this (dart http, dio, etc.) when you're not on the stable channel. I also tested it with some native libraries like Retrofit on Android which are not having those issues.

[sortie]

@zichangg Is this due to any of your recent changes? Did the HttpHeaders parsing change accidentally make it into any release before it was reverted?

[zichangg]

This is because value is not UTF8 encoded. HttpParser decode the value using UTF8 and it will throw exception if it broke.

@sortie Not the recent one. But I made httpparser throwing if validation failed.

[bnxm]

But is there a reason why it has to throw? Couldn't the parser simply ignore the value as it did before (I assume)? The average developer has no influence over what the API is sending back and is probably also more interested in parsing some JSON, XML, etc. in the body. In my case this one character μ is breaking the whole request, even though the JSON that's returned is totally fine.

[zichangg]

But is there a reason why it has to throw?

Previously it was even worse. It broke without even notifying with a correct exception.

Couldn't the parser simply ignore the value as it did before (I assume)?

In terms of encoding scheme, what parser receives is only a stream of bytes. Some decoding schemes have to be applied here. We use UTF8 to decode the bytes to string.

[bnxm]

Previously it was even worse. It broke without even notifying with a correct exception.

You certainly have more insights into this. All I can say is that the exact same request is successful on stable, but fails on beta. This is the header I get on stable:

~
{
x-powered-by: rxstreamer-waqi/1.3,
connection: keep-alive,
date: Tue, 28 Apr 2020 19:45:35 GMT,
transfer-encoding: chunked,
access-control-allow-origin: *,
content-encoding: gzip,
vary: Accept-Encoding,
content-type: application/json; charset=UTF-8,
x-gen-time: 266.419µs,
server: nginx
}
~

It doesn't throw any exception and more importantly I can access the response.body.

[zichangg]

Oh. Forget about what i said. That's a different issue.
It is my mistake. https://dart-review.googlesource.com/c/sdk/+/138860

This is the fix.

[mraleph]

/cc @pcsosinski @franklinyow @mit-mit - this is a accidental breaking change to HttpClient behavior, might warrant cherry pick into stable to prevent breaking people code.

[sortie]

Here's what happened:

Dart used to parse HTTP header values as Latin-1 (by virtue of passing the individual bytes as input to String.fromCharCodes). 6a5b87f92c568f201caa95baed5581dd0f452164 began validating HTTP header names and values are composed of allowed characters in all code paths, including the HTTP parser. It wasn't caught that this change made the HTTP parser reject bytes 128 through 255, instead of parsing them as Latin-1 codepoints.

(To correct misleading information above, Dart does not decode headers as UTF-8. In fact, the problem is that the the server is sending the µ as UTF-8 which Dart used to decode as µ in Latin-1.)

This is technically a breaking change and wasn't intended/approved as such.

We should revert the breaking part of the change and cherry-pick it into the beta/stable channel.

I'll land Zichang's commit and update the cherry-pick request.

[sortie]

The fix has landed and the cherry-pick is being handled in https://github.com/dart-lang/sdk/issues/41701. We'll add a test to avoid this issue reappearing in https://github.com/dart-lang/sdk/issues/41706.