Add documentation for WordPress.Security.NonceVerification

[dhanukanuwan]

Add documentation for WordPress.Security.NonceVerification sniff. Related to #1722

[rodrigoprimo]

Thanks for your work on this PR, @dhanukanuwan! This is looking good.

I left a few inline comments for us to discuss.

[rodrigoprimo]

What about the following:

Suggested change

	When processing form data, the nonce value must be verified before processing the form data. Nonce verification will protect your website from Cross-Site Request Forgery (CSRF) attacks.
	The nonce value must be verified before processing data from $_POST or $_FILES, and should be verified for $_GET or $_REQUEST. Nonce verification helps protect against Cross-Site Request Forgery (CSRF) attacks.

Here is a breakdown of my suggestions:

• Remove the "processing form data" repetition.
• Mention "must" for $_POST/$_FILES and "should" for $_GET/$_REQUEST, as the sniff generates an error for the first group and a warning for the second (NonceVerificationSniff.php#L45-L56).
• Use "helps protect" instead of "will protect".
• Remove "your website", as the person reading the message is not necessarily a website owner (they could be a plugin developer, and so on).

[rodrigoprimo]

Before I review the specifics of the code sample, there is something I would like to discuss.

The valid code sample raises four WordPress.Security.ValidatedSanitizedInput errors:

---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 2 LINES
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  5 | ERROR | Detected usage of a possibly undefined superglobal array index: $_POST['foo_nonce']. Check that the array index exists before using it. (WordPress.Security.ValidatedSanitizedInput.InputNotValidated)
  5 | ERROR | $_POST['foo_nonce'] not unslashed before sanitization. Use wp_unslash() or similar (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
  5 | ERROR | Detected usage of a non-sanitized input variable: $_POST['foo_nonce'] (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 10 | ERROR | Detected usage of a possibly undefined superglobal array index: $_POST['foo']. Check that the array index exists before using it. (WordPress.Security.ValidatedSanitizedInput.InputNotValidated)
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Fixing those errors makes the code example more complex and may make it harder to see exactly what relates to this sniff:

if (
    isset( $_POST['foo'], $_POST['foo_nonce'] )
    && wp_verify_nonce(
        sanitize_key( $_POST['foo_nonce'] ),
        'foo_action'
    )
) {
    $foo = sanitize_text_field(
        wp_unslash( $_POST['foo'] )
    );
}

I'm not sure if in this case it is better to leave it as is or fix the errors.

An alternative would be to use check_admin_referer():

check_admin_referer( 'foo_action' );

if ( isset( $_POST['foo'] ) ) {
    $foo = sanitize_text_field(
        wp_unslash( $_POST['foo'] )
    );
}

I think the first approach is more illustrative (the reader can see the actual nonce verification) and is consistent with the example already used on the Fixing errors for input data wiki page. The con is that it is a bit more complex if the errors are fixed.

The second approach is simpler and more direct, but it is specific to admin requests and hides the nonce verification mechanics, so it may be less illustrative.

I know we discussed this briefly during Contributor Day, but I'm having second thoughts now that I'm reviewing the PR, which is why I'm raising it.

I slightly prefer the first option, which, if I recall correctly, is similar to what you initially had, but I don't have a strong opinion. What do you think? @dingo-d, @jrfnl, @GaryJones, any thoughts?

[rodrigoprimo]

Depending on the outcome of the discussion above regarding the valid example, this isset() might not be necessary (if we decide to keep the valid example as is without an isset() check).

[dhanukanuwan]

Thank you, @rodrigoprimo. I'm still on my way home after the WordCamp. Will take a proper look at this tomorrow.