Code

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#6160 closed (fixed)

Escaping of validation errors

Reported by: Petr Marhoun <petr.marhoun@…> Owned by: kmtracey
Component: Forms Version: master
Severity: Keywords: easy-pickings
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Error classes from django.newforms.utils.py don't escape error messages:

class ErrorList(list, StrAndUnicode):

    def as_ul(self):
        if not self: return u''
        return mark_safe(u'<ul class="errorlist">%s</ul>'
                % ''.join([u'<li>%s</li>' % force_unicode(e) for e in self]))

I don't thing that author of validation error (or translator) should use html escaping - message can be used in another context. But utility method as_ul is for html so I think it should escape it.

I am not sure if my patch is the right solution but it works for me.

Attachments (6)

00-validation-error-escape.diff (510 bytes) - added by Petr Marhoun <petr.marhoun@…> 7 years ago.
validation-escaping.diff (7.7 KB) - added by Petr Marhoun <petr.marhoun@…> 6 years ago.
validation-escaping.2.diff (8.0 KB) - added by Petr Marhoun <petr.marhoun@…> 6 years ago.
validation-escaping.3.diff (9.2 KB) - added by Petr Marhoun <petr.marhoun@…> 6 years ago.
validation-escaping.4.diff (9.7 KB) - added by Petr Marhoun <petr.marhoun@…> 6 years ago.
validation-escaping.5.diff (4.9 KB) - added by Petr Marhoun <petr.marhoun@…> 6 years ago.

Download all attachments as: .zip

Change History (16)

Changed 7 years ago by Petr Marhoun <petr.marhoun@…>

comment:1 Changed 7 years ago by gwilson

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

Should probably be using conditional_escape() instead of escape() here so that it won't get double-escaped if the developer already escaped the text.

comment:2 Changed 6 years ago by SmileyChris

  • Keywords easy-pickings added

Changed 6 years ago by Petr Marhoun <petr.marhoun@…>

comment:3 Changed 6 years ago by Petr Marhoun <petr.marhoun@…>

It is not so simple as it seemed to be:

  • There are more problems in django.newforms.utils.
  • There are some interactions between ErrorList and ErrorDict.
  • There are some interactions between ErrorList and BaseForm.

The new patch should solve it and its tests should prove it.

Changed 6 years ago by Petr Marhoun <petr.marhoun@…>

comment:4 Changed 6 years ago by Petr Marhoun <petr.marhoun@…>

New version of patch does not remove escape in _html_output - it only replaces it with conditional_escape. I think it is safer.

Changed 6 years ago by Petr Marhoun <petr.marhoun@…>

Changed 6 years ago by Petr Marhoun <petr.marhoun@…>

Changed 6 years ago by Petr Marhoun <petr.marhoun@…>

comment:5 Changed 6 years ago by Petr Marhoun <petr.marhoun@…>

  • Needs tests unset
  • Patch needs improvement unset

I thing the last version of patch has sensible tests (the previous one was quite strange for a validation error) and also quite similar solution as [8601] (Allow safe-strings in the "attrs" parameter to form widgets). So I changed "Needs tests" and "Patch needs improvement".

comment:6 Changed 6 years ago by guettli

  • Cc hv@… added

I installed validation-escaping.5.diff today and it works the way it should. The unittest "./runtests.py forms" pass. I think it is ready for checkin.

comment:7 Changed 6 years ago by kmtracey

  • Owner changed from nobody to kmtracey
  • Status changed from new to assigned

comment:8 Changed 6 years ago by kmtracey

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

(In [9365]) Fixed #6160, #9111 -- Consistently apply conditional_escape to form errors and labels when outputing them as HTML.

comment:9 Changed 6 years ago by kmtracey

(In [9366]) [1.0.X] Fixed #6160, #9111 -- Consistently apply conditional_escape to form errors and labels when outputing them as HTML.

[9365] from trunk.

comment:10 Changed 6 years ago by guettli

  • Cc hv@… removed

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.