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.