Repository navigation
Rethink how we work with meta #1425
Description
Since we first implemented meta, we've been very restrictive. Meta available through the API is limited in the following ways:
- Serialised data is not available at all. You cannot read serialised data, you cannot write serialised data, and you cannot write normal data to a field that has serialised data already.
- Protected meta fields (typically, those beginning with
_) are not available at all. - All other meta is available only to those with the ability to edit the post.
These restrictions apply to all meta fields, with no real way to enable exposing meta in any further extensive way.
We always wanted to start out with the most restrictive policy on meta and then open it up slowly depending on use cases. I think it's time we did that. :)
Serialised Data
Serialised data should still not be available. We cannot open access to serialised data:
- Allowing read or write access is lossy. Internal PHP objects cannot be exposed as JSON without losing at least the class information. Saving the objects would always be a
stdClassinstance. - Converting PHP objects to JSON has privacy problems. Public properties are converted directly to a JSON object, which is a problem if the class does not mark protected properties correctly.
- Exposing PHP objects as a serialised string has privacy problems. All properties, both public and private, are exposed, which may be dangerous depending on the implementation of the class.
- Allowing write access for serialised strings has security problems. Serialised data can instantiate any class with any properties it needs, which essentially allows Remote Code Execution and a whole class of bugs.
Protected Meta
Rather than checking if meta fields are protected or not, we should leave this responsibility to the capabilities system. WP's meta-cap system already has meta capabilities for add_post_meta, edit_post_meta, and delete_post_meta. Internally, these use is_protected_meta by default, but allow register_meta to override this. This would allow overriding the protected meta system using WP's existing APIs, and would open meta up a bit more nicely. Bonus: we encourage use of the register_meta API, which is pretty neat.
One notable missing part here is read_post_meta. @nacin and I have discussed this in the past, and we're in favour of adding this. In the meantime, we could easily add a shim on the map_meta_cap filter.
Authenticated Access
Assuming we change protected meta, internally these all still check edit_post on the post, since the existing meta-caps are all write-based. However, with read_post_meta, we'd want to expose this publicly.
Ideally, the system would have been designed with this in mind. auth_callback already receives the capability we're trying to use, so it'd be basically perfect. However, using the existing auth callback from register_meta may be dangerous, as it usually relies on edit_post.
As a stop-gap, I'd suggest we check this, and check against a whitelist of meta keys to expose publicly. By default, this would be a blank array, but with the ability to add extra keys into it, similar to the query_vars filter.
It's a suboptimal solution, but it does allow us to retrofit the system to allow this.
With this, you could expose meta by doing:
register_meta( 'post', 'my_meta_key', null, 'my_validation_callback' );
function my_validation_callback( $allowed, $key, $post_id, $user_id, $cap, $caps ) {
switch ( $cap ) {
case 'read_post_meta':
case 'update_post_meta':
case 'add_post_meta':
case 'delete_post_meta':
return true;
default:
return false;
}
}
add_filter( 'rest_public_meta_keys', function ( $keys ) {
$keys[] = 'my_meta_key';
return $keys;
} );Anyone have anything they'd like to add? Anything I've overlooked here?
Activity
TimothyBJacobs commentedon Jul 24, 2015
Why not expose serialized data if it is an
arrayorstdClass?rmccue commentedon Jul 24, 2015
arrayandstdClasscan still be lossy.array( 'a' => 1 )and$x = new stdClass; $x->a = 1;are both represented as{"a": 1}in JSON.TimothyBJacobs commentedon Jul 24, 2015
Right, that is a good thing. But we know the data type before we update, don't we? So check the stored value, if the current value is an
arrayconvert the JSON to anarray, if the current value is anstdClassstore it as anstdClass.joehoyle commentedon Jul 24, 2015
What if you wanted to change the datatype from an array to an object? I don't know why - but I think JSON being lossy doesn't make this a good idea.
TimothyBJacobs commentedon Jul 24, 2015
An API consumer of any kind would never want to change the datatype because that would change what the server ( probably some plugin ) expects. The fact that they are both expressed the same way in JSON I think is a benefit. I'm just concerned that leaving out arrays especially is a huge loss. To flatten every array into individual meta rows would put a big strain on the DB.
If it is decided not to support both because it is lossy, then why not just pick
array. I'd venture to say that far more plugins store anarrayof data in post meta as opposed to anstdClass.TimothyBJacobs commentedon Jul 24, 2015
Realized a case where this might be impossible. When the value is set to an empty string, and then a client updates the value. Maybe only display serialized meta that is registered with
register_metaand add an argument to specify data type (stdClassorarray)? I just think that losing out on all forms of condensed meta is a huge loss.rmccue commentedon Jul 27, 2015
I think for plugins it's reasonably easy to use
register_api_fieldand handle structured data themselves, but it depends on what our common use case is here. If it's opt-in only, I could be OK with allowingarray. Our validation code may become a little more complex though, as we'll have to walk the array deeply to check there's no serialized-looking strings or objects in there.xavivars commentedon Aug 6, 2015
Is there any reason not to support other object types (even if they are PHP objects) if, let's say, plugins registered them as @TimothyBJacobs says? I think it would be really good for plugins to be able to serialize objects as meta being able to get them trough the API if needed.
rmccue commentedon Aug 7, 2015
See points 1, 3, and 4 from my list under Serialised Data; these still apply. If you only use arrays of scalars or more arrays, I think we can probably allow that, but it'll still be lossy. I think that's an OK tradeoff for now, but still need to consider it further.
xavivars commentedon Aug 7, 2015
I read your points, but I still don't get why is a problem, let's say that plugin X has a class (a DTO, for example, as if it was an associative array) that is saving as a post meta and it wants to expose through the API so it can be read (or written) through it.
Wouldn't make sense to allow that plugin to expose that meta key through the API so there is no need to do a single request to acces it instead of as many requests as properties had that class?
TimothyBJacobs commentedon Aug 11, 2015
Objects of any kind besides
stdClasswould be a significant pain unless they implemented theJsonSerializableinterface. Even then, they'd have to be read only. Figuring out an interface for them to be updated doesn't seem worth it when registering an API field would be so much simpler.I do think arrays should be given a try, I'll try working on a patch.
kmiskell commentedon Aug 18, 2015
I'm very much in favor of more ways to expose meta information. I just started looking into this today in the hopes that it would help me create an API to interface with the Modern Tribe Events Calendar without having to roll too much of my own stuff. Sadly, the plugin is based on protected meta keys, so that's out.
1 remaining item
digitalkaoz commentedon Jan 28, 2016
im with @TimothyBJacobs i think simple data structures (array,stdclass) should be allowed as serialized values. hacking around that limitation is a bit ugly here. maybe we can provide a global switch to allow sensitive data in
metaendpoints or not. since the wholemetaendpoints are secured anyways i think i doesnt makes sense the dictate even further by removing serialized meta fields, i mean im already authenticated, so i have access to the data anyway...maybe introduce a
and check that variable in the few places
danielbachhuber commentedon Jan 28, 2016
@digitalkaoz I've removed your comment, as we can't officially support insecure code snippets.
digitalkaoz commentedon Jan 28, 2016
yeah sure, i thought i put enough security disclaimers aside ;) anyway, would love to see this switch implemented
danielbachhuber commentedon Jan 30, 2016
We'll be splitting out the meta endpoints into a separate plugin.
danielbachhuber commentedon Jan 30, 2016
Here's the separate plugin https://github.com/WP-API/wp-api-meta-endpoints
digitalkaoz commentedon Jan 30, 2016
But its still forbidden to get/save serialized data...so what is exactly
the use case for a separate plugin?
Daniel Bachhuber notifications@github.com schrieb am Sa., 30. Jan. 2016
18:39:
danielbachhuber commentedon Jan 30, 2016
For those who are using the existing endpoints, as the meta endpoints have been removed from this core plugin.
joelsamuelk commentedon May 10, 2016
status code: 200, headers {
"Access-Control-Allow-Headers" = Authorization;
"Access-Control-Expose-Headers" = "X-WP-Total, X-WP-TotalPages";
Allow = GET;
"CF-RAY" = "2a0c3255a06c15f8-JNB";
Connection = "keep-alive";
"Content-Encoding" = gzip;
"Content-Length" = 272;
"Content-Type" = "application/json; charset=UTF-8";
Date = "Tue, 10 May 2016 08:59:42 GMT";
Server = "cloudflare-nginx";
"Set-Cookie" = "wfvt_-811174103=5731a2fea8ca0; expires=Tue, 10-May-2016 09:29:42 GMT; Max-Age=1800; path=/; httponly";
Vary = "Accept-Encoding";
"X-Content-Type-Options" = nosniff;
"X-Powered-By" = "PHP/5.6.21-1~dotdeb+7.1";
} })
joelsamuelk commentedon May 10, 2016
anyone can help?