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!

[desrosj]

Unfortunately, this has not received the needed attention during the 6.7 cycle. With the beta 1 feature/enhancement deadline in less than 24 hours, I'm going to punt this one.

[audrasjb]

As per today's 6.8 bug scrub: as this ticket has not received attention for more than a year, let's move it to Future Release.

[bor0]

@SergeyBiryukov would appreciate if you can help with this one 🙏

[jorbin]

There are currently over ​900 plugins that add an exception handler and ​3453 that add an error handler. A fairly large percentage of them don't implement calling a previusly registered handler. This means that there are going to be millions of sites that never can take advantage of this functionality. I worry this is going it is thus going to be hard for extenders to count on and the existing expectation to escape exceptions/errors is the safest.

[SirLouen]

Replying to jorbin:

There are currently over ​900 plugins that add an exception handler and ​3453 that add an error handler. A fairly large percentage of them don't implement calling a previusly registered handler. This means that there are going to be millions of sites that never can take advantage of this functionality. I worry this is going it is thus going to be hard for extenders to count on and the existing expectation to escape exceptions/errors is the safest.

Your concern makes all sense, but I think it doesn't fully justify the fact that the core itself could benefit from this and anyone who is not doing an intensive use of such plugins. Moreover, some plugins over time could evolve to adapt error handling aligned with WP standards. Also, maybe this could be at some point part of ​PCP

We cannot turn the head to the fact that there is space for improvement, and despite there could be an adoption barrier by extenders, it's something preferable long-term

Anyway, I think I need to see in practice some real code example collisions and some test info in general for the proposed idea (apart from a bunch of text bullet points). It can take a while to research for this info for a tester, while, in theory, the reporter/patch maker should have this info available if he has been working on this topic. These theoretical proposals of code are a bit hard to digest, ngl. @bjorsch this report has had a needs-test-info status for more than a year, so now I don't wonder why this has been stuck for a good while. First, someone should have satisfied this for progress.

[bjorsch]

Replying to jorbin:

There are currently over ​900 plugins that add an exception handler and ​3453 that add an error handler. A fairly large percentage of them don't implement calling a previusly registered handler.

That doesn't tell the whole story though. If a plugin's exception or error handler exits or returns true, that's not a problem. If the plugin also somehow forces display_errors off, that also wouldn't be a problem for this particular change.

And if it returns false, it could easily be argued that not chaining to any prior registered handler is already a bug in the plugin, since it's already overriding any other plugins' handlers.

Plus, as SirLouen mentioned, it may still be worth improving Core even if some plugins will override the improvement.

Replying to SirLouen:

Anyway, I think I need to see in practice some real code example collisions and some test info in general for the proposed idea (apart from a bunch of text bullet points). It can take a while to research for this info for a tester, while, in theory, the reporter/patch maker should have this info available if he has been working on this topic. These theoretical proposals of code are a bit hard to digest, ngl. @bjorsch this report has had a needs-test-info status for more than a year, so now I don't wonder why this has been stuck for a good while. First, someone should have satisfied this for progress.

"Needs-testing-info" was added even longer ago, but without any comment as to what, if anything, was needed from me. It was accompanied with a request for two people to look, who did not reply here or on the PR. I don't think you can legitimately blame the lack of progress here on me not providing something that was never asked for beyond the unexplained addition of a tag.

For that matter, it's still not clear to me from your reply what "test info" you're asking for. Unit tests were added when that was requested on the PR back in 2023. What additional testing info are you looking for? Instruction that you could arrange for warnings or errors (containing unescaped HTML) to be raised, with various configurations for PHP's display_errors and other ini settings, and see that HTML (if any) returned to the browser is properly escaped while log files don't have unnecessary escaping? Are you needing more detail than that? If so, please in turn be specific about what detail you need. 🙂

[SirLouen]

Replying to bjorsch:

"Needs-testing-info" was added even longer ago, but without any comment as to what, if anything, was needed from me.

True, you are right, needs-test-info was thrown at @anomiex, who created this patch. But since you commented in the dev-chat, I supposed that you could follow along with the patch easily (in the last couple of months I've seen that generally who write in dev-chat are people who reported + wrote the patch at the same time and are trying to make the whole point).

Instruction that you could arrange for warnings or errors (containing unescaped HTML) to be raised, with various configurations for PHP's display_errors and other ini settings, and see that HTML (if any) returned to the browser is properly escaped while log files don't have unnecessary escaping? Are you needing more detail than that? If so, please in turn be specific about what detail you need. 🙂

Yes, throwing some code examples to see in action. It's a good starting point, specially because they help to put people on the mood without having to do a massive effort to read through all the code and having to understand it (specially, when there are like 1 zillion reports waiting in the backlog). For this massive patch, I think we should play around a lot with it to convince someone that its time to move it forward. We should build an entire Testing Case to prove it's totally consistent (apart from Unit Tests). As I say, a couple of code examples to replicate and see clear differences before and after, is the best starting point. I forgot ​to link to this post that maybe, could be helpful in this process.

[bjorsch]

I'm still not very clear on what you're actually asking for, but let's give it a try.

The Use-Case is as described in the ticket: We want PHP error messages (including from uncaught exceptions) and warnings displayed in the browser to be properly encoded to avoid HTML injection, but escaping at the time the message is generated is not a good solution because (1) we can only do that for code we control and (2) we don't know whether the message is going to be displayed in a browser, or from wp-cli, or written to a log file, or what else. The generally recommended practice (outside of WordPress) is to escape at the time the value is being output, which is what the PR here implements.

As for code to be used for testing, here's a simple mu-plugin to generate various types of errors.

<?php

// Set appropriate ini settings. You'll at least want to test with html_errors both on and off.
// You could also do this in php.ini or the like instead of here.
ini_set( 'display_errors', true );
ini_set( 'html_errors', false );

function xxx_generate_error() {
        // Set this to 'warning', 'error', 'exception', or 'internal' to test different kinds of error messaging.
        $test = 'internal';

        // Test error message that contains some HTML that should not be rendered.
        $msg = 'This is a test: <script>alert("oops");</script>';

        switch ( $test ) {
                case 'warning':
                        // A warning. The value from `error_get_last()` should reflect the message above.
                        trigger_error( $msg, E_USER_WARNING );
                        error_log( var_export( error_get_last(), true ) );
                        break;

                case 'error':
                        // An error.
                        trigger_error( $msg, E_USER_ERROR );
                        break;

                case 'exception':
                        // An uncaught exception.
                        throw new RuntimeException( $msg );
                        break;

                case 'internal':
                        // A warning generated by PHP itself, where we can't reasonably HTML-encode the input.
                        $arr = array();
                        $var = $arr[ $msg ];
                        break;
        }
}

// You may want to try different actions to test handling during later parts of the request, or even just call it directly to test handling during the mu-plugin load.
add_action( 'plugins_loaded', 'xxx_generate_error' );

The comments indicate parts of the mu-plugin that you may want to edit to test different scenarios. In real life, errors and warnings would be generated by error cases or unintentional bugs in core, plugins, and themes.

As for triggering the mu-plugin, you'd want to do so in various ways:

1. Visit a page in a browser.
2. Run a wp-cli command.
3. Either of the above, but then look at what is written to debug.log.

Acceptance criteria for each case are:

1. The message is shown as text, not rendered as HTML.
2. On the console, the wp-cli output of the message does not have HTML-style encoding.
3. The log file does not contain HTML-style encoding either.

WordPress's current behavior with this mu-plugin will incorrectly render the message as HTML for 'warning', 'error', and 'internal', and for 'exception' too when html_errors is off.

There may be additional triggering methods to test. For example, if we have API endpoints that catch and report errors as properly formatted responses rather than letting PHP spit out HTML or something malformed, those should produce output that does escaping appropriate to the output format rather than HTML-style encoding. For example, a JSON response might represent it as "This is a test: \u003Cscript\u003Ealert(\"oops\");\u003C/script\u003E" (with the client expected to escape it properly for however it's planning to output the response), not "This is a test: <script>alert("oops");<\/script>" where the client would have to HTML-decode it before using it in a non-HTML context.