#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 ?
Change History (16)
comment:1 by , 13 years ago
Cc: | added |
---|
comment:2 by , 13 years ago
comment:3 by , 13 years ago
Triage Stage: | Unreviewed → 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.
comment:4 by , 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 , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 12 years ago
Keywords: | sprint2013 added |
---|
comment:8 by , 12 years ago
Patch looks fine to me, although I bikeshedded a possible improvement (in github per-line comment)
comment:9 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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 %}
r17176 added a test for this behavior.