-
-
Notifications
You must be signed in to change notification settings - Fork 519
Add documentation for WordPress.Security.NonceVerification #2737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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. | ||
| ]]> | ||
| </standard> | ||
| <code_comparison> | ||
| <code title="Valid: The nonce value is verified before processing the form data."> | ||
| <![CDATA[ | ||
| if ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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( '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'] ) ) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| $foo = sanitize_text_field( | ||
| wp_unslash( $_POST['foo'] ) | ||
| ); | ||
| } | ||
| ]]> | ||
| </code> | ||
| </code_comparison> | ||
| </documentation> | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the following:
Here is a breakdown of my suggestions:
$_POST/$_FILESand "should" for$_GET/$_REQUEST, as the sniff generates an error for the first group and a warning for the second (NonceVerificationSniff.php#L45-L56).