Opened 3 years ago

Closed 2 years ago

#33631 closed Bug (fixed)

Blocktranslate asvar escapes variables, but stores the result as str instance, leading to double escaping

Reported by: Richard Ebeling Owned by: Cheng Yuan
Component: Uncategorized Version: 4.0
Severity: Normal Keywords: blocktranslate asvar 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

In the docs, this snippet is given as an example usage of blocktranslate with the asvar argument (here: https://docs.djangoproject.com/en/4.0/topics/i18n/translation/#blocktranslate-template-tag:

{% blocktranslate asvar the_title %}The title is {{ title }}.{% endblocktranslate %}
<title>{{ the_title }}</title>
<meta name="description" content="{{ the_title }}">

However, this template is buggy when title is a string, which I'd argue is a common use case.

title will be escaped when formatting the content of the blocktranslate block, but the "was escaped" information is discarded, and the_title will be a str instance with escaped content.
When later using the the_title variable, it will be conditionally escaped. Since it is a str, it will be escaped, so control characters are escaped again, breaking their display on the final page.

Minimal example to reproduce (can be put in any view):

    from django.template import Template, Context
    template_content = """
{% blocktranslate asvar the_title %}The title is {{ title }}.{% endblocktranslate %}
<title>{{ the_title }}</title>
<meta name="description" content="{{ the_title }}">
"""
    rendered = Template(template_content).render(Context({"title": "<>& Title"}))
    assert "&amp;lt;" not in rendered, "> was escaped two times"

I'd argue that blocktranslate should:

  • Either assign a SafeString instance to prevent future escaping
  • or not escape the variables used within the translation, and store them marked as unsafe (= as str instance)

Change History (7)

comment:1 by Carlton Gibson, 3 years ago

Resolution: wontfix
Status: newclosed

Hi Richard, thanks for the report.

So this would be the way forward:

... assign a SafeString instance to prevent future escaping

But it's not at all clear how feasible it would be to correctly mark the returned string as safe. Individual variables are run via render_value_in_context() which escapes them assuming autoescape is enabled, but then the final output is constructed after that, so it's not clear we can reliably mark it safe.

Rather if, in your example, you know the_title is safe, declare it as so: {{ the_title|safe }}. The following test case passes:

diff --git a/tests/template_tests/syntax_tests/i18n/test_blocktranslate.py b/tests/template_tests/syntax_tests/i18n/test_blocktranslate.py
index 4a162362c6..967a7c1829 100644
--- a/tests/template_tests/syntax_tests/i18n/test_blocktranslate.py
+++ b/tests/template_tests/syntax_tests/i18n/test_blocktranslate.py
@@ -388,6 +388,23 @@ class I18nBlockTransTagTests(SimpleTestCase):
             output = self.engine.render_to_string("i18n39")
         self.assertEqual(output, ">Seite nicht gefunden<")
 
+    @setup(
+        {
+            "issue33631": (
+                """
+                {% load i18n %}
+                {% blocktranslate asvar the_title %}The title is {{title}}.{% endblocktranslate %}
+                < title > {{the_title|safe}} < / title >
+                < meta name="description" content="{{ the_title|safe }}" >
+                """
+            )
+        }
+    )
+    def test_issue33631(self):
+        with translation.override("en"):
+            output = self.engine.render_to_string("issue33631", {"title": "<>& Title"})
+        self.assertNotIn("&amp;lt;", output)
+
     @setup(
         {

... and it avoids trying to resolve the difficulty above.

As such, I'm going to say wontfix here initially, and ask that you follow-up on the Internationalization Topics section of the Django Forum to get a wider audience if you'd like to discuss it further.

Thanks.

comment:2 by Carlton Gibson, 3 years ago

Resolution: wontfix
Status: closednew
Triage Stage: UnreviewedAccepted

Reopening to assess a patch based on forum discussion.

comment:3 by Cheng Yuan, 2 years ago

Owner: changed from nobody to Cheng Yuan
Status: newassigned

comment:5 by Mariusz Felisiak, 2 years ago

Needs documentation: set

comment:6 by Mariusz Felisiak, 2 years ago

Needs documentation: unset
Triage Stage: AcceptedReady for checkin

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In d4c5d2b5:

Fixed #33631 -- Marked {% blocktranslate asvar %} result as HTML safe.

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