Code

Opened 7 years ago

Closed 7 years ago

#6041 closed (wontfix)

help_text not escaped in _html_output

Reported by: SmileyChris Owned by: nobody
Component: Forms Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

Help text should be conditionally escaped.

Attachments (0)

Change History (6)

comment:1 Changed 7 years ago by SmileyChris

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

comment:2 Changed 7 years ago by mtredinnick

This is not trivial to do correctly at the moment. The problem is, we can't just pass it to conditional_escape() because that doesn't allow for the fact that the output might not require auto-escaping (e.g. if it is going into a template inside an "autoescape off" section).

At the moment, help_text is deliberately not escaped, since it's under the control of the developer and, for newforms, (unfortunately) the developer and designer have to work closely together in any case, so agreeing on auto-escaping settings is just part of that. Might be nice to make it smoother, but it will require more than just passing in the auto-escaping setting to _as_html(); every place we return HTML from newforms will need similar treatment.

comment:3 Changed 7 years ago by SmileyChris

Maybe I'm not getting it still, but if you're using the _html_output method then you are going to get HTML output. If the help text in this HTML segment gets escaped, so what?

comment:4 Changed 7 years ago by mtredinnick

For consistency we should only autoescape when autoescaping is enabled and calling escape() does the escaping too early. Since we support not using auto-escaping in templates, those people don't need to mark strings as safe, so we'll be double-escaping. We have to live with the shackles of providing semi-backwards-compatibility if we can. So I want to think about if there's a nice solution (which will also be applied throughout newforms). If I can't think of anything credible by the weekend, I'll drop in the six line patch I wrote for this today, but I want to think about it a bit first to see if there's a better solution.

comment:5 Changed 7 years ago by SmileyChris

This isn't "auto escaping" though (in the new meaning of the word), just a change in functionality: to escape help_text (while still providing the ability to use help_text with html by using mark_safe).

I guess this is the point you're making, that it introduces the need to think about mark_safe for those trying to avoid thinking about it, but in this case it's not directly related to the template output, only changing the form's HTML output.

comment:6 Changed 7 years ago by mtredinnick

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

So, we (Jacob, myself, James and Joseph K) talked about this and decided that help_text shouldn't be escaped. It's created by developers (not by website users) and, hence, the developers can get it correct in the first place. So help_text is to be inserted verbatim.

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.