Deprecate the "escape" half of django.utils.safestring

[Aymeric Augustin]

Since any data that isn't explicitly marked as safe must be treated as unsafe, I don't understand why we have EscapeData and its subclasses nor the mark_for_escaping function.

It seems to me that we could keep only the "safe" half of django.utils.safestring and deprecate the "escape" half.

As a matter of fact the "escape" isn't used meaningfully anywhere in Django.

[Collin Anderson]

The idea is you should use conditional_escape() instead of escape() This makes sense to me.

It would be great if we could even have escape = conditional_escape, but that (might?) have the possibility of silently not escaping something?

[Aymeric Augustin]

Just thinking about an upgrade path for something as security-sensitive as escape makes my head hurt. This will not be an easy task.

[Tim Graham]

I started ​some exploratory work. I remove usage of mark_for_escaping() and EscapeData in the template engine to give an idea of what the code might look like with them removed. It wasn't difficult to get the tests passing besides a couple modifications to tests in test_force_escape.py which seem like edge cases (chained usage of escape|force_escape).

Aymeric, I didn't think through the upgrade path completely, but could elaborate on the difficulties you foresaw in that last comment if you remember them? Maybe the tests don't capture why this is tricky.

[Aymeric Augustin]

I was in the middle of a large refactor of templates when I filed this issue, which made me worry a lot about anything that looked like it could extend the scope of what I was doing.

I don't remember specific problems other than guaranteeing that this change wouldn't introduce security issues.

[Tim Graham]

Here's a problematic test case for my proof of concept:

@setup({'chaining111': '{% autoescape off %}{{ a|escape|add:"<script>" }}{% endautoescape %}'})
def test_chaining111(self):
    output = self.engine.render_to_string('chaining111', {'a': 'a < b'})
    self.assertEqual(output, 'a &lt; b&lt;script&gt;')

The documentation for the escape filter says,

The escaping is only applied when the string is output, so it does not matter where in a chained sequence of filters you put escape: it will always be applied as though it were the last filter. If you want escaping to be applied immediately, use the force_escape filter.

My implementation changes this so that the escape filter no longer executes after all other filters (it runs conditional_escape() right away). This means that the above test fails and the output is: 'a &lt; b<script>'.

The behavior of escape running last no matter its position doesn't seem so intuitive, but it could be problematic to simply change it. A way forward could be to deprecate the escape filter in favor of a new filter called conditional_escape which would simply call the function of the same name. With the new filter, template authors will get equivalent behavior to escape as long as they put this filter last.

Alternatively, we could raise a deprecation warning if the escape filter isn't last in the list of filters and then change the behavior to use conditional_escape() at the end of the deprecation period. This has the potential to be less safe for users, however, as a project might skip over the Django versions with the warnings and not realize the behavior has changed. On the other hand, I hope few users are running with autoscape off and writing code like the test.

I'll probably take this to the mailing list in a few days. In the meantime, feel free to comment with any thoughts.

[Tim Graham]

​django-developers discussion

[Tim Graham]

​PR

[Tim Graham <timograham@…>]

In bf3057d:

Refs #24046 -- Removed redundant condition in render_value_in_context()

Calling conditional_escape() on SafeData won't change it.

[Tim Graham <timograham@…>]

In 2f0e0eee:

Fixed #24046 -- Deprecated the "escape" half of utils.safestring.

[Tim Graham <timograham@…>]

In 60b095cc:

Refs #24046 -- Fixed a template test when run in reverse.

[Tim Graham <timograham@…>]

In 60ca37d:

Refs #24046 -- Removed mark_for_escaping() per deprecation timeline.