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

Should support upper-case headers #56

Open
aaronatbissell opened this issue Jun 12, 2024 · 9 comments
Open

Should support upper-case headers #56

aaronatbissell opened this issue Jun 12, 2024 · 9 comments

Comments

@aaronatbissell
Copy link

aaronatbissell commented Jun 12, 2024

According to RFC 9110, I believe headers are meant to be case-insensitive.

Field names are case-insensitive ...

Running the test suite in this project with a header of Content-Type yields errors. Would be nice to case-insensitively process those headers.

I understand most clients will lower-case headers on the way out, but we have been using this in machine-to-machine APIs that don't always get lower-cased on the way out.

@dougwilson
Copy link
Contributor

It is already case insensitive as Node.js HTTP lower cases them already. Try out the example at https://github.com/jshttp/type-is?tab=readme-ov-file#api and send upper case headers to see.

@aaronatbissell
Copy link
Author

That's true if you are using Node.js HTTP. We are using this in a project that does not use Node.js HTTP and has some clients that happen to send headers with upper-case characters.

@dougwilson
Copy link
Contributor

dougwilson commented Jun 12, 2024

This project only works with Node.js HTTP. If you are using something else, you should be using something designed to work with whatever you are using instead.

@dougwilson
Copy link
Contributor

From the docs:

The request argument is expected to be a Node.js HTTP request.

@aaronatbissell
Copy link
Author

I'm sure you know this already, but express uses this package. It's unfortunate that now express doesn't work because of a case sensitivity issue with a header.

@dougwilson
Copy link
Contributor

dougwilson commented Jun 12, 2024

Express uses Node.js HTTP. The req.headers object that enters into this module only contains lower case header names, even when the client sends them in capitals. This is because Node.js HTTP (and thus Express.js) will always lower case the headers when they enter the server.

Node.js does this because if you have a plan object like req.headers and want the header Content-Type, it is not ezpected to try and look up every possible upper and lower variant in the object to find it, thus why Node.js will always lower case the header names when it parses the request from the client.

Perhaps you can provide a full Express.js app and show us a cURL request or similar with upper case headers that does not work.

https://nodejs.org/api/http.html#messageheaders

The request/response headers object.

Key-value pairs of header names and values. Header names are lower-cased.

@aaronatbissell
Copy link
Author

Here's an issue from another project where they were seeing the exact same problem. I'm confident it's because their Content-Type header is capitalized. I'll be the first to admit that this isn't technically going over HTTP because it's invoking a lambda function directly instead of using something like API Gateway, but it's still possible to invoke express with a request that technically satisfies the shape of the request object, but has upper-case headers.

@dougwilson
Copy link
Contributor

it's still possible to invoke express with a request that technically satisfies the shape of the request object, but has upper-case headers.

Then it wouldn't be the correct shape, as req.headers must be in lower case keys. I linked to the docs above that specify it. I suggest of this is the case, you can close this issue and let them know the bug is with invoking Express.js manually outside of using Node.js HTTP and not actually adhering to their API contract of req.headers needs to be lower case keys.

The reason Node.js sets all the keys to lower case is because in Javascript a key lookup is case sensitive and in order to accommodate case insensitive this module would have to look up every possible combination. And what is req.headers were to then contain both a content-type key and a Content-Type key? This module.would now have to.procide some kind of way to reconcile that. All of that, thoigh, is taken care of in the Node.js HTTP layer ans this module uses the API contract it provides in order to not have to duplicate all that behavior (and slow down every request by now needing to double-check it all if it cannot trust it). The bug is clearly in that lamfa thing bypassing the Node.js HTTP processing and not adhering to it's API contract.

@aaronatbissell
Copy link
Author

I've requested an enhancement to the other library to mimic the behavior of Node.js HTTP

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

No branches or pull requests

2 participants