escape Message translate params value to prevent XSS attack

[mjauvin]

Fixes #553

[bennothommo]

@mjauvin This constitutes a breaking change, as it has been this way for ages. I would just be encouraging people to use the |e filter in Twig variables.

[mjauvin]

@bennothommo this is what twig does already by default. So if you output the variable with twig directly like so {{ user.firstName }} it WILL be escaped. Shouldn't we have the same behavior here?

[LukeTowers]

@bennothommo @mjauvin I would concur. I can't think of a single valid reason that someone would want parameters passed into the | _() filter to not be escaped, unless someone was already escaping that result with | e. However, as it hasn't been documented that | _() is unaffected by Twig's default escaping I think it's acceptable to make this change in the interest of security for our wider userbase at the expense of developers relying on the current undocumented innate insecurity of | _().

[bennothommo]

@mjauvin Normal output of variables through Twig is escaped, but as soon as a filter is involved, it is passed through unescaped to the filter, and it then becomes the responsibility of the filter to escape the result as necessary. October allows filters (both in-built and pluign-provided) to run in HTML mode here: https://github.com/octobercms/october/blob/master/modules/system/classes/MarkupManager.php#L276

@LukeTowers while you are right in that it is unlikely, there are cases for HTML being added to the parameters for translation, like for example, super or sub-script characters for a title.

Eg. {{ 'Title: :title', _({title: "E = MC<sup>2</sup>"}) }}

Given that Twig is not only used for just user-provided variables, but also for default markup in plugin components where data might be provided by something already handling escaping, I don't think it's safe to just blanket escape everything.

[mjauvin]

Hmmm, interesting counter argument @bennothommo

[LukeTowers]

Hmm, I missed the fact that all custom filters registered in October through the plugin registration files bypass the escape protection provided by Twig by default. That seems a bit like a foot gun to me. What would you recommend as the solution then? Should we just update the docs (both https://octobercms.com/docs/plugin/registration#extending-twig and the plugin docs) to recommend using | e after any custom filter that you use that will have user input in it? Perhaps @daftspunk can take a look at this in a week when he's back as well.

[bennothommo]

@LukeTowers I think that's probably the only thing we can do that won't constitute a BC break for that functionality.

[mjauvin]

hmm, what about escaping only if a "<script>" tag is found in the string?

[bennothommo]

@mjauvin that might be too brittle a check - Javascript can be run in different ways, for example, through event handler attributes like onload.

[mjauvin]

@bennothommo true, so I guess we'll just leave the code as it is, and add a note to documentation.

[LukeTowers]

Still waiting for @daftspunk's input on this so we can leave it open for now.

[mjauvin]

@daftspunk did you have a chance to look into this one?

[daftspunk]

This is clearly an oversight in the original design, since escaped variables is the common/expected functionality. This change is fine to push through so long as everyone is made aware of the modified behavior, but importantly a workaround is in place for those that intend to have unescaped variables. What is the solution for these people? Is it to use |raw?

{{ 'Title: :title', _({title: "E = MC<sup>2</sup>"})|raw }}

Once a confirmed solution is in place to retain the original functionality (for the unlikely few that may want it), this can be merged and we should include appropriate guidance in the release notes.

[daftspunk]

@mjauvin Don't double down by adding more args to _, just introduce transRaw for the people that need it. We assert that nobody does (so no 4th arg needed), but just in case someone does, we are there for them

• transRaw
• transRawPlural

[multiwebinc]

Just as a comment, I encountered this issue when including links in a string, which I wouldn't think would be that uncommon in websites.

{{ 'Please go to our :contact_page.'|_({contact_page: '<b><a href="' ~ 'contact'|page ~ '">' ~ 'contact page'|_ ~ '</a></b>'}) }}

Since this is a breaking change, would it not make more sense to change it in two steps? First introduce transRaw and transRawPlural, and on a future update change the default behavior?

[LukeTowers]

@multiwebinc I don't think that would help much, but we should do a better job warning users about the change.

@mjauvin can you release a 1.7.0 !!! update that warns about the change? Just make the change to the version file in a PR and I'll review it and then update the marketplace.

[mjauvin]

@LukeTowers does it have to be version 1.7.0 ? My next milestone is 1.6.11

[LukeTowers]

@mjauvin technically it should be 2.0.1, it's a bit of a breaking change. Feel free to rename 1.6.11 to 1.7.0 and move any un-finished issues to 1.7.1. It's not quite semantic versioning but it's a bit more accurate than changing just the minor version number.

[mjauvin]

@LukeTowers do you actually want me to create a release or just a PR with the version.yaml update?

[LukeTowers]

@mjauvin just make it a new release.

[mjauvin]

@LukeTowers do you still want to approve PR #573 before I cut the release?