[Breaking Change Request] Fix HeaderValue parsing, toString(), and support null values

[sortie]

Note: This proposed behavior has been amended, see below for the current proposal

Intended change

The HeaderValue class will now parse parameters without a value as having the empty string as value instead of a null value. This is the class used to parse semicolon delimited parameters used in the Accept, Authorization, Content-Type, and other such HTTP headers. RFC 7231 section 3.1.1.1 and other locations require such parameters to have a value, but HeaderValue was parsing empty parameters as null value which was wrong according to the specification. The Sec-WebSocket-Extensions websocket header is a special case that allows absent values. The HeaderValues class now parses absent values and empty values as the empty string. For instance

HeaderValue.parse('a; b=B; c; d=; e=E').parameters

used to give {b: "B", c: null, d: null, e: "E"} but now gives {b: "B", c: "", d: "", e: "E"}.

Additionally HeaderValue.toString() was incorrectly not quoting the empty parameter values, which wasn't allowed by the specification. I.e.

new HeaderValue("a", {"b": ""})

used to give text/plain; q= but now gives text/plain; q="".

Rationale

The non-nullable-by-default migration required making subtle changes to some dart:io semantics in order to provide a better API. The HeaderValue.parameters map never contains null values except if parameters not allowed by the standard have been parsed, or in the special case of Sec-WebSocket-Extensions. To avoid code using these parameters from needing to null check the parsed parameter values, these invalid parameters are instead parsed as the empty string.

These absent values also did not survice a roundtrip through HeaderValue as HeaderValue.parse('a; b') used to give a; b=null where null has no special meaning, but will now give a; b="" which is closer to the original input. That means HeaderValue could not and still cannot be used and to construct the Sec-WebSocket-Extensions parameters that don't have values.

Expected impact

HTTP headers such as Accept, Authorization, Content-Type that abide by the standard will be unaffected.

Code handling the Sec-WebSocket-Extensions using HeaderValue may be affected if it doesn't simply check if the parameter is set in the map, but instead handles an empty string differently from null. The relevant code and tests in the SDK has been updated.

It's hard to rule out if someone has built other non-standard extensions, where the class has been used for valueless parameters, such as foo; bar where the header's value is foo with the bar parameter, which is handled distinctly from if the bar parameter had a value (foo; bar=qux). Though the HeaderValue can only receive such invalid parameters, as it was not able to construct them, instead producing foo; bar=null.

Steps for mitigation

If code relies on HeaderValue returns has a null value for valueless parameters, it'll need to be updated to allow the empty string as well. However, if code relies on being able to tell a parameter without a value apart from a parameter with the empty string, then head then the HeaderValue class can't solve that problem anymore and one will instead need to build a custom parser.

Alternatively if this sort of non-standard valueless parameters turns out to exist in practice or there's non-SDK code parsing Sec-WebSocket-Extensions, we can decide not to make this breaking change, and embrace this non-standard behavior as a feature.

Implementation

This change was implemented in https://dart-review.googlesource.com/c/sdk/+/136620.

Note: This proposed behavior has been amended, see below for the current proposal

cc @franklinyow @mit-mit @lrhn @athomas

[franklinyow]

cc @Hixie @matanlurey @dgrove @vsmenon for review and approval.

[Hixie]

It would also break Accept header value parsing.

[Hixie]

I don't have an objection to this change. I think it's fine to say that HeaderValue is only useful for the subset of headers that have the syntax it expects. In practice each header has its own syntax and so it's perfectly reasonable for different headers to have different semantics here, and no API is going to be able to support all of them regardless.

That said, we should probably just deprecate this class entirely. As far as I can tell, it doesn't provide an interface that works for any header value. As noted above, Accept and Sec-WebSocket-Extensions don't work with this because it can't represent missing values in parameter extensions (not to mention that an Accept API should probably expose q weights as an int, should expose the default weight as 1000, etc). But there are lots of other differences. For example, TE headers allow whitespace around the = character of all their parameters except the q= value. The API only allows one parameter value per name, but there's nothing in the specs that says that parameter values in general can't be listed multiple times. The API doesn't reflect the order of the parameters, but in principle the order can be meaningful (Accept does this, for instance, with the "q" parameter separating the media range parameters from the accept extension parameters). There's probably other theoretical cases where this API doesn't quite work.

[sortie]

@Hixie Note that the Accept header and others are supposed to be parsed by first splitting on the commas, and then parsing each of those using HeaderValue. The HeaderValue class was never able to parse a full Accept header on its own, despite that the documentation seems to imply so. With that in mind, I don't see how this change breaks parsing of the Accept header compared to the current situation? Can you give an example? I do understand the current situation is suboptimal, the question is whether this change breaks anything additional.

[Hixie]

Accept allows the =value part to be omitted, it's the same problem as Sec-WebSocket-Extensions.

[matanlurey]

No input

[vsmenon]

lgtm

[leafpetersen]

I'm reading the status of this as approved for the breaking change, yes?

[franklinyow]

Approved