WordPress-Coding-Standards: Unexpected WordPress.CSRF.NonceVerification.NoNonceVerification warnings

There was a bug in 0.10.0 that would never show (me) warnings because the default severity level of 5 > 'warning'

I can provide code that proves this bug existed, but since it’s now fixed, I think it’s pointless? For the sake of this discussion there was, however, a change. This bug suppressed warnings.

I will accept that I now have to re-factor for warnings that I had previously not seen based on the tips provided.

Thanks for the discussion. Good times.

@connerbw Yes, I guess this may have been a bug in the sniff that was inadvertantly fixed.

This particular sniff is making a lot of assumptions as to how one should write code. Good points made but is this about a standard or something else?

The sniff has to make assumptions to be able to work; phpcs is not designed for complex code analysis, so it can’t be used to detect if a function is being called in a different part of the code.

However, we actually see this as a good thing. Why should it take complex analysis to determine if your code is secure? Isn’t it much better if anybody can see at a glance whether it is secure or not? That way, a lack of an nonce check is conspicuous and worth investigating, rather than it looking no different from code that is nonce protected, just with the nonce check somewhere else.

The sniff is not about code style, as you point out, and that is why it is part of the WordPress-Extra ruleset, and not WordPress-Core. So you are welcome to exclude it from your ruleset (though perhaps at your own peril).

If there are safe cases that we can detect better, I’m happy to improve the sniff, but the examples and suggestions presented so far would result in false negatives. For security-related sniffs like this we err on the side of having false positives, rather than false negatives. The risk of a missed security issue outweighs the burden of dealing with non-issues, at least in my mind.

@connerbw It’s not going to help with your general issue, but a couple of points:

• If your project is something where you control the PHP setup (as opposed to a publicly distrubuted plugin), then you could use filter_input. It’s enabled by default for PHP 5.2.0+, but it can be disabled.
• I’m not sure if your code example is simplified, but I read that as "if a POST request is made, or a GET request with at least one querystring param is made, and the POST data / querystring contains page that matches the value in a static property, then consider it to be a form submission.

If that code was powering example.com/foo, then it would be trivial to send an external POST request, or a GET request like example.com/foo?page=foo (or whatever the static property value is), and have your function return true.

This is precisely the case where you do need a nonce - if your form has a hidden field with a nonce value (use wp_nonce_field()), then you can check against this (wp_verify_nonce()). It has an option to also add in a referer field (or manually with wp_referer_field()). As you can see, it takes into account various factors, so is more reliable that this is indeed a form submission and not just a rogue request that happens to meet the trivial requirements.

Here are some examples that demonstrate why a blanket rule for the suggested cases is not a good idea:

if ( ! empty( $_GET['somevar'] ) ) {
    somefunc_that_should_be_protected();
}

if ( 'some_value' === $_GET['somevar'] ) {
    somefunc_that_should_be_protected();
}

Any time that you access request vars in any way, you are handling a request in some way. Your code is taking into account that a request has been made, and therefore you need to ensure that what it does as a result of that is not something that needs to be protected by an nonce. Just because you are not using the input value does not mean that your code is not responding to the request.

There may be something that we can do here to better decide when to issue a warning as opposed to an error, but I don’t think the sniff should be completely silent in 99+% of cases.

Just detecting a function call doesn’t work either, consider this:

function is_some_request() {
   if ( ! empty( $_GET['somevar'] ) ) {
       return true;
   }
}

// Elsewhere...

if ( is_some_request() ) {
   somefunc_that_should_be_protected();
}

If you follow certain patterns in your code, you may not need some cases flagged because they will always be safe, but just assuming that your code is going to follow the patterns and be safe is not a good idea.