[Django] #25872: Title attribute for related widget links lacks escaping (and breaks html)
Django
-------------------------------+--------------------
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
This issue appears when using russian locale because corresponding
translated text fragment contains quotes there. While english locale and
manu 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25872>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.9
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by a1tus):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
> This issue appears when using russian locale because corresponding
> translated text fragment contains quotes there. While english locale and
> manu 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.
New description:
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.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:1>
Django
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* version: 1.9 => 1.8
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:2>
Django
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:3>
Django
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:4>
Django
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
Probably somewhere in
https://docs.djangoproject.com/en/stable/topics/i18n/translation
/#internationalization-in-template-code
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:5>
Django
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by a1tus):
Replying to [comment:3 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:6>
Django
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by a1tus):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:7>
Django
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:8>
Django
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by a1tus):
Ok! I'll try to inspect admin templates tomorrow to find out if there some
other places that need escaping.
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:9>
Django
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by a1tus):
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> </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>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:10>
Django
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
Thanks for these thorough findings. I think we should now evaluate the
feasibility to add some (`escape`?) keyword to trans/blocktrans tags.
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:11>
Django
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by a1tus):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:12>
Django
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Should we consider this a bug in the translations for 1.9 and older and
remove the "release blocker" designation?
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:13>
Django
Reporter: a1tus | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by a1tus):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:14>
Django
Reporter: a1tus | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* type: Bug => New feature
* severity: Release blocker => Normal
Comment:
I think adding a keyword for blocktrans to force escaping would be a nice
addition.
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:15>
Django
---------------------------------+------------------------------------
Reporter: a1tus | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* version: 1.8 => master
* component: contrib.admin => Template system
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:16>
Django
---------------------------------+------------------------------------
Type: New feature | Status: new
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by emre):
* cc: hello@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:17>
Django
---------------------------------+------------------------------------
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sasha0):
* status: new => assigned
* owner: nobody => sasha0
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:18>
Django
---------------------------------+------------------------------------
Reporter: a1tus | Owner: sasha0
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by sasha0):
Replying to [comment:15 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 %}`
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:19>
Django
---------------------------------+------------------------------------
Reporter: a1tus | Owner: sasha0
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sasha0):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/6077 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:20>
Django
---------------------------------+------------------------------------
Reporter: a1tus | Owner: sasha0
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: 0 => 1
Comment:
Comments left on the pull request.
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:21>
Django
---------------------------------+------------------------------------
Reporter: a1tus | Owner: sasha0
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sasha0):
* needs_better_patch: 1 => 0
Comment:
Updated PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:22>
Django
---------------------------------+------------------------------------
Reporter: a1tus | Owner: sasha0
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:23>
Django
---------------------------------+------------------------------------
Reporter: a1tus | Owner: sasha0
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by sasha0):
* needs_better_patch: 1 => 0
Comment:
I've updated [https://github.com/django/django/pull/6077 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:24>
Django
---------------------------------+------------------------------------
Reporter: a1tus | Owner: sasha0
Type: New feature | Status: assigned
Component: Template system | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:25>
Django
---------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
* owner: Sasha Gaevsky => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/25872#comment:26>