Skip to content

Commit

Permalink
Prevent open redirect allow list bypass due to encodeurl
Browse files Browse the repository at this point in the history
Co-authored-by: Jon Church <me@jonchurch.com>
  • Loading branch information
2 people authored and wesleytodd committed Mar 20, 2024
1 parent 567c9c6 commit 0867302
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 2 deletions.
20 changes: 19 additions & 1 deletion lib/response.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
};

/**
Expand Down
46 changes: 45 additions & 1 deletion test/res.location.js
Original file line number Diff line number Diff line change
@@ -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();
});
Expand All @@ -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()

Expand Down

0 comments on commit 0867302

Please sign in to comment.