From 9afde46cd101acb250d09677fdde2d2940f39ee5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Kohlschu=CC=88tter?= Date: Wed, 27 Dec 2023 18:29:13 +0100 Subject: [PATCH] Add support for "sane" strict variable checks The current "strict variables" checking strategy in Liquid Templates is somewhat unusable as one cannot even check for the existence of an object property without raising an error in strict mode. This is a known limitation, and actively being discussed upstream (liquid issue 1034). Several Jekyll template make use of checks like {% if page.mermaid %}, etc., preventing enforcement of strict mode. This patch adds a "sane" strict variable policy, which allows for such checks to succeed. Since this is new functionality specific to liqp, we keep the existing strict mode configuration unchanged. https://github.com/Shopify/liquid/issues/1034 --- src/main/java/liqp/TemplateParser.java | 50 ++++++++++++++++++++---- src/main/java/liqp/nodes/LookupNode.java | 45 +++++++++++++++++---- src/test/java/liqp/blocks/IfTest.java | 36 +++++++++++++++++ 3 files changed, 117 insertions(+), 14 deletions(-) diff --git a/src/main/java/liqp/TemplateParser.java b/src/main/java/liqp/TemplateParser.java index 1177c323..4c60c2a0 100644 --- a/src/main/java/liqp/TemplateParser.java +++ b/src/main/java/liqp/TemplateParser.java @@ -53,6 +53,30 @@ public enum ErrorMode { LAX } + /** + * Controls the "strict variables" checking strategy. + * + * @see liquid issue 1034 + */ + public enum StrictVariablesMode { + /** + * No strict variable checking. + */ + OFF, + + /** + * Strict variables, but allow checking for the existence of directly-nested variables without + * throwing an error (e.g., checking for "object.missing" is acceptable, but + * "object.nested.missing" isn't). + */ + SANE, + + /** + * Strict variable checking. + */ + STRICT + } + public final Flavor flavor; public final boolean stripSpacesAroundTags; public final boolean stripSingleLine; @@ -68,8 +92,13 @@ public enum ErrorMode { /** * The same as template.render!({}, strict_variables: true) in ruby + * + * @see #strictVariablesMode */ + @Deprecated(forRemoval = true) public final boolean strictVariables; + public final StrictVariablesMode strictVariablesMode; + /** * This field doesn't have equivalent in ruby. */ @@ -138,7 +167,7 @@ public static class Builder { private Boolean liquidStyleWhere; - private boolean strictVariables = false; + private StrictVariablesMode strictVariablesMode = StrictVariablesMode.OFF; private boolean showExceptionsFromInclude; private EvaluateMode evaluateMode = EvaluateMode.LAZY; private Locale locale = DEFAULT_LOCALE; @@ -165,7 +194,7 @@ public Builder(TemplateParser parser) { this.insertions = new ArrayList<>(parser.insertions.values()); this.filters = new ArrayList<>(parser.filters.values()); - this.strictVariables = parser.strictVariables; + this.strictVariablesMode = parser.strictVariablesMode; this.evaluateMode = parser.evaluateMode; this.locale = parser.locale; this.renderTransformer = parser.renderTransformer; @@ -248,9 +277,15 @@ public Builder withLiquidStyleInclude(boolean liquidStyleInclude) { return this; } - @SuppressWarnings("hiding") public Builder withStrictVariables(boolean strictVariables) { - this.strictVariables = strictVariables; + this.strictVariablesMode = strictVariables ? StrictVariablesMode.STRICT + : StrictVariablesMode.OFF; + return this; + } + + @SuppressWarnings("hiding") + public Builder withStrictVariables(StrictVariablesMode strictVariablesMode) { + this.strictVariablesMode = Objects.requireNonNull(strictVariablesMode); return this; } @@ -404,12 +439,12 @@ public TemplateParser build() { nameResolver = new LocalFSNameResolver(snippetsFolderName); } - return new TemplateParser(strictVariables, showExceptionsFromInclude, evaluateMode, renderTransformer, locale, defaultTimeZone, environmentMapConfigurator, errorMode, fl, stripSpacesAroundTags, stripSingleLine, mapper, + return new TemplateParser(strictVariablesMode, showExceptionsFromInclude, evaluateMode, renderTransformer, locale, defaultTimeZone, environmentMapConfigurator, errorMode, fl, stripSpacesAroundTags, stripSingleLine, mapper, allInsertions, finalFilters, evaluateInOutputTag, strictTypedExpressions, liquidStyleInclude, liquidStyleWhere, nameResolver, limitMaxIterations, limitMaxSizeRenderedString, limitMaxRenderTimeMillis, limitMaxTemplateSizeBytes); } } - TemplateParser(boolean strictVariables, boolean showExceptionsFromInclude, EvaluateMode evaluateMode, + TemplateParser(StrictVariablesMode strictVariablesMode, boolean showExceptionsFromInclude, EvaluateMode evaluateMode, RenderTransformer renderTransformer, Locale locale, ZoneId defaultTimeZone, Consumer> environmentMapConfigurator, ErrorMode errorMode, Flavor flavor, boolean stripSpacesAroundTags, boolean stripSingleLine, ObjectMapper mapper, Insertions insertions, Filters filters, boolean evaluateInOutputTag, @@ -427,7 +462,8 @@ public TemplateParser build() { this.liquidStyleInclude = liquidStyleInclude; this.liquidStyleWhere = liquidStyleWhere; - this.strictVariables = strictVariables; + this.strictVariablesMode = strictVariablesMode; + this.strictVariables = strictVariablesMode != StrictVariablesMode.OFF; this.showExceptionsFromInclude = showExceptionsFromInclude; this.evaluateMode = evaluateMode; this.renderTransformer = renderTransformer == null ? RenderTransformerDefaultImpl.INSTANCE diff --git a/src/main/java/liqp/nodes/LookupNode.java b/src/main/java/liqp/nodes/LookupNode.java index 173253cd..5df3a3a9 100644 --- a/src/main/java/liqp/nodes/LookupNode.java +++ b/src/main/java/liqp/nodes/LookupNode.java @@ -51,15 +51,46 @@ public Object render(TemplateContext context) { } } - for(Indexable index : indexes) { - value = index.get(value, context, found); + boolean foundAllButLastOne = false; + for (Iterator it = indexes.iterator(); it.hasNext();) { + Indexable index = it.next(); + if (it.hasNext()) { + value = index.get(value, context, found); + if (value == null) { + found.set(false); + break; + } + } else { + // last item + value = index.get(value, context, found); + if (!found.get()) { + foundAllButLastOne = true; + } + } } - if(value == null && !found.get() && context.getParser().strictVariables) { - RuntimeException e = new VariableNotExistException(getVariableName()); - context.addError(e); - if (context.getErrorMode() == TemplateParser.ErrorMode.STRICT) { - throw e; + if (value == null && !found.get()) { + final boolean error; + switch (context.getParser().strictVariablesMode) { + case OFF: + error = false; + break; + case STRICT: + error = true; + break; + case SANE: + error = !foundAllButLastOne; + break; + default: + throw new UnsupportedOperationException(); + } + + if (error) { + RuntimeException e = new VariableNotExistException(getVariableName()); + context.addError(e); + if (context.getErrorMode() == TemplateParser.ErrorMode.STRICT) { + throw e; + } } } diff --git a/src/test/java/liqp/blocks/IfTest.java b/src/test/java/liqp/blocks/IfTest.java index 81ace433..8bb3204a 100644 --- a/src/test/java/liqp/blocks/IfTest.java +++ b/src/test/java/liqp/blocks/IfTest.java @@ -2,13 +2,18 @@ import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.assertThrows; + +import java.util.Collections; import org.antlr.v4.runtime.RecognitionException; import org.junit.Test; import liqp.Template; import liqp.TemplateParser; +import liqp.TemplateParser.StrictVariablesMode; import liqp.exceptions.LiquidException; +import liqp.exceptions.VariableNotExistException; public class IfTest { @@ -410,4 +415,35 @@ public void and_or_evaluation_orderTest() throws RecognitionException { assertThat(TemplateParser.DEFAULT.parse("{% if true or false and false %}TRUE{% else %}FALSE{% endif %}").render(), is("TRUE")); assertThat(TemplateParser.DEFAULT.parse("{% if true and false and false or true %}TRUE{% else %}FALSE{% endif %}").render(), is("FALSE")); } + + @Test + public void strictVariablesTest() throws RecognitionException { + assertThat(new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.STRICT).build() + .parse("{% if obj.var %}true{% else %}false{% endif %}").render(Collections.singletonMap( + "obj", Collections.singletonMap("var", "val"))), is("true")); + + assertThrows(VariableNotExistException.class, () -> { + new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.STRICT).build().parse( + "{% if obj.missing %}true{% else %}false{% endif %}").render(Collections.singletonMap( + "obj", Collections.singletonMap("var", "val"))); + }); + + assertThat(new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.SANE).build() + .parse("{% if obj.missing %}true{% else %}false{% endif %}").render(Collections.singletonMap( + "obj", Collections.singletonMap("var", "val"))), is("false")); + + assertThrows(VariableNotExistException.class, () -> { + new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.SANE).build().parse( + "{% if obj.nested.missing %}true{% else %}false{% endif %}").render(Collections + .singletonMap("obj", Collections.singletonMap("var", "val"))); + }); + + assertThat(new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.OFF).build() + .parse("{% if obj.missing %}true{% else %}false{% endif %}").render(Collections.singletonMap( + "obj", Collections.singletonMap("var", "val"))), is("false")); + + assertThat(new TemplateParser.Builder().withStrictVariables(StrictVariablesMode.OFF).build() + .parse("{% if obj.nested.missing %}true{% else %}false{% endif %}").render(Collections + .singletonMap("obj", Collections.singletonMap("var", "val"))), is("false")); + } }