Improved fix for open redirect allow list bypass

Co-authored-by: Jon Church <me@jonchurch.com>
Co-authored-by: Blake Embrey <hello@blakeembrey.com>

History.md

@@ -1,3 +1,8 @@
+unreleased
+==========
+
+  * Improved fix for open redirect allow list bypass
+
 4.19.1 / 2024-03-20
 ==========
 

lib/response.js

@@ -34,7 +34,6 @@ var extname = path.extname;
 var mime = send.mime;
 var resolve = path.resolve;
 var vary = require('vary');
-var urlParse = require('url').parse;
 
 /**
  * Response prototype.
@@ -56,6 +55,7 @@ module.exports = res
  */
 
 var charsetRegExp = /;\s*charset\s*=/;
+var schemaAndHostRegExp = /^(?:[a-zA-Z][a-zA-Z0-9+.-]*:)?\/\/[^\\\/\?]+/;
 
 /**
  * Set status `code`.
@@ -905,32 +905,23 @@ res.cookie = function (name, value, options) {
  */
 
 res.location = function location(url) {
-  var loc = String(url);
+  var loc;
 
   // "back" is an alias for the referrer
   if (url === 'back') {
     loc = this.req.get('Referrer') || '/';
+  } else {
+    loc = String(url);
   }
 
-  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);
-    }
-  }
+  var m = schemaAndHostRegExp.exec(loc);
+  var pos = m ? m[0].length + 1 : 0;
 
-  // set location
-  return this.set('Location', encodedUrl);
+  // Only encode after host to avoid invalid encoding which can introduce
+  // vulnerabilities (e.g. `\\` to `%5C`).
+  loc = loc.slice(0, pos) + encodeUrl(loc.slice(pos));
+
+  return this.set('Location', loc);
 };
 
 /**

test/res.location.js

@@ -2,6 +2,7 @@
 
 var express = require('../')
   , request = require('supertest')
+  , assert = require('assert')
   , url = require('url');
 
 describe('res', function(){
@@ -45,49 +46,6 @@ 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' + encodeURIComponent('\\@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' + encodeURIComponent('\\@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()
-
-      app.use(function (req, res) {
-        res.location('https://google.com?q=%A710').end()
-      })
-
-      request(app)
-      .get('/')
-      .expect('Location', 'https://google.com?q=%A710')
-      .expect(200, done)
-    })
-
     describe('when url is "back"', function () {
       it('should set location from "Referer" header', function (done) {
         var app = express()
@@ -146,6 +104,79 @@ describe('res', function(){
       })
     })
 
+    it('should encode data uri', function (done) {
+      var app = express()
+      app.use(function (req, res) {
+        res.location('data:text/javascript,export default () => { }').end();
+      });
+
+      request(app)
+        .get('/')
+        .expect('Location', 'data:text/javascript,export%20default%20()%20=%3E%20%7B%20%7D')
+        .expect(200, done)
+    })
+
+    it('should encode data uri', function (done) {
+      var app = express()
+      app.use(function (req, res) {
+        res.location('data:text/javascript,export default () => { }').end();
+      });
+
+      request(app)
+        .get('/')
+        .expect('Location', 'data:text/javascript,export%20default%20()%20=%3E%20%7B%20%7D')
+        .expect(200, done)
+    })
+
+    it('should consistently handle non-string input: boolean', function (done) {
+      var app = express()
+      app.use(function (req, res) {
+        res.location(true).end();
+      });
+
+      request(app)
+        .get('/')
+        .expect('Location', 'true')
+        .expect(200, done)
+    });
+
+    it('should consistently handle non-string inputs: object', function (done) {
+      var app = express()
+      app.use(function (req, res) {
+        res.location({}).end();
+      });
+
+      request(app)
+        .get('/')
+        .expect('Location', '[object%20Object]')
+        .expect(200, done)
+    });
+
+    it('should consistently handle non-string inputs: array', function (done) {
+      var app = express()
+      app.use(function (req, res) {
+        res.location([]).end();
+      });
+
+      request(app)
+        .get('/')
+        .expect('Location', '')
+        .expect(200, done)
+    });
+
+    it('should consistently handle empty string input', function (done) {
+      var app = express()
+      app.use(function (req, res) {
+        res.location('').end();
+      });
+
+      request(app)
+        .get('/')
+        .expect('Location', '')
+        .expect(200, done)
+    });
+
+
     if (typeof URL !== 'undefined') {
       it('should accept an instance of URL', function (done) {
         var app = express();
@@ -161,4 +192,194 @@ describe('res', function(){
       });
     }
   })
+
+  describe('location header encoding', function() {
+    function createRedirectServerForDomain (domain) {
+      var app = express();
+      app.use(function (req, res) {
+        var host = url.parse(req.query.q, false, true).host;
+        // This is here to show a basic check one might do which
+        // would pass but then the location header would still be bad
+        if (host !== domain) {
+          res.status(400).end('Bad host: ' + host + ' !== ' + domain);
+        }
+        res.location(req.query.q).end();
+      });
+      return app;
+    }
+
+    function testRequestedRedirect (app, inputUrl, expected, expectedHost, done) {
+      return request(app)
+        // Encode uri because old supertest does not and is required
+        // to test older node versions. New supertest doesn't re-encode
+        // so this works in both.
+        .get('/?q=' + encodeURIComponent(inputUrl))
+        .expect('') // No body.
+        .expect(200)
+        .expect('Location', expected)
+        .end(function (err, res) {
+          if (err) {
+            console.log('headers:', res.headers)
+            console.error('error', res.error, err);
+            return done(err, res);
+          }
+
+          // Parse the hosts from the input URL and the Location header
+          var inputHost = url.parse(inputUrl, false, true).host;
+          var locationHost = url.parse(res.headers['location'], false, true).host;
+
+          assert.strictEqual(locationHost, expectedHost);
+
+          // Assert that the hosts are the same
+          if (inputHost !== locationHost) {
+            return done(new Error('Hosts do not match: ' + inputHost + " !== " +  locationHost));
+          }
+
+          return done(null, res);
+        });
+    }
+
+    it('should not touch already-encoded sequences in "url"', function (done) {
+      var app = createRedirectServerForDomain('google.com');
+      testRequestedRedirect(
+        app,
+        'https://google.com?q=%A710',
+        'https://google.com?q=%A710',
+        'google.com',
+        done
+      );
+    });
+
+    it('should consistently handle relative urls', function (done) {
+      var app = createRedirectServerForDomain(null);
+      testRequestedRedirect(
+        app,
+        '/foo/bar',
+        '/foo/bar',
+        null,
+        done
+      );
+    });
+
+    it('should not encode urls in such a way that they can bypass redirect allow lists', function (done) {
+      var app = createRedirectServerForDomain('google.com');
+      testRequestedRedirect(
+        app,
+        'http://google.com\\@apple.com',
+        'http://google.com\\@apple.com',
+        'google.com',
+        done
+      );
+    });
+
+    it('should not be case sensitive', function (done) {
+      var app = createRedirectServerForDomain('google.com');
+      testRequestedRedirect(
+        app,
+        'HTTP://google.com\\@apple.com',
+        'HTTP://google.com\\@apple.com',
+        'google.com',
+        done
+      );
+    });
+
+    it('should work with https', function (done) {
+      var app = createRedirectServerForDomain('google.com');
+      testRequestedRedirect(
+        app,
+        'https://google.com\\@apple.com',
+        'https://google.com\\@apple.com',
+        'google.com',
+        done
+      );
+    });
+
+    it('should correctly encode schemaless paths', function (done) {
+      var app = createRedirectServerForDomain('google.com');
+      testRequestedRedirect(
+        app,
+        '//google.com\\@apple.com/',
+        '//google.com\\@apple.com/',
+        'google.com',
+        done
+      );
+    });
+
+    it('should percent encode backslashes in the path', function (done) {
+      var app = createRedirectServerForDomain('google.com');
+      testRequestedRedirect(
+        app,
+        'https://google.com/foo\\bar\\baz',
+        'https://google.com/foo%5Cbar%5Cbaz',
+        'google.com',
+        done
+      );
+    });
+
+    it('should encode backslashes in the path after the first backslash that triggered path parsing', function (done) {
+      var app = createRedirectServerForDomain('google.com');
+      testRequestedRedirect(
+        app,
+        'https://google.com\\@app\\l\\e.com',
+        'https://google.com\\@app%5Cl%5Ce.com',
+        'google.com',
+        done
+      );
+    });
+
+    it('should escape header splitting for old node versions', function (done) {
+      var app = createRedirectServerForDomain('google.com');
+      testRequestedRedirect(
+        app,
+        'http://google.com\\@apple.com/%0d%0afoo:%20bar',
+        'http://google.com\\@apple.com/%0d%0afoo:%20bar',
+        'google.com',
+        done
+      );
+    });
+
+    it('should encode unicode correctly', function (done) {
+      var app = createRedirectServerForDomain(null);
+      testRequestedRedirect(
+        app,
+        '/%e2%98%83',
+        '/%e2%98%83',
+        null,
+        done
+      );
+    });
+
+    it('should encode unicode correctly even with a bad host', function (done) {
+      var app = createRedirectServerForDomain('google.com');
+      testRequestedRedirect(
+        app,
+        'http://google.com\\@apple.com/%e2%98%83',
+        'http://google.com\\@apple.com/%e2%98%83',
+        'google.com',
+        done
+      );
+    });
+
+    it('should work correctly despite using deprecated url.parse', function (done) {
+      var app = createRedirectServerForDomain('google.com');
+      testRequestedRedirect(
+        app,
+        'https://google.com\'.bb.com/1.html',
+        'https://google.com\'.bb.com/1.html',
+        'google.com',
+        done
+      );
+    });
+
+    it('should encode file uri path', function (done) {
+      var app = createRedirectServerForDomain('');
+      testRequestedRedirect(
+        app,
+        'file:///etc\\passwd',
+        'file:///etc%5Cpasswd',
+        '',
+        done
+      );
+    });
+  });
 })