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

{{ xx | safe }} filter is always being "applied" #1236

Closed
ang-zeyu opened this issue Jun 9, 2020 · 2 comments · Fixed by #1239
Closed

{{ xx | safe }} filter is always being "applied" #1236

ang-zeyu opened this issue Jun 9, 2020 · 2 comments · Fixed by #1239
Labels
c.Bug 🐛 s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding

Comments

@ang-zeyu
Copy link
Contributor

ang-zeyu commented Jun 9, 2020

Tell us about your environment

  • MarkBind Version: 2.14.1

What did you do? Please include the actual source code causing the issue.

// Suppose you have
<variable name="xx">
<span>... any more html ...</span>
... html
</variable>

{{ xx }}

What did you expect to happen?

Should output the escaped version of the html &lt; ... &gt; .....

What actually happened? Please include the actual, raw output.

However the unescaped version is outputted. (<span>...</span>)


This occurs because during the resolveBaseUrl stage, decodeEntities is turned on, which turns previously escaped characters back to the unescaped versions. (noticed this in an upcoming pr that inadvertedly fixes this).

Is this intended? (note that this goes against standard nunjucks behaviour)

Our docs have also been relying on this behaviour a lot, as does the 2103 site.
While it is convenient from a user standpoint, once we start adding features that pipe in data/variables from potentially unsafe sources, it poses a security problem.

On the other hand, containing html-in-variables is really quite convenient.
If we do decide to preserve the current behaviour formally (turning off autoEscape in nunjucks variable rendering), we should provide an unsafe filter of sorts

@ang-zeyu ang-zeyu added c.Bug 🐛 s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding labels Jun 9, 2020
@damithc
Copy link
Contributor

damithc commented Jun 9, 2020

The current behavior is that the adding of | safe is not needed most of the time. I'd rather choose the option that results in less work for author but for rare cases that pose security threats, can provide a safety mechanism. i.e., follow the principle 'make common things easy, rare things possible'.
What do you guys think? OK to go that route?

@ang-zeyu
Copy link
Contributor Author

ang-zeyu commented Jun 9, 2020

Yes think that's better, having to type | safe constantly would be quite inconvenient. Less work to do in an upcoming refactor as well 😅.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Bug 🐛 s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants