Opened 16 years ago

Closed 13 years ago

Last modified 13 years ago

#6283 closed New feature (fixed)

NewForms labels are not conditionally escaped

Reported by: Paul Hummer Owned by: Paul Hummer
Component: Forms Version: dev
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: no

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 16 years ago.
Patch to fix issue 6283
bug-6283.patch (1.8 KB ) - added by Paul Hummer 16 years ago.

Download all attachments as: .zip

Change History (15)

by Paul Hummer, 16 years ago

Attachment: forms.patch added

Patch to fix issue 6283

comment:1 by Paul Hummer, 16 years ago

Owner: changed from nobody to Paul Hummer

comment:2 by Paul Hummer, 16 years ago

Status: newassigned

comment:3 by Chris Beaven, 16 years ago

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 by Chris Beaven, 16 years ago

in your form definitions I mean :)

comment:5 by Paul Hummer, 16 years ago

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 by Chris Beaven, 16 years ago

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>'))

by Paul Hummer, 16 years ago

Attachment: bug-6283.patch added

comment:7 by Paul Hummer, 16 years ago

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 by Paul Hummer, 16 years ago

Triage Stage: AcceptedDesign decision needed

comment:9 by Chris Beaven, 16 years ago

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 by Julien Phalip, 13 years ago

Severity: Normal
Type: New feature

Technically a new feature.

comment:11 by Jacob, 13 years ago

Easy pickings: set

comment:12 by anonymous, 13 years ago

Resolution: fixed
Status: assignedclosed

comment:13 by bedmondmark, 13 years ago

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