Revisit escaping practices/policies

[asmecher]

Historically PKP software has had a fairly consistent escaping strategy. The emphasis is on safety at the time of preservation (rather than input filtering); typically there are three types of content for which filtering/escaping must be considered:

• Raw (trusted) HTML: This is presented as entered, and should only be entered by privileged users (admins and journal managers). Example: page header field in settings, where e.g. meta tags may be entered.
• Filtered HTML: This is passed through a sanitizing filter to ensure dangerous content is removed, but does contain legitimate HTML tags. Entities such as <>& are expected to be HTML-encoded if they come from source text. Example: submission abstracts.
• Plaintext: HTML tags are not expected to be treated as such; <>& literals should be translated to HTML equivalents (< et al) if content of this type is to be presented in an HTML-capable context. Example: author given names.

However, there are several problems with this approach:

• Our historical approach has required a high degree of care and consistency to prevent XSS attacks -- for example, content must be explicitly escaped in Smarty; it does not escape anything by default.
• As we move to greater use of UI toolkits (e.g. the UI library), the same presentation code needs to support all 3 cases, and this makes it harder to be consistent and to know when to apply escaping. Example: grids (old UI toolkit) and lists (new UI toolkit) will generally present content in plaintext and thus should escape that content, but may also need to support content including submission titles, which contain limited HTML.
• The addition of VueJS means new types of attacks (e.g. template injections), which are not well handled by the current escaping tools/strategy.
• We have never had a clear strategy on HTML entities in translations (whether they should include markup, and if so, how rich). This gets even more complicated when we replace variables into translated content (e.g. email templates or locale keys) which themselves may be limited HTML or plaintext.

Time to review strategies and set down some new patterns!

[asmecher]

Idea: Rather than expecting the code to know exactly what kind of content it's dealing with, can we have the content type come along with the content?

For example, the gettext translation tools support flags (we currently use the fuzzy flag to indicate content that may need re-translating). Weblate appears to support safe-html and ignore-safe-html flags, which could be used to designate "limited HTML" and "unlimited HTML" translations, respectively, with the default (no flags specified) indicating plaintext. Any code that uses the translations therefore would be informed about what kind of content it's working with

• The dev team could keep close control over the English translation's flags in order to ensure that 3rd-party translations only allow HTML as and where we've specified.
• The translation functions could be made smarter about meeting needs for both
  • the context they're being called in (do I need to generate content that's going to be in an HTML page?) and
  • the content they've been given: Is this an HTML translation to start with? In what form are the parameters I'm adding?

Questions...

• How could this work at the REST API level? (It would be pretty weird to invent something like {content: 'my string', format: 'plaintext'} everywhere we serve up a string!

[jonasraoni]

I think it's complex to filter/tag all translations with these flags, and we'll have to take safe measures anyway. I mean, if a given translation allows HTML, we still can't let it pass through unfiltered.

I like the Vue's approach, to escape by default. Perhaps we can enable the auto-escape (https://smarty-php.github.io/smarty/stable/programmers/api-variables/variable-escape-html/) on Smarty or move to another template engine (Blade?).

There are too many contexts (HTML, JavaScript value, attribute value, CSV, XML, email, Vue templates, ...), and sometimes they are chained, so I think the best we can do is to escape by default wherever we can.

[jardakotesovec]

If we have translations tagged. Than on PHP side when getting translation, it could just add html comment at the beginning <!--html-->Are you sure you want to delete <b>everything!!</b>. Or something similar to basically tag the html translations. This would be good enough to be able detect on frontend whether the content its html and apply html specific sanitisation. And to know how to interpret <>.

Certainly agree with Jonas that by default we want to fully escape html (smarty btw has that option). And html content always sanitise. Very rare exception really can be if we have options to inject some html to theme header or something similar from admin interface.

With actual content that might be more difficult to tell whether it might contain html as its not coming via one php function as translation. Do we actually have information anywhere in code/db to know which fields are html content?

[jardakotesovec]

Whats interesting, that DOMPurify manages to decide whether its tag or lesser greater character quite well.

I know that we are exploring how to make things more explicit and I would like to explore that more. But we might end up with pragmatic conclusion that moving v-html to v-allowed-html which would be using DOMPurify just works for translations.

[jardakotesovec]

And maybe there would be opportunity to piggy back on the multilingual structures we have. Something like
title: {'en': 'Hi', type:'html'}. Because one thing I realised that we might need to introduce multilingual translations for newly designed forms anyway, where you have labels, descriptions and tooltips in multiple languages.

But still early on this topic to have strong opinion.

[asmecher]

Some details for consideration from Laravel:

Laravel has a HtmlString class and Htmlable interface. These do very little but wrap arbitrary content in a class and provide a toHtml method. However, more to our purposes, they are used in Laravel's e(...) escaping helper function to distinguish between literal (string) content, which should be escaped, and HTML content that should not. Note that this does not provide XSS protection for HtmlString! If you're using it, it's assumed you're adding safety elsewhere.