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

[SantosGuillamot]

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:

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:

<!-- wp:paragraph {"metadata":{"bindings":{"content":{"source":"core/post-meta","args":{"key":"protected"}}}}} -->
<p>Text</p>
<!-- /wp:paragraph -->

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

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props santosguillamot, swissspidy, gziolo, xknown, peterwilsoncc.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[gziolo]

That looks good, as discussed in the original PR in the Gutenberg repository. The approach taken might be to defensive, but we prefer to stay on the safe side and open it later if that makes more sense rather than take any risk something goes wrong.

[gziolo]

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.

[swissspidy]

I would appreciate unit tests as part of this change

[gziolo]

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

[SantosGuillamot]

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 🙂

[gziolo]

We also need to cover the following cases:

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

[SantosGuillamot]

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

[xknown]

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?

[peterwilsoncc]

A couple of notes for the tests.

I think this is a sensible change.

[SantosGuillamot]

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.

[SantosGuillamot]

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.

[gziolo]

There isn't much value in having that code extracted from its method since it's used only once. It would make the code easier to follow if it was inlined.

You could also avoid adding inline comments by making the code structured like that:

global $post;
$post = self::$post;
$post->post_content = $content;
return apply_filters( 'the_content', self::$post->post_content );

[swissspidy]

The self::$post variable is useless anyway because the whole class only operates on the global post.

We can just always work on the global post if that is required.

[SantosGuillamot]

I've tried using the global $post variable only for all the tests, but something is not working as I expected.

It seems that the value of the global $post variable is undefined (or the value I defined in wpSetUpBeforeClass) for the FIRST tests. For the following tests, the global $post variable becomes always null unless I redefine it.

This happens even if I remove all the logic and I just access that variable.

The way I'm solving that is storing the $post object in a private property when the post is initialized: link. And for each test I assign the global variable to that: link.

However, this feels like a workaround. Any idea if that is expected or how it could be solved?

[gziolo]

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 #6197 (comment). 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.

[swissspidy]

The self::$post variable is useless anyway because the whole class only operates on the global post.

We can just always work on the global post if that is required.

[swissspidy]

As per my other comment, the static $post variable is redundant if you make it global anyway.

[SantosGuillamot]

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?

[gziolo]

We should also add another test for a Button block where we put something tricky as the link's URL. Example: javascript:alert('Unsafe HTML'). I'm not sure if that is the best one, but that's definitely how someone could run JS by updating the dynamic source.

[SantosGuillamot]

I'm wondering what if we should handle that case in the bindings because passing javascript:alert('1') to the href it is actually valid HTML. We would only be consuming the value stored in the custom field and exposed in the REST API. Not sure if it should be checked in the block bindings API, as part of the meta fields registration, or as part of the button block.

[gziolo]

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.

[SantosGuillamot]

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

[swissspidy]

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