Opened 6 years ago

Closed 5 years ago

Last modified 4 years 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


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


Change History (15)

comment:1 Changed 6 years ago by harm

Cc: harm.verhagen+django@… added

comment:2 Changed 6 years ago by Aymeric Augustin

r17176 added a test for this behavior.

comment:3 Changed 6 years ago by Paul McMillan

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 6 years ago by Paul McMillan (previous) (diff)

comment:4 Changed 6 years ago by Russell Keith-Magee

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 Changed 5 years ago by Vladimir.Filonov

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

comment:6 Changed 5 years ago by Aymeric Augustin

Keywords: sprint2013 added

comment:7 Changed 5 years ago by Vladimir.Filonov

comment:8 Changed 5 years ago by Grzegorz Nosek

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

comment:9 Changed 5 years ago by Aymeric Augustin <aymeric.augustin@…>

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

comment:10 Changed 5 years ago by Aymeric Augustin <aymeric.augustin@…>

In c10ed58746d341dc83169018030b8dbe823fc4eb:

Caught warnings in the templates tests. Refs #17906.

This was missing from f49e9a517f2fdc1d9ed7ac841ace77636cbd6747.

comment:11 Changed 5 years ago by Aymeric Augustin <aymeric.augustin@…>

In bc787f6a3222c2d425d96dea57a1516b31677bf5:

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

This was missing from f49e9a517f2fdc1d9ed7ac841ace77636cbd6747.

comment:12 Changed 4 years ago by Tim Graham <timograham@…>

In 1ea44a3abd4e58777247a095afd03dd01efdef55:

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

refs #17906.

comment:13 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

In ad3942d325fff29e78d02b454b8fec3afb3871a7:

The cycle and firstof tags no longer raise warnings.

Refs #17906.

comment:14 Changed 4 years ago by Ralph Broenink

Added a PR for a small docs error:

comment:15 Changed 4 years ago by Tim Graham <timograham@…>

In 3a34e45fdbecc1f1ead0a3c2f1c01111a865710e:

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

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