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

Transform user set headers to lowercase #613

Merged
merged 2 commits into from
Sep 23, 2018

Conversation

alextes
Copy link
Contributor

@alextes alextes commented Sep 14, 2018

There's one thing that should happen which is to lowercase all the headers passed so no duplicate lowercase keys can exist. Because 1. we have logic that reads and writes headers, but 2. we have no clue what node will do when we pass it duplicate header keys.

The reason this is hard to test is that we use cacheable-request and by the time the request event fires the headers have already been merged, with keys set later in time overwriting ones set earlier in time. So I test by setting some header in uppercase I know with got's current implementation is followed by a key set in lowercase before everything is passed to cacheable-request. That's brittle. If someone has a better idea let me know.

Fixes #612 .

@alextes alextes changed the title Lowercase user set headers Transform user set headers to lowercase Sep 14, 2018
@brandon93s
Copy link
Contributor

brandon93s commented Sep 14, 2018

I advise strongly against this approach. Instead, we should update code in got that directly modifies (adds, removes, edits) headers to handle case sensitivity (e.g. find existing header in a case insensitive manner).

Headers in HTTP/1.1, per RFC 7230, are case insensitive. The problem is, any server that is checking for a specific header (case-sensitive) value, could now fail because we are forcing the user's headers to be all lowercase. I have experienced this in a production application and had resulting outages.

HTTP/2 makes this more complicated: headers are required to be all lowercase. If got is to ever adopt native HTTP/2 support, this would need to be accounted for at that time.

Example:

// Client
await got('/api', {
  headers: {
      'X-My-Custom-Header': 'unicorn'
  }
})


// API
if (request.headers['X-My-Custom-Header']) {
   // this fails now if headers are forced to lowercase
}

This very scenario is one of several issues with legacy applications migrating to HTTP/2 support in the wild.

Copy link
Contributor

@brandon93s brandon93s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sindresorhus
Copy link
Owner

@brandon93s I strongly disagree. We should not add complicated logic just to work around faulty servers. HTTP1 specifies headers as case-insensitive and HTTP2 enforces lowercase headers, so the most future-proof way forward is to lowercase headers. Got has always lowercased headers and it has not been a problem. It was only accidentally left out recently.

@brandon93s
Copy link
Contributor

Fair enough. If this was the previous behavior, it shouldn't be a surprise to any current users.

@szmarczak szmarczak merged commit a07b2be into sindresorhus:master Sep 23, 2018
@szmarczak szmarczak added this to the 9.3.0 milestone Oct 20, 2018
@gajus
Copy link
Contributor

gajus commented Jan 31, 2019

@brandon93s I strongly disagree. We should not add complicated logic just to work around faulty servers. HTTP1 specifies headers as case-insensitive and HTTP2 enforces lowercase headers, so the most future-proof way forward is to lowercase headers. Got has always lowercased headers and it has not been a problem. It was only accidentally left out recently.

This is a bummer.

I have been using got to make HTTP requests in a proxy service.

This proxy service interacts with tens of thousands different websites.

After recent Got upgrade, I got reports of many requests failing.

After some digging, it was traced back to Got lower-casing the HTTP request headers.

I am digging the argument let's keep it simple. However, this literally makes the library unusable for those of us who need to interact with Internet at large, not just select websites that respect the standard, and as our experience shows, there are still websites (some with copyright dates of pre-2000s era) that require headers in a certain way.

I have published a library that abstracts working with headers in case-insensitive way.

https://github.com/gajus/http-header

Happy to raise a PR.

@szmarczak
Copy link
Collaborator

@gajus PR won't be necessary. Got just follows the standard. I agree with @sindresorhus - we shouldn't add some bloat because of the faulty servers. That's not our fault - it's done right here.

@alextes alextes deleted the lowercase-headers branch February 8, 2019 21:03
@issuefiler
Copy link

But getting the body is not all there is to requests. We also do tests with them.

If everyone starts (or more precisely, “is forced”) to request only with lowercase headers, how would you test an HTTP<2 server if it well accepts uppercase headers as well, as per the specification you guys are talking about? How would you test a web server with uppercase, lowercase, mixed, and fuzzed headers?

@szmarczak
Copy link
Collaborator

How would you test a web server with uppercase, lowercase, mixed, and fuzzed headers?

Raw TCP sockets and precomputed payloads and expected outputs.

Got is an HTTP client, not a server testing tool.

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

Successfully merging this pull request may close these issues.

6 participants