Opened 8 years ago

Closed 11 months ago

Last modified 11 months ago

#4991 closed Cleanup/optimization (fixed)

Emphasize XSS ramifications of help_text not being escaped

Reported by: anonymous Owned by: adrian
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@…> 8 years ago.
4991.diff (614 bytes) - added by timgraham 11 months ago.

Download all attachments as: .zip

Change History (15)

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

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from help_text not escaped in admin to contrib.admin does not escape help_text (potential XSS)
  • Triage Stage changed from Unreviewed to Ready for checkin

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

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

  • Summary changed from contrib.admin does not escape help_text (potential XSS) to 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 8 years ago by SmileyChris

  • Triage Stage changed from Ready for checkin to Design 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 8 years ago by adrian

  • Resolution set to wontfix
  • Status changed from new to closed

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 2 years ago by russellm

  • Easy pickings unset
  • Resolution wontfix deleted
  • Severity set to Normal
  • Status changed from closed to new
  • Triage Stage changed from Design decision needed to Someday/Maybe
  • Type set to 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 2 years ago by claudep

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 2 years ago by russellm

@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 18 months ago by aaugustin

  • Component changed from contrib.admin to Documentation
  • Has patch unset
  • Triage Stage changed from Someday/Maybe to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

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

Changed 11 months ago by timgraham

comment:9 Changed 11 months ago by timgraham

  • Has patch set
  • Summary changed from contrib.admin does not escape help_text to Emphasize XSS ramifications of help_text not being escaped

comment:10 Changed 11 months ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

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

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

In 5dbe2a9431dc7335359f58c58dc02e9f92c47525:

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

comment:12 Changed 11 months 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 11 months 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