Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#24046 closed Cleanup/optimization (fixed)

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

Reported by: Aymeric Augustin Owned by: nobody
Component: Utilities Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (12)

comment:1 by Collin Anderson, 9 years ago

Triage Stage: UnreviewedAccepted

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?

comment:2 by Aymeric Augustin, 9 years ago

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

comment:3 by Tim Graham, 8 years ago

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.

comment:4 by Aymeric Augustin, 8 years ago

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.

comment:5 by Tim Graham, 8 years ago

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.

comment:7 by Tim Graham, 8 years ago

Has patch: set

comment:8 by Tim Graham <timograham@…>, 8 years ago

In bf3057d:

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

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

comment:9 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: newclosed

In 2f0e0eee:

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

comment:10 by Tim Graham, 8 years ago

Summary: Consider deprecating the "escape" half of django.utils.safestringDeprecate the "escape" half of django.utils.safestring

comment:11 by Tim Graham <timograham@…>, 8 years ago

In 60b095cc:

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

comment:12 by Tim Graham <timograham@…>, 7 years ago

In 60ca37d:

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

Note: See TracTickets for help on using tickets.
Back to Top