Opened 8 years ago

Closed 7 years ago

#6529 closed (duplicate)

flatattr() doesn't check if a string is marked as "safe"

Reported by: Martin Geber Owned by: nobody
Component: Forms Version: master
Severity: Keywords:
Cc: martin@…, antisvin@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


In file in line 13:

It might be better to use conditional_escape instead of escape ( line 30, and 35)

I'm not able to set the attribute onchange in a form widget with JavaScript.


    if 1 == '1' {return false;}


    if 1 == '1' {return false;}

This is kind of annoying.

Hope my explanation is understandable... :)


Attachments (2)

escape_conditional_for_attrs.patch (859 bytes) - added by antisvin 7 years ago.
flatatt_with_conditional_escape.diff (1.5 KB) - added by versae 7 years ago.
Patch file and regression test.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 8 years ago by guettli

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to worksforme
  • Status changed from new to closed

Yes, your explanation is understandable.

from django import newforms as forms
class MyForm(forms.Form):
 query=forms.CharField(widget=forms.TextInput(attrs={'onchange': '''alert('abc')'''}))

html result

<input onchange="alert(&#39;abc&#39;)"

As far as I know this is correct. It worked in IE6.0 and Firefox.

Please provide more information and reopen it, if you still think this is a bug in django.

comment:2 Changed 8 years ago by ubernostrum

(and you can place strings with HTML meaning, such as JS, here, you just have to use mark_safe() to do so)

comment:3 Changed 7 years ago by antisvin

  • Cc antisvin@… added
  • Has patch set
  • Resolution worksforme deleted
  • Status changed from closed to reopened

Original author didn't reopen the ticket, but I'll have to do it as I've just got bitten by this.

Note that what ubernostrum suggested doesn't work because flatatt function makes a call to escape instead of conditional_escape - it's the point of this ticket to make it possible to turn off autoescaping for attrs. I'm not aware of any reason for current behaviour, so likely it's accidental.

I'm also attaching an obvious patch just in case.

Changed 7 years ago by antisvin

comment:4 Changed 7 years ago by guettli

  • Triage Stage changed from Unreviewed to Ready for checkin

I think this patch can be committed to SVN.

comment:5 Changed 7 years ago by russellm

  • Needs tests set
  • Triage Stage changed from Ready for checkin to Accepted

guettli - thanks for the enthusiasm, but this patch is missing one essential thing - a regression test case. regressiontests/forms already contains some flatatt tests; they need to be expanded.

Changed 7 years ago by versae

Patch file and regression test.

comment:6 Changed 7 years ago by versae

  • Needs tests unset

comment:7 Changed 7 years ago by msaelices

  • Resolution set to duplicate
  • Status changed from reopened to closed

I mark this ticket as duplicated of #8566. The changeset [8601] is similar to this ticket patch.

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