Global styles: Allow indirect properties when unfiltered_html is not allowed #46388

andrewserong · 2022-12-08T00:53:28Z

What?

This PR adds a new INDIRECT_PROPERTIES_METADATA array to the theme JSON class, to store properties metadata for properties that are not directly output by compute_style_properties, but are instead used elsewhere in global styles output.

This array is then used in remove_insecure_styles to ensure that style values that will be used in other cases are validated against an indirect property. The main use case at the moment is for blockGap style values, which are output as either top margins or gap styles, depending on the layout type. For the purposes of validation, though, we can safely validate against a small list of gap properties.

Why?

Over in #45520 it was flagged that the absence of blockGap in PROPERTIES_METADATA meant that the value was being stripped in situations where a user doesn't have the unfiltered_html capability and is saving global styles (e.g. for multisite regular admins). Hopefully, this PR should resolve that.

How?

Add an INDIRECT_PROPERTIES_METADATA array to the theme JSON class.
Validate against this array in remove_insecure_styles.
Add a Gutenberg version of wp_filter_global_styles_post (gutenberg_filter_global_styles_post) so that we can test this change in Gutenberg
If the core versions of wp_filter_global_styles_post callbacks are registered, then de-register, and re-register the Gutenberg versions instead

Testing Instructions

Note: a quick way to test this PR is to place the following in lib/init.php in the Gutenberg plugin, rather than worrying about changing user capabilities:

add_action( 'init', 'kses_init_filters' );

With this PR applied and with a user with unfiltered_html capability (e.g. a regular admin), you should be able to save root block spacing values in global styles, as well as block level spacing (e.g. set Buttons spacing, and axial row/column spacing for Social Icons).
Repeating the above with a user without unfiltered_html capability (or enable the KSES filters with add_action( 'init', 'kses_init_filters' );) you should now still be able to save root block spacing values in global styles, and block level spacing.

Ensure tests pass:

npm run test:unit:php -- --filter WP_Theme_JSON_Gutenberg_Test

Screenshots or screencast

Before (note value is reset when saved)	After (value is saved)

mmtr

Lovely, thanks for working on this!

It tests quite well for me, and the proposed approach makes sense to me. I'd love though to hear more feedback from someone with more experience than me in this area such as @ramonjd or @tellthemachines, since I'm not confident enough to approve this.

I left a couple of minor comments, but to me this is ready to go.

lib/experimental/class-wp-theme-json-gutenberg.php

lib/compat/wordpress-6.2/class-wp-theme-json-6-2.php

andrewserong · 2022-12-09T00:02:04Z

Thanks for giving this a test and for adding your thoughts @mmtr! 🙇

I'd love though to hear more feedback from someone with more experience than me in this area

Good thinking! In that case, I'll just ping @WordPress/gutenberg-core and @scruffian too for visibility to see if there's anyone else with thoughts on the direction to take here, or potential implications on changing the logic here to add the concept of inferred properties in addition to explicit/direct properties that are handled by compute_style_properties.

…perties when unfiltered_html is not allowed

tellthemachines

Testing locally with the add_action( 'init', 'kses_init_filters' ) snippet, this fixes the issue ✅

Just a couple of readability/code organisation points below, otherwise code looks good!

lib/compat/wordpress-6.2/class-wp-theme-json-6-2.php

tellthemachines · 2022-12-11T23:31:45Z

lib/experimental/class-wp-theme-json-gutenberg.php

+
+}
+add_action( 'init', 'gutenberg_override_core_kses_init_filters' );
+add_action( 'set_current_user', 'gutenberg_override_core_kses_init_filters' );


Should these changes be in a kses.php file instead, to mirror core? Or perhaps we should have a file somewhere for changes that are not ever meant to be backported? It's best to keep class files for just the class.

Should these changes be in a kses.php file instead, to mirror core?

Great point, yes! The lib/experimental directory appears to be a good place for things that won't be backported necessarily since it isn't yet in a release, so I think lib/experimental/kses.php might work. I can give that a go 🙂

ramonjd

Looking pretty good! Thanks for working on this.

On a multisite installation I created an admin and tried to set blockGap:

Trunk	This branch

Had a couple of non-blocking questions around naming and tests.

Do we need a dev note for this? I'm thinking yes since it's affecting WP 6.1?

lib/compat/wordpress-6.2/class-wp-theme-json-6-2.php

phpunit/class-wp-theme-json-test.php

andrewserong · 2022-12-12T00:42:40Z

Thanks for the reviews @tellthemachines and @ramonjd! I'll rename the property and move the code around a little bit.

Would we need to cover block-level spacing as well or is that being piped through the same mechanism?

It winds up being piped through the same mechanism. The test currently just looks at the root level — I think to get the block level working in the test, we might need to register blocks, too 🤔. The change in this PR doesn't explicitly look at the nesting / recursive behaviour of remove_insecure_properties so I wasn't sure how much of a deal breaker it might be to not test it explicitly, but happy to dig in a bit further!

Do we need a dev note for this? I'm thinking yes since it's affecting WP 6.1?

Good call. Once this PR lands, I'll open up a corresponding backport PR for core. I think an appropriate time for a dev note could be once the backport PR is approved, in case there's any additional feedback / work required for the core side of things. Let me know if you think a dev note should be put up sooner, though!

ramonjd · 2022-12-12T00:44:26Z

I think an appropriate time for a dev note could be once the backport PR is approved, in case there's any additional feedback / work required for the core side of things. Let me know if you think a dev note should be put up sooner, though!

Sounds good. Cheers!

andrewserong · 2022-12-12T04:12:14Z

Update: I've done the following:

Renamed inferred to indirect
Moved hooks to a new kses.php file
Updated the test to also incorporate block-level spacing and make sure it isn't stripped

I'll leave this PR open overnight just in case anyone else pinged would like to chime in, but otherwise I think we should be able to merge this in tomorrow 🤞

tellthemachines

Changes look good!

andrewserong · 2022-12-12T23:08:17Z

Thanks again for the reviews! Since this PR only updates internal implementation details of the theme JSON class, I'll merge this in now. If there's any follow-up feedback, or if folks would like further changes, we should be able to happily make changes without affecting the API at all.

#46388

…allowed (#46712) ## What? Follow-up of #46388. Ensures that the content and wide width settings set via Global Styles are considered safe and thus output as custom properties on the body element. ## Why? Because secure `layout.contentSize` and `layout.wideSize` settings are being stripped out for users without the `unfiltered_html` capability as part of `remove_insecure_settings`. ## How? - Updates the `INDIRECT_PROPERTIES_METADATA` array introduced in #46388 to allow the `layout.contentSize` and `layout.wideSize` settings. - Modifies the `remove_insecure_settings` to check the `INDIRECT_PROPERTIES_METADATA` array for settings that are not included in `PRESETS_METADATA`.

andrewserong · 2023-02-20T00:12:59Z

Dev note for 6.2

This bug fix is an internal implementation detail of the Theme JSON class, and doesn't really directly affect extenders / people working with theme.json in that no changes are required for themes. So, it probably fits best in a Miscellaneous dev note. Draft below:

In WordPress 6.1 the layout block support was refactored to output CSS values for block spacing, with additional layout controls within global styles being made available. This resulted in a bug for admin users without the unfiltered_html capability (e.g. admin users within a multisite instance) where block spacing or layout content sizes were not saved when updated within the global styles interface. The cause was that within the Theme JSON class, theme.json values are filtered for users without the unfiltered_html capability based on a lookup to a direct CSS property, e.g. margin or padding. For layout features, the styling rules are stored in indirect properties that don't directly map to a real CSS property, e.g. blockGap and contentSize.

For WordPress 6.2 (and backported to WordPress 6.1.2) this issue is resolved by the Theme JSON class storing a list of indirect properties (INDIRECT_PROPERTIES_METADATA), with a mapping between a CSS property to use for validation, and the path to the indirect property as stored in theme.json. In this way, blockGap will be validated against the CSS gap property, and contentSize against the CSS max-width property, etc.

For themes using layout features, no changes are required, as the bug fix is an internal implementation detail of how Theme JSON styles are validated and output.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global styles: Allow indirect properties when unfiltered_html is not allowed #46388

Global styles: Allow indirect properties when unfiltered_html is not allowed #46388

andrewserong commented Dec 8, 2022 •

edited

Loading

mmtr left a comment

andrewserong commented Dec 9, 2022

tellthemachines left a comment

tellthemachines Dec 11, 2022

andrewserong Dec 12, 2022

ramonjd left a comment

andrewserong commented Dec 12, 2022

ramonjd commented Dec 12, 2022

andrewserong commented Dec 12, 2022 •

edited

Loading

tellthemachines left a comment

andrewserong commented Dec 12, 2022

andrewserong commented Feb 20, 2023

Global styles: Allow indirect properties when unfiltered_html is not allowed #46388

Global styles: Allow indirect properties when unfiltered_html is not allowed #46388

Conversation

andrewserong commented Dec 8, 2022 • edited Loading

What?

Why?

How?

Testing Instructions

Screenshots or screencast

mmtr left a comment

Choose a reason for hiding this comment

andrewserong commented Dec 9, 2022

tellthemachines left a comment

Choose a reason for hiding this comment

tellthemachines Dec 11, 2022

Choose a reason for hiding this comment

andrewserong Dec 12, 2022

Choose a reason for hiding this comment

ramonjd left a comment

Choose a reason for hiding this comment

andrewserong commented Dec 12, 2022

ramonjd commented Dec 12, 2022

andrewserong commented Dec 12, 2022 • edited Loading

tellthemachines left a comment

Choose a reason for hiding this comment

andrewserong commented Dec 12, 2022

andrewserong commented Feb 20, 2023

Dev note for 6.2

Further reading

andrewserong commented Dec 8, 2022 •

edited

Loading

andrewserong commented Dec 12, 2022 •

edited

Loading