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.

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

Add forceescape filter #782

Closed
ArmorDarks opened this issue Jun 13, 2016 · 7 comments · Fixed by #1090 · May be fixed by dubscratch/MagicMirror#16
Closed

Add forceescape filter #782

ArmorDarks opened this issue Jun 13, 2016 · 7 comments · Fixed by #1090 · May be fixed by dubscratch/MagicMirror#16

Comments

@ArmorDarks
Copy link

Seems that escaping of caller() no longer works with switched off default auto escaping and returns raw html.

I'm not sure is it intentional, or is it a bug.

{% macro example() %}
<pre><code>{{ caller()|escape }}</code></pre>
{% endmacro %}

{% call example() %}
First line <br> second line
{% endcall %}

should return

First line &lt;br&gt; second line

but returns

First line
second line
@ArmorDarks
Copy link
Author

I think that the issue is here.

|escape expecting input to be a string, but caller() now returns Object instead of a string. I didn't dig deeply into history, but seem that in previous version of caller() returned a string, not an Object, and thus |escape worked on it

I wonder, should we change it to:

escape: function(input) {
    // Some dirty way to ensure that it is an Object,
    // and thus probably a return of caller()
    if(typeof input === 'object' && input && !Array.isArray(input)) {
        // Explicitly pass value of caller() as input
        input = input.val;
    }

    if(typeof input === 'string') {
        return r.markSafe(lib.escape(input));
    }
    return input;
},

@carljm
Copy link
Contributor

carljm commented Jun 20, 2016

Can you bisect to find the origin of the change to caller()? That's the part I'm more interested in, as I'm not aware of why or when that should have changed, and I'd expect it to return a string.

@ArmorDarks
Copy link
Author

ArmorDarks commented Jun 20, 2016

Hm, seems issue not related to caller() itself.

I've checked it on older version — caller() always returned object. However, on 2.4.1 |escape stopped to work on it.

{% macro test() %}
    test
    {{ log(caller()) }}
    {{ log(caller()|escape) }}
{% endmacro %}

{% call test() %}
1111<br>test
{% endcall %}
  • on 2.4.1:
    • logged caller():

      { [String: '\n1111<br>test\n'] val: '\n1111<br>test\n', length: 14 }
    • logged caller()|escape:

      { [String: '\n1111<br>test\n'] val: '\n1111<br>test\n', length: 14 }
  • on 2.4.0:
    • logged caller():

      { [String: '\n1111<br>test\n'] val: '\n1111<br>test\n', length: 14 }
    • logged caller()|escape:

      1111&lt;br&gt;test
  • on 2.3.0:
    • logged caller():

      { [String: '\n1111<br>test\n'] val: '\n1111<br>test\n', length: 14 }
    • logged caller()|escape:

      1111&lt;br&gt;test
  • on 2.1.0:
    • logged caller():

      { [String: '\n1111<br>test\n'] val: '\n1111<br>test\n', length: 14 }
    • logged caller()|escape:

      1111&lt;br&gt;test
  • on 1.2.0:
    • logged caller():

      { [String: '\n1111<br>test\n'] val: '\n1111<br>test\n', length: 14 }
    • logged caller()|escape:

      1111&lt;br&gt;test

After some testing it started to be clear, that those two PRs leading to those breaking changes:

Please note, that to revert old behavior, your have to revert both PRs.

So, maybe we have to change caller() return to string? Since purpose of that returned object is unclear for me.

@carljm
Copy link
Contributor

carljm commented Jun 20, 2016

I think the object you are referring to is a SafeString, meant to represent a string that is already safe for output without escaping. Caller() returns this because the output of a nunjucks macro generally can contain markup and should not be escaped (any unsafe values within it should already have been escaped when output the first time within the macro.) I'm not sure what use case you have for forcing the escaping of caller() output, but it seems really what you are requesting here is the addition of a forceescape filter to nunjucks, which will force another escaping even if the input is already a SafeString. See http://jinja.pocoo.org/docs/dev/templates/#forceescape

@carljm carljm changed the title Escaping of caller() Add forceescape filter Jun 20, 2016
@ArmorDarks
Copy link
Author

I always thought that purpose of |escape is to convert input to free from HTMl entities output. In other words, < to &lt; and so on. See here

Isn't return of caller()|escape with not escaped < and > doesn't break that purpose?

Maybe I'm missing something, but I have a feeling that Nunjucks assumes that string safe when it doesn't contain any unsafe JavaScript values, but when it comes to Jinja, it assumes under safe string a string without any HTML entities. Isn't it?

Regardless of it, if Jinja2 behaves in same way and caller()|escape doesn't escape HTML entities, like it does with regular strings, I'll be happy to go with |forceescape too.

I assume |forceescape can be same as old |escape (which allowed double escaping), right?

About what we're doing with it — we're outputting content of the macro is <code>{{ caller()|escape }}</code> to showcase CSS and HTML snippets. Same code used for rendering of preview. I provided a bit more details in that issue: #783

@carljm
Copy link
Contributor

carljm commented Jun 20, 2016

No, |escape is supposed to be smarter than that -- if you have previously declared a string safe (e.g. with the |safe filter, or by using SafeString -- equivalent is Markup in Jinja2), it avoids double-escaping. I'm quite sure I checked to be sure we were consistent with Jinja2 behavior when merging #623, but you can check again.

And no, I don't think there's a difference between how nunjucks and Jinja evaluate string safeness, but again, if you can find a counter-example please do. Neither nunjucks nor Jinja ever mark a string safe automatically based on its contents; safeness is a property derived from the source of the string (and it's purpose is specifically to declare that even though a string contains unescaped HTML markup, that markup is from a safe source, and should be rendered, not escaped). E.g. caller() marks its output safe for the reason I mentioned above: the source is itself template output, which is likely to contain markup that should be rendered as markup, not auto-escaped; any unsafe user-sourced content should have already been auto-escaped when rendered as part of the caller() template.

If you want an unconditional escaping that disregards the source context, that's what |forceescape is for.

@ArmorDarks
Copy link
Author

@carljm Thanks for explanation. Your explanation sound argumentative, but I still have some doubts (sorry! :). It would really be good to have tested this in Jinja2. Unfortunately, I can't do this myself, since I'm quite far from Python world.

@vecmezoni @jbmoelker this is quite old issue, I wonder can we do something about it. In any situation, we would need forceescape filter. The old version of |escape have enforced escaping on almost anything. I wonder, if I will make PR with older version of |escape named as |forceescape, will it do the job?

Here is PRs where can be seen code of old |escape:

https://github.com/mozilla/nunjucks/pull/701/files
https://github.com/mozilla/nunjucks/pull/623/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants