Opened 9 years ago

Closed 5 years ago

Last modified 5 years ago

#6283 closed New feature (fixed)

NewForms labels are not conditionally escaped

Reported by: Paul Hummer Owned by: Paul Hummer
Component: Forms Version: master
Severity: Normal Keywords: html escape easy-pickings
Cc: Paul Hummer Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: yes UI/UX:

Description

Consider the following code:

required = '<span class="required">*</span>'required = '<span class="required">*</span>'
class FooForm(forms.Form):
    email = forms.EmailField(label='%sEmail Address' % required)
    username = forms.CharField(label='%sUsername' % required)
    password = forms.CharField(label='%sPassword' % required, widget=forms.PasswordInput)
    password2 = forms.CharField(label='%sPassword (Again)' % required,
      widget=forms.PasswordInput)
    firstname = forms.CharField(label='First Name')
    lastname = forms.CharField(label='Last Name')

The labels are currently being escaped. Considering that the labels are usually developer/designer created instead of user created, it's probably safe to assume that most times, they are safe from XSS attacks.

Attachments (2)

forms.patch (2.8 KB) - added by Paul Hummer 9 years ago.
Patch to fix issue 6283
bug-6283.patch (1.8 KB) - added by Paul Hummer 9 years ago.

Download all attachments as: .zip

Change History (15)

Changed 9 years ago by Paul Hummer

Attachment: forms.patch added

Patch to fix issue 6283

comment:1 Changed 9 years ago by Paul Hummer

Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to Paul Hummer
Patch needs improvement: unset

comment:2 Changed 9 years ago by Paul Hummer

Status: newassigned

comment:3 Changed 9 years ago by Chris Beaven

Needs tests: set
Patch needs improvement: set
Summary: NewForms labels are not marked as safeNewForms labels are not conditionally escaped
Triage Stage: UnreviewedAccepted

The conditional_escape-ing is good, but it shouldn't be marked safe by default. People (like you) can do that in your models if you need it.

comment:4 Changed 9 years ago by Chris Beaven

in your form definitions I mean :)

comment:5 Changed 9 years ago by Paul Hummer

Wow, I guess I haven't been getting email notifications. Came back because the bug still manifests itself, and it's my fault! Submitted a new patch so that fields can now be passed a safe parameter so as not to escape the label if set to True.

For instance:

name = forms.CharField(label='<span class="foo">Name</span>', safe=True)

comment:6 Changed 9 years ago by Chris Beaven

I don't like the new patch at all.

Like I said earlier, all we need is the conditional_escape bit. Then in your code you'd just do:

name = forms.CharField(label=mark_safe('<span class="foo">Name</span>'))

Changed 9 years ago by Paul Hummer

Attachment: bug-6283.patch added

comment:7 Changed 9 years ago by Paul Hummer

Cc: Paul Hummer added

While I personally don't agree at all with the method for which this is marked safe (adding an interface to the framework instead of pulling in part of the framework to work with code seems messy to me), all I really care about is getting a fix into Django, instead of trying to get it fixed. Obviously, having markup in there is not ideal, but it's still a bug regardless.

However, I've fixed the patch to require mark_safe() in the label

comment:8 Changed 9 years ago by Paul Hummer

Triage Stage: AcceptedDesign decision needed

comment:9 Changed 9 years ago by Chris Beaven

Keywords: easy-pickings added
Patch needs improvement: unset
Triage Stage: Design decision neededAccepted

We decided to push it back to design decision after a chat on irc - but I didn't realise during the discussion that it already escaped it (the design decision was whether it should be escaped in the first place).

Latest patch is now good, but could still do with some tests.

comment:10 Changed 5 years ago by Julien Phalip

Severity: Normal
Type: New feature

Technically a new feature.

comment:11 Changed 5 years ago by Jacob

Easy pickings: set

comment:12 Changed 5 years ago by anonymous

Resolution: fixed
Status: assignedclosed

comment:13 Changed 5 years ago by bedmondmark

Sorry, I resolved this ticket as anonymous without a comment - I'm a noob :)

The changes from this patch are already in the trunk, and tests can be found in forms.py (tests_escaping() method).

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