#17850 closed defect (bug) (fixed)
XMLRPC API Clients can't edit underscore-prefixed custom fields
Reported by: | redsweater | Owned by: | ryan |
---|---|---|---|
Milestone: | 3.3 | Priority: | normal |
Severity: | normal | Version: | 3.1.3 |
Component: | XML-RPC | Keywords: | has-patch |
Focuses: | Cc: |
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:
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.
Attachments (20)
Change History (56)
#4
@
13 years ago
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.
#5
@
13 years ago
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?
#6
@
13 years ago
Should is_hidden_meta() assume is_protected_meta(), perhaps calling it internally (purpose being the filter, I imagine), rather than requiring the double check?
#7
@
13 years ago
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.
#9
@
13 years ago
Although, I kinda prefer retaining is_protected_meta() || is_hidden_meta() for clarity.
#10
@
13 years ago
- Milestone changed from 3.2 to 3.1.4
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.
#12
@
13 years ago
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.
#13
@
13 years ago
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.
#14
@
13 years ago
- Keywords commit removed
Removing commit. You're right, things like _oembed_ absolutely need to be protected.
#15
@
13 years ago
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.
#16
@
13 years ago
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.
#17
@
13 years ago
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.
#18
@
13 years ago
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.
#19
@
13 years ago
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.
#20
@
13 years ago
- Keywords 3.3-early added
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).
#21
@
13 years ago
- Keywords punt added
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.
#26
@
13 years ago
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.
#28
@
13 years ago
- Owner set to ryan
- Resolution set to fixed
- Status changed from new to closed
In [18445]:
#29
@
13 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
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 theadd_meta
function.
@
13 years ago
Do not call stripslashes two times and add verify if the new meta_key is not protected when editing an existing meta
#31
@
13 years ago
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.
#32
@
13 years ago
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().
Patch more selectively marks meta as protected.