WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions
MIT License
2.54k stars 474 forks source link

False positive: Allow trimming input before sanitizing #2180

Open Luc45 opened 1 year ago

Luc45 commented 1 year ago

Sometimes, an input is flagged as not sanitized if we trim it:

$foo = absint( $_GET['foo'] ); // OK.

$foo = absint( trim( $_GET['foo'] ) ); // InputNotSanitized.
$foo = absint( ltrim( $_GET['foo'] ) ); // InputNotSanitized.
$foo = absint( rtrim( $_GET['foo'] ) ); // InputNotSanitized.

I believe trimming should be accepted before sanitizing an input.

Maybe we could add ltrim, rtrim, and trim as a trimmingFunctions array and allow them here? https://github.com/WordPress/WordPress-Coding-Standards/blob/2ac765f2b0ae653efef9f5dda74cd2324f3abe78/WordPress/Sniff.php#L1852

dingo-d commented 1 year ago

What error are you getting that is bothering you (I saw several when running against the WordPress standard), so just wanted to be sure, about the sniff you're targeting.

Luc45 commented 1 year ago

@dingo-d the sniff I'm referring to is WordPress.Security.ValidatedSanitizedInput.InputNotSanitized

Luc45 commented 1 year ago

Read the code example carefully and you will understand, it flags the usage of trim as if we were using the unsanitized value, but trim doesn't use the values, only manipulate it.

kkmuffme commented 1 year ago

Technically, it should not report any error for all pure functions before sanitizing. For the default PHP ones (trim, strpos, str_contains,...) I guess it can be hardcoded.

Not sure if phpcs is aware/able to parse e.g. @psalm-pure tags in functions/methods to make this even better

jrfnl commented 1 year ago

Just had a quick look at this, but I don't understand the use of *trim() here in the first place - it's not needed, so why use it ?

https://3v4l.org/9Ih5I