WordPress / WordPress-Coding-Standards

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

dont require sanitization for strpos and stripos #1691

Open kkmuffme opened 5 years ago

kkmuffme commented 5 years ago

strpos and stripos also do not have to have sanitized variables, since they do not modify either (and its just useless to sanitize e.g. the HTTP_HOST when its only used in strpos)

Sniff::$unslashingSanitizingFunctions - akin how count(),... don't require sanitization as added here https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/pull/1673

GaryJones commented 5 years ago

Per the bug report template, please provide the minimal snippet, error code, etc. This helps us to agree on exactly what the problem is.

kkmuffme commented 5 years ago

this is exactly the same as the change that was implemented for count(),... in the referenced pull request, but just with a different function

if ( strpos( $haystack, $_POST['needle'] ) ) {

Latest PHP, PHPCS, WPCS

rmccue commented 4 years ago

Strong +1 for this.

in_array counts as a sanitising function, and I think strpos should too. The canonical way of validating a string begins with a given substring in PHP is strpos( $haystack, 'start' ) === 0 (similar for the ending), and there's no way for security issues to arise from the use of strpos, as the function never reflects the input back.

For example, the following code requires sanitisation:

strpos( $_SERVER['REQUEST_URI'], '/test' ) === 0

However, you really only know if you should bother sanitising this value if it passes that check.

kkmuffme commented 2 years ago

This applies for all functions used in if/elseif

Since the only security risk would be if their results are assigned in the if/elseif: however this is reported/disallowed by WordPress.CodeAnalysis.AssignmentInCondition already (if this function doesn't exist, allowing unsanitized in if/elseif would be restricted to functions that return bool/float/int only)

kkmuffme commented 2 years ago

Furthermore: (I think there was a ticket, but I cannot find it anymore): simple comparisons in if/elseif:

if ( $_POST['foo'] == 'bar' )

jrfnl commented 2 years ago

This applies for all functions used in if/elseif

Since the only security risk would be if their results are assigned in the if/elseif: however this is reported/disallowed by WordPress.CodeAnalysis.AssignmentInCondition already (if this function doesn't exist, allowing unsanitized in if/elseif would be restricted to functions that return bool/float/int only)

WPCS strongly discourages assignments in conditions, but that doesn't mean they aren't used. Sniffs have to function independently of each other as they can also be excluded/ignored independently of each other. A sniff which contains a presumption on another sniff being enabled is flawed by design.

kkmuffme commented 2 years ago

Ok, so then only for functions that return bool/int/float, since these are safe.