Opened 17 years ago

Closed 10 years ago

Last modified 10 years ago

#4991 closed Cleanup/optimization (fixed)

Emphasize XSS ramifications of help_text not being escaped

Reported by: anonymous Owned by: Adrian Holovaty
Component: Documentation Version: dev
Severity: Normal Keywords: help_text escape
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Help text in models is not escaped in admin.
<code>
class Post(models.Model) :

title = models.CharField(maxlength=1000,null=True,blank=True,

help_text='<obvious>This is the title</obvious>')

</code>
The admin interface will not show it (and using non-closed <blink> could be fun)
I guess this is a bug or it should be documented in models that help_text must be html.
I would propose no html in models.py, (so escaping help_text and no formatting in help text).
Maybe ReST would be the best of both worlds.

Attachments (2)

contrib.diff (1.4 KB ) - added by Simon G. <dev@…> 17 years ago.
4991.diff (614 bytes ) - added by Tim Graham 10 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 by Simon G. <dev@…>, 17 years ago

Has patch: set
Summary: help_text not escaped in admincontrib.admin does not escape help_text (potential XSS)
Triage Stage: UnreviewedReady for checkin

by Simon G. <dev@…>, 17 years ago

Attachment: contrib.diff added

comment:2 by Simon G. <dev@…>, 17 years ago

Summary: contrib.admin does not escape help_text (potential XSS)contrib.admin does not escape help_text

removing the "potential xss" flag as that was alarmist of me - if someone can edit models.py, then you've got bigger issues than an XSS attack!

comment:3 by Chris Beaven, 17 years ago

Triage Stage: Ready for checkinDesign decision needed

This is, of course, slightly breaking backwards compatibility. People may be relying on putting html in help text (eg help_text='Please use the following format: <em>yyyy-mm-dd</em>')

Are we sure we don't just want to change the documentation instead?

comment:4 by Adrian Holovaty, 17 years ago

Resolution: wontfix
Status: newclosed

The help_text is intentionally unescaped, exactly for the reason SmileyChris pointed out. I've added this to the documentation in [5816].

comment:5 by Russell Keith-Magee, 11 years ago

Easy pickings: unset
Resolution: wontfix
Severity: Normal
Status: closednew
Triage Stage: Design decision neededSomeday/Maybe
Type: Uncategorized
UI/UX: unset

Although the documentation from [5816] does say help_text isn't escaped, it could be more clear that this means user-derived content shouldn't be exposed through help_text. Putting user content in help_text is an edge case, but it's a conceivable edge case (e.g., user-submitted translations), and we're departing from Django's usual "security by default" policy here, so it's worth being explicit.

Longer term, in the interests of consistency and "security by default", it might also be worth reversing this decision; we have mark_safe(), so if someone needs markup in help_text. However, there's obviously backwards compatibility concerns in doing this, so we'd need a plan for making this change.

comment:6 by Claude Paroz, 11 years ago

As far as translations are concerned, see #18208. We chose to consider translations as a safe source. It's the developper's duty to check that no suspicious code is inserted by mean of translations. So I think we should exclude that use case from the current issue, unless someone wants to reconsider #18208.

comment:7 by Russell Keith-Magee, 11 years ago

@claudep - In this case, I'm referring to a slightly different case for translations. Translations from po files are relatively safe because they need to be committed to the repository before use. I was referring to the possible case of someone storing translations in the database as runtime data, and using those as help_text. It's an edge case on the edge of an edge case, but it's possible.

comment:8 by Aymeric Augustin, 10 years ago

Component: contrib.adminDocumentation
Has patch: unset
Triage Stage: Someday/MaybeAccepted
Type: UncategorizedCleanup/optimization

If we want to document the edge case on the edge of an edge case, we can do it now ;-)

by Tim Graham, 10 years ago

Attachment: 4991.diff added

comment:9 by Tim Graham, 10 years ago

Has patch: set
Summary: contrib.admin does not escape help_textEmphasize XSS ramifications of help_text not being escaped

comment:10 by Claude Paroz, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:11 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: newclosed

In 5dbe2a9431dc7335359f58c58dc02e9f92c47525:

Fixed #4991 -- Emphasized XSS ramifications of help_text not being escaped.

comment:12 by Tim Graham <timograham@…>, 10 years ago

In c3c686b92de20b99161d1208e8aec7112213bb28:

[1.7.x] Fixed #4991 -- Emphasized XSS ramifications of help_text not being escaped.

Backport of 5dbe2a9431 from master

comment:13 by Tim Graham <timograham@…>, 10 years ago

In 190d81179f5eba6f2a1042321f0d39f04860163a:

[1.6.x] Fixed #4991 -- Emphasized XSS ramifications of help_text not being escaped.

Backport of 5dbe2a9431 from master

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