Case where `WordPress.Security.ValidatedSanitizedInput.InputNotSanitized` might be a bit counter-intuitive

[leonidasmi]

Bug Description

The WordPress.Security.ValidatedSanitizedInput.InputNotSanitized error is thrown when a GET parameter is unslashed and then only used in a direct comparison, but it's not thrown when it's only used in a direct comparison.

I would expect the same behavior between those two cases, ideally and more specifically for that error to not appear in both cases.

Minimal Code Snippet

The issue happens when running this command:

phpcs ...

... over a file containing this code:

if ( isset( $_GET['foo'] ) && wp_unslash( $_GET['foo'] ) === 'bar' ) {
	//code;
}

but not over a file containing this code:

if ( isset( $_GET['foo'] ) && $_GET['foo'] === 'bar' ) {
	//code;
}

Error Code

ERROR | [ ] Detected usage of a non-sanitized input variable:
    |       |     $_GET['foo']
    |       |     (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)

It would probably make sense if the error should not appear in both cases? 🙂

Environment

Question	Answer
PHP version	7.4.20
PHP_CodeSniffer version	3.7.2
WPCS version	2.3.0
WPCS install type	Composer project local and git clone (haven't tested others)

Tested Against develop branch?

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

[owaisahmed5300]

yes, also if this is POST/REQUEST/SERVER/COOKIE. not only get.

As long as this is a variable existence check. this should not raise Sanitize error.

[GaryJones]

Have you tried with the develop branch?

[jrfnl]

I know this is something which still needs looking at, I have some ideas, but nothing ready to pull yet.

Idea is along the lines of:

• Check if the use is within a control structure condition.
• If so, use a while loop to go though the nested parentheses from deepest to max the control structure parentheses
• For each parentheses, do a number of checks, like "in comparison" (for the token before the current open parens + the close parens)
• Check if the function being used is not one which changes the param by reference (in as far as possible, i.e. PHP native functions only + based on Reflection)

[ve3]

What sanitize function I have to use? sanitize_url(wp_unslash($_SERVER['REQUEST_URI'])) does not work.

Result:

Detected usage of a non-sanitized input variable:
| | $_SERVER['REQUEST_URI']
| | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)

[owaisahmed5300]

@ve3 esc_url_raw() would be used to sanitized URLs before saving them into database.

[banago]

This is still happening in the Plugin Check plugin.

I end up ignoring it manually:

// phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized

[kkmuffme]

@jrfnl to add on to

Idea is along the lines of:

if there is a function used, it must be a PHP built-in function (e.g. strtolower, trim,...) or from (a to be added) property of the rule (e.g. how there is <property name="customSanitizingFunctions" type="array"> it would be great if there was a similar property for functions that are "safe" to be used on unsanitized data in control structures)

e.g. if ( strtoupper( $_SERVER['REQUEST_METHOD'] ) === 'POST' ) { shouldn't report an error

[kkmuffme]

@jrfnl why 👎 ?

[jrfnl]

@kkmuffme Because your comment has absolutely nothing to do with the original issue and is just (unwelcome) noise.
While it may be about the same sniff, it is not addressing the same issue and the issue you are talking about is one which is unlikely to ever be addressed at all due to the complexities it brings - which you completely ignore and show no understanding off.

[kkmuffme]

You wrote:

Idea is along the lines of:

• Check if the use is within a control structure condition.
• If so, use a while loop to go though the nested parentheses from deepest to max the control structure parentheses
• For each parentheses, do a number of checks, like "in comparison" (for the token before the current open parens + the close parens)
• Check if the function being used is not one which changes the param by reference (in as far as possible, i.e. PHP native functions only + based on Reflection)

So how is if ( strtoupper( $_SERVER['REQUEST_METHOD'] ) === 'POST' ) {

not addressing the same issue

as if ( isset( $_GET['foo'] ) && wp_unslash( $_GET['foo'] ) === 'bar' ) {

?

unlikely to ever be addressed at all due to the complexities it brings - which you completely ignore and show no understanding off.

Thanks for being condescending.

At least I understand enough that using phpcs for carpet bombing code with unnecessary sanitizations was a best-practice in a time long gone.
It would make way more sense to actually start using and contributing to taint analysis (like psalm's for example) instead of wasting more time trying to fix the shortcomings of these rules in phpcs. These sanitizing rules have nothing to do with "code style", which is where using phpcs makes sense.
If the only tool you have is a hammer, it is tempting to treat everything as if it were a nail.