XMLRPC API Clients can't edit underscore-prefixed custom fields

Description

Changes that were committed in r17994 changed the behavior of the XMLRPC API such that clients who attempt to create or modify custom field values that start with a "_" underscore, encounter a quiet failure for those values to stick.

It seems at least somewhat common for 3rd party plugins and themes to use underscore-prefixed custom field values, in particular when they provide their own UI for setting those values, and don't want the values to show up in the custom fields UI of the WordPress admin panel.

Because a remote client using the XMLRPC API doesn't cause the plugin's custom code or UI to appear, we need the ability to continue editing these fields via the API.

A concrete example of this problem in action is the All In One SEO pack, which uses custom fields that have a prefix of _aiseop. My customers (MarsEdit) who have custom fields configured to edit these values are finding since 3.1.3 that the specified values simply "don't take" on the server. I have traced this to the changes in r17994:

Particularly to the new is_protected_meta which counts as protected any custom fields that begin with an underscore. Presumably this function ends up getting called on behalf of the API from the "edit_post" API call in post.php, but I haven't confirmed the exact call chain.

It would be great if the XMLRPC API could continue having unfettered access to editing underscore-prefixed custom fields.

Introduces is_hidden_meta(). Prevents both hidden and protected from being updated by the ajax handlers and the edit form handler. Allows hidden to be updated by delete_meta() and update_meta(). Introduces a new prefix, '__', that plugins can use to protect their meta.

is_hidden_meta() is a superset of is_protected_meta(). It will always return true whenever is_protected_meta() returns true. (In which case, we should probably reverse the checks so is_hidden_meta() comes first.) That said, there's an exception: When the protected filter is used to add a meta key there. I'm not sure we should expect them to know they need to filter it in both spots.

As I said in the mail, it would be better if all the internal WP meta keys are standardized. At some point in the future, WordPress will probably have security issues when someone will add a new meta key that is not protected in is_protected_meta. This already happened with the previous approach.

Just to cite one example, with the latest patch (17850.4.diff), WP is vulnerable to persistent XSS attacks because the '_oembed_MD5...' is not covered in is_protected_meta -- an user with the edit_posts capability can also use the XMLRPC API.

For 3.2, we can maybe use your approach, but for 3.3 it would be good to have a strong solution to this problem.

This stuff is a mess. Editing through either XML-RPC or the post custom meta box is fraught with peril. I think we need to leave all underscore prefixed meta items as protected from XML-RPC and the custom meta box. Plugins really need to register a sanitizer to make these safe to edit.

To do this right I think we need register_meta() API that allows registering sanitize and auth callbacks. Then use current_user_can('edit_post_meta', $meta_key, $post_id). edit_post_meta would be a meta cap that checks the edit_post cap and also checks for an auth callback for the particular meta key.

The latest two patches seem good. Both have a little type in wp-includes/post-template.php, it should be is_hidden_meta( $key ), an not is_hidden_meta( $keyt ). Also the code to protect delete_meta at wp_xml_rpc_server may be theoretically bypassed -- one can just send the mid of a protected meta with any random meta key.

Regarding 17850.7.diff, adding a is_callable check would also be good. Will you intend to push this for 3.2? I think it should be tested extensively.

That patch is not even close to finished. It is just a proof-of-concept along one code path. I think this is too much for 3.2. I suggest doing this early in 3.3, letting it soak a bit, and then back porting to 3.2.1. A survey of a few plugin authors indicates the expectation is that underscore prefixed meta keys should not be editable by xml-rpc. Hopefully this means not too many will be hampered by these remaning protected awhile longer.

In conclusion, opening up underscore meta without introducing some cap and sanitize hooks is just opening the security hole back up, whether it be for protected core meta or protected plugin meta. Since registering cap and sanitize hooks is too big an undertaking for 3.2, I suggest punting to 3.3.

Agree with @ryan on this. Seems too big a change to go in now. Also this whole API/UI would probably need some re-thinking in 3.3. I personally don't see a point of the meta postbox at all (but that's for another discussion).

But thinking about it some more though, I don't know if having a callback should be enough to un-protect metadata. Come 3.3 when we introduce register_meta(), we're going to want to undo that, and have some sort of a visibility setting.

Maybe we just leave everything as is, and get a plugin like AIOSEOP to add a filter to unprotect certain meta. That then means those meta pieces are restricted by the same capabilities as editing the rest of the post.

Currently this protection can be easily bypassed in two different ways using the ajax or xmlrpc api. I am able for example to add the _wp_attached_file meta to some post. I describe the steps to reproduce the problems using the ajax api.

Create a new meta key, for example "foo" using the post editor. Then, rename this meta key to _wp_attached_file.

Create a new meta with the following key \_wp_attached_file. The stripslashes function is called to times when adding creating a new meta with the add_meta function.

add_meta() was double stripping and calling maybe_serialize() too early. 17850.18.diff is a derivation of 17.diff. It retrains the stripping of the key so that is_protected_meta() and current_user_can() have the proper key to work with. esc_sql() adds the slashes back prior to calling add_post_meta().