From 89c5d0b841c2ffb39be1c0f5cdccf70073ed5055 Mon Sep 17 00:00:00 2001 From: Tim Perry Date: Tue, 9 Jan 2024 12:13:37 +0100 Subject: [PATCH] Fix bug where duplicate headers were lost in passthrough when using H2 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. --- src/util/request-utils.ts | 15 +++++++++----- test/integration/http2.spec.ts | 37 ++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/src/util/request-utils.ts b/src/util/request-utils.ts index 63513e039..0acda2ae0 100644 --- a/src/util/request-utils.ts +++ b/src/util/request-utils.ts @@ -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: @@ -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); } }; diff --git a/test/integration/http2.spec.ts b/test/integration/http2.spec.ts index 05ae9ed8d..9878884dc 100644 --- a/test/integration/http2.spec.ts +++ b/test/integration/http2.spec.ts @@ -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); + }); }); });