Block Bindings: Don't show protected fields that are bound to blocks and post meta

[santosguillamot]

After more testing in the block bindings API, it might make sense to add some limitations for the blocks connected to post meta before it is included in 6.5. This means that fields that are protected or are not shown in the REST API shouldn't be shown in this initial version even if they are bound to blocks. This way, it ensures no unwanted data is leaked. It can be explored in a later phase how to loosen these restrictions.

Related changes proposed are also in the Gutenberg plugin: ​https://github.com/WordPress/gutenberg/pull/59326

As ​discussed in Gutenberg plugin, this pull request adds two safety checks to ensure block bindings don't leak private post meta:

• Check if the meta field is protected.
• Check if the meta field is available through the REST API.

It seems safer to add these limitations to ensure no unwanted data is leaked. And it can be explored later how to loosen these restrictions.

To do that this pull request:

• Use the ​is_protected_meta function to check if it is protected.
• Access the $meta_keys through ​get_registered_meta_keys function to check if the specific key has the show_in_rest property enabled.

## Testing Instructions

Test it doesn't show the protected value when show_in_rest is false

1. Register a custom field with show_in_rest set to false:

{{{PHP
register_meta(

'post',
'protected',
array(

'show_in_rest' => false,
'single' => true,
'type' => 'string',
'default' => 'Protected value',

)

);
}}}

3. Add a paragraph block in a page pointing to the protected block:

Text

4. Save the page, go to the front and check it doesn't show the protected value.

Test protected custom field

1. Change the register source to show_in_rest true but protect it. It can be done adding a _ at the beginning of the key or using a filter like this one:

function protect_meta( $protected, $meta_key, $meta_type ) {
        return true;
}
add_filter( 'is_protected_meta', 'protect_meta', 10, 3 );

2. Check that the paragraph bound to the protected field doesn't show the protected value in the frontend.

---

Trac ticket: https://core.trac.wordpress.org/ticket/60651

[swissspidy]

Since you are proposing this for 6.5 I am tentatively moving this to the milestone for visibility. I'll also flag this to the security team.

Given the risk of exposing protected data, this should really have unit tests.

As a follow-up during the RC phase, we should figure out how to cover the meta source with unit tests that prevent future regressions.

I would appreciate unit tests as part of this change

I saw the comment on Trac and I agree with the reasoning. We will work on tests as soon as possible.

Adding tests totally makes sense to me. Thanks for pointing that out.

I've added some tests for the post meta source, covering the protected values, in ​this commit. I will take a deeper look tomorrow. Let me know any feedback 🙂

We also need to cover the following cases:

• post is password protected
• other reasons where the post can be viewable for the user

We also need to cover the following cases:

• post is password protected
• other reasons where the post can be viewable for the user

I wasn't sure how to handle these cases. I added two new tests for this using filters. Let me know if that's not correct.

• ​Post is password protected
• ​Post is not viewable

In ​this commit, I've refactored the way the post content is updated because it seems not all tests were getting the correct post context.

I'd appreciate a new review on this one. I'm really sorry for all the changes, I just realized after trying to add more tests.

The block bindings API relies on the HTML API, which takes care of sanitizing the unsafe HTML.

That's only true for HTML attributes, as outlined in ​https://github.com/WordPress/wordpress-develop/pull/6197#discussion_r1507348594. For inner HTML, there is wp_kses_post call that takes care of it. It also made me realize that we need tests for both the attribute and inner HTML being connected to post meta.

It'd probably good to add other tests that do the following:

• Test for post meta keys that aren't registered (no register_meta call) to make sure any random post meta key is not exposed by default.
• Tests for meta values that might have unsafe HTML, are they sanitised?

I've added a test for meta keys that aren't registered ​here.

I've added a test to check unsafe HTML ​here. _EDIT: I'll take a look at ​this._

The block bindings API relies on the HTML API, which takes care of sanitizing the unsafe HTML.

Let me know if that is what you have in mind.

It also made me realize that we need tests for both the attribute and inner HTML being connected to post meta.

I assumed that if the attributes or the HTML are replaced properly should be covered for all sources in the binding render tests, right?

Do you think that having just one test specific for the post meta like ​this one would be enough or should we replicate all the tests?

Do you think that having just one test specific for the post meta like ​this one would be enough or should we replicate all the tests?

I hope it's enough because block rending covers the same functionality.

Do you think that having just one test specific for the post meta like ​this one would be enough or should we replicate all the tests?

I hope it's enough because block rending covers the same functionality.

I added a new test for bindings modifying an HTML attribute like the image src.

[swissspidy]

In 57754:

Editor: do not expose protected post meta fields in block bindings.

Ignores meta keys which are considered protected or not registered to be shown in the REST API. Adds tests.

Props santosguillamot, swissspidy, gziolo, xknown, peterwilsoncc.
Fixes #60651.

Committed in https://core.trac.wordpress.org/changeset/57754