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: lib/response.js, test/res.location.js
Commit date: 2024-03-20 10:18:54 -0500
Commit message: Prevent open redirect allow list bypass due to encodeurl

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

Diff: diff --git a/lib/response.js b/lib/response.js
index fede486c..f7c94d10 100644
--- a/lib/response.js
+++ b/lib/response.js
@@ -32,10 +32,11 @@ 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
  */
@@ -909,12 +910,29 @@ res.location = function location(url) {
   // "back" is an alias for the referrer
   if (url === 'back') {
     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);
 };
 
 /**
  * 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 158afac0..38cab027 100644
--- a/test/res.location.js
+++ b/test/res.location.js
@@ -1,15 +1,29 @@
 '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();
       });
 
       request(app)
@@ -29,10 +43,40 @@ 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\\@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()
 
       app.use(function (req, res) {
         res.location('https://google.com?q=%A710').end()

Vulnerable code:   return this.set('Location', encodeUrl(loc));
  , request = require('supertest');
Fixed code: var urlParse = require('url').parse;
  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);
    }
  }

  return this.set('Location', encodedUrl);
  , request = require('supertest')
  , url = require('url');
      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();

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