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

req.hostname: check first comma-delimited host #3494

Closed
iconoeugen opened this issue Dec 3, 2017 · 11 comments
Closed

req.hostname: check first comma-delimited host #3494

iconoeugen opened this issue Dec 3, 2017 · 11 comments
Labels
Milestone

Comments

@iconoeugen
Copy link
Contributor

@iconoeugen iconoeugen commented Dec 3, 2017

I have a nodejs application using express that is deployed on Openshift that brings a HAProxy LB that adds X-Forwarded headers and an Apache HTTP that acts as WAF (web application firewall - mod_security) in front of app. The Apache extends the X-Forwarded headers so I end up with:

'x-forwarded-host': 'first.example.com:3000, second.example.com:3000'

But this doesn't work with req.hostname as it currently assumes there's only one proxy.

See also #1646

@dougwilson

This comment has been minimized.

Copy link
Member

@dougwilson dougwilson commented Dec 3, 2017

Thanks for the report! I'll check it out. Would you also be able to say what is the value you are getting back for req.hostname for that header? Is it the header string or just the wrong hostname of the two?

@iconoeugen

This comment has been minimized.

Copy link
Contributor Author

@iconoeugen iconoeugen commented Dec 3, 2017

The last test I have done was on the HTTPS standard port and I got in the app "first.example.com, second.example.com" (without port number). The returned value for req.hostname is the complete X-Forwarded-Host value.

The PR #3495 is fixing this issue.

@dougwilson

This comment has been minimized.

Copy link
Member

@dougwilson dougwilson commented Dec 3, 2017

Oh, there is already a PR for this issue? I'll take a look. Is that an old closed PR?

@iconoeugen

This comment has been minimized.

Copy link
Contributor Author

@iconoeugen iconoeugen commented Dec 3, 2017

I just came into this problem and I thought I'll just provide the fix ;)

@dougwilson

This comment has been minimized.

Copy link
Member

@dougwilson dougwilson commented Dec 3, 2017

Are you saying that right now the header "first.example.com:3000, second.example.com:3000" from your post is being returned as "first.example.com, second.example.com"? That seems weird, not sure we would be stripping the port multiple times. Will need to investigate that. Would you be able to provide the followig?

  1. Version of Express.js
  2. A complete simple server that demonstrates that output when i make a request with that header?

Thanks!

@dougwilson

This comment has been minimized.

Copy link
Member

@dougwilson dougwilson commented Dec 3, 2017

Sorry, looks lile we may have posted at the same time. I'm confused: so what is the issue here? Im happy to take a look into the issue reported, just trying tl gather some more information to take a look. I see the PR you linked to is 4 years old, not sure whaf happened there.

@dougwilson

This comment has been minimized.

Copy link
Member

@dougwilson dougwilson commented Dec 3, 2017

Ah, the PR was fixing req.protocol (and thus req.secure). You're saying we need to apply a similar fix to req.hostname, is that the correct understanding? If so that makes sense :)

@iconoeugen

This comment has been minimized.

Copy link
Contributor Author

@iconoeugen iconoeugen commented Dec 3, 2017

This is correct. The same issue applies to req.hostname as it used to be for req.prtocol. The old issue is a similiar one, just fixing the req.protocol.

Right now the server is getting a request from the HAproxy + Apache HTTP reverse proxy chain with the X-Forwarded-Host value first.example.com, second.example.com

  • Before the fix the value returned by req.hostname is wrong: first.example.com, second.example.com.
  • After the fix the value returned by req.hostname is correct: first.example.com.
@sosoba

This comment has been minimized.

Copy link

@sosoba sosoba commented Feb 7, 2018

Simple middleware fix:

app.use((req,res,next)=>{
	const hostname = req.hostname.split(',',1)[0];
	Object.defineProperty(req, "hostname", {
		enumerable: true,
		configurable: true,
		value : hostname
	});
	next();
});
@sosoba

This comment has been minimized.

Copy link

@sosoba sosoba commented Aug 14, 2018

Express 5 fixed this problem but now req.host became wrong.

@dougwilson dougwilson mentioned this issue Oct 27, 2018
23 of 23 tasks complete
@dougwilson dougwilson added this to the 4.17 milestone Oct 27, 2018
@sla100

This comment has been minimized.

Copy link

@sla100 sla100 commented Mar 20, 2019

There is middleware fix for wrong "host" request property:

app.use((req, res, next) => {

  let host = req.get('X-Forwarded-Host');
  if (host) {
    const trust = req.app.get('trust proxy fn');
    if (!trust(req.connection.remoteAddress, 0)) {
      host = req.get('Host');
    }
  }else {
    host = req.get('Host');
  }
  if (host) {
    host = host.split(',', 1)[0];
  }
  Object.defineProperty(req, 'host', {
    configurable: true,
    enumerable: true,
    value:host,
  });

  next();
});
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.