Block Bindings: Don't show protected fields that are bound to blocks #59326
Conversation
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.5/block-bindings/post-meta.php |
SantosGuillamot
added
[Type] Bug
|
We need to decide what happens when a field is protected. Should we replace the value with an empty string, or should we return the paragraph value without replacing it at all? Any thoughts? <!-- wp:paragraph {"metadata":{"bindings":{"content":{"source":"core/post-meta","args":{"key":"protected"}}}}} -->
<p>Text</p>
<!-- /wp:paragraph -->In this case return "Text" or "". This is why tests are failing, because we were assuming we wanted to return an empty value, and in this case, we are returning the original value. |
| @@ -15,17 +15,30 @@ | |||
| */ | |||
| function gutenberg_block_bindings_post_meta_callback( $source_attrs, $block_instance ) { | |||
| if ( empty( $source_attrs['key'] ) ) { | |||
| return null; | |||
| return ''; | |||
There was a problem hiding this comment.
If the configuration for the block binding is incorrect, then we probably should return null to skip processing. In general, when returning null, the original value will stay in the block's saved HTML:
gutenberg/lib/compat/wordpress-6.5/blocks.php
Lines 202 to 205 in 702f78a
In other cases, we could replace the value with an empty string. It's really hard to tell what is the best way forward.
There was a problem hiding this comment.
I agree. I just made the change for that conditional.
I don't know what we should do for the rest of the use cases, as you say. That's why I shared this comment.
There was a problem hiding this comment.
Feels to me like we should keep the block attribute value as is for all the use-cases where the field is unavailable or protected or anything. I see it as a fallback value.
There was a problem hiding this comment.
I'm not opposed to the idea. It would have certain implications, like we would have to come up with a good strategy for providing the fallback value serialized in the saved block. I raised a very similar concern in #58895 (comment) where the proposed implementation tries to keep in sync the external data with the block attribute. In that case, the fallback would be the last value present in the external source and it would remain the fallback value. So definitely, we should look at it all holistically.
There was a problem hiding this comment.
At the moment for WP 6.5 the fallback value is going to be hardcoded in the HTML manually crafted in the Code Editor, so going with null should be safe as it would mean that the used on the front end sees the fallback value.
There was a problem hiding this comment.
I agree it makes sense to keep it as a fallback value. I'll make the changes to adapt for that.
|
@sc0ttkclark, @lgladdy – what are your thoughts regarding the changes proposed? We are considering a very defensive approach in the initial rollout. |
I think this makes sense... what happens if the meta isn't officially registered though? At the moment, you can access any post meta that's saved, and most of the demos we've seen of folks using this in the the 6.5 beta have relied on that behaviour. As those fields won't be shown in REST, would this prevent you accessing those? |
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
So far, the REST API limitation was already present in the editor, but it is working in the server even if they have We would like to explore how to overcome that limitation in the future. |
SantosGuillamot
requested review from
kevin940726,
Mamaduka,
ntwb,
nerrad and
ajitbohra
as code owners
|
The new tests fail because we still need to decide what should be the fallback value as discussed in this conversation. Right now, it seems the fallback value is the source key, which we might need to review. |
|
Size Change: +1.88 kB (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 8697058. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8068598804
|
|
@SantosGuillamot, do we need to commit the same PHP changes to WordPress core first to make the e2e tests pass? |
I believe that's not the issue with tests. If I am not mistaken, the problem is that, when we add a block with bindings like this, the editor hook replaces the "content" with the placeholder (in this case the meta key). Then, when the post is published, instead of having the initial fallback value, we have the placeholder instead. This comes from the issue you mentioned here. I want to explore a bit more how to manage that. |
There was a problem hiding this comment.
@SantosGuillamot, maybe we updated the failing e2e tests to make them pass in this PR and tackle the fallback issue separately? That is a separate issue in my opinion, and I'm really curious whether it's still going to be the case after the following PR lands:
It makes sense. I just made the change. Indeed, I've been testing it and it seems the refactoring solves the issue 🙌 I'll confirm it once this PR gets merged and we rebase the refactoring. Or the other way around, whichever PR gets merged first. |
Co-authored-by: Pascal Birchler <[email protected]>
SantosGuillamot
deleted the
fix/only-show-non-protected-fields-in-bindings
branch
|
@getdave and @youknowriad, technically speaking, we don't need to cherry-pick these changes to the |
|
I think it would be good to include this in |
|
I've started to see:
If the former label isn't present I'm assuming it's changes to
For anyone coming to this wondering why it's "ok", it's my understanding that it won't matter (i.e. no harm will be done) if these changes get into the Gutenberg
|
…59326) * Check if the meta field is protected * Check if the meta field is available in the REST API * Use `get_registered_meta_keys` function * Return empty string instead of null * Return null if the bindings config is not correct * Return `null` when the field is unavailable or protected * Add tests for protected fields * Update tests to match current behavior * Remove unnecessary `show_in_rest` conditional Co-authored-by: Pascal Birchler <[email protected]> --------- Co-authored-by: Pascal Birchler <[email protected]>
|
I just cherry-picked this PR to the update/packages-6.5-rc1 branch to get it included in the next release: 87a55e2 |
youknowriad
added
Backported to WP Core
…59326) * Check if the meta field is protected * Check if the meta field is available in the REST API * Use `get_registered_meta_keys` function * Return empty string instead of null * Return null if the bindings config is not correct * Return `null` when the field is unavailable or protected * Add tests for protected fields * Update tests to match current behavior * Remove unnecessary `show_in_rest` conditional Co-authored-by: Pascal Birchler <[email protected]> --------- Co-authored-by: Pascal Birchler <[email protected]>
What?
This pull request adds two safety checks to ensure block bindings don't leak private post meta:
Why?
It seems safer to add these limitations to ensure no unwanted data is leaked. And we can explore later how to to loosen these restrictions.
How?
$meta_keysthrough get_registered_meta_keys function to check if the specific key has theshow_in_restproperty enabled.Testing Instructions
Test it doesn't show the protected value when
show_in_restis falseshow_in_restset to false:Test protected custom field
show_in_resttrue but protect it. It can be done adding a_at the beginning of the key or using a filter like this one: