Prevent errors from `printf()` and `sprintf()` calls

[grapestain]

I've just faced a strange issue today. At least it was strange at first. I've just updated a Canadian English site from WP v6.6.2 to v6.7.1 and when the redirect to the wp-admin/about.php page happened the page threw a Fatal error:

Fatal error: Uncaught Error: 3 arguments are required, 2 given
in /home/happyeco/public_html/wp-admin/about.php on line 260

Looking into it further I've found that the en_CA translated string for the given printf call contained two placeholders while the PHP source string only had one and only passed one argument.

So okay, that is an issue with the translation itself, but that is fairly simple to fix and I think I'll just go to the translations site and fix it there. Meanwhile I've monkey patched my about.php to pass an extra argument to prevent fatal errors.

But I'm opening this issue to address the problem in a broader sense.

Since PHP v8.0 fprintf(), sprintf() and similar functions no longer emit a warning but they throw an error if the passed number of arguments are less than the number of placeholders in the format specifier string. E.g.: ​https://www.php.net/manual/en/function.fprintf.php#refsect1-function.fprintf-changelog

What this means to me is that sites can be crashed by translation mistakes, which seem to be an issue to me. Just think about it how bad it'd look if somehow someone added one extra format specifier to one of a widely used string of a popular language's translations. WordPress sites could suddenly start to crash in masse. I think the whole platform is sitting on a ticking bomb with this PHP change.

For me it seems to be quite obvious that a wrong/missing/incorrect UI string is way better than a crashed site.

And this means to me that WordPress should implement wrapper functions for fprintf(), sprintf() and similar function (not sure which ones are used in the codebase) that implement proper error handling preventing crashes and instead omitting warnings or log entries. All calls to fprintf(), sprintf() should be replaced with calls to the wrapper functions and plugin and theme authors should also be encouraged to replace usages in their codebase.

What do you think?

I assume such large scale change should pass several checks to happen, but in theory it is a pretty low risk change in terms of the complexity of the refactoring. Basically it is just adding few new functions and doing search-n-replace on the rest of the codebase.

[swissspidy]

While this may be tempting, hiding errors is dangerous, especially when plugin developers start using these functions as well and then don't care about their code quality.

The general rule is that WordPress trusts the translations, which is why they aren't escaped either. To mitigate such cases, GlotPress generally adds warnings if it encounters things like missing placeholders or unbalanced HTML tags. IMO this is the preferred route.

[grapestain]

Well, I'm definitely not suggesting hiding errors, but fatal errors may be still a bit too harsh. More like calling a wp_trigger_error() instead with a warning level.

I'm just thinking this need to be reconsidered because obviously any decision regarding the translation system was made before PHP v8.0, so there's a chance that assumptions made back then would not hold up any more.

But I get it what you mean, like if a sneaky translator can add a chunk of JavaScript to e.g. trigger an XSS why would WP not trust the translations for correctness? One argument for that is adding an attack vector to a translation cannot happen by accident, but crashing thousands of WP sites by adding an extra placeholder can happen by honest mistake, so they are not the same.

In other words when you say trust there's two aspect to that: security and quality. And while the circumstances for evaluating the trust for security are probably the same as when the decision was originally made, the circumstances regarding the trust for quality are significantly different prior and from PHP v8.0, since what used to be an innocent warning is not a fatal error.

Obviously my case was rather mundane as the about page is not mission critical, but the same could happen for any customer facing page, like a checkout page for a popular webshop engine or the login page for the admin UI. Such issues would hurt WordPress's reputation on top of causing large scale stress and financial losses.

[swissspidy]

There was a suggestion on Slack to simply ensure that GlotPress won't export such strings with incorrect placeholders. I think that addresses the problem quite nicely.

[grapestain]

Alright. Thank you!

[tobifjellner]

For what it's worth, I've updated that string translation, an updated language pack should happen soon-ish.

[dd32]

Replying to swissspidy:

There was a suggestion on Slack to simply ensure that GlotPress won't export such strings with incorrect placeholders. I think that addresses the problem quite nicely.

There's a warning in GlotPress for when there's a mismatch in placeholders, but every release cycle we find someone has seen the warning and discarded it :)

Currently we don't "track" strings that are in a printf() style function, we just assume that anything that has "printf-like" placeholders is a printf string, which for the most part works, but there's the possibility of false-positives.

This is likely just a suggestion that the translation system needs to be far more strict about that warning, and likely not allowing it to be discarded at all.

[swissspidy]

I know there are warnings, that's where the ​discussion started.

Warnings and strict warnings are nice. But skipping these messages from export would be more bulletproof.

[grapestain]

We can say that the severity in the translation system should match that of the runtime. What I mean is that warning in the translation UI, but fatal error during runtime is a mismatch that is a recipe for issues. If the runtime throws fatal error, the translation system should be at least as strict, preventing those strings to end up loaded by the runtime. If the translation system allows ignoring mismatched placeholder counts, the runtime should as well be modified to only issue warnings instead of crashing.

[swissspidy]

​https://github.com/GlotPress/GlotPress/issues/1877 was just opened, FWIW.