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

Processing nonce data without verification when checking $_GET #1878

Closed
kkmuffme opened this issue Apr 5, 2020 · 6 comments
Closed

Processing nonce data without verification when checking $_GET #1878

kkmuffme opened this issue Apr 5, 2020 · 6 comments

Comments

@kkmuffme
Copy link

kkmuffme commented Apr 5, 2020

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 #2217

@dingo-d
Copy link
Member

dingo-d commented Apr 5, 2020

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
Copy link
Author

kkmuffme commented Apr 5, 2020

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
Copy link
Member

dingo-d commented Apr 6, 2020

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
Copy link
Author

kkmuffme commented Apr 7, 2020

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
Copy link
Member

jrfnl commented Jul 7, 2023

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: #737 (comment)

@jrfnl jrfnl closed this as not planned Won't fix, can't repro, duplicate, stale Jul 7, 2023
@kkmuffme
Copy link
Author

kkmuffme commented Jul 11, 2023

Replied there #737 (comment) 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 Add verification checks against $_SERVER header/env reads. #2217
  3. default case where $_POST/$_GET is used - no change, this is fine already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants