proxyReq can't safely modify headers when used with keep alive (ERR_HTTP_HEADERS_SENT)

[nickwesselman]

The use of the socket event to drive proxyReq and allow header modification appears to be problematic when used with keepAlive and maxSockets on an HTTP agent. When a socket is immediately available to a request, the event fires in the ClientRequest constructor, so the request headers can be modified. But with keepAlive enabled on the agent, a socket may not be immediately available. In that case the request continues on and begins buffering its output, so by the time socket is fired, the request header has been 'sent'.

I have run into this while using chimurai/http-proxy-middleware (chimurai/http-proxy-middleware#472) but I was able to reproduce with a basic proxy example that has an agent limited to 5 sockets, and a k6 test which ramps up beyond that. Likely there needs to be a better extension point for initializing the request options, since node doesn't appear to provide an alternative hook for before the request starts buffering... unless I'm missing something here.

Basic Proxy

var http = require('http'),
    httpProxy = require('http-proxy');

var proxy = httpProxy.createProxyServer({
  target:'http://localhost:9003',
  agent: new http.Agent({
    keepAlive: true,
    maxSockets: 5,
    maxFreeSockets: 5
  })
}).listen(3000);

proxy.on('proxyReq', function(proxyReq, req, res, options) {
    proxyReq.setHeader('TEST', req.headers['k6-vu'] || "(none)");
});

http.createServer(function (req, res) {
  res.writeHead(200, { 'Content-Type': 'text/plain' });
  res.write('request successfully proxied to: ' + req.url + '\n' + JSON.stringify(req.headers, true, 2));
  res.end();
}).listen(9003);

console.log("Proxy listening on http://localhost:3000");

k6 Test

import http from 'k6/http';
import { Counter } from "k6/metrics";

export let options = {
    discardResponseBodies: false,
    scenarios: {
        headRequest: {
            executor: 'ramping-vus',
            exec: 'basicRequest',
            startVUs: 5,
            stages: [
                { duration: '5s', target: 10 }
            ]
        }
    },
    thresholds: {
        "pageError": [{threshold: "count<1", abortOnFail: true}],
    }
};

const pageError = new Counter("pageError");

export function basicRequest() {
    const params = {
        headers: {
            'K6-VU': __VU
        }
    };
    var response = http.request('GET', 'http://localhost:3000/', null, params);
    if (response.error || response.status !== 200) {
        pageError.add(1);
    }
}

Output

_http_outgoing.js:518
    throw new ERR_HTTP_HEADERS_SENT('set');
    ^

Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
    at ClientRequest.setHeader (_http_outgoing.js:518:11)
    at ProxyServer.<anonymous> (C:\dev\nwe-jss-test\jss13.0.1-sc9.3.0\keepalive-middleware-test\basic-proxy.js:14:14)
    at ProxyServer.emit (C:\dev\nwe-jss-test\jss13.0.1-sc9.3.0\keepalive-middleware-test\node_modules\eventemitter3\index.js:184:35)
    at ClientRequest.<anonymous> (C:\dev\nwe-jss-test\jss13.0.1-sc9.3.0\keepalive-middleware-test\node_modules\http-proxy\lib\http-proxy\passes\web-incoming.js:133:16)
    at ClientRequest.emit (events.js:327:22)
    at tickOnSocket (_http_client.js:703:7)
    at onSocketNT (_http_client.js:744:5)
    at processTicksAndRejections (internal/process/task_queues.js:84:21) {
  code: 'ERR_HTTP_HEADERS_SENT'
}

[Simra987]

What's the issue inside this proxy?

[nickwesselman]

What's the issue inside this proxy?

The proxyReq event is ostensibly there to "enable developers to modify the proxyReq before headers are sent" ... that's not practically possible because keepAlive is needed under load, and in cloud providers like Azure, SNAT port limits make it absolutely necessary to reuse sockets. So effectively it seems there is no way of modifying the request dynamically, e.g. dynamically setting header values.

[Simra987]

So, it means everything alright.

[nickwesselman]

So, it means everything alright.

What gives you that idea? At a minimum, there is a feature in this module which breaks under certain usage, which should be documented. However I think the actual fix would be to introduce a new event/callback which allows modifying the proxy request options prior to proxy request construction, so that values from the original request can be used safely.

There may be other recipes/examples in this repo that break when keep-alive connections are used as well.

[Simra987]

Will try to improve it.

[delin55]

There may be other recipes/examples in this repo that break when keep-alive connections are used as well.

Agree with @nickwesselman.

This works

 .on('proxyReq', function(proxyReq, req, res, options) {
     proxyReq.removeHeader('referer')
}

This fails

const proxyAgent = new ProxyAgent('socks://127.0.0.1:9050')

.on('proxyReq', function(proxyReq, req, res, options) {
     proxyReq.removeHeader('referer')
     options.agent = proxyAgent
}

The error: 'ERR_HTTP_HEADERS_SENT'
_http_outgoing.js:586
throw new ERR_HTTP_HEADERS_SENT('remove')
On node v14

Took me a long time to come here. Immediate fix could be to update the docs:

Some (or all?) agents may break changing headers in .on('proxyReq')

A fix:

https.createServer( function(req, res) {
	delete req.headers.referer
	let opts ={
                ......
		agent: proxyAgent,
		}
	proxy.web(req, res, opts)	
}).listen(8000 )

[chasetec]

Does the proxy need to avoid piping to the request until the socket is really connected? And also make sure the proxyReq event is emitted before any piping occurs so there is no chance of the headers and first chunk being sent already?

    proxyReq.on('socket', function(socket) {
      if (socket.pending) {
        // if not connected, wait till connect to pipe
        socket.on ('connect', () => {
          if(server && !proxyReq.getHeader('expect')) {
            server.emit('proxyReq', proxyReq, req, res, options);
          }
          (options.buffer || req).pipe(proxyReq);
        });
      }
      else {
        if(server && !proxyReq.getHeader('expect')) {
          server.emit('proxyReq', proxyReq, req, res, options);
        }
        // socket is connected (reused?), just pipe
        (options.buffer || req).pipe(proxyReq);
      }
    });

Modified from #1495

[bc2009]

any progress for fixing this? observing this may be an issue for HTTP/2 streaming for clouds as here