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

Duplicate HTTP headers in writeHead are lost, but only if setHeader is also used #50387

Closed
pimterry opened this issue Oct 25, 2023 · 0 comments · Fixed by #50394
Closed

Duplicate HTTP headers in writeHead are lost, but only if setHeader is also used #50387

pimterry opened this issue Oct 25, 2023 · 0 comments · Fixed by #50394
Labels
http Issues or PRs related to the http subsystem.

Comments

@pimterry
Copy link
Member

pimterry commented Oct 25, 2023

Version

v21.1.0 (and all other modern versions AFAICT)

Platform

Linux 6.2.0-34-generic #34-Ubuntu SMP PREEMPT_DYNAMIC Mon Sep 4 13:06:55 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

http

What steps will reproduce the bug?

const http = require('http');

http.createServer((req, res) => {
    if (req.url.includes('setHeader')) {
        res.setHeader('set-test', 'abc');
    }

    res.writeHead(200, [
        'array-val', '1',
        'array-val', '2'
    ]);
    res.end();
}).listen(8080, () => {
    http.get("http://localhost:8080?setHeader", (res) => {
        console.log('With set-header, returns:', res.rawHeaders);
    });

    http.get("http://localhost:8080", (res) => {
        console.log('Without set-header, returns:', res.rawHeaders);
    });
});

How often does it reproduce? Is there a required condition?

Always reproducible

What is the expected behavior? Why is that the expected behavior?

writeHead accepts an array format, which may contain duplicate headers. Duplicate headers are valid (under some constraints) and in practice this is required in some cases (lots of discussion here: #3591).

When writeHead is called an array with duplicate headers, those should be sent in the response.

What do you see instead?

With set-header, returns: [
  'set-test',
  'abc',
  'array-val',
  '2',
  'Date',
  'Wed, 25 Oct 2023 13:19:45 GMT',
  'Connection',
  'keep-alive',
  'Keep-Alive',
  'timeout=5',
  'Transfer-Encoding',
  'chunked'
]
Without set-header, returns: [
  'array-val',
  '1',
  'array-val',
  '2',
  'Date',
  'Wed, 25 Oct 2023 13:19:45 GMT',
  'Connection',
  'keep-alive',
  'Keep-Alive',
  'timeout=5',
  'Transfer-Encoding',
  'chunked'
]

I.e. in the set-header case, array-val: 1 is lost.

Here this is working correctly when only writeHead is called, but if setHeader has previously been called then the behaviour of writeHead changes unexpectedly - retaining the setHeader value but only the last of the two writeHead values.

This is confusing, since it loses data that's provided explicitly to writeHead, it works correctly when run by itself, and it makes it very difficult to reliably use setHeader and writeHead together (even though the docs say that's explicitly supported).

Additional information

That happens because of the use of setHeader here, which makes the array of values override one another.

One option is to make that appendHeader instead - that would mean if you have already called setHeader, and then you call writeHead, the headers would be merged.

That would work correctly for this case. Unfortunately though it's a breaking change for other cases because the docs say:

When headers have been set with response.setHeader(), they will be merged with any headers passed to response.writeHead(), with the headers passed to response.writeHead() given precedence.

That's not what's happening in this case (note that setHeader sets a completely different header to the headers set in writeHead - the writeHead arguments are effectively conflicting with themselves) but using appendHeader would break this constraint in cases where the header keys were all the same - it would set all 3 headers values (instead of the 2 in writeHead overriding previous setHeader).

I think there are some routes to solve this cleanly - preserving current behaviour but avoiding lost values when raw header arrays are passed. I'll look into that and open a PR soon - if anybody has thoughts or suggestions for the correct approach here those are very welcome 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants