Sniff for nonce verification

[JDGrimes]

This is the first pass at a sniff that checks for the use of nonce verification where needed.

It works by checking for the usage of the $_GET, $_REQUEST, and $_POST superglobals. Wherever these are used, it checks to make sure that an nonce verification function is used in the same scope.

Whether to give a warning or an error can be configured per-superglobal. Currently the default is to give errors for the $_POST super global, and only warnings for $_GET and $_REQUST. This can be changed via XML config:

    <rule ref="WordPress.CSRF.NonceVerification">
        <properties>
            <!-- Give errors for both $_POST and $_REQUEST -->
            <property name="errorForSuperGlobals" value="$_POST,$_REQUEST" type="array" />
        </properties>
    </rule>

It is also possible to add to the list of nonce verification functions via an XML config file like this:

    <rule ref="WordPress.CSRF.NonceVerification">
        <properties>
            <property name="customNonceVerificationFunctions" value="custom_verify_nonce,custom_check_referrer" type="array" />
        </properties>
    </rule>

By default, the list includes only wp_verify_nonce(), check_admin_referer(), and check_ajax_referer().

Even if a function contains an nonce check, any use of the superglobals within the function before that point will be flagged, except within isset() or empty() checks:

some_callback() {
    // This is OK.
    if ( ! isset( $_POST['test'] ) ) {
         return;
    }

    // But this isn't.
    do_something( $_POST['test'] );

    if ( ! check_ajax_referer( 'test_action' ) ) {
         return;
    }

    // Do things.
}

I've added this sniff to the WordPress-VIP and WordPress-Extra rulesets. Possibly the errorForSuperGlobals config should be set differently from the default for VIP.

TODO:

• Possibly add a configuration option for a list of functions which are allowed to be called with request parameters before the nonce check.
• Add tests for superglobals within class methods.
• Possibly add the ability to ignore a particular superglobal usage with a // comment.
• Maybe don't flag when a value is assigned to a superglobal.
• Probably something else I forgot.

Related: #73

[GaryJones]

At first glance, I didn't think this was going to be stable, but you've captured the typical format of functions well, good job.

Another todo item please - support filter_input() usage instead of superglobals.

[westonruter]

@GaryJones I don't think filter_input() should be encouraged, as I was told to not use it for Core: xwp/wp-widget-customizer#74

[GaryJones]

It might not be used in Core, but nothing in the standards say it can't be used in themes and plugin development. As a start, it passes the sanitisation checks that WPCS makes, since superglobals are not used directly.

[westonruter]

@GaryJones yeah, but I'm not sure if such a sniff enabled by default in WordPress-Extra. Sure, it can go into the WordPress standard, but I think it should be an opt-in via a project-specific standard PHPCS XML. In any case, we're talking about an entirely different sniff which should get its own GitHub issue.

[JDGrimes]

In any case, we're talking about an entirely different sniff which should get its own GitHub issue.

Yeah, I think it might be better as a separate sniff. We can abstract out parts of this one if needed.

[JDGrimes]

I've added support for whitelisting comments. There is one more thing that needs to be done, and that is allow for some sanitization functions to be called on input before an nonce check. This is needed for code like this:

$slug = sanitize_key( $_POST['thing_slug'] );

check_admin_referer( 'something_' . $slug );

I'll work on that, and try to have this ready to merge right after 0.4.0 is released. That way it will get plenty of time in develop, so we can see how stable it is.

[JDGrimes]

There is one more thing that needs to be done, and that is allow for some sanitization functions to be called on input before an nonce check.

Done. This is ready to be merged as soon as develop becomes 0.5.0.