[Django] #24469: forms, form fields and media are escaped wrongfully in non django templates
Django
-------------------------+-------------------------------------------------
Reporter: MoritzS | Owner: nobody
Type: Bug | Status: new
Component: | Version: 1.8beta2
Template system | Keywords: forms fields media escape template
Severity: Normal | jinja2
Triage Stage: | Has patch: 0
Unreviewed |
Easy pickings: 0 | UI/UX: 0
-------------------------+-------------------------------------------------
Django uses `django.utils.safestring` for marking strings as escaped. This
prevents already escaped text to be escaped again.
It also uses the `__html__` magic method used by many other web
frameworks.
However the information about a string being safe won't be carried on if
an object gets converted to a string.
This mostly happens with forms, form fields an the `Media` class.
The django template backend "knows" about them so it doesn't escape them,
however that's not the case with any other backend.
For example
{{{
{{ my_form.my_field }}}
}}}
will be rendered as
{{{
<input name=&34;my_field&34; type=&34;text&34; />
}}}
when using jinja2 backend.
In my opinion the best way to fix this is to add `__html__` methods to the
classes that should not be escaped.
--
Ticket URL: <https://code.djangoproject.com/ticket/24469>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: MoritzS | Owner: MoritzS
Type: Bug | Status: assigned
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution:
Keywords: forms fields media | Triage Stage:
escape template jinja2 | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MoritzS):
* status: new => assigned
* needs_better_patch: => 0
* owner: nobody => MoritzS
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:1>
Django
Reporter: MoritzS | Owner: MoritzS
Type: Bug | Status: assigned
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution:
Keywords: forms fields media | Triage Stage:
escape template jinja2 | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Changes (by MoritzS):
* has_patch: 0 => 1
Comment:
The pull request contains a regression test and the `__html__` methods for
Form, BoundField and Media.
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:2>
Django
Reporter: MoritzS | Owner: aaugustin
Type: Bug | Status: assigned
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution:
Keywords: forms fields media | Triage Stage:
escape template jinja2 | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by aaugustin):
* owner: MoritzS => aaugustin
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:3>
Django
Reporter: MoritzS | Owner: aaugustin
Type: Bug | Status: assigned
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution:
Keywords: forms fields media | Triage Stage:
escape template jinja2 | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by MoritzS):
I looked into django's and jinja2's template code and found out what the
problem is:
The django template engine calls
`django.template.base.render_value_in_context` for each variable. There
the object gets converted to a string with `force_text`. That just calls
`__str__` or `__unicode__` of the object and correctly gets a `SafeText`.
jinja2 however doesn't use `force_text` or `str()`, it uses `escape` from
the markupsafe library.
Markupsafe then sees that the form, field or media doesn't have a
`__html__` method so it decides to mark it unsafe and escape the html
characters.
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:4>
Django
Reporter: MoritzS | Owner: aaugustin
Type: Bug | Status: assigned
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution:
Keywords: forms fields media | Triage Stage:
escape template jinja2 | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by aaugustin):
Thanks a lot for doing this research. That's the reason I suspected, but I
wasn't sure.
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:5>
Django
Reporter: MoritzS | Owner: aaugustin
Type: Bug | Status: assigned
Component: Template system | Version: 1.8beta2
Keywords: forms fields media | Triage Stage: Accepted
escape template jinja2 |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by aaugustin):
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:6>
Django
Reporter: MoritzS | Owner: aaugustin
Type: Bug | Status: assigned
Component: Template system | Version: 1.8beta2
Severity: Release blocker | Resolution:
escape template jinja2 | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
Comment:
Pending Aymeric's review.
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:7>
Django
Reporter: MoritzS | Owner: aaugustin
Type: Bug | Status: assigned
Component: Template system | Version: 1.8beta2
Severity: Release blocker | Resolution:
Keywords: forms fields media | Triage Stage: Ready for
escape template jinja2 | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by aaugustin):
I'm fine with the patch as is. Let's commit it rather than delay RC1.
Like I said on IRC, generally speaking, I'm wondering if adding this
`__html__` method on a case by case basis to some classes is the best idea
in the long run.
Can we identify every class whose `__str__` method returns a `SafeStr`? If
there's three of them, perhaps the ad-hoc method is fine. If there's
seventeen, then it's a different story.
Turning it into a mixin wouldn't save much code, would add a layer of
indirection and a small overhead, but it would also allow us to provide a
docstring.
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:8>
Django
Reporter: MoritzS | Owner: aaugustin
Type: Bug | Status: assigned
Component: Template system | Version: 1.8beta2
Severity: Release blocker | Resolution:
Keywords: forms fields media | Triage Stage: Ready for
escape template jinja2 | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"6bff3439894ac22d80f270f36513fc86586273f3" 6bff343]:
{{{
#!CommitTicketReference repository=""
revision="6bff3439894ac22d80f270f36513fc86586273f3"
Refs #24469 -- Fixed escaping of forms, fields, and media in non-Django
templates.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:9>
Django
Reporter: MoritzS | Owner: aaugustin
Type: Bug | Status: assigned
Component: Template system | Version: 1.8beta2
Severity: Release blocker | Resolution:
Keywords: forms fields media | Triage Stage: Ready for
escape template jinja2 | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"571e093a258b00b25c24481af7acf0d0a034ec8c" 571e093a]:
{{{
#!CommitTicketReference repository=""
revision="571e093a258b00b25c24481af7acf0d0a034ec8c"
[1.8.x] Refs #24469 -- Fixed escaping of forms, fields, and media in non-
Django templates.
Backport of 6bff3439894ac22d80f270f36513fc86586273f3 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: MoritzS | Owner: aaugustin
Cleanup/optimization |
Component: Template system | Version: 1.8beta2
Keywords: forms fields media | Triage Stage: Accepted
escape template jinja2 |
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* has_patch: 1 => 0
* type: Bug => Cleanup/optimization
* severity: Release blocker => Normal
* stage: Ready for checkin => Accepted
Comment:
Leaving this ticket open at Aymeric's request pending his further
investigation.
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: MoritzS | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution:
Keywords: forms fields media | Triage Stage: Accepted
escape template jinja2 |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by MoritzS):
I looked into it and found 8 more classes that return `SafeStr` in their
`__str__`:
`django.contrib.gis.maps.google.overlays.GEvent`
`django.contrib.gis.maps.google.overlays.GOverlayBase`
`django.forms.formsets.BaseFormSet`
`django.forms.utils.ErrorDict`
`django.forms.utils.ErrorList`
`django.forms.widgets.SubWidget`
`django.forms.widgets.ChoiceInput`
`django.forms.widgets.ChoiceFieldRenderer`
I agree that using a mixin would add too much overhead, what about a
decorator that adds the `__html__` method instead?
Something like:
{{{
@html_safe
class MyClass(object):
def __str__(self):
...
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: MoritzS | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution:
Keywords: forms fields media | Triage Stage: Accepted
escape template jinja2 |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by timgraham):
Sounds good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: MoritzS | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution:
Keywords: forms fields media | Triage Stage: Accepted
escape template jinja2 |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by MoritzS):
I added the decorator in a [https://github.com/django/django/pull/4347 new
PR].
I'll work on adding it to where it's needed.
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: MoritzS | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution:
Keywords: forms fields media | Triage Stage: Accepted
escape template jinja2 |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by MoritzS):
* cc: MoritzS (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: MoritzS | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution:
Keywords: forms fields media | Triage Stage: Accepted
escape template jinja2 |
Needs tests: 0 | Patch needs improvement: 0
Changes (by MoritzS):
* has_patch: 0 => 1
Comment:
I added the decorator to the affected classes.
Tests are passing on python 2.7 and 3.4 with selenium.
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: MoritzS | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution:
Keywords: forms fields media | Triage Stage: Accepted
escape template jinja2 |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Reviewed the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: MoritzS | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution:
escape template jinja2 | checkin
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
Looks good to me. Aymeric, are you happy with the approach? If so, we can
squash commits and changed "Refs" to "Fixed" in the commit message when
merging.
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: MoritzS | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution:
Keywords: forms fields media | Triage Stage: Ready for
escape template jinja2 | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by MoritzS):
I squashed the commits and tests are passing.
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: MoritzS | Owner: aaugustin
Type: | Status: assigned
Cleanup/optimization |
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution:
Keywords: forms fields media | Triage Stage: Ready for
escape template jinja2 | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by aaugustin):
I'm happy with the general approach. It looks appropriate given the number
of classes that needed the decorator.
I left two questions on the PR about the implementation of the decorator.
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: MoritzS | Owner: aaugustin
Cleanup/optimization |
Component: Template system | Version: 1.8beta2
Keywords: forms fields media | Triage Stage: Ready for
escape template jinja2 | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"1f2abf784a9fe550959de242d91963b2ad6f7e9c" 1f2abf7]:
{{{
#!CommitTicketReference repository=""
revision="1f2abf784a9fe550959de242d91963b2ad6f7e9c"
Fixed #24469 -- Refined escaping of Django's form elements in non-Django
templates.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: MoritzS | Owner: aaugustin
Type: | Status: closed
Cleanup/optimization |
Component: Template system | Version: 1.8beta2
Severity: Normal | Resolution: fixed
Keywords: forms fields media | Triage Stage: Ready for
escape template jinja2 | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"44a05a8a912596c44f37f050dcde85b45827b3b6" 44a05a8a]:
{{{
#!CommitTicketReference repository=""
revision="44a05a8a912596c44f37f050dcde85b45827b3b6"
[1.8.x] Fixed #24469 -- Refined escaping of Django's form elements in non-
Django templates.
Backport of 1f2abf784a9fe550959de242d91963b2ad6f7e9c from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24469#comment:22>