Encode URL in res.location/res.redirect if not already encoded

fixes #2897 fixes #3003
expressjs · Jun 14, 2016 · 76eaa32 · 76eaa32
1 parent c12cc88
commit 76eaa32
Show file tree

Hide file tree

Showing 5 changed files with 63 additions and 10 deletions.
diff --git a/History.md b/History.md
@@ -5,6 +5,7 @@ unreleased
   * Add `cacheControl` option to `res.sendFile`/`res.sendfile`
   * Add `options` argument to `req.range`
     - Includes the `combine` option
+  * Encode URL in `res.location`/`res.redirect` if not already encoded
   * Fix some redirect handling in `res.sendFile`/`res.sendfile`
   * Fix Windows absolute path check using forward slashes
   * Improve error with invalid arguments to `req.get()`

diff --git a/lib/response.js b/lib/response.js
@@ -14,6 +14,7 @@
 
 var contentDisposition = require('content-disposition');
 var deprecate = require('depd')('express');
+var encodeUrl = require('encodeurl');
 var escapeHtml = require('escape-html');
 var http = require('http');
 var isAbsolute = require('./utils').isAbsolute;
@@ -832,8 +833,7 @@ res.location = function location(url) {
   }
 
   // set location
-  this.set('Location', loc);
-  return this;
+  return this.set('Location', encodeUrl(loc));
 };
 
 /**
@@ -871,13 +871,12 @@ res.redirect = function redirect(url) {
   }
 
   // Set location header
-  this.location(address);
-  address = this.get('Location');
+  address = this.location(address).get('Location');
 
   // Support text/{plain,html} by default
   this.format({
     text: function(){
-      body = statusCodes[status] + '. Redirecting to ' + encodeURI(address);
+      body = statusCodes[status] + '. Redirecting to ' + address;
     },
 
     html: function(){

diff --git a/package.json b/package.json
@@ -35,6 +35,7 @@
     "cookie-signature": "1.0.6",
     "debug": "~2.2.0",
     "depd": "~1.1.0",
+    "encodeurl": "~1.0.1",
     "escape-html": "~1.0.3",
     "etag": "~1.7.0",
     "finalhandler": "0.4.1",

diff --git a/test/res.location.js b/test/res.location.js
@@ -16,5 +16,31 @@ describe('res', function(){
       .expect('Location', 'http://google.com')
       .expect(200, done)
     })
+
+    it('should encode "url"', function (done) {
+      var app = express()
+
+      app.use(function (req, res) {
+        res.location('https://google.com?q=\u2603 §10').end()
+      })
+
+      request(app)
+      .get('/')
+      .expect('Location', 'https://google.com?q=%E2%98%83%20%C2%A710')
+      .expect(200, 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)
+    })
   })
 })
diff --git a/test/res.redirect.js b/test/res.redirect.js
@@ -18,6 +18,32 @@ describe('res', function(){
       .expect('location', 'http://google.com')
       .expect(302, done)
     })
+
+    it('should encode "url"', function (done) {
+      var app = express()
+
+      app.use(function (req, res) {
+        res.redirect('https://google.com?q=\u2603 §10')
+      })
+
+      request(app)
+      .get('/')
+      .expect('Location', 'https://google.com?q=%E2%98%83%20%C2%A710')
+      .expect(302, done)
+    })
+
+    it('should not touch already-encoded sequences in "url"', function (done) {
+      var app = express()
+
+      app.use(function (req, res) {
+        res.redirect('https://google.com?q=%A710')
+      })
+
+      request(app)
+      .get('/')
+      .expect('Location', 'https://google.com?q=%A710')
+      .expect(302, done)
+    })
   })
 
   describe('.redirect(status, url)', function(){
@@ -85,16 +111,16 @@ describe('res', function(){
       var app = express();
 
       app.use(function(req, res){
-        res.redirect('<lame>');
+        res.redirect('<la\'me>');
       });
 
       request(app)
       .get('/')
       .set('Host', 'http://example.com')
       .set('Accept', 'text/html')
       .expect('Content-Type', /html/)
-      .expect('Location', '<lame>')
-      .expect(302, '<p>' + http.STATUS_CODES[302] + '. Redirecting to <a href="&lt;lame&gt;">&lt;lame&gt;</a></p>', done);
+      .expect('Location', '%3Cla\'me%3E')
+      .expect(302, '<p>' + http.STATUS_CODES[302] + '. Redirecting to <a href="%3Cla&#39;me%3E">%3Cla&#39;me%3E</a></p>', done)
     })
 
     it('should include the redirect type', function(done){
@@ -141,8 +167,8 @@ describe('res', function(){
       .set('Host', 'http://example.com')
       .set('Accept', 'text/plain, */*')
       .expect('Content-Type', /plain/)
-      .expect('Location', 'http://example.com/?param=<script>alert("hax");</script>')
-      .expect(302, http.STATUS_CODES[302] + '. Redirecting to http://example.com/?param=%3Cscript%3Ealert(%22hax%22);%3C/script%3E', done);
+      .expect('Location', 'http://example.com/?param=%3Cscript%3Ealert(%22hax%22);%3C/script%3E')
+      .expect(302, http.STATUS_CODES[302] + '. Redirecting to http://example.com/?param=%3Cscript%3Ealert(%22hax%22);%3C/script%3E', done)
     })
 
     it('should include the redirect type', function(done){