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.

Sign up for GitHub

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

Add layout-related CSS properties to safe_style_css_filter in kses.php #2928

Closed
wants to merge 3 commits into from
Closed

Add layout-related CSS properties to safe_style_css_filter in kses.php #2928

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8fbcf2b
Add layout-related CSS properties to safe_style_css_filter in kses.php
andrewserong Jul 1, 2022
f0c2515
Fix expected spacing in tests
andrewserong Jul 1, 2022
62928d9
Remove display property
andrewserong Jul 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/wp-includes/kses.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2320,6 +2320,8 @@ function safecss_filter_attr( $css, $deprecated = '' ) {
'text-indent',
'text-transform',

'display',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not allow display in it's entirety. I wasn't around for the original specification but I expect it was left out very deliberately to prevent authors and contributors from doing something like:

<p style="display: none"><a href="spammy-seo-link.html">Certainly nothing dodgy here</a></p>

Are you able to limit it to a set of values?

'display' => array( 'flex', 'grid' )

...

// Something clever in the loop below to determine whether to use the key or value as the allowed selector if this is possible.

Copy link
Contributor Author

@andrewserong andrewserong Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks Peter, that's exactly the kind of feedback I was hoping for on this PR! I'll have a play, yes the goal is to allow a couple of particular kinds of layouts, not to expose display: none, so I'll see what we can do to limit this to just what's required for the current and shorter-term goals of different layouts. Or, alternately, if we need to specifically handle display within the Layout support and not expose it in the kses file. I'll give it some more thought! 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: I had another look over the Layout work in the Gutenberg repo, and I think it makes more sense for the Layout code to use its own special handling for the display property (constructing the rule based on an allow list) rather than exposing it directly in kses.php. Even if we limit the options here, the objective wasn't really to allow display to be used anywhere, it was really tied to this one use case. Whereas the goal for the other properties added in this PR is for them to be usable pretty much anywhere.

I've removed display from this PR (and added special handling for display over in the Layout PR WordPress/gutenberg#40875). But, we can always revisit this in the future if we do think it makes more sense in kses.php! 🙂

Copy link
Member

@ramonjd ramonjd Aug 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wasn't around for the original specification but I expect it was left out very deliberately to prevent authors and contributors from doing something like:

Are there any discussions you could point to that would help us understand this concern?

I mean, what's stopping authors and contributors doing that by assigning the background color to text and links?

E.g.,

<!-- this is invisible on a white background -->
<p class="has-white-color has-text-color has-link-color"><a href="http://test.com" data-type="URL" data-id="http://test.com">Spammy text</a></p>

Copy link
Member

Choose a reason for hiding this comment

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

And if it ends up that we have to constrain values for display, would it be okay to have a custom check within safecss_filter_attr ?

Copy link
Member

Choose a reason for hiding this comment

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

@peterwilsoncc @andrewserong How do you think we could proceed with this one?

Would a compromise be a white list of display properties, excluding none, assuming the concern alluded to above is valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, thanks for the ping @ramonjd! For 6.1, do we need display to be included? I believe the only place we're currently using it is in the layout support's rendering in the Theme JSON class (here: https://github.com/WordPress/gutenberg/blob/ffd75bd84e15b7ac611d3c25bc11e0fcd4840773/lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php#L1410)

To avoid being blocked on display, I was wondering if the current state of this PR is good enough to land as-is, given that it addresses the remaining layout properties that the style engine will need to validate?

Alternately, if we think it's worth it to handle display as well, I think adding a list of allowed values sounds like a reasonable way to go 👍

Copy link
Member

Choose a reason for hiding this comment

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

For 6.1, do we need display to be included?

Good question! I assumed "yes" because of the eventual migration of class-wp-theme-json-6-1.php 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed "yes" because of the eventual migration of class-wp-theme-json-6-1.php 🤷

In the current implementation, on this line the allow-listed display mode is output directly, rather than being parsed through safecss_filter_attr, so I didn't think it was needed until we move global styles output to being run through the style engine.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks for clarifying! 🙇


'height',
'min-height',
'max-height',
Expand All @@ -2333,19 +2335,32 @@ function safecss_filter_attr( $css, $deprecated = '' ) {
'margin-bottom',
'margin-left',
'margin-top',
'margin-block-start',
'margin-block-end',
'margin-inline-start',
'margin-inline-end',

'padding',
'padding-right',
'padding-bottom',
'padding-left',
'padding-top',
'padding-block-start',
'padding-block-end',
'padding-inline-start',
'padding-inline-end',

'flex',
'flex-basis',
'flex-direction',
'flex-flow',
'flex-grow',
'flex-shrink',
'flex-wrap',

'gap',
'column-gap',
'row-gap',

'grid-template-columns',
'grid-auto-columns',
Expand Down
19 changes: 17 additions & 2 deletions tests/phpunit/tests/kses.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1033,8 +1033,8 @@ public function data_test_safecss_filter_attr() {
),
// `flex` and related attributes introduced in 5.3.
array(
'css' => 'flex: 0 1 auto;flex-basis: 75%;flex-direction: row-reverse;flex-flow: row-reverse nowrap;flex-grow: 2;flex-shrink: 1',
'expected' => 'flex: 0 1 auto;flex-basis: 75%;flex-direction: row-reverse;flex-flow: row-reverse nowrap;flex-grow: 2;flex-shrink: 1',
'css' => 'flex: 0 1 auto;flex-basis: 75%;flex-direction: row-reverse;flex-flow: row-reverse nowrap;flex-grow: 2;flex-shrink: 1;flex-wrap: nowrap',
'expected' => 'flex: 0 1 auto;flex-basis: 75%;flex-direction: row-reverse;flex-flow: row-reverse nowrap;flex-grow: 2;flex-shrink: 1;flex-wrap: nowrap',
),
// `grid` and related attributes introduced in 5.3.
array(
Expand Down Expand Up @@ -1120,6 +1120,21 @@ public function data_test_safecss_filter_attr() {
'css' => 'color: rgb( 100, 100, 100, .4 )',
'expected' => '',
),
// Gap introduced in 6.1.
array(
'css' => 'gap: 10px;column-gap: 5px;row-gap: 20px',
'expected' => 'gap: 10px;column-gap: 5px;row-gap: 20px',
),
// Margin and padding logical properties introduced in 6.1.
array(
'css' => 'margin-block-start: 1px;margin-block-end: 2px;margin-inline-start: 3px;margin-inline-end: 4px;padding-block-start: 1px;padding-block-end: 2px;padding-inline-start: 3px;padding-inline-end: 4px',
'expected' => 'margin-block-start: 1px;margin-block-end: 2px;margin-inline-start: 3px;margin-inline-end: 4px;padding-block-start: 1px;padding-block-end: 2px;padding-inline-start: 3px;padding-inline-end: 4px',
),
// Display property introduced in 6.1.
array(
'css' => 'display: grid',
'expected' => 'display: grid',
),
);
}

Expand Down