To be congruent with other popular http clients the port should be stripped from the 'Host' field when the port is 80. #404
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…some *cough-cloudfoundry.com* services to have issues
|
Please see For details about how each client handles a URL with port 80 specified. |
|
this assumes 80 is default, what about https? |
|
So the behavior appears to be he same with all the clients - although a bit harder to test. But they all strip 443 from the host field when using https. So I used a simple nodejs server and the various development tools with the browsers to verify the behavior. var fs = require('fs');
var http = require('https');
var privateKey = fs.readFileSync('privatekey.pem').toString();
var certificate = fs.readFileSync('certificate.pem').toString();
var credentials = {key: privateKey, cert: certificate};
var express = require('express');
var server = express();
var app = express();
app.use(express.bodyParser());
app.use(express.methodOverride());
app.use(app.router);
app.get("/",function(req,res){
console.log(req.headers);
console.log("Got /");
res.send("hello");
});
var https = http.createServer(credentials, app);
https.listen(443); |
| @@ -237,7 +237,8 @@ HttpProxy.prototype.proxyRequest = function (req, res, buffer) { | |||
| // don't revert this without documenting it! | |||
| // | |||
| if (this.changeOrigin) { | |||
| outgoing.headers.host = this.target.host + ':' + this.target.port; | |||
| var port = (this.target.port-80==0?"":":"+this.target.port); | |||
There was a problem hiding this comment.
Use this.target.port == 80 for type coercion
There was a problem hiding this comment.
Use this.target.protocol to determine if it should be 443 or 80.
|
No updates from @celer. Closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi,
So we encountered an interesting problem today, less so a bug with node-http-proxy but more of a incongruent behavior from when compared against other browsers and client's we've tested.
When working with cloudfoundry.com we found that their proxy did not consider a request to port 80 the same if the port was explicitly specified in the 'Host' header field.
So a request to http://foo.cloudfoundry.com:80/ and http://foo.cloudfoundry.com/ behaved differently. We narrowed it down to the difference in the Host field. The changeOrign logic included the port when crafting the 'Host' header regardless of the port number.
No big deal right? Well so everyone else strips port 80 because it is the default :(
You can test the behavior using "http://helloworld-ng.cloudify.com:80/" and "http://helloworld-ng.cloudify.com/" - notice that both URLs work the same in all of the above listed browsers
In curl we can see the failure case
(We've asked cloudfoundry to fix this issue, but to be clear the behavior in node-http-proxy is incongruent with every other client and browser we've tested)
So then the question becomes - should node-http-proxy do the same? Sadly it seems like this shouldn't be an issue but it is an unexpected behavior and no one else does it - hence why I think node-http-proxy should strip port 80 from the Host field if it is specified.