Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#57321 closed defect (bug) (fixed)

Global Styles: Block spacing and content/wide layout values are not saved if user does not have unfiltered_html capability

Reported by: andrewserong Owned by: peterwilsoncc
Priority: normal Milestone: 6.2
Component: Editor Version: 6.1
Severity: normal Keywords: has-patch has-unit-tests gutenberg-merge commit fixed-major dev-reviewed
Cc: Focuses:

Description

This issue was already raised in Gutenberg over in: โ€‹https://github.com/WordPress/gutenberg/issues/45520. A fix has landed in Gutenberg in โ€‹https://github.com/WordPress/gutenberg/pull/46388. This trac issue exists to ensure that the fix is backported to core.

To recap: for users without the unfiltered_html capability (e.g. an Admin user within a multi site WordPress instance), when saving custom block spacing values within global styles, the value is stripped on save.

This is due to the blockGap value not being factored in within the theme JSON class's remove_insecure_styles function, which currently only validates styles that are output via compute_style_properties.

The proposed fix that landed in Gutenberg is to also support indirect values in remove_insecure_styles, that is โ€” values that will be output separately to compute_style_properties. For these values, a list of approved CSS properties is included, to be used for testing whether or not a value is allowed.

Change History (25)

This ticket was mentioned in โ€‹PR #3742 on โ€‹WordPress/wordpress-develop by โ€‹@andrewserong.


4 years ago
#1

  • Keywords has-patch has-unit-tests added

This PR backports โ€‹https://github.com/WordPress/gutenberg/pull/46388 which resolved โ€‹https://github.com/WordPress/gutenberg/issues/45520 in Gutenberg.

To recap: for users without the unfiltered_html capability (e.g. an Admin user within a multi site WordPress instance), when saving custom block spacing values within global styles, the value is stripped on save. The fix is to ensure that indirect properties (that is, Theme JSON properties that are not directly output via compute_style_properties within `remove_insecure_styles) are also allowed.

For more context, please see: โ€‹https://github.com/WordPress/gutenberg/pull/46388

Trac ticket: https://core.trac.wordpress.org/ticket/57321

## Testing instructions

  1. To quickly test, add the following somewhere (e.g. at the bottom of src/wp-includes/default-filters.php): add_action( 'init', 'kses_init_filters' ); โ€” this switching on the kses filters used when a user does not have the unfiltered_html capability.
  2. With a blocks theme active, go to global styles within the site editor, and adjust block spacing within layout. When you go to save a value, without this PR, the value will revert to what it was when you originally opened the site editor.
  3. With this PR applied, the value should save correctly.
  4. You can repeat this process at the block level in global styles, with a block that supports block spacing/ block gap, e.g. Social Icons/Links.

#2 @andrewserong
4 years ago

I've opened up a PR to backport the Gutenberg fix. Also, just a note that it'd be great if this fix can make it into 6.1.2 rather than waiting for 6.2, since it's an unfortunate bug for admins running multi-site WordPress instances.

โ€‹@andrewserong commented on โ€‹PR #3742:


4 years ago
#3

Thanks for the feedback @costdev, all good notes! I've implemented those changes. I'm wrapping up for the day, but happy to do any other follow-ups tomorrow (and of course, anyone with access, feel free to jump in and make any changes you might need ๐Ÿ˜€)

โ€‹@andrewserong commented on โ€‹PR #3742:


4 years ago
#4

Just an update that I'm wrapping up for the year now, so likely won't catch continued discussion here. If this change looks suitable, though, please feel free to go ahead and commit it if anyone would like to. I think ideally this would be a good one to make it into 6.1.2 if it can, rather than waiting until 6.2. Thanks again for the reviews!

โ€‹mmtr commented on โ€‹PR #3742:


4 years ago
#5

Just a heads up that I just merged โ€‹https://github.com/WordPress/gutenberg/pull/46712 in Gutenberg which is a follow-up of the changes backported here, so I think we could use this PR to include the follow-up as well.

โ€‹@peterwilsoncc commented on โ€‹PR #3742:


4 years ago
#6

That makes sense @mmtr

I've dismissed my review pending any follow up. I'm not sure when Andrew returns from EOY holidays so someone else may need to create a follow up PR with both issues.

โ€‹@andrewserong commented on โ€‹PR #3742:


4 years ago
#7

Thanks for the heads-up @mmtr and Peter! I'm back today, so I'll roll those changes into this PR โ€” if I don't get to it today, I'll fix it up tomorrow.

โ€‹@andrewserong commented on โ€‹PR #3742:


4 years ago
#8

Alrighty, I've merged in the changes from โ€‹https://github.com/WordPress/gutenberg/pull/46712 and re-tested locally, and it appears to all be working well for me, so this should be ready for another look now.

#9 @andrewserong
4 years ago

  • Summary Global Styles: Block spacing values are not saved if user does not have unfiltered_html capability โ†’ Global Styles: Block spacing and content/wide layout values are not saved if user does not have unfiltered_html capability

โ€‹mmtr commented on โ€‹PR #3742:


4 years ago
#10

Thanks @andrewserong for backporting โ€‹https://github.com/WordPress/gutenberg/pull/46712 as well!

#11 @hellofromTonya
4 years ago

  • Keywords gutenberg-merge commit added
  • Milestone Awaiting Review โ†’ 6.1.2
  • Version trunk โ†’ 6.1

I see @peterwilsoncc approved the PR. I pushed a change to simplify the code.

Marking for commit and moving it into 6.1.2 milestone. If it's to be in 6.2 and not 6.1.2, please update the milestone.

โ€‹@andrewserong commented on โ€‹PR #3742:


4 years ago
#12

Thanks for tidying this one up @hellofromtonya! Because this resolves a bug in 6.1, the hope was that this could make it into 6.1.2 (and therefore also 6.2).

โ€‹@peterwilsoncc commented on โ€‹PR #3742:


4 years ago
#13

@andrewserong @hellofromtonya If the bug, or relevant block, was introduced in 6.1 then I'm happy for this to go in the next minor release too.

โ€‹@andrewserong commented on โ€‹PR #3742:


4 years ago
#14

@Mamaduka @ntsekouras just double-checking the status of this one being included in 6.2? Ideally the fix should land for both 6.1.2 and 6.2.

โ€‹@Mamaduka commented on โ€‹PR #3742:


4 years ago
#15

@andrewserong, I believe the fix is merged into the trunk first and then backported into the minor branches.

โ€‹@hellofromTonya commented on โ€‹PR #3742:


4 years ago
#16

@peterwilsoncc did you want to commit this for 6.2 and then backport to 6.1 branch?

#17 @peterwilsoncc
4 years ago

  • Owner set to peterwilsoncc
  • Resolution โ†’ fixed
  • Status new โ†’ closed

In 55345:

Editor: Prevent KSES stripping global layout style properties.

Layout style properties are stored using indirect values, rather than direct CSS properties.

Allow users without the unfiltered_html capability to modify global styles using the indirect block spacing properties contentSize, wideSize, and blockGap, using a mapping of the eventual CSS property to the indirect property stored in theme.json. The mapped CSS property is then used for CSS validation.

Props andrewserong, costdev, hellofromtonya, mamaduka, mmtr86.
Fixes #57321.

โ€‹@andrewserong commented on โ€‹PR #3742:


4 years ago
#19

Wonderful, thanks for committing, Peter! ๐Ÿ™‡

#20 @peterwilsoncc
4 years ago

  • Keywords fixed-major added
  • Resolution fixed
  • Status closed โ†’ reopened

Reopening for merging in to the 6.2 branch.

Version 0, edited 4 years ago by peterwilsoncc (next)

#21 @hellofromTonya
3 years ago

  • Keywords dev-reviewed added

LGTM for backport to the 6.1 branch, i.e. for a 6.1.2 minor.

โ€‹@oandregal commented on โ€‹PR #3742:


3 years ago
#22

:wave: There are some changes here that are not part of the original gutenberg PR. I've backported them to gutenberg at โ€‹https://github.com/WordPress/gutenberg/pull/48646

โ€‹@andrewserong commented on โ€‹PR #3742:


3 years ago
#23

Thanks so much @oandregal! I'll give that PR a review today.

#24 @SergeyBiryukov
3 years ago

  • Milestone 6.1.2 โ†’ 6.2.1

Moving to 6.2.1, as there are no plans for 6.1.2 at this time.

#25 @SergeyBiryukov
3 years ago

  • Milestone 6.2.1 โ†’ 6.2
  • Resolution โ†’ fixed
  • Status reopened โ†’ closed

This is already included in the 6.2 branch as of [55504], no backport needed.

Note: See TracTickets for help on using tickets.