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

Preserve block style variations when securing theme json #53466

Merged
merged 2 commits into from
Aug 11, 2023

Conversation

dsas
Copy link
Contributor

@dsas dsas commented Aug 9, 2023

What?

Preserves global styles block variations when securing styles.

Why?

Global Styles block variations were being removed for users whose capabilities dictated that their styles changes needed securing, despite not actually being insecure. When this was a problem varied depending upon site configuration, but out-of-the-box it was a problem for administrators on multi-site installs.

Valid and safe block style variations were being removed by WP_Theme_JSON_Gutenberg::remove_insecure_properties when securing styles.

Closes #53428

How?

Adds explicit processing of variations in remove_insecure_properties so that they are preserved not removed.

Testing Instructions

  1. Activate latest Gutenberg on a multisite install, or test with a user not having the unfiltered_html capability, or enable the KSES filters with add_action( 'init', 'kses_init_filters' );
  2. Go to Appearance > Editor
  3. Open the Styles sidebar
  4. Select "Blocks"
  5. Select "Button"
  6. Select the Outline style variation
  7. Change a property away from the initial setting, e.g. set the background
  8. Save the changes
  9. ⚠️ Note how the changes are reverted

Apply this PR and try again - note how the changes are now not reverted.

Testing Instructions for Keyboard

N/A

Screenshots or screencast

@dsas dsas requested a review from spacedmonkey as a code owner August 9, 2023 10:09
@dsas dsas self-assigned this Aug 9, 2023
@dsas dsas added the [Type] Bug An existing feature does not function as intended label Aug 9, 2023
Valid and safe block style variations were being removed by
`WP_Theme_JSON_Gutenberg::remove_insecure_properties` when securing the
theme.json. When this was a problem varied depending upon site
configuration, but out-of-the-box it was a problem for administrators on
multi-site installs.

This change adds explicit processing of variations in
`remove_insecure_properties` so that they won't get removed.
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.

This is testing great for me, thanks for following up with the PR and for the thorough testing instructions!

✅ Confirmed the issue on trunk when running without unfiltered_html capability
✅ Confirmed this fixes the issue and I'm able to save styling changes to a variation within the site editor
✅ Logic change looks good to me and is consistent with the rest of the code in remove_insecure_properties 👍

Just left an optional comment for the test about whether we should also check that invalid properties for the variation are stripped, but otherwise this LGTM! ✨

'variations' => array(
'outline' => array(
'color' => array(
'background' => 'purple',
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker, but to be extra thorough about the test, would it be worth adding an invalid property to the outline array, to test that it correctly gets removed by the call to remove_insecure_styles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cheers @andrewserong I've added another test for that

This test checks that when removing insecure properties an
unknown/unsupported property is removed from the variation.
@andrewserong andrewserong added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Aug 11, 2023
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.

Thanks for the update, the additional test covers things nicely! LGTM, I'll merge this in now 🙂

@andrewserong andrewserong merged commit 84adb3c into WordPress:trunk Aug 11, 2023
48 of 49 checks passed
@github-actions github-actions bot added this to the Gutenberg 16.5 milestone Aug 11, 2023
@andrewserong andrewserong added Backport to Gutenberg Minor Release Pull request that needs to be backported to a Gutenberg minor release Needs PHP backport Needs PHP backport to Core Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release and removed Backport to Gutenberg Minor Release Pull request that needs to be backported to a Gutenberg minor release labels Aug 11, 2023
tellthemachines pushed a commit that referenced this pull request Aug 14, 2023
* Preserve block style variations when securing theme json

Valid and safe block style variations were being removed by
`WP_Theme_JSON_Gutenberg::remove_insecure_properties` when securing the
theme.json. When this was a problem varied depending upon site
configuration, but out-of-the-box it was a problem for administrators on
multi-site installs.

This change adds explicit processing of variations in
`remove_insecure_properties` so that they won't get removed.

* Add another variation sanitisation test

This test checks that when removing insecure properties an
unknown/unsupported property is removed from the variation.
@tellthemachines
Copy link
Contributor

I just cherry-picked this PR to the update/bugfixes-6-3-1 branch to get it included in the next release: ee2786a

@tellthemachines tellthemachines removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Aug 14, 2023
@ramonjd
Copy link
Member

ramonjd commented Aug 15, 2023

Hi @dsas

These changes will need to be synced with Core. Are you okay to do that? No worries if not. Let us know and I can take care of it.

If you want to take it on, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository.

Thank you! ❤️

@dsas
Copy link
Contributor Author

dsas commented Aug 15, 2023

Thanks @ramonjd I've opened WordPress/wordpress-develop#5013, I haven't done it before so I hope I've got everything right!

tellthemachines pushed a commit that referenced this pull request Aug 31, 2023
* Preserve block style variations when securing theme json

Valid and safe block style variations were being removed by
`WP_Theme_JSON_Gutenberg::remove_insecure_properties` when securing the
theme.json. When this was a problem varied depending upon site
configuration, but out-of-the-box it was a problem for administrators on
multi-site installs.

This change adds explicit processing of variations in
`remove_insecure_properties` so that they won't get removed.

* Add another variation sanitisation test

This test checks that when removing insecure properties an
unknown/unsupported property is removed from the variation.
tellthemachines added a commit that referenced this pull request Sep 1, 2023
* Update document title buttons radius (#53221)

* Fix: Sync status overlaps for some languages in Patterns post type page (#53243)

* Image block: Fix stretched images constrained by max-width (#53274)

* Fix dragging to resize from stretching image in the frontend

* Add image block v7 deprecation

* Update deprecation comment

* Prevent image losing its aspect ratio at smaller window dimensions

* Revert "Prevent image losing its aspect ratio at smaller window dimensions"

This reverts commit 8ac9f8c.

---------

Co-authored-by: scruffian <ben@scruffian.com>

* Image Block: Don't render `DimensionsTool` if it is not resizable (#53181)

* Fix missing Replace button in content-locked Image blocks (#53410)

* Revert "don't display BlockContextualToolbar at all in contentonly locking (#53110)"

This reverts commit 5efce0e.

* Alternative fix to hide BlockContextualToolbar when there are no controls

* fix the go to for non pages by showing it only for pages (#53408)

* Site Editor: Fix document actions label helper method (#52974)

* Site Editor: Fix document actions label helper method
* Add missing useSelect dep
* Fix e2e tests
* Move the label map at the file level to avoid recreating the object on every render

* Image: Clear aspect ratio when wide aligned (#53439)

* RichText: Remove 'Footnotes' when interactive formatting is disabled (#53474)

Introduce a new 'interactive' setting for format types

* Preserve block style variations when securing theme json (#53466)

* Preserve block style variations when securing theme json

Valid and safe block style variations were being removed by
`WP_Theme_JSON_Gutenberg::remove_insecure_properties` when securing the
theme.json. When this was a problem varied depending upon site
configuration, but out-of-the-box it was a problem for administrators on
multi-site installs.

This change adds explicit processing of variations in
`remove_insecure_properties` so that they won't get removed.

* Add another variation sanitisation test

This test checks that when removing insecure properties an
unknown/unsupported property is removed from the variation.

* Site editor: add missing i18n in `HomeTemplateDetails` (#53543)

* Site editor: add missing i18n in `HomeTemplateDetails`

* Lint fix

* Fix: Snack bar not fixed on certain pages in the Site Editor (#53207)

* Fix document title alignment in command palette button (#53224)

* Fallback to default max viewport if layout wide size is fluid. (#53551)

* Link Control: persist advanced settings toggle state to preferences if available (#52799)

* Link Control: Create a preference for whether the advanced section is open

* move the useSelect to the component that uses it

* Supply preference setter via settings

* Pass setter to Post Editor

* Provide local state fallbacks in absence of preference store settings

* Conditionalise display of settings drawer to “edit” mode only

* Extract to constant to improve comprehension

* Fix bug in preferences resolution

* Improve comments

* Add e2e scaffold

* Fix e2e test to correctly assert on feature

* Remove focused test

* Reinstate original logic to hide settings when not required

* Scaffold documentation

* Revert providing prefs via settings

* Refactor to use `core/block-editor` as preference scope

* Update docs

* Reinstate remaining original conditional

* tentative fix for the e2e test

* Try a different syntax for shiftAlt

* another attempt to fix the e2e test

---------

Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: MaggieCabrera <maggie.cabrera@automattic.com>

* Add tests for fluid layout + typography (#53554)

* Fix support of sticky position in non-iframed post editor (#53540)

* Fix support of sticky position in non-iframed post editor

* Revert "Footnotes: Fix recursion into updating attributes when attributes is not an object (#53257)"

This reverts commit ab04074.

* Fix: indicator style when block moving mode (#53972)

* Fix post editor top toolbar with custom fields in Safari (#53688)

* Set top toolbar size dynamically (#53526)

* fix the top toolbar size in the space remaining after plugin items are pinned

* only resize above the tablet breakpoint

* fix fixed block toolbar collapse button when icon labels are shown

* Update height and scroll behavior

* move the layout effect to the affected component, fixes for fullscreen, no block selected, icon labels height

---------

Co-authored-by: jasmussen <joen@automattic.com>

* Roll back camelCase change in 96b6b1e

---------

Co-authored-by: James Koster <james@jameskoster.co.uk>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Alex Lende <alex+github.com@lende.xyz>
Co-authored-by: scruffian <ben@scruffian.com>
Co-authored-by: Robert Anderson <robert@noisysocks.com>
Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
Co-authored-by: Dean Sas <dean.sas@automattic.com>
Co-authored-by: Pascal Birchler <pascalb@google.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: MaggieCabrera <maggie.cabrera@automattic.com>
Co-authored-by: Mitchell Austin <mr.fye@oneandthesame.net>
Co-authored-by: jasmussen <joen@automattic.com>
Co-authored-by: ramon <ramonjd@gmail.com>
@tellthemachines tellthemachines removed the Needs PHP backport Needs PHP backport to Core label Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 variations when KSES is active
4 participants