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

[redsweater]

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:

​http://core.trac.wordpress.org/changeset/17994

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.

[ryan]

Patch more selectively marks meta as protected.

[ryan]

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.

[redsweater]

Ryan - thanks for taking this bug and running with it.

I applied the patch and it doesn't address the crux of the problem, that XMLRPC clients are unable to update '_' prefixed custom fields.

Is it possible that in post.php it was a mistake to add the is_hidden_meta( $meta->meta_key ) clause to the tests that bail out on editing or deleting a custom field value?

[nacin]

Should is_hidden_meta() assume is_protected_meta(), perhaps calling it internally (purpose being the filter, I imagine), rather than requiring the double check?

[ryan]

As I finished the patch, I considered doing that and collapsing is_protected_meta() || is_hidden_meta() into just is_hidden_meta(). Could go ahead with that if it sounds good to you.

[ryan]

Although, I kinda prefer retaining is_protected_meta() || is_hidden_meta() for clarity.

[nacin]

3.1.4 for tracking purposes.

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.

[ryan]

Reverse order of checks

[nacin]

After IRC discussion, +1 from me.

[ryan]

Avoid add_meta()

[redsweater]

I can confirm that the 17580.4.diff does seem to address the original problem. I was able to edit a custom field with a leading underscore, and it stuck across refreshes of the blog content.

[xknown]

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.

[nacin]

Removing commit. You're right, things like _oembed_ absolutely need to be protected.

[ryan]

That adds _oembed_*. A search across the repo for update_post_meta, *_post_custom, and update_metadata suggests that was the only post meta key that was missed. User and comment meta need a look.

[ryan]

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.

[ryan]

All underscore prefixed are protected unless sanitizer is registered

[ryan]

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.

[ryan]

register_meta() and edit_post_meta cap, proof of concept.

[xknown]

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.

[ryan]

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.

[azaozz]

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).

[nacin]

I like 17850.6.diff​ for 3.2. I think it's clever.

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.

[ryan]

get_metadata_by_id(), error checking in register_meta(), barely tested

[ryan]

​http://unit-tests.trac.wordpress.org/changeset/374

[ryan]

​http://unit-tests.trac.wordpress.org/changeset/375

[ryan]

s/create_user_meta/add_user_meta/

[nacin]

Our future selves will want to know that today's IRC logs have extensive conversations on this API. Wanted to drop a note before I forgot.

[ryan]

​http://unit-tests.trac.wordpress.org/changeset/376

[ryan]

Allows editing _foo custom field through XML-RPC

[ryan]

In [18445]:

Introduce register_meta(), get_metadata_by_mid(), and *_post_meta capabilities. fixes #17850

[xknown]

I think I missed the party :)

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.

[xknown]

Do not call stripslashes two times and add verify if the new meta_key is not protected when editing an existing meta

[xknown]

About the problem of calling stripslashes two times, I don't like the "fix" I proposed. It assumes that the data is properly escaped when calling add_meta, if not, then there will be problems.

[ryan]

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().

[xknown]

That's better. I was thinking to do the same, but I got distracted.

[ryan]

In [18449]:

Check caps for both old and new meta keys when changing the key for a mid. Properly handle slashes when checking meta caps. Props xknown. see #17850