Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send path in req.path and not the url #416

Merged
merged 1 commit into from Dec 18, 2013
Merged

Conversation

MiLk
Copy link
Contributor

@MiLk MiLk commented May 4, 2013

According to http://nodejs.org/api/http.html#http_http_request_options_callback and http://nodejs.org/api/https.html#https_https_request_options_callback

Instead of pass the full url as path, the url is parsed and the path extracted.

Without this change, it's not possible to access to several pages on dailymotion.

Signed-off-by: Emilien Kenler <hello@emilienkenler.com>
@mwilliamson
Copy link

I've hit the same issue, and just passing the path seems to fix it -- if this could be merged, it would be much appreciated.

@mmalecki
Copy link
Contributor

Do you have an example of full URL being passed as a path?
It's been referenced a couple of times in http-proxy issues already, never with a test or a code example.

@MiLk
Copy link
Contributor Author

MiLk commented Sep 27, 2013

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

httpProxy.createServer(function (req, res, proxy) {
  var options = {
    port: url.parse(req.url).port || 80 ,
    host: url.parse(req.url).hostname,
    method: req.method,
    path: url.parse(req.url).path,
    headers: req.headers,
  };
  proxy.proxyRequest(req, res, options);
}).listen(3128);
http_proxy=http://127.0.0.1:3128/ curl -I "http://www.dailymotion.com/video/xno3qf_another-episode-01-vostfr-hd_shortfilms"

On master:

HTTP/1.1 404 Not Found

With the fix:

HTTP/1.1 200 OK

@mwilliamson
Copy link

Similarly:

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

httpProxy.createServer(80, 'www.dmdc.osd.mil').listen(8000);
http_proxy=http://localhost:8000/ curl -I http://www.dmdc.osd.mil/

On master, wrong location header:

HTTP/1.1 302 Moved Temporarily
location: https://www.dmdc.osd.milhttp://www.dmdc.osd.mil/
server: BigIP
connection: Keep-Alive
content-length: 0
Date: Fri, 27 Sep 2013 14:27:44 GMT

With the fix:

HTTP/1.1 302 Moved Temporarily
location: https://www.dmdc.osd.mil/
server: BigIP
connection: Keep-Alive
content-length: 0
Date: Fri, 27 Sep 2013 14:29:32 GMT

@mwilliamson
Copy link

As a piece of wild speculation, I'd assume this happens since the first line of HTTP requests include an absolute URL when sending requests to a proxy.

http://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html#sec5.1.2

The absoluteURI form is REQUIRED when the request is being made to a proxy.

@MiLk
Copy link
Contributor Author

MiLk commented Oct 30, 2013

@mmalecki Do you want more informations ?

@indexzero
Copy link
Contributor

@MiLk We are in the process of a complete rewrite of node-http-proxy. Can you try out the caronte branch and see if this resolves your problem?

@mwilliamson
Copy link

I still get the same issue. Source file:

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

httpProxy.createProxyServer({target:'http://www.dmdc.osd.mil'}).listen(8000);

Output:

http_proxy=http://localhost:8000/ curl -I http://www.dmdc.osd.mil/
HTTP/1.1 302 Moved Temporarily
location: https://www.dmdc.osd.milhttp://www.dmdc.osd.mil/
server: BigIP
connection: Keep-Alive
content-length: 0
Date: Wed, 30 Oct 2013 15:59:37 GMT

Presumably this is because common.js still has the same (incorrect) logic as before:

outgoing.path = req.url;

@sequoiar
Copy link

@mwilliamson the fix should be

outgoing.path = req.url.match(/^(https?:)/gi)? url.parse(req.url).path : req.url;

@mwilliamson
Copy link

@sequoiar: I think the fix originally given by @MiLk suffices, and has the benefit of being slightly simpler.

@boutell
Copy link

boutell commented Dec 8, 2013

This bug also breaks socket.io's built-in server which delivers its custom builds of browser side javascript, making socket.io effectively incompatible with node-http-proxy at this time. The symptom is the dreaded "Welcome to socket.io" message appearing instead of the expected javascript source code.

The source code of socket.io parses the URL on the assumption that it begins with a / and does not have a protocol, host and port.

This should probably be fixed in socket.io for long-term compatibility with a future in which URIs will supposedly someday always be fully absolute in requests, however there are too many other sites and servers out there with the same issue for it to be sensible not to also address it in node-http-proxy.

The spec does say this is a bug in socket.io, but only the weakest sense that it's not a great example of being ready for HTTP 1.2; it also says that HTTP 1.1 clients will never present an absolute URL except in a request to a proxy. (A request from a proxy is not the same thing (: )

To allow for transition to absoluteURIs in all requests in future
versions of HTTP, all HTTP/1.1 servers MUST accept the absoluteURI
form in requests, even though HTTP/1.1 clients will only generate
them in requests to proxies.

yawnt added a commit that referenced this pull request Dec 18, 2013
Send path in req.path and not the url
@yawnt yawnt merged commit 3638938 into http-party:master Dec 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants