-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
multiple headers of the "same name" allowed in http #3591
Comments
It should be the last one. Headers are added in object insertion order, the last one wins out. The behavior itself is most likely undocumented but I think we have tests for this (not easy to grep for though.) |
Should it allow multiple by the same at all? Is it too costly to check for this? |
FYI -> http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
|
/cc @nodejs/http |
Potentially there are a few solutions here:
Basically, this solution would be to say that since HTTP headers are not case-sensitive, if
This would signal to users that there is an issue in the given object, which was expected to only contains header-value pairs. |
Option two sounds more appealing since if you can comma separated list syntax to define multiple headers otherwise. Another option to consider is allowing an optional array syntax for that: { cookie: ['foo=bar', 'foo=meh'] } The problem I saw in production was where one library was using |
I didn't mention it, because that is already supported; each element in the array will be a different header, making that response look like the following:
Assuming we are talking about |
Perfect :) |
In that case I presume we can close an issue. |
Why would you make that presumption? |
FYI -> http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
|
Having a similar problem with X-Robots-Tag. I need to support the following
As specified here. |
@cressie176 According to the documentation:
So I think this would work for your case:
|
@Trott, thanks for the quick response. Unfortunately I've tried that - it generates a single header which I don't think conforms to googles spec.
Furthermore if I add an additional directive to apply to all user agents, it gets concatenated with the last one in the list, i.e.
Results in
|
Ah, I see. Thanks for the clarification. (Wish I had more to add than just that.) |
lol! |
Try to use power of RegExp to split string to an array. |
@bricss Sorry - not sure I follow. Are you suggesting I use regex to split the header sent in the response? In this situation my code is running on the server and I'm using node (via express) to send a response header. Node is concatenating header values with the same name into a single string, then transmitting the response. If I was writing the client I could split the string, but in this case the client is googlebot. |
@cressie176 Ouh, sorry, now I see where is a problem. |
@bricss no problem. Intentions were good. |
should maybe result in something like: {
"x-robots-tag": "nofollow,nocache",
"x-robots-tag: googlebot": "noindex",
"x-robots-tag: unavailable_after": "1-Jan-3000 00:00:00 EST"
} Also, how would we set such headers with Edit: HTTP header field names do not allow the ":" character, so Google was never following the specification when they designed these headers. It should've been something like |
Server Push feature of HTTP/2.0 also needs this feature, it will use multiple headers named Link |
Though we are using http/1.1 in express/nodejs, we are using CDN that supports http/2.0, this header will be sent to CDN server. This commit can not really benifit from that feature since nodejs doesn't support multiple headers with the same header name yet, see: - nodejs/node#3591 - expressjs/express#2845
Though we are using http/1.1 in express/nodejs, we are using CDN that supports http/2.0, this header will be sent to CDN server. This commit can not really benifit from that feature since nodejs doesn't support multiple headers with the same header name yet, see: - nodejs/node#3591 - expressjs/express#2845
Though we are using http/1.1 in express/nodejs, we are using CDN that supports http/2.0, this header will be sent to CDN server. This commit can not really benifit from that feature since nodejs doesn't support multiple headers with the same header name yet, see: - nodejs/node#3591 - expressjs/express#2845
Let http server support response multiple same-named header as RFC2612 defined, this feature is also used in http/2.0 "server push" feature. Fix nodejs#3591
@refack please reopen. |
It seems like the solution here is to either allow multiple "same name" headers, or, fail loudly when the user tries to send them. If anyone is interested in solving this, the two things that would likely be helpful are:
|
Failing loudly would be a breaking change, and also isn't really a solution since sending multiple headers with the same name is supported by the HTTP specification and required by certain clients. Failing loudly in response to a valid action could also result in someone posting another issue, and we'd be back to square one. Therefore I suggest the only actual solution is to allow multiple headers with the same name. |
I don't agree with continuing doing something bad simply because fixing it would break things. Can we start with a warning "depreciated" message ? |
Maybe I've misunderstood something, or not explained myself clearly enough. As I understand things, what node does now is bad, because instead of honouring multiple headers with the same name (which is allowed in the HTTP specification and necessary in some circumstance) it squashes them. Changing this to instead of honouring multiple headers with the same name, node reports an error, is still bad and arguably even worse. |
You mean it's allowed because it's not expressly prohibited? Under what circumstances would it be necessary? (Aside from a mistake) |
In current situation 💡, it would make sense to add Headers into Request and Response and gently deprecate ⛔ old methods, or even leave them as is for backward compatibility with few notes 📝 |
@danielb2 The title of this issue is somewhat confusing. It talks about multiple headers of the "same name", but does not differentiate between requests and responses. From my understanding there are different problems with Node's implementation of both, however I am only talking about responses. Section 4.2 of the HTTP specification explicitly allows multiple headers with the same name to be sent, providing their values can be combined into a comma separated list.
Node does not currently support adding multiple headers to a response. The closest you can get is to call
Google's X-Robots-Tag specification depends on being able to send multiple
The above headers:
Combining this into a single header would result in:
The X-Robots-Tag specification is silent of whether this syntax would even work, but at best the Others have cited the following examples where the need to be able to send multiple headers with the same name as part of the HTTP response.
It is not unreasonable to suspect there may be more examples which have not yet been discovered or reported. |
Unfortunately this still wouldn't work where multiple response headers with the same name are necessary since according to the append will combine values, rather than writing multiple response headers, which is necessary in some situations.
|
@cressie176 what about semicolon or newline delimiter '\n' as an option to set multiple values for the same header? 🤔 |
Hi @bricss, I don't see anything in the HTTP specification about semicolons or new line delimiters having special meaning. I assume that they would be treated as part of the field value, rather than indicating multiple headers. |
@ljharb what do you think about this PR: #6865 😅 I know it's old |
Construct header using
|
Relative PR has just landed in master. Closing this. |
If
{ 'cookie: 'foo=bar;', 'Cookie: 'foo=meh;' }
is passed, you have no idea which one will be used.If a request relies on, for example, cookie information, a request may work seemingly at random and can be rather difficult to debug.
The text was updated successfully, but these errors were encountered: