Code

Opened 2 years ago

Closed 14 months ago

#18208 closed Uncategorized (wontfix)

Escape translations by default

Reported by: tonnzor Owned by: nobody
Component: Internationalization Version: 1.4
Severity: Normal Keywords:
Cc: tonn81@… Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Proposal:

  1. By default treat translations (trans and blocktrans templatetags, ugettext and ungettext functions) as unsafe strings.
  2. Add safe flag to trans and blocktrans templatetags that would mark their output as safe.
  3. When using ugettext and ungettext functions -- explicitly use |safe filter to skip escaping.

Safety must be turned on by default. Noone knows how its app would be translated. Always escape it manually is not a solution for the same reasons as escaping every string used on the page.

Use-case

Translators may easily use special chars in the translations trying to make them better.

See this example:

<p>{% blocktrans %}Check Terms and Conditions or 5 is more than 4</p>
<p>Even worse: <input type="button" value="{% trans 'Do not save' %}" />

It looks fine until they translated string using special chars:

Check Terms & Conditions or 5 > 4
Don't save "{{ item }}"

By default blocktrans and trans are "safe" strings that would lead to showing translations as-is. This would break the page completely and may lead to XSS and other unpleasant stuff.

Current woraround

To escape special chars in translations you have to always apply force_escape filter manually on each translation:

<p>{% filter force_escape %}{% blocktrans %}Check Terms and Conditions or 5 is more than 4{% endfilter %}</p>
<p>Even worse: <input type="button" value="{% filter force_escape %}{% trans 'Do not save {{ item }}}' %}{% endfilter %}" />

Attachments (0)

Change History (7)

comment:1 follow-up: Changed 2 years ago by ramiro

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

See #10449. I'd go even further and say this is a duplicate.

comment:2 in reply to: ↑ 1 Changed 2 years ago by tonnzor

Replying to ramiro:

See #10449. I'd go even further and say this is a duplicate.

It is similar but covers a different case -- when translations are used in widgets and is not very clear. My issue is about direct translations usage.

Moreover -- #10449 didn't have any activity for a year, so maybe this one will have more attention being quite different case.

comment:3 follow-up: Changed 2 years ago by claudep

  • Triage Stage changed from Unreviewed to Design decision needed

Difficult issue. If on one side I understand your concern, on the other side, translations are assimilated to your own code. We can not decide to simply make all translations unsafe, because it is perfectly legal to add markup inside translatable strings. For example:

{% blocktrans %}This is <b>very</b> important{% endblocktrans%}

would then lead to content like:

C'est &lt;b&gt;très&lt;/b&gt; important

So I'm afraid the only viable thing we can do is to warn about such possible issues and recommend to check translations before putting them in production. Note that bad translations can also lead to data loss ("Delete" translated by "OK" on a button), so generally speaking, you have to trust translators anyway.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 2 years ago by tonnzor

Replying to claudep:

We can not decide to simply make all translations unsafe, because it is perfectly legal to add markup inside translatable strings.

If we have any suspicion on escaping translations -- let's make it explicit -- to escape them or not.

Then your case would look this way:

{% blocktrans safe %}This is <b>very</b> important{% endblocktrans %}
=>
This is <b>very</b> important

For cases when safe was not provided or unsafe parameter was given -- it would work the way you explained.

If it makes more sense for your -- we can treat translations as safe by default (though that was the proposal). It will be still very handy for developers to explicitly set escaping (or disabling it).

comment:5 in reply to: ↑ 4 ; follow-up: Changed 2 years ago by claudep

Replying to tonnzor:

Then your case would look this way:

{% blocktrans safe %}This is <b>very</b> important{% endblocktrans %}
=>
This is <b>very</b> important

And now your security argument is lost. How can you know that the translation will be really safe? For me, this is mere complication for a small benefit.

comment:6 in reply to: ↑ 5 Changed 2 years ago by Tonnzor

Replying to claudep:

And now your security argument is lost. How can you know that the translation will be really safe?

It is the same secure as |safe :) You can use it in specific places you trust or wrap into sandbox.

However, with translation tags we cannot use safe or escape filters so easy.

Finally, we can turn unsafe by default if you think it more reasonable.

comment:7 Changed 14 months ago by jezdez

  • Resolution set to wontfix
  • Status changed from new to closed

"Noone knows how its app would be translated"

That is not true. It's your duty as a developer to check how your translations have been created, it's part of your code base after all.

I wouldn't mind adding a note to the i18n docs about that, but disagree with adding an option to the trans/blocktrans tags. Feel free to open a separate ticket for that.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.