Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#24469 closed Cleanup/optimization (fixed)

Revisit strategy of escaping Django's form elements in non-Django forms

Reported by: Moritz Sichert Owned by: Aymeric Augustin
Component: Template system Version: 1.8beta2
Severity: Normal Keywords: forms fields media escape template jinja2
Cc: Moritz Sichert Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (22)

comment:1 Changed 7 years ago by Moritz Sichert

Owner: changed from nobody to Moritz Sichert
Status: newassigned

comment:2 Changed 7 years ago by Moritz Sichert

Has patch: set

The pull request contains a regression test and the __html__ methods for Form, BoundField and Media.

comment:3 Changed 7 years ago by Aymeric Augustin

Owner: changed from Moritz Sichert to Aymeric Augustin

comment:4 Changed 7 years ago by Moritz Sichert

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.

comment:5 Changed 7 years ago by Aymeric Augustin

Thanks a lot for doing this research. That's the reason I suspected, but I wasn't sure.

comment:6 Changed 7 years ago by Aymeric Augustin

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

comment:7 Changed 7 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

Pending Aymeric's review.

comment:8 Changed 7 years ago by Aymeric Augustin

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.

comment:9 Changed 7 years ago by Tim Graham <timograham@…>

In 6bff343:

Refs #24469 -- Fixed escaping of forms, fields, and media in non-Django templates.

comment:10 Changed 7 years ago by Tim Graham <timograham@…>

In 571e093a:

[1.8.x] Refs #24469 -- Fixed escaping of forms, fields, and media in non-Django templates.

Backport of 6bff3439894ac22d80f270f36513fc86586273f3 from master

comment:11 Changed 7 years ago by Tim Graham

Has patch: unset
Severity: Release blockerNormal
Summary: forms, form fields and media are escaped wrongfully in non django templatesRevisit strategy of escaping Django's form elements in non-Django forms
Triage Stage: Ready for checkinAccepted
Type: BugCleanup/optimization

Leaving this ticket open at Aymeric's request pending his further investigation.

comment:12 Changed 7 years ago by Moritz Sichert

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):
        ...

comment:13 Changed 7 years ago by Tim Graham

Sounds good to me.

comment:14 Changed 7 years ago by Moritz Sichert

I added the decorator in a new PR.
I'll work on adding it to where it's needed.

comment:15 Changed 7 years ago by Moritz Sichert

Cc: Moritz Sichert added

comment:16 Changed 7 years ago by Moritz Sichert

Has patch: set

I added the decorator to the affected classes.
Tests are passing on python 2.7 and 3.4 with selenium.

comment:17 Changed 7 years ago by Tim Graham

Patch needs improvement: set

Reviewed the PR.

comment:18 Changed 7 years ago by Tim Graham

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

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.

comment:19 Changed 7 years ago by Moritz Sichert

I squashed the commits and tests are passing.

comment:20 Changed 7 years ago by Aymeric Augustin

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.

comment:21 Changed 7 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 1f2abf7:

Fixed #24469 -- Refined escaping of Django's form elements in non-Django templates.

comment:22 Changed 7 years ago by Tim Graham <timograham@…>

In 44a05a8a:

[1.8.x] Fixed #24469 -- Refined escaping of Django's form elements in non-Django templates.

Backport of 1f2abf784a9fe550959de242d91963b2ad6f7e9c from master

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