Make WordPress Core

Opened 7 weeks ago

Closed 7 weeks ago

Last modified 6 weeks ago

#60651 closed defect (bug) (fixed)

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

Reported by: santosguillamot's profile santosguillamot Owned by: swissspidy's profile swissspidy
Milestone: 6.5 Priority: high
Severity: normal Version:
Component: Editor Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

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

Change History (20)

This ticket was mentioned in PR #6197 on WordPress/wordpress-develop by @santosguillamot.


7 weeks ago
#1

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:

## 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',

)

);
}}}

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

Text

  1. 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 );
  1. 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

#2 @swissspidy
7 weeks ago

  • Component changed from Security to Editor
  • Keywords needs-unit-tests added

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.

#3 @johnbillion
7 weeks ago

  • Milestone changed from Awaiting Review to 6.5

@gziolo commented on PR #6197:


7 weeks ago
#4

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 commented on PR #6197:


7 weeks ago
#5

I would appreciate unit tests as part of this change

@gziolo commented on PR #6197:


7 weeks ago
#6

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

@santosguillamot commented on PR #6197:


7 weeks ago
#7

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 commented on PR #6197:


7 weeks ago
#8

We also need to cover the following cases:

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

@santosguillamot commented on PR #6197:


7 weeks ago
#9

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.

@santosguillamot commented on PR #6197:


7 weeks ago
#10

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.

@gziolo commented on PR #6197:


7 weeks ago
#11

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.

@santosguillamot commented on PR #6197:


7 weeks ago
#12

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.

@santosguillamot commented on PR #6197:


7 weeks ago
#13

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 commented on PR #6197:


7 weeks ago
#14

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.

@gziolo commented on PR #6197:


7 weeks ago
#15

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 commented on PR #6197:


7 weeks ago
#16

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

#17 @swissspidy
7 weeks ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Priority changed from normal to high

#18 @swissspidy
7 weeks ago

  • Owner set to swissspidy
  • Resolution set to fixed
  • Status changed from new to closed

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.

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


6 weeks ago

Note: See TracTickets for help on using tickets.