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

Optional disable setting headers keys in lowercase #304

Closed
andresgutgon opened this issue May 15, 2016 · 42 comments · Fixed by #476
Closed

Optional disable setting headers keys in lowercase #304

andresgutgon opened this issue May 15, 2016 · 42 comments · Fixed by #476

Comments

@andresgutgon
Copy link

andresgutgon commented May 15, 2016

Hi, I'm trying to use fetch in Chrome but I think it's not working for me for one reason. I'll explain my use case and you can tell me if I can do something to use fetch.

I'm developing a Chrome extension (a content script) that reads from localStorage an auth token. This way I can make request agains a server. I'm not managing that server and I'm not the owner. I can't change anything on the server. The thing is that I'm trying this:

fetch(url, {
  method: "GET",
  headers: {
    "Token": "auth-token-content-goes-hear"
  },
})

But when I see the request on Chrome Dev Tools I see token instead of Token and the server is responding with 401. My first implementation is doing the same with jQuery and the server is returning results.

I understand the standard is put headers in lowercase. But maybe could be posible set a flag to skip it. In situations like mine (user can not change server) is not posible to use fetch.

BTW great work! I appreciate all the work you are doing here 💪

Thanks!

@annevk
Copy link
Member

annevk commented May 16, 2016

@tyoshino @tzik @hiroshige-g @jdm @youennf @mnot, what do you think? Perhaps it's better if Headers does not attempt to change the input casing and we just pass through things literally. And instead we use "ASCII-case insensitive" matching to find any existing header names in Headers (so whatever casing the developer picked as a start wins).

@mnot
Copy link
Member

mnot commented May 16, 2016

@annevk that's probably more natural-looking to most people. OTOH I kind of like that this breaks the server, which is clearly being badly behaved.

@andresgutgon any chance you could tell us (here or privately) what the server is? I have this pile of pitchforks...

@andresgutgon
Copy link
Author

andresgutgon commented May 16, 2016

any chance you could tell us (here or privately) what the server is?

What do you mean? I've not seen the server inside. I know is python/django but I don't know if is served by an Nginx or an Apache

I kind of like that this breaks the server, which is clearly being badly behaved.

Agree, but that is not going to solve my situation. IMO something like fetch should be backwards compatible with outdated servers. This way is easy to use for folks like me that can not change server technology at all.

Thanks for your answer @mnot

@jdm
Copy link
Member

jdm commented May 16, 2016

Case-insensitive matching seems sensible to me, but I don't really have any context for why the original choice was made.

@annevk
Copy link
Member

annevk commented May 16, 2016

The original choice was somewhat arbitrary. I have found that it is usually more convenient for tokens to have a canonical representation. That way you operate on the input and then compare for equivalence. Otherwise you have to operate on both input and what is compared against, and then compare for equivalence. For headers it probably does not matter much either way and given the existence of broken servers it's probably better if we don't canonicalize.

@andresgutgon I suspect @mnot is interested in the server's address where you are encountering this issue.

@tyoshino
Copy link
Member

I was thinking that this lower casing was introduced for some strong reason. There could be already some existing Fetch user codes that are relying on this behavior. Hmm, should we introduce an option to the Headers constructor to suppress lower casing than changing the behavior? Hm

@annevk
Copy link
Member

annevk commented May 16, 2016

Yeah, the alternative is that we create an opt-out. sigh Should probably also ping @wanderview and @jakearchibald.

@wanderview
Copy link
Member

I seem to recall I suggested the lower casing to @annevk during implementation. I think normalizing data is generally nicer than loose checking. If its creating compat issues in the wild, though, I don't mind changing.

@mnot
Copy link
Member

mnot commented May 17, 2016

@tyoshino - Anyone relying on the case of HTTP headers is going to see things broken; code shouldn't depend upon it.

@andresgutgon - I meant the hostname. This could be fixed through evangelisation.

@andresgutgon
Copy link
Author

@mnot is a bit embarrasing, right now there is not an engilenier in charge of that serve :/. For that reason I'm doing a Chrome app to fetch data that is not possible to check with current UI. Also it's a private back office so I prefer not to share it.

Evangelization is great, thanks for the offer. But I think it would be great if the tool can work with outdated software.

@annevk
Copy link
Member

annevk commented May 17, 2016

@mnot I think what @tyoshino meant is that since the JavaScript API will currently always return things lowercase, that could be depended upon already. As that is the API contract. If we are worried about that this would indeed need to be some kind of opt-in "preserve initial casing" switch.

@mnot
Copy link
Member

mnot commented May 17, 2016

Ah.

Would it be reasonable to preserve case on the wire, but return lowercase in the API?

@annevk
Copy link
Member

annevk commented May 17, 2016

Hmm yeah, we could make the accessors lowercase and preserve case in the internal data structure. That is probably the least disruptive way to go.

Unless anyone sees a problem with that I'll try to specify that. Might take a few weeks since I'm otherwise occupied at the moment.

@youennf
Copy link
Collaborator

youennf commented May 17, 2016

FWIW, in WebKit, the internal headers structure makes a difference between common/precompiled header names and less frequent header names.
I don't think WebKit is able to keep casing information for common header names.
The list of common headers is available at https://github.com/WebKit/webkit/blob/master/Source/WebCore/platform/network/HTTPHeaderNames.in
I understand the compat issue, but I also like the idea of making aware applications that they should move to case-insensitive header processing.

@annevk
Copy link
Member

annevk commented May 17, 2016

@youennf I guess you have those atomized and therefore treat that table as the canonical casing?

@youennf
Copy link
Collaborator

youennf commented May 17, 2016

@annevk, yes. That said, I have not checked what the HTTP clients below WebKit do.
I guess that they are free to alter this canonical casing.
Isn't it the case that for HTTP/2, the header names will be lower-cased anyway?

@mnot
Copy link
Member

mnot commented May 17, 2016

@annevk
Copy link
Member

annevk commented May 17, 2016

Ah okay, in that case we definitely want the API to continue to always return lowercased header names. But the internal casing will have to account for HTTP/1 and broken servers and standardizing on some known set of headers might not be that bad, especially for those user agents are expected to include in a request.

@wanderview
Copy link
Member

If we let the wire case be different than the API returned case, then I think we should add an API to get the exact wire value as well. I think script should always be able to observe what they are going to send.

@wanderview
Copy link
Member

So in gecko we actually normalize standard headers to a mixed case form in our network stack. If the header matches one of these in a case-insensitive comparison, then we send the atomize header string:

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpAtomList.h

We probably keep the case of the first new header added as well to use an atom for later matching headers.

Not sure how much of this we can easily change without fear of breaking servers. FYI in case that changes plans here at all.

@annevk
Copy link
Member

annevk commented Jun 27, 2016

@wanderview that matches the behavior from WebKit. I think we should just standardize on that kind of behavior. I haven't checked if the lists match, but I assume that for the most case they do.

@phistuck
Copy link

phistuck commented Jun 27, 2016

Not only do they differ, but the case differs in some case(s?). Example - ETag (WebKit) versus Etag (Firefox). :(

@phistuck
Copy link

And personally, I think X-Requested-With should also be on that list (jQuery adds it).

@annevk
Copy link
Member

annevk commented Jun 27, 2016

Sure, there will be some cases where we need to break a tie. And no, we should not add to the list. Definitely not userland headers were lowercase might be expected (if used as such).

@phistuck
Copy link

But it was X-Requested-With using XMLHttpRequest up until now, not using fetch (and I do not think jQuery 3 uses fetch, so usage will grow even more) and so the case is preserved and servers may expect it. There are countless websites that add it due to their jQuery AJAX usage.

@annevk
Copy link
Member

annevk commented Jun 27, 2016

No, it is whatever case folks use with XMLHttpRequest and that will also be the case for fetch() once this is resolved.

@DDecoene
Copy link

I too have this problem where I'm making a new frontend for an old python flask API. I can understand that lowercase is the fetch standard, but I get a 400 when I use fetch. JQuery $.get is ok. I would like to move away from JQuery because the only thing I'm using is get and post.
So, what is the resolution to this? I believe an optional "option" is the right way. normalize to lower case unless the user forces fetch to send as-is.

@annevk
Copy link
Member

annevk commented Aug 10, 2016

The current plan is that for H1 network traffic (H2 is always lowercase) we canonicalize case for certain known headers per table mentioned earlier and preserve case for unknown headers (well, the first, after that every ASCII case-insensitive match gets the same casing). The API will continue to return everything in lowercase.

Implementations are free to start implementing this behavior, changing the specification for this is not a high priority, though patches are always appreciated.

@octatone
Copy link

octatone commented Sep 26, 2016

I just ran into this lowercasing behavior and it is preventing me from consuming a third-party API that expects capital casing for headers. If I explicitly set my headers to capital case for a fetch request, I would expect that it is sent to the server as such. I have no control over the third-party API, and I am sad that I will have to abandon the fetch polyfill as it is making an assumption that all servers match headers against lowercase or that they normalize casing. Not all APIs are optimal.

@phistuck
Copy link

@octatone - try to contact the provider (or file a bug against their issue tracking system, if there is one), or tweet to them. Who knows, perhaps they are just oblivious to the fact that they should not count on the casing, or that it disturbs developers and it may just be a small change. They may run into problems anyway if they switch to HTTP/2 (which has its benefits)

@andresgutgon
Copy link
Author

try to contact the provider

IMO that's not a solution for old/unmaintained codes. A standard like fetch should be backward compatible

@phistuck
Copy link

Currently, XMLHttpRequest is not going anywhere and fetch is a new API, so there is no need for it to be backward compatible.
Old and unmaintained code usually means that it is not an active third party API, so I am not sure this is relevant here.
If you have internal old and unmaintained code, I do not have a good advise other than to use XMLHttpRequest or use a server to proxy your requests instead.

The whole point of a new API is that it does not need to be "compatible" with anything in that sense.

@octatone
Copy link

Web APIs have been traditionally backwards compatible, however I see that the standards spec for fetch is not. If this polyfill assumes a strict adherence to the spec, then it simply doesn't fit my needs. I can just as easily wrap an XHR in a promise and continue on with my day :)

@youennf
Copy link
Collaborator

youennf commented Sep 26, 2016

XHR usually using the same path as fetch API for network load, I would expect that the changes done to header casing will impact/have already impacted XHR.

@andresgutgon
Copy link
Author

The whole point of a new API is that it does not need to be "compatible"

Maybe, but is a shame because less project will be able to start using fetch. Not cool

@annevk
Copy link
Member

annevk commented Sep 27, 2016

Sure, and that's why we decided to fix it per #304 (comment). That would match the behavior of XMLHttpRequest in most user agents, at least prior to the introduction of fetch(). Adding additional comments to this issue is not going to make it fixed any faster.

@FFX01
Copy link

FFX01 commented Jan 23, 2017

Hi, I am having the same issue as the OP. I need to pass in a csrf token header populated from a cookie. For the most part this works fine. The only issue is that fetch is lower casing the header name. My back end is Django and it requires the header have precise casing. I, in no way, believe that this is bad practice or "outdated". Judging by the posts in this thread, I am very disappointed in fetch's stance on the subject. This is unexpected behavior. Fetch should not automatically lower case all headers. Not only is this behavior unneeded, it causes problems for people who do not want to pull in all of jQuery just for asynchronous requests.

Compare fetch's behavior with axios. Axios does not automatically lower case all headers. I consider axios to be a modern solution just as much as fetch. Can you give one concrete example as to why fetch chose to take this stance? I don't believe calling all servers that require precisely cased headers "outdated" is a good enough reason to introduce completely arbitrary unexpected behavior. I will not change the behavior of my framework's security mechanisms because fetch has decided it knows better than I do.

I hope that fetch will remove this behavior or introduce an option to prevent it. Until then, I will need to find some other work around.

@annevk
Copy link
Member

annevk commented Jan 23, 2017

@FFX01 the very last comment in this thread just before yours indicates it's going to be fixed. Nevertheless, any server framework or otherwise that doesn't treat header names as case-insensitive is broken, since that's what they are per HTTP.

@annevk
Copy link
Member

annevk commented Jan 30, 2017

@youennf so it seems Firefox and Safari have atom-lists of builtin headers, but as far as I can tell Chrome and Edge do not. E.g., they do not change CONTENT-LENGTH in a response to Content-Length (can be observed through XMLHttpRequest's getAllResponseHeaders()).

Another problem here is that wptserve from web-platform-tests does not allow inspection of the request headers to the detail of the casing used. @jgraham is that correct or did I miss something?

whatwg/xhr#108 also has some discussion of related issues.

I guess I can work on a patch that preserves casing and then open a new issue with regards to builtin header names. Still, it would be good to actually be able to test things in some manner.

annevk added a commit that referenced this issue Jan 30, 2017
Instead compare header names using a byte-case-insensitive match and
lowercase them when exposed through the API.

Fixes #203 and fixes #304.
@annevk
Copy link
Member

annevk commented Jan 31, 2017

I managed to look into request headers through the private request._raw_headers API from wptserve. Browsers preserve case for requests. Only Chrome currently lowercases for fetch() (preserves case for XMLHttpRequest).

@andresgutgon
Copy link
Author

👏

annevk added a commit that referenced this issue Jan 31, 2017
Instead compare header names using a byte-case-insensitive match and
lowercase them when exposed through the API.

Fixes #203 and fixes #304.
@annevk
Copy link
Member

annevk commented Jan 31, 2017

There is now a PR available for review. Formal tests are blocked on _raw_headers becoming an official API.

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

Successfully merging a pull request may close this issue.