Opened 9 years ago

Closed 2 years ago

Last modified 2 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: master
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@…> 9 years ago.
4991.diff (614 bytes) - added by Tim Graham 2 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 years ago by Simon G. <dev@…>

Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Summary: help_text not escaped in admincontrib.admin does not escape help_text (potential XSS)
Triage Stage: UnreviewedReady for checkin

Changed 9 years ago by Simon G. <dev@…>

Attachment: contrib.diff added

comment:2 Changed 9 years ago by Simon G. <dev@…>

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 Changed 9 years ago by Chris Beaven

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 Changed 9 years ago by Adrian Holovaty

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 Changed 3 years ago by Russell Keith-Magee

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 Changed 3 years ago by Claude Paroz

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 Changed 3 years ago by Russell Keith-Magee

@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 Changed 3 years ago by Aymeric Augustin

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

Changed 2 years ago by Tim Graham

Attachment: 4991.diff added

comment:9 Changed 2 years ago by Tim Graham

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

comment:10 Changed 2 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

comment:11 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 5dbe2a9431dc7335359f58c58dc02e9f92c47525:

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

comment:12 Changed 2 years ago by Tim Graham <timograham@…>

In c3c686b92de20b99161d1208e8aec7112213bb28:

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

Backport of 5dbe2a9431 from master

comment:13 Changed 2 years ago by Tim Graham <timograham@…>

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