diff --git a/index.js b/index.js index fa88baa4..f5ed4eca 100644 --- a/index.js +++ b/index.js @@ -11,7 +11,6 @@ const { filterPseudoHeaders, copyHeaders, stripHttp1ConnectionHeaders, - getConnectionHeaders, buildURL } = require('./lib/utils') @@ -89,18 +88,18 @@ const fastifyReplyFrom = fp(function from (fastify, opts, next) { } const sourceHttp2 = req.httpVersionMajor === 2 - const headers = sourceHttp2 ? filterPseudoHeaders(req.headers) : { ...req.headers } - - // Strip client-supplied Connection header and all header names listed in it - // before rewriteRequestHeaders runs. This prevents clients from stripping - // headers added by the proxy itself. - const connectionHeaderNames = getConnectionHeaders(headers) - if (headers.connection || connectionHeaderNames.length > 0) { - delete headers.connection - for (let i = 0; i < connectionHeaderNames.length; i++) { - delete headers[connectionHeaderNames[i]] - } - } + + // Strip hop-by-hop headers (RFC 7230 Section 6.1) and every header name + // listed in the client's Connection header, before rewriteRequestHeaders + // runs. This removes the same set regardless of the upstream transport: the + // http2 request path already did it via stripHttp1ConnectionHeaders, but the + // default (undici) path only dropped Connection-listed names, so hop-by-hop + // headers such as `te`, `proxy-connection`, `keep-alive` and `upgrade` were + // forwarded to the upstream over the default transport. Doing it here also + // keeps preventing clients from stripping headers the proxy itself adds. + const headers = stripHttp1ConnectionHeaders( + sourceHttp2 ? filterPseudoHeaders(req.headers) : req.headers + ) headers.host = url.host const qs = getQueryString(url.search, req.url, opts, this.request) diff --git a/test/strip-connection-headers.test.js b/test/strip-connection-headers.test.js index 3eca1d89..b39c6f69 100644 --- a/test/strip-connection-headers.test.js +++ b/test/strip-connection-headers.test.js @@ -436,3 +436,110 @@ t.test('strips headers listed in Connection header before undici request', async t.assert.strictEqual(result.statusCode, 200) }) + +// RFC 7230 Section 6.1 - hop-by-hop headers are consumed by the proxy and must +// not be forwarded to the upstream. The http2 request path already dropped +// these via stripHttp1ConnectionHeaders; these cover the default (undici) and +// core-http transports, where they used to leak. + +t.test('strips hop-by-hop headers (undici)', async (t) => { + t.plan(6) + const instance = Fastify() + instance.register(From) + + t.after(() => instance.close()) + + const target = http.createServer((req, res) => { + t.assert.ok('request proxied') + t.assert.strictEqual(req.headers.te, undefined, 'TE should be stripped') + t.assert.strictEqual(req.headers['keep-alive'], undefined, 'Keep-Alive should be stripped') + t.assert.strictEqual(req.headers['proxy-connection'], undefined, 'Proxy-Connection should be stripped') + t.assert.strictEqual(req.headers['x-keep'], 'keep-me', 'a normal header should be preserved') + res.statusCode = 200 + res.end('ok') + }) + + instance.get('/', (_request, reply) => { + reply.from(`http://localhost:${target.address().port}`) + }) + + t.after(() => target.close()) + + await new Promise((resolve) => instance.listen({ port: 0 }, resolve)) + await new Promise((resolve) => target.listen({ port: 0 }, resolve)) + + const result = await makeRequest(instance.server.address().port, { + TE: 'gzip', + 'Keep-Alive': 'timeout=5', + 'Proxy-Connection': 'keep-alive', + 'X-Keep': 'keep-me' + }) + + t.assert.strictEqual(result.statusCode, 200) +}) + +t.test('strips hop-by-hop headers (http)', async (t) => { + t.plan(6) + const instance = Fastify() + instance.register(From, { undici: false }) + + t.after(() => instance.close()) + + const target = http.createServer((req, res) => { + t.assert.ok('request proxied') + t.assert.strictEqual(req.headers.te, undefined, 'TE should be stripped') + t.assert.strictEqual(req.headers['keep-alive'], undefined, 'Keep-Alive should be stripped') + t.assert.strictEqual(req.headers['proxy-connection'], undefined, 'Proxy-Connection should be stripped') + t.assert.strictEqual(req.headers['x-keep'], 'keep-me', 'a normal header should be preserved') + res.statusCode = 200 + res.end('ok') + }) + + instance.get('/', (_request, reply) => { + reply.from(`http://localhost:${target.address().port}`) + }) + + t.after(() => target.close()) + + await new Promise((resolve) => instance.listen({ port: 0 }, resolve)) + await new Promise((resolve) => target.listen({ port: 0 }, resolve)) + + const result = await makeRequest(instance.server.address().port, { + TE: 'gzip', + 'Keep-Alive': 'timeout=5', + 'Proxy-Connection': 'keep-alive', + 'X-Keep': 'keep-me' + }) + + t.assert.strictEqual(result.statusCode, 200) +}) + +t.test('preserves TE: trailers, which is not hop-by-hop (undici)', async (t) => { + // Node treats `TE: trailers` as legal to forward (see stripHttp1ConnectionHeaders), + // so it must survive while other TE values are dropped. + t.plan(3) + const instance = Fastify() + instance.register(From) + + t.after(() => instance.close()) + + const target = http.createServer((req, res) => { + t.assert.strictEqual(req.headers.te, 'trailers', 'TE: trailers should be preserved') + res.statusCode = 200 + res.end('ok') + }) + + instance.get('/', (_request, reply) => { + reply.from(`http://localhost:${target.address().port}`) + }) + + t.after(() => target.close()) + + await new Promise((resolve) => instance.listen({ port: 0 }, resolve)) + await new Promise((resolve) => target.listen({ port: 0 }, resolve)) + + const result = await makeRequest(instance.server.address().port, { TE: 'trailers' }) + + t.assert.strictEqual(result.statusCode, 200) + t.assert.strictEqual(result.body, 'ok') +})