-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
Signed-off-by: Emilien Kenler <[email protected]>
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. |
Do you have an example of full URL being passed as a path? |
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);
On master:
With the fix:
|
Similarly: var httpProxy = require('http-proxy');
httpProxy.createServer(80, 'www.dmdc.osd.mil').listen(8000);
On master, wrong location header:
With the fix:
|
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. https://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html#sec5.1.2
|
@mmalecki Do you want more informations ? |
@MiLk We are in the process of a complete rewrite of |
I still get the same issue. Source file: var httpProxy = require('http-proxy');
httpProxy.createProxyServer({target:'https://www.dmdc.osd.mil'}).listen(8000); Output:
Presumably this is because
|
@mwilliamson the fix should be outgoing.path = req.url.match(/^(https?:)/gi)? url.parse(req.url).path : req.url; |
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 |
Send path in req.path and not the url
According to https://nodejs.org/api/http.html#http_http_request_options_callback and https://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.