Custom Fields
Re-Envisioned
Support General Sanitization of Fields input

This topic contains 19 replies, has 3 voices, and was last updated by  Anh Tran 1 week ago.

Viewing 9 replies - 11 through 19 (of 19 total)
  • Author
    Replies
  • #15668

    Rao Abid
    Participant

    Hi Anh,

    Thanks for the reply.

    I am a plugin developer myself and Problem 1 was the main concern when you push something like this that can affect current users.

    i myself has written this helper class:
    https://github.com/boospot/boo-settings-helper

    and also author of this course:
    https://www.udemy.com/wordpress-plugin-development-using-boilerplate/

    I can well understand your situation as the authour of this framework and also the grievance of the situation where current 400k+ sites are using this framework without sanitization.

    I bet more than 50% of these developers are not using the sanitization filter you have referred before, simply due to one reason: They are using this framework for its “plug-and-play” nature so that they dont have to worry about setting up (explicit), saving (explicit) and sanitization (implicit) of metaboxes.

    Since its an established framework, so it will (soon) become the next target of Security researchers and i dont want metabox.io to make to headlines as some plugins have already done like “Revolution Slider”, “Social Media Buttons”, “Yellow Pencils” and many more as pointed here: https://www.pluginvulnerabilities.com/blog/

    Any such news can seriously undermine the reputation of the framework.

    In my opinion, following solutions are on the table:

    1. We provide a value like this for default sanitization:
      'sanitize_callback => 'default'
      Any developer using this will get default sanitization as per the nature of the field.
      [Workable but has extra typing for each field defination and make the repeating code]
    2. We can provide a constant based sanitiation like a constant is defined in wp-config.php or the plugin piggy back to the framework. if that constant is there, default sanitization will prevail.
      Example: METABOX_FIELDS_SANITIZE_DEFAULT = true
      [Most Desirable as default sanitization behavior can kick-in with just one Constant definition]

    3. We push this change is major future realeas and let the users know that its coming and they can modify their code accordingly. WooCommerce does this all the time and Developers are required to make their code compatible.
      [This solutin is most secure and will definitly raise support tickets once pushed but its the best path to follow. Any person not using default sanitization should conciously explicitly configurs in the field config array]

    Let me know your thoughts on this.

    Cheers,

    Rao

    #15698

    Anh Tran
    Keymaster

    Hey guys,

    I added some sanitization callbacks. More will come later. The full code of the sanitization is here.

    This is the 1st step on this. Any feedback is welcomed.

    #15705

    david.h
    Participant

    Hi Anh,

    Having dry-run the code, I pleased to see this heading in the right direction. Remember to keep this on your to-do list, because security should not be a one-time “set it and forget it” thing, but a constant test, evolution, improvement and refinement of the requirements.

    Before release, I suggest you employ some white-hat tactics and ask a core-team (maybe from your Facebook group) to try thwarting or circumventing the sanitization using combinations of complex field groups, custom tables, front-end posting and carefully malformed data.

    Thanks,
    David

    #15706

    Anh Tran
    Keymaster

    Hi David,

    Glad you like it. I’ve just pushed the finished code to the “sanitize” branch. The full code is now available here.

    There are also some things left that I would love to hear your feedback:

    • textarea field: at the moment, I force to use wp_kses_post as default. But as I said in a reply earlier, some developers make this field available for users to enter scripts (such as Google Analytics or Google Adsense). What do you think best in this way? Is forcing a callback is good?
    • Custom field type are not sanitized by default. Developers are required to sanitize themselves. Do you think it’s fine? Or forcing a default sanitize callback?
    #15709

    david.h
    Participant

    Hi Anh,

    Good job prioritising this – my feedback:

    First:

    It would be best if all data entry points are sanitized with the most appropriate WordPress function, so this would mean textarea would utilise ‘sanitize_textarea_field’:

    https://developer.wordpress.org/reference/functions/sanitize_textarea_field/

    Second:

    For any field employing ‘wp_kses_post’ (WYSIWYG or Custom), allow developers to pass an optional sub-array of tags/protocols to be allowed using:

    https://codex.wordpress.org/Plugin_API/Filter_Reference/pre_kses

    which is principally based on:

    https://codex.wordpress.org/Function_Reference/wp_kses_allowed_html

    Third:

    Enforce a minimum of ‘sanitize_textarea_field’ to all custom field types and allow developers to change this, but limited to only one of the built-in WordPress sanitization functions.

    Fourth:

    Allow developers to bypass WordPress security altogether, (obviously not advised) and implement their own sanitization.

    This should only be enabled if a developer (understanding the risks) changes the default setting of sanitize:true to sanitize:custom_callback and provides a working callback function.

    Hope this helps,
    David

    #15711

    Anh Tran
    Keymaster

    Hi David,

    Thanks for your feedback. Yes, I want to complete this thing as it’s important and I don’t want to postpone at the middle.

    Regarding your feedback:

    1. I think sanitize_textarea_field is too limited. It seems fine with the function name, but people usually use it for HTML content. I’m using wp_kses_post, which I think is okay.
    2. Adding params for wp_kses_post can be done. But it will make the code more complicated in both plugin’s side and developer’s side. I think in this case, developers can do that easier with a custom sanitize callback like this:
    'sanitize_callback' => function( $value ) {
        $allowed_tags = [];
        return wp_kses( $value, $allowed_tags );
    }
    1. I’m thinking about this, too. Need to find a good way to implement that without breaking things.
    2. That’s available via custom sanitize_callback param (see the example above). Developers can define their own sanitize callback or bypass the sanitization with this code:
    'sanitize_callback' => function( $value ) { return $value; }
    
    #15714

    david.h
    Participant

    Ok, although I’m not really sure what feedback you are looking for?

    1) sanitize_textarea_field is for a textarea field which should be used as per https://developer.mozilla.org/en-US/docs/Web/HTML/Element/textarea

    2) I’m not sure this would be more complicated, its simply a nested array passed to a function which you have already outlined.

    #15719

    Anh Tran
    Keymaster

    Hi David,

    Your feedback is great and that’s exactly what I’m looking for. My reply above was just a discussion about your ideas.

    I’ll consider your suggestions. Thanks.

    #15730

    Anh Tran
    Keymaster

    Hey guys,

    I finish the code for the sanitizer part.

    It works like this:

    • Sanitization is auto applied for all built-in types.
    • For custom field types, developers needs to implement their sanitization via sanitize_callback parameter. If no sanitize callback is provided, the field won’t be sanitize. I do this to not break the websites at the moment. In the future, I might add default sanitization for all custom field types if sanitize_callback is not defined. At this stage, I think this is enough.
    • Developers can also bypass sanitization by setting 'sanitize_callback' => 'none' (if set to a callable, it will be used for sanitization).

    I’ll merge the commits and release a new version for Meta Box next week.

    Thank you all for your feedback!

Viewing 9 replies - 11 through 19 (of 19 total)

You must be logged in to reply to this topic.