Opened 13 years ago

Closed 12 years ago

Last modified 19 months ago

#17906 closed Uncategorized (fixed)

'firstof' and 'cycle' should autoescape

Reported by: anonymous Owned by: Vladimir.Filonov
Component: Template system Version: 1.3
Severity: Normal Keywords: sprint2013
Cc: harm.verhagen+django@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

'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/

Change History (16)

comment:1 by harm, 13 years ago

Cc: harm.verhagen+django@… added

comment:2 by Aymeric Augustin, 13 years ago

r17176 added a test for this behavior.

comment:3 by Paul McMillan, 13 years ago

Triage Stage: UnreviewedAccepted

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. I don't believe the documentation note of "widgets don't escape" is a good reason to keep this behavior as-is.

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.

Last edited 13 years ago by Paul McMillan (previous) (diff)

comment:4 by Russell Keith-Magee, 13 years ago

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.

comment:5 by Vladimir.Filonov, 12 years ago

Owner: changed from nobody to Vladimir.Filonov
Status: newassigned

comment:6 by Aymeric Augustin, 12 years ago

Keywords: sprint2013 added

comment:7 by Vladimir.Filonov, 12 years ago

comment:8 by Grzegorz Nosek, 12 years ago

Patch looks fine to me, although I bikeshedded a possible improvement (in github per-line comment)

comment:9 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

Resolution: fixed
Status: assignedclosed

In f49e9a517f2fdc1d9ed7ac841ace77636cbd6747:

Fixed #17906 - Autoescaping {% cycle %} and {% firstof %} templatetags.

This commit adds "future" version of these two tags with auto-escaping
enabled.

comment:10 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

In c10ed58746d341dc83169018030b8dbe823fc4eb:

Caught warnings in the templates tests. Refs #17906.

This was missing from f49e9a517f2fdc1d9ed7ac841ace77636cbd6747.

comment:11 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

In bc787f6a3222c2d425d96dea57a1516b31677bf5:

Loaded cycle and firstof from future in built-in templates. Refs #17906.

This was missing from f49e9a517f2fdc1d9ed7ac841ace77636cbd6747.

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

In 1ea44a3abd4e58777247a095afd03dd01efdef55:

Switched {% cycle %} and {% firstof %} tags to auto-escape their variables per deprecation timeline.

refs #17906.

comment:13 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In ad3942d325fff29e78d02b454b8fec3afb3871a7:

The cycle and firstof tags no longer raise warnings.

Refs #17906.

comment:14 by Ralph Broenink, 10 years ago

Added a PR for a small docs error:

https://github.com/django/django/pull/3401

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

In 3a34e45fdbecc1f1ead0a3c2f1c01111a865710e:

Fixed firstof docs error introduced in 1ea44a; refs #17906.

comment:16 by Алексей Поклонский, 19 months ago

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 %}
Note: See TracTickets for help on using tickets.
Back to Top