Prevent open redirect allow list bypass due to encodeurl

Co-authored-by: Jon Church <me@jonchurch.com>

Author: 2 people

lib/response.js

@@ -34,6 +34,7 @@ var extname = path.extname;
 var mime = send.mime;
 var resolve = path.resolve;
 var vary = require('vary');
+var urlParse = require('url').parse;
 
 /**
  * Response prototype.
@@ -911,8 +912,25 @@ res.location = function location(url) {
  loc = this.req.get('Referrer') || '/';
  }
 
+ var lowerLoc = loc.toLowerCase();
+ var encodedUrl = encodeUrl(loc);
+ if (lowerLoc.indexOf('https://') === 0 || lowerLoc.indexOf('http://') === 0) {
+ try {
+ var parsedUrl = urlParse(loc);
+ var parsedEncodedUrl = urlParse(encodedUrl);
+ // Because this can encode the host, check that we did not change the host
+ if (parsedUrl.host !== parsedEncodedUrl.host) {
+ // If the host changes after encodeUrl, return the original url
+ return this.set('Location', loc);
+ }
+ } catch (e) {
+ // If parse fails, return the original url
+ return this.set('Location', loc);
+ }
+ }
+
  // set location
- return this.set('Location', encodeUrl(loc));
+ return this.set('Location', encodedUrl);
 };
 
 /**

test/res.location.js

@@ -1,13 +1,27 @@
 'use strict'
 
 var express = require('../')
- , request = require('supertest');
+ , request = require('supertest')
+ , url = require('url');
 
 describe('res', function(){
  describe('.location(url)', function(){
  it('should set the header', function(done){
  var app = express();
 
+ app.use(function(req, res){
+ res.location('http://google.com/').end();
+ });
+
+ request(app)
+ .get('/')
+ .expect('Location', 'http://google.com/')
+ .expect(200, done)
+ })
+
+ it('should preserve trailing slashes when not present', function(done){
+ var app = express();
+
  app.use(function(req, res){
  res.location('http://google.com').end();
  });
@@ -31,6 +45,36 @@ describe('res', function(){
  .expect(200, done)
  })
 
+ it('should not encode bad "url"', function (done) {
+ var app = express()
+
+ app.use(function (req, res) {
+ // This is here to show a basic check one might do which
+ // would pass but then the location header would still be bad
+ if (url.parse(req.query.q).host !== 'google.com') {
+ res.status(400).end('Bad url');
+ }
+ res.location(req.query.q).end();
+ });
+
+ request(app)
+ .get('/?q=http://google.com\\@apple.com')
+ .expect(200)
+ .expect('Location', 'http://google.com\\@apple.com')
+ .end(function (err) {
+ if (err) {
+ throw err;
+ }
+
+ // This ensures that our protocol check is case insensitive
+ request(app)
+ .get('/?q=HTTP://google.com\\@apple.com')
+ .expect(200)
+ .expect('Location', 'HTTP://google.com\\@apple.com')
+ .end(done)
+ });
+ });
+
  it('should not touch already-encoded sequences in "url"', function (done) {
  var app = express()