Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions WordPress/Docs/Security/NonceVerificationStandard.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?xml version="1.0"?>
<documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd"
title="Nonce Verification"
>
<standard>
<![CDATA[
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

]]>
</standard>
<code_comparison>
<code title="Valid: The nonce value is verified before processing the form data.">
<![CDATA[
if (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

<em>wp_verify_nonce</em>(
$_POST['foo_nonce'],
'foo_action'
)
) {
$foo = sanitize_text_field(
wp_unslash( $_POST['foo'] )
);
}
]]>
</code>
<code title="Invalid: The form data is processed without verifying the nonce value.">
<![CDATA[
if ( isset( $_POST['foo'] ) ) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

$foo = sanitize_text_field(
wp_unslash( $_POST['foo'] )
);
}
]]>
</code>
</code_comparison>
</documentation>
Loading