Avoid using dangerouslySetInnerHTML

[lslezak]

Problem

Using the dangerouslySetInnerHTML React feature is as the name suggests dangerous. It is pretty much similar to using eval() in the code.

• An invalid translation can break the markup completely. If a translator makes a typo and uses <b><b> instead of <b></b> then the bold markup is not finished and might affect the following text.
• It is insecure, in theory a malicious translator could put <script src="http://example.com/hack.js> tag into the translation and inject arbitrary code into the page.

Solution

• Use special characters [] in the plain text and find out the bold part of the text using code.
• If there is a typo or missing bracket then the bold text will not be displayed correctly but the wrong markup cannot affect the following text, it is limited only to that label.
• Javascript injection is not possible, the <script> tag would be escaped an displayed literately in the text. Although we review the translation pull requests it is pretty easy to overlook a <script> tag at end of a veeeery long line unless you scroll.

Testing

• Tested manually

Screenshots

After fixing the problem it still looks exactly the same.

[coveralls]

coverage: 74.819%. remained the same
when pulling faaab89 on remove_dangerouslySetInnerHTML
into 1fcc535 on master.

[dgdavid]

LGTM, thanks a lot for taking care.

[dgdavid]

LGTM, thanks a lot for taking care.

BTW, I'd like to apologize for overlooking both, the link to this PR this morning in IRC and an old comment you wrote back in September (although I had a reason for overlooking that one 🦶)

If I remember correctly, the use of dangerouslySetInnerHtml was introduced in #995, because a mix of reasons: the use of <abbr >, an overlook of your quoted comment, and the fact that we felt safe on that regard because the feeling of being in control of those translations. But yeah, look at what happened recently with the https://xzhack.com.

As a disclaimer, I have to say that I was aware of the React recommendations and I was not comfortable with the use of such attribute. I thought I had wrote about it somewhere, but I'm not able to find these comments now. I even had a few plans in mind, like

• Preparing a component for simplifying and "securing" its use by relying on DOMPurify or similar too.
• To look for something to automatically check all translations files and warning about potential security issues that it might found
• To look for something Markdown based.

But time, priorities and other stuff have been relegating them to background all the time. You know, I guess. Either way, right now your solution is better and straight forward, indeed.

So, again, thanks a lot for the PR which, along with recently merged #1149 , completely removes the use of such a dangerous prop.

And please, accept the apologies for the mentioned overlooks and/or lack of sync.

[dgdavid]

To look for something to automatically check all translations files and warning about potential security issues that it might found

Maybe, in the future, we can resume that idea but for warning us about open but not closed brackets in the translations. Although not sure if it payoff, but if possible most probably it would help to anticipate a malformed translation.

[lslezak]

But yeah, look at what happened recently with the https://xzhack.com.

Yes, that was my motivation behind this change...

• To look for something Markdown based.

I was thinking about this as well (react-markdown), but IMHO it's an overkill, we do not need full markdown support (at least in the current cases).

Maybe, in the future, we can resume that idea but for warning us about open but not closed brackets in the translations.

Probably not worth of doing that, we use this functionality at just few places and a missing bracket will cause just a cosmetic problem, not a functional or security problem.