WordPress should register custom error and exception handlers

[bjorsch]

WordPress relies on PHP's display_errors setting for error reporting. One unfortunate result of that is that errors containing HTML may be served to the browser unescaped.

​WordPress/WordPress-Coding-Standards tries to help avoid this by having a sniff (WordPress.Security.EscapeOutput) that, among other things, asks that strings passed to trigger_error() and throw new SomeClass() be escaped. This is not a very good solution for a few reasons:

• It doesn't do anything about errors or exceptions thrown by PHP itself.
• It doesn't do anything about errors or exceptions thrown by libraries that might be bundled into themes or plugins or any custom code blog authors might inject.
• It means that text-based log files, non-HTML emails, CLI output, and the like will contain HTML entities.

Instead we should follow the principle that values should be escaped close to the point of output, when we know what kind of escaping will be needed. Which, in the context of PHP, means using set_error_handler() and set_exception_handler() to handle the outputting.

While #43712 is similar to this issue, the reasons and solution space are different. That ticket wants enhanced handling of errors for better user experience. This ticket is satisfied with the existing UX, it just wants error output fixed properly instead of trying to make every generated error be "safe" in order to avoid the potential bugs.

See also ​https://github.com/WordPress/WordPress-Coding-Standards/issues/2374#issuecomment-1703370109 where @jrf suggested I file this ticket.

WordPress relies on PHP's display_errors setting for error reporting. One unfortunate result of that is that errors containing HTML may be served to the browser unescaped.

WordPress/WordPress-Coding-Standards tries to help avoid this by having a sniff (WordPress.Security.EscapeOutput) that, among other things, asks that strings passed to trigger_error() and throw new SomeClass() be escaped. This is not a very good solution for a few reasons:

• It doesn't do anything about errors or exceptions thrown by PHP itself.
• It doesn't do anything about errors or exceptions thrown by libraries that might be bundled into themes or plugins or any custom code blog authors might inject.
• It means that text-based log files, non-HTML emails, CLI output, and the like will contain HTML entities.

Instead we should follow the principle that values should be escaped close to the point of output, when we know what kind of escaping will be needed. Which, in the context of PHP, means using set_error_handler() and set_exception_handler() to handle the outputting.

But WordPress also relies on various other things that PHP's default handlers do, not all of which we can duplicate. But on the plus side since display_errors is not exactly a boolean (and PHP doesn't care anyway) we can have the handlers themselves set display_errors to a sentinel value that PHP will interpret as "off", do any necessary output, and then hand off to PHP's default handlers to do everything else.

Trac ticket: https://core.trac.wordpress.org/ticket/59282

[ayeshrajans]

I like the idea of sanitizing HTML at the error/exception handler level. Like you mentioned in the description, sanitizing HTML can be annoying when the exceptions end up in log files, error trackers, etc.

A couple things I'm concerned about are how external exception/error handlers can "handoff" errors, and the somewhat uneasy feeling as we overload the INI values.

Code in PR looks good to me, although I would rather optimize it for the happy path (for example with early return statements), so it's not deeply nested or chains if/elseif.

[bjorsch]

how external exception/error handlers can "handoff" errors

What they should do is store the return value from their own set_error_handler() or set_exception_handler() call, and tail-call to that if there is one where they'd otherwise return false.

and the somewhat uneasy feeling as we overload the INI values.

display_errors is the only one being "overloaded", the rest are just being used in the same way PHP itself uses them.

I'm not entirely happy with the way display_errors is being used either, but I can't think of a better idea. We need to turn it off before returning false to call PHP's default handler, and we need to be able to differentiate our own turning-off from someone else's.

[oglekler]

@SergeyBiryukov @hellofromTonya can you please take a look? 🙏 There is a lot of work has done )

[oglekler]

We have Beta 1 in one week, and it looks that no one has time to look at the patch so far, so, I am moving it to the next milestone.

@kraftbj, I hope you can move this forward before next Beta 1. Thank you!