RoutingProxy.proxyRequest modifies the options parameter

[tjanczuk]

Node: v0.7.6
http-proxy: v0.8.0 (via NPM)
MacOS

Problem:

RoutingProxy.prototype.proxyRequest modifies the 'options' parameter by adding new properties to the object. This is unexpected and leads to unintended consequences as described below. I was expecting the API to not modify the options parameter.

Scenario:

I have a single node process that runs two reverse proxies on two different TCP ports: an HTTPS->HTTP proxy and HTTP->HTTP proxy. The two reverse proxies use two instances of RoutingProxy, but the data structure that defines routing rules is shared between the two proxies. Specifically, the object instance I pass as 'options' parameter to RoutingProxy.proxyRequest has a global lifetime. What I notice is that whenever the first routing request is HTTPS, the modifications RoutingProxy.proxyRequest makes in the 'options' parameter cause subsequent HTTP routing requests to fail. It appears that the RoutingProxy is picking HTTPS module instead of HTTP to make the outbound request to the backend.

[Marak]

Could you create a failing test that replicates the issue?

[tjanczuk]

It would be something along these lines:

var options = { 
    https: { 
        cert: '--your cert goes here--', 
        key: '--your key goes here--'
    } 
};

require('http-proxy').createServer(options, onRouteRequest).listen(443);

var route = {
    host: 'www.google.com',
    port: 80
};

function onRouteRequest(req, res, proxy) {
    proxy.proxyRequest(req, res, route);
    require('assert').equal(2 == Object.getOwnPropertyNames(route).length);
}

[Marak]

I'm not sure what you are trying to say with that code snippet. It not runnable and contains method names not in http-proxy, i.e. proxy.routeRequest.

You mentioned the issue only occurs when you have two proxies? I see only one in that demo.

I want to help you, but without an adequate test to replicate the issue, there is little I can do.

[tjanczuk]

The method name should have been proxyRequest not routeRequest, fixed it inline.

The snippet simplifies my scenario to the barebones without introducing the complexity of a second proxy. The key point is that the route object passed to proxyRequest should not have any new members added to it by proxyRequest. I am finding it not the case.

[Marak]

Without more information, I can make a guess.

If you are seeing options being modified when it shouldn't, would it be possible to solve this if you declared your options in two different variables. Using each options hash for each server?

[tjanczuk]

Yes, the issue has a very simple workaround once you identify it. It just took me 4 hours of pulling my hair out before zeroing in on the issue, so I thought a fix in http-proxy could save this time for others. As far as I could tell one place that adds members to the instance is https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy/routing-proxy.js#L87.

[Marak]

I think I got it now.

Maybe we can just create a new object internally and copy all the options props so this won't happen?

What do you think?

[tjanczuk]

Either that or wrap the options I am passing you and make sure they remain immutable while you can add whatever state you need to the wrapper object. The former is probably less intrusive to the code base, the latter probably easier on the garbage collector.

[coderarity]

Yeah, could just clone the options object, like options = utile.clone(options);.

[Marak]

Yes, probably better to just clone the object inside http-proxy so that the passed in options don't get modified.

[indexzero]

So the immutability of the options object is preserved for every call to RoutingProxy.prototype.proxyRequest and RoutingProxy.prototype.proxyWebSocketRequest except the first one. With that in mind the following:

https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy/routing-proxy.js#L205-207
(and)
https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy/routing-proxy.js#L242-244

should be:

  if (!this.proxies[key]) {
    this.add(clone(options));
  }

@chjj I'd prefer not to bring the entire utile dependency for a single method which we could better implement specifically for this task.