[Django] #24046: Consider deprecating the "escape" half of django.utils.safestring
Django
------------------------------------------------+------------------------
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24046>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* stage: Unreviewed => Accepted
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/24046#comment:1>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
Just thinking about an upgrade path for something as security-sensitive as
`escape` makes my head hurt. This will not be an easy task.
--
Ticket URL: <https://code.djangoproject.com/ticket/24046#comment:2>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I started [https://github.com/django/django/pull/6015 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24046#comment:3>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24046#comment:4>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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 < b<script>')
}}}
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 <
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24046#comment:5>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
[https://groups.google.com/d/topic/django-
developers/cb8zyOKSdZY/discussion django-developers discussion]
--
Ticket URL: <https://code.djangoproject.com/ticket/24046#comment:6>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/6576 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/24046#comment:7>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"bf3057d10bc1e78a8e45142a8288a733b3e908a2" bf3057d]:
{{{
#!CommitTicketReference repository=""
revision="bf3057d10bc1e78a8e45142a8288a733b3e908a2"
Refs #24046 -- Removed redundant condition in render_value_in_context()
Calling conditional_escape() on SafeData won't change it.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24046#comment:8>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: Utilities | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"2f0e0eee450775a71ac3eb42707dcd970ede42c8" 2f0e0eee]:
{{{
#!CommitTicketReference repository=""
revision="2f0e0eee450775a71ac3eb42707dcd970ede42c8"
Fixed #24046 -- Deprecated the "escape" half of utils.safestring.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24046#comment:9>
Django
--------------------------------------+------------------------------------
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: Utilities | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24046#comment:10>
Django
--------------------------------------+------------------------------------
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: Utilities | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"60b095cc4c3dbf8bf41f7ce0939b0c90dbcc9b1a" 60b095cc]:
{{{
#!CommitTicketReference repository=""
revision="60b095cc4c3dbf8bf41f7ce0939b0c90dbcc9b1a"
Refs #24046 -- Fixed a template test when run in reverse.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24046#comment:11>
Django
--------------------------------------+------------------------------------
Reporter: Aymeric Augustin | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: Utilities | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"60ca37d2e56e435521a4aa5ba56b1b11cb2a78e5" 60ca37d]:
{{{
#!CommitTicketReference repository=""
revision="60ca37d2e56e435521a4aa5ba56b1b11cb2a78e5"
Refs #24046 -- Removed mark_for_escaping() per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24046#comment:12>