Merge pull request #667 from carljm/scoping

Fix scoping issues with macros and includes.
mozilla · Feb 5, 2016 · 3cfb135 · 3cfb135
2 parents b00b322 + 51a70a1
commit 3cfb135
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -43,6 +43,10 @@ master (unreleased)
 * Fix whitespace control around nested tags/variables/comments. Thanks Ouyang
   Yadong. Merge of [#631](https://github.com/mozilla/nunjucks/pull/631).
 
+* [Breaking Change] Prevent macros from seeing or affecting their calling
+  scope; prevent includes from writing to the including scope. Merge of
+  [#667](https://github.com/mozilla/nunjucks/pull/667).
+
 * [Breaking Change] Prevent filter.escape from escaping SafeString. Thanks
   atian25. Merge of [#623](https://github.com/mozilla/nunjucks/pull/623).
 

diff --git a/src/compiler.js b/src/compiler.js
@@ -831,7 +831,8 @@ var Compiler = Object.extend({
             '[' + argNames.join(', ') + '], ',
             '[' + kwargNames.join(', ') + '], ',
             'function (' + realNames.join(', ') + ') {',
-            'frame = frame.push(true);',
+            'var callerFrame = frame;',
+            'frame = new runtime.Frame();',
             'kwargs = kwargs || {};',
             'if (kwargs.hasOwnProperty("caller")) {',
             'frame.set("caller", kwargs.caller); }'
@@ -866,7 +867,7 @@ var Compiler = Object.extend({
         });
 
         frame = frame.pop();
-        this.emitLine('frame = frame.pop();');
+        this.emitLine('frame = callerFrame;');
         this.emitLine('return new runtime.SafeString(' + bufferId + ');');
         this.emitLine('});');
         this.popBufferId();

diff --git a/src/environment.js b/src/environment.js
@@ -471,7 +471,7 @@ Template = Obj.extend({
         }
 
         var context = new Context(ctx || {}, _this.blocks, _this.env);
-        var frame = parentFrame ? parentFrame.push() : new Frame();
+        var frame = parentFrame ? parentFrame.push(true) : new Frame();
         frame.topLevel = true;
         var syncResult = null;
 

diff --git a/src/runtime.js b/src/runtime.js
@@ -7,11 +7,13 @@ var Obj = require('./object');
 // we know how to access variables. Block tags can introduce special
 // variables, for example.
 var Frame = Obj.extend({
-    init: function(parent, isolate) {
+    init: function(parent, isolateWrites) {
         this.variables = {};
         this.parent = parent;
         this.topLevel = false;
-        this.isolate = isolate;
+        // if this is true, writes (set) should never propagate upwards past
+        // this frame to its parent (though reads may).
+        this.isolateWrites = isolateWrites;
     },
 
     set: function(name, val, resolveUp) {
@@ -21,12 +23,11 @@ var Frame = Obj.extend({
         var obj = this.variables;
         var frame = this;
 
-        if(resolveUp && !frame.isolate) {
-            if((frame = this.resolve(parts[0]))) {
+        if(resolveUp) {
+            if((frame = this.resolve(parts[0], true))) {
                 frame.set(name, val);
                 return;
             }
-            frame = this;
         }
 
         for(var i=0; i<parts.length - 1; i++) {
@@ -58,17 +59,17 @@ var Frame = Obj.extend({
         return p && p.lookup(name);
     },
 
-    resolve: function(name) {
-        var p = this.parent;
+    resolve: function(name, forWrite) {
+        var p = (forWrite && this.isolateWrites) ? undefined : this.parent;
         var val = this.variables[name];
         if(val !== undefined && val !== null) {
             return this;
         }
         return p && p.resolve(name);
     },
 
-    push: function(isolate) {
-        return new Frame(this, isolate);
+    push: function(isolateWrites) {
+        return new Frame(this, isolateWrites);
     },
 
     pop: function() {

diff --git a/tests/compiler.js b/tests/compiler.js
@@ -584,6 +584,21 @@
             finish(done);
         });
 
+        it('should not leak variables set in nested scope within macro out to calling scope', function(done) {
+            equal('{% macro setFoo() %}' +
+                  '{% for y in [1] %}{% set x = "foo" %}{{ x }}{% endfor %}' +
+                  '{% endmacro %}' +
+                  '{% macro display() %}' +
+                  '{% set x = "bar" %}' +
+                  '{{ setFoo() }}' +
+                  '{{ x }}' +
+                  '{% endmacro %}' +
+                  '{{ display() }}',
+                  'foobar');
+
+            finish(done);
+        });
+
         it('should compile macros without leaking set to calling scope', function(done) {
             // This test checks that the issue #577 is resolved.
             // If the bug is not fixed, and set variables leak into the
@@ -607,6 +622,14 @@
             finish(done);
         });
 
+        it('should compile macros that cannot see variables in caller scope', function(done) {
+            equal('{% macro one(var) %}{{ two() }}{% endmacro %}' +
+                  '{% macro two() %}{{ var }}{% endmacro %}' +
+                  '{{ one("foo") }}',
+                  '');
+            finish(done);
+        });
+
         it('should compile call blocks', function(done) {
             equal('{% macro wrap(el) %}' +
                   '<{{ el }}>{{ caller() }}</{{ el }}>' +
@@ -844,6 +867,11 @@
             finish(done);
         });
 
+        it('should include templates that can see including scope, but not write to it', function(done) {
+            equal('{% set var = 1 %}{% include "include-set.html" %}{{ var }}', '12\n1');
+            finish(done);
+        });
+
         it('should include templates dynamically', function(done) {
             equal('hello world {% include tmpl %}',
                   { name: 'thedude', tmpl: 'include.html' },

diff --git a/tests/templates/include-set.html b/tests/templates/include-set.html
@@ -0,0 +1 @@
+{{ var }}{% set var = 2 %}{{ var }}