Merge non validated and non sanitized errors #934
Conversation
The ValidatedSanitizedInput Sniff is currently reporting two different errors for the same code, in case the global variable is not sanitized nor validated. This commit is merging the two errors into one, in caes both conditions are met. It's preserving the individual errors for cases when only one of them is true.
…mit, where non-sanitized and non-validated errors, if both are present, are merged into a single error. This commit is lowering the number of expected errors on lines 5 and 33 from 3 to 2 (which is the case due to the error merging).
| @@ -122,8 +125,7 @@ public function process_token( $stackPtr ) { | |||
|
|
|||
| // Check for validation first. | |||
| if ( ! $this->is_validated( $stackPtr, $array_key, $this->check_validation_in_scope_only ) ) { | |||
| $this->phpcsFile->addError( 'Detected usage of a non-validated input variable: %s', $stackPtr, 'InputNotValidated', $error_data ); | |||
| // return; // Should we just return and not look for sanitizing functions ? | |||
| $notValidated = true; | |||
| } | |||
|
|
|||
| if ( $this->has_whitelist_comment( 'sanitization', $stackPtr ) ) { | |||
There was a problem hiding this comment.
Because of this if condition and the one below it, if sanitization isn't needed the validation error would no longer be given either. This logic needs to be updated to fix that. (And also the unit tests, so that they'd catch this.)
There was a problem hiding this comment.
Oh, good catch! I'll look into it, thanks!
The previous commits introduced bug which lead to loosing errors in case the sanitisation has been whitelisted via comment or in case the validation was missing for usage in comparison.
…ts - loosing errors on whitelisted sanitisation and missing validation on comparison
| @@ -82,6 +82,9 @@ public function register() { | |||
| */ | |||
| public function process_token( $stackPtr ) { | |||
|
|
|||
| $notValidated = false; | |||
| $notSanitized = false; | |||
There was a problem hiding this comment.
Just wondering if the code would be easier to read if these variables would be reversed in meaning, i.e.
$validated = true;
$sanitized = true;Also wondering whether the default should be true (or false as the current code stands), though changing that might mean more extensive changes to the logic of the sniff are needed.
|
Has merge conflicts. |
|
I've had another look at this and IMO this change will make the sniff less easy to work with for end-users and should not be merged. The error codes, as they are, address separate problems which can currently be ignored via the ruleset or via inline comments separately using the appropriate error code. For example, say someone has non-validated, non-sanitized code, but has in another method already validated and sanitized all superglobal keys they use. In that case, they could ignore the new Now, in a refactor, they move the sanitization to the places where the superglobals are used. As the error code will now change, all the ignores will need to be updated, while in the original situation, this would not be necessary and the ignores (covering both original codes) would still work as expected. With that in mind, I'm going to suggest closing this PR. Aside from that:
|
|
Closing for lack of response/progress. |
The ValidatedSanitizedInput Sniff is currently reporting two different errors for the same code, in case the global variable is not sanitized nor validated. This commit is merging the two errors into one, in caes both conditions are met. It's preserving the individual errors for cases when only one of them is true.
We have been using this improvement on the WordPress.com VIP for a while already (introduced in Automattic/VIP-Coding-Standards#12 )
cc. @grappler