Skip to content

Add support for reverse proxies by checking req.hostname or req.host. Fixes #20 #21

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

domharrington
Copy link

@domharrington domharrington commented Dec 7, 2016

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

…ixes expressjs#20

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`
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Yes, as you noted in the other issue, we do strive to support raw Node.js server, not only Express. I think this is a good start, but has a few issues. I'll put together a more comprehensive list when I'm at a computer, but it's missing added docs, shouldn't be adding a connect dependency for tests, and you should not be applying that Host parsing to the result of hostname (nor req.host when express is below 5).

@domharrington
Copy link
Author

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

@dougwilson
Copy link
Contributor

I'll follow up on this next week.

@dougwilson
Copy link
Contributor

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.

@dougwilson dougwilson self-assigned this Dec 7, 2016
@dougwilson dougwilson added the pr label Dec 7, 2016
@dougwilson
Copy link
Contributor

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.

@dougwilson
Copy link
Contributor

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.

@domharrington
Copy link
Author

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?

@dougwilson
Copy link
Contributor

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].

test/test.js Outdated
next()
// Running tests on next tick so we can modify the request object via
// the `request` event on http.Server
process.nextTick(next)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be done some different way.

test/test.js Outdated

request(app)
.get('/')
.set('Host', '::1')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a valid Host header. Don't need to test that, imo.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, from briefly reading the ipv6 wiki page i falsely assumed that they didnt need to have square brackets if there was no port.

test/test.js Outdated
})

app.on('request', function (req) {
req.host = '::1'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a valid req.host.

@domharrington
Copy link
Author

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?

@dougwilson
Copy link
Contributor

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.

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still pending fixing the double host decoding in express 4 and below.

test/test.js Outdated
return http.createServer(function onRequest (req, res) {
// This allows you to perform changes to the request/response
// objects before our assertions
pretest = pretest || function () {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically, you don't want to reassign variables that represent function arguments.

test/test.js Outdated
// objects before our assertions
pretest = pretest || function () {}

return http.createServer().on('request', pretest).on('request', function onRequest (req, res) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably just call pretest(req, res) inside the onRequest function rather than adding an additional listener.

@domharrington
Copy link
Author

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:

[email protected]:

{"host":"[::1]"}

[email protected]:

{"host":"[::1]","hostname":"[::1]"}

[email protected]

{"host":"[::1]:3000","hostname":"[::1]"}

@dougwilson
Copy link
Contributor

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 :)

@domharrington
Copy link
Author

Have you had a chance to review this?

@dougwilson
Copy link
Contributor

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 💃

@domharrington
Copy link
Author

Ohh that always takes longer than you want it to. No rush - good luck!

@dougwilson
Copy link
Contributor

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.

@chrisblossom
Copy link

Any headway on this or anything that I can help with to get this moving?

@domharrington
Copy link
Author

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.

@Bramzor
Copy link

Bramzor commented Mar 5, 2019

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?

@PabloSzx PabloSzx mentioned this pull request Aug 3, 2019
kevinansfield added a commit to TryGhost/Utils that referenced this pull request Oct 24, 2019
no issue

- extracted from expressjs/vhost#21
- uses `req.hostname` if it's available. `req.hostname` is set to the `x-forwarded-host` value when `'trust proxy'` setting is enabled in express
@kateile
Copy link

kateile commented Mar 16, 2021

Can someone please merge this

daniellockyer pushed a commit to TryGhost/Ghost that referenced this pull request Jul 26, 2022
no issue

- extracted from expressjs/vhost#21
- uses `req.hostname` if it's available. `req.hostname` is set to the `x-forwarded-host` value when `'trust proxy'` setting is enabled in express
@erunion
Copy link

erunion commented Jan 18, 2023

Just checking in, @dougwilson can this get merged and released?

@erunion
Copy link

erunion commented Jan 23, 2023

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.

@dougwilson
Copy link
Contributor

dougwilson commented Jan 23, 2023

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 👍

@erunion
Copy link

erunion commented Jan 23, 2023

@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.

@dougwilson
Copy link
Contributor

@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.

@erunion
Copy link

erunion commented Jan 23, 2023

@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.

@dougwilson
Copy link
Contributor

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.

I think this was a duplicate line from the one above, so this node-version
wasn't getting applied when trying to install from 14.x
@domharrington
Copy link
Author

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: domharrington@0f7cd48

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants