fix: strip hop-by-hop headers on the default transport, not only http2

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)

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')
+})