Best Practice for Validating and Sanitizing Data

Question

I developed a plugin which processes data following user input into a number of forms on the front end. The field types within the forms and the data input varies significantly.

To process the data, I hook into init and use the following function;

function mh_post_actions() {
    if ( isset( $_POST['mh_action'] ) ) {   
        do_action( 'mh_' . sanitize_text_field( $_POST['mh_action'] ), $_POST );
    }
} // mh_post_actions
add_action( 'init', 'mh_post_actions' );

Validation and sanitizing then takes place in the resulting functions hooked into mh_*.

However, recently I took the decision to publish the plugin via the WordPress.org repo.

On review, my code has been rejected stating that per the guidelines, I need to validate and sanitize the data before any WordPress processing takes place.

That is fine, but as mentioned the data can be anything, array, int, str, url, email etc...

I'd like to keep the function I have as it works well for me, so wondered if there was any easier way to sanitize general $_POST data which I could add to the above function in order to meet the guidelines?

Answer 1

Inputs need to be validated/sanitized before making any execution flow decision based on it. Actually a +100 to the reviewer that caught it (or whoever wrote the automated tool) as I would have missed it.

Sanitization is something that needs context. Just because function A does a sanitization in the context of storing an displaying text input , doesn't make it appropriate to be used in execution flow context.

In your specific case an "hostile" can trigger any hook that starts with 'mh_' by sending a specialy crafted value in the mh_action field. What you need to verify before triggering any action is that the value is one of those you expect to get from your form.

If (in_array($_POST['mh_action'], array('string',int','array'....)
  do_action('mn_'.$_POST['mh_action'],$_POST);

Not sure if it will be enough for the review team, but it will be a (more) secure code.

Answer 2

The first problem I see with you code is that you are assuming that $_POST['mh_action'] is a string, coming from $_POST you can never be sure that's true.

And if that's an array, you could not do sanitize_text_field( $_POST['mh_action'] ) without triggering an error.

Second problem is that do_action returns nothing. It means that you are changing super global $_POST directly, which I strongly suggest you to avoid.

One of the many reasons is that the piece of code that processes data would need to access to $_POST and even if you sanitized using the action, looking only at the code that make used of data, that would not be unclear.

Moreover, some other code (maybe malicious, maybe not) could always remove_action( 'mh_foo' ) and your "foo" field would never be escaped, and so the code that access $_POST['mh_foo'] and processes it, would process potentially unsafe data.

What you should do is to take the data from $_POST, extract the data of interest, validate them (what is expected to be a string is a string...) after that you can pass the validate data as argument to the processing function, where you can first sanitize it and then make use of it, eg. store, display...

Just a proof of concept:

function mh_post_action() {

    $action = isset($_POST['mh_action']) ? $_POST['mh_action'] : '';
    if ( ! $action ) {
       return;
    }

    $validated_action = mh_post_action_validate( $action );
    if ( ! $validated_action ) {
       // handle the error

       return;
    }

    mh_post_action_process( $validated_action );
}


function mh_post_action_validate( $action ) {
    // as example I'm validating it is a string that starts with "mh_"
    if ( is_string( $action ) && strpos( $action, 'mh_' ) === 0 ) {
       return $action;
    }

    return '';
}


function mh_post_action_process( $action ) {
    $sanitized_action = sanitize_text_field( $action );
    // process sanitized_action here
}


add_action( 'init', 'mh_post_action' );

Now if I look at mh_post_action I can clearly see that data coming from $_POST is being validated, and I can clearly see that mh_post_action_process() sanitize data before doing anything with it.

I think that you can easily convert your existing function to follow such scheme and you will improve readability of your code (that is good for you), its understandability (that is good for the wp.org review team) and its security (that is good for everyone).