Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

http: concatenate duplicate headers by default #6821

Closed
wants to merge 2 commits into from

Conversation

rlidwka
Copy link

@rlidwka rlidwka commented Jan 8, 2014

Currently node.js has a list of headers which are allowed to be duplicated, and is dropping any headers that aren't on that list (or start with x-). I'm suggesting to do the opposite: build a list of known singular headers and treat others as multiple.

Relevant discussion is in issue #2750 (note: I believe a bug with not concatenating DAV still exists).

I took a list of singular headers from firefox source code, since firefox seem to do the same thing I'm suggesting here.

More detailed explanation. There are four kinds of headers:

  1. known singular (i.e. Content-Type)
  2. known multiple (i.e. Accept)
  3. unknown regular (i.e. FooBar)
  4. unknown starting with "x-" (i.e. X-FooBar)

Currently node.js treats 2 & 4 as multiple, and 1 & 3 as singular. Suggested change: treat 2 & 3 & 4 as multiple, 1 as singular.

Current behaviour creates a few issues:

   Implementations of application protocols MUST NOT make any
   assumptions about the status of a parameter, nor take automatic
   action regarding a parameter, based solely on the presence or absence
   of "X-" or a similar construct in the parameter's name.

Suggested approach will solve all of this at the cost of concatenating unknown singular headers (but those requests are invalid anyway).

@indutny
Copy link
Member

indutny commented Jan 8, 2014

+1 for this PR, need to hear @tjfontaine or @trevnorris opinion before landing.

@trevnorris
Copy link

The code LGTM, and I'm fine w/ the change. But also suspect there will be unforeseen consequences from module authors that do things with HTTP headers (e.g. connect module)

case 'location':
case 'max-forwards':
// drop duplicates
if (util.isUndefined(dest[field])) dest[field] = value;
Copy link
Member

Choose a reason for hiding this comment

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

May I ask you to move dest[field] to the next line (with proper indent)

@indutny
Copy link
Member

indutny commented Jan 8, 2014

Ok, very minor style nits, otherwise LGTM too.

@rlidwka
Copy link
Author

rlidwka commented Jan 9, 2014

@indutny , style issues fixed

@indutny
Copy link
Member

indutny commented Jan 13, 2014

Thank you, landed in ec57ecc

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

Successfully merging this pull request may close these issues.

None yet

3 participants