Skip to content
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

Falcon should not be changing headers to lower case #578

Closed
dukedougal opened this issue Jul 29, 2015 · 27 comments
Closed

Falcon should not be changing headers to lower case #578

dukedougal opened this issue Jul 29, 2015 · 27 comments
Labels
enhancement needs contributor Comment on this issue if you'd like to volunteer to work on this. Thanks! wontfix

Comments

@dukedougal
Copy link

Falcon seems to be intentionally changing headers to lowercase as follows:

    # NOTE(kgriffs): normalize name by lowercasing it
    self._headers[name.lower()] = value

So if I set X-Auth-Token at the server, the client gets x-auth-token.

I'm aware that HTTP headers are case insensitive but even so I think it's more for the application to deal with that - the server shouldn't be changing them IMO.

@kgriffs kgriffs added this to the 0.5 milestone Aug 3, 2015
@kgriffs
Copy link
Member

kgriffs commented Aug 3, 2015

A while ago I created an issue (#420) to look into this but haven't had the opportunity to work on it yet. I think we can preserve case sensitivity in the response headers without incurring a significant performance penalty when setting headers.

@dukedougal dukedougal changed the title Falcon changing headers to lowercase Falcon should not be changing headers to lower case Aug 12, 2015
@dukedougal
Copy link
Author

Changed the title to "Falcon should not be changing headers to lower case" just to be clear.

@BenjamenMeyer
Copy link
Contributor

@kgriffs Python Requests uses a CaseInsensitiveDict for this (https://github.com/kennethreitz/requests/blob/master/requests/structures.py). Perhaps falcon can do something similar?

@kgriffs
Copy link
Member

kgriffs commented Oct 6, 2015

@BenjamenMeyer Unfortunately, I think we'll have to be more clever than that, since CaseInsensitiveDict is significantly slower than a regular dict.

@philiptzou
Copy link
Contributor

@kgriffs I think I can use Cython to build a faster version of CaseInsensitiveDict. Would you like to accept that as a pull request? Of course if user installs falcon without Cython it will fallback to the same speed of the original CaseInsensitiveDict.

@swistakm
Copy link
Contributor

swistakm commented Oct 6, 2015

Is this really a problem? Could someone explain where lowercased header names make something more difficult?

@bragatto
Copy link

bragatto commented Oct 7, 2015

I just noticed Falcon changes headers into lowercase and a quick search got me here.

Although I agree that the HTTP protocol doesn't require headers to be case senstive and that is a reasonable argument to end dicussion (since it improves performance), I'm also a huge fan of the Robustness principle: "Be conservative in what you do, be liberal in what you accept from others" -- and turning the headers lowercase, breaks the non-spoken rule of writing each word with a capital letter, so in that sense, we are not being conservative when we "do".

I understand there's an optimization gain when headers are turned into lowercase for comparsion, but I think the suggestion on #420 : "Similar to above, but keep (OriginalCaseName, case_folded_name, value) so that when searching the list we only have to do a case fold on the incoming name, vs. also having to do it on each name already in the list." might be worth the performance loss (i.e. the developer can send case-senstive headers to be conservative, but have it treated in a "liberal" way when comparing only lowercase strings).

@dukedougal
Copy link
Author

Is this really a problem? Could someone explain where lowercased header names make something more difficult?

Hmmmm.... I'm not sure that it matters whether or not it makes things more difficult (even though it does) - it's just not something it should do.

For example I spent a fair amount of time trying to debug an authentication system that uses X-AuthToken headers and it was just plain broken for no obvious reason - the code seemed all good. Eventually I watched the wire to see what was going on and to my surprise what was being sent by the server was not what I instructed should be sent. I sent a case sensitive header from the server and expected one on the client but instead got one that had been lowercased along the way. I tracked it down to the Falcon code.

Don't quote me on this but I seem to recall at that point looking up the appropriate standards to see what is meant to be happening and as I recall I found that there should be no modification of the headers and it should be reasonable to treat them as case sensitive. I'd have to go back and do that research again to confirm it but that's my vague recollection.

Even if I'm wrong and the standards say that headers should be case insensitive, I'd still firmly disagree that an HTTP server should modify them.

@dukedougal
Copy link
Author

"Be conservative in what you do, be liberal in what you accept from others" - I thought this approach is now thought to be a bad way to design software. Better to design systems that are extremely clear on their expectations in and out.

@dukedougal
Copy link
Author

After digging around the docs again it appears that HTTP headers are case insensitive. Even so it doesn't seem right that the data that I ask Falcon to send is not what is sent. My take would be that my application should treat the headers as case insensitive but the server should send exactly what it is asked to.

@philiptzou
Copy link
Contributor

Assume that we have a broken client software which only accepts Hyphen-Camel-Case format. We know it's bad and unstandard but sadly the client software is a commercial software thus we don't have the source code to correct the error (or we just simply lose the source code in the long past). For other frameworks like Flask since they do not modify your response headers, you can adapt the server software to such "subset" of RFC easily. But in Falcon you don't have any chance to keep your original header strings untouched. Isn't it a bit annoying when you are facing such problems but you can't solve it with the most realism way?

@bragatto
Copy link

bragatto commented Oct 7, 2015

"My take would be that my application should treat the headers as case insensitive but the server should send exactly what it is asked to." -- In case I wasn't clear, I agree with you. Outdated or not, I mentioned the robustness principle because that's precisely what it tells us in this case.

@swistakm
Copy link
Contributor

swistakm commented Oct 7, 2015

Then such change require also decision to be made: "which header name version to choose if multiple case variations were used for assigning specific header?" Last/first/other? This will be arbitrary.

Maybe I'm in minority but I really like existing solution because in my opinion has following benefits:

  • economical in code and resources
  • simple and predictable
  • giving consistent results even if you are not consistent

It also has the rare (and maybe unintended) benefit of proliferating the knowledge of some HTTP spec facts by showing that those header names are indeed case insensitive. @dukedougal already benefited from that because what seemed to be "all good code" turned out to make wrong assumptions about HTTP protocol - issue that now can and should be fixed (@dukedougal from your message I understood that client code also belongs to you, sorry if I'm wrong).

In my opinion only by the fact that specification says that case does not matter then we should not even think of making extra work to preserve case in header name if this makes any inconvenience for us (more resources, more code, more decisions).

@philiptzou I really understand potential scenario of broken clients not following standards but I think if such cases really exist and those clients cannot be fixed then they simply deserve workaround in form of custom middleware.

Anyway if such solution that keeps header case will perform better and will be more convenient then I think it should be implemented. Still, saying that we can move that to cython is not enough to say "perform better".

@kgriffs kgriffs removed this from the 1.2 milestone Feb 17, 2016
@eukaryote
Copy link

I also would prefer case be preserved and was surprised by this.

@kgriffs kgriffs added this to the 1.0 milestone Feb 29, 2016
@kgriffs
Copy link
Member

kgriffs commented Feb 29, 2016

Prioritizing for 1.0

@qwesda
Copy link

qwesda commented Mar 24, 2016

CaseInsensitiveDict is also not an option because (as stated on their github page) this implementation does not preserve original key case. It also just lower cases the keys:

def __setitem__(self, key, value):
    self._data[key.lower()] = self.__create(value)

I think the options would be:

a) store name, value pairs in the Response._headers dictionary
b) have a separate _header_names dictionary

I would prefer b – or can somebody think of a better option.

I can implement the changes and prepare a PR. Are there any other places other than falcon/response.py and tests/test_headers.py that have to be updated?

@kgriffs
Copy link
Member

kgriffs commented May 25, 2016

@qwesda are you still interesting in taking this on? I'd like to learn more about you preference for (b) over (a). Thanks!

@kgriffs
Copy link
Member

kgriffs commented May 25, 2016

P.S. It's interesting to note that HTTP/2 requires lowercased headers. I suspect it is an acknowledgement that allowing for case insensitivity leads to clients not always doing the right thing, with performance perhaps being a secondary concern (albeit a minor one).

@qwesda
Copy link

qwesda commented May 25, 2016

option a would change the existing structure and possible introduce breaking changes wherever _headers is accessed. b would leave Response._headers as is.

With b the only changes would be at the points where headers are set (something like Response._header_names[header_name.lower()] = header_name), and where the headers are output (where instead of just using the _headers keys the corresponding value from the _header_names dict will be used).

@BenjamenMeyer
Copy link
Contributor

I think the import things here are and kind of summing up the conversation:

  • enable app developers to access headers in a case-insensitive manner on the Request object
  • enable app developers to get access to the header key from the Request object as the client provided them
  • enable app developers to specify headers as they like to clients in the Response object

IIRC, Falcon already did that last part...but I could very well be wrong, and that does seem to be the point raised at the start of this issue. (I might not have caught that in my code since I was also developing clients that used requests so that might have hidden anything that way.)

💭
This seems to be solvable by:

  • Uses a Case-Insenstive dict for the Request object's headers
headers = CaseInsensitiveDict()
for k, v in request.headers.items():
    headers[k.lower()] = v
  • Uses a Case-Insensitive dict for look-up with the values as provided:
client_headers_keys = CaseInsensitiveDict()
for k in request.headers.keys():
    client_headers_keys[k.lower()] = k
  • use a standard dict on the Response object. If the app developer specifies several values with different cases Falcon doesn't try to fix it and just sends it on.
  • if the app developers wants/needs to return the header key the same as the client provided it, then they can look it up in the client_header_keys dict on the Request object.

This should allow the optimization on the request side, while providing the explicit capabilities on the return side.

💰

I kept the proposed use of the CaseInsentiveDict (and assuming using the proposed Cython enhancement for it) on the Request side mostly so developers don't have to care which spelling they use to try to look something up.

@qwesda this is essentially your option b, no?

@kgriffs kgriffs modified the milestones: Version 1.2, Version 1.1 Jul 9, 2016
@kgriffs
Copy link
Member

kgriffs commented Aug 14, 2018

Given that HTTP/2 lowercases all headers anyway, and the protocol is becoming prevalent, is it worth the effort of addressing this after all?

@BenjamenMeyer
Copy link
Contributor

@kgriffs just a thought after catching up on this, but perhaps a good way to resolve this would be to add a proper to the Falcon app that users can set for what to use to create the dict. By default it set to dict, users can override with CaseInsensitiveDict if they want that, and an LowerCaseOnlyDict can be provided for HTTP/2 support.

This would...

  • maintain the current status,
  • allow users the flexibility requested,
  • and be future compatible

So something like:

import falcon
import requests.structures as rs
...
falcon.API.HEADER_DICT = rs.CaseInsensitiveDict
app = falcon.API()
...

@vytas7
Copy link
Member

vytas7 commented Jul 31, 2020

Worth noting that the ASGI spec also mandates that header names must be lowercased. Which sort of adds even more weight on the Wontfix side.

@BenjamenMeyer
Copy link
Contributor

@vytas7 folks still need to be able to manage bad clients that they have no control over. Guess it'll get done when someone cares enough to do it; but it'd probably be good to at least maintain what the desired implementation should be should someone want to

@vytas7
Copy link
Member

vytas7 commented Jul 31, 2020

@BenjamenMeyer yeah, that is a fair point.
I was just pointing out that it was impossible to emit anything but lower case header names from an async ASGI app. I.e., in the ASGI flavour of Falcon, we would be unable to preserve the case even if we wanted to. Same for HTTP2.

@vytas7
Copy link
Member

vytas7 commented May 9, 2024

OK, I am going to close this issue since we don't have the bandwidth to work on this. Moreover, as pointed out above, both HTTP/2 and ASGI mandate lowercase headers anyway; for WSGI there is my recipe to patch capitalization (although I understand it isn't particularly elegant).

If anyone wants to submit a pull request, it may still be considered provided the solution is not too complex, and doesn't affect the performance.

@vytas7 vytas7 closed this as not planned Won't fix, can't repro, duplicate, stale May 9, 2024
@vytas7 vytas7 added the needs contributor Comment on this issue if you'd like to volunteer to work on this. Thanks! label May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs contributor Comment on this issue if you'd like to volunteer to work on this. Thanks! wontfix
Projects
None yet
Development

No branches or pull requests

9 participants