Code

Opened 2 years ago

Closed 14 months ago

Last modified 5 weeks 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/

Attachments (0)

Change History (13)

comment:1 Changed 2 years ago by harm

  • Cc harm.verhagen+django@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 2 years ago by aaugustin

r17176 added a test for this behavior.

comment:3 Changed 2 years ago by PaulM

  • Triage Stage changed from Unreviewed to Accepted

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 2 years ago by PaulM (previous) (diff)

comment:4 Changed 2 years ago 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.

comment:5 Changed 14 months ago by Vladimir.Filonov

  • Owner changed from nobody to Vladimir.Filonov
  • Status changed from new to assigned

comment:6 Changed 14 months ago by aaugustin

  • Keywords sprint2013 added

comment:7 Changed 14 months ago by Vladimir.Filonov

comment:8 Changed 14 months ago by gnosek

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

comment:9 Changed 14 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In f49e9a517f2fdc1d9ed7ac841ace77636cbd6747:

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

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

comment:10 Changed 14 months ago by Aymeric Augustin <aymeric.augustin@…>

In c10ed58746d341dc83169018030b8dbe823fc4eb:

Caught warnings in the templates tests. Refs #17906.

This was missing from f49e9a517f2fdc1d9ed7ac841ace77636cbd6747.

comment:11 Changed 14 months 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 5 weeks 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 5 weeks ago by Aymeric Augustin <aymeric.augustin@…>

In ad3942d325fff29e78d02b454b8fec3afb3871a7:

The cycle and firstof tags no longer raise warnings.

Refs #17906.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.