[Django] #17906: 'firstof' and 'cycle' should autoescape
Django
---------------------------------+--------------------
Reporter: anonymous | Owner: nobody
Type: Uncategorized | Status: new
Component: Template system | Version: 1.3
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------
'firstof' and 'cycle' do not Autoescaping when used in a template.
My expected behavior for Django is: The results of all template tags
should be escaped unless marked safe.
Related to #10912
In the context of #10912, the current behavior is documented. I don't
think that is enough.
The current behavior is NOT a good approach. Instead of documenting such
pitt-falls, django should be safe by default.
When I manually inspect the usage of 'firstof' and 'cycle' in several
projects its almost a 100% hit with XSS vulnerable code.
Is there any reason why the current (and documented) behaviour is better
than just fixing this ?
ref: http://www.pythonsecurity.org/wiki/django/
--
Ticket URL: <https://code.djangoproject.com/ticket/17906>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Type: Uncategorized | Status: new
Component: Template system | Version: 1.3
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by harm):
* cc: harm.verhagen+django@… (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:1>
Django
Type: Uncategorized | Status: new
Component: Template system | Version: 1.3
Keywords: | Triage Stage: Unreviewed
Comment (by aaugustin):
r17176 added a test for this behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:2>
Django
Type: Uncategorized | Status: new
Component: Template system | Version: 1.3
Keywords: | Triage Stage: Accepted
Changes (by PaulM):
* stage: Unreviewed => Accepted
Comment:
The current documented behavior is unfortunate, but firmly entrenched
enough that backwards compatibility makes it very hard to just outright
change the behavior.
I too would like to see this change happen. I'm marking this ticket as
accepted, with the caveat that any solution needs to meet the standard
requirements - it's not enough to say "we must change the behavior and
break everyone's code". I'd prefer to see a solution that didn't involve
adding settings, but that may not be possible.
One backwards compatible idea to improve the situation would be to add a
warning when these widgets render strings that are not explicitly marked
safe. I'd also like to see an easier way for these widgets to optionally
escape their output - the recommended format is very clumsy. Perhaps a
first step to changing the behavior would be to add a way for template
authors to explicitly state which behavior they want. This, combined with
a warning when the behavior is not explicit, would pave the way for a
deprecation of the existing behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:3>
Django
Type: Uncategorized | Status: new
Component: Template system | Version: 1.3
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Comment (by russellm):
If the problem can be fixed with a clean implementation of the template
tag in question, we already have a way to smoothly introduce this sort of
backwards incompatible change. We have a template tag library called
"future" that contains updated implementations of core template tags; As
part of a forward compatibility move, you can put:
{{{
{% load cycle from future %}
}}}
at the top of your template, and the new behaviour will be used for the
tag. The base libraries output warnings when they are used (following the
usual Django deprecation pattern); once we've transitioned to the new
tags, the versions in the future library will be deprecated.
The {% url %} and {% ssi %} tags are in the middle of just such a
transition. If we add updated, autoescaping implementations of {% cycle %}
and {% firstof %} to the future library, we can gradually introduce new
behaviour for them, too.
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:4>
Django
Reporter: anonymous | Owner:
Type: Uncategorized | Vladimir.Filonov
Component: Template system | Status: assigned
Severity: Normal | Version: 1.3
Keywords: | Resolution:
Has patch: 0 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Vladimir.Filonov):
* owner: nobody => Vladimir.Filonov
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:5>
Django
Reporter: anonymous | Owner:
Type: Uncategorized | Vladimir.Filonov
Component: Template system | Status: assigned
Severity: Normal | Version: 1.3
Has patch: 0 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* keywords: => sprint2013
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:6>
Django
Reporter: anonymous | Owner:
Type: Uncategorized | Vladimir.Filonov
Component: Template system | Status: assigned
Severity: Normal | Version: 1.3
Keywords: sprint2013 | Resolution:
Has patch: 0 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Vladimir.Filonov):
Here is a pull request - https://github.com/django/django/pull/766
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:7>
Django
Reporter: anonymous | Owner:
Type: Uncategorized | Vladimir.Filonov
Component: Template system | Status: assigned
Severity: Normal | Version: 1.3
Keywords: sprint2013 | Resolution:
Has patch: 0 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by gnosek):
Patch looks fine to me, although I bikeshedded a possible improvement (in
github per-line comment)
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:8>
Django
Reporter: anonymous | Owner:
Type: Uncategorized | Vladimir.Filonov
Severity: Normal | Version: 1.3
Keywords: sprint2013 | Resolution: fixed
Has patch: 0 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"f49e9a517f2fdc1d9ed7ac841ace77636cbd6747"]:
{{{
#!CommitTicketReference repository=""
revision="f49e9a517f2fdc1d9ed7ac841ace77636cbd6747"
Fixed #17906 - Autoescaping {% cycle %} and {% firstof %} templatetags.
This commit adds "future" version of these two tags with auto-escaping
enabled.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:9>
Django
Reporter: anonymous | Owner:
Type: Uncategorized | Vladimir.Filonov
Component: Template system | Status: closed
Severity: Normal | Version: 1.3
Keywords: sprint2013 | Resolution: fixed
Has patch: 0 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"c10ed58746d341dc83169018030b8dbe823fc4eb"]:
{{{
#!CommitTicketReference repository=""
revision="c10ed58746d341dc83169018030b8dbe823fc4eb"
Caught warnings in the templates tests. Refs #17906.
This was missing from f49e9a517f2fdc1d9ed7ac841ace77636cbd6747.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:10>
Django
Reporter: anonymous | Owner:
Type: Uncategorized | Vladimir.Filonov
Component: Template system | Status: closed
Severity: Normal | Version: 1.3
Keywords: sprint2013 | Resolution: fixed
Has patch: 0 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"bc787f6a3222c2d425d96dea57a1516b31677bf5"]:
{{{
#!CommitTicketReference repository=""
revision="bc787f6a3222c2d425d96dea57a1516b31677bf5"
Loaded cycle and firstof from future in built-in templates. Refs #17906.
This was missing from f49e9a517f2fdc1d9ed7ac841ace77636cbd6747.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:11>
Django
Reporter: anonymous | Owner:
Type: Uncategorized | Vladimir.Filonov
Component: Template system | Status: closed
Severity: Normal | Version: 1.3
Keywords: sprint2013 | Resolution: fixed
Has patch: 0 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"1ea44a3abd4e58777247a095afd03dd01efdef55"]:
{{{
#!CommitTicketReference repository=""
revision="1ea44a3abd4e58777247a095afd03dd01efdef55"
Switched {% cycle %} and {% firstof %} tags to auto-escape their variables
per deprecation timeline.
refs #17906.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:12>
Django
Reporter: anonymous | Owner:
Type: Uncategorized | Vladimir.Filonov
Component: Template system | Status: closed
Severity: Normal | Version: 1.3
Keywords: sprint2013 | Resolution: fixed
Has patch: 0 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"ad3942d325fff29e78d02b454b8fec3afb3871a7"]:
{{{
#!CommitTicketReference repository=""
revision="ad3942d325fff29e78d02b454b8fec3afb3871a7"
The cycle and firstof tags no longer raise warnings.
Refs #17906.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:13>
Django
Reporter: anonymous | Owner:
Type: Uncategorized | Vladimir.Filonov
Component: Template system | Status: closed
Severity: Normal | Version: 1.3
Keywords: sprint2013 | Resolution: fixed
Has patch: 0 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by ralphje):
Added a PR for a small docs error:
https://github.com/django/django/pull/3401
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:14>
Django
Reporter: anonymous | Owner:
Type: Uncategorized | Vladimir.Filonov
Component: Template system | Status: closed
Severity: Normal | Version: 1.3
Keywords: sprint2013 | Resolution: fixed
Has patch: 0 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"3a34e45fdbecc1f1ead0a3c2f1c01111a865710e"]:
{{{
#!CommitTicketReference repository=""
revision="3a34e45fdbecc1f1ead0a3c2f1c01111a865710e"
Fixed firstof docs error introduced in 1ea44a; refs #17906.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:15>
Django
Reporter: anonymous | Owner:
| Vladimir.Filonov
Type: Uncategorized | Status: closed
Component: Template system | Version: 1.3
Keywords: sprint2013 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Алексей Поклонский):
It's important to note that firstof escapes only variables! not passe
string literals:
so you should use
{{{
{% filter force_escape %}
{% firstof var1 var2 var3 "<script>alert('XSS');</script>" %}
{% endfilter %}
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17906#comment:16>