Skip to content

Commit

Permalink
Fix bug where duplicate headers were lost in passthrough when using H2
Browse files Browse the repository at this point in the history
Previously, if a client connected with HTTP/2, a request was proxied,
and the response contained duplicated headers (e.g. multiple set-cookie
headers) then all but the last header value was lost. This is due to a
bug in Node's HTTP/2 compatability layer, which will be fixed
separately. In the meantime, this patch should let us work around the
issue with minimal impact.

This patch works by collapsing raw headers back to an object. The only
situation where this will reorder or lose header info is for duplicate
headers, which are conveniently broken by this bug already. Other raw
header cases like key-casing don't matter for H2 because it enforces
lowercase keys regardless.
  • Loading branch information
pimterry committed Jan 9, 2024
1 parent d545f9f commit 89c5d0b
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 5 deletions.
15 changes: 10 additions & 5 deletions src/util/request-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,16 @@ export const writeHead = (
statusMessage?: string | undefined,
headers?: Headers | RawHeaders | undefined
) => {
const flatHeaders =
const flatHeaders: http.OutgoingHttpHeaders | string[] =
headers === undefined
? {}
: isHttp2(response) && Array.isArray(headers)
// H2 raw headers support is poor so we map to object here.
// We should revert to flat headers once the below is resolved in LTS:
// https://github.com/nodejs/node/issues/51402
? rawHeadersToObject(headers)
: isHttp2(response)
// Due to a Node.js bug, H2 never expects flat headers
? headers as {}
? headers as Headers // H2 supports object headers just fine
: !Array.isArray(headers)
? objectHeadersToFlat(headers)
// RawHeaders for H1, must be flattened:
Expand All @@ -102,9 +106,10 @@ export const writeHead = (
// different casing can't be represented with setHeader at all (the latter overwrites).

if (statusMessage === undefined) {
response.writeHead(status, flatHeaders);
// Cast is required as Node H2 types don't know about raw headers:
response.writeHead(status, flatHeaders as http.OutgoingHttpHeaders);
} else {
response.writeHead(status, statusMessage, flatHeaders);
response.writeHead(status, statusMessage, flatHeaders as http.OutgoingHttpHeaders);
}
};

Expand Down
37 changes: 37 additions & 0 deletions test/integration/http2.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,43 @@ nodeOnly(() => {

await cleanup(proxiedRequest, proxiedClient, client);
});

it("preserves duplicated-key HTTP/2 response headers", async () => {
const mockedEndpoint = await remoteServer.forGet('/mocked-endpoint')
.thenReply(200, "Remote HTTP2 response!", {
'set-cookie': ['a', 'b']
});
await server.forGet(remoteServer.urlFor('/mocked-endpoint'))
.thenPassThrough();

const client = http2.connect(server.url);

const req = client.request({
':method': 'CONNECT',
':authority': `localhost:${remoteServer.port}`
});

// Initial response, so the proxy has set up our tunnel:
const responseHeaders = await getHttp2Response(req);
expect(responseHeaders[':status']).to.equal(200);

// We can now read/write to req as a raw TCP socket to remoteServer:
const proxiedClient = http2.connect(remoteServer.url, {
// Tunnel this request through the proxy stream
createConnection: () => req
});

const proxiedReq = proxiedClient.request({
':path': '/mocked-endpoint'
});
const response = await getHttp2Response(proxiedReq);

expect(response['set-cookie']).to.deep.equal(
['a', 'b']
);

await cleanup(proxiedReq, proxiedClient, client);
});
});

});
Expand Down

0 comments on commit 89c5d0b

Please sign in to comment.