A while ago I was working on a patch to refresh the code for the default widgets that are included with WordPress Core. One of the changes made was to replace the i18n methods currently in-place with their counterparts that escape and translate the output. This is a pretty common practice as translation files can be a potential attack vector for hackers. WordPress.com VIP will usually request that this is added to any strings being translated and it is part of the 10up best practices.
One of the comments made was that Core trusts it’s translation files. For the sake of moving the ticket forward, I reverted those addition but it does lead me to a larger discussion of why we are trusting translation files. Another conversation referenced (#30724) pretty clearly states the reasons that core strings are not escaped but I think we should re-examine that policy.
The argument that we should trust the because they can be vetted by the team is somewhat valid in my opinion however, what if a malicious script is used change the location of file that is loaded? At that point, it doesn’t matter how well-vetted the original file is, it’s been replaced. If the escaping methods were being used, then the worst that could happen is that the strings look strange – which is a lot better than malicious script tags being rendered all over the page.
Adding these functions into Core, doesn’t hurt anything. It just makes Core a bit more secure. We are already escaping data being rendered in attributes and form fields so why not translated strings as well?
Another benefit to having using these functions in Core is the education factor. Many developers ( myself included ) learn the “WordPress way” of doing things by looking at Core source code. Developers may have no idea that their translation files are a potential entry point for hackers and so by having the code they are learning from set the example, they ( hopefully ) will follow suit.
I’d love to hear your thoughts in the comments below.
This Post Has 2 Comments
I ran into this last year while working on localizing one of my plugins and realized that unescaped strings in core were a great attack method. When I asked Nacin about it at LoopConf, he told me that since core translations passed through an external checker (via https://translate.wordpress.org/) before being allowed in, there was no need to escape them in the code.
While I understand that there are systems in place to prevent malicious code via translations in core, I don’t agree that this means we don’t need to use escaping functions. Developers will (and often, should) follow the example of core standards, and not using escaping is one that is problematic. At the very least, there needs to be inline documentation explaining *why* core doesn’t escape, and direct developers to the escaping functions instead, but even that is papering over the problem.
If there is a performance hit in escaping, or the code relies on unescaped strings for some reason, then it needs to be fixed or documented in the code, not just in Trac discussions.
Morgan – I am 100% in agreement with you. As I said in the post, checking the files is great but if those files are changed maliciously then there’s really no way to protect a site. I’m sure there are areas that rely on unescaped translated strings ( I seem to remember some in the default widgets code ? ) so I expect an iterative approach – but adding escaping functions to even 10% of core is making it 10% more secure.
Thanks for the comment and hope all is well!