WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 8 weeks ago

Last modified 8 weeks ago

#47367 closed enhancement (fixed)

KSES: Update CSS properties considered safe for all users.

Reported by: peterwilsoncc Owned by: marybaum
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-patch
Focuses: Cc:
PR Number:

Description

Apart from some minor updates to account for the block editor, the KSES list of allowed CSS properties for authors and contributors has not been updated for quite some time.

Most significantly, not all safe grid and flex box attributes are supported. There are several other new features of CSS missing too.

Reference: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference

Tasks

  • add support for new individual properties implicitly supported by shorthand attributes
  • determine what is considered safe

Related #47281, #37248, #45067, #42729.

Attachments (6)

added__idea_files.patch (2.2 MB) - added by marybaum 5 months ago.
Adds properties from CSS-Grid, Flexbox and CSS columns to safe-styles array
kses_php.patch (435 bytes) - added by marybaum 5 months ago.
This patch has just one file in it.
kses-190609.patch (1.3 KB) - added by marybaum 5 months ago.
Many modern CSS properties, many lines, one file!
47367.diff (1.3 KB) - added by peterwilsoncc 3 months ago.
47367.2.diff (3.8 KB) - added by peterwilsoncc 8 weeks ago.
47367.3.diff (5.2 KB) - added by peterwilsoncc 8 weeks ago.
Test all new CSS properties supported.

Change History (24)

This ticket was mentioned in Slack in #core-editor by peterwilsoncc. View the logs.


6 months ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


6 months ago

#3 @desrosj
6 months ago

  • Keywords needs-patch added

#4 @peterwilsoncc
6 months ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to marybaum
  • Status changed from new to assigned
  • Type changed from defect (bug) to enhancement

Assigning @marybaum as owner per discussion in Slack.

Assigning to future release to assist with triage, once a patch exists it can be moved to a version number.

@marybaum
5 months ago

Adds properties from CSS-Grid, Flexbox and CSS columns to safe-styles array

#5 @marybaum
5 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 5.2.2

Probably could have named that patch better. 😜
Oh well.

I'm also changing the milestone to 5.2.2 per my conversation with @peterwilsoncc on Slack last week.

#MommasFirstPatch

#6 @peterwilsoncc
5 months ago

  • Keywords needs-refresh added
  • Milestone changed from 5.2.2 to 5.3

@marybaum

I was unclear when chatting, I was thinking next major release rather next minor. In this case, auto updating KSES is probably fine but I'd prefer to wait until the major release.

I'm sorry but the patch will need a refresh, too. It contains changes to quite a few files rather than just the kses file. I'll reach out to you via Slack to help you with the process.

@marybaum
5 months ago

This patch has just one file in it.

#7 @marybaum
5 months ago

@peterwilsoncc I've refreshed the patch with a new one but will leave the workflow as needs-refresh until you've looked at it.

@marybaum
5 months ago

Many modern CSS properties, many lines, one file!

#8 @marybaum
5 months ago

  • Keywords 2nd-opinion commit added

New patch!

Would love y'all to look it over, make sure it's right and then commit.

#9 @birgire
5 months ago

Thanks for the patch @marybaum

I collected a list of grid attributes in #46597 that might be of use here.

Most of them are now already patched in kses-190609.patch, so I've marked them with an x:

grid
x grid-column
x grid-row
grid-area
x grid-gap
x grid-column-gap
x grid-row-gap
grid-template
grid-template-areas
x grid-template-columns
grid-auto-columns
grid-auto-rows
grid-auto-flow
x grid-column-start
x grid-column-end
x grid-row-start
grid-row-end
x justify-self
x justify-items
x justify-content
x align-self
x align-items
x align-content
place-self
place-content

What do you think about the other ones on the list not marked with x?

In kses-190609.patch I noticed there are two new empty lines added in few places. I guess it should only be single ones.

There's also duplicate entry for grid-column-start in kses-190609.patch

All the best.

@peterwilsoncc
3 months ago

#10 @peterwilsoncc
3 months ago

Some minor coding standards fixes in 47367.diff. I haven't made any changes to the properties in there

#11 @desrosj
8 weeks ago

  • Keywords needs-unit-tests added; commit removed

Can we add a few unit tests for a few of the new properties? Other than that, this is looking good.

@peterwilsoncc are you able to see this one through or confirm it's ready?

#12 follow-up: @peterwilsoncc
8 weeks ago

Flex and columns support are good to go. Grid I am in two minds about.

Attachment with tests incoming.


grid, grid-row, grid-column and grid-template-* each accept brackets, eg repeat( 6, 1fr ) which are blocked in KSES. grid additionally accepts \ which KSES doesn't like in CSS either.

This was done in #10336 back in the days of the browser wars, CSS hacks, and IE6 support. @azaozz do you remember the nastiness you were trying to avoid?

None of the characters \() are required for CSS Grid but rather nice to haves. As a result I am not sure if this is good to go with documentation informing users that some grid attributes have limited support or whether it is better to hold off until the next release:

  • repeat( 6, 1fr ) can be worked around
  • minmax( 1fr, 700px ) less so

As the block editor already uses grid-template-columns, I'm leaning towards merge and document.

#13 @peterwilsoncc
8 weeks ago

In 47367.2.diff

  • No support for properties supporting \, grid, grid-column, grid-row. They're long hand only for now.
  • Unit tests for various flex and grid properties
  • Extended list of supported column layout properties on previous patch
  • Removed clip-path and shape-outside because I am undecided as to whether they're safe
  • Ignored the brackets problem

#14 in reply to: ↑ 12 ; follow-up: @azaozz
8 weeks ago

Replying to peterwilsoncc:

This was done in #10336 back in the days of the browser wars, CSS hacks, and IE6 support. @azaozz do you remember the nastiness you were trying to avoid?

That was ...ten years ago! As far as I remember it was a security fix/hardening at the time. The code came from (then separate) WP Multisite.

At the time there were quite a few possibilities for misuse, different for the different browsers and browser versions. Many of these "CSS features" have been removed since then, but some may still be aground.

If we want to remove the restriction seems we will need to add a lot more granular filtering for inline style attributes.

#15 in reply to: ↑ 14 @peterwilsoncc
8 weeks ago

  • Keywords needs-refresh 2nd-opinion removed

Replying to azaozz:

That was ...ten years ago! As far as I remember it was a security fix/hardening at the time. The code came from (then separate) WP Multisite.

At the time there were quite a few possibilities for misuse, different for the different browsers and browser versions. Many of these "CSS features" have been removed since then, but some may still be aground.

If we want to remove the restriction seems we will need to add a lot more granular filtering for inline style attributes.

Thanks, I appreciate the help.

I've thought about this over the last 48 hours. Let's put it in as even in a limited form grid and flex are very helpful for using the block editor to create interesting layouts. A complete lack of support may result in plugin authors enabling unfiltered_html for all users which is problematic in itself.

@peterwilsoncc
8 weeks ago

Test all new CSS properties supported.

#16 @peterwilsoncc
8 weeks ago

In 46235:

KSES: Add support for modern layout techniques in style attribute.

Adds support for flex, grid and column layout techniques to the list of CSS attributes considered safe for inline CSS. The \ character and CSS functions, eg minmax() are not yet supported.

Extends support of border properties to include border-radius and individual background properties to include all those implicitly supported by the shorthand attribute.

Props mrahmadawais, marybaum, birgire, peterwilsoncc, azaozz.
Fixes #37248.
See #47367.

#17 @desrosj
8 weeks ago

  • Keywords needs-unit-tests removed
  • Resolution set to fixed
  • Status changed from assigned to closed

I'm going to close this out. I think that everything has been addressed here. @peterwilsoncc if not, can you reopen or open follow up tickets?

#18 @peterwilsoncc
8 weeks ago

@desrosj Can do. Supporting functions warrants it's own ticket and I'll set up a 5.4 edition for considering further safe properties.

Note: See TracTickets for help on using tickets.