telosnetwork / telos-wallet

Native & EVM Web Wallet for transfers, staking, digital collectibles, and more
https://wallet.telos.net
Apache License 2.0
9 stars 7 forks source link

refactor i18n entries containing html to avoid XSS (telos-wallet) #710

Closed Viterbo closed 1 month ago

Viterbo commented 9 months ago

Description

Inside the i18n text file, we are using HTML tags to enrich the text but this can incur security issues. Use i18n component interpolation and/or slots where appropriate.

Viterbo commented 9 months ago

Is not that straightforward to replace all html texts for the suggested component for the following reason:

Currently, we use our own implementation of different types of notifications and all of them rely on the Quasar service Notify.create({...}). The only way to display a custom notification is to pass a rich text as a message using the HTML-flag turned on like this:

Message template:

const html = `
    <div class="c-notify__container c-notify__container--{type} c-notify__container--{random}">
        <div class="c-notify__header"></div>
        <div class="c-notify__title c-notify__title--{type}">
            <img src='{svg}' class="c-notify__icon" />
            <span>{title}</span>
        </div>
        <div class="c-notify__message">
            {message}
        </div>
        <div class="c-notify__checkbox-container c-notify__checkbox--{remember}">
            <input type="checkbox" id="c-notify__checkbox--{remember}" class="c-notify__checkbox" />
            <label for="c-notify__checkbox--{remember}" class="c-notify__label">{remember-my-choice}</label>
        </div>
    </div>
`;

Message preparation:

    const replacements = {
        '{svg}': icon,
        '{type}': type,
        '{title}': title,
        '{random}': random,
        '{remember}': remember,
        '{message}': final_message,
        '{remember-my-choice}': this.$t('notification.dont_show_message_again'),
    };

    const finalHtml = replaceAllOccurrences(html, replacements);

Showing Notification:

    Notify.create({
        timeout,
        position,
        message: finalHtml,
        html: true,
        classes: 'c-notify',
        actions,
    });

Pros: we can call this notification from whatever place on the code and it always shows. Cons: you can't use components because it does not use any html template.


What options do we have?

1 - Workaround:

we can change the current messages and replace the present html with our own reduced set of tags and include those in the replaceAllOccurrences function. e.g:

{
   "neutral_message_withdrawing": "Withdrawing <b>{quantity} {symbol}</b>",
}

Will be replaced for

{
   "neutral_message_withdrawing": "Withdrawing [bold]{quantity} {symbol}[/bold]",
}

and finally the function replaceAllOccurrences replaces those [bold] and [/bold] tags for actual <b> and </b> html tags

Pros: Straightforward and fast to implement. We also keep flexibility in the text-writing composition. Cons: We don't actually avoid the use of the html flag, which is the main cause of this issue.

2 - Refactoring

A complete refactoring of the notification system should be implemented to be able to incorporate the suggested solution using the component <i18n-t>

This is an example taken from the component site:

<div id="app">
  <!-- ... -->
  <i18n-t keypath="term" tag="label" for="tos">
    <a :href="url" target="_blank">{{ $t('tos') }}</a>
  </i18n-t>
  <!-- ... -->
</div>
const i18n = createI18n({
  locale: 'en',
  messages: {
    en: {
      tos: 'Term of Service',
      term: 'I accept xxx {0}.'
    },
    ja: {
      tos: '利用規約',
      term: '私は xxx の{0}に同意します。'
    }
  }
})

As you can see in that example, the only thing that is driven by the texts is the relative position (in this case indicated by the {0}). But the actual tag is not part of the text, it is part of a specific template placed ina specific place, that is not generic at all.

We already did something similar in the component EVMSidebarPage which is used this way but only for intercalated bold texts.

3 - Markdown I haven't explored this option yet. It might be the best of all options, but I still need to consider this idea.