Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent recursion in Jinjava. #142

Merged
merged 4 commits into from
Aug 15, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/main/java/com/hubspot/jinjava/interpret/Context.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.Stack;
import java.util.stream.Collectors;

import com.google.common.collect.HashMultimap;
Expand Down Expand Up @@ -74,6 +75,8 @@ public enum Library {
private Boolean autoEscape;
private List<? extends Node> superBlock;

private final Stack<String> renderStack = new Stack<>();

public Context() {
this(null, null, null);
}
Expand Down Expand Up @@ -385,6 +388,18 @@ public void setRenderDepth(int renderDepth) {
this.renderDepth = renderDepth;
}

public void pushRenderStack(String template) {
renderStack.push(template);
}

public String popRenderStack() {
return renderStack.pop();
}

public Stack getRenderStack() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we really want to protect the underlying stack from being modified outside the Context, then I think you just need to implement doesStackContain(). Otherwise this allows total access to the stack contents.

return renderStack;
}

public void addDependency(String type, String identification) {
this.dependencies.get(type).add(identification);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import com.hubspot.jinjava.tree.output.BlockPlaceholderOutputNode;
import com.hubspot.jinjava.tree.output.OutputList;
import com.hubspot.jinjava.tree.output.OutputNode;
import com.hubspot.jinjava.tree.output.RenderedOutputNode;
import com.hubspot.jinjava.util.Variable;
import com.hubspot.jinjava.util.WhitespaceUtils;

Expand Down Expand Up @@ -207,8 +208,15 @@ public String render(Node root, boolean processExtendRoots) {

for (Node node : root.getChildren()) {
lineNumber = node.getLineNumber();
OutputNode out = node.render(this);
output.addNode(out);
if (context.getRenderStack().contains(node.getMaster().getImage())) {
// This is a circular rendering. Stop rendering it here.
output.addNode(new RenderedOutputNode(node.getMaster().getImage()));
} else {
context.pushRenderStack(node.getMaster().getImage());
OutputNode out = node.render(this);
context.popRenderStack();
output.addNode(out);
}
}

// render all extend parents, keeping the last as the root output
Expand Down
22 changes: 21 additions & 1 deletion src/test/java/com/hubspot/jinjava/lib/tag/MacroTagTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,27 @@ public void itAllowsMacroRecursionWhenEnabledInConfiguration() throws IOExceptio
}
}


@Test
public void itPreventsRecursionForMacroWithVar() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this test is not necessary, since the recursion cases are tested in the ExpressionNodeTest. This is to confirm that the recursion happens even when macro is called.

String jinja = "{%- macro allSpans(spans) %}" +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this example be simplified? It reflects the original issue, but I think the the test would be easier to understand if it the content of spans was smaller and it wasn't so specific to the original issue with spans, since it really could be a problem with any type of map.

"{%- for span in spans %}" +
"{{ span.tag }}" +
"{%- endfor %}" +
"{%- endmacro %}" +
"{%- set spans = {" +
" 'html_1' : {" +
" 'tag' : 'html {{ selector }}'," +
" 'span' : 12" +
" }" +
"} %}" +
"{% set selector='{{spans}}' %}" +
"{{ allSpans(spans, '') }}" +
"";
Node node = new TreeParser(interpreter, jinja).buildTree();
assertThat(JinjavaInterpreter.getCurrent() == interpreter).isTrue();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this line testing for?

assertThat(interpreter.render(node)).isEqualTo(
"html {html_1={tag=html {html_1={tag=html {{ selector }}, span=12}}, span=12}}");
}

private Node snippet(String jinja) {
return new TreeParser(interpreter, jinja).buildTree().getChildren().getFirst();
Expand Down
29 changes: 29 additions & 0 deletions src/test/java/com/hubspot/jinjava/tree/ExpressionNodeTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,35 @@ public void itAvoidsInfiniteRecursionWhenVarsContainBraceBlocks() throws Excepti
assertThat(node.render(interpreter).toString()).isEqualTo("hello {{ place }}");
}

@Test
public void itAvoidsInfiniteRecursionOfItself() throws Exception {
context.put("myvar", "hello {{myvar}}");

ExpressionNode node = fixture("simplevar");
// It renders once, and then stop further rendering after detecting recursion.
assertThat(node.render(interpreter).toString()).isEqualTo("hello hello {{myvar}}");
}

@Test
public void itNoRecursionHere() throws Exception {
context.put("myvar", "hello {{ place }}");
context.put("place", "{{location}}");
context.put("location", "this is a place.");

ExpressionNode node = fixture("simplevar");
assertThat(node.render(interpreter).toString()).isEqualTo("hello this is a place.");
}

@Test
public void itAvoidsInfiniteRecursion() throws Exception {
context.put("myvar", "hello {{ place }}");
context.put("place", "there, {{ location }}");
context.put("location", "this is {{ place }}");

ExpressionNode node = fixture("simplevar");
assertThat(node.render(interpreter).toString()).isEqualTo("hello there, this is {{ place }}");
}

@Test
public void itRendersStringRange() throws Exception {
context.put("theString", "1234567890");
Expand Down