From 4946e04cfc5d3f516578fdc0daeee99928129688 Mon Sep 17 00:00:00 2001 From: Matt Coley Date: Tue, 12 Mar 2019 15:52:56 -0400 Subject: [PATCH 1/3] Allow ability to set a max recursion depth in config. --- .../com/hubspot/jinjava/JinjavaConfig.java | 18 ++++++++- .../jinjava/el/ext/AstMacroFunction.java | 16 +++++++- .../hubspot/jinjava/interpret/CallStack.java | 15 +++++++ .../hubspot/jinjava/lib/tag/MacroTagTest.java | 40 +++++++++++++++++++ 4 files changed, 85 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/JinjavaConfig.java b/src/main/java/com/hubspot/jinjava/JinjavaConfig.java index 3dccd85d0..2e82f5ba4 100644 --- a/src/main/java/com/hubspot/jinjava/JinjavaConfig.java +++ b/src/main/java/com/hubspot/jinjava/JinjavaConfig.java @@ -41,6 +41,7 @@ public class JinjavaConfig { private final boolean readOnlyResolver; private final boolean enableRecursiveMacroCalls; + private final int maxMacroRecursionDepth; private Map> disabled; private final boolean failOnUnknownTokens; @@ -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); } private JinjavaConfig(Charset charset, @@ -70,6 +71,7 @@ private JinjavaConfig(Charset charset, boolean lstripBlocks, boolean readOnlyResolver, boolean enableRecursiveMacroCalls, + int maxMacroRecursionDepth, boolean failOnUnknownTokens, long maxOutputSize, boolean nestedInterpretationEnabled, @@ -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; @@ -133,6 +136,10 @@ public boolean isEnableRecursiveMacroCalls() { return enableRecursiveMacroCalls; } + public int getMaxMacroRecursionDepth() { + return maxMacroRecursionDepth; + } + public Map> getDisabled() { return disabled; } @@ -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; @@ -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; @@ -260,6 +273,7 @@ public JinjavaConfig build() { lstripBlocks, readOnlyResolver, enableRecursiveMacroCalls, + maxMacroRecursionDepth, failOnUnknownTokens, maxOutputSize, nestedInterpretationEnabled, diff --git a/src/main/java/com/hubspot/jinjava/el/ext/AstMacroFunction.java b/src/main/java/com/hubspot/jinjava/el/ext/AstMacroFunction.java index 31c03b479..2910b1afb 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/AstMacroFunction.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/AstMacroFunction.java @@ -34,15 +34,27 @@ 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; + if (maxDepth == 0) { + message = String.format("Cycle detected for macro '%s'", getName()); + } else { + message = 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(), diff --git a/src/main/java/com/hubspot/jinjava/interpret/CallStack.java b/src/main/java/com/hubspot/jinjava/interpret/CallStack.java index e07951f59..952cf0929 100644 --- a/src/main/java/com/hubspot/jinjava/interpret/CallStack.java +++ b/src/main/java/com/hubspot/jinjava/interpret/CallStack.java @@ -8,10 +8,17 @@ public class CallStack { private final CallStack parent; private final Class exceptionClass; private final Stack stack = new Stack<>(); + private final int depth; public CallStack(CallStack parent, Class exceptionClass) { this.parent = parent; this.exceptionClass = exceptionClass; + + if (parent == null) { + this.depth = 0; + } else { + this.depth = parent.depth + 1; + } } public boolean contains(String path) { @@ -35,6 +42,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)); diff --git a/src/test/java/com/hubspot/jinjava/lib/tag/MacroTagTest.java b/src/test/java/com/hubspot/jinjava/lib/tag/MacroTagTest.java index 71077885f..883618380 100644 --- a/src/test/java/com/hubspot/jinjava/lib/tag/MacroTagTest.java +++ b/src/test/java/com/hubspot/jinjava/lib/tag/MacroTagTest.java @@ -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) %}" + From 4e0b303ceacf9e594f49244b3fbfe6c3757d8ec7 Mon Sep 17 00:00:00 2001 From: Matt Coley Date: Tue, 12 Mar 2019 15:57:52 -0400 Subject: [PATCH 2/3] refactor. --- src/main/java/com/hubspot/jinjava/interpret/CallStack.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/interpret/CallStack.java b/src/main/java/com/hubspot/jinjava/interpret/CallStack.java index 952cf0929..0e224ae56 100644 --- a/src/main/java/com/hubspot/jinjava/interpret/CallStack.java +++ b/src/main/java/com/hubspot/jinjava/interpret/CallStack.java @@ -14,11 +14,7 @@ public CallStack(CallStack parent, Class exceptionC this.parent = parent; this.exceptionClass = exceptionClass; - if (parent == null) { - this.depth = 0; - } else { - this.depth = parent.depth + 1; - } + this.depth = parent == null ? 0 : parent.depth + 1; } public boolean contains(String path) { From f5e23517f01885e08f869be0a356d7f998770e86 Mon Sep 17 00:00:00 2001 From: Matt Coley Date: Wed, 13 Mar 2019 10:26:26 -0400 Subject: [PATCH 3/3] nit. --- .../com/hubspot/jinjava/el/ext/AstMacroFunction.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/el/ext/AstMacroFunction.java b/src/main/java/com/hubspot/jinjava/el/ext/AstMacroFunction.java index 2910b1afb..375d61e5d 100644 --- a/src/main/java/com/hubspot/jinjava/el/ext/AstMacroFunction.java +++ b/src/main/java/com/hubspot/jinjava/el/ext/AstMacroFunction.java @@ -45,12 +45,10 @@ public Object eval(Bindings bindings, ELContext context) { } catch (MacroTagCycleException e) { int maxDepth = interpreter.getConfig().getMaxMacroRecursionDepth(); - String message; - if (maxDepth == 0) { - message = String.format("Cycle detected for macro '%s'", getName()); - } else { - message = String.format("Max recursion limit of %d reached for macro '%s'", maxDepth, getName()); - } + 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,