Fix "trust proxy" setting to inherit when app is mounted

fixes expressjs#2550
fixes expressjs#2551

Author: dougwilson

History.md

@@ -1,6 +1,7 @@
 3.x
 ===
 
+  * Fix `"trust proxy"` setting to inherit when app is mounted
   * Generate `ETag`s for all request responses
     - No longer restricted to only responses for `GET` and `HEAD` requests
   * Use `content-type` to parse `Content-Type` headers

lib/application.js

@@ -1,5 +1,14 @@
+/*!
+ * express
+ * Copyright(c) 2009-2013 TJ Holowaychuk
+ * Copyright(c) 2013 Roman Shtylman
+ * Copyright(c) 2014-2015 Douglas Christopher Wilson
+ * MIT Licensed
+ */
+
 /**
  * Module dependencies.
+ * @api private
  */
 
 var connect = require('connect')
@@ -21,6 +30,13 @@ var merge = require('utils-merge');
 
 var app = exports = module.exports = {};
 
+/**
+ * Variable for trust proxy inheritance back-compat
+ * @api private
+ */
+
+var trustProxyDefaultSymbol = '@@symbol:trust_proxy_default';
+
 /**
  * Initialize the server.
  *
@@ -53,14 +69,27 @@ app.defaultConfiguration = function(){
   this.set('subdomain offset', 2);
   this.set('trust proxy', false);
 
+  // trust proxy inherit back-compat
+  Object.defineProperty(this.settings, trustProxyDefaultSymbol, {
+    configurable: true,
+    value: true
+  });
+
   debug('booting in %s mode', env);
 
   // implicit middleware
   this.use(connect.query());
   this.use(middleware.init(this));
 
-  // inherit protos
-  this.on('mount', function(parent){
+  this.on('mount', function onmount(parent) {
+    // inherit trust proxy
+    if (this.settings[trustProxyDefaultSymbol] === true
+      && typeof parent.settings['trust proxy fn'] === 'function') {
+      delete this.settings['trust proxy'];
+      delete this.settings['trust proxy fn'];
+    }
+
+    // inherit protos
     this.request.__proto__ = parent.request;
     this.response.__proto__ = parent.response;
     this.engines.__proto__ = parent.engines;
@@ -271,6 +300,13 @@ app.set = function(setting, val){
     case 'trust proxy':
       debug('compile trust proxy %s', val);
       this.set('trust proxy fn', compileTrust(val));
+
+      // trust proxy inherit back-compat
+      Object.defineProperty(this.settings, trustProxyDefaultSymbol, {
+        configurable: true,
+        value: false
+      });
+
       break;
   }
 

test/app.use.js

@@ -1,6 +1,7 @@
 
-var express = require('../')
-  , request = require('supertest');
+var assert = require('assert');
+var express = require('..');
+var request = require('supertest');
 
 describe('app', function(){
   it('should emit "mount" when mounted', function(done){

test/config.js

@@ -1,30 +1,36 @@
 
-var express = require('../')
-  , assert = require('assert');
+var assert = require('assert');
+var express = require('..');
 
-describe('config', function(){
-  describe('.set()', function(){
-    it('should set a value', function(){
+describe('config', function () {
+  describe('.set()', function () {
+    it('should set a value', function () {
       var app = express();
-      app.set('foo', 'bar').should.equal(app);
+      app.set('foo', 'bar');
+      assert.equal(app.get('foo'), 'bar');
+    })
+
+    it('should return the app', function () {
+      var app = express();
+      assert.equal(app.set('foo', 'bar'), app);
     })
 
-    it('should return the app when undefined', function(){
+    it('should return the app when undefined', function () {
       var app = express();
-      app.set('foo', undefined).should.equal(app);
+      assert.equal(app.set('foo', undefined), app);
     })
 
     describe('"etag"', function(){
       it('should throw on bad value', function(){
-        var app = express()
-        app.set.bind(app, 'etag', 42).should.throw(/unknown value/)
+        var app = express();
+        assert.throws(app.set.bind(app, 'etag', 42), /unknown value/);
       })
 
       it('should set "etag fn"', function(){
         var app = express()
         var fn = function(){}
         app.set('etag', fn)
-        app.get('etag fn').should.equal(fn)
+        assert.equal(app.get('etag fn'), fn)
       })
     })
 
@@ -33,85 +39,124 @@ describe('config', function(){
         var app = express()
         var fn = function(){}
         app.set('trust proxy', fn)
-        app.get('trust proxy fn').should.equal(fn)
+        assert.equal(app.get('trust proxy fn'), fn)
       })
     })
   })
 
   describe('.get()', function(){
     it('should return undefined when unset', function(){
       var app = express();
-      assert(undefined === app.get('foo'));
+      assert.strictEqual(app.get('foo'), undefined);
     })
 
     it('should otherwise return the value', function(){
       var app = express();
       app.set('foo', 'bar');
-      app.get('foo').should.equal('bar');
+      assert.equal(app.get('foo'), 'bar');
     })
 
     describe('when mounted', function(){
       it('should default to the parent app', function(){
-        var app = express()
-          , blog = express();
+        var app = express();
+        var blog = express();
 
         app.set('title', 'Express');
         app.use(blog);
-        blog.get('title').should.equal('Express');
+        assert.equal(blog.get('title'), 'Express');
       })
-      
+
       it('should given precedence to the child', function(){
-        var app = express()
-          , blog = express();
+        var app = express();
+        var blog = express();
 
         app.use(blog);
         app.set('title', 'Express');
         blog.set('title', 'Some Blog');
 
-        blog.get('title').should.equal('Some Blog');
+        assert.equal(blog.get('title'), 'Some Blog');
+      })
+
+      it('should inherit "trust proxy" setting', function () {
+        var app = express();
+        var blog = express();
+
+        function fn() { return false }
+
+        app.set('trust proxy', fn);
+        assert.equal(app.get('trust proxy'), fn);
+        assert.equal(app.get('trust proxy fn'), fn);
+
+        app.use(blog);
+
+        assert.equal(blog.get('trust proxy'), fn);
+        assert.equal(blog.get('trust proxy fn'), fn);
+      })
+
+      it('should prefer child "trust proxy" setting', function () {
+        var app = express();
+        var blog = express();
+
+        function fn1() { return false }
+        function fn2() { return true }
+
+        app.set('trust proxy', fn1);
+        assert.equal(app.get('trust proxy'), fn1);
+        assert.equal(app.get('trust proxy fn'), fn1);
+
+        blog.set('trust proxy', fn2);
+        assert.equal(blog.get('trust proxy'), fn2);
+        assert.equal(blog.get('trust proxy fn'), fn2);
+
+        app.use(blog);
+
+        assert.equal(app.get('trust proxy'), fn1);
+        assert.equal(app.get('trust proxy fn'), fn1);
+        assert.equal(blog.get('trust proxy'), fn2);
+        assert.equal(blog.get('trust proxy fn'), fn2);
       })
     })
   })
 
   describe('.enable()', function(){
     it('should set the value to true', function(){
       var app = express();
-      app.enable('tobi').should.equal(app);
-      app.get('tobi').should.be.true;
+      assert.equal(app.enable('tobi'), app);
+      assert.strictEqual(app.get('tobi'), true);
     })
   })
 
   describe('.disable()', function(){
     it('should set the value to false', function(){
       var app = express();
-      app.disable('tobi').should.equal(app);
-      app.get('tobi').should.be.false;
+      assert.equal(app.disable('tobi'), app);
+      assert.strictEqual(app.get('tobi'), false);
     })
   })
 
   describe('.enabled()', function(){
     it('should default to false', function(){
       var app = express();
-      app.enabled('foo').should.be.false;
+      assert.strictEqual(app.enabled('foo'), false);
     })
 
     it('should return true when set', function(){
       var app = express();
       app.set('foo', 'bar');
-      app.enabled('foo').should.be.true;
+      assert.strictEqual(app.enabled('foo'), true);
     })
   })
 
   describe('.disabled()', function(){
     it('should default to true', function(){
       var app = express();
-      app.disabled('foo').should.be.true;
+      assert.strictEqual(app.disabled('foo'), true);
     })
 
     it('should return false when set', function(){
       var app = express();
       app.set('foo', 'bar');
-      app.disabled('foo').should.be.false;
+      assert.strictEqual(app.disabled('foo'), false);
     })
   })
 })

test/req.ip.js

@@ -35,6 +35,23 @@ describe('req', function(){
           .set('X-Forwarded-For', 'client, p1, p2')
           .expect('p1', done);
         })
+
+        it('should return the addr after trusted proxy, from sub app', function (done) {
+          var app = express();
+          var sub = express();
+
+          app.set('trust proxy', 2);
+          app.use(sub);
+
+          sub.use(function (req, res, next) {
+            res.send(req.ip);
+          });
+
+          request(app)
+          .get('/')
+          .set('X-Forwarded-For', 'client, p1, p2')
+          .expect(200, 'p1', done);
+        })
       })
 
       describe('when "trust proxy" is disabled', function(){