Code

Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#6283 closed New feature (fixed)

NewForms labels are not conditionally escaped

Reported by: rockstar Owned by: rockstar
Component: Forms Version: master
Severity: Normal Keywords: html escape easy-pickings
Cc: rockstar 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 rockstar 6 years ago.
Patch to fix issue 6283
bug-6283.patch (1.8 KB) - added by rockstar 6 years ago.

Download all attachments as: .zip

Change History (15)

Changed 6 years ago by rockstar

Patch to fix issue 6283

comment:1 Changed 6 years ago by rockstar

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to rockstar
  • Patch needs improvement unset

comment:2 Changed 6 years ago by rockstar

  • Status changed from new to assigned

comment:3 Changed 6 years ago by SmileyChris

  • Needs tests set
  • Patch needs improvement set
  • Summary changed from NewForms labels are not marked as safe to NewForms labels are not conditionally escaped
  • Triage Stage changed from Unreviewed to Accepted

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 6 years ago by SmileyChris

in your form definitions I mean :)

comment:5 Changed 6 years ago by rockstar

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 6 years ago by SmileyChris

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 6 years ago by rockstar

comment:7 Changed 6 years ago by rockstar

  • Cc rockstar 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 6 years ago by rockstar

  • Triage Stage changed from Accepted to Design decision needed

comment:9 Changed 6 years ago by SmileyChris

  • Keywords easy-pickings added
  • Patch needs improvement unset
  • Triage Stage changed from Design decision needed to Accepted

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 3 years ago by julien

  • Severity set to Normal
  • Type set to New feature

Technically a new feature.

comment:11 Changed 3 years ago by jacob

  • Easy pickings set

comment:12 Changed 3 years ago by anonymous

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:13 Changed 3 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).

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.