The "previous" exception object can not be escaped

[gmazzap]

Bug Description

We can only escape strings. Exception constructors accept a third argument "previous" which is an exception object and can not be escaped.

I would expect EscapeOutputSniff to only look at the first argument of exception constructors.

Minimal Code Snippet

throw new \Exception(esc_html("Something when wrong"), 0, $previous);

throw new \Exception(esc_html("Something when wrong"), previous: $previous);

throw new \Exception(message: esc_html("Something when wrong"), previous: $previous);

throw new \Exception(previous: $previous);

Error Code

All the throw statements above emit: WordPress.Security.EscapeOutput.ExceptionNotEscaped

Custom Ruleset

N/A

Environment

Question	Answer
PHP version	all
PHP_CodeSniffer version	^3.9.2
WordPressCS version	^3.1.0
PHPCSUtils version	^1.0.11
PHPCSExtra version	^1.2.1
WordPressCS install type	Composer global, Composer project local
IDE (if relevant)	--

Additional Context (optional)

I saw that the EscapeOutputSniff class finds the parameter used to construct the exception, then it loops all of them to search for escaping tokens.

I think this issue could be solved only looking at the 1st positional parameter or message, if named. And so replace usage of:

$params = PassedParameters::getParameters( $this->phpcsFile, $call_token );

with:

$messageParam = PassedParameters::getParameter( $this->phpcsFile, $call_token, 1, 'message' );

And then check the parameter, if found.

I'm glad to provide a PR that does this (and test), if wanted.

Tested Against develop Branch?

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

[jrfnl]

@gmazzap Thanks for reporting this.

I'm a bit in two minds about this (which is, I suspect, also why I implemented it like it currently is).

The thing is, constructors for Exceptions can have any arguments. Exception constructors don't have to follow the same argument pattern as commonly used by the PHP native exceptions.
Additionally, it is also a common pattern to use throw MyException::create($arg1, $arg2, $arg3), where the parameters are all used in a sprintf() to create the exception message.

So, blindly only checking the first parameter is not the right solution as it presumes use of the PHP native exceptions and/or of exceptions which have the same arguments as the PHP native exceptions for their constructor....

Here's the commit in which the escaping check for exceptions being thrown was added: 90f291c

[gmazzap]

Thanks @jrfnl

Additionally, it is also a common pattern to use throw MyException::create($arg1, $arg2, $arg3), where the parameters are all used in a sprintf() to create the exception message.

I would need to look at data to agree with that, because if I stick to my personal experience, static constructors very rarely use all string parameters. When parameters are supposed to be used with sprintf (which is indeed common in my experience as well) they are often numbers (that don't need escaping) or objects (whose methods call results are used in sprintf, but you can't escape when they are passed).

Not to mention that, often, named constructors also support a $previous exception they then pass internally to the default constructor, even because otherwise it is not possible to set a $previous exception at all after the exception is instantiated.

That means:

• For normal constructor having more than one argument the chance of a false positive is 100%
• For static constructor is very high

Summing up the two, the total chance of this rule creating false positives with exceptions using more than one arg is extremely high.

That, added to the fact that the use case for this rule is pretty narrow to begin with (basically it helps mitigate a server misconfiguration) makes me think that always escaping all arguments is definitively overkill.

I want to ask: do you foresee the possibility of introducing configurations for:

• only looking at the first argument/message named argument for default constructors
• ignore anything that does not follow throw new (so only targeting default constructors).

If you agree to have these two configurations, I volunteer to write the code.