Repository: mozilla/nunjucks (js)

Issue:
Bypass autoescape / XSS
The following string works as expected: 

``` javascript
var res = nunjucks.renderString('Hello {{ username }}', { username: '
Matt
' });
console.log(res); //Hello &lt;s&gt;Matt&lt;/s&gt;
```

If however the variable passed to the template is an array autoescape does nothing: 

``` javascript
var res = nunjucks.renderString('Hello {{ username }}', { username: ['
Matt
'] });
console.log(res); // Hello 
Matt

```

This looks to be intentional https://github.com/mozilla/nunjucks/blob/master/src/runtime.js#L209 
However if a autoescape is on any variable that is rendered in a {{ }} block and appended to the output I would expect it to escaped no matter the type.  If the var is just going to be part of a string concat anyway why not toString first then escape?  

In express / Koa / (anything else using qs or body-parser) is is trivial to coerce query params types. See the following simple example in express: 

``` javascript
var app = express();
var env = nunjucks.configure('views', {
    autoescape: true,
    express: app
});

app.get('/', function(req, res) {     
    // This should be fine autoescape is on...
    res.send(nunjucks.renderString('Hello {{ username }}', { username: req.query.name }));
    //res.render('index.html', { username: req.query.name }); 
});
```
`http://127.0.0.1:3000/?name[]=<script>alert(1)</script>matt`

I created a more detailed writeup and example app at:  https://github.com/matt-/nunjucks_test

Pull request description:
fix autoescape for non-string values; fixes #835
#835

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 87a224f..33eb539 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -26,6 +26,10 @@ Changelog
 * Fix handling of macro arg with default value which shares a name with another
   macro. Merge of [#791](https://github.com/mozilla/nunjucks/pull/791).
 
+* Fix potential cast-related XSS vulnerability in autoescape mode, and with `escape` filter.
+  Thanks Matt Austin for the report and Thomas Hunkapiller for the fix.
+  [#836](https://github.com/mozilla/nunjucks/pull/836)
+
 
 2.x (unreleased)
 ----------------
diff --git a/src/filters.js b/src/filters.js
index e12b1d3..1a53229 100644
--- a/src/filters.js
+++ b/src/filters.js
@@ -116,10 +116,10 @@ var filters = {
     },
 
     escape: function(str) {
-        if(typeof str === 'string') {
-            return r.markSafe(lib.escape(str));
+        if(str instanceof r.SafeString) {
+            return str;
         }
-        return str;
+        return r.markSafe(lib.escape(str.toString()));
     },
 
     safe: function(str) {
diff --git a/src/runtime.js b/src/runtime.js
index 32d721a..5b03d08 100644
--- a/src/runtime.js
+++ b/src/runtime.js
@@ -206,8 +206,8 @@ function markSafe(val) {
 function suppressValue(val, autoescape) {
     val = (val !== undefined && val !== null) ? val : '';
 
-    if(autoescape && typeof val === 'string') {
-        val = lib.escape(val);
+    if(autoescape && !(val instanceof SafeString)) {
+        val = lib.escape(val.toString());
     }
 
     return val;


Test patch:
diff --git a/tests/compiler.js b/tests/compiler.js
index 100e9a5..c295772 100644
--- a/tests/compiler.js
+++ b/tests/compiler.js
@@ -1395,6 +1395,14 @@
                 expect(res).to.be('&><\'"');
             });
 
+            render('{{ foo }}', { foo: ['<p>foo</p>']}, { autoescape: true }, function(err, res) {
+                expect(res).to.be('&lt;p&gt;foo&lt;/p&gt;');
+            });
+
+            render('{{ foo }}', { foo: {toString: function() {return '<p>foo</p>'}}}, { autoescape: true }, function(err, res) {
+                expect(res).to.be('&lt;p&gt;foo&lt;/p&gt;');
+            });
+
             finish(done);
         });
 
diff --git a/tests/filters.js b/tests/filters.js
index b26fb6b..d349a58 100644
--- a/tests/filters.js
+++ b/tests/filters.js
@@ -110,6 +110,15 @@
             finish(done);
         });
 
+        it('should work with non-string values', function(done) {
+            var res1 = render('{{ foo | escape }}', {foo: ['<html>']}, { autoescape: false });
+            expect(res1).to.be('&lt;html&gt;');
+
+            var res2 = render('{{ foo | escape }}', {foo: {toString: function() { return '<html>'; }}}, { autoescape: false });
+            expect(res2).to.be('&lt;html&gt;');
+            finish(done);
+        });
+
         it('should not escape safe strings with autoescape on', function(done) {
             var res1 = render('{{ "<html>" | safe | escape }}', {}, { autoescape: true });
             expect(res1).to.be('<html>');