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

Should set blocks be safe by default? #490

Closed
ThiefMaster opened this issue Sep 11, 2015 · 6 comments
Closed

Should set blocks be safe by default? #490

ThiefMaster opened this issue Sep 11, 2015 · 6 comments

Comments

@ThiefMaster
Copy link
Member

Right now this testcase would fail:

    def test_set_block(self):
        env = Environment(extensions=['jinja2.ext.autoescape'],
                          autoescape=True)
        tmpl = env.from_string('{% set foo %}<br>{% endset %}{{ foo }}')
        assert tmpl.render() == '<br>'
>       assert tmpl.render() == '<br>'
E       assert '&lt;br&gt;' == '<br>'
E         - &lt;br&gt;
E         + <br>

The contents of a set block is very similar to a macro though, so I think this behavior is not what you'd expect. Especially since you easily get double escaping, as shown in an extended version of the testcase above:

    def test_set_block(self):
        env = Environment(extensions=['jinja2.ext.autoescape'],
                          autoescape=True)
        tmpl = env.from_string('{% set foo %}{{ bar }}<br>{% endset %}'
                               '{{ foo }}')
>       assert tmpl.render(bar='<hr>') == '&lt;hr&gt;<br>'
E       assert '&amp;lt;hr&amp;gt;&lt;br&gt;' == '&lt;hr&gt;<br>'
E         - &amp;lt;hr&amp;gt;&lt;br&gt;
E         + &lt;hr&gt;<br>

If I add the safe filter to my set block (added in #489) the testcase passes fine, but I think this is the most common use case and thus shouldn't require a filter.

@cboos
Copy link

cboos commented Jan 25, 2016

+1, as I also got bitten by this...

Since the doc gives this example:

{% set navigation %}
   <li><a href="/">Index</a>
   <li><a href="/downloads">Downloads</a>
{% endset %}

The navigation variable then contains the navigation HTML source.

This really makes you expect that you'll get non-escaped HTML when using it, like you'd get for a macro.

@eli-collins
Copy link

If/when this does get implemented, I think something deeper is off about the 'block set' implementation that probably needs addressing at the same time...

Over in issue #486, it was suggested to use a {% filter safe %} block as a workaround, until either support for either 486 or this issue was complete.

However, the {% filter safe %} workaround yielded a surprising (to me) result:

def test_set_block(self):
    env = Environment(extensions=['jinja2.ext.autoescape'],
                      autoescape=True)
    tmpl = env.from_string('{% set foo %}{% filter safe %}<br>{% endfilter %}'
                           '{% endset %}{{ foo }}')
    result = tmpl.render()
    assert result == '<br>', result

As of v2.8, this renders &lt;br&gt; instead of the expected value... I'm guessing somewhere the input to the block set is being forcably escaped, despite the filter returning an html-safe string?

@mitsuhiko
Copy link
Contributor

I agree it would be reasonable to change this. I think this might have been the result of an optimization.

@ThiefMaster
Copy link
Member Author

When fixing this let's make sure that in case of something like this, unsafe is still properly escaped:

{% set foo %}<div>{{ unsafe }}</div>{% endset %}

@aaugustin
Copy link

I just hit this issue as well.

My workaround:

{% set foo %} ... {% endset %}
{% set foo = foo | safe %}{# workaround for https://github.com/pallets/jinja/issues/490 #}

@aaugustin
Copy link

Thanks Armin!

cboos added a commit to cboos/trac that referenced this issue Jan 10, 2017
And make this version the minimal required version.
cboos added a commit to cboos/trac that referenced this issue Jan 27, 2017
And make this version the minimal required version.
cboos added a commit to cboos/trac that referenced this issue Jan 30, 2017
And make this version the minimal required version.
cboos added a commit to cboos/trac that referenced this issue Jan 31, 2017
And make this version the minimal required version.
cboos pushed a commit to edgewall/trac that referenced this issue Feb 1, 2017
And make this version the minimal required version.



git-svn-id: http://trac.edgewall.org/intertrac/log:/trunk@15449 af82e41b-90c4-0310-8c96-b1721e28e2e2
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants