RoutingProxy.proxyRequest modifies the options parameter #248

Closed
tjanczuk opened this Issue May 18, 2012 · 11 comments

Comments

Projects
None yet
4 participants
@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

This comment has been minimized.

Show comment Hide comment
@Marak

Marak May 18, 2012

Contributor

Could you create a failing test that replicates the issue?

Contributor

Marak commented May 18, 2012

Could you create a failing test that replicates the issue?

@tjanczuk

This comment has been minimized.

Show comment Hide comment
@tjanczuk

tjanczuk May 18, 2012

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);
}

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

This comment has been minimized.

Show comment Hide comment
@Marak

Marak May 18, 2012

Contributor

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.

Contributor

Marak commented May 18, 2012

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

This comment has been minimized.

Show comment Hide comment
@tjanczuk

tjanczuk May 18, 2012

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.

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

This comment has been minimized.

Show comment Hide comment
@Marak

Marak May 18, 2012

Contributor

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?

Contributor

Marak commented May 18, 2012

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

This comment has been minimized.

Show comment Hide comment
@tjanczuk

tjanczuk May 18, 2012

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.

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

This comment has been minimized.

Show comment Hide comment
@Marak

Marak May 18, 2012

Contributor

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?

Contributor

Marak commented May 18, 2012

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

This comment has been minimized.

Show comment Hide comment
@tjanczuk

tjanczuk May 18, 2012

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.

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

This comment has been minimized.

Show comment Hide comment
@coderarity

coderarity May 19, 2012

Contributor

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

Contributor

coderarity commented May 19, 2012

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

@Marak

This comment has been minimized.

Show comment Hide comment
@Marak

Marak May 20, 2012

Contributor

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

Contributor

Marak commented May 20, 2012

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

@indexzero

This comment has been minimized.

Show comment Hide comment
@indexzero

indexzero May 21, 2012

Member

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.

Member

indexzero commented May 21, 2012

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.

@indexzero indexzero closed this in 4c1a2c1 Mar 9, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment