Opened 12 years ago
Closed 10 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: | dev |
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 |
Description
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)) '<foo>' >>> str(conditional_escape(conditional_escape(ss))) '&lt;foo&gt;'
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 by , 12 years ago
Has patch: | set |
---|
comment:2 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 12 years ago
Needs tests: | set |
---|---|
Status: | new → 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('&', '&').replace('<', '<').replace('>', '>').replace('"', '"').replace("'", ''') 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 by , 12 years ago
I've prepared a pull request for this ticket that include fixes for 3 other ones: https://github.com/django/django/pull/1007
comment:5 by , 10 years ago
Needs tests: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:6 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
I added some tests for
conditional_escape
and tried to make them pass.I think the
allow_lazy
decorator applied toescape
was the issue here, so I removed and added some special-casing ofPromise
objects in the function itself.Here's the branch I'm working on: https://github.com/bmispelon/django/compare/ticket-20221