Rethink how we work with meta #1425

Closed
rmccue opened this Issue Jul 24, 2015 · 21 comments

Comments

Projects
None yet
8 participants
@rmccue
Member

rmccue commented Jul 24, 2015

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 stdClass instance.
  • 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?

@rmccue rmccue added this to the 2.0 milestone Jul 24, 2015

@TimothyBJacobs

This comment has been minimized.

Show comment
Hide comment
@TimothyBJacobs

TimothyBJacobs Jul 24, 2015

Why not expose serialized data if it is an array or stdClass?

Why not expose serialized data if it is an array or stdClass?

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Jul 24, 2015

Member

array and stdClass can still be lossy. array( 'a' => 1 ) and $x = new stdClass; $x->a = 1; are both represented as {"a": 1} in JSON.

Member

rmccue commented Jul 24, 2015

array and stdClass can still be lossy. array( 'a' => 1 ) and $x = new stdClass; $x->a = 1; are both represented as {"a": 1} in JSON.

@TimothyBJacobs

This comment has been minimized.

Show comment
Hide comment
@TimothyBJacobs

TimothyBJacobs 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 array convert the JSON to an array, if the current value is an stdClass store it as an stdClass.

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 array convert the JSON to an array, if the current value is an stdClass store it as an stdClass.

@joehoyle

This comment has been minimized.

Show comment
Hide comment
@joehoyle

joehoyle Jul 24, 2015

Contributor

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.

Contributor

joehoyle commented 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

This comment has been minimized.

Show comment
Hide comment
@TimothyBJacobs

TimothyBJacobs 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 an array of data in post meta as opposed to an stdClass.

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 an array of data in post meta as opposed to an stdClass.

@TimothyBJacobs

This comment has been minimized.

Show comment
Hide comment
@TimothyBJacobs

TimothyBJacobs 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_meta and add an argument to specify data type ( stdClass or array)? I just think that losing out on all forms of condensed meta is a huge loss.

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_meta and add an argument to specify data type ( stdClass or array)? I just think that losing out on all forms of condensed meta is a huge loss.

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Jul 27, 2015

Member

I think for plugins it's reasonably easy to use register_api_field and 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 allowing array. 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.

Member

rmccue commented Jul 27, 2015

I think for plugins it's reasonably easy to use register_api_field and 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 allowing array. 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

This comment has been minimized.

Show comment
Hide comment
@xavivars

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

xavivars commented 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

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Aug 7, 2015

Member

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.

Member

rmccue commented 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

This comment has been minimized.

Show comment
Hide comment
@xavivars

xavivars 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?

xavivars commented 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

This comment has been minimized.

Show comment
Hide comment
@TimothyBJacobs

TimothyBJacobs Aug 11, 2015

Objects of any kind besides stdClass would be a significant pain unless they implemented the JsonSerializable interface. 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.

Objects of any kind besides stdClass would be a significant pain unless they implemented the JsonSerializable interface. 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

This comment has been minimized.

Show comment
Hide comment
@kmiskell

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

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.

@chriscct7 chriscct7 referenced this issue in easydigitaldownloads/easy-digital-downloads Nov 14, 2015

Closed

Register all of our meta keys with register_meta() #2480

@danielbachhuber danielbachhuber modified the milestones: 2.0, 2.0 Beta 12 Jan 20, 2016

@digitalkaoz

This comment has been minimized.

Show comment
Hide comment
@digitalkaoz

digitalkaoz 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 meta endpoints or not. since the whole meta endpoints 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

define('REST_META_SECURED', false) 

and check that variable in the few places

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 meta endpoints or not. since the whole meta endpoints 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

define('REST_META_SECURED', false) 

and check that variable in the few places

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jan 28, 2016

Member

@digitalkaoz I've removed your comment, as we can't officially support insecure code snippets.

Member

danielbachhuber commented Jan 28, 2016

@digitalkaoz I've removed your comment, as we can't officially support insecure code snippets.

@digitalkaoz

This comment has been minimized.

Show comment
Hide comment
@digitalkaoz

digitalkaoz Jan 28, 2016

yeah sure, i thought i put enough security disclaimers aside ;) anyway, would love to see this switch implemented

yeah sure, i thought i put enough security disclaimers aside ;) anyway, would love to see this switch implemented

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jan 30, 2016

Member

We'll be splitting out the meta endpoints into a separate plugin.

Member

danielbachhuber commented Jan 30, 2016

We'll be splitting out the meta endpoints into a separate plugin.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@digitalkaoz

This comment has been minimized.

Show comment
Hide comment
@digitalkaoz

digitalkaoz 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:

Closed #1425 #1425.


Reply to this email directly or view it on GitHub
#1425 (comment).

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:

Closed #1425 #1425.


Reply to this email directly or view it on GitHub
#1425 (comment).

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Jan 30, 2016

Member

But its still forbidden to get/save serialized data...so what is exactly
the use case for a separate plugin?

For those who are using the existing endpoints, as the meta endpoints have been removed from this core plugin.

Member

danielbachhuber commented Jan 30, 2016

But its still forbidden to get/save serialized data...so what is exactly
the use case for a separate plugin?

For those who are using the existing endpoints, as the meta endpoints have been removed from this core plugin.

@joelsamuelk

This comment has been minimized.

Show comment
Hide comment
@joelsamuelk

joelsamuelk 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";
} })

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

This comment has been minimized.

Show comment
Hide comment
@joelsamuelk

joelsamuelk May 10, 2016

anyone can help?

anyone can help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment