Implement safe filter as SafeString and handle SafeString in filters, functions and expressions by jkollmann · Pull Request #385 · HubSpot/jinjava

jkollmann · 2020-01-09T15:03:20Z

Safe filter:

Documentation

Marks the value as safe which means that in an environment with automatic escaping enabled the variable will not be escaped.

Jinja docs: http://jinja.palletsprojects.com/en/2.10.x/templates/#working-with-automatic-escaping

Approach:

Wrap String with new class SafeString when the |safe filter is applied to the value. This way we can check the type in ExpressionNode and not escape it. I overwrote toString to make it work as expected here:

jinjava/src/main/java/com/hubspot/jinjava/tree/ExpressionNode.java

Line 52 in 1667977

String result = Objects.toString(var, "");

Considerations:

Since I introduce a new type here we might need to add some implementation for this in ExpressionResolver or JinjavaInterpreterResolver. The implementation is currently only tested with |safe being used at the end of an expression.

jboulter · 2020-01-09T18:05:50Z

+      return null;
+    }
+
+    if (!(var instanceof String)) {


perhaps the friendlier thing to do here is just to return the passed in object

Joeoh · 2020-01-21T13:31:01Z

@jboulter Would you mind taking a look over my changes here

boulter · 2020-01-21T15:31:32Z

        throw new InvalidInputException(interpreter, this, InvalidReason.NUMBER_FORMAT, object.toString());
      }
    }
+    if (object instanceof SafeString) {


Perhaps you could use a Delegate pattern here and implement SafeStringFilter which takes care of this rather than manually checking for SafeStrings in every filter?

You could have the default interface implement filter, then delegate to a safeFilter method in each of these classes, unwrapping and wrapping the SafeString around it.

Sounds good, thanks!

jkollmann · 2020-01-23T17:31:33Z

  public Object filter(Object var, JinjavaInterpreter interpreter, String... args) {
-    return StringUtils.trim(Objects.toString(var));
+    if (var instanceof String) {
+      return safeFilter(StringUtils.trim(var.toString()), interpreter, args);


Should this be return StringUtils.trim(Objects.toString(var)); instead?

Why? If we know its a string, whats the need to wrap it in the call. We know we can cast or do .toString

Joeoh · 2020-01-23T17:35:00Z

@boulter This is ready for review again. Sorry about the formatting changes but there was a lot of inconsistencies between the filters.

My general approach here was to handle SafeString as an input to any function where it made sense and to preserve the SafeString as the output where it made sense.

For the output, if a filter takes a SafeString and manipulates it eg. replace it should return a safe string.
If it takes a SafeString and does an operation like md5 or len it should not return a safe string.

There is an extra case here is urlize which takes a string and converts any URLs in it to tags. If we want this to render properly when autoEscape is enabled then it needs to return a SafeString. This might not be the best solution for the general case, so let me know if you have any thoughts about this.

boulter · 2020-01-23T19:06:15Z

-    String input = Objects.toString(objectToFilter, "");
-    LengthLimitingStringBuilder builder = new LengthLimitingStringBuilder(jinjavaInterpreter.getConfig().getMaxOutputSize());
+    if (objectToFilter instanceof String) {
+      String input = Objects.toString(objectToFilter, "");


nit: unnecessary variable

boulter · 2020-01-23T19:08:47Z

@@ -1,17 +1,17 @@
 /**********************************************************************
-Copyright (c) 2014 HubSpot Inc.
+ Copyright (c) 2014 HubSpot Inc.


in general, it's nice to separate formatting changes like this out into a separate PR so make your PRs clean and easy to understand.

boulter · 2020-01-23T19:11:14Z

@@ -0,0 +1,19 @@
+package com.hubspot.jinjava.objects;
+
+public class SafeString {


could you have this extend String? That might simplify all your instanceof checks.

I wish! Unfortunately String is final so we can't. String with an extra property would have made this a lot more simple. I think if we were designing this from scratch we wouldn't use strings anywhere and instead always pass some sort of StringToken that could be safe/not safe and have some other properties.

boulter · 2020-01-23T19:12:22Z

      return calculateBigRoot(interpreter, new BigDecimal((BigInteger) object), root);
    }
-    if (object instanceof String) {
+    if (object instanceof String || object  instanceof SafeString) {


nit: extra space after object

boulter · 2020-01-23T19:13:15Z

+    this.value = value;
+  }
+
+  public String getValue() {


is this method needed?

Yes its needed to resolve the token to a string when rendering

Never-mind. Its actually not. I got mixed up with tokens.

…filters, functions and expressions (#385)" This reverts commit a6bea47.

* First draft of deferring from tag including macros * Checkstyle * Add more tests * Remove incorrect assert * Add test that checks deferring macros in depth and update DeferredValue & MacroFunction * Checkstyle * Make constructor private and overload instance method * Fixes #159: Adds dict support for DefaultFilter * removes undesired code from Filter.java * removes whitespaces from DateTimeFormatFilter.java * Fixes Style Check Errors * Removes stringArgs from DateTimeFormatFilter * Revert "Fixes #159: Adds dict support for DefaultFilter" * Changes DefaultFilter to extend AdvancedFilter * adds PyList support to ForTag * adds tests for ForTag * removes escape filter from fortag test * Fix resoncstruct end to honor trim tags * Add reconstructImage to MacroFunction * Add test for reconstructing macro with no trim tags * Whitespace fix * Implement safe filter as SafeString and handle SafeString in filters, functions and expressions (#385) * Start implementing safe filter * Remove comment about pass-through implementation * Return var if it's not instance of string instead of throwing * Add test for pass-through * Add support for SafeString to all filters which handle Strings * Remove utils for string reverse filter * Handle SafeString in truncate function * Formatting fix * Add SafeStringFilter interface * Handle safe strings in filters * rm trailing space * Formatting fixes * Move safeFilter method to Filter IF and remove SafeFilter IF * rm space * Change behaviour of Urlize filter to not always return a SafeString * Code style changes * Remove unnecessary call to safeString * Style fix * Add tests to handle Urlize string being escaped and made safe * rm hardcoded string * rm uneeded getValue * Add SafeString type as str * Handle SafeString in expressions Co-authored-by: Joe <Joeoh@users.noreply.github.com> * Fix template error line numbers (#380) * Fix line numbers * add to some more places * two levels deep test * Fix case with child interpreter * Add deprecation * Add another test * Update error messages * Fix up error messages and tests * Fix case with scopes * Add check for inherit * Put everything on the path stack * always push parent * remove callstack crud * Set back path management * Fix extend lineNo/position and keep track for each block * cleanup * Add test for extends * Add more tests * Check parent call stack for emtpy and line numbers * Fix test * Reorient line numbers when evaluating macros, make sure to pop import path off of stack * Add test for imported macros * Reorient line numbers when resolving blocks * Reorient line numbers when processing extend parents * Add tests for extends + includes * Revert "Implement safe filter as SafeString and handle SafeString in filters, functions and expressions (#385)" This reverts commit a6bea47. * Implement safe filter as SafeString and handle SafeString in filters, functions and expressions (#394) * Start implementing safe filter * Remove comment about pass-through implementation * Return var if it's not instance of string instead of throwing * Add test for pass-through * Handle safestrings and call implementor * Add tests * Handle SafeString in filter using kwargs * Handle safe strings in most simple expressions * Handle SafeString in IsUpperExp * Handle SafeString in filters. Allow overriding from within filter * Formatting fixes Co-authored-by: jkollmann <48923512+jkollmann@users.noreply.github.com> * First draft of deferring from tag including macros * Checkstyle * Add more tests * Remove incorrect assert * Add test that checks deferring macros in depth and update DeferredValue & MacroFunction * Checkstyle * Make constructor private and overload instance method * Fix resoncstruct end to honor trim tags * Add reconstructImage to MacroFunction * Add test for reconstructing macro with no trim tags * Whitespace fix * rm duplicate statement after merge Co-authored-by: Manish Devgan <manish.nsit8@gmail.com> Co-authored-by: Jeff Boulter <jeff@boulter.com> Co-authored-by: Joe <Joeoh@users.noreply.github.com> Co-authored-by: Matt Coley <matthewjcoley@gmail.com>

jkollmann added 2 commits January 9, 2020 13:54

Start implementing safe filter

1667977

Remove comment about pass-through implementation

7fe49fc

jkollmann requested review from boulter and mattcoley January 9, 2020 15:05

jboulter approved these changes Jan 9, 2020

View reviewed changes

jkollmann and others added 6 commits January 10, 2020 11:03

Return var if it's not instance of string instead of throwing

9f8a076

Add test for pass-through

2e8bcdd

Add support for SafeString to all filters which handle Strings

cb11ed4

Remove utils for string reverse filter

b637aac

Handle SafeString in truncate function

93bb78e

Formatting fix

2f07452

boulter requested changes Jan 21, 2020

View reviewed changes

Joeoh added 6 commits January 23, 2020 16:51

Add SafeStringFilter interface

b75330f

Handle safe strings in filters

9b4d844

rm trailing space

882a1df

Formatting fixes

2204dbd

Move safeFilter method to Filter IF and remove SafeFilter IF

e55ee85

rm space

6b624b9

jkollmann commented Jan 23, 2020

View reviewed changes

boulter reviewed Jan 23, 2020

View reviewed changes

Joeoh and others added 8 commits January 24, 2020 13:39

Change behaviour of Urlize filter to not always return a SafeString

8ef1752

Code style changes

b6d398f

Remove unnecessary call to safeString

7ceb66e

Style fix

2b981e7

Add tests to handle Urlize string being escaped and made safe

e265bb5

rm hardcoded string

1b4f615

rm uneeded getValue

fee9566

Add SafeString type as str

aaeeb97

Handle SafeString in expressions

4fc8284

Joeoh changed the title ~~Implement safe filter~~ Implement safe filter as SafeString and handle SafeString in filters, functions and expressions Jan 29, 2020

Joeoh merged commit a6bea47 into master Jan 29, 2020

Joeoh deleted the implement-safe-filter branch January 29, 2020 15:51

mattcoley mentioned this pull request Jan 30, 2020

Replicate string bug #392

Closed

mattcoley added a commit that referenced this pull request Jan 30, 2020

Revert "Implement safe filter as SafeString and handle SafeString in …

e28ce0f

…filters, functions and expressions (#385)" This reverts commit a6bea47.

mattcoley mentioned this pull request Jan 30, 2020

Revert "Implement safe filter as SafeString and handle SafeString in filters, functions and expressions" #393

Merged

Joeoh mentioned this pull request Feb 3, 2020

Implement safe filter as SafeString and handle SafeString in filters, functions and expressions #394

Merged

		@@ -0,0 +1,19 @@
		package com.hubspot.jinjava.objects;

		public class SafeString {

Uh oh!

Conversation

jkollmann commented Jan 9, 2020

Safe filter:

Documentation

Approach:

Considerations:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joeoh commented Jan 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joeoh commented Jan 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants