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

Handle missing Host: header #1579

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions lib/request.js
@@ -1,4 +1,3 @@

/**
* Module dependencies.
*/
Expand Down Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

if (!host) return [];
return host
.split('.')
.reverse()
.slice(offset);
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

but why would you want an empty string?

Copy link
Author

Choose a reason for hiding this comment

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

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.

host = this.get('Host');
if (host === undefined)
Copy link
Member

Choose a reason for hiding this comment

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

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

return null;
return host.split(':')[0];
});

Expand Down
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -43,7 +43,7 @@
"should": "*",
"connect-redis": "*",
"github-flavored-markdown": "*",
"supertest": "0.0.1"
"supertest": "0.6.0"
},
"keywords": [
"express",
Expand Down
157 changes: 157 additions & 0 deletions 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);
})
})
})
})
68 changes: 65 additions & 3 deletions test/req.subdomains.js
Expand Up @@ -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);
});

Expand All @@ -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){
Expand All @@ -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(){
Expand Down
6 changes: 5 additions & 1 deletion 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;
};