Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Web compat implications of making getAllResponseHeaders lowercase #146

Closed
RByers opened this Issue Jul 28, 2017 · 45 comments

Comments

Projects
None yet

RByers commented Jul 28, 2017

Issue #109 changed getAllResponseHeaders to lowercase the values, and we shipped that in Chrome 60. We've had two reports of serious application breakage (deployed servers that can't just be updated in one place) as a result of this (though we didn't get any such feedback during beta, so it's too late for us to avoid impacting most Chrome users).

@annevk how valuable is it to do this lower casing? Should we consider reverting to the old behavior given the known web compat impact if we could get consensus from everyone?

@youennf / @cdumez, I see you guys changed WebKit as well. If your users report similar issues how likely are you to revert the change?

I'm still OK doing whatever has browser consensus. But if other browsers are just going to revert once they feel the compat pain then I'd rather make that decision now for the sake of the impacted Chrome users/developers.

Owner

annevk commented Jul 31, 2017 edited

@RByers there's a couple of reasons why it would be nice to keep it lowercase:

  1. Less primitives in browsers. We can just use one kind of casing throughout for response headers and don't have to preserve casing. (fetch() always has lowercase response headers. It also exposes request headers as lowercase, but for those we do have to preserve casing under the hood unfortunately so on the H/1 wire they have casing.)
  2. Related to 1, we also don't have to decide if we get duplicate headers with the same name which one ends up deciding the casing for both.
  3. H/2 only has lowercase so any application that depends on the casing would already not be robust to server switches.
  4. Intermediaries are allowed to change casing so the moment you inject some third-party caching in your application it might end up breaking if you're not robust against this.

Of course, if too much breaks it's not tenable and we'll have to add more H/1+XMLHttpRequest special cases.

cpuy commented Jul 31, 2017

Hi,

Thanks for taking our issue in consideration.
As mentioned by @RByers, this is a major application breakage for us. We are an on-premises and open source Low-code development platform. Meaning that users install our platform on site and build web applications with.
This change makes not only our platform unusable but also the application created by our users.

Let's take a small example: we build our pagination components around the 'Content-Range' response header. Obviously header['Content-Range'] is now undefined so our pagination is not displayed anymore.

This is the same for all response headers we use including a CSRF token mandatory for all our APIs calls.

When reading the specification I can't see any place saying that getAllResponseHeaders() must returns header names in lower case, furthermore since HTTP2 servers must return header in lower case getAllResponseHeaders() will return header in lower case for this specific protocol.

We know that we need to change some code to support HTTP2 and this is totally acceptable for us, nevertheless we would expect xhr API to stay stable for HTTP1 protocol so legacy applications don't break.

mnot commented Jul 31, 2017

You use Content-Range for pagination?

Owner

annevk commented Jul 31, 2017

@cpuy the specification currently does return them lowercased. It seems like you didn't follow the links, since https://fetch.spec.whatwg.org/#concept-header-list-sort-and-combine clearly says byte-lowercased.

cpuy commented Jul 31, 2017

@annevk you're right, my bad

@mnot yes we use Content-Range: 1-20/123 for a kind of 'offset-limit/numberResults'

mnot commented Jul 31, 2017

The only defined range-unit is bytes -- seems like your application is pretty far off the reservation already...

cpuy commented Jul 31, 2017

@mnot getting csrf token from response header is not the "right" way to do as well.
Anyway I'm just here to expose how my legacy app has been impacted by xhr API break and how it put my company in danger.

Up to you to decide whether or not you'll keep this change

Owner

foolip commented Jul 31, 2017

Some more reports of breakage:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/_oxlCPNsrck/6i5-IQiSAgAJ
https://twitter.com/thegecko/status/890346862875742210 (linked from above)

@thegecko @oniric85 FYI

It's easy to agree with #146 (comment) and that it would be nice to do lowercasing here, but I wonder how many more reports will be trickling in.

@youennf @cdumez, your input would be most valued, whatever we do here we should make sure we end up interoperable, and not flip-flopping our way to some state that makes everyone sad.

Apparently Firefox had several reports of breakage, enough for us to back out the change. See https://bugzilla.mozilla.org/show_bug.cgi?id=1370485 and https://bugzilla.mozilla.org/showdependencytree.cgi?id=1370485&maxdepth=1&hide_resolved=0

I am sorry that this breakage did not get surfaced in a spec issue; I was not cced on that Firefox bug and wasn't aware of it. :(

cdumez commented Jul 31, 2017 edited

@youennf is the right contact for this. I don't think there would be any issues with reverting the change in WebKit if it causes breakage. However, the behavior change is already in Safari 11 which already went through a lot of beta releases (I am not aware of any critical bug reports that related to this). Wether we can revert the change in Safari 11 at this point is unclear.

As reporter in the Google Group (thanks for bringing it up here @foolip), I would like to ask a little attention here too.

In our application, we use a literal comparison based on the header field names in the response. After this change in Chrome 60, we're now converting all header field name to lowercase, because other browsers still leave it untouched, in order to make it work. So, after a long fight against Internet Explorer, we're back in a situation where various browsers respond differently to the same functions... I do not think we want to go there!

I agree it's nice to do lowercasing, but as @cpuy notes, as a developer, you would expect the XHR-function to be stable, to prevent applications from breaking down all of a sudden...

thegecko commented Aug 1, 2017

In our case, we had a legacy codebase which relied on checking headers in a case-sensitive way.

We have since patched the site and although the fix was small, it was tricky to get to the bottom of.

youennf commented Aug 1, 2017

I agree it's nice to do lowercasing, but as @cpuy notes, as a developer, you would expect the XHR-function to be stable, to prevent applications from breaking down all of a sudden...

There is no guarantee that browsers will keep header casing consistent over time.
WebKit (and probably other browsers as well) are not always propagating network response header casing to XHR, and this propagation is probably not consistent between browsers.
Header casing may break at any browser/network stack/intermediary level.

Lowercasing makes sense to me.
I am not sure how we can best handle the transition though.

mcmanus commented Aug 1, 2017

Lowercasing makes sense to me. I am not sure how we can best handle the transition though.

you need to do a case insensitive comparison - header names have always been defined case insensitively (in both versions of HTTP) - so doing a case sensitive comparison is an application bug that should be fixed. That will give you consistent behavior.

So.. for example: In my PHP-code i use the following code (serverside):

header('Foo: Bar')

which will make my response look like this:

image

and after making a XHR-request, the output of the xhr.getAllResponseHeaders() looks like this:

foo: Bar..

you're saying that when I'm matching on the header field name 'Foo' (as I provided myself in my code) it's a bug in the application when i'm doing a case sensitive comparison and not able to find it?

I think it's a bug in the getAllResponseHeaders-function by returning modified values!

bzbarsky commented Aug 1, 2017

which will make my response look like this:

Maybe it will, or maybe it won't. Since HTTP header names are case-insensitive, a proxy would be totally within its rights to change the casing. So while the response looks like that when it leaves the server, it may not look like that when it reaches the client. Neither the server nor the client have control over that.

it's a bug in the application when i'm doing a case sensitive comparison

Yes. It happens to work in some cases, but is not guaranteed to work, given the semantics of HTTP.

None of which detracts from the point that changing XHR may still be problematic, or that breaking code that was already broken in some cases in a larger set of cases might be a problem. But we should all be clear that code that was not doing case-insensitive header comparisons was already broken in various cases.

phistuck commented Aug 1, 2017 edited

you're saying that when I'm matching on the header field name 'Foo' (as I provided myself in my code) it's a bug in the application when i'm doing a case sensitive comparison and not able to find it?

Yes. If I understand correctly, if you move to HTTP/2, your application would be broken in all of the browsers.
Unfortunately, you relied on an unreliable feature.

mcmanus commented Aug 1, 2017

this is not an http/2 issue. The header name is case insensitive in both versions of http. The original query I responded to asked how to handle the transition, and that answer is easy because the semantics of http are the same independent of http version or xhr implementation - the header name is not case sensitive, so just use a case insensitive comparison to get reliable operation. If your app isn't doing that it should be fixed like any other discovered latent bug (we've all got them.).

I don't have a strong opinion on whether there are sufficient legacy problems here to warrant a particular xhr implementation.

Unfortunately, you relied on an unreliable feature.

I think this is the best conclusion that can be drawn from this. The fact that now, all of a sudden, things happen that did not happen before is not because this adjustment is wrong, but it was never implemented correctly. So I guess lowercasing is, after all, the only right thing to do.

We have now modified our code and everything works properly. Hopefully, @cpuy will also be able to fix everything.

Owner

foolip commented Aug 2, 2017

However, the behavior change is already in Safari 11 which already went through a lot of beta releases

@youennf, looks like that'll be in macOS High Sierra, which isn't yet released, but pretty far along the process, right?

Since it's already in Chrome 60, much of the damage has already been done. A revert for a point release of 60 might be possible, but pointless if it's going to be released in Safari 11. @youennf, I take it you'd like to go ahead rather than pulling the brakes?

cpuy commented Aug 2, 2017 edited

@claskfosmic indeed we've provided a fix right away (by the way it brings us one step forward for HTTP2 protocol support). Nevertheless, as mentioned in comment above, since we are an on-premises software, the fix will be available in next product version and we need to provide patches for production environments managed by our users.

Currently we're thinking about monkey patching xhr.getAllResponseHeaders to keep its behaviour consistent for previous versions of our products 😞

So yes I'm still hoping that this change will be reverted and keep my eyes close to this discussion.
Thank you all for those constructive comments.

cdumez commented Aug 2, 2017

I think it would take some significant breakage (I.e. top site) for us to roll out from WebKit in the short term.

@BrianHicks BrianHicks referenced this issue in elm-lang/http Aug 2, 2017

Open

Headers now lowercase in Chrome 60 #31

youennf commented Aug 3, 2017

@youennf, looks like that'll be in macOS High Sierra, which isn't yet released, but pretty far along the process, right?

Right, this change is in Safari Sierra/High Sierra latest betas.

Since it's already in Chrome 60, much of the damage has already been done. A revert for a point release of 60 might be possible, but pointless if it's going to be released in Safari 11. @youennf, I take it you'd like to go ahead rather than pulling the brakes?

Yes, it seems that websites already started to fix that issue, probably thanks to Chrome 60.

Owner

foolip commented Aug 3, 2017

@rakuco @tyoshino, looks like unless something new comes to light, the best path will be to keep the change. One thing that could still go wrong is if Gecko's compat constraints are different due to UA sniffing. Could either of you check if any of the regressions in https://bugzilla.mozilla.org/showdependencytree.cgi?id=1370485&maxdepth=1&hide_resolved=0 apply to Chrome 60, do those sites still assume a certain casing, and is the code reachable for Chromium? Anything that would prevent Gecko from eventually relanding the change would be bad news for interop here.

I wish there were a lesson to learn here except that all changes are potentially breaking. Due diligence was done, but perhaps this is a case where no large web properties depend on it, and yet there's a long tail of content that does.

Owner

annevk commented Aug 3, 2017

I think the main lesson is that you don't want to expose case-insensitive identifiers without normalization or randomization. Otherwise you effectively end up making it case-sensitive (as is the case for H/1 requests where we could not normalize to lowercase).

rakuco commented Aug 3, 2017 edited

Could either of you check if any of the regressions in https://bugzilla.mozilla.org/showdependencytree.cgi?id=1370485&maxdepth=1&hide_resolved=0 apply to Chrome 60, do those sites still assume a certain casing, and is the code reachable for Chromium? Anything that would prevent Gecko from eventually relanding the change would be bad news for interop here.

  • ExHentai Easy 2: the bug's in the Firefox extension. The code there's still case-sensitive.
  • bilibili: a new version of their main app_bla.js seems to have been deployed; it worked fine on both Chrome 60 and Firefox 54.
  • Nike's Japanese page: that specific product no longer exists. I tried opening another random product page with Chrome 60 and it worked fine. The recommendations for other products that's shown in that page that no longer exists is broken in Chrome 60 though, but I think it's unrelated to this change (I get several security errors when the page tries to access localStorage).
Owner

foolip commented Aug 3, 2017

Awesome, thanks for investigating. So it's still plausible that Gecko will be able to follow if the changes stick in Chrome and Safari. Unless a Gecko dev shouts no, let's proceed then.

rakuco commented Aug 3, 2017

Nike's Japanese page: that specific product no longer exists. I tried opening another random product page with Chrome 60 and it worked fine. The recommendations for other products that's shown in that page that no longer exists is broken in Chrome 60 though, but I think it's unrelated to this change (I get several security errors when the page tries to access localStorage).

Just for the sake of completeness, I've built the chrome binary from master (so what will be M62 at some point) and did manage to get the list of recommendations on Nike's website. Browsing existing products still works as well.

natechadwick commented Aug 3, 2017 edited

This broke one of our products. It was especially confusing to debug as the http header was in fact "Content-Type" on the response.

var isJsonResp = response.headers["Content-Type"] == "application/json";
var resp = {
     data: isJsonResp ? JSON.parse(response.data) : response.data,

After scanning our product's code base there are 856 references to response.headers in JavaScript files. This is a significant amount of potential breakage, work, and rework because someone decided that they just don't like capital letters anymore...

As @cpuy mentioned, we also have a number of customer's deployed on premise , the update process for on-premise customers is non trivial. We have already started this work - but it seems very unnecessary. Headers have been case insensitive in http since the first rfc, why force them to be lowercase now? Forcing headers to lowercase is actually forcing them to be case sensitive. If the server sends Content-Type, the client should see Content-Type. If the server sends content-type, the client should see content-type.

Please reconsider this breakage of a long standing API/protocol contract.

-n

mnot commented Aug 4, 2017

It is not a protocol contract, never has been; field-names are case-insensitive.

If you upgrade your servers to HTTP/2, or use a CDN that implements it for you, your application will break.

If one of your users configure a HTTP/2 proxy -- for example, the Chrome Data Saver proxy -- your application will break. Try it.

If one of your users installs a virus scanning proxy, extension, etc., your application might break (because they're allowed to do this too).

RByers commented Aug 4, 2017

FWIW I agree with the developing consensus here that we should continue with the change as planned, based on:

  • No user complaints were reported during either Chrome 60 beta or Safari 11 beta process suggesting (in concert with the other compat analysis) that the total user impact is quite low (Minimizing end-user impact)
  • Any broken app has long been broken in some legitimate scenarios (relying on non-standard behavior), this change just makes the breakage more consistent and predictable (Maximizing ecosystem benefit)
  • It's too late for the latest Chrome and Safari releases to avoid the bulk of the harm. Reverting at this point at best reduces the impact for Chrome users ~6 weeks from now but likely not for Safari users (so, eg. iPhone users who have no choice but to use the Safari engine for web browsing would still be broken with no option but to switch device).

I'm sorry for pain this has caused the few impacted applications. No amount of breakage ever seems acceptable when it's your app that's impacted. But please understand that our primary concern here is to make the best possible trade-off for the health of the web overall, and at this point it seems likely that keeping this change will result in less total developer and user pain in the long-run than changing course would now.

If it would help, we could explore adding a temporary option in chrome://flags to restore the old behavior. But that's a chromium implementation discussion, not a spec discussion, so please register any interest in that on the chromium bug.

mihondo commented Aug 7, 2017

V60 is still getting pushed out. I'll bet you will be seeing more issues shortly...

@mnot You are providing an API that has handled the implementation of HTTP by being case sensitive up until this update. Most applications echo'd the HTTP specifications for Headers and followed the casing in the spec. Can you point out 10 major HTTP 1.0/1.1 server's in production use that don't send 'Content-Type' for the Content-Type header? Note the casing. By forcing lower case you are forcing everyone to lowercase every header or have a conditional when evaluating every header. That is as case sensitive as the code example that I posted is.

If you made the headers array TRULY case insensitive e.g. request.headers['Content-type'] request.headers['content-type'] request.headers['Content-Type'] request.headers['Content-tYpe'] all come back with the same value - well then you have a change that is fit for global deployment. Why can't/wasn't that implemented since it is a case sensitivity problem?

If this was an HTTP2 introduction then my main problem is with whoever hates capital letters on the HTTP2 spec team.

@RByers I appreciate your desire to roll this out worldwide but what the heck man? Why the rush?

-n

bzbarsky commented Aug 9, 2017

If you made the headers array TRULY case insensitive e.g. request.headers['Content-type'] request.headers['content-type'] request.headers['Content-Type'] request.headers['Content-tYpe'] all come back with the same value

They do, if request.headers is a Headers object: they come back undefined.

I'm guessing that in your case request.headers is provided by some library that populates it based on getAllResponseHeaders and just stuffs whatever it gets from that API in there instead of defining request.headers such that it does case-insensitive lookups (which it could, if it really wanted to).

mnot commented Aug 9, 2017

@natechadwick FWIW, I didn't provide this API; I'm just the chair of the HTTP Working Group.

@mnot haha Does that mean that you are the capital letter guy? Or that you know who he is? :)
@bzbarsky We are definitely using an older version of a library that calls getAllResponseHeaders.
-n

Adam-Reynolds commented Aug 9, 2017 edited

@bzbarsky In a legacy application, we're using YUI's connection class (from 2.9) to handle ajax requests.. this uses getAllResponseHeaders and actually REPLACES getResponseHeader with the result object from getAllResponseHeaders.. It's crazy.

I had hoped I'd never have to get back into maintenance of this application.. especially given the number of on premise installs with have behind firewalls. Oh well, hurray for hotfixing legacy code!

First you take away Tea Pot Rights, and then you break my pagination. You guys are evil sometimes.

RByers commented Aug 9, 2017

I know it's not much help now, but FWIW if anyone had filed a bug on Chrome describing some of the breakage here during the couple months while Chrome 60 was in dev or beta stage, I personally would have argued for holding off from shipping this change to better understand the web compat trade-off (and, from what I'm hearing here, I suspect we'd have decided to undo the change). We have a substantial fraction of Chrome users running dev and beta builds and have generally found we can rely on the regression bugs filed during beta (or lack thereof) being a strong predictor of the risk.

So if you have applications deployed in a way you can't easily update, then please do at least some of your testing or dogfooding on Chrome dev channel and file bugs when you hit problems (issues of the form "this page at this URL works in Chrome N but not Chrome N+1" do generally get handled quite quickly). Beyond intentional breaking changes like this (which are rare), there are always bugs. Regression issues in chromium do tend to get resolved quickly.

But in this case we learned of these issues too late to change Chrome 60 and Safari 11, so most of the damage is done and we're stuck with the change.

mihondo commented Aug 10, 2017 edited

Subscribing to dev is very difficult behind corporate firewalls that block all but approved download sites ... and dev sites don't get approved.

.. and trying to find that install to downgrade to a specific version has been made into a scavenger hunt or you have to trust third party stashes.

@RByers I totally understand the difficult position you're in. Personally I'm all about moving forward, following spec, and getting ahead of this before HTTP/2.0. It sucks to deal with but truth is accessing headers by a string in and object is a dumb way to do it anyways, we should all have been using the function IMHO.

@natechadwick - you seem to miss the point that even before HTTP/2.0, proxy servers and other middlewares were totally allowed (and even did in some cases) to change the case of the headers.
So while your application might have never encountered a (complaining) user behind such a middleware, it does not mean that it never would have encountered one.

I agree that this is a breaking change and that breaking changes (should and do) have a pretty low bar for being reverted, but it is also your responsibility (or the responsibility of the maintaining company) to test your software as long as it is maintained with every version of any browser (preferably - early channel versions, in order to catch the issues earlier and either fix them or report them).
We are no longer living in a world where you can count on your users to use Internet Explorer 6 for a decade, fortunately and unfortunately. ;)

rakuco commented Sep 8, 2017

Given Chromium has recently promoted M61 to its stable channel and there have been no changes to Blink or the XHR spec, I guess it's safe to say we're not changing the spec and we can close the issue?

Owner

annevk commented Sep 8, 2017

I'll go ahead and do that. If there's a need to keep this open please leave rationale in a new comment. Thanks everyone!

@annevk annevk closed this Sep 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment