Skip to content
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

Enable NonceVerification sniff. #38

Open
1 task done
peterwilsoncc opened this issue Feb 22, 2023 · 1 comment · May be fixed by #39
Open
1 task done

Enable NonceVerification sniff. #38

peterwilsoncc opened this issue Feb 22, 2023 · 1 comment · May be fixed by #39

Comments

@peterwilsoncc
Copy link
Contributor

peterwilsoncc commented Feb 22, 2023

Describe the bug

Currently the WordPress.Security.NonceVerification.Missing is disabled with a comment that it rarely works properly.

My guess is the source of this comment is related to pages that use a custom query string parameter such as /wp-admin/admin.php?page=my-plugin&my-plugin-tab=sub-page or similar on the front-end.

<!-- This nonce sniff almost never works right -->
<exclude name="WordPress.Security.NonceVerification.Missing"/>

My view is that this treats the perfect as the enemy of the good by allowing potential security flaws in to our code bases to avoid disabling the sniff on an as needs basis.

I suggest two changes to the coding sniffs:

  • enable the sniff
  • add wp_verify_nonce as a sanitized & unslashed. The function uses the input to make comparisons rather than for outputs or database storage
    <rule ref="WordPress.Security.ValidatedSanitizedInput">
    	<properties>
    		<property name="customUnslashingSanitizingFunctions" type="array">
    			<!-- Allow checking nonces without sanitization. -->
    			<element value="wp_verify_nonce" />
    		</property>
    	</properties>
     </rule>

In terms of documented coding standards, for functions in which the nonce is not needed there should be two requirements

  1. A detailed comment stating why the sniff is not needed
  2. //phpcs:disable WordPress.Security.NonceVerification.Missing be added above the processing of form data
  3. //phpcs:enable following the processing of form data (the sniff doesn't need to be specified & I've experienced bugs when it is in older versions of phpcs).

Steps to Reproduce

N/A

Screenshots, screen recording, code snippet

For functions that do require a nonce, this is simplest form of code I've found for ensuring there are no false negatives in the coding standards reports:

// Check nonce for cross site scripting protection.
if ( empty( $_POST['_nonce'] ) ) {
	return;
}

// Check if nonce is valid.
if ( ! wp_verify_nonce( $_POST['_nonce'], 'my-plugin-action-name' ) ) {
	return;
}

// Followed by usual capability checks, etc.

Environment information

No response

WordPress information

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@dkotter
Copy link

dkotter commented Feb 24, 2023

I agree that we should enable this by default. I understand why it's currently disabled but I agree with Peter's take here that by globally disabling this, we risk security issues creeping in.

What we've started doing on OSS projects is we ignore this sniff on a line by line basis if it isn't needed and we add a comment detailing why we are ignoring it (for example, verification is happening elsewhere so doesn't need to happen here). This makes it clear to any future developers why we are ignoring that rule.

@peterwilsoncc peterwilsoncc linked a pull request Feb 27, 2023 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants