Opened 4 years ago

Closed 2 years ago

#20221 closed Bug (fixed)

`conditional_escape` does not work with lazy strings

Reported by: Baptiste Mispelon Owned by: Baptiste Mispelon
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 4 years ago by Baptiste Mispelon

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

Triage Stage: UnreviewedAccepted

comment:3 Changed 4 years ago by Baptiste Mispelon

Needs tests: set
Status: newassigned

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 4 years ago by Baptiste Mispelon

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

comment:5 Changed 2 years ago by Tim Graham

Needs tests: unset
Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

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