-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Web compat implications of making getAllResponseHeaders lowercase #146
Comments
@RByers there's a couple of reasons why it would be nice to keep it lowercase:
Of course, if too much breaks it's not tenable and we'll have to add more H/1+XMLHttpRequest special cases. |
Hi, Thanks for taking our issue in consideration. 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. |
You use |
@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. |
The only defined range-unit is |
@mnot getting csrf token from response header is not the "right" way to do as well. Up to you to decide whether or not you'll keep this change |
Some more reports of breakage: 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. :( |
@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... |
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. |
There is no guarantee that browsers will keep header casing consistent over time. Lowercasing makes sense to me. |
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):
which will make my response look like this: and after making a XHR-request, the output of the xhr.getAllResponseHeaders() looks like this:
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! |
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.
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. |
Yes. If I understand correctly, if you move to HTTP/2, your application would be broken in all of the browsers. |
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. |
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. |
@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? |
@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. |
I think it would take some significant breakage (I.e. top site) for us to roll out from WebKit in the short term. |
Right, this change is in Safari Sierra/High Sierra latest betas.
Yes, it seems that websites already started to fix that issue, probably thanks to Chrome 60. |
@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. |
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). |
|
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. |
Just for the sake of completeness, I've built the |
This broke one of our products. It was especially confusing to debug as the http header was in fact "Content-Type" on the response.
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 |
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). |
FWIW I agree with the developing consensus here that we should continue with the change as planned, based on:
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. |
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 |
They do, if I'm guessing that in your case |
@natechadwick FWIW, I didn't provide this API; I'm just the chair of the HTTP Working Group. |
@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. |
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. |
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. 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). |
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? |
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! |
When using an HTTPCalback, response headers are now stored in lower cases. Headers must be retrieved using their lower case name. This changes the way we use header until now nevertheless this is required due to a recent [xhr specification change](whatwg/xhr#146) Moreover this is a step forward HTTP/2 since response headers must be sent in lower case by HTTP/2 servers Relates to: - [BS-16954](https://bonitasoft.atlassian.net/browse/BS-16954) - whatwg/xhr#146 - https://bugs.chromium.org/p/chromium/issues/detail?id=749086 - https://xhr.spec.whatwg.org/#the-getallresponseheaders%28%29-method
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.
The text was updated successfully, but these errors were encountered: