Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review SanitizationHelperTrait::is_only_sanitized() in depth #2357

Open
jrfnl opened this issue Aug 18, 2023 · 0 comments
Open

Review SanitizationHelperTrait::is_only_sanitized() in depth #2357

jrfnl opened this issue Aug 18, 2023 · 0 comments

Comments

@jrfnl
Copy link
Member

jrfnl commented Aug 18, 2023

Per #2356 (comment):

I still have a niggly feeling there is a logic error in the is_only_sanitized() method, but I haven't been able to figure out the reason this was originally coded this way, so I'm not touching it for now.

The logic error I suspect is in the following code:

		// The only parentheses should belong to the sanitizing function. If there's
		// more than one set, this isn't *only* sanitization.
		return ( \count( $tokens[ $stackPtr ]['nested_parenthesis'] ) === 1 );

This seems to presume that parentheses could only be for function calls, while the extra parentheses may just as well be for control structure conditions.

If the other parentheses are for control structure conditions, I believe the variable should still count as "only sanitized".
It also discounts situations where unslashing is nested within a sanitization function call (which is taken into account in the NonceVerification sniff, but that is now inconsistent).

$a = is_email( $_GET['email'] ); // is_ony_sanitized(): true
$a = is_email( wp_unslash( $_GET['email'] ) ); // is_ony_sanitized(): false
if ( is_email( $_GET['email'] ) ) {} // is_ony_sanitized(): false

This should probably be further investigated when adding dedicated tests for these methods (#2272).

Might be a good idea for this and some other issues to involve the security team for second opinions on what the sniffs should accept as valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant