Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Handle missing Host: header #1579

Closed
wants to merge 9 commits into from

4 participants

@SLaks

Right now, a request without a Host: header (nor X-Forwarded-Host) will throw an error every time I access req.host.

I fixed that to return an empty string.

This also affects req.subdomains.

"".split(".") returns [ "" ].

However, since subdomains slices the initial portion of the host anyway, this will still correctly return []. (unless subdomain offset is 0)

@tj
tj commented

cool, just need a test for each if you dont mind! thanks man

@SLaks

Done

@SLaks SLaks referenced this pull request
Closed

Fix unit tests on Windows #1580

@SLaks

I'm not sure whether req.host should return '' or undefined if there is no header.

Thoughts?

@hallas

It should definitely be either undefined or null, since the "" (empty string) is an actual value, null is when the reference is there, but it doesn't hold anything, and undefined is when the reference doesn't exists.

I think it should be null.

@SLaks

Good idea.

What about req.subdomains? null or []?

Also, what if the Host: header is present but empty?
I think it should then be "".

@hallas

yeah, if it's present but empty, it should be the empty string, its the precise representation, subdomains is fine as an empty array.

@tj
tj commented

I'd vote undefined for host and empty array for subdomains

@SLaks SLaks Return null on missing Host: header
Breaking change; if X-Forwarded-Host: is present but empty, it will no
longer fall back to Host:
cfbd895
@SLaks

Done.

Note that this is a breaking change; if X-Forwarded-Host: is present but empty, it will now return '' rather than falling back to Host:

@SLaks

This tests a breaking change; if X-Forwarded-Host: is present but empty, it will now return '' rather than falling back to Host:

lib/request.js
@@ -472,7 +473,10 @@ req.__defineGetter__('path', function(){
req.__defineGetter__('host', function(){
var trustProxy = this.app.get('trust proxy');
var host = trustProxy && this.get('X-Forwarded-Host');
- host = host || this.get('Host');
+ if (host === false || typeof host === 'undefined') // If X-Forwarded-Host is present but empty, return it.
@tj
tj added a note

if (!host) here should be just fine, it shouldn't ever be a boolean and then there's no need for typeof

@tj
tj added a note

oh oops read that wrong, this seems a little weird to me still though

@SLaks
SLaks added a note

Javascript truthiness strikes again!

This allows missing headers, or trustProxy being false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/request.js
@@ -472,7 +473,10 @@ req.__defineGetter__('path', function(){
req.__defineGetter__('host', function(){
var trustProxy = this.app.get('trust proxy');
var host = trustProxy && this.get('X-Forwarded-Host');
- host = host || this.get('Host');
+ if (host === false || typeof host === 'undefined') // If X-Forwarded-Host is present but empty, return it.
+ host = this.get('Host');
+ if (typeof host === 'undefined')
@tj
tj added a note

this one wont need the typeof just if (!host) since it will never be "0" etc

@SLaks
SLaks added a note

No; '' is falsy.

typeof is required to distinguish between empty & missing headers.

It certainly is less clean that ! or &&, but I don't think there is any simpler way to get the desired behavior.

@SLaks
SLaks added a note

No; '' is falsy.

typeof is required to distinguish between empty & missing headers.

It certainly is less clean that ! or &&, but I don't think there is any simpler way to get the desired behavior.

@tj
tj added a note

not true, typeof isn't necessary unless something may not be declared, which is very rare. host == null covers both null and undefined

@SLaks
SLaks added a note

True, but I prefer to be more explicit (using strict equality).

Using === undefined would work, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
test/req.subdomains.js
((5 lines not shown))
+ describe('when there is no Host header', function () {
+ it('should return an empty array', function (done) {
+ var app = express();
+ app.set('subdomain offset', 0);
+
+ app.use(function (req, res, next) {
+ res.send(req.subdomains);
+ });
+
+ request(app)
+ .get('/')
+ .unset('Host')
+ .expect([], done);
+ })
+ })
+ describe('when there is an empty Host header', function () {
@tj
tj added a note

rad thanks for all the tests, only nitpick is a newline after each of these so they're not squished together, and function(){ to stay consistent

@SLaks
SLaks added a note

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@SLaks

Is this OK to merge?

@justinpincar

+1 please merge. A few search bots don't send the Host: header and it's irritating to have to wrap the getter with error handling.

@tj tj commented on the diff
lib/request.js
@@ -445,7 +444,9 @@ req.__defineGetter__('auth', function(){
req.__defineGetter__('subdomains', function(){
var offset = this.app.get('subdomain offset');
- return this.get('Host')
+ var host = this.get('Host');
@tj
tj added a note

we could use this.host here since we already handle it

@SLaks
SLaks added a note

Yes; in fact, subdomains currently ignores trust proxy completely, which is a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tj tj commented on the diff
lib/request.js
@@ -472,7 +473,10 @@ req.__defineGetter__('path', function(){
req.__defineGetter__('host', function(){
var trustProxy = this.app.get('trust proxy');
var host = trustProxy && this.get('X-Forwarded-Host');
- host = host || this.get('Host');
+ if (host === false || host === undefined) // If X-Forwarded-Host is present but empty, return it.
@tj
tj added a note

this doesn't make sense to me, if trustProxy is false it will roll over to Host anyway

@SLaks
SLaks added a note

The point of these ifs is to return "" if the appropriate header is present but empty, and to return null if the header is not there at all.

@tj
tj added a note

but why would you want an empty string?

@SLaks
SLaks added a note

To make it possible to distinguish between missing headers and empty headers.

I don't know how useful that is in practice, but I put it in case people want it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tj tj commented on the diff
lib/request.js
@@ -472,7 +473,10 @@ req.__defineGetter__('path', function(){
req.__defineGetter__('host', function(){
var trustProxy = this.app.get('trust proxy');
var host = trustProxy && this.get('X-Forwarded-Host');
- host = host || this.get('Host');
+ if (host === false || host === undefined) // If X-Forwarded-Host is present but empty, return it.
+ host = this.get('Host');
+ if (host === undefined)
@tj
tj added a note

likewise the host wont be falsey, so just if (!host) return; is fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tj tj closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 15, 2013
  1. @SLaks

    Handle missing Host: header

    SLaks authored
Commits on Apr 16, 2013
  1. @SLaks

    Add tests for Host: header

    SLaks authored
  2. @SLaks

    Add more tests for missing-Host:

    SLaks authored
  3. @SLaks

    Remove stray settings

    SLaks authored
  4. @SLaks

    Fix req.host tests

    SLaks authored
    Unset host: header, change from undefined to ""
    Upgrade supertest to catch undefined
  5. @SLaks
  6. @SLaks

    Return null on missing Host: header

    SLaks authored
    Breaking change; if X-Forwarded-Host: is present but empty, it will no
    longer fall back to Host:
  7. @SLaks

    Tweak test spacing

    SLaks authored
  8. @SLaks

    Use simpler undefined checks

    SLaks authored
This page is out of date. Refresh to see the latest.
View
10 lib/request.js
@@ -1,4 +1,3 @@
-
/**
* Module dependencies.
*/
@@ -445,7 +444,9 @@ req.__defineGetter__('auth', function(){
req.__defineGetter__('subdomains', function(){
var offset = this.app.get('subdomain offset');
- return this.get('Host')
+ var host = this.get('Host');
@tj
tj added a note

we could use this.host here since we already handle it

@SLaks
SLaks added a note

Yes; in fact, subdomains currently ignores trust proxy completely, which is a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ if (!host) return [];
+ return host
.split('.')
.reverse()
.slice(offset);
@@ -472,7 +473,10 @@ req.__defineGetter__('path', function(){
req.__defineGetter__('host', function(){
var trustProxy = this.app.get('trust proxy');
var host = trustProxy && this.get('X-Forwarded-Host');
- host = host || this.get('Host');
+ if (host === false || host === undefined) // If X-Forwarded-Host is present but empty, return it.
@tj
tj added a note

this doesn't make sense to me, if trustProxy is false it will roll over to Host anyway

@SLaks
SLaks added a note

The point of these ifs is to return "" if the appropriate header is present but empty, and to return null if the header is not there at all.

@tj
tj added a note

but why would you want an empty string?

@SLaks
SLaks added a note

To make it possible to distinguish between missing headers and empty headers.

I don't know how useful that is in practice, but I put it in case people want it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ host = this.get('Host');
+ if (host === undefined)
@tj
tj added a note

likewise the host wont be falsey, so just if (!host) return; is fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ return null;
return host.split(':')[0];
});
View
2  package.json
@@ -43,7 +43,7 @@
"should": "*",
"connect-redis": "*",
"github-flavored-markdown": "*",
- "supertest": "0.0.1"
+ "supertest": "0.6.0"
},
"keywords": [
"express",
View
157 test/req.host.js
@@ -0,0 +1,157 @@
+
+var express = require('../')
+ , request = require('./support/http');
+
+
+describe('req', function(){
+ describe('.host', function(){
+ describe('when X-Forwarded-Host is present', function(){
+ describe('when "trust proxy" is enabled', function(){
+ it('should return the forwarded header', function (done){
+ var app = express();
+
+ app.enable('trust proxy');
+
+ app.use(function (req, res, next){
+ res.send(req.host);
+ });
+
+ request(app)
+ .get('/')
+ .set('Host', 'proxy.example.com:80')
+ .set('X-Forwarded-Host', 'example.com:80')
+ .expect('example.com', done);
+ })
+
+ it('should return the forwarded header even if empty', function (done){
+ var app = express();
+
+ app.enable('trust proxy');
+
+ app.use(function (req, res, next){
+ res.send(req.host);
+ });
+
+ request(app)
+ .get('/')
+ .set('Host', 'proxy.example.com:80')
+ .set('X-Forwarded-Host', '')
+ .expect('', done);
+ })
+
+ it('should return null when there are no headers', function (done){
+ var app = express();
+
+ app.enable('trust proxy');
+
+ app.use(function (req, res, next){
+ res.send(JSON.stringify(req.host));
+ });
+
+ request(app)
+ .get('/')
+ .unset('Host')
+ .expect('null', done);
+ })
+ })
+
+ describe('when "trust proxy" is disabled', function(){
+ it('should return null when there is no Host header', function (done){
+ var app = express();
+
+ app.use(function (req, res, next){
+ res.send(JSON.stringify(req.host));
+ });
+
+ request(app)
+ .get('/')
+ .set('X-Forwarded-Host', 'example.com:80')
+ .unset('Host')
+ .expect('null', done);
+ })
+
+ it('should return empty string when there is an empty Host header', function (done){
+ var app = express();
+
+ app.use(function (req, res, next){
+ res.send(req.host);
+ });
+
+ request(app)
+ .get('/')
+ .set('X-Forwarded-Host', 'example.com:80')
+ .set('Host', '')
+ .expect('', done);
+ })
+
+ it('should return the actual host address', function(done){
+ var app = express();
+
+ app.use(function(req, res, next){
+ res.send(req.host);
+ });
+
+ request(app)
+ .get('/')
+ .set('Host', 'proxy.example.com:80')
+ .set('X-Forwarded-Host', 'example.com:80')
+ .expect('proxy.example.com', done);
+ })
+ })
+ })
+
+ describe('when X-Forwarded-Host is not present', function (){
+ it('should return the domain only', function(done){
+ var app = express();
+
+ app.use(function(req, res, next){
+ res.send(req.host);
+ });
+
+ request(app)
+ .get('/')
+ .set('Host', 'example.com:8080')
+ .expect('example.com', done);
+ })
+
+ it('should return the domain only even when there is no port', function(done){
+ var app = express();
+
+ app.use(function(req, res, next){
+ res.send(req.host);
+ });
+
+ request(app)
+ .get('/')
+ .set('Host', 'example.com')
+ .expect('example.com', done);
+ })
+
+ it('should return null when there is no Host header', function (done){
+ var app = express();
+
+ app.use(function(req, res, next){
+ res.send(JSON.stringify(req.host));
+ });
+
+ request(app)
+ .get('/')
+ .unset('Host')
+ .expect('null', done);
+ })
+
+ it('should return empty string when there is an empty Host header', function (done){
+ var app = express();
+
+ app.use(function (req, res, next){
+ res.send(req.host);
+ });
+
+ request(app)
+ .get('/')
+ .set('Host', '')
+ .expect('', done);
+ })
+ })
+ })
+})
View
68 test/req.subdomains.js
@@ -19,11 +19,11 @@ describe('req', function(){
})
})
- describe('otherwise', function(){
- it('should return an empty array', function(done){
+ describe('otherwise', function (){
+ it('should return an empty array', function (done){
var app = express();
- app.use(function(req, res){
+ app.use(function (req, res){
res.send(req.subdomains);
});
@@ -34,6 +34,36 @@ describe('req', function(){
})
})
+ describe('when there is no Host header', function (){
+ it('should return an empty array', function (done){
+ var app = express();
+
+ app.use(function (req, res, next){
+ res.send(req.subdomains);
+ });
+
+ request(app)
+ .get('/')
+ .unset('Host')
+ .expect([], done);
+ })
+ })
+
+ describe('when there is an empty Host header', function (){
+ it('should return an empty array', function (done){
+ var app = express();
+
+ app.use(function (req, res, next){
+ res.send(req.subdomains);
+ });
+
+ request(app)
+ .get('/')
+ .set('Host', '')
+ .expect([], done);
+ })
+ })
+
describe('when subdomain offset is set', function(){
describe('when subdomain offset is zero', function(){
it('should return an array with the whole domain', function(done){
@@ -49,6 +79,38 @@ describe('req', function(){
.set('Host', 'tobi.ferrets.sub.example.com')
.expect('["com","example","sub","ferrets","tobi"]', done);
})
+
+ describe('when there is no Host header', function (){
+ it('should return an empty array', function (done){
+ var app = express();
+ app.set('subdomain offset', 0);
+
+ app.use(function (req, res, next){
+ res.send(req.subdomains);
+ });
+
+ request(app)
+ .get('/')
+ .unset('Host')
+ .expect([], done);
+ })
+ })
+
+ describe('when there is an empty Host header', function (){
+ it('should return an empty array', function (done){
+ var app = express();
+ app.set('subdomain offset', 0);
+
+ app.use(function (req, res, next){
+ res.send(req.subdomains);
+ });
+
+ request(app)
+ .get('/')
+ .set('Host', '')
+ .expect([], done);
+ })
+ })
})
describe('when present', function(){
View
6 test/support/http.js
@@ -1,2 +1,6 @@
-module.exports = require('supertest');
+module.exports = require('supertest');
+module.exports.Test.prototype.unset = function (field) {
+ this.request().removeHeader(field);
+ return this;
+};
Something went wrong with that request. Please try again.