Fix encoding of sanitized translation strings

[ChristophWurst]

Fixesnextcloud/calendar#1735

Before /settings/user/security showed Devices & sessions, not it shows Devices & session again.

Trick stolen from https://forum.vuejs.org/t/how-to-display-escaped-html-special-characters-the-right-way/33845/5 and https://jsfiddle.net/jamesbrndwgn/3pfmw4cd/1/

Edited by @georgehrke:
fixes#18662

[rullzer]

🙈

[georgehrke]

If I'm not completely mistaken, this will also return stuff like < instead of <, possible allowing injection of HTML.

Since this is used all over the place and not just inside Vue or Handlebar templates, I'm not sure that we can assume that it will later on be escaped.

Or am i wrong here?

[rullzer]

If I'm not completely mistaken, this will also return stuff like < instead of <, possible allowing injection of HTML.

Since this is used all over the place and not just inside Vue or Handlebar templates, I'm not sure that we can assume that it will later on be escaped.

Or am i wrong here?

Am mmm yes.
but then those will have the same issues I guess... with escaping the & again...

[ChristophWurst]

Since this is used all over the place and not just inside Vue or Handlebar templates, I'm not sure that we can assume that it will later on be escaped.

That would only be a problem for the cases where we use jQuery's .html(...), Vue's v-html or write HTML code to nodes manually, right?

[georgehrke]

That would only be a problem for the cases where we use jQuery's .html(...), Vue's v-html or write HTML code to nodes manually, right?

Yes, and I wouldn't be surprised if in some dark corner of javascript we haven't touched since 2015 we write code to nodes manually. Or I wouldn't rule out that some apps do that, since up until now translate was supposed to escape translations.

[ChristophWurst]

Yes, and I wouldn't be surprised if in some dark corner of javascript we haven't touched since 2015 we write code to nodes manually. Or I wouldn't rule out that some apps do that, since up until now translate was supposed to escape translations.

True, but up until #17123translate returned &s.

[georgehrke]

True, but up until #17123 translate returned &s.

Which most Browsers can deal with, can't they? And returning unescaped ampersands is still less of a threat than not escaping at all.

My point is just that this is a pretty severe breaking change and I'm worried about introducing XSSs or other injections.

My suggestion would be to keep the current behaviour as default and put the proposed behaviour behind an option useForTemplating (or some better name). I would also be fine with defaulting useForTemplating to true in the nextcloud-l10n package.

[rullzer]

Or we do it the stupid sumple way for now.

just do a replace of & with & of the string we get back from DOMPurify

[nickvergessen]

This looks very dangerous. I think it should be fixed in the respective place instead?

[ChristophWurst]

I think it should be fixed in the respective place instead?

Easier said than done as we're talking about translations. You might assume & is only used where it's found in the original string but you can't be sure.

[nickvergessen]

Same for ' from french translations. Just fix it once reported, instead of potentially creating security problems or breaking html were we used it intentionally

[nickvergessen]

Okay, seems I misunderstood this a bit. I thought this was a misbehaviour of the single instance here and you tried to fix it globally.
But instead the problem is that an update of DOMPurify broke all usages and automatically escapes them now.

E.g.:

console.info(t('spreed','You: {lastMessage}',{lastMessage: '<3',}))// Before 'You: &lt;3'// After 'You: &lt;3'console.info(t('spreed','You: {lastMessage}',{lastMessage: '<3',},undefined,{escape: false,}))// Before 'You: <3'// After 'You: &lt;3'

this worked before, but it doesn't work now anymore. You can see this e.g. in the last message line of talk. I'd very much like to not have to do things like: nextcloud/spreed#2815

[georgehrke]

Yet another problem caused by DomPurify: nextcloud/calendar#1915

[georgehrke]

Alternative suggestion:

• Introduce unescapedTranslate and unescapedTranslatePlural, which doesn't call the final DomPurify on the entire string:
  

  server/core/src/OC/l10n.js

  Line 125 in 6bc9969

  returnsanitize(_build(translation,vars,count))

Everywhere you know that you will use proper escaping (like in vue or handlebar templates), just use the unescaped one and let vue do it.

For most vue apps we have, changing to the unescaped version is just a matter of changing one line:
https://github.com/nextcloud/calendar/blob/master/src/main.js#L56

[nickvergessen]

That would work, although from my pov what we would want is: Purify the translations, just not the inserted parameters.

[ChristophWurst]

although from my pov what we would want is: Purify the translations

I think this is where the evil comes from. We assume that translations are put into HTML as they are. But they might be used in other APIs or put somewhere where escaping is done automatically. Hence the double escaping. IMO the translation helper should only translate. Escaping should be an explicit step.

[ChristophWurst]

Closing due to lack of progress. None of the proposed solutions were acceptable.