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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Javascript truthiness strikes again!

This allows missing headers, or trustProxy being false.

host = this.get('Host');
if (typeof 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.

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

True, but I prefer to be more explicit (using strict equality).
Using === undefined would work, though.

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
150 changes: 150 additions & 0 deletions test/req.host.js
@@ -0,0 +1,150 @@

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);
})
})
})
})
66 changes: 63 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,35 @@ 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 +78,37 @@ 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 () {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Done

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;
};