Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Section Styles: Fix insecure properties removal for inner block types and elements #66896

Conversation

aaronrobertshaw
Copy link
Contributor

Fixes: #66799

What?

Fixes an issue where block style variations containing inner block type and element styles would have those inner styles stripped when the user attempting to save Global Styles does not have the unfiltered_html capability.

Why?

The bug prevents proper saving of Global Styles with block style variations containing inner block/element styles when on multisite setups. This leads to unexpected styling when switching style variations etc.

How?

Extends the remove_insecure_properties function to process the inner block type styles for variations including their nested element styles.

Testing Instructions

Setup

To test this fix, we need a theme that defines block style variations with inner block type styles. The easiest approach is to use an existing theme that has already done all that config. The catch with the below theme is that it uses color-mix styles that needs to be added to the allowed safe css atts (this is being handled separately).

  1. Create a local multisite install.
    • Alternatively, set up a user that doesn't have unfiltered_html caps but can access the site editor. See linked issue for options.
  2. Install and activate the Assembler theme.
  3. Create a page or template using the following block markup:
Block markup with section style applied
<!-- wp:group {"tagName":"main","metadata":{"name":"Main"},"className":"is-style-section-1","style":{"spacing":{"blockGap":"0","margin":{"top":"0"}}},"layout":{"type":"default"}} -->
<main class="wp-block-group is-style-section-1" style="margin-top:0"><!-- wp:cover {"url":"https://richptabor64.wordpress.com/wp-content/uploads/2024/11/art.png","id":13691,"alt":"art","dimRatio":50,"customOverlayColor":"#9cb1ab","isUserOverlayColor":false,"className":"alignfull is-style-default","style":{"spacing":{"padding":{"right":"var:preset|spacing|50","left":"var:preset|spacing|50"},"margin":{"top":"0","bottom":"0"}},"color":{"text":"#000000"}},"layout":{"type":"default"}} -->
<div class="wp-block-cover alignfull is-style-default has-text-color" style="color:#000000;margin-top:0;margin-bottom:0;padding-right:var(--wp--preset--spacing--50);padding-left:var(--wp--preset--spacing--50)"><img class="wp-block-cover__image-background wp-image-13691" alt="art" src="https://richptabor64.wordpress.com/wp-content/uploads/2024/11/art.png" data-object-fit="cover" /><span aria-hidden="true" class="wp-block-cover__background has-background-dim" style="background-color:#9cb1ab"></span><div class="wp-block-cover__inner-container"><!-- wp:spacer {"height":"var:preset|spacing|70"} -->
<div style="height:var(--wp--preset--spacing--70)" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->

<!-- wp:group {"align":"wide","style":{"spacing":{"blockGap":"0"}},"layout":{"type":"constrained","contentSize":"1200px"}} -->
<div class="wp-block-group alignwide"><!-- wp:heading {"textAlign":"center","className":"text-balance","style":{"spacing":{"margin":{"top":"0","bottom":"0","left":"0","right":"0"}}},"fontSize":"xx-large"} -->
<h2 class="wp-block-heading has-text-align-center text-balance has-xx-large-font-size" style="margin-top:0;margin-right:0;margin-bottom:0;margin-left:0">Experience Abstract Art</h2>
<!-- /wp:heading -->

<!-- wp:spacer {"height":"var:preset|spacing|40"} -->
<div style="height:var(--wp--preset--spacing--40)" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->

<!-- wp:buttons {"layout":{"type":"flex","flexWrap":"wrap","justifyContent":"center"}} -->
<div class="wp-block-buttons"><!-- wp:button {"width":50} -->
<div class="wp-block-button has-custom-width wp-block-button__width-50"><a class="wp-block-button__link wp-element-button" href="#contact-section">Get in touch</a></div>
<!-- /wp:button --></div>
<!-- /wp:buttons --></div>
<!-- /wp:group -->

<!-- wp:separator -->
<hr class="wp-block-separator has-alpha-channel-opacity" />
<!-- /wp:separator -->

<!-- wp:spacer {"height":"var:preset|spacing|70"} -->
<div style="height:var(--wp--preset--spacing--70)" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer --></div></div>
<!-- /wp:cover --></main>
<!-- /wp:group -->
  1. Update kses.php in your local WP instance to allow color-mix style values.
diff --git a/src/wp-includes/kses.php b/src/wp-includes/kses.php
index cd30d845f8..e64b5a615a 100644
--- a/src/wp-includes/kses.php
+++ b/src/wp-includes/kses.php
@@ -2655,7 +2655,7 @@ function safecss_filter_attr( $css, $deprecated = '' ) {
 			 * Nested functions and parentheses are also removed, so long as the parentheses are balanced.
 			 */
 			$css_test_string = preg_replace(
-				'/\b(?:var|calc|min|max|minmax|clamp|repeat)(\((?:[^()]|(?1))*\))/',
+				'/\b(?:var|calc|min|max|minmax|clamp|repeat|color-mix)(\((?:[^()]|(?1))*\))/',
 				'',
 				$css_test_string
 			);

Steps

Easy

  1. Make sure the theme.json unit tests are passing:
    • npm run test:unit:php:base -- --filter WP_Theme_JSON_Gutenberg_Test

Manual

On trunk:

  1. Login with a user lacking the unfiltered_html caps and visit the site editor
  2. Open the Global Styles sidebar panel and choose Browse Styles
  3. Select the Noir style variation
  4. Save the changes
  5. Confirm the button elements hover styles match between editor and frontend
  6. Now inspect the separator below the button on the frontend. Note the lack of section styles.
    • Screenshot 2024-11-11 at 1 10 08 pm

This PR:

  1. Reset global styles

  2. Repeat the steps for trunk

  3. Note that the separator's styles on the frontend now include the correct section style

    • Screenshot 2024-11-11 at 1 12 05 pm
  4. Bonus points: If the unit tests aren't enough; add some debugging to log the variation input/output in remove_insecure_properties and validate the expected output is shown in error logs when attempting to save Global Styles.

Quick and dirty debug log diff
diff --git a/lib/class-wp-theme-json-gutenberg.php b/lib/class-wp-theme-json-gutenberg.php
index 7127628b63..00c64ec06e 100644
--- a/lib/class-wp-theme-json-gutenberg.php
+++ b/lib/class-wp-theme-json-gutenberg.php
@@ -3563,6 +3563,10 @@ class WP_Theme_JSON_Gutenberg {
 						continue;
 					}
 
+					$debug_variations = true;
+					if ( $debug_variations && $metadata['name'] === 'core/group' ) {
+						error_log( json_encode( $variation_input, JSON_PRETTY_PRINT ) );
+					}
 					$variation_output = static::remove_insecure_styles( $variation_input );
 
 					if ( isset( $variation_input['blocks'] ) ) {
@@ -3573,6 +3577,11 @@ class WP_Theme_JSON_Gutenberg {
 						$variation_output['elements'] = static::remove_insecure_element_styles( $variation_input['elements'] );
 					}
 
+					if ( $debug_variations && $metadata['name'] === 'core/group' ) {
+						error_log( json_encode( $variation_output, JSON_PRETTY_PRINT ) );
+						exit();
+					}
+
 					if ( ! empty( $variation_output ) ) {
 						_wp_array_set( $sanitized, $variation['path'], $variation_output );
 					}

Screenshots or screencast

Before After
Screenshot 2024-11-11 at 1 10 08 pm Screenshot 2024-11-11 at 1 12 05 pm

@aaronrobertshaw aaronrobertshaw added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Feature] Block Style Variations Issues or PRs that are related to the style variations for blocks labels Nov 11, 2024
@aaronrobertshaw aaronrobertshaw self-assigned this Nov 11, 2024
Copy link

github-actions bot commented Nov 11, 2024

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.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
Co-authored-by: BogdanUngureanu <bogdanungureanu@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>

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

@aaronrobertshaw
Copy link
Contributor Author

@BogdanUngureanu if you have some time to test this one and make sure it is actually fixing the issue for you, that would be awesome, cheers 🙏

@aaronrobertshaw
Copy link
Contributor Author

This bug fix might be a candidate for a 6.7.1 point release. In the meantime, I'll get the backport sorted later today.

@andrewserong andrewserong requested a review from talldan November 11, 2024 03:46
@aaronrobertshaw aaronrobertshaw force-pushed the fix/insecure-properties-removal-for-block-style-variations branch from 35dcba4 to 0f006ca Compare November 11, 2024 04:22
Copy link

Flaky tests detected in 0f006ca.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11772082349
📝 Reported issues:

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great description @aaronrobertshaw, thanks for all the extra context here! This is testing nicely for me:

✅ Confirmed the issue on trunk with my local environment updated to allow color-mix and with the admin user without the unfiltered_html capability
✅ Confirmed that with this PR applied, the section styles are output correctly
✅ The added functions improve readability while also allowing for re-use of the elements logic

Also, I like the comments in the tests that make it clear which bits shouldn't appear in the results 👍

Trunk (missing section styles) This PR
image image

LGTM! 🚀

@BogdanUngureanu
Copy link
Contributor

Works like a charm for me too, thanks!

@ramonjd
Copy link
Member

ramonjd commented Nov 11, 2024

For some reason I opted to test this locally with a custom theme. Anyway, here's what I eventually did:

  1. Disallowed unfiltered_html for my admin user.
  2. Added my own block style variation theme.json with block types and inner elements. all using the color-mix values.
  3. Edited a template in the site editor
  4. Created some markup corresponding to the block style variation tree. Applied the variation to the block.
  5. Saved the template.
  6. The values were stripped.
  7. After applying this PR, I updated kses.php to accept color-mix
  8. The styles are correctly rendered 🎉

@aaronrobertshaw aaronrobertshaw merged commit caf9d0b into trunk Nov 11, 2024
64 checks passed
@aaronrobertshaw aaronrobertshaw deleted the fix/insecure-properties-removal-for-block-style-variations branch November 11, 2024 23:47
@github-actions github-actions bot added this to the Gutenberg 19.7 milestone Nov 11, 2024
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
… and elements (WordPress#66896)

Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
Co-authored-by: BogdanUngureanu <bogdanungureanu@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Style Variations Issues or PRs that are related to the style variations for blocks Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Global Styles: unable to save block some variation styles when KSES is active
4 participants