Security/NonceVerification: handle function names case correctly

[rodrigoprimo]

The sniff was incorrectly treating function names as case-sensitive when checking for nonce verification functions. This led to false positives when developers used valid nonce verification functions with mixed or uppercase letters.

The fix ensures function names are properly converted to lowercase before comparing against the allowed nonce verification functions list, respecting PHP's case-insensitive function name behavior.

[jrfnl]

Good catch.

... respecting PHP's case-insensitive function name behavior.

PHP isn't completely case-insensitive for function names. Only for the ASCII characters and as this sniff allows for end-users to provide a list of customNonceVerificationFunctions via a public property, we cannot rely on all nonce-verification functions we need to check against being completely ASCII-based.

In other words, if someone would add a custom nonce verification function called déjà_vu(), this function would now never be recognized as a valid nonce verification function.

So, we can only use strtolower() to compare the names case-insensitively if we have a known list of functions we compare against and we are 100% sure all those function names are ASCII-based names.

When the list of functions to compare against is not known in advance and/or doesn't consist of all ASCII-based functions we need to loop through all "allowed functions" using the PHPCSUtils NamingConventions::isEqual() method - which does a comparison between two names in a way which is similar to how PHP does it.

[rodrigoprimo]

In other words, if someone would add a custom nonce verification function called déjà_vu(), this function would now never be recognized as a valid nonce verification function.

I might be missing something, but I believe the sniff would handle déjà_vu() correctly. strtolower() ignores multibyte characters/non-ASCII characters. Is this not similar to what we discussed in a call and resulted in PHPCSStandards/PHP_CodeSniffer#372 (comment)?

Here is how I'm checking this:

// phpcs:set WordPress.Security.NonceVerification customNonceVerificationFunctions[] déjà_vu
function non_ascii_characters() {
    déjà_vu( 'something' ); // Ok.

    update_post_meta( (int) $_POST['id'], 'a_key', $_POST['a_value'] );
}

function non_ascii_characters() {
    DéJà_VU( 'something' ); // Ok.

    update_post_meta( (int) $_POST['id'], 'a_key', $_POST['a_value'] );
}

function non_ascii_characters() {
    dÉjÀ_vu( 'something' ); // Error, but that is correct as déjà_vu and dÉjÀ_vu are different function names.

    update_post_meta( (int) $_POST['id'], 'a_key', $_POST['a_value'] );
}

That being said, one thing that I realize only after seeing your comment is that if a function, even one with just ASCII characters, is added to customNonceVerificationFunctions using uppercase letters, this could lead to false positives:

// phpcs:set WordPress.Security.NonceVerification customNonceVerificationFunctions[] UPPERCASE_NAME
function non_ascii_characters() {
    UPPERCASE_NAME( 'something' ); // Should be ok, but the sniff generates an error.

    update_post_meta( (int) $_POST['id'], 'a_key', $_POST['a_value'] );
}

Maybe using NamingConventions::isEqual() will be necessary anyway to handle this case? Or do you have a different suggestion?

[jrfnl]

@rodrigoprimo and me discussed this PR. Valid point about the call to strtolower(), however, that also means we need to ensure that the $customNonceVerificationFunctions are all lowercased before doing a comparison.

And we then also need to safeguard that via tests.

@rodrigoprimo Could you update the PR to ensure this is handled correctly please ?

[rodrigoprimo]

Thanks for your reply, @jrfnl. I just added two new commits, one fixing the sniff to handle custom nonce verification functions correctly regardless of the case, and another adding tests safeguarding that non-ASCII characters are handled properly.

[jrfnl]

Why are you changing the user-provided value ? instead of changing the merge result $this->nonceVerificationFunctions ?

[rodrigoprimo]

I had in my mind that the user-provided value is the only one that required change, and that is why I opted to change it. After seeing your question, I imagine you are suggesting changing $this->nonceVerificationFunctions to ensure the sniff behaves correctly if, in the future, a new function is added to this property without all lowercase letters, and also to ensure it behaves correctly for sniffs extending this one. Is that the case?

I went ahead and pushed a new commit changing $this->nonceVerificationFunctions instead of $this->customNonceVerificationFunctions.

[jrfnl]

LGTM Thanks @rodrigoprimo !

[GaryJones]

All looks sensible - and I learned something too!

• Case-insensitive for ASCII letters only.
• Accented letters (é, É, à) are byte-sensitive.

[jrfnl]

@rodrigoprimo Could you please rebase the PR to make it mergable ?

[rodrigoprimo]

@jrfnl, I force-pushed without changes, but now the PHPStan check is failing. I'm checking to see if I can reproduce this problem locally.

[rodrigoprimo]

PHPStan is failing due to an upstream issue: shivammathur/setup-php#1000

shivammathur/setup-php installed the incorrect PHPStan version (1.9.9 instead of 1.12.29). There is a comment in the issue saying that it was fixed, but I'm still seeing this error. I will investigate a bit more.

[jrfnl]

@rodrigoprimo Was just looking and saw the same (the wrong versions). Looks like the fix is currently in setup-php develop, not yet in the v2 branch which we use in CI. Shivam is generally pretty on top of things though, so I'd say give it a few more hours ;-)

[jrfnl]

Also - for the record - looks like this is not strictly a problem with setup-php, but with GitHub itself breaking the sort order of an API response.

[jrfnl]

@rodrigoprimo I retriggered PHPStan after the release of setup-php 2.35.5 and things are okay again.

When I look at the commits, I think they should all be squashed. Agreed ? I can do that on merge.

[rodrigoprimo]

When I look at the commits, I think they should all be squashed. Agreed ? I can do that on merge.

@jrfnl, yes, I agree that those commits should have been merged. @dingo-d, any reason why you opted not to squash them before merging?