vhost
vhost copied to clipboard
expressjs
Add support for reverse proxies by checking req.hostname or req.host. Fixes #20
For those that are coming to this issue and want to use this code, you can run the following to install from my fork:
npm i "github:domharrington/vhost#496f19be396ab2531149862bfacfdd72e8ae1751"
This is so that the module works when express has trust proxy enabled
and the Host header is expected to come from X-Forwarded-Host
I've added docs to the readme. I've also removed the connect dependency from the tests. To do this i had to change the tests to execute on the nextTick of the event loop so that I have a chance modify the request object in http.Server.on('request'); if you can think of a better way to do this that would be great.
Regarding the host parsing, is there any harm in doing it twice? We can definitely prevent it from happening in express v3 as this is documented to be void of portno http://expressjs.com/en/3x/api.html#req.host.
Regarding express v4/v5 (both of them have req.hostname, v4 just returns req.host https://github.com/expressjs/express/blob/c762b16f62e2c9f5de33481c250ef6f94a99eb7f/lib/request.js#L435-L437). Looks as though req.host has been added back in in v5, preserving the port number and req.hostname is void of portno?
We could do a check to see if express has been used and return early if you want?
var isexpress = req.hostname || req.host
if (isexpress) return isexpress
But the short, quick thought I have right now is that yes, you need to remove the double unwrapping and you need to find a better solution over adding that process.nextTick. I'll see if I can find some time next week to write a better follow up.
Also, keep in mind that this module strives to work with any server implementation, not just Express. This means that the presence of req.host / req.hostname doesn't mean Express is even being used.
A good example of why double-parsing is not going to work: IPv6. Take the header Host: [::1]:3000. First parse gives you ::1, but then the second parse by this module would mange that to an empty string.
I see that makes sense. I'm not too familiar with ipv6 addresses. Wouldn't the IPv6 issue already be a problem with this module, if the host header is Host: ::1?
We could look to use node's URL module to do this instead? That way we'd only need to perform the port removal if it doesnt exist?
Wouldn't the IPv6 issue already be a problem with this module, if the host header is Host: ::1?
No, because that's not even a valid Host header. It would be Host: [::1].
I've removed the invalid host headers and the tests are all still passing even with the portless IPv6 addresses. I've also removed the process.nextTick, is this an okay solution?
Hi @domharrington , awesome! It looks like the req.host issue still has not been resolved. Remember, in express below 5, req.host is not the host, but actually incorrectly the hostname. That means that in express below 4, when Host: [::1]:3000, the req.host property will incorrectly be populated as ::1 and req.hostname will not be defined in 3 and most of 4. This results in empty strings with this PR.
I can't seem to reproduce express below 4 returning a req.host of ::1.
Here are the outcomes for these properties in different versions of express:
With the following express app:
var app = require('express')()
app.get('/', function (req, res) {
res.end(JSON.stringify({ host: req.host, hostname: req.hostname }))
})
app.listen(3000)
And the following curl command:
curl "http://[::1]:3000"
We get these outcomes:
{"host":"[::1]"}
{"host":"[::1]","hostname":"[::1]"}
{"host":"[::1]:3000","hostname":"[::1]"}
Huh. I'm sooo sorry if I made to run against nothing :/ I probably should have left the PR at my comment that I would revisit this next week. I got carried away thinking I could actually review thins on my phone with running the PR or really looking at the source code for anything. I think let's leave it as is for now and I take that real look next week, as promised :)
I haven't had a chance yet, due to machine issues :S I did just earlier today get a brand new machine! So been working on setting up git and such so I'll be taking a look this weekend 💃
Ohh that always takes longer than you want it to. No rush - good luck!
Sorry I kinda forgot about this :/ Working on getting it fixed up & merged. I noticed that req.vhost.host property is incorrect with the changes, so need to fix that up.
Any headway on this or anything that I can help with to get this moving?
I've not had a chance to revisit this recently. @dougwilson mentioned that req.vhost.host is incorrect after the changes, but i'm not sure what changes need to be made.
Noticed today while trying to put an express behind a load balancer that it does not work yet with X-Forwarded-Host. What is the current status of this PR?
I feel it's a little passive aggressive to silently mark our comments asking for updates on this as spam. If you have no plans to merge this work in then maybe this PR should be closed out, or if this project is deprecated then maybe the repo should be archived, otherwise we would greatly appreciate it if you could tell us whatever we need to get this through the door.
Hi @erunion I'm not sure who marked them that way, as the project has various triagers who help with issues. I unhid them if that makes you feel better. As I mentioned above, this PR ends up where the req.vhost.host property is incorrect after them (it refects the non-proxied value instead of the proxied value) and the test suite also just hangs with this PR, both of which need to be fixed. I volunteered to fix them, but yes, that feel through, I'm sorry. The CI is broken in this project and I am working to fix that too, but didn't have the time to respond yet or fix them as I am working on a security release for a different project at the moment when you commented.
The project works just fine without this and is not deprecated. Perhaps instead of just ignoring the comment that there is an issue with the PR and demanding it be merged and released without any other comment, our triagers would not have marked your comment as spam. If you are looking for it to get released quicker, I look forward to you contributing the above fixes help to get it moving 👍
@dougwilson Sorry I wasn't demanding, we just wanted some clarification on what needed to be done here to wrap this up as it had been a couple years and all the tests are passing locally on versions of Node.
@erunion I added some inline comments on the code issues. Tests can easily pass when they are incomplete, which is the case here. Tests need to be built out as well to cover all the scenarios, particularly the couple where they fail with the existing code. Also tests do pass, but npm test then just hangs, which also needs to be addressed.
@dougwilson Thanks for the comments, we'll work on getting these fixed up.
What version of Node are you seeing npm test hang? I don't see this happening on Node 14, 16, or 18.
Hi @erunion it was happened on the CI server (though that whole things needs to be migrated now that Travis CI is gone), but also locally on Node.js 18 (node v18.13.0). Just FYI I need to head out, but I may be back later this week if you have more questions. The CI system needs to get re-setup on GH Actions here before merge, which will also confirm if the npm test hangs there too or not.
Hey! I just merged in your latest changes to get GH Actions CI working on my fork: https://github.com/domharrington/vhost/actions.
Having some issues with getting the tests running... I think I fixed one issue with the 14.x version: https://github.com/domharrington/vhost/commit/0f7cd48f6c1edb38e3e7f821ff83214854861454
Now there's a 500 coming back from the Node.js download for v11.x: https://github.com/domharrington/vhost/actions/runs/5068450197/jobs/9100808151. Any ideas?