WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions
MIT License
2.54k stars 479 forks source link

Processing nonce data without verification when checking $_GET #1878

Closed kkmuffme closed 1 year ago

kkmuffme commented 4 years ago

I get Processing form data without nonce verification error for code like:

if ( empty( $_GET['author'] ) ) {    
    echo 'This is not an author page';
}

For obvious reasons it is impossible to use a nonce here as I am checking if something does NOT exist.

Also for the reverse:

if ( !empty( $_GET['author'] ) ) {    
    echo 'This is not an author page';
}

~~No processing of the $_GET happens whatsoever, so there is no need to add a nonce here. The nonce requirement should be changed to NOT trigger if $_GET is checked with empty/isset~~

EDIT: the striked text is potentially unsafe, see https://github.com/WordPress/WordPress-Coding-Standards/issues/2217

dingo-d commented 4 years ago

For obvious reasons it is impossible to use a nonce here as I am checking if something does NOT exist.

Not sure why you cannot check for nonce before checking for that $_GET value?

Have you checked wiki about this: https://github.com/WordPress/WordPress-Coding-Standards/wiki/Fixing-errors-for-input-data#nonces ?

kkmuffme commented 4 years ago

Because I am checking if it is EMPTY, which indicates that it is not bound to a specific submit action.

Also there is no increase in security whatsoever by always checking for a nonce, when the $_GET data isn't used, it's just annoying.

dingo-d commented 4 years ago

You can then just silence this by placing // phpcs:ignore comment in the line where it happens. We won't exclude a nonce check in WordPress Coding Standards, just because some people find it annoying 😉

What is the context in which you are using the $_GET['author'] variable?

kkmuffme commented 4 years ago

Ok, the issue is that the nonce check is way too broad. A nonce is only required if $_GET or $_POST data are used.

When doing empty( $_GET ) ( or !isset or !=(=)) the $_GET data is NEVER used. This is not about annoying, this is about the nonce checking being incorrect in ALL of these cases.

The reverse case I describe initially, there we can keep the nonce rule as it is, as I realised there are some possible cases where devs abuse this incorrectly, so we should still require nonce for !empty.

jrfnl commented 1 year ago

I'm going to mark this issue as a duplicate of #737.

I think this is the most relevant response in that ticket and it explains why the ticket is stagnant:

However, I wouldn't favor making it ignore all isset() and empty() checks. Even if no submitted values are being used, the fact that user input is being checked at all indicates that a user action is being reacted to, and thus CSRF protection is likely still needed.

Ref: https://github.com/WordPress/WordPress-Coding-Standards/issues/737#issuecomment-269012729

kkmuffme commented 1 year ago

Replied there https://github.com/WordPress/WordPress-Coding-Standards/issues/737#issuecomment-1630876118 now to make it clear that these are 2 (actually 3) different things.

1) !isset/empty when the $_POST/$_GET is not used - using a nonce is impossible and makes no sense logically 2) isset/!empty when the $_POST/$_GET is not used - I agree, to err on the save side here and actually require a nonce here, since we act upon user input EDIT: it must stay in this case, as it's unsafe otherwise, see https://github.com/WordPress/WordPress-Coding-Standards/issues/2217 3) default case where $_POST/$_GET is used - no change, this is fine already.