Repository: mozilla/nunjucks (js)

Issue:
Macros can see variables from their caller's scope.
In nunjucks currently, macros can see variables from their caller's scope, that were not explicitly passed in as arguments. E.g. the following template:

```
{% macro one(var) %}
  {{ two() }}
{% endmacro %}

{% macro two() %}
  {{ var }}
{% endmacro %}

{{ two("hello") }}
```

renders `hello`; macro `two` is able to see the value of `var` from macro `one`, even though it is not passed in as an argument.

 In Jinja2 that template renders nothing.

I consider the Jinja2 behavior correct, and the nunjucks behavior a bug.

It is possible, however, that people have written templates that depend on this bug to render correctly. For that reason, I'll probably fix it in 3.0 rather than in the 2.x series.

Pull request description:
Fix scoping issues with macros and includes.
This improves the fix for #577 and #561, expanding the fix for the latter to also cover includes, as well as macros.

The previous fix for #577 (PR #653) didn't take into account nested scopes within macros, which would happily bypass the `isolate` property of their parent frame and go right up into the calling frame.

A better fix for that is to make the macro frame actually completely isolated from the calling frame, breaking the parent relationship. This also fixes #666.

For includes (#561) we want included templates to _see_ variables in the including scope, but we don't ever want to write to the including scope. So we still need an `isolateWrites` attr on the frame, but it's specific to writes only, and (unlike in #653) it is respected not only at the originating level of the write, but also if a write from a child frame tries to go through it to its parent.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 67ec1c2..c893280 100644
--- 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
index c518c3e..b3351f8 100644
--- 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
index f9d9c69..1ed12e0 100644
--- 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
index 6ef8f74..90f01be 100644
--- 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,8 +59,8 @@ 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;
@@ -67,8 +68,8 @@ var Frame = Obj.extend({
         return p && p.resolve(name);
     },
 
-    push: function(isolate) {
-        return new Frame(this, isolate);
+    push: function(isolateWrites) {
+        return new Frame(this, isolateWrites);
     },
 
     pop: function() {


Test patch:
diff --git a/tests/compiler.js b/tests/compiler.js
index 2aaf604..2aa3af9 100644
--- 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
new file mode 100644
index 0000000..3a340b7
--- /dev/null
+++ b/tests/templates/include-set.html
@@ -0,0 +1,1 @@
+{{ var }}{% set var = 2 %}{{ var }}