Add a trans/blocktrans option to force HTML escaping

[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.

[Tim Graham]

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.

[Claude Paroz]

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.

[Tim Graham]

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?

[Claude Paroz]

Probably somewhere in ​https://docs.djangoproject.com/en/stable/topics/i18n/translation/#internationalization-in-template-code

[ILYA]

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.

[ILYA]

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.

[Claude Paroz]

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.

[ILYA]

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

[ILYA]

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>

[Claude Paroz]

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

[ILYA]

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.

[Tim Graham]

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

[ILYA]

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?

[Claude Paroz]

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

[Sasha Gaevsky]

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 %}

[Sasha Gaevsky]

​PR

[Claude Paroz]

Comments left on the pull request.

[Sasha Gaevsky]

Updated PR.

[Sasha Gaevsky]

I've updated ​PR.