Support

Account

Home Forums Feedback acf_form security documentation

Solving

acf_form security documentation

  • As mentioned in this thread, the security guidance for acf_form will destroy data saved in a repeater or flexible content field.

    For anyone using those fields, this code should be used:

    add_filter( 'acf/update_value/type=text', 'wp_kses_post', 10, 1 );
    add_filter( 'acf/update_value/type=textarea', 'wp_kses_post', 10, 1 );
    add_filter( 'acf/update_value/type=wysiwyg', 'wp_kses_post', 10, 1 );
    add_filter( 'acf/update_value/type=password', 'wp_kses_post', 10, 1 );
    add_filter( 'acf/update_value/type=radio', 'wp_kses_post', 10, 1 );
    add_filter( 'acf/update_value/type=oembed', 'wp_kses_post', 10, 1 );

    This should really be clarified in the documentation, as the current advice there is very destructive to anyone who uses repeater/fc fields.

    I think the above code takes care of all XSS possibilities. The email and url fields should already contain sufficient validation to prevent script injection. Also, when post_title or post_content is enabled in the form, WordPress automatically handles script removal based on user capabilities. It appears that a script can be run on the form page when the oEmbed field is used, as that will run the input via ajax before the value is checked, so that’s a remaining potential vulnerability.

  • It’s also worth noting that if the user has any access to the backend, those filters should be placed in functions.php, and then they would no longer need to be added above acf_form_head.

  • Just adding to what David has posted. The problem is specifically with fields that store values as arrays that get killed. The reason this happens is that wp_kses_post() expects a string input and returns a string as output.

    The fields that can cause problems are

    • checkbox
    • multi select
    • Any field that allows the selection of multiple values, like a relationship field, a post object field set to allow multiple, a taxonomy field set to allow multiple, etc.

    I’ve been looking and It does not appear that there’s anything that will work on arrays.

  • Although a one-line solution would be nice, I think the most imperative thing is simply updating the documentation. I feel like I need to be clear that the current recommendation on there is DANGEROUS to anyone using repeater and FC fields.

    In my case, I came across the suggestion, thought “more security and XSS removal is good”, installed it and tested it without testing the more complex fields. That was sitting on my website for about a week, until a user finally contacted me telling me that their fields were “weird” and they couldn’t figure it out. That means that any user who updated their listing on my site that week would have destroyed any data they had previously entered into a repeater field, even if they were just changing one simple field.

    Not only is the current documentation potentially harmful, but at the very least it’s confusing to determine what the problem is. Is there somewhere else I should be recommending documentation changes?

  • I have marked this thread for E to take a look at. This is probably the best place to post something about the documentation. I don’t know if there’s anyone other than the developer that works on the documentation on the site, but I don’t think so.

    While I’m here I’ll also post a function that might be used in place of calling wp_kses_post directly that should solve the problem with arrays.

    
    function acf_wp_kses_post($data) {
      if (!is_array($data)) {
        return wp_kses_post($data);
      }
      $return = array();
      foreach ($data as $index => $value) {
        $return[$index] = acf_wp_kses_post($value);
      }
      return $return;
    }
    
  • That’s a nifty little recursive function you made. Would that be called like this?

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

    I haven’t tried that yet, but I do want to clarify that my code in the initial post works on repeater fields. If someone knows they will not use a certain field on their site, they can delete that line. The main goal is just to ensure that the function doesn’t get run on a repeater itself.

  • Yep, sorry I should have made it clear, you would use this function in the filter the way you have it.

    I was thinking about it and, I’m a bit of a security fanatic. Just because there’s JavaScript validation on a lot of ACF fields does not mean someone couldn’t submit unwanted values for any type of field. Some would say that I’m a bit overzealous when it comes to security.

    I’m pretty sure that my function is safe for all of the built in ACF field types, but I have not tested it with every type of field. There are a few add on fields that I know of that it may not be safe for. On the other hand, if I was using acf_form to allow users to submit content I’d be extremely careful about the types of field I allowed and in some cases I’d build my own validation to ensure that nothing noxious was submitted.

  • Hi everyone,

    I would like to add my voice to David’s call to please add the relevant information that David and John have shared here to the documentation as soon as possible! It wouldn’t take long and it would do a world of good to anyone trying to use repeater fields with acf_form() securely.

    Yesterday I spent hours trying to figure out why my front-end form’s repeater fields wouldn’t save! I had followed the security guidance provided in the documentation and didn’t think to question it. This particular thread did not come up in my searches, so I had to figure it out on my own. The only reason I found this thread actually is because I was planning on posting my findings on this issue myself.

    Thank you very much for the information that you both have provided on how we can keep our forms secure while still retaining the functionality of the repeater fields.

  • Im in exactly same boat as kisabelle. But nice to finely have to clearance on this – Thanks David.

Viewing 9 posts - 1 through 9 (of 9 total)

The topic ‘acf_form security documentation’ is closed to new replies.