Support

Account

Home Forums Front-end Issues Is sanitization required for front end form?

Solving

Is sanitization required for front end form?

  • Hi,
    Many thanks for the brilliant plugin – I use it on nearly all my websites!

    I’m using ACF for a front end form which creates a new custom post (‘registration’) as a draft (before sending the user an email asking them to confirm registration – at which point their ‘registration’ is moved to ‘published’).

    My question – do I need to worry about data security/sanitization, or is this already handled by ACF? The form will be freely accessible in the front end.

    Many thanks,
    Sarah

  • Hi @sarah@hexagonwebworks.com

    Good question. In short, I’m not completely sure.

    ACF does not perform any PHP based sanitation of the values being saved, but I have a feeling that the WP functions would.

    ACF uses the WP functions such as update_postmeta to save a value. Perhaps you could look into that function for the answer?

    Thanks
    E

  • Hi Elliot,
    Many thanks for the quick response.
    I’ve had a look at update_postmeta in post.php, which in turn calls update_metadata in meta.php.

    update_metadata calls a sanitize_meta function (also in meta.php), but what I’m not sure is what this does exactly – this is the function:

    function sanitize_meta( $meta_key, $meta_value, $meta_type ) {	        return apply_filters( "sanitize_{$meta_type}_meta_{$meta_key}", $meta_value, $meta_key, $meta_type );
    }

    According to Codex:

    ‘Applies filters that can be hooked to perform specific sanitization procedures for the particular metadata type and key. Does not sanitize anything on it’s own. Custom filters must be hooked in to do the work. The filter hook tag has the form “sanitize_{$meta_type}_meta_{$meta_key}”. ‘

    But this is where I get a bit lost… does this mean data is being sanatized or not?!

    Also – do you know whether it’s just update_postmeta I need to worry about, or are there any other methods used too?

    Many thanks for your help.
    Sarah

  • Ok, done some more digging/reading.

    The sanitize_meta function I mentioned above simply adds a filter that you can then hook to do custom sanitization. As per the codex function – it doesn’t do any sanitization itself. So that’s kind of irrelevant.

    However, further down, the actual DB update is done using $wpdb->update, which does escape the data.

    What I’m not entirely sure yet is whether one then needs to do any additional sanitization. Also whether I’m ok to use the_field, get_field etc as-is when outputting data, or whether these need escaping?

    This post was helpful: http://wordpress.stackexchange.com/questions/44807/sanitize-vimeo-embed-code

  • Hi @Sarah

    Thanks for your research.

    The easiest way to figure out the above, is to run some simple tests. The term ‘sanitization’ can be interpreted in many ways, so I would recommend that you attempt saving some data which you deem needing ‘sanitization’ and see what it does.

    You can then use the filters available via WP to hook in and modify the value if need be.

    Thanks
    E

  • Thanks. I have done some initial testing.

    If one has a front end form with text field, a malicious user could potentially input <script>alert('You've been hacked');</script> and if echo get_field or the_field is used to output that field, the script will then run.

    As far as I understand it, one would therefore use echo esc_html (get_field(..)); to escape the data on output (or esc_attr, esc_url, etc as appropriate)

    I think this needs highlighting right at the top of your Front End form ‘how-to’!

    What I have yet to establish is whether any additional sanitization needs to be done before the data is input to the database. According to http://codex.wordpress.org/Class_Reference/wpdb#UPDATE_rows, $wpdb->update takes raw input values (they should not be SQL escaped) – so perhaps that’s enough?

    The only thing that bothers me is that potentially one could then still end up with malicious js script in the db – maybe the answer is to use esc_html on input to db as well as output.

    Any further thoughts welcome…

  • Hi @Sarah

    Perhaps you could hook into the update_value filter and escape all values before they are saved to the DB?

    The filter in question can be read about here:
    http://www.advancedcustomfields.com/resources/filters/acfupdate_value/

    Thanks
    E

  • Thanks for raising this @Sarah, I was wondering the same thing.

    Skimming through the code it looks like most of the actual data saving seems to be via prepared statements or sql escaped sql code (@Elliot is that right??) so I think you’re right in thinking that it’s the output escaping that needs to be watched out for…

    The approach I’m taking now is following the codex guide on output sanitisation (http://codex.wordpress.org/Data_Validation).

    So for anything on the form that needs to contain some html I’m using wp_kses() or wp_kses_post(), anything that comes from a field not allowed to have html tags gets echo’d through esc_html()

  • So if I were using the acf/update_value to scrub all of the fields, how should I format the code? What needs to go in the “Custom value”; area? Sorry, I’m a little lost. Thanks!

    function my_acf_update_value( $value, $post_id, $field  )
    {
        $value = "Custom value";
    
        // do something else to the $post object via the $post_id
    
        return $value;
    }
  • This reply has been marked as private.
  • This excellent article on WordPress VIP might be of interest:
    Validating, Sanitizing, and Escaping

    In particular, they go into detail as to why it’s important to “late escape” everything:
    The Importance of Escaping All The Things

    Their advice?

    If it’s not escaped on output, it’s potentially exploitable. Never underestimate the abilities of an attacker – they’re experts at finding the way to make the ‘this should never, ever, be possible‘ things happen :). For maximum security, we must escape all the things.

  • I am appalled, that customers of ACF get code suggestions like (get_field) that are vulnerable.

    http://snippets.khromov.se/sanitizing-and-securing-advanced-custom-fields-output/


    @Elliot
    : Do you really think, that is the problem of your customers? You provide us with codesnippets, and this should sanitize html. There are a lot of unexperienced customers, with none or only a few programming skills (like me) and they/we trust your codesnippets blindly.

  • with all the WordPress XSS Vulnerabilities of late it would be nice if this topic could get some attention again?

  • @nickfmc – thanks for raising this again. It’s certainly a valid and topical point. In essence, EVERYTHING should be escaped on output – as per earlier links in this post.


    @elliot
    as per previous comments, I think it’s really crucial that the documentation is updated to make users aware that data is not escaped automatically (& indeed, couldn’t be, as it will depend on context).

    Thanks,
    Sarah

  • Just following up on this. Was wondering if @elliot as a response.

  • I think it should be an option when declaring the field. For now you can add a filter like this:

    add_filter('acf/update_value', function ($value, $id, $field) {
    	// Decide whether you want to sanitize based on $id or $field
    	// In my case, I just wanted to sanitize the user fields
    	if (strpos($id, 'user_') !== 0)
    	{
    		return $value;
    	}
    	return sanitize_text_field($value);
    }, 10, 3);

    Hope this helps until it has been properly fixed.

  • Elliot now allows sanitization of the code by adding this filter. I tested it and it works great. This should remove a lot of headaches at all 🙂

    http://www.advancedcustomfields.com/resources/acf_form/#security

  • Hi sododesign,

    thanks for the heads up! Very appreciated.

  • If your form provides repeatable fields, add this in your functions.php

    https://github.com/Hube2/acf-filters-and-functions/blob/master/acf-form-kses.php

    and replace default filter from:

    add_filter('acf/update_value', 'wp_kses_post', 10, 1);

    to:

    add_filter('acf/update_value', 'acf_wp_kses_post', 10, 1);

  • Thank you again. Now I am a little bit confused. What, if I have both: “normal” and repeatable fields?

  • No matter the fields you have, are equally sanitized and hassle. The function that Elliot updated serves precisely to both types of fields: normal and repeatable. 🙂

    http://www.advancedcustomfields.com/resources/acf_form/#security

  • @sododesign I tested your function and I found an error.

    if ( ! function_exists( 'acf_wp_kses_post' ) ) :
    
    	function acf_wp_kses_post($data, $post_id=0, $field=array()) {
    	
    		if (isset($field['type']) && ($field['type'] == 'repeater' || $field['type'] == 'flexible_content')) {
    			// no need to run it on repeaters
    			// will be called again for each subfield
    			return $value;
    		}
    
    		if (!is_array($data)) {
    			return wp_kses_post($data);
    		}
    
    		$return = array();
    		if (count($data)) {
    			foreach ($data as $index => $value) {
    				$return[$index] = acf_wp_kses_post($value);
    			}
    		}
    
    		return $return;
    		
    	}
    	
    endif;
    
    add_filter('acf/update_value', 'acf_wp_kses_post', 10, 3);

    I use repeater fields at option pages. If I use this function, after clicking the update button, all values are lost and not saved.

    What is the reason for this issue?

  • ok, after removing return $value; the repeater fields will saved after clicking the update button.

  • Hi @wootimes, this function is most appropriate for what you need to do 🙂
    It covers all fields without having to specify one by one.

    
    function my_kses_post( $value ) {
    	// is array
    	if( is_array($value) ) {
    		return array_map('my_kses_post', $value);
    	}
    	// return
    	return wp_kses_post( $value );
    }
    add_filter('acf/update_value', 'my_kses_post', 10, 1);
    
Viewing 25 posts - 1 through 25 (of 27 total)

The topic ‘Is sanitization required for front end form?’ is closed to new replies.