Skip to content
This repository

http->https 302 redirects are improperly rewritten #192

Closed
quartzjer opened this Issue February 09, 2012 · 9 comments

5 participants

Jeremie Miller Charlie Robbins Ask Bjørn Hansen Olivier Vaillancourt Chris Manson
Jeremie Miller

The pull req #165 introduced broken logic, it rewrites every redirect, such that if a proxied services 302's any url (in our instance, our https-fronted and http-back-end sends a redirect to instagram's http cdn).

https://github.com/elfsternberg/node-http-proxy/blob/411bb51cc6/lib/node-http-proxy/http-proxy.js#L216

It rewrites every single redirect regardless :(

Ask Bjørn Hansen

It also breaks redirects to other protocols (however uncommon that is).

Charlie Robbins
Owner

@quartzjer How would you go about fixing it? Honestly I'm not sure I follow what's wrong.

Jeremie Miller

If there's a front-end of https://external/ and a back-end of http://internal/ when the back-end sends a redirect to http://some-other-site.com/foo.bar this code will rewrite it to https://sime-other-site.com/foo.bar, which is generally a bad thing to be rewriting every redirect :)

I honestly don't understand the original issue to suggest a fix as I'm not sure what the desired behavior would be, all I know is the current one is not desirable.

Jeremie Miller

Is there any way this can be fixed in master? I'm (undesirably) using a fork that just has this code commented out, and I don't know how this condition isn't horribly broken for everyone, every single redirect response is rewritten in this case.

Olivier Vaillancourt

+1

@quartzjer -> You could create a clean pull request from that fork to accelerate the fix.

Jeremie Miller
Olivier Vaillancourt

Yeah, looking at it, these lines (those commented out) do solve the specific problem of the user that submitted the fix but I'm pretty sure it's not really a desirable behaviour for a proxy...

You pretty much irreversibly loose control over your protocol with this "fix". I think it should either be configurable in the proxy or managed on the app side.

We're running an internal build of http-proxy that has the exact same lines commented out :P.

Chris Manson

Has there been any update on this issue?

Charlie Robbins
Owner

Should be fixed in 6a278b3

Charlie Robbins indexzero closed this March 09, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.