[BUG] Broken Response set-cookie headers when cors is enabled

[ballercat]

👋

First of, thanks for creating this awesome library. I've been hacking on top of it to build my own testing utility for playing back requests and have been very happy with it for the most part!

I recently ran into an issue when a request is forwarded via the server. Turns out that when cors is enabled Response headers that are meant to be sent as arrays don't work.

I created detailed reproduction here https://github.com/ballercat/mockttp-multi-header-issue

The headers are passed in as a flat array into writeHead and multiple values of the same header are discarded due to this block of code inside node _http_server.js lib:

https://github.com/nodejs/node/blob/04f9385311d7bc7df0beabf10541e599c2592c6f/lib/_http_server.js#L368-L380

  if (this[kOutHeaders]) {
    // Slow-case: when progressive API and header fields are passed.
    let k;
    if (ArrayIsArray(obj)) {
      if (obj.length % 2 !== 0) {
        throw new ERR_INVALID_ARG_VALUE('headers', obj);
      }


      for (let n = 0; n < obj.length; n += 2) {
        k = obj[n + 0];
        if (k) this.setHeader(k, obj[n + 1]);
      }
    } else if (obj) {

Because Access-Control-Allow-Origin header is already set on the response (thanks to cors mode being enabled) every header in the list is set via this.setHeader. So the last set-cookie header will "win" while the others are overwritten. Whereas when there are no headers already present on the response the headers array is used properly and there is no problem. So when cors is disabled we hit this branch

  } else {
    // Only writeHead() called
    headers = obj;
  }

I tracked the problem down to this line in the mockttp library:

mockttp/src/util/header-utils.ts

Line 130 in 30bfb86

value.forEach((v) => rawHeaders.push([key, v]));

Where each header value that is an array is "unwound". I have attempted simply removing this logic and everything is working fine, so I wonder why it's necessary. Ideally we could just leave array values as they are, then this isn't a terribly difficult patch 😓 (and I would be happy to make one if that's the case)

I'm hoping you have some advice. Because I'd like to keep cors: true enabled but then I lose the ability to properly pass through requests where the responses attempt to set multiple cookie values.

Thanks!

[pimterry]

Interesting report, thanks @ballercat.

That header-utils logic is because RawHeaders is explicitly just Array<[string, string]>, where the 2nd element of the tuple should never be an array. Processing elsewhere generally assumes this is true, and it's definitely the preferable format because it allows fully preserving raw header order (e.g. [[a, 1], [b, 1], [a, 2]] loses the detail of its ordering if header a is ever stored as a [string, string[]] format).

I think unwinding that will be complicated, and it really would be much better to try to continue to use that flat format so far as possible, since fundamentally that is the raw format of HTTP on the wire (a list of pairs).

Really, I think this is a node bug. If you call setHeaders and then call writeHeaders, and it tries to merge them, it shouldn't unexpectedly lose values along the way. I'm going to open a PR and get this fixed there.

Beyond that, there are other mitigations we could try to make here (hooking writeHead where we already sort-of do so inside request-utils, or changing how the CORS headers themselves are added). I'm open to PRs to patch that if you'd like, if that's possible as a temporary fix that doesn't break or significantly complicate things, but really it would be better to fix it at the root instead. Node shouldn't just lose header values!

[pimterry]

Filed a Node bug here: nodejs/node#50387

[ballercat]

Cool. Thanks for putting together that fix for Node. I think I'll disable the cors option for now and handle OPTIONS if I need to, since it'll be a while before Node itself is handling these headers as it should. Might be a good idea to call this edge-case out in the docs for others though since it could waste some time chasing the problem down. Up to you though.

Thanks again!

[pimterry]

Good news: my proper fix for this has now been merged into Node, so this will work out of the box once that's released.

Bad news: it's semver-major (potentially breaking change for some edge cases) which means I think it'll be included in the first release of v22, which isn't out until April.

I think I'm going to close this here now regardless. This only affects cases where you use cors: true + beforeResponse + repeated values for the same header, and the short-term workaround is to disable the automatic CORS setup and add CORS headers yourself (which should be easy enough, since you have a beforeResponse callback where you can do that already. I think there might also be a workaround where you deduplicate the headers yourself too, if you prefer.

Regardless, in future with node updated this should work correctly for all cases, and then you can re-enable the cors option if you like (or sticking with the workaround indefinitely is fine too, there's no serious downsides).

[ballercat]

Thanks Tim!

I forgot to follow-up after my last comment. Yeah for anyone potentially running into this in the future I went the route of disabling cors settings and just handling it myself if/when I needed to. Everything works fine.