Intent to Deprecate and Remove: obsolete header values in Fetch API

[Hiroshige Hayashizaki]

Primary eng emails

shiv...@samsung.com

hiro...@chromium.org

Summary

We want to normalize and check header values in Fetch API's JavaScript interfaces according to the latest Fetch API spec [1] and RFC 7230 instead of RFC2616.

This change will make Blink to:

• Throw TypeError for header values containing the following octets: 0x01 -- 0x08, 0x0B, 0x0C, 0x0E -- 0x1F, and 0x7F (previously accepted).

• Remove leading/trailing HTTP whitespace bytes (i.e. 0x09, 0x0A, 0x0D, and 0x20) from header values (previously accepted as-is).

When header values are set in:

• Headers constructor

• Headers.append()

• Headers.set()

• Request constructor (from RequestInit.headers, RequestInit's body's content type)

• Response constructor (from ResponseInit.headers)

Currently Blink just checks the header values don't contain 0x00, 0x0A, and 0x0D.

Motivation

To follow the latest spec (Fetch API spec [1] and RFC 7230) and avoid compatibility issues in the future.

Because Fetch API is a new API for now, there is little reason to continue support for obsolete things, and compatibility risks will increase as time passes.

The impact on code complexity is low (adding a function + modifying a couple of lines).

Compatibility Risk

Very low for now because Fetch API is a relatively new API (shipped in M-40 on serviceworkers and M-42 on window/other workers).

Allowing deprecated things might result in larger compatibility risks in the future.

We leave XHR and Chromium //net stack unmodified, so Fetch API is stricter than other parts of Chromium/Blink.

Anne van Kesteren (as comment in [2]) and Ryan Sleevi [3] showed concerns about the inconsistency, but we'd like to weigh the risk of breaking existing systems more against the consistency and the benefits of updating XHRs and //net stack (and also weigh the benefit of updating Fetch API more against the consistency).

• XHR is already widely used for a long time. We inserted UseCounter for XHR's setRequestHeader, and the percentage of affected XHRs (if we updated XHRs) is about 0.04% of all XHR requests, but we are afraid that we still have chances of breaking existing systems.

• Changing //net would have larger compatibility risks and implementation costs.

Alternative implementation suggestion for web developers

Applications can use URL encoding for passing invalid octets above.

Usage information from UseCounter

No UseCounter. We expect affected application is rare.

(Also, currently Fetch API is used mainly on serviceworkers, and UseCounter doesn't report counts on serviceworkers.)

OWP launch tracking bug

None.

Entry on the feature dashboard

https://www.chromestatus.com/features/6457425448140800

Requesting approval to remove too?

Yes.

[1] https://fetch.spec.whatwg.org/

[2] https://docs.google.com/document/d/1eH4aQNaYT6PDFugUXfAcgK4J1aU7m-iu9x_YVGsNel4

[3] https://groups.google.com/a/chromium.org/forum/#!msg/net-dev/PBQ6Y_ai0jg/P1EDLokxwtoJ

[Joshua Bell]

FWIW, fetch() has broader uptake than SWs already. GitHub uses it from Window contexts if available (e.g. on the "timeline" page). As discovered when we broke text encoding via fetch a few releases back. :)

But agreed that the risk is low, though. Better to do this sooner rather than later, and if it does break anything the early adopters will give good feedback and/or fix their code.

(non-API owner or I'd LGTM)

[Domenic Denicola]

From: blin...@chromium.org [mailto:blin...@chromium.org] On Behalf Of Hiroshige Hayashizaki

> We leave XHR and Chromium //net stack unmodified, so Fetch API is stricter than other parts of Chromium/Blink.
> Anne van Kesteren (as comment in [2]) and Ryan Sleevi [3] showed concerns about the inconsistency, but we'd like to weigh the risk of breaking existing systems more against the consistency and the benefits of updating XHRs and //net stack (and also weigh the benefit of updating Fetch API more against the consistency).
> • XHR is already widely used for a long time. We inserted UseCounter for XHR's setRequestHeader, and the percentage of affected XHRs (if we updated XHRs) is about 0.04% of all XHR requests, but we are afraid that we still have chances of breaking existing systems.
> • Changing //net would have larger compatibility risks and implementation costs.

This seems bad. The Fetch Standard is meant to govern all of these areas, not just the Fetch API. As such, I think we should either fix the spec to reflect the more lenient rules of browsers, or we should try to do all of these at once. It would be a bad outcome if we got stuck in an in-between state where only some of the platform's fetches have the rules from the Fetch Standard applied to them.

[Yutaka Hirano]

I agree with HIroshige's proposal.

Blink XHR already has some non-conformant behaviors because of legacy user code. I don't like the inconsistency, but I don't think it terribly nasty compared with the current XHR situation.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Takeshi Yoshino]

On Fri, Sep 11, 2015 at 12:32 AM, Domenic Denicola <d...@domenic.me> wrote:

I understand your concern. We could deprecate it for XHR too but should have some moratorium. //net has wider customers and not all of them are governed by the Fetch algorithm. Ones which should conform to the Fetch could also be fixed later. But it takes time to sort it out and have clearer stack. Leaving the Fetch API follow the old policy until everyone is ready is just increasing compatibility risk as Hiroshige stated.

Or, do you think it might be better to leave all of them follow RFC 2616? It could still be discussed a little more.

 

[Elliott Sprehn]

Fetch is supposed to be the primitive that explains the platform. This seems like it's taking us farther away from that. I'd rather we tried to change XHR at the same time and see what breaks.

I'd suggest putting this new behavior behind a flag, turn it on for everyone and see who shouts. Worst case we merge a flag flip to stable.

[Domenic Denicola]

From: tyos...@google.com [mailto:tyos...@google.com] On Behalf Of Takeshi Yoshino

> I understand your concern. We could deprecate it for XHR too but should have some moratorium.

I guess I am worried about getting stuck in a world where Fetch has one set of rules and XHR has another. Then we have to go update the Fetch spec to have "XHR header parsing mode" and "Fetch header parsing mode" and all that. Not fun.

> //net has wider customers and not all of them are governed by the Fetch algorithm.

Curious which ones those are. Are they also not governed by RFC 7230?

> Or, do you think it might be better to leave all of them follow RFC 2616? It could still be discussed a little more.

This was part of the motivation for my question, yes. If we are not sure it's web compatible to make this change, why does Fetch/RFC 7230 differ from RFC 2616? If the motivation for the change is strong enough, then we should change it everywhere. If the motivation is weak, then we should roll back the changes in the specs.

[Anne van Kesteren]

On Fri, Sep 11, 2015 at 5:42 PM, Domenic Denicola <d...@domenic.me> wrote:
> This was part of the motivation for my question, yes. If we are not sure it's web compatible to make this change, why does Fetch/RFC 7230 differ from RFC 2616?

So I think the way this was phrased in OP was incorrect. Fetch started
using 7230 because it was a newer reference, but other than whitespace
handling we made no normative changes. However, given "[c]urrently

Blink just checks the header values don't contain 0x00, 0x0A, and

0x0D" the problem is likely that Blink was never conforming with Fetch
in the first place.


--
https://annevankesteren.nl/

[Takeshi Yoshino]

On Sat, Sep 12, 2015 at 12:42 AM, Domenic Denicola <d...@domenic.me> wrote:

From: tyos...@google.com [mailto:tyos...@google.com] On Behalf Of Takeshi Yoshino

> I understand your concern. We could deprecate it for XHR too but should have some moratorium.

I guess I am worried about getting stuck in a world where Fetch has one set of rules and XHR has another. Then we have to go update the Fetch spec to have "XHR header parsing mode" and "Fetch header parsing mode" and all that. Not fun.

Current goal is to make both conform to RFC 7230 as spec-ed in the Fetch Standard. If it turns out the goal is wrong, we can revert the stricter rule to easily align both to RFC 2616 again (though actually, we're even currently not conforming to RFC 2616 strictly as Anne said). I think we can avoid getting stuck to the situation you've described. The Fetch spec could be also updated to reflect the lessons learned.

So, I think there's no such risk, and we can just start making the Fetch stricter experimentally. IMO, it makes sense to focus on Fetch API's future than XHR given XHR is under way to being deprecated.

It's not urgent. I'm fine with postponing it a little to align with XHR.

 

> //net has wider customers and not all of them are governed by the Fetch algorithm.

Curious which ones those are. Are they also not governed by RFC 7230?

In Chromium, there're 18 users of the net::URLRequest. The ResourceHostMsg_RequestResource IPC is one of the users.

ResourceHostMsg_RequestResource has two users,

- NPAPI PluginURLFetcher

- WebURLLoader implementations

WebURLLoader implementations include

- 3 NaCl related code

- 1 NPAPI

- 1 Pepper

- several content/ level users

- and then, blink::ResourceLoader which together with DocumentThreadableLoader is supposed to implement the Fetch algorithm in Chromium.

Ideally they should also be investigated to see if they should be updated to follow RFC 7230 but so many.

 

> Or, do you think it might be better to leave all of them follow RFC 2616? It could still be discussed a little more.

This was part of the motivation for my question, yes. If we are not sure it's web compatible to make this change, why does Fetch/RFC 7230 differ from RFC 2616? If the motivation for the change is strong enough, then we should change it everywhere. If the motivation is weak, then we should roll back the changes in the specs.

The change was made on release of draft-ietf-httpbis-p1-messaging-06 I-D and made it to be in the RFC.

The changeset is http://trac.tools.ietf.org/wg/httpbis/trac/changeset/395.

Out of the 4 issues resolved by the changeset, http://trac.tools.ietf.org/wg/httpbis/trac/ticket/74 and Roy's comment at http://trac.tools.ietf.org/wg/httpbis/trac/ticket/111#comment:2 describe the idea behind the change on character restriction.

The motivation is basically to reduce complexity and reflect implementation practices.

I think this makes sense and we should follow unless there's any difficulty.

 

[Takeshi Yoshino]

Right. Sorry for lack of it. Hiroshige will follow up with some description of the current Blink implementation.

Takeshi

[Hiroshige Hayashizaki]

> but other than whitespace handling we made no normative changes.

> However, given "[c]urrently
> Blink just checks the header values don't contain 0x00, 0x0A, and
> 0x0D" the problem is likely that Blink was never conforming with Fetch
> in the first place.

My description was somehow confusing, let me summarize again.

I think the current Blink/Chromium Implementation, RFC 2616 and RFC 7230 are all different in handling of control characters (0x01 -- 0x08, 0x0B, 0x0C, 0x0E -- 0x1F, and 0x7F):

Current Implementation (//net, XHR, and Fetch API): Allowed.

RFC 2616: Allowed only if quoted by a backslash in quoted-pair.

RFC 7230: Not allowed.

"The quoted-pair rule no longer allows escaping control characters other than HTAB."

(https://tools.ietf.org/html/rfc7230#appendix-A.2)

So I think there are differences between RFC 2616 and 7230.

Checking header values strictly against RFC 2616 is somehow complex (requiring a parser to recognize quoted-pair rules).

Strict checks against RFC 7230 is much simpler (just check octets used).

[Domenic Denicola]

I guess you all in the networking team are better positioned to know what the right engineering approach is here for switching things over. I would have thought doing everything at once (including those //net consumers that need to follow RFC 7230) would be better, as it decreases the chance of getting stuck in an intermediate and inconsistent state, and decreases the number of times developers might need to change part of their setup. But if this is what the team prefers then I'm happy to defer to them.

[Anne van Kesteren]

On Mon, Sep 14, 2015 at 11:57 AM, Hiroshige Hayashizaki
<hiro...@chromium.org> wrote:
> Checking header values strictly against RFC 2616 is somehow complex
> (requiring a parser to recognize quoted-pair rules).

Hmm yeah, that would be weird. I guess the idea would be that you use
quoted-pair when you serialize the header if the value input included
0x02 or some such? It does seem better if we could serialize header
values as literals, always.

I do agree with Domenic et al though that this should be consistently
enforced. If we cannot do that we should just go back to allowing
everything but 0x00, 0x0A, and 0x0D. And serialize using quoted-pair
if needed.


--
https://annevankesteren.nl/

[David Benjamin]

(On the networking team.) If this is enforcing an HTTP invariant, putting it in net::URLRequest is, I believe, correct. While there are many consumers of URLRequest, the long tail are things like talking to random services like Sync or the like. I would not expect them to be a problem as far as enforcing rules go. The consumers are all things we control.

Certainly adding histograms within URLRequest would be an easy first step. And, yeah, I agree with others that having different rules between Fetch and XHR would be a very bad state and, above all, we should not write ourselves into that hole.

[Caitlin Potter]

I've heard reports today of broken applications in WebKit on iOS9 (which normalizes values in xhr.setRequestHeader()), which use a pattern like `setRequestHeader("Authorization", "Bearer " + someUserDefinedValue)`, which sometimes results in a value like "Bearer ", so that's the kind of thing which would typically break.

Maybe in a situation like that, breaking the app isn't really a bad thing, though.

[Hiroshige Hayashizaki]

Thanks for all inputs and suggestions!

Because the consistency between Fetch API, XHR, and //net stack is strongly preferred, I'll estimate whether updating the whole stack is realistic or not by inserting more stats code to XHR and //net to get numbers of affected header values.

As for //net,

> davidben@

> Certainly adding histograms within URLRequest would be an easy first step.

How about inserting UMA to HttpRequestHeaders and HttpResponseHeaders for the first step?

Headers are added by various layers, e.g. by HttpNetworkTransaction, so adding UMA to HttpRequestHeaders can catch all.

Stats (% of affected header values) might be a little inaccurate (e.g. a header value can be counted multiple times due to HttpRequestHeaders::MergeFrom()), but it is better than missing some headers in stats.

[Chris Bentzel]

Just checking in on this - has there been progress on the UMA for HttpRequestHeaders and HttpResponseHeaders? That seems like a good first step. It would be great to make this consistent across the different APIs.

[Takeshi Yoshino]

Yeah, Hiroshige and David are working on it: https://codereview.chromium.org/1374883002/

Takeshi

[Hiroshige Hayashizaki]

Hi,

We added UMAs (M-49 or later) to URLRequestHttpJob:

- Net.HttpRequest.ContainsInvalidHeaderValuesInRFC7230

- Net.HttpResponse.ContainsInvalidHeaderValuesInRFC7230

  (https://codereview.chromium.org/1374883002/)

and got some numbers.

Let:

A = "the number of requests(*1) that contain invalid header value(s) according to RFC 7230"

B = "the number of all requests"

C = "the number of page loads"

~0.003% (=A/B) of requests contain at least one invalid header value.

~0.1% (=A/C) of page loads is estimated to contain a request that contain at least one invalid header value. (*2)

This (~0.1%) is higher than the deprecation threshold (0.03%).

The numbers for responses are similar.

What do you think of the number?

A possible next step is to investigate what kind of invalid header values are used.

Notes:

(*1) The requests are counted per redirect leg.

(*2) I just calculated ratio of A/C, so the actual ratio of page loads that contains invalid header values might be lower if some page loads contains multiple requests that contains invalid header values.

[Philip Jägenstedt]

Interesting! Finding out in which ways the headers are usually invalid sounds like a good idea. If it doesn't look feasible to enforce the requirements of RFC7230 then ideally that RFC should be updated to reflect that reality.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.

[Takeshi Yoshino]

-shiva.jm

+yhirano

-philipj +foolip

Update on this effort.

WHATWG Fetch spec has been updated to have its own validness definition for header values than referring to RFC 7230. The new definition is:

- Has no leading or trailing HTTP whitespace bytes.

- Contains no 0x00, 0x0A or 0x0D bytes.

I.e., from WHATWG spec perspective, "0x01 -- 0x08, 0x0B, 0x0C, 0x0E -- 0x1F, and 0x7F" in Hiroshige's I-to-DR are no longer disallowed.

We already have IsValidHTTPHeaderValue() call for XHR and fetch() which bans CR, LF and 0x00.

So, input with leading/trailing HTTP whitespaces are the only invalid inputs that are currently accepted by Blink incorrectly.

Once we introduce the normalization to append and set as specified, such invalid inputs will get normalized into valid input. So, the only thing we now need to do is addition of normalization. Then, no change on the logic in Chromium that is performing input validation step is required.

Since //net has been already stripping spaces around a header value (possibly already concatenated with commas inside Blink), the only effect on the resulting bytes on the wire is removal of \t and SP around commas in header values with multiple elements e.g. "Some-Header: abc ,  def" becomes "Some-Header: abc, def". Regarding its impact, according to the UMA Blink.XHR.setRequestHeader.HeaderValueCategoryInRFC7230 introduced by Hiroshige, the ratio of kHeaderValueAffectedByNormalization is ~0.000004%. Even when summed with kHeaderValueInvalid, it's ~0.000005%.

So, I'd like to make the normalization addition happen. It's being worked on by Raphael at http://crbug.com/681769 now.

Thanks

 

[Takeshi Yoshino]

+Raphael

Given the impact is much much smaller and is aligned with //net, we'd like to convert this to be just a PSA and proceed to land the change.

If you see any issue, please let us know.

Thanks

 

[Philip Jägenstedt]

Thanks for the update, Takeshi! Just adding normalization sounds safer than the original plan, and the use usage metrics you have support that.