OutgoingMessage.setHeaders(Headers) doesn't handle Set-Cookie headers properly.

[danfuzz]

Version

21.4.0

Platform

Darwin chug.lan 22.6.0 Darwin Kernel Version 22.6.0: Tue Nov 7 21:42:27 PST 2023; root:xnu-8796.141.3.702.9~2/RELEASE_ARM64_T8103 arm64

Subsystem

http

What steps will reproduce the bug?

Run the following code, and use the curl commands as seen in the transcript under "what do you see instead."

import http from 'node:http';

const server = http.createServer((req, res) => {
  switch (req.url) {
    case '/setHeaders/Headers': {
      const headers = new Headers();
      headers.append('Set-Cookie', 'a=b');
      headers.append('Set-Cookie', 'c=d');
      headers.append('Florp', 'beep');
      headers.append('Florp', 'boop');
      res.setHeaders(headers);
      res.writeHead(204);
      res.end();
      break;
    }
    case '/setHeaders/Map': {
      const map = new Map();
      map.set('Set-Cookie', ['a=b', 'c=d']);
      map.set('Florp', ['beep', 'boop']);
      res.setHeaders(map);
      res.writeHead(204);
      res.end();
      break;
    }
    case '/setHeader': {
      res.setHeader('Set-Cookie', ['a=b', 'c=d'])
      res.setHeader('Florp', ['beep', 'boop']);
      res.writeHead(204);
      res.end();
      break;
    }
    case '/writeHead': {
      res.writeHead(204, {
        'Set-Cookie': ['a=b', 'c=d'],
        'Florp': ['beep', 'boop']
      });
      res.end();
      break;
    }
    default: {
      res.writeHead(500);
      res.end();
    }
  }
});

server.listen(8080);
console.log('Listening on 8080.');

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

Always reproducible.

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

In all three cases, the reported headers should include both header pairs in a way that the client can interpret them. In the setHeaders/Headers case, I would expect the result to be:

$ curl -D - http://localhost:8080/setHeaders/Headers
HTTP/1.1 204 No Content
florp: beep, boop
set-cookie: a=b
set-cookie: c=d
Date: Mon, 29 Jan 2024 17:57:51 GMT
Connection: keep-alive
Keep-Alive: timeout=5

What do you see instead?

In the /setHeaders/Headers case (first curl command), the Set-Cookie header is sent as a=b, c=d which is incorrect.

$ curl -D - http://localhost:8080/setHeaders/Headers
HTTP/1.1 204 No Content
florp: beep, boop
set-cookie: a=b, c=d
Date: Mon, 29 Jan 2024 17:57:51 GMT
Connection: keep-alive
Keep-Alive: timeout=5

$ curl -D - http://localhost:8080/setHeaders/Map
HTTP/1.1 204 No Content
Set-Cookie: a=b
Set-Cookie: c=d
Florp: beep
Florp: boop
Date: Mon, 29 Jan 2024 17:57:56 GMT
Connection: keep-alive
Keep-Alive: timeout=5

$ curl -D - http://localhost:8080/writeHead
HTTP/1.1 204 No Content
Set-Cookie: a=b
Set-Cookie: c=d
Florp: beep
Florp: boop
Date: Mon, 29 Jan 2024 17:58:46 GMT
Connection: keep-alive
Keep-Alive: timeout=5

Additional information

Set-Cookie headers are typically sent as multiple separate header lines, and specifically they cannot safely be joined with , due to a conflict with the spec for the Expires attribute on Set-Cookie headers (which the spec requires a comma in). Technically, Set-Cookie headers are allowed to be joined with ;, though in practice (AIUI) this would be an unusual implementation choice.

See https://datatracker.ietf.org/doc/html/rfc6265 for the gruesome details.

[marco-ippolito]

Part of the reason is that Headers.append joins them as an array with ,

[danfuzz]

Headers.append() actually special-cases Set-Cookie, but clients of Headers also need to be aware of that special case. Notably, if you use get('set-cookie') and there are multiple Set-Cookie headers, Headers will give you an incorrectly-combined string. However, the spread operator does something reasonable, or you can explicitly use getSetCookie(). Quick transcript:

$ node
Welcome to Node.js v21.4.0.
Type ".help" for more information.
> x = new Headers()
HeadersList {
  cookies: null,
  [Symbol(headers map)]: Map(0) {},
  [Symbol(headers map sorted)]: null
}
> x.append('set-cookie', 'a=b')
undefined
> x.append('set-cookie', 'c=d')
undefined
> x.get('set-cookie')
'a=b, c=d'     <-- OOPS THIS WILL LEAD TO TROUBLE!
> [...x]
[ [ 'set-cookie', 'a=b' ], [ 'set-cookie', 'c=d' ] ]    <-- YAY PRETTY USEFUL!
> x.getSetCookie()
[ 'a=b', 'c=d' ]    <-- YAY ALSO USEFUL!
> 

[danfuzz]

BTW you can find how I deal with the problem here: https://github.com/danfuzz/lactoserv/blob/main/src/net-util/export/HttpHeaders.js Specifically, I have a variant of entries() which (among other things) lets you call OutgoingMessage.setHeader(...entry) on each entry, e.g.:

const entries = headers.entriesForVersion(req.httpVersion);
for (const [name, value] of entries) {
  res.setHeader(name, value);
}