WordPress.Security.EscapeOutput encourages bad practice wrt exceptions

[anomiex]

Bug Description

Escaping should normally be done close to the point of output (or at least the point where HTML is being generated). The WordPress.Security.EscapeOutput sniff instead encourages escaping at the point where exceptions are generated, without knowing if the exception is going to be caught, or output to a CLI, or logged to a text-based log, etc.

Apparently this is because someone was worried about people having display_errors on (which is PHP's default) and html_errors off (which is not the default). The sensible thing would have been to have WordPress use set_exception_handler() to set a default exception handler early in the request, which could easily ensure any output of the exception message to the browser is escaped even on such misconfigured sites and which would also catch errors generated by PHP itself and errors generated by other libraries that don't use WPCS.

I note a similar argument could be made for WPCS's insisting on trigger_error() having escaping rather than having WordPress use set_error_handler(). Although in that case html_errors being on doesn't prevent the problem.

Minimal Code Snippet

The issue happens when running this command:

phpcs --standard=WordPress-Extra

... over a file containing this code:

throw new Exception( "Text is $text" );

Error Code

WordPress.Security.EscapeOutput.OutputNotEscaped

Custom Ruleset

Use --standard=WordPress-Extra

Environment

Question	Answer
PHP version	8.2.7
PHP_CodeSniffer version	3.7.2
WordPressCS version	3.0.0
PHPCSUtils version	1.0.8
PHPCSExtra version	1.1.1
WordPressCS install type	Composer project local
IDE (if relevant)	None

Additional Context (optional)

Tested Against develop Branch?

• I have verified the issue still exists in the develop branch of WordPressCS.

[jrfnl]

@anomiex I agree that would be the ideal solution, except WP does not do that. I suggest you open a Trac ticket to get WordPress fixed. I.e. implement callbacks to set_exception_handler() and set_error_handler() which do the escaping when necessary and do so while playing nice with other handlers from plugins and themes.

Once that fix is in place, I'd be very happy to remove sniffing for both those issues which annoy you.

Until that time, however, the messages from the sniff are perfectly valid IMO and this bug report is not.

[anomiex]

@jrfnl From what I've seen I'm not terribly confident that a fix to WordPress would be quickly merged, but I'll give it a try on Monday if nothing else comes up.

In the mean time, would you at least accept a PR (and then soon release a version 3.0.1 including it) to change this situation from WordPress.Security.EscapeOutput.OutputNotEscaped to something like WordPress.Security.EscapeOutput.ExceptionNotEscaped so those of us who don't find this "perfectly valid" can easily turn it off?

[jrfnl]

From what I've seen I'm not terribly confident that a fix to WordPress would be quickly merged, but I'll give it a try on Monday if nothing else comes up.

🤞🏻

would you at least accept a PR (and then soon release a version 3.0.1 including it) to change this situation from WordPress.Security.EscapeOutput.OutputNotEscaped to something like WordPress.Security.EscapeOutput.ExceptionNotEscaped so those of us who don't find this "perfectly valid" can easily turn it off?

@anomiex Changing the error code would be a breaking change, then again this is a recent addition and there are not that many users on WPCS 3.0 yet. I'd be open to it, but would like to hear the opinions of the other maintainers.

[dingo-d]

I am currently working on updating that sniff, so I'd be open to adding this. Although I wouldn't add this in 3.0.1, but 3.1.0 (which I thought my addition would be tagged under, as it contains extra checks, not just bug fixing).

[anomiex]

3.0.1 or 3.1.0 doesn't matter to me, as long as the option will get there soon to unblock our "update to WPCS 3" task without having to add 140 bogus escapings (possibly breaking other code) or 140 phpcs:ignores or fork the rule.

@dingo-d If you're going to work on it anyway I'm happy to leave it to you. I was just going to add a $code parameter to check_code_is_escaped() so the one call from the T_THROW case could specify a code other than OutputNotEscaped.

[jrfnl]

From a practical point of view, I'd rather have these as separate PRs as I wouldn't want one PR blocking the other from being merged and released.

[dingo-d]

From a practical point of view, I'd rather have these as separate PRs as I wouldn't want one PR blocking the other from being merged and released.

Yes, I'll work on the static one first, and then work on this one.

[jrfnl]

Or we could take @anomiex up on their offer to submit a PR for this one ?

[anomiex]

Turned out to be slightly more work than I had thought since the code had to be passed through in another function and some recursive calls, but #2378.

I went ahead and added a new code for trigger_error too since it turned out to be pretty trivial, but since that one has existed since 2015 it looks like I can understand if you'd rather not change that one in a version 3.1.0. I left a comment on the PR showing where to undo that code change.

[jrfnl]

I went ahead and added a new code for trigger_error too since it turned out to be pretty trivial, but since that one has existed since 2015 it looks like I can understand if you'd rather not change that one in a version 3.1.0. I left a comment on the PR showing where to undo that code change.

Yeah, changing that really would be a bigger breaking change, which I'm not keen on making in a patch/minor version.

It was also the reason I didn't add a separate error code for throw when I initially added the extra check as basically everything uses the same error code, independently of what type of code triggered the sniff, and adding error code differentiation just for one situation makes the sniff inconsistent in its behaviour and also dilutes the underlying message that everything which results in output to screen should be escaped.

[anomiex]

From what I've seen I'm not terribly confident that a fix to WordPress would be quickly merged, but I'll give it a try on Monday if nothing else comes up.

🤞🏻

Filed https://core.trac.wordpress.org/ticket/59282 and a PR to go with it. Let's see where that goes.

[jrfnl]

Closing as fixed via #2378