"Can't remove headers after they are sent" error when using interceptor with proxy

[David-Hari]

Hi,

I have a mock server (for development purposes) that serves client-side files and proxies API requests to a real server. I am using node-http-proxy for the proxy, and have it set up as a middleware.

Now I need to intercept the data coming back from the server and mangle it a bit before sending it back to the browser.
I have set up express-interceptor to run before the proxy, intercepting all requests of content type 'application/json'.

var HttpProxy = require('http-proxy');
var Interceptor = require('express-interceptor');
// other stuff...

apiProxy = HttpProxy.createProxyServer({
    target: config.proxyBaseUrl,
    secure: false
});

/**
 * Creates an Express server to host static files from a local directory, and proxy or mock API requests.
 * @returns {Express}
 */
function createServer() {
    var app = Express();

    // some stuff...

    app.use(Interceptor(function(request, response) {
        return {
            isInterceptable: function() {
                debug('*** isInterceptable called for %s, %s', request.url, response.getHeader('Content-Type'));
                return response.getHeader('Content-Type') === 'application/json';
            },
            intercept: function(body, send) {
                debug('*** intercept called %s, body length = %s', request.url, body.length);
                send(body);
            }
        };
    }));

    // more stuff...

    app.all(/.*/, proxyRequest);
}

/**
 * Proxies the given request to the proxy URL defined in the config.
 * @param request
 * @param response
 * @param {Function} [next] - The next middleware function to call.
 */
function proxyRequest(request, response, next) {
    debug('*** Proxying request %s', request.url);

    // Disable gzip encoding, so we can more easily process the response body if needed
    request.headers['accept-encoding'] = 'identity';

    apiProxy.web(request, response);
    if (next) {
        response.on('close', function() {
            debug('*** Proxy request closed. url = %s', request.url);
            next();
        });
        response.on('finish', function() {
            debug('*** Proxy request finished. url = %s', request.url);
            next();
        });
    }
}

When I make a request to the API I get a "Can't remove headers after they are sent" error. Below is the debug log:

*** Proxying request /api/blah
express-interceptor write called
*** isInterceptable called for /api/blah, application/json
express-interceptor isIntercepting? true
express-interceptor write called
express-interceptor isIntercepting? true
express-interceptor write called
express-interceptor isIntercepting? true
express-interceptor end called
express-interceptor isIntercepting? true
_http_outgoing.js:367
    throw new Error('Can\'t remove headers after they are sent.');
          ^
Error: Can't remove headers after they are sent.
    at ServerResponse.OutgoingMessage.removeHeader (_http_outgoing.js:367:11)
    at ServerResponse.module.exports.res.end (c:\dev\ecm\dev-server\node_modules\express-interceptor\index.js:70:15)
    at IncomingMessage.onend (_stream_readable.js:505:10)
    at IncomingMessage.g (events.js:199:16)
    at IncomingMessage.emit (events.js:129:20)
    at _stream_readable.js:908:16
    at process._tickCallback (node.js:355:11)

I should point out that the tamper library does work when used this way, so I know it is possible to do.

[David-Hari]

This is what the log looks like when using Tamper:

*** Proxying request /api/blah
*** Tamper called for /api/blah, type = application/json
*** Tamper intercept function called for /api/blah, body length = 1824
**** Proxy request finished. url = /api/blah

[flockonus]

Hey @David-Hari, the way interceptor works is by overriding res.write and res.end, and it's a Node.js thing (perhaps more general to say HTTP 1.x thing?) that it's not possible to write headers after the first chunk of body is sent, in this particular case, the logs point out that something is writing to the socket, and then there is a header invalid change, which is a node error in this case.

Some ideas to try out:

1. Did you try to setup a middleware before all that that just change the header to what you need?
2. If you want to make this change to the header conditional to something on Interceptor middleware, i'd recommend you do so before the return of the function you defined as isInterceptable

[David-Hari]

The only bit of my code that is intentionally modifying the headers, is the line in proxyRequest which sets 'accept-encoding'. But that's just changing the property directly, and that error is only triggered when the private _headers property is set in OutgoingMessage.

Again, the tamper library works perfectly in this case (and indeed I am using it for the moment, although it seems to currently be unmaintained), so it must be something wrong with express-interceptor.

[David-Hari]

I can see that tamper is overwriting "writeHead", whereas interceptor does not appear to. I think that may have something to do with the problem.

[gustavnikolaj]

@David-Hari Your theory sounds plausible.

The http-proxy module is indeed writing headers directly. https://github.com/nodejitsu/node-http-proxy/blob/master/lib/http-proxy/passes/web-outgoing.js#L99

I have a test case locally that reproduces the error, and I will push a pull request as soon as I have had some time to look into it.

[flockonus]

Maybe i'm missing something but the problem as i've perceived is not in interceptor according to the logs presented. Headers setting doesn't cause write, but the first write cause the headers to be sent (and then can't be modified. If interceptor go ahead with buffer all the content before writing, it makes #15 streaming impossible afaik.

I vote this a no-fix, both because it can create complexities for the future, and there are ways as I've suggested above to make it work properly within existing mechanisms. Altho this lib exist in the hacky as can be space it seems that node-http-proxy messing with the write shouldn't be our problem, unless it's a widespread problem.

[gustavnikolaj]

One doesn't rule out the other. We can proxy the res.writeHead method, and disable streaming functionality when it is used, while allowing ourselves to handle this case as well... At least I think so :-) Still haven't had time to look into it. I've been pretty busy at work.

[p1nox]

I haven't had the time to review either, crazy last weeks. I think I can taking a look on next week. Thanks @flockonus and @gustavnikolaj for the quick response 💯.

[gustavnikolaj]

I have some partial work on my checkout at work. I'll try to see what I can come up with, hopefully early next week.

[David-Hari]

Perhaps there is a way I can get the proxy to not send the header at that point. It's being done on response of the proxy request (https://github.com/nodejitsu/node-http-proxy/blob/master/lib/http-proxy/passes/web-incoming.js#L149).

If not, @gustavnikolaj's idea of disabling streaming when {{writeHead}} has been called sounds good.

[gustavnikolaj]

Unfortunately, streaming is not yet a thing. We buffer up every thing until
the end event on the original response

I read some of my comments this morning and they seemed non-sensical until
I jad time to trawl them up, so I'll supply a little more detail.

Everything is buffered, and the Content-Length is set in the end-event
handler, just before we go to flush the data we have buffered.

The call to writeHead also flushes headers, which is why we get the error
when writing our header, event though we havemt written anything yet.

When we introduce streaming we would have to change this last-minute header
writing anyway.

If we are buffering and we see a call to writeHead, we should remember the
content and write them when we are ready to write headers ourselves.

Hope this makes more sense. And I hope that my memory of the code is good
enough that there's not major holes in my theory.

Den fredag den 4. september 2015 skrev David notifications@github.com:

Perhaps there is a way I can get the proxy to not send the header at that
point. It's being done on response of the proxy request (
https://github.com/nodejitsu/node-http-proxy/blob/master/lib/http-proxy/passes/web-incoming.js#L149
).

If not, @gustavnikolaj https://github.com/gustavnikolaj's idea of
disabling streaming when {{writeHead}} has been called sounds good.

—
Reply to this email directly or view it on GitHub
#32 (comment)
.

[David-Hari]

If we are buffering and we see a call to writeHead, we should remember the content and write them when we are ready to write headers ourselves.

Yes, that's what the tamper library appears to be doing: https://github.com/fgnass/tamper/blob/master/lib/tamper.js#L63

[gustavnikolaj]

We were not setting the content-length header but removing it. But the result was the same. Please take a look at my pr above :-)

[p1nox]

A new version was released @David-Hari. If there is a problem with it let us know, and in case you're using streaming check #15 or https://github.com/axiomzen/express-interceptor#similar-to.