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

Get rid of all $_POST errors #510

Closed
hiddenpearls opened this issue Jan 16, 2016 · 18 comments
Closed

Get rid of all $_POST errors #510

hiddenpearls opened this issue Jan 16, 2016 · 18 comments

Comments

@hiddenpearls
Copy link

hello everyone, I am using PHPCS and WP Coding Standards setup for cleaning all the errors in my plugin code but $_POST errors are really confusing like

if ( 'Yes' === $_POST['auth_step'] ) )

above statement triggers these errors

 WARNING | Detected access of super global var $_POST,probably need manual inspection.
 ERROR   | Detected usage of a non-validated input variable: $_POST 

How I can get of this error and warning ? What is the best way to write this statement ?

@GaryJones
Copy link
Member

https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Flagged-superglobal-usage-in-WordPress-VIP

So, either whitelist it with a flag (Also see https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Whitelisting-code-which-flags-errors) or if it's a known system, you can use filter_input() (It isn't always enabled on some setups, so may not be available to users if this is for a distributed plugin or theme).

@JDGrimes
Copy link
Contributor

To fix the error, you need to validate that the auth_step key even exists in the $_POST array:

if ( isset( $_POST['auth_step'] ) && 'Yes' === $_POST['auth_step'] ) ) {}

@hiddenpearls
Copy link
Author

@JDGrimes @GaryJones

If I use isset and sanitize and also comment it to flag the variable.

if ( isset( $_POST['auth_step'] ) && 'Yes' === sanitize_text_field( $_POST['auth_step'] ) ) { // Input var okay.

I still get

ERROR   | Missing wp_unslash() before sanitization.

@hiddenpearls
Copy link
Author

Should I use sanitize_text_field() in Yoda condition ?

@JDGrimes
Copy link
Contributor

Should I use sanitize_text_field() in Yoda condition ?

No. PHPCS will correctly detect that sanitization isn't required here, since this is just a comparison of the value. This should avoid all of those errors:

if ( isset( $_POST['auth_step'] ) && 'Yes' === $_POST['auth_step'] ) { // Input var okay

@hiddenpearls
Copy link
Author

@JDGrimes yes, It worked. Needed a dot at the end of comment though.

if ( isset( $_POST['auth_step'] ) && 'Yes' === $_POST['auth_step'] ) { // Input var okay.

btw What will be the scenario for the statement below ??

if ( isset( $_POST['save_code'] ) && isset( $_POST['advanced_tab_nonce'] ) && wp_verify_nonce( $_POST['advanced_tab_nonce'] , 'advanced_tab_action' ) ) { // Input var okay.

Should I sanitize and unslash in this case ?

@JDGrimes
Copy link
Contributor

Should I sanitize and unslash in this case ?

Yes. In fact, since an nonce doesn't contain characters that would need to be slashed, so you could just use sanitize_key() to sanitize the nonce, and WPCS will know that wp_unslash() isn't even needed in that case.

So you can use sanitize_text_field() and wp_unslash(), or just sanitize_key().

@hiddenpearls
Copy link
Author

I see.

@JDGrimes I am moving into right direction now

So, for the scenarios below.

update_option( 'ANALYTIFY_USER_KEYS' ,      sanitize_text_field( $_POST['auth_step'] ) ); // Input var okay.

update_option( 'ANALYTIFY_CLIENTSECRET' ,   sanitize_text_field( $_POST['analytify_clientsecret'] ) ); // Input var okay.

I should refactor the above statements like this

update_option( 'ANALYTIFY_USER_KEYS' ,      sanitize_text_field( wp_unslash( $_POST['auth_step'] ) ) ); // Input var okay.

update_option( 'ANALYTIFY_CLIENTID' ,       sanitize_text_field( wp_unslash( $_POST['analytify_clientid'] ) ) ); // Input var okay.

but why it needs to be wp_unslash() ??

@JDGrimes
Copy link
Contributor

but why it needs to be wp_unslash() ??

It needs wp_unslash() because WordPress magic-quotes all of the input superglobals ($_GET, $_POST, etc.). Any time there is a quote mark (' or ") in an input string, it is "escaped" with a backslash: \' or \". So if you had a string like That's cool, it would be "slashed", turning it into That\'s cool. To undo WordPress's slashing, we need to use wp_unslash() to remove any slashes that it added to the user input. Otherwise, when the data is saved to the database and then later pulled back out of the database and displayed, it will have those extra slashes in it.

It is strange, I know. See https://core.trac.wordpress.org/ticket/24106, https://core.trac.wordpress.org/ticket/18322, and #172.

@hiddenpearls
Copy link
Author

I see
@JDGrimes Thanks.

What would be the best case for URL inputs ?

update_option( 'ANALYTIFY_REDIRECT_URI' ,   esc_url( wp_unslash( $_POST['analytify_redirect_uri'] ) ) ); // Input var okay.

can we refactor the above statement like this ?

update_option( 'ANALYTIFY_REDIRECT_URI' ,   filter_input( INPUT_POST, wp_unslash( $_POST['analytify_redirect_uri'] ), FILTER_VALIDATE_URL ) ); // Input var okay.

@JDGrimes
Copy link
Contributor

I think the easiest thing to do is to use esc_url_raw() like this:

update_option( 'ANALYTIFY_REDIRECT_URI', esc_url_raw( wp_unslash( $_POST['analytify_redirect_uri'] ) ) ); // Input var okay.

esc_url() escapes the URL for display, but esc_url_raw() really only sanitizes it (and it actually is a replacement for the deprecated sanitize_url() function). So esc_url_raw() is the recommended function to use for sanitizing a URL before saving it to the database.

I think if you want to use filter_input() you would need to unslash the value before calling filter input, like this:

$_POST['analytify_redirect_uri'] = wp_unslash( $_POST['analytify_redirect_uri'] ); // Input var, sanitization okay.
update_option( 'ANALYTIFY_REDIRECT_URI', filter_input( INPUT_POST, 'analytify_redirect_uri', FILTER_VALIDATE_URL ) ); // Input var okay.

You could use wp_unslash() after filter_input() instead, but I'm not entirely sure that it would always work correctly:

update_option( 'ANALYTIFY_REDIRECT_URI', wp_unslash( filter_input( INPUT_POST, 'analytify_redirect_uri', FILTER_VALIDATE_URL ) ) ); // Input var okay.

@hiddenpearls
Copy link
Author

@JDGrimes Thanks a ton. This discussion is really adding a lot of value to me and everyone who wants their code according to WPCS.

What would be the best way for Array inputs e.g I have a multi select dropdown and if I use

    if ( isset( $_POST['webprofile_dashboard'] ) ) {
        $web_profile_dashboard  = sanitize_text_field( wp_unslash( $_POST['webprofile_dashboard'] ) ); // Input var okay.
    }

It returns an error that Array to String Conversion.

@JDGrimes
Copy link
Contributor

@hiddenpearls There is a page in our wiki that discusses how to sanitize array inputs.

@hiddenpearls
Copy link
Author

@JDGrimes I edited that page. Actually, first parameter of the array_map should be valid callback function like below.

update_option( 'post_analytics_access_back', array_map( 'sanitize_text_field', wp_unslash( $_POST['my_var_array'] ) ) ); // Input var okay.

@JDGrimes
Copy link
Contributor

@emfluenceindia
Copy link

I am still getting the error with
if( isset( $_POST['user_role'] ) && '' !== $_POST['user_role'] ) { ... }

Screenshot: https://tinyurl.com/ycvt3e73
Editor: PHP Storm
Coding standard used: WordPress

@jrfnl
Copy link
Member

jrfnl commented May 16, 2018

@emfluenceindia This issue has been closed for two years. Opening a new issue will probably have more effect.

@emfluenceindia
Copy link

Thank you. I will do as you suggested.

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

5 participants