Skip to content

Allow ability to set a max recursion depth in config. #308

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.

Sign up for GitHub

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

Merged
mattcoley merged 3 commits into from
Mar 13, 2019
Merged

Allow ability to set a max recursion depth in config. #308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4946e04
Allow ability to set a max recursion depth in config.
Mar 12, 2019
4e0b303
refactor.
Mar 12, 2019
f5e2351
nit.
Mar 13, 2019
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
18 changes: 16 additions & 2 deletions src/main/java/com/hubspot/jinjava/JinjavaConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public class JinjavaConfig {

private final boolean readOnlyResolver;
private final boolean enableRecursiveMacroCalls;
private final int maxMacroRecursionDepth;

private Map<Context.Library, Set<String>> disabled;
private final boolean failOnUnknownTokens;
Expand All @@ -54,11 +55,11 @@ public static Builder newBuilder() {
}

public JinjavaConfig() {
this(StandardCharsets.UTF_8, Locale.ENGLISH, ZoneOffset.UTC, 10, new HashMap<>(), false, false, true, false, false, 0, true, RandomNumberGeneratorStrategy.THREAD_LOCAL, false, 0);
this(StandardCharsets.UTF_8, Locale.ENGLISH, ZoneOffset.UTC, 10, new HashMap<>(), false, false, true, false, 0, false, 0, true, RandomNumberGeneratorStrategy.THREAD_LOCAL, false, 0);
}

public JinjavaConfig(Charset charset, Locale locale, ZoneId timeZone, int maxRenderDepth) {
this(charset, locale, timeZone, maxRenderDepth, new HashMap<>(), false, false, true, false, false, 0, true, RandomNumberGeneratorStrategy.THREAD_LOCAL, false, 0);
this(charset, locale, timeZone, maxRenderDepth, new HashMap<>(), false, false, true, false, 0, false, 0, true, RandomNumberGeneratorStrategy.THREAD_LOCAL, false, 0);
Copy link
Contributor

@boulter boulter Mar 13, 2019

Choose a reason for hiding this comment

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

might be nigh time for a builder here.

}

private JinjavaConfig(Charset charset,
Expand All @@ -70,6 +71,7 @@ private JinjavaConfig(Charset charset,
boolean lstripBlocks,
boolean readOnlyResolver,
boolean enableRecursiveMacroCalls,
int maxMacroRecursionDepth,
boolean failOnUnknownTokens,
long maxOutputSize,
boolean nestedInterpretationEnabled,
Expand All @@ -85,6 +87,7 @@ private JinjavaConfig(Charset charset,
this.lstripBlocks = lstripBlocks;
this.readOnlyResolver = readOnlyResolver;
this.enableRecursiveMacroCalls = enableRecursiveMacroCalls;
this.maxMacroRecursionDepth = maxMacroRecursionDepth;
this.failOnUnknownTokens = failOnUnknownTokens;
this.maxOutputSize = maxOutputSize;
this.nestedInterpretationEnabled = nestedInterpretationEnabled;
Expand Down Expand Up @@ -133,6 +136,10 @@ public boolean isEnableRecursiveMacroCalls() {
return enableRecursiveMacroCalls;
}

public int getMaxMacroRecursionDepth() {
return maxMacroRecursionDepth;
}

public Map<Library, Set<String>> getDisabled() {
return disabled;
}
Expand Down Expand Up @@ -166,6 +173,7 @@ public static class Builder {

private boolean readOnlyResolver = true;
private boolean enableRecursiveMacroCalls;
private int maxMacroRecursionDepth;
private boolean failOnUnknownTokens;
private boolean nestedInterpretationEnabled = true;
private RandomNumberGeneratorStrategy randomNumberGeneratorStrategy = RandomNumberGeneratorStrategy.THREAD_LOCAL;
Expand Down Expand Up @@ -220,6 +228,11 @@ public Builder withEnableRecursiveMacroCalls(boolean enableRecursiveMacroCalls)
return this;
}

public Builder withMaxMacroRecursionDepth(int maxMacroRecursionDepth) {
this.maxMacroRecursionDepth = maxMacroRecursionDepth;
return this;
}

public Builder withReadOnlyResolver(boolean readOnlyResolver) {
this.readOnlyResolver = readOnlyResolver;
return this;
Expand Down Expand Up @@ -260,6 +273,7 @@ public JinjavaConfig build() {
lstripBlocks,
readOnlyResolver,
enableRecursiveMacroCalls,
maxMacroRecursionDepth,
failOnUnknownTokens,
maxOutputSize,
nestedInterpretationEnabled,
Expand Down
14 changes: 12 additions & 2 deletions src/main/java/com/hubspot/jinjava/el/ext/AstMacroFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,25 @@ public Object eval(Bindings bindings, ELContext context) {
if (!macroFunction.isCaller()) {
try {
if (interpreter.getConfig().isEnableRecursiveMacroCalls()) {
macroStack.pushWithoutCycleCheck(getName());
if (interpreter.getConfig().getMaxMacroRecursionDepth() != 0) {
macroStack.pushWithMaxDepth(getName(), interpreter.getConfig().getMaxMacroRecursionDepth(), -1, -1);
} else {
macroStack.pushWithoutCycleCheck(getName());
}
} else {
macroStack.push(getName(), -1, -1);
}
} catch (MacroTagCycleException e) {

int maxDepth = interpreter.getConfig().getMaxMacroRecursionDepth();
String message = maxDepth == 0
? String.format("Cycle detected for macro '%s'", getName())
: String.format("Max recursion limit of %d reached for macro '%s'", maxDepth, getName());

interpreter.addError(new TemplateError(TemplateError.ErrorType.WARNING,
TemplateError.ErrorReason.EXCEPTION,
TemplateError.ErrorItem.TAG,
"Cycle detected for macro '" + getName() + "'",
message,
null,
e.getLineNumber(),
e.getStartPosition(),
Expand Down
11 changes: 11 additions & 0 deletions src/main/java/com/hubspot/jinjava/interpret/CallStack.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@ public class CallStack {
private final CallStack parent;
private final Class<? extends TagCycleException> exceptionClass;
private final Stack<String> stack = new Stack<>();
private final int depth;

public CallStack(CallStack parent, Class<? extends TagCycleException> exceptionClass) {
this.parent = parent;
this.exceptionClass = exceptionClass;

this.depth = parent == null ? 0 : parent.depth + 1;
}

public boolean contains(String path) {
Expand All @@ -35,6 +38,14 @@ public void pushWithoutCycleCheck(String path) {
stack.push(path);
}

public void pushWithMaxDepth(String path, int maxDepth, int lineNumber, int startPosition) {
if (depth < maxDepth) {
stack.push(path);
} else {
throw TagCycleException.create(exceptionClass, path, Optional.of(lineNumber), Optional.of(startPosition));
}
}

public void push(String path, int lineNumber, int startPosition) {
if (contains(path)) {
throw TagCycleException.create(exceptionClass, path, Optional.of(lineNumber), Optional.of(startPosition));
Expand Down
40 changes: 40 additions & 0 deletions src/test/java/com/hubspot/jinjava/lib/tag/MacroTagTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,46 @@ public void itAllowsMacroRecursionWhenEnabledInConfiguration() throws IOExceptio
}
}

@Test
public void itAllowsMacroRecursionWithMaxDepth() throws IOException {

interpreter = new Jinjava(JinjavaConfig.newBuilder()
.withEnableRecursiveMacroCalls(true)
.withMaxMacroRecursionDepth(10)
.build()).newInterpreter();
JinjavaInterpreter.pushCurrent(interpreter);

try {
String template = fixtureText("ending-recursion");
String out = interpreter.render(template);
assertThat(interpreter.getErrorsCopy()).isEmpty();
assertThat(out).contains("Hello Hello Hello Hello Hello");
}
finally {
JinjavaInterpreter.popCurrent();
}
}

@Test
public void itEnforcesMacroRecursionWithMaxDepth() throws IOException {

interpreter = new Jinjava(JinjavaConfig.newBuilder()
.withEnableRecursiveMacroCalls(true)
.withMaxMacroRecursionDepth(2)
.build()).newInterpreter();
JinjavaInterpreter.pushCurrent(interpreter);

try {
String template = fixtureText("ending-recursion");
String out = interpreter.render(template);
assertThat(interpreter.getErrorsCopy().get(0).getMessage()).contains("Max recursion limit of 2 reached for macro 'hello'");
assertThat(out).contains("Hello Hello");
}
finally {
JinjavaInterpreter.popCurrent();
}
}

@Test
public void itPreventsRecursionForMacroWithVar() {
String jinja = "{%- macro func(var) %}" +
Expand Down