Opened 8 years ago

Last modified 3 years ago

#25872 new New feature

Add a trans/blocktrans option to force HTML escaping

Reported by: ILYA Owned by:
Component: Template system Version: dev
Severity: Normal Keywords:
Cc: hello@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by ILYA)

This issue appears when using russian locale because corresponding translated text fragment contains quotes there. While english locale and many others does not.

The problem is that title attributes for links in this file:
https://github.com/django/django/blob/1.9/django/contrib/admin/templates/admin/related_widget_wrapper.html
are not escaped at all. So title attribute ends before it must do and so breaks html.

I suggest to wrap blocktrans blocks with {% filter force_escape %} (already done it on my local django conf and everything worked seamless). If it's the way to go, I can make PR.

This bug affects version 1.8 too.

Change History (26)

comment:1 by ILYA, 8 years ago

Description: modified (diff)

comment:2 by Tim Graham, 8 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Version: 1.91.8

Confirmed -- seems to affect 1.8 too. Claude, can you advise on the solution?

I'm a bit surprised that results of {% blocktrans %} aren't HTML escaped but perhaps this is obvious to those more familiar with internalization? Didn't see any mention of the behavior in the docs.

comment:3 by Claude Paroz, 8 years ago

This was discussed already AFAIR. We have currently no way to tell if the markup from a translated string should be escaped or not, as it is perfectly legal to mark for translation strings having HTML markup. Until now, we simply trust translations, as if they were code. I'm open to some strategy to change this behavior if anyone has good ideas.
Until then, the workaround is to update the translation with escaped content.

comment:4 by Tim Graham, 8 years ago

So this is a case of incorrect translations (at least for now) and we should close the ticket as invalid and perhaps open a new one for ideas for improvements? Do you have a suggestion of where to document these translations rules if it's not done already?

in reply to:  3 comment:6 by ILYA, 8 years ago

Replying to claudep:

This was discussed already AFAIR. We have currently no way to tell if the markup from a translated string should be escaped or not, as it is perfectly legal to mark for translation strings having HTML markup. Until now, we simply trust translations, as if they were code. I'm open to some strategy to change this behavior if anyone has good ideas.
Until then, the workaround is to update the translation with escaped content.

I agree that in general there can be some choices but in this particular place of code (where translated string is in title attribute) there should not be any unescaped chars. So I don't see any drawbacks of escaping it in this template. In english and all locales that don't have quotes everything will remain as it is now while bug will be fixed in others.

comment:7 by ILYA, 8 years ago

I mean, in translation files we don't usually know where this string will be used. It can be used in python or printed as text/html as always or like in our case in some html attribute. So responsibility about escaping must be taken by the one who knows the context. In this particular case I think that's our template file.

comment:8 by Claude Paroz, 8 years ago

I'm not against the force_escape solution proposed in the description. But we should first audit the current Django code to see if other parts would also need it, to be consistent. Could you try that?
We can then also evaluate if adding some keyword to blocktrans to force escaping might make sense.

comment:9 by ILYA, 8 years ago

Ok! I'll try to inspect admin templates tomorrow to find out if there some other places that need escaping.

comment:10 by ILYA, 8 years ago

I'm back with my super grep investigation!

I've checked Django 1.9 templates.
In contrib.admin we have 20 usages of blocktrans tag and 88 usages of a simple trans tag.

Possibly unsafe usages of blocktrans:

# admin\change_list_results.html (1 hit)
Line 18: {% if num_sorted_fields > 1 %}<span class="sortpriority" title="{% blocktrans with priority_number=header.sort_priority %}Sorting priority: {{ priority_number }}{% endblocktrans %}">{{ header.sort_priority }}</span>{% endif %}

# admin\index.html (1 hit)
Line 20: <a href="{{ app.app_url }}" class="section" title="{% blocktrans with name=app.name %}Models in the {{ name }} application{% endblocktrans %}">{{ app.name }}</a>

# admin\related_widget_wrapper.html (3 hits)
Line 8: title="{% blocktrans %}Change selected {{ model }}{% endblocktrans %}">
Line 15: title="{% blocktrans %}Add another {{ model }}{% endblocktrans %}">
Line 22: title="{% blocktrans %}Delete selected {{ model }}{% endblocktrans %}">

... and trans:

C:\Users\alTus\Desktop\Django-1.9\django\contrib\admin\templates\admin\actions.html (4 hits)
Line 4: <button type="submit" class="button" title="{% trans "Run the selected action" %}" name="index" value="{{ action_index|default:0 }}">{% trans "Go" %}</button>
Line 4: <button type="submit" class="button" title="{% trans "Run the selected action" %}" name="index" value="{{ action_index|default:0 }}">{% trans "Go" %}</button>
Line 11: <a href="javascript:;" title="{% trans "Click here to select the objects across all pages" %}">{% blocktrans with cl.result_count as total_count %}Select all {{ total_count }} {{ module_name }}{% endblocktrans %}</a>
Line 54: <input type="submit" value="{% trans 'Change password' %}" class="default" />

C:\Users\alTus\Desktop\Django-1.9\django\contrib\admin\templates\admin\change_list_results.html (2 hits)
Line 17: <a class="sortremove" href="{{ header.url_remove }}" title="{% trans "Remove from sorting" %}"></a>
Line 19: <a href="{{ header.url_toggle }}" class="toggle {% if header.ascending %}ascending{% else %}descending{% endif %}" title="{% trans "Toggle sorting" %}"></a>

C:\Users\alTus\Desktop\Django-1.9\django\contrib\admin\templates\admin\delete_confirmation.html (5 hits)
Line 41: <input type="submit" value="{% trans "Yes, I'm sure" %}" />

C:\Users\alTus\Desktop\Django-1.9\django\contrib\admin\templates\admin\delete_selected_confirmation.html (5 hits)
Line 44: <input type="submit" value="{% trans "Yes, I'm sure" %}" />

C:\Users\alTus\Desktop\Django-1.9\django\contrib\admin\templates\admin\login.html (4 hits)
Line 61: <label>&nbsp;</label><input type="submit" value="{% trans 'Log in' %}" />

C:\Users\alTus\Desktop\Django-1.9\django\contrib\admin\templates\admin\pagination.html (2 hits)
Line 11: {% if cl.formset and cl.result_count %}<input type="submit" name="_save" class="default" value="{% trans 'Save' %}"/>{% endif %}

C:\Users\alTus\Desktop\Django-1.9\django\contrib\admin\templates\admin\related_widget_wrapper.html (3 hits)
Line 9: <img src="{% static 'admin/img/icon-changelink.svg' %}" alt="{% trans 'Change' %}"/>
Line 16: <img src="{% static 'admin/img/icon-addlink.svg' %}" alt="{% trans 'Add' %}"/>
Line 23: <img src="{% static 'admin/img/icon-deletelink.svg' %}" alt="{% trans 'Delete' %}"/>

C:\Users\alTus\Desktop\Django-1.9\django\contrib\admin\templates\admin\search_form.html (2 hits)
Line 7: <input type="submit" value="{% trans 'Search' %}" />

C:\Users\alTus\Desktop\Django-1.9\django\contrib\admin\templates\admin\submit_line.html (5 hits)
Line 3: {% if show_save %}<input type="submit" value="{% trans 'Save' %}" class="default" name="_save" />{% endif %}
Line 8: {% if show_save_as_new %}<input type="submit" value="{% trans 'Save as new' %}" name="_saveasnew" />{% endif %}
Line 9: {% if show_save_and_add_another %}<input type="submit" value="{% trans 'Save and add another' %}" name="_addanother" />{% endif %}
Line 10: {% if show_save_and_continue %}<input type="submit" value="{% trans 'Save and continue editing' %}" name="_continue" />{% endif %}

Some of the usages (like "save") seem to be quite safe but anyway I think that some solid approach in escaping translated string is needed.

Also as I've seen the method of force escaping blocktrans contents is already used in admin for js code.
For example {% filter escapejs %} in tabulars:

<script type="text/javascript">
(function($) {
  $("#{{ inline_admin_formset.formset.prefix|escapejs }}-group .inline-related").stackedFormset({
    prefix: "{{ inline_admin_formset.formset.prefix|escapejs }}",
    deleteText: "{% filter escapejs %}{% trans "Remove" %}{% endfilter %}",
    addText: "{% filter escapejs %}{% blocktrans with verbose_name=inline_admin_formset.opts.verbose_name|capfirst %}Add another {{ verbose_name }}{% endblocktrans %}{% endfilter %}"
  });
})(django.jQuery);
</script>

comment:11 by Claude Paroz, 8 years ago

Thanks for these thorough findings. I think we should now evaluate the feasibility to add some (escape?) keyword to trans/blocktrans tags.

comment:12 by ILYA, 8 years ago

There're not so much places in admin that need escaping (quite easy to wrap them all with {% filter %} in a few minutes). On the other hand it's hard to say how often django developers would use it.
I use translations quite rare in my work but anyway I think this escape attribute would be useful and will improve readability. Also introducing it can force some of us to remember about escaping our translations :)
One more thing is that it can be added, as I see, without breaking any compatibility so I don't see any downsides.

comment:13 by Tim Graham, 8 years ago

Should we consider this a bug in the translations for 1.9 and older and remove the "release blocker" designation?

comment:14 by ILYA, 8 years ago

It depends. The bug exists but it doesn't break anything hard (title attribute just ends before it should and "hides" the object name). But it can be fixed easily.

More important question is what do we need to make a decision about escape attribute for translations tags?

comment:15 by Claude Paroz, 8 years ago

Severity: Release blockerNormal
Type: BugNew feature

I think adding a keyword for blocktrans to force escaping would be a nice addition.

comment:16 by Tim Graham, 8 years ago

Component: contrib.adminTemplate system
Summary: Title attribute for related widget links lacks escaping (and breaks html)Add a blocktrans option to force HTML escaping
Version: 1.8master

comment:17 by Emre Yılmaz, 8 years ago

Cc: hello@… added

comment:18 by Sasha Gaevsky, 8 years ago

Owner: changed from nobody to Sasha Gaevsky
Status: newassigned

in reply to:  15 comment:19 by Sasha Gaevsky, 8 years ago

Replying to claudep:

I think adding a keyword for blocktrans to force escaping would be a nice addition.

Should I include force_escape parameters to necessary template tag calls in the admin, which would enable HTML escaping?

For instance, calls like this:

{% blocktrans with cl.opts.verbose_name as name %}Add {{ name }}{% endblocktrans %}

comment:20 by Sasha Gaevsky, 8 years ago

Has patch: set

comment:21 by Claude Paroz, 8 years ago

Patch needs improvement: set

Comments left on the pull request.

comment:22 by Sasha Gaevsky, 8 years ago

Patch needs improvement: unset

Updated PR.

comment:23 by Tim Graham, 8 years ago

Patch needs improvement: set

comment:24 by Sasha Gaevsky, 8 years ago

Patch needs improvement: unset

I've updated PR.

comment:25 by Tim Graham, 8 years ago

Patch needs improvement: set
Summary: Add a blocktrans option to force HTML escapingAdd a trans/blocktrans option to force HTML escaping

comment:26 by Mariusz Felisiak, 3 years ago

Owner: Sasha Gaevsky removed
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top