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.

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

Prevent accidental expressions from being output #1123

Merged
merged 3 commits into from
Oct 28, 2023
Merged

Prevent accidental expressions from being output #1123

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6fe39df
Add feature stub for preventing accidental expressions
jasmith-hs Oct 20, 2023
1a8d43a
Add trimming note block to prevent accidentally creating expressions/…
jasmith-hs Oct 24, 2023
160c254
Extract static function isNoteTagOrExprChar
jasmith-hs Oct 27, 2023
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
68 changes: 68 additions & 0 deletions src/main/java/com/hubspot/jinjava/tree/output/OutputList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@
import com.hubspot.jinjava.interpret.JinjavaInterpreter;
import com.hubspot.jinjava.interpret.OutputTooBigException;
import com.hubspot.jinjava.interpret.TemplateError;
import com.hubspot.jinjava.tree.parse.TokenScannerSymbols;
import com.hubspot.jinjava.util.LengthLimitingStringBuilder;
import java.util.LinkedList;
import java.util.List;

public class OutputList {
public static final String PREVENT_ACCIDENTAL_EXPRESSIONS =
"PREVENT_ACCIDENTAL_EXPRESSIONS";
private final List<OutputNode> nodes = new LinkedList<>();
private final List<BlockPlaceholderOutputNode> blocks = new LinkedList<>();
private final long maxOutputSize;
Expand Down Expand Up @@ -48,6 +51,71 @@ public List<BlockPlaceholderOutputNode> getBlocks() {
public String getValue() {
LengthLimitingStringBuilder val = new LengthLimitingStringBuilder(maxOutputSize);

return JinjavaInterpreter
.getCurrentMaybe()
.map(JinjavaInterpreter::getConfig)
.filter(
config ->
config
.getFeatures()
.getActivationStrategy(PREVENT_ACCIDENTAL_EXPRESSIONS)
.isActive(null)
)
.map(
config -> joinNodesWithoutAddingExpressions(val, config.getTokenScannerSymbols())
)
.orElseGet(() -> joinNodes(val));
}

private String joinNodesWithoutAddingExpressions(
LengthLimitingStringBuilder val,
TokenScannerSymbols tokenScannerSymbols
) {
@SuppressWarnings("StringBufferReplaceableByString")
String separator = new StringBuilder()
.append('\n')
.append(tokenScannerSymbols.getPrefixChar())
.append(tokenScannerSymbols.getNoteChar())
.append(tokenScannerSymbols.getTrimChar())
.append(' ')
.append(tokenScannerSymbols.getNoteChar())
.append(tokenScannerSymbols.getExprEndChar())
.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

You could create this statically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. Do you mean memoize it inside of TokenScannerSymbols? The result is different depending on what symbols are used

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking that TokenScannerSymbols were static. Doesn't seem that this will be called too often. If it does, then I think memoization would be helpful.

String prev = null;
String cur;
for (OutputNode node : nodes) {
try {
cur = node.getValue();
if (
prev != null &&
prev.length() > 0 &&
prev.charAt(prev.length() - 1) == tokenScannerSymbols.getExprStartChar()
) {
if (
cur.length() > 0 &&
(
cur.charAt(0) == tokenScannerSymbols.getTag() ||
cur.charAt(0) == tokenScannerSymbols.getExprStartChar() ||
cur.charAt(0) == tokenScannerSymbols.getNoteChar()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this could be extracted to a useful function for checking if it's the start of something?

)
) {
val.append(separator);
}
}
prev = cur;
val.append(node.getValue());
} catch (OutputTooBigException e) {
JinjavaInterpreter
.getCurrent()
.addError(TemplateError.fromOutputTooBigException(e));
return val.toString();
}
}

return val.toString();
}

private String joinNodes(LengthLimitingStringBuilder val) {
for (OutputNode node : nodes) {
try {
val.append(node.getValue());
Expand Down
53 changes: 53 additions & 0 deletions src/test/java/com/hubspot/jinjava/interpret/JinjavaInterpreterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,19 @@
import com.google.common.collect.Lists;
import com.hubspot.jinjava.Jinjava;
import com.hubspot.jinjava.JinjavaConfig;
import com.hubspot.jinjava.features.FeatureConfig;
import com.hubspot.jinjava.features.FeatureStrategies;
import com.hubspot.jinjava.interpret.JinjavaInterpreter.InterpreterScopeClosable;
import com.hubspot.jinjava.interpret.TemplateError.ErrorItem;
import com.hubspot.jinjava.interpret.TemplateError.ErrorReason;
import com.hubspot.jinjava.interpret.TemplateError.ErrorType;
import com.hubspot.jinjava.mode.EagerExecutionMode;
import com.hubspot.jinjava.mode.PreserveRawExecutionMode;
import com.hubspot.jinjava.objects.date.FormattedDate;
import com.hubspot.jinjava.objects.date.StrftimeFormatter;
import com.hubspot.jinjava.tree.TextNode;
import com.hubspot.jinjava.tree.output.BlockInfo;
import com.hubspot.jinjava.tree.output.OutputList;
import com.hubspot.jinjava.tree.parse.TextToken;
import com.hubspot.jinjava.tree.parse.TokenScannerSymbols;
import java.time.ZoneId;
Expand Down Expand Up @@ -503,4 +507,53 @@ public void itFiltersDuplicateErrors() {

assertThat(interpreter.getErrors()).containsExactly(error1, error2);
}

@Test
public void itPreventsAccidentalExpressions() {
String makeExpression = "if (true) {\n{%- print deferred -%}\n}";
String makeTag = "if (true) {\n{%- print '% print 123 %' -%}\n}";
String makeNote = "if (true) {\n{%- print '# note #' -%}\n}";
jinjava.getGlobalContext().put("deferred", DeferredValue.instance());

JinjavaInterpreter normalInterpreter = new JinjavaInterpreter(
jinjava,
jinjava.getGlobalContext(),
JinjavaConfig.newBuilder().withExecutionMode(EagerExecutionMode.instance()).build()
);
JinjavaInterpreter preventingInterpreter = new JinjavaInterpreter(
jinjava,
jinjava.getGlobalContext(),
JinjavaConfig
.newBuilder()
.withFeatureConfig(
FeatureConfig
.newBuilder()
.add(OutputList.PREVENT_ACCIDENTAL_EXPRESSIONS, FeatureStrategies.ACTIVE)
.build()
)
.withExecutionMode(EagerExecutionMode.instance())
.build()
);
JinjavaInterpreter.pushCurrent(normalInterpreter);
try {
assertThat(normalInterpreter.render(makeExpression))
.isEqualTo("if (true) {{% print deferred %}}");
assertThat(normalInterpreter.render(makeTag))
.isEqualTo("if (true) {% print 123 %}");
assertThat(normalInterpreter.render(makeNote)).isEqualTo("if (true) {# note #}");
} finally {
JinjavaInterpreter.popCurrent();
}
JinjavaInterpreter.pushCurrent(preventingInterpreter);
try {
assertThat(preventingInterpreter.render(makeExpression))
.isEqualTo("if (true) {\n" + "{#- #}{% print deferred %}}");
assertThat(preventingInterpreter.render(makeTag))
.isEqualTo("if (true) {\n" + "{#- #}% print 123 %}");
assertThat(preventingInterpreter.render(makeNote))
.isEqualTo("if (true) {\n" + "{#- #}# note #}");
} finally {
JinjavaInterpreter.popCurrent();
}
}
}