Opened 3 years ago

Closed 13 months ago

#20221 closed Bug (fixed)

`conditional_escape` does not work with lazy strings

Reported by: bmispelon Owned by: bmispelon
Component: Utilities Version: master
Severity: Normal Keywords:
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


When passing the result of ugettext_lazy to conditional_escape, the result is not marked properly as safe which results in the data being escaped multiple times if conditional_escape is applied more than once.

>>> from django.utils.html import conditional_escape
>>> from django.utils.translation import ugettext_lazy
>>> s = '<foo>'
>>> ss = ugettext_lazy(s)
>>> conditional_escape(conditional_escape(s))
>>> str(conditional_escape(conditional_escape(ss)))

I ran into this issue by accident when working on #20211 where some old code had been left in and was escaping some strings twice in some cases. In that case, it was easy to work around the bug by simply removing the redundant calls to conditional_escape.

Change History (6)

comment:1 Changed 3 years ago by bmispelon

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I added some tests for conditional_escape and tried to make them pass.

I think the allow_lazy decorator applied to escape was the issue here, so I removed and added some special-casing of Promise objects in the function itself.

Here's the branch I'm working on:

comment:2 Changed 3 years ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 3 years ago by bmispelon

  • Needs tests set
  • Status changed from new to assigned

The more general problem is that functions marking their output as safe using mark_safe are not compatible with allow_lazy.

I've identified two other functions that have this issue: utils.html.escapejs and utils.text.slugify.

I've checked all functions using allow_lazy and only those three are combined with mark_safe.

It's actually quite simple to solve this while maintaining backwards-compatibility (as long as you weren't relying on the bug itself, that is), because the machinery is there to mark lazy strings as safe.

To solve this, I propose the introduction of a mark_safe_dec decorator (with a better name hopefully) that wraps the output of a function in a mark_safe call.

With that, one can then decorate the lazified function, like so:

def escape(text):
    Returns the given text with ampersands, quotes and angle brackets encoded for use in HTML.
    return force_text(text).replace('&', '&amp;').replace('<', '&lt;').replace('>', '&gt;').replace('"', '&quot;').replace("'", '&#39;')
escape = mark_safe(allow_lazy(escape, six.text_type))

Note the order of the two decorators, it's important (doing it the other way leads to the behavior we have now).

This passes the tests I added with this ticket.

I'll add more tests for the two other utils functions then submit a pull request within the next few days.

comment:4 Changed 3 years ago by bmispelon

I've prepared a pull request for this ticket that include fixes for 3 other ones:

comment:5 Changed 13 months ago by timgraham

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

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

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

In 54e695331b07a572e0f37085725d23df69980628:

Fixed #20221 -- Allowed some functions that use mark_safe() to result in SafeText.

Thanks Baptiste Mispelon for the report.

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