Make WordPress Core

Opened 6 weeks ago

Last modified 4 days ago

#64771 new defect (bug)

Existing block level custom CSS in a post breaks when the post is edited by user without unfiltered_html

Reported by: glendaviesnz's profile glendaviesnz Owned by:
Milestone: 7.0 Priority: high
Severity: critical Version: trunk
Component: Editor Keywords: has-patch has-unit-tests 2nd-opinion
Focuses: Cc:

Description (last modified by glendaviesnz)

https://github.com/WordPress/gutenberg/pull/73959 introduced block level custom CSS.

Everything works as expected unless a user without unfiltered_html edits a page/post with existing block-level custom CSS that includes nested selectors, eg.

color: green;
& p {color: blue}

In these cases, entities like & are encoded and the CSS breaks in the editor and the frontend.

This is caused by KSES filtering in core, not in Editor codes, so filing here, not in the GB repo.

Change History (51)

This ticket was mentioned in PR #11104 on WordPress/wordpress-develop by @glendaviesnz.


6 weeks ago
#1

  • Keywords has-patch added

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

## Use of AI Tools

@glendaviesnz commented on PR #11104:


6 weeks ago
#2

FYI - I will add tests for this once there is confirmation that this is the correct approach for fixing this bug. There may be a better solution. If there is feel free to close this PR and open an alternative.

#3 @wildworks
6 weeks ago

#64770 was marked as a duplicate.

#4 @wildworks
6 weeks ago

  • Component changed from Canonical to Editor
  • Milestone changed from Awaiting Review to 7.0

@glendaviesnz commented on PR #11104:


6 weeks ago
#5

Related: https://core.trac.wordpress.org/changeset/61486 / https://github.com/WordPress/wordpress-develop/pull/10641 fixed the same class of KSES-mangling issue for Global Styles custom CSS by pre-escaping the JSON with JSON_HEX_TAG | JSON_HEX_AMP. This PR addresses the same problem for per-block custom CSS (attrs.style.css), which goes through the separate filter_block_kses() pipeline. I don't think the same pre-escaping approach can work in this case, as parse_blocks() calls json_decode() on the entire attributes object, which converts \u0026 back to & before KSES runs - but I do not know a lot about these flows, and don't have time to look closer as currenlty travelling.

@jonsurrell commented on PR #11104:


6 weeks ago
#6

I had some trouble reproducing the issue.

In order to test this, I had to enable a recent version of the Gutenberg plugin. The individual block CSS feature is not yet available in Core yet, is it?

When I used an _author_ role, I don't see the additional CSS panel for blocks. When I used an _editor_ role, I was unable to reproduce the issue because it seems to have unfiltered_html capability already.

Am I doing something wrong in the reproduction steps?

---

Some thoughts based on the issue:

  • KSES is unsuitable for processing data that is not HTML. This type of issue appears again and again.
  • The KSES filters were added in r46896 / https://github.com/WordPress/wordpress-develop/commit/7c38cf178af78ba28d9a19ac945d6506f951fb2f. That seems to be a security fix with limited public information.
  • I wish we could avoid applying KSES (HTML) filtering _everywhere_, but that seems unlikely at this time.
  • I see "double-encoding" mentioned a few times. That doesn't seem accurate given the description. CSS text that will be used in rawtext STYLE (where HTML character references like & _are not used_) has had HTML character reference escaping applied to it. & appears in the CSS text because the character references will never be decoded in this context. It seems more accurate to talk about "mis-encoded" or even just "mangled." This is akin to applying any other unsuitable escaping mechanism.

---

I was very happy with the solution in r61486. Post content exclusively contained JSON which has some flexibility in escaping. JSON can be made to be plain HTML text by escaping HTML syntax characters (<>&). KSES ignores this. This approach escapes data before KSES can mangle it.

This PR tries to recover _after_ KSES has mangled the data. That seems inherently more risky. It should be possible to decode HTML character references (like done here) but what if KSES starts to remove things that look like tags? What if I'd like to use content: '<data> here';? (KSES will likely strip <data> from this).

Another option is to protect the data _before_ KSES can mangle it by encoding it ourselves in an HTML-text safe way. A few quick options come to mind:

Of course, before _using_ the value it will need to be decoded appropriately. Either of these seem likely to prevent the issue by ensuring HTML syntax <>& are not present, so KSES should not take any action.

@glendaviesnz commented on PR #11104:


6 weeks ago
#7

I had some trouble reproducing the issue.
In order to test this, I had to enable a recent version of the Gutenberg plugin. The individual block CSS feature is not yet available in Core yet, is it?
When I used an author role, I don't see the additional CSS panel for blocks. When I used an editor role, I was unable to reproduce the issue because it seems to have unfiltered_html capability already.
Am I doing something wrong in the reproduction steps?

@sirreal when logged in as the author user you do not need to see the custom CSS input box, you just need to edit the post content with the existing custom CSS in place that was added when you created the post as the admin user. This bug only occurs if a user without unfiltered_html edits a post that had block level customCSS add by a higher level user.

@ramonopoly commented on PR #11104:


6 weeks ago
#8

A few quick options come to mind:

Thanks a lot @sirreal, this is great.

Could "don’t run KSES on block attribute attrs.style.css" be another option?

Above, I was thinking of an allowlist of “non-HTML” attribute paths that are not HTML, e.g. ['css'], and in filter_block_kses_value(), when the current path is in that list, use a non-HTML sanitizer, e.g. wp_strip_all_tags or a variant of it

If there are hidden gotchas there...

protect the data before KSES can mangle it by encoding it ourselves in an HTML-text safe way

Maybe @glendaviesnz can answer this: let's say we encode in filter_block_kses (and decode when output in custom-css.php), do we need to worry about backwards compat at all? For example, for folks that have already used this feature in the plugin or elsewhere, would we have to infer “is this encoded or plain?”

#9 @westonruter
6 weeks ago

  • Description modified (diff)

@jonsurrell commented on PR #11104:


6 weeks ago
#10

when logged in as the author user you… need to edit the post content with the existing custom CSS in place

Got it, that worked. I did have to change the post author so that the _author_ role user could edit the post.

Could "don’t run KSES on block attribute attrs.style.css" be another option?

That's a way to prevent this issue. The problem is that exceptions like that often create vulnerabilities. If a bad actor knows attrs.style.css will not be sanitized, they can often find a way to abuse it.

This ticket was mentioned in Slack in #core-test by r1k0. View the logs.


6 weeks ago

@dmsnell commented on PR #11104:


6 weeks ago
#12

would we have to infer “is this encoded or plain?”

this is going to be a dead-end, because it’s largely not possible to do that. we can build in signals into the storage to communicate it though. for instance, prefix a base64-encoded string.

or in that same vein but better, store the attribute as a data URI which says explicitly what the content is.

{
	"style": {
		"css": "data:text/css;base64,eyBjb2xvcjogcmVkOyB9"
	}	
}

for the sake of transparency we can always escape the CSS from any characters that would be “dangerous,” but I think we’ve seen a number of cases where this has gone wrong because downstream code likes to unescape and re-escape, which ends up eliminating the escaping we intentionally applied.

wp_strip_all_tags

wp_strip_all_tags() is never going to be appropriate for CSS, but CSS should still go through some process like KSES, which is what functions like safecss_filter_attr() are for. there are rules applied to things like URLs inside of CSS declarations which WordPress will want to apply.

---

the $context parameter of filter_block_kses_value() offers a potential place to raise the bar on CSS handling. if we had a sentinel value indicating that the attribute is supposed to be CSS we could apply more appropriate sanitization, but we would want to make sure we don’t make it easy for people to set that context from user-supplied inputs.

#13 @ozgursar
6 weeks ago

Patch Testing Report

Yesterday I wasn't able to reproduce the bug because I didn't notice the clear test instructions in the PR. Thanks to @r1k0 for pointing this out.

Patch Tested: https://github.com/WordPress/wordpress-develop/pull/11104

Environment

  • WordPress: 7.0-beta2-61752-src
  • PHP: 8.2.29
  • Server: nginx/1.29.4
  • Database: mysqli (Server: 8.4.7 / Client: mysqlnd 8.2.29)
  • Browser: Chrome 145.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-Five 1.4
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.1

Steps taken

  1. Followed the test steps provided in the PR for each test1 to test5
  2. ✅ Patch is solving the problem except the Test5 foo&bar additional classes

Expected result

  • We are expecting the Additional CSS not to be changed after being edited by a user that doesn't have unfiltered_html capability.

Screenshots/Screencast with results

Test1 (Before Patch)
https://i.imgur.com/YmfOrvv.png

Test2 (Before Patch)
https://i.imgur.com/75EGjR2.png

Test3 (Before Patch)
https://i.imgur.com/QMkilvv.png

Test4 (Before Patch)
https://i.imgur.com/J0ThJhi.png

Test5 (Before Patch)
https://i.imgur.com/sgi24Y5.png


Test1 (After Patch)
https://i.imgur.com/J588fNp.png

Test2 (After Patch)
https://i.imgur.com/JnoI2FG.png

Test3 (After Patch)
https://i.imgur.com/Y6zqikA.png

Test4 (After Patch)
https://i.imgur.com/5TgOtKj.png

Test5 (After Patch)
https://i.imgur.com/gx2zeLF.png

Last edited 6 weeks ago by ozgursar (previous) (diff)

#14 @ugyensupport
6 weeks ago

The new block level custom CSS breaks when edited by user without unfiltered_html

Description

https://github.com/WordPress/gutenberg/pull/73959 introduced block level custom CSS.

Everything works as expected unless a user without unfiltered_html edits a page/post with block level custom CSS that includes nested selectors, eg.

color: green;
& p {color: blue}
In these cases, entities like & are encoded and the CSS breaks in the editor and the frontend.

This is caused by KSES filtering in core, not in Editor codes, so filing here, not in the GB repo.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/11104

Environment

  • WordPress: 7.0-beta2-61784
  • PHP: 8.3.27
  • Server: nginx/1.21.4
  • Database: mysqli (Server: 5.7.44-log / Client: mysqlnd 8.3.27)
  • Browser: Chrome 145.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-Five 1.4
  • MU Plugins: None activated
  • Plugins:
    • Debug Log Viewer 2.1
    • Test Reports 1.2.1
    • WordPress Beta Tester 4.0.0
    • WP File Manager 8.0.2

Actual Results

  1. ✅ Issue resolved with patch.

Supplemental Artifacts

Patch works: https://i.postimg.cc/5NPcFWMg/test.png

@jonsurrell commented on PR #11104:


6 weeks ago
#15

Why are these attributes allowed _at all_ for folks without the appropriate capability?

  • The panel is not displayed for those users, suggesting that the intention is to prevent them from adding the CSS.
  • This PR and ticket 64771 indicate that they _do_ have access to author the CSS.

That is incoherent. If they can use the feature, let's show the UI (and make sure it works correctly). Otherwise, they should not be able to add custom CSS at all.

I've just confirmed that an author can add this and access the feature.

<p class="has-custom-css">asdf</p>

---

How about a completely different approach:

  • Strip an individual block's custom CSS for users without the correct capability.
  • Display a warning in the editor on blocks that have custom CSS for users that will cause the custom CSS to be lost. "Warning: This block contains custom CSS and you do not have the appropriate capability. If you update this post, the custom CSS will be removed." (or something along those lines).

#16 @glendaviesnz
6 weeks ago

  • Description modified (diff)
  • Summary changed from The new block level custom CSS breaks when edited by user without unfiltered_html to Existing block level custom CSS in a post breaks when the post is edited by user without unfiltered_html

@glendaviesnz commented on PR #11104:


6 weeks ago
#17

This PR and ticket 64771 indicate that they do have access to author the CSS.

That is incoherent.

Apologies, the wording of the ticket and PR was unclear, I have updated it to
Existing block level custom CSS in a post breaks when the post is edited by user without unfiltered_html to make it clearer.

Before finalising a fix for this, we probably need a decision on whether users without unfiltered_html should or should not be able to add/edit block-level custom CSS, eg. just show them the box if they can edit/add and fix the KSES issue, or just strip them and add the warning as @sirreal suggests.

@ramonjd - any thoughts on the best way to get a decision on that?

@ramonopoly commented on PR #11104:


6 weeks ago
#18

Before finalising a fix for this, we probably need a decision on whether users without unfiltered_html should or should not be able to add/edit block-level custom CSS

My bag of 2c coins:

Here's the scenario I've been working with (I'm using authors as catch-all role for no unfiltered_html permissions):

  1. An author creates a post called Y. Nice.
  2. An admin/editor (or anyone with unfiltered_html permissions) logs in and edits post Y, adding custom CSS to a block.
  3. Our author returns and edits anything in post Y (not custom CSS), then saves the post.
  4. Custom CSS is mangled silently.

So they can "edit" it technically, because they can edit the post content, but in the regular editor UI flow they cannot. I get @sirreal's point about incoherency.

Following that I see the choice between:

A) keep the status quo and deal with CSS integrity preservation/kses mangling when saving the post OR
B) opening up custom CSS to users without unfiltered_html permissions OR
C) stripping with a warning (a general rule for life!)

With A, we have the very helpful suggestions from John, Dennis and folks.

In relation to B, I'm not sure we'd want to add new, potential security holes. Authors can't currently add CSS, so we probably shouldn't let them. Happy to be persuaded on this and all points.

As for C, my instinct was that stripping wouldn't be appropriate, because authors by default can't see the custom css field (at least in my testing), so from their point of view they're not editing that attribute at all. And editors might wonder why the custom CSS they created is broken or stripped, BUT I was chatting with @tellthemachines, who made a good point to check the HTML block's behaviour in this regard.

Authors can't add CSS/JSS, and <style> tags are stripped. Any subsequent changes made by editors to the same block will be flagged in the editor the next time an author attempts to save the post:

https://github.com/user-attachments/assets/70ce3b95-23ae-4e85-bf76-d74a5e640765

So stripping would be more consistent with that block, and also John's "alternative" approach. The only difference is that it's not immediately visible to the author (without some sort of warning).

any thoughts on the best way to get a decision on that?

Extending authors' permissions, I expect, would be something that needs to be run past the core team and security folks.

Honestly, I think the quickest and safest approach right now is the HTML-block/Sirreal™️ approach because:

  • it's consistent with existing flows (HTML block)
  • it maintains current security/permission arrangements

Optionally, there could be some help text underneath the UI control to tell editors that

I say "for now" because maybe there's a better way down the road that preserves permissions and the intentions of admins when they are making changes, and, furthermore, promotes CSS content as generally safe under the right conditions.

I threw up a rough PR to help my brain work through this:

https://github.com/user-attachments/assets/047295cd-66bc-4279-ba48-b7627159b7c6

@glendaviesnz commented on PR #11104:


6 weeks ago
#19

Unfortunately, stripping it is not a good solution for us in a multi-site scenario where the site admins do not have unfiltered_html permissions, and the block custom CSS is going to be added by AI agents.

@ramonopoly commented on PR #11104:


6 weeks ago
#20

We might just have to abandon the plans we had for this if stripping it is the only option for now.

I don't have a strong opinion. Maybe block css is the exception and it can be preserved?

Also, sorry @sirreal I spelled your name wrong (Jon with an h).

@jonsurrell commented on PR #11104:


5 weeks ago
#21

I want to clarify one thing. An _author_ role user, right now, can access the code editor and write this:

<p class="has-custom-css">asdf</p>

@ramonopoly commented on PR #11104:


5 weeks ago
#22

Either the UI should provide access for them, or they should not be allowed to save that css attribute in the post content. That's what I see as incoherent.

Thanks for confirming. That was my take away.

If authors can already set classnames, and inline styles (via block supports) and save the CSS attributes like you say, then I think that dilutes the value of stripping as an interim patch. Glen's use case suggests it probably isn't. I don't know.

CSS should still go through some process like KSES

Yeah, so if unfiltered_html is the wrong capability gate for CSS, and if KSES is the wrong sanitizer for CSS, then maybe a CSS-aware processor[1] or wp_kses_css_block() is required.

I'm just trying to get my head around the scope: would we need a real CSS sanitizer (nested selectors, at rules, URLs...) or is the main security concern here </style> injection?

Pursuant to the latter, I messed around with skipping KSES and leaning on the validate_custom_css work done previously in https://core.trac.wordpress.org/changeset/61486. It's pretty bent, but I just wanted to try it.

[1]: 😉😉😉😉😉

@glendaviesnz commented on PR #11104:


5 weeks ago
#23

Either the UI should provide access for them, or they should not be allowed to save that css attribute in the post
content. That's what I see as incoherent.

Yes, that makes no sense. I think the approach @ramonjd suggests is a better solution than deleting existing CSS if edited by a user without unfiltered_html

@glendaviesnz commented on PR #11104:


5 weeks ago
#24

@sirreal, @ramonjd I pushed a change that rather than letting KSES process the CSS and then trying to undo the damage (the previous approach in this PR), the CSS is extracted before KSES runs and sanitized using methods appropriate for CSS.

### How it works

In filter_block_kses():

  1. Extract $block['attrs']['style']['css'] and temporarily unset it
  2. Run KSES on the remaining block attributes as normal
  3. Sanitize the extracted CSS using wp_sanitize_block_custom_css(), which applies:
    • wp_strip_all_tags() — valid CSS never contains HTML tags
    • wp_validate_css_for_style_element() — rejects CSS containing </style> (or partial prefixes), preventing breakout from the <style> element
  4. Reinsert the sanitized CSS into the block attributes

KSES never sees the CSS string, so it cannot mangle it. If we decide that this is appropriate then I suggest we also add a GB PR that shows the block level CSS input to users without unfiltered_html.

I have not spent any time testing or tidying up the approach, I will worry about that if there is some agreement that this might be a valid approach.

@isabel_brison commented on PR #11104:


5 weeks ago
#25

CSS should still go through some process like KSES, which is what functions like safecss_filter_attr() are for. there are rules applied to things like URLs inside of CSS declarations which WordPress will want to apply.

This is a good point. safecss_filter_attr() checks blocks of property/value pairs, so in order to make it work here we'd probably have to run it on the top-level declarations and then separately on any declarations inside selectors/brackets. But it would be worth doing to ensure the actual CSS isn't dodgy.

@jonsurrell commented on PR #11104:


5 weeks ago
#26

It doesn't seem like a good idea to me to hide specific attributes from KSES processing. Perhaps if there were a more general sanitization system, but 1-off exceptions to KSES content filtering isn't something I'm in favor of introducing here.

I don't think we should introduce a new wp_validate_css_for_style_element() function. There's some duplication, but I don't think this function should exist or be used in new features. The only reason the logic exists was because it was considered risky to allow </style> in those pre-existing systems.

With the HTML API, anything that's newly developed _does not need to worry about whether the contents of a <style> tag are safe._ This will produce a safe style tag with arbitrary contents:

https://github.com/WordPress/wordpress-develop/blob/56a6768ee322b1881a83746d7a38ef62085038e5/src/wp-includes/class-wp-styles.php#L335-L339

---

wp_strip_all_tags() — valid CSS never contains HTML tags

This is not appropriate for sanitizing CSS. It function removes text that looks like tags, and valid CSS certainly may contain things that look like tags. See this ticket for example:

@property --animate {
  syntax: "<custom-ident>";
  inherits: true;
  initial-value: false;
}

It's also worth considering any of the arbitrary strings that can be used in CSS, for example:

div:has(input:first-child)::before {
  content: "<text>";
}
ul {
  list-style: "<li>";
}

@jonsurrell commented on PR #11104:


5 weeks ago
#27

Using an HTML- and KSES-safe encoding for the serialized data seemed promising. Has that been explored?

Another option is to protect the data _before_ KSES can mangle it by encoding it ourselves in an HTML-text safe way. A few quick options come to mind:

@ramonopoly commented on PR #11104:


5 weeks ago
#28

base64 the CSS string

I looked into this briefly and found that it required some identifiable token in order to flag it as "to-be-decoded" on the way out. Similar to what Dennis spoke of above.

I baulked at this initially for the reason that, once it's there, we'll have to support it forever even if some CSS processor comes along down the track to replace it. Maybe I'm overthinking it.

@glendaviesnz commented on PR #11104:


5 weeks ago
#29

This commit uses JSON encode as @sirreal suggested. @ramonjd this seems a little less heavy-handed than base64 and matches existing approaches.

So, in summary, as the PR stands now:

Instead of hiding CSS from KSES or trying to undo KSES damage after the fact, we encode the CSS value using wp_json_encode() with JSON_HEX_TAG | JSON_HEX_AMP before KSES runs. This converts <, >, and & into JSON unicode escapes (\u003C, \u003E, \u0026) that KSES passes through untouched. After KSES, we json_decode() them back to the original characters.

### src/wp-includes/blocks.phpfilter_block_kses()

  • Before KSES: encode attrs.style.css via wp_json_encode($css, JSON_HEX_TAG | JSON_HEX_AMP)
  • After KSES: decode via json_decode()
  • Removed undo_block_custom_css_kses_entities() — no longer needed

### src/wp-includes/block-supports/custom-css.phpwp_render_custom_css_support_styles()

  • Removed preg_match( '#</?\w+#', $custom_css ) check — this was overly broad and rejected valid CSS (e.g. @property syntax with <custom-ident>, or content: "<text>"). The HTML API's set_modifiable_text() already handles safe <style> output, making this input-side check unnecessary for new features (per sirreal's feedback).

This of course only prevents the existing KSES from mangling the CSS attribute, it does not address effective sanitisation of the CSS string.

@jonsurrell commented on PR #11104:


5 weeks ago
#30

I don't think transforming the data before and after KSES runs is the right approach. The system should be responsible for transforming its data without requiring exceptions built into general filters.

KSES is designed to prevent problematic data from being stored. When KSES is circumvented, it often leads to things like stored XSS vulnerabilities. It's annoying, but that's kind of the point. The goal is to find ways to store the content to satisfy KSES, not circumvent it.

The encoding and decoding of the data should happen in the system using the data. General filters should not have exceptional behavior to deal with specific attributes. The system that is storing the data should encode it on save and decode it for use. @dmsnell shared a good idea for how to indicate the encoding by using a data-uri.

---

it does not address effective sanitisation of the CSS string. What thoughts do people have about how to address that for 7.0?

I don't expect to have any CSS API for 7.0, which greatly limits the options. I still think the most reasonable thing at this point is to remove the custom block CSS from the post if a user without the capability tries to save it.

@glendaviesnz commented on PR #11104:


5 weeks ago
#31

@sirreal, @ramonjd serializeAttributes() already encodes the CSS in the serialised content, but parse_blocks() → json_decode() reverses that before it is passed into KSES, so it looks like base64 is the best option, but does that go against any Gutenberg principles that the block attribute content should be human-readable?

I am not aware of any other attributes that you can't make sense of by reading the serialised block content, and I have a vague feeling that this is by design, but I could be wrong.

If we are all happy with base64 I will close this PR and open an new Gutenberg PR to make that change.

@jonsurrell commented on PR #11104:


5 weeks ago
#32

The essential idea is that the data stored in the attribute is something that KSES leaves alone because it's harmless.

Either base64 (more opaque) or serializeAttributes (more readable, but perhaps more surprising) seem well suited to the task. Storing a string that is a base64- or json-encoded (via serializeAttributes) string of CSS text should serve. Note that either or these is an addition encoding of the stored data.

@ramonopoly commented on PR #11104:


5 weeks ago
#33

<details>

<summary>This seems to work for me with base64_decode</summary>

  • src/wp-includes/blocks.php

    diff --git a/src/wp-includes/blocks.php b/src/wp-includes/blocks.php
    index 89007d0d0d..7e577a4dd0 100644
    a b function _filter_block_content_callback( $matches ) { 
    20752075 * @return array The filtered and sanitized block object result.
    20762076 */
    20772077function filter_block_kses( $block, $allowed_html, $allowed_protocols = array() ) {
     2078        /*
     2079         * Per-block custom CSS is not HTML; encode it before KSES (so it is not mangled)
     2080         * and decode it back to plain CSS immediately after.
     2081         */
     2082        if ( isset( $block['attrs']['style']['css'] ) && is_string( $block['attrs']['style']['css'] ) ) {
     2083                $css = trim( $block['attrs']['style']['css'] );
     2084                if ( '' !== $css ) {
     2085                        /**
     2086                         * Prefix applied to block custom CSS when base64-encoded for the KSES pass.
     2087                         * The prefix lets wp_decode_block_custom_css_after_kses() recognise values it encoded.
     2088                         */
     2089                        $block['attrs']['style']['css'] = 'data:text/css;base64,' . base64_encode( $css );
     2090                }
     2091        }
     2092
    20782093        $block['attrs'] = filter_block_kses_value( $block['attrs'], $allowed_html, $allowed_protocols, $block );
    20792094
     2095        if ( isset( $block['attrs']['style']['css'] ) && is_string( $block['attrs']['style']['css'] ) ) {
     2096                /**
     2097                 * Decodes block custom CSS from base64 back to plain CSS after the KSES pass.
     2098                 *
     2099                 * Only decodes values that start with prefix so we never
     2100                 * attempt to decode CSS that wasn't encoded above.
     2101                 */
     2102                $css = $block['attrs']['style']['css'];
     2103                if ( str_starts_with( $css, 'data:text/css;base64,' ) ) {
     2104                        $decoded                        = base64_decode( substr( $css, strlen( 'data:text/css;base64,' ) ) );
     2105                        // If the decoded string contains characters from outside the base64 alphabet or a null byte, set the CSS to an empty string.
     2106                        $block['attrs']['style']['css'] = ( false === $decoded || str_contains( $decoded, "\0" ) ) ? '' : $decoded;
     2107                }
     2108        }
     2109
    20802110        if ( is_array( $block['innerBlocks'] ) ) {
    20812111                foreach ( $block['innerBlocks'] as $i => $inner_block ) {
    20822112                        $block['innerBlocks'][ $i ] = filter_block_kses( $inner_block, $allowed_html, $allowed_protocols );

</details>

@jonsurrell commented on PR #11104:


5 weeks ago
#34

The motivation was to keep "encode and decode" in PHP and also keep plain CSS in the database

I don't think this is feasible.

the implication is to store the _encoded_ string?

Yes, that's my thinking.

Here is my reasoning:

  • The CSS has to be stored in block attributes somehow.
  • KSES should run on what's stored in the DB, transforms after KSES tend to be dangerous.
  • KSES will mangle some plain CSS text.

If those hold true, it seems to follow that the data stored in the DB _must_ be encoded in some way that doesn't contain special HTML characters.

I have my own lacunas! There may be a completely different way to achieve this, but encoding the data in the attribute seems like a straightforward and relatively simple way to resolve this issue.

---

It's still possible to strip this data for users without the appropriate capability. That's probably the simplest thing to do for WordPress 7.0 which is right around the corner. That wouldn't involve any other changes and would just rely on KSES not operating. That may not be the best solution in the long term.

---

I still think the data-URIs are _very interesting_:

const originalCSS =
`@property --animate {
  syntax: "<custom-ident>";
  inherits: true;
  initial-value: false;
}
div:has(input:first-child)::before {
  content: "<text>";
}
ul {
  list-style: "<li>";
}`;
const dataURI = `data:text/css,${ encodeURIComponent( originalCSS ) }`;
const parsedCSS = ( await import( dataURI, { with: { type: 'css' } } ) ).default;
const reconstructedCSS = Object.values( parsedCSS.cssRules ).map( rule => rule.cssText ).join( '\n' );

The result in reconstructedCSS is:

@property --animate { syntax: "<custom-ident>"; inherits: true; initial-value: false; }
div:has(input:first-child)::before { content: "<text>"; }
ul { list-style: "<li>"; }

Of course, I think the style system will need to handle decoding the data-URI in PHP, but the native browser APIs in this space are exciting.

@jonsurrell commented on PR #11104:


5 weeks ago
#35

Of course, I think the style system will need to handle decoding the data-URI in PHP, but the native browser APIs in this space are exciting.

Actually, PHP seems to handle base64 or %-encoded data URIs just fine in my testing:

<?php
$c = file_get_contents( 'data:text/css;base64,QHByb3BlcnR5IC0tYW5pbWF0ZSB7CiAgc3ludGF4OiAiPGN1c3RvbS1pZGVudD4iOwogIGluaGVyaXRzOiB0cnVlOwogIGluaXRpYWwtdmFsdWU6IGZhbHNlOwp9CmRpdjpoYXMoaW5wdXQ6Zmlyc3QtY2hpbGQpOjpiZWZvcmUgewogIGNvbnRlbnQ6ICI8dGV4dD4iOwp9CnVsIHsKICBsaXN0LXN0eWxlOiAiPGxpPiI7Cn0=' );
$c2 = file_get_contents( 'data:text/css,%40property%20--animate%20%7B%0A%20%20syntax%3A%20%22%3Ccustom-ident%3E%22%3B%0A%20%20inherits%3A%20true%3B%0A%20%20initial-value%3A%20false%3B%0A%7D%0Adiv%3Ahas(input%3Afirst-child)%3A%3Abefore%20%7B%0A%20%20content%3A%20%22%3Ctext%3E%22%3B%0A%7D%0Aul%20%7B%0A%20%20list-style%3A%20%22%3Cli%3E%22%3B%0A%7D' );
echo "{$c}\n{$c2}";

@ramonopoly commented on PR #11104:


5 weeks ago
#36

The CSS has to be stored in block attributes somehow.
KSES should run on what's stored in the DB, transforms after KSES tend to be dangerous.
KSES will mangle some plain CSS text.

I see, thanks for laying that out. 🍺

I was thinking about how folks reading content programmatically might need to handle this, e.g.,
WP-CLI or get_post().. even SELECT * FROM wp_posts WHERE post_content LIKE '%color: red%'.

Or we make it part of the contract that they have to decode or run the raw post content through parse blocks anyway.

I don't really know either. What smells to me though is that we're dancing around KSES's limitations for a compromise.

For WP 7.0, and assuming there is some time pressure, maybe storing as a data URI is pragmatic.

Later, if there's some CSS API™️ that leaves no fingerprints on the stored format, we'd have to support both formats forever.

@glendaviesnz commented on PR #11104:


4 weeks ago
#37

I am going to close this PR as if taking the suggested approach, this is going to need a Gutenberg fix.

See https://github.com/WordPress/gutenberg/issues/76472

@dmsnell commented on PR #11104:


4 weeks ago
#38

it looks like base64 is the best option, but does that go against any Gutenberg principles that the block attribute content should be human-readable?

how folks reading content programmatically might need to handle this

It can be liberating and helpful to start with the primary thing: we’re attempting to make it possible for folks to add CSS to their blocks individually. To me, the most important “Gutenberg principle” is that the feature works for the editor and doesn’t lose their trust by randomly breaking without explanation. I don’t want people to spend hours working on a design, have someone else look at it who saves the post, maybe because they correct some unrelated typo, and now the page is broken on render and that person who invested their time has proverbial “egg on their face.”

To that end I want to be open-minded about what technical challenges present themselves as obstacles to that goal. And granted, I think you both are raising awesome and exceptional points: having human-readable plaintext is the most open and transparent and interoperable thing we can reach for.

but unfortunately I think the situation we find ourselves in is a huge quagmire with no easy way to achieve all those goals. we can also note that plenty of Core blocks already hide their content from post_content: from the very beginning we had recent-posts and recent-comments blocks with zero content.

we're dancing around KSES's limitations for a compromise

There might be misunderstanding on what @sirreal is saying. KSES has its issues, but in fairness, it’s designed to “sanitize” HTML, not CSS. We had a long call yesterday to discuss the entire concept of CSS sanitization and it seems even more nebulous to discuss than HTML sanitization anyway.

There’s a reason we don’t run eslint against PHP code — they are different languages. The problem is that right now KSES is applying HTML logic against CSS. So it’s not that Jon is recommending a compromise, but separating out these two concerns.

The database stores HTML and received kind of broad blanket HTML-based linting/sanitization. The CSS _needs_ to run through policy-based transformations (sanitization), but the only pragmatic tool we may have is to do that at the point of demarcation between the raw CSS string and when it is rendered back into an HTML document into either a STYLE element or style attribute. First we process it as CSS, then that provides us a new raw CSS value, then we run it through the appropriate HTML processing (which because it’s CSS and going to these two destinations, requires almost no further transformation).

we'd have to support both formats forever.

This is a compelling reason to think about prefixing the content with some kind of indicator or _sigil_, such as the data URI prefix. _At least_ with a data URI the content is somewhat self-descriptive and people can relatively easily recover the source content.

Source code blocks and the HTML block have this same problem: WordPress transformations are complicated and make it hard to _find any way to get it to save what you type_: _KSES_ in point.

---

Just wanted to share some thought in summary on this. Everyone is doing a great job, this is a difficult and common challenge, so thank you for working on it.

For the rushed timeline it may be worth compromises in _scope_ rather than _security_. The best perk of limiting scope now is that we can announce enhancements in the future rather than apologize for shipping things that are broken. It’s generally easier to accept limitations if they are communicated and then liberated than if those boundaries aren’t knowable until you cross them.

@ramonopoly commented on PR #11104:


4 weeks ago
#39

Thanks for helping me understand the issues better, Dennis!

KSES has its issues, but in fairness, it’s designed to “sanitize” HTML, not CSS. We had a long call yesterday to discuss the entire concept of CSS sanitization and it seems even more nebulous to discuss than HTML sanitization anyway.

Ultimately I'll defer to Jon's and your wisdom on this matter - I've been reflecting, and I believe the kink in my thinking stemmed from considering all post_content as valid input for wp_kses. Right or wrong, that's why I was fixated on adding some short circuit there.

I also get that the surface of CSS processing in WP is a heck of a lot larger than the problem we're facing here, so I appreciate you folks stepping back and reasoning about it from wider design perspective.

@glendaviesnz commented on PR #11104:


4 weeks ago
#40

@sirreal, @ramonjd I have added a PR to strip the css for users without edit-css permissions here

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


3 weeks ago

#42 follow-up: @jorbin
3 weeks ago

This was discussed during an impromptu scrub. Myself and @joedolson both are concerned about the idea of just adding a notice as it won't stop users from potentially breaking a page without the ability to unbreak it.

This needs either a proper solution that won't lead to broken webpages or the per block css feature should be punted from 7.0.

This ticket was mentioned in PR #11347 on WordPress/wordpress-develop by @glendaviesnz.


3 weeks ago
#43

  • Keywords has-unit-tests added

Adds capability-gated CSS stripping so that when a user without edit_css saves a post, any style.css attributes are surgically removed from block comments using WP_Block_Parser::next_token().

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

#44 @ramonopoly
3 weeks ago

won't stop users from potentially breaking a page without the ability to unbreak it.

Wouldn't revisions mitigate this somewhat?

Given the back and forth on this issue, and the lengths folks went to explore various solutions, I don't see a path to 7.0 that will satisfy everyone.

See, for example, https://github.com/WordPress/wordpress-develop/pull/11104

Stripping was seen as the quickest and most consistent approach, similar to how the HTML block operates.

What do you folks think? Interested to hear alternatives.

#45 in reply to: ↑ 42 @jonsurrell
3 weeks ago

Replying to jorbin:

[There are concerns] about the idea of just adding a notice as it won't stop users from potentially breaking a page without the ability to unbreak it.

This is a fair concern, however it's not unique to this situation. If a user with unfiltered_html creates a post with a custom HTML block with CSS and/or JavaScript and another user edits it, the content will be mangled. This has long been the case because post content will be filtered based on the user saving the post.

Furthermore, I don't know what a proper solution would look like. One could imagine applying filters only to the blocks that were actually modified in an edit, but nothing like that exists right now.

In practice, I believe this situation is uncommon because unprivileged users like authors often don't have access to edit posts by more privileged users like admins. In my testing, it's necessary to create a post as an admin and then change the post author to another user in order for them to access and edit it.

I think the proposed solution (show a warning) is an acceptable solution right now. It's better than the custom HTML blocks and strikes a balance between respecting capabilities while allowing users to make informed decisions.

Last edited 2 weeks ago by jonsurrell (previous) (diff)

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


12 days ago

#47 @audrasjb
12 days ago

  • Keywords 2nd-opinion added

As per today's bug scrub:

We have several options with this one:

  1. Revert the whole block level css feature and punt it to 7.1
  2. Find a solution for the issue on this feature during the small amount of time we have until 7.0 is released
  3. Show a simple warning notice and punt a more complete solution to 7.1

#48 @ramonopoly
12 days ago

  1. Revert the whole block level css feature and punt it to 7.1
  2. Find a solution for the issue on this feature during the small amount of time we have until 7.0 is released
  3. Show a simple warning notice and punt a more complete solution to 7.1

From what I've read so far, 2 isn't feasible and the strip + warning was the compromise to all the suggestions offered so far.

The strip + warning PR has been approved for the plugin, with a Core patch still to come I guess: https://github.com/WordPress/gutenberg/pull/76650

So that implies 3 could be acceptable to folks?

@glendaviesnz might be able to shed light on the implications of a full revert + punt.

#49 @jonsurrell
6 days ago

The recent assessments seem correct to me.

  1. Revert the whole block level css feature and punt it to 7.1
  2. Find a solution for the issue on this feature during the small amount of time we have until 7.0 is released
  3. Show a simple warning notice and punt a more complete solution to 7.1

I advocate 3.

2 seems too complex and risky for this point in the in the release cycle. I discourage it.

If folks are not happy with the proposed compromise (option 3), I recommend a the revert (option 1).

#50 @apeatling
4 days ago

Adding a concrete data point in support of keeping this feature in 7.0.

I'm building Miles (bymiles.ai), an AI site builder for WordPress with paying customers. This approach works well. Per-block custom CSS is exactly the primitive Miles needs. It's the natural output target for section-level styling that doesn't belong in theme.json globals.

A full revert (option 1) would be a real setback given how much of the block editor's recent direction this unlocks.

For what it's worth, the data-URI / base64 approach from @dmsnell and @sirreal looks like the right long-term answer to me. It keeps KSES doing its one job and moves the encoding responsibility to the system that owns the data.

Short term 3 looks okay, assuming there is a notice, but it's a killer for the user experience on managed hosts that strip this capability from all roles.

Happy to help test whatever lands.

Last edited 4 days ago by apeatling (previous) (diff)

#51 @apeatling
4 days ago

Will edit_css be a standalone capability, or does it resolve to unfiltered_html via map_meta_cap? This materially affects whether trusted tools and AI agents can operate on hardened hosts without implementing questionable workarounds.

Edit: I looked it up and it is not a standalone cap. This means all hosts that disable unfiltered_html are going to break either way. What about making it standalone so it's not painted with the same broad brush as HTML filtering?

Last edited 4 days ago by apeatling (previous) (diff)
Note: See TracTickets for help on using tickets.