CVE-2024-29041: Insufficient Information (NVD-CWE-noinfo)
value: Express.js minimalist web framework for node. Versions of Express.js prior to 4.19.0 and all pre-release alpha and beta versions of 5.0 are affected by an open redirect vulnerability using malformed URLs. When a user of Express performs a redirect using a user-provided URL Express performs an encode [using `encodeurl`](https://github.com/pillarjs/encodeurl) on the contents before passing it to the `location` header. This can cause malformed URLs to be evaluated in unexpected ways by common redirect allow list implementations in Express applications, leading to an Open Redirect via bypass of a properly implemented allow list. The main method impacted is `res.location()` but this is also called from within `res.redirect()`. The vulnerability is fixed in 4.19.2 and 5.0.0-beta.3.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
Published: 2024-03-25T21:15Z
Repository: https://github.com/expressjs/express
Language: JavaScript
Files changed: History.md, lib/response.js, test/res.location.js
Commit date: 2024-03-25 09:29:41 -0500
Commit message: Improved fix for open redirect allow list bypass

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

Diff: diff --git a/History.md b/History.md
index c4cdd94d..f62574a7 100644
--- a/History.md
+++ b/History.md
@@ -1,5 +1,10 @@
+unreleased
+==========
+
+  * Improved fix for open redirect allow list bypass
+
 4.19.1 / 2024-03-20
 ==========
 
   * Allow passing non-strings to res.location with new encoding handling checks
 
diff --git a/lib/response.js b/lib/response.js
index 6fddbf35..dd7b3c82 100644
--- a/lib/response.js
+++ b/lib/response.js
@@ -32,11 +32,10 @@ var cookie = require('cookie');
 var send = require('send');
 var extname = path.extname;
 var mime = send.mime;
 var resolve = path.resolve;
 var vary = require('vary');
-var urlParse = require('url').parse;
 
 /**
  * Response prototype.
  * @public
  */
@@ -54,10 +53,11 @@ module.exports = res
  * Module variables.
  * @private
  */
 
 var charsetRegExp = /;\s*charset\s*=/;
+var schemaAndHostRegExp = /^(?:[a-zA-Z][a-zA-Z0-9+.-]*:)?\/\/[^\\\/\?]+/;
 
 /**
  * Set status `code`.
  *
  * @param {Number} code
@@ -903,36 +903,27 @@ res.cookie = function (name, value, options) {
  * @return {ServerResponse} for chaining
  * @public
  */
 
 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;
+
+  // 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));
 
-  // set location
-  return this.set('Location', encodedUrl);
+  return this.set('Location', loc);
 };
 
 /**
  * Redirect to the given `url` with optional response `status`
  * defaulting to 302.
diff --git a/test/res.location.js b/test/res.location.js
index d1bbf4b6..c80b38de 100644
--- a/test/res.location.js
+++ b/test/res.location.js
@@ -1,9 +1,10 @@
 'use strict'
 
 var express = require('../')
   , request = require('supertest')
+  , assert = require('assert')
   , url = require('url');
 
 describe('res', function(){
   describe('.location(url)', function(){
     it('should set the header', function(done){
@@ -43,53 +44,10 @@ describe('res', function(){
       .get('/')
       .expect('Location', 'https://google.com?q=%E2%98%83%20%C2%A710')
       .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()
 
         app.use(function (req, res) {
@@ -144,10 +102,83 @@ describe('res', function(){
         .expect('Location', '/')
         .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 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();
 
         app.use(function(req, res){
@@ -159,6 +190,196 @@ describe('res', function(){
           .expect('Location', 'http://google.com/')
           .expect(200, done);
       });
     }
   })
+
+  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
+      );
+    });
+  });
 })

Vulnerable code: var urlParse = require('url').parse;
  var 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);
    }
  }
  // set location
  return this.set('Location', encodedUrl);
    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)
    })

Fixed code: unreleased
==========

  * Improved fix for open redirect allow list bypass

var schemaAndHostRegExp = /^(?:[a-zA-Z][a-zA-Z0-9+.-]*:)?\/\/[^\\\/\?]+/;
  var loc;
  } else {
    loc = String(url);
  var m = schemaAndHostRegExp.exec(loc);
  var pos = m ? m[0].length + 1 : 0;

  // 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);
  , assert = require('assert')
    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)
    });



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