Valid CSS is causing failure in the Additional CSS panel

[drw158]

When using the Gutenberg Additional CSS panel, using some CSS properties get stripped even though they are valid. I believe this is the same sanitization as the Customizer, so I'm issuing here.

Actually, I guess it doesn't strip the CSS, the edit just fails to save. I get an error in the block editor when I try to save — "Save failed". Related, the error message should at least say "Save failed: conflict in CSS" or something like that.

WordPress CSS sanitizer strips @property rules due to angle brackets in syntax values.

When saving CSS that contains @property rules with the required syntax descriptor (e.g., syntax: "<custom-ident>"). This is valid CSS per the spec. The @property at-rule requires syntax values to be wrapped in angle brackets.

Here is the CSS that is causing the failure:

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

Also, the same error happens when you have html in your CSS comments. I believe I should be able to include example html in my CSS comments.

[westonruter]

The problematic code is ​located in WP_Customize_Custom_CSS_Setting::validate():

<?php
if ( preg_match( '#</?\w+#', $css ) ) {
        $validity->add( 'illegal_markup', __( 'Markup is not allowed in CSS.' ) );
}

Clearly this is very naïve and it should be removed.

Commenting out that logic allows the setting to save, but then the angle brackets get stripped out:

@property --animate {
  syntax: "";
  inherits: true;
  initial-value: false;
}

The problem then is inside wp_custom_css_cb() (​source) because it runs the CSS through strip_tags().

[westonruter]

@jonsurrell @dmsnell Should we change the validation to disallow </style? I'm reminded of the code in \WP_HTML_Tag_Processor::set_modifiable_text. Or should escaping on output similarly do:

<?php
$plaintext_content = preg_replace_callback(
        '~</(?P<TAG_NAME>style)~i',
        static function ( $tag_match ) {
                return "\\3c\\2f{$tag_match['TAG_NAME']}";
        },
        $plaintext_content
);

[sabernhardt]

This ticket can address the error in the Customizer panel, but the original report was about using the Site Editor (with a block theme).

@drw158 Could you open an issue in the ​Gutenberg repository about your experience?

[dmsnell]

@westonruter disallowing </style would be one idea, but as you shared from the HTML API, escaping the characters is a more reliable mechanism. STYLE is simpler than SCRIPT in this regard too, because I think we are 100% safe to escape </style without messing anything up, unlike how in SCRIPT we have complications between JavaScript code and JavaScript data.

Still, I would expect this problem to go deep in the KSES stack. Here are typical aspects of the issue that can slow down updates:

• KSES applies at unexpected times in the stack, meaning fixes in one place can reappear in others
• the rules are based more on impulse and ad-hoc fixes than spec, meaning that established behaviors enshrine unwanted outputs from specific input test cases
• downstream code isn’t robust, so Core prefers to corrupt the content rather than leave open a nebulous opportunity for other code to get confused. e.g. stripping tags to remove the chance that it might escape the STYLE element and be interpreted as HTML.

for this particular issue, my initial thought is that we look at this box as a CSS box. it’s not HTML, and we handle it in the editor outside of an HTML context, so we should prepare it on save by turning it into a string that will avoid issues with Core’s treatment of it as HTML.

• can we escape it before save to preserve the input?
• is the existing “sanitizer” aware of CSS syntax? let’s update it for a tactical approach, if we can, to give us more time to resolve the issue properly and for the general case.
• in the long term we should analyze the entire pipeline from CSS box in the browser through Core to the database and back, and ensure that we can stop applying hazardous rules to the content and preserve it.

[sabernhardt]

This ticket can address the error in the Customizer panel

I could have checked previous tickets before writing this. #45168 reported the same error with tags in CSS comments in the Customizer, and replacing the regular expression might fix both situations.

[jonsurrell]

@sabernhardt I think this is the correct place for the ticket because this is Core behavior. There is some CSS validation on the frontend, but it's disconnected from (and often does not agree with) the logic that actually accepts or rejects the CSS.

In the case of the site editor, I believe it's the global styles REST handler that's rejecting here, and ​it appears to have kept the same problematic regular expression:

<?php
function validate_custom_css( $css ) {
        if ( preg_match( '#</?\w+#', $css ) ) {
                return new WP_Error(
                        'rest_custom_css_illegal_markup',
                        __( 'Markup is not allowed in CSS.' ),
                        array( 'status' => 400 )
                );
        }
        return true;
}

[jonsurrell]

I agree with what was already expressed by dmsnell.

It should be trivial to detect potentially problematic STYLE tag contents. It's likely feasible to escape problematic STYLE tag contents.

The difficult part of resolving this will be working through different systems like KSES that expect and rely on STYLE tag contents not matching #</?\w+#.

[soyebsalar01]

I can reproduced the issue.

Adding valid modern css using @property (for example with syntax: "<custom-ident>" ) in the additional css panel fails to save, even though the css is valid per the specification and work proper in the browser.

This appears because of overlay aggresive css sanitization treating angle-bracket css grammer as html.

A possible solution is make a sanitizer context-aware so valid css construct are not rejected.

[westonruter]

#45168 was marked as a duplicate.

[westonruter]

@dmsnell @jonsurrell I'm not sure that KSES applies here? The wp_custom_css_cb() function outputs the CSS without KSES. It just uses strip_tags().

Relax the CSS validation for custom CSS and global styles to allow any text that will not break an inline <style> tag container.

STYLE tags are processed using the "generic raw text parsing algorithm." They contain raw text up until a matching closing tag.

See ​https://html.spec.whatwg.org/multipage/parsing.html#generic-raw-text-element-parsing-algorithm

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

[jonsurrell]

Replying to westonruter:

@dmsnell @jonsurrell I'm not sure that KSES applies here? The wp_custom_css_cb() function outputs the CSS without KSES. It just uses strip_tags().

That certainly makes this easier. I put together an idea for relaxing the validation to reject only problematic text in ​https://github.com/WordPress/wordpress-develop/pull/10641. It works, allowing CSS like the following:

@property --animate {
  syntax: "<custom-ident>";
  inherits: true;
  initial-value: false;
}
@keyframes marquee {
  0% { transform: translateX(0); }
  100% { transform: translateX(-100%); }
}
h1 {
  --animate: marquee;
  overflow-x: hidden;
  overflow-y: visible;
  &::before {
    content: '<cool>';
    color: red;
    display: inline-block;
    animation: var(--animate) 5s linear infinite;
  }
}

However, when I viewed the page it had concatenated my CSS into a larger stylesheet:

<style id='global-styles-inline-css'>
:root { /* … a lot */ }@property --animate {
  syntax: "<custom-ident>";
  inherits: true;
  initial-value: false;
}
@keyframes marquee {
  0% { transform: translateX(0); }
  100% { transform: translateX(-100%); }
}
h1 {
  --animate: marquee;
  overflow-x: hidden;
  overflow-y: visible;
  &::before {
    content: '<cool>';
    color: red;
    display: inline-block;
    animation: var(--animate) 5s linear infinite;
  }
}

/*# sourceURL=global-styles-inline-css */
</style>

The concatenation allows me to bypass the check in my initial version (preg_match( '#</style[ \\t\\f\\n\\r/>]#', $css )) because it appends a newline to whatever the CSS contains. That concatenation concerns me in general.

I still think the CSS check can and should be relaxed, it's obviously wrong and limiting the ability to use valid CSS.

Ensuring that the STYLE tag works as expected really doesn't seem like business of these validation functions. It should probably happen wherever the STYLE tag is being constructed. Working with snippets of text that will be printed inside of HTML is error prone, it's too easy to introduce issues when snippets are naively stitched together.

Relying on the HTML API to construct these tags seems like a much better approch. This has common ground with #64419 and ​PR 10639 where I'm proposing the HTML API for constructing safe, sane SCRIPT tags.

As I explored the space, this PR grew in scope. There are at least two concerns that I plan to split up:

• Use the HTML API to correctly produce STYLE tags across the codebase.
• Relax CSS "validation," (requires additional safety on STYLE tags).

[westonruter]

Replying to westonruter:

@dmsnell @jonsurrell I'm not sure that KSES applies here? The wp_custom_css_cb() function outputs the CSS without KSES. It just uses strip_tags().

Actually, after looking at #38715, I am reminded that KSES may actually be a problem here. It's not that that we need to fix KSES but rather to prevent KSES from running in the first place.

For the Customizer's Custom CSS, the CSS is stored in a custom_css CPT via wp_update_custom_css_post() which does:

<?php
$r = wp_insert_post( wp_slash( $post_data ), true );

This means if a user can't do unfiltered_html, the data will get sanitized with KSES.

I believe we need a workaround similar to what was done in WP_Customize_Manager::save_changeset_post() for storing Customizer changesets (the customize_changeset CPT) where it wraps the call to wp_update_post() as follows:

<?php
add_filter( 'wp_insert_post_data', array( $this, 'preserve_insert_changeset_post_content' ), 5, 3 );
// ...
$r = wp_update_post( wp_slash( $post_array ), true );
// ...
remove_filter( 'wp_insert_post_data', array( $this, 'preserve_insert_changeset_post_content' ), 5 );

The WP_Customize_Manager::preserve_insert_changeset_post_content() method looks like this:

<?php
        public function preserve_insert_changeset_post_content( $data, $postarr, $unsanitized_postarr ) {
                if (
                        isset( $data['post_type'] ) &&
                        isset( $unsanitized_postarr['post_content'] ) &&
                        'customize_changeset' === $data['post_type'] ||
                        (
                                'revision' === $data['post_type'] &&
                                ! empty( $data['post_parent'] ) &&
                                'customize_changeset' === get_post_type( $data['post_parent'] )
                        )
                ) {
                        $data['post_content'] = $unsanitized_postarr['post_content'];
                }
                return $data;
        }

It's ugly, but it bypasses KSES corruption. It also prevents other plugins from corrupting the JSON stored in the post_content in arbitrary ways, which I recall being the added benefit to this approach. Otherwise, this could seem to be achieved via:

<?php
$priority = has_filter( 'content_save_pre', 'wp_filter_post_kses' );
if ( false !== $priority ) {
    remove_filter( 'content_save_pre', 'wp_filter_post_kses', $priority );
}
$r = wp_update_post( wp_slash( $post_array ), true );
if ( false !== $priority ) {
    add_filter( 'content_save_pre', 'wp_filter_post_kses', $priority );
}

I see this is also the case for the wp_global_styles CPT in addition to custom_css. Note how WP_Theme_JSON_Resolver::get_user_data() somewhat anticipates this by checking for JSON decoding errors. It doesn't seem that WP_REST_Global_Styles_Controller is doing anything to prevent KSES from applying. In contrast, kses_init_filters() actually adds a filter specifically for wp_global_styles:

<?php
// Global Styles filtering: Global Styles filters should be executed before normal post_kses HTML filters.
add_filter( 'content_save_pre', 'wp_filter_global_styles_post', 9 );
add_filter( 'content_filtered_save_pre', 'wp_filter_global_styles_post', 9 );

Note that I can't seem to access Additional CSS in the Site Editor when DISALLOW_UNFILTERED_HTML is enabled, so this KSES issue may not be relevant here.

​YES! KSES indeed breaks this as can be seen in the multisite tests:

1) WP_REST_Global_Styles_Controller_Test::test_update_allows_valid_css_with_more_syntax
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 '@property --animate {
-	syntax: "<custom-ident>";
+	syntax: "";
 	inherits: true;
 	initial-value: false;
 }'

That's almost certainly a missing unfiltered_html cap triggering KSES and strip_html.

[jonsurrell]

Replying to westonruter:

I believe we need a workaround similar to what was done in WP_Customize_Manager::save_changeset_post() for storing Customizer changesets (the customize_changeset CPT) where it wraps the call to wp_update_post() as follows…

I think you're right. These default filters on post content are all intended to run on HTML and are unsuitable for other content types like CSS (even if it's CSS intended for a <style> element).

I have concerns because global styles stores JSON with a bunch of fields and I'm not familiar with the entire structure and how it's used or may be used in the future.

Below is an example of a wp_global_styles post type. This all appears to be intended for use inside of CSS, but it's not a single CSS text. It's parts of CSS that's going to be stitched together in some way. That makes it very difficult to reason about what's actually safe in here until we rely on things like the HTML API for constructing STYLE tags where we can be confident that the CSS text is correctly prepared (escaped) for an HTML STYLE tag.

{
  "styles": {
    "css": "@property --animate {\n  syntax: \"<custom-ident>\";\n  inherits: true;\n  initial-value: false;\n}\n@property --base-animation-duration {\n  syntax: \"<time>\";\n  inherits: true;\n  initial-value: 300ms;\n}\n@keyframes marquee {\n  0% {\n    transform: rotate(-10deg) scale(0.9);\n  }\n  100% {\n    transform: rotate(12deg) scale(1.2);\n  }\n}\nmain {\n  h1,\n  h2,\n  h3,\n  h4,\n  h5,\n  h6 {\n    --animate: marquee;\n    position: relative;\n\n    &::after {\n      color: red;\n      display: inline-block;\n      animation: var(--animate) 600ms linear infinite alternate;\n    }\n  }\n\n  h1 {\n    counter-increment: heading-1;\n    counter-reset: heading-2 heading-3 heading-4 heading-5 heading-6;\n    &::after {\n      content: \"<h1> \" counter(heading-1);\n      animation-duration: calc(var(--base-animation-duration) * 2);\n    }\n  }\n  h2 {\n    counter-increment: heading-2;\n    counter-reset: heading-3 heading-4 heading-5 heading-6;\n    &::after {\n      content: \"<h2> \" counter(heading-1) \".\" counter(heading-2);\n      animation-duration: calc(var(--base-animation-duration) * 3);\n    }\n  }\n  h3 {\n    counter-increment: heading-3;\n    counter-reset: heading-4 heading-5 heading-6;\n    &::after {\n      content: \"<h3> \" counter(heading-1) \".\" counter(heading-2) \".\"\n        counter(heading-3);\n      animation-duration: calc(var(--base-animation-duration) * 5);\n    }\n  }\n  h4 {\n    counter-increment: heading-4;\n    counter-reset: heading-5 heading-6;\n    &::after {\n      content: \"<h4> \" counter(heading-1) \".\" counter(heading-2) \".\"\n        counter(heading-3) \".\" counter(heading-4);\n      animation-duration: calc(var(--base-animation-duration) * 7);\n    }\n  }\n\n  h5 {\n    counter-increment: heading-5;\n    counter-reset: heading-6;\n    &::after {\n      content: \"<h5> \" counter(heading-1) \".\" counter(heading-2) \".\"\n        counter(heading-3) \".\" counter(heading-4) \".\" counter(heading-5);\n      animation-duration: calc(var(--base-animation-duration) * 11);\n    }\n  }\n\n  h6 {\n    counter-increment: heading-6;\n    &::after {\n      content: \"<h6> \" counter(heading-1) \".\" counter(heading-2) \".\"\n        counter(heading-3) \".\" counter(heading-4) \".\" counter(heading-5) \".\"\n        counter(heading-6);\n      animation-duration: calc(var(--base-animation-duration) * 13);\n    }\n  }\n}",
    "blocks": {
      "core/paragraph": {
        "css": "content: \"<script>alert(1);</script>\""
      },
      "core/site-title": {
        "typography": {
          "fontFamily": "var(--wp--preset--font-family--beiruti)",
          "fontWeight": "600",
          "letterSpacing": "2.4px",
          "textTransform": "uppercase"
        }
      }
    },
    "color": {
      "background": "var(--wp--preset--color--accent-5)"
    },
    "elements": {
      "h4": {
        "typography": {
          "fontSize": "var(--wp--preset--font-size--large)"
        }
      },
      "heading": {
        "typography": {
          "fontFamily": "var(--wp--preset--font-family--beiruti)",
          "fontWeight": "500",
          "letterSpacing": "-0.02em",
          "lineHeight": "1.02"
        }
      }
    },
    "typography": {
      "fontFamily": "var(--wp--preset--font-family--literata)",
      "fontSize": "var(--wp--preset--font-size--medium)",
      "letterSpacing": "-0.01em",
      "lineHeight": "1.6"
    }
  },
  "settings": {
    "color": {
      "palette": {
        "theme": [
          {
            "color": "#FFFFFF",
            "name": "Base",
            "slug": "base"
          },
          {
            "color": "color-mix(in srgb, currentColor 20%, transparent)",
            "name": "Accent 6",
            "slug": "accent-6"
          }
        ]
      }
    },
    "typography": {
      "fontFamilies": {
        "theme": [
          {
            "name": "Beiruti",
            "slug": "beiruti",
            "fontFamily": "Beiruti, sans-serif",
            "fontFace": [
              {
                "fontFamily": "Beiruti",
                "fontStyle": "normal",
                "fontWeight": "200 900",
                "src": [
                  "file:./assets/fonts/beiruti/Beiruti-VariableFont_wght.woff2"
                ]
              }
            ]
          }
        ]
      },
      "fontSizes": {
        "theme": [
          {
            "fluid": {
              "max": "2.8rem",
              "min": "2rem"
            },
            "name": "Extra Extra Large",
            "size": "2rem",
            "slug": "xx-large"
          }
        ]
      }
    }
  },
  "isGlobalStylesUserThemeJSON": true,
  "version": 3
}

[jonsurrell]

Those KSES filters that target global styles specifically ​run more "santiziation" based on edit_css cap.

​There's a mapping from `edit_css` to `unfiltered_html`, I think those are analogous.

So for users with unfiltered_html (aka edit_css), CSS is allowed and is not sanitized. The unfiltered_html capability these users have also bypasses the HTML stripping behavior of KSES.

But for other users without these capabilities, ​safecss_filter_attr is running as part of the global styles kses filters, then the regular kses post content filters for HTML are running like they do for any user without unfiltered_html. The actual CSS is filtered, then it's embedded in JSON, and the entire JSON string is then filtered as if it were HTML.

I am a bit confused about a multisite test failing with this result, I'm having a hard time reproducing it on a single site. It seems to suggest that the content is being HTML-filtered by KSES for a user with unfiltered_html capability:

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

The HTML API correctly escapes the content and attributes of STYLE tags and is preferable to other methods of creating the tags like simple string concatenation.

This allows for richer CSS to be embedded in STYLE tags without sacrificing safety, like the example CSS in ticket 64418:

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

This does not fix the ticket, but is an important part of the solution to ensure that arbitrary CSS can be safely embedded inside of HTML STYLE tags.

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

• Allow arbitrary text in Customizer custom CSS.
• Protect the CSS data from mangling by KSES HTML filters.

Under some circumstances KSES would run post content filters and change the resulting content like this:

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

This depends on #10656 to ensure styles output is safely printed in the HTML (merged here).

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

[jonsurrell]

I've attached 3 PRs to address this. I've chosen to allow any arbitrary CSS because the HTML API makes it safe to do so. Another option is to reject </style in the CSS content. The only reason to do that is in case extenders are retrieving the custom CSS and dangerously printing it in HTML.

​Use HTML API to generate <style> tags: The HTML API will produce safe HTML by escaping STYLE tag contents as needed. This makes it simple to relax other arbitrary restrictions on the CSS content.

​Global Styles: Allow arbitrary CSS, protect from KSES mangling removes the arbitrary CSS restrictions, allowing any arbitrary CSS. The CSS is stored as JSON in post content. Problematic HTML characters <>& are escaped in the JSON to prevent confusion and mangling by post filters like KSES.

​Customizer: Allow arbitrary custom CSS is similar but for the customizer. The data is not JSON encoded, so the problematic KSES filters are removed before updating or inserting the custom_css post.

[jonsurrell]

Ping @dlh as the ​Customize component maintainer for help reviewing the changes, ​especially for the customizer.

It seems much of this PR is the same as #10667

Yes, this is a prerequisite and is merged to both ​https://github.com/WordPress/wordpress-develop/pull/10667 and #10641.

An aside, this will have to be synced with Gutenberg:

​https://github.com/WordPress/gutenberg/blob/trunk/lib/class-wp-rest-global-styles-controller-gutenberg.php

Thank you for flagging that @ramonjd. This is an interesting case and I'd like to get your feedback.

This depends on the work in ​https://github.com/WordPress/wordpress-develop/pull/10656. That's another Core change to how STYLE tags are constructed. Without that change, unsafe styles can be produced. That's a problem if Gutenberg removes the CSS content restrictions because it won't include the necessary changes to STYLE tags.

How should we approach that?

One option is to maintain the validation in such a way that unsafe CSS is not allowed, @dmsnell shared a suitable snippet ​in this comment. This relaxes the check and should be safe to include in Gutenberg today because the STYLE tag cannot be prematurely closed.

This depends on the work in ​https://github.com/WordPress/wordpress-develop/pull/10656. That's another Core change to how STYLE tags are constructed. Without that change, unsafe styles can be produced. That's a problem if Gutenberg removes the CSS content restrictions because it won't include the necessary changes to STYLE tags.

Dang! Can I take that back about syncing? :trollface:

Yeah, I think we'll have to have validate_custom_css do something if plugin users aren't running 7.0 or whatever version these changes will live in.

In my opinion, Dennis's comment is a safe bet.

perhaps we should deprecate it and leave in the check.

Or, since it's plugin-only, use Dennis's new and improved version! Does that sound reasonable?

I'm quite ignorant on a lot of this.

The last change to that file in Gutenberg was ​https://github.com/WordPress/gutenberg/commit/f77a46674dcf775f1f9b4f234679bb12c9a888a4.

• Does Gutenberg need to keep its version or can it be removed and rely on Core now?
• Does it strictly need to be in sync ASAP, or could it come into sync when Core is ready?
• Gutenberg could use a simplified version of the check until it requires WordPress 7.0, then fully sync and remove the validation check.

[jonsurrell]

In 61418:

Use the HTML API to generate style tags.

The HTML API escapes <style> tag contents to ensure the correct HTML structure. Common HTML escaping is unsuitable for <style> tags because they contain "raw text." The additional safety allows other restrictions, such as rejecting content with <>, to be relaxed or removed because the resulting tag will be well-formed.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/10656.

Props jonsurrell, westonruter, dmsnell, ramonopoly, soyebsalar01, drw158, sabernhardt.
See #64418.

Merged in r61418.

Good questions:

Does Gutenberg need to keep its version or can it be removed and rely on Core now?

For context, the global styles controller was ​created as a dedicated, bundled class because the Core equivalent was being extended and overridden so frequently, and referenced others classes like WP_Theme_JSON and others that were also being extended and overridden, that it became a burden to deal with the compat files and classes.

For example, the controller relies on WP_Theme_JSON_Resolver_Gutenberg or WP_Theme_JSON_Gutenberg, which has been modified in the plugin, and then these might further rely on modified functions/methods and so on.

My instinct is that Gutenberg needs to keep its changes for this reason.

Folks are pretty good at migrating said changes to Core, and theoretically they should be synced in that direction (plugin > Core).

Does it strictly need to be in sync ASAP, or could it come into sync when Core is ready?

I reckon after this merges is fine. Not speaking specifically this PR, but if Core introduces a bugfix/new feature in global styles controller is generally good to sync that back before the next plugin update so plugin users get the benefit (since the plugin uses it's own class WP_REST_Global_Styles_Controller_Gutenberg)

Gutenberg could use a simplified version of the check (a stripos() check for </style is probably sufficient) until it requires WordPress 7.0, then fully sync and remove the validation check.

This sounds wholly reasonable to me. I appreciate you being on top of this! Thank you!

@dmsnell and I discussed this recently. We decided to:

• restore the validate_custom_css check
• implement it as a check for </style in the CSS

This is likely sufficient to allow the valid CSS folks want to use without introducing problems in Gutenberg for WordPress versions that won't include the improved safety when constructing style tags from #10656.

I've updated the check to disallow:

• CSS content that _will_ close a STYLE tag (</style + tag name terminator: " \t\r\f\n/>").
• CSS content that _ends_ with a partial style tag closer (<, </, … </style)

These changes should make the style tag contents safe to save, print, and concatenate with other arbitrary contents even without the STYLE tag change in #10656. They are safe to port to Gutenberg.

(This was another approach I discussed with @dmsnell and it seems like the most appropriate.)

​https://github.com/WordPress/wordpress-develop/pull/10641 includes some protection to ensure bad style tag contents are not saved (including </style> or ending in something that could become </style> after concatenation).

This change should be find because of the work in ​https://github.com/WordPress/wordpress-develop/pull/10656. Is it worth including that additional safety here?

On one hand, this change should be perfectly safe now.

On the other hand, there's not a very good reason to allow possible style tag closers inside of style tags. It will be safely escaped by the HTML API, but I wonder whether it's more helpful to prevent it from ever being stored.

In other words, to restore the validate method but to change this:

if ( preg_match( '#</?\w+#', $css ) ) {

To be rather:

if ( false !== stripos( $css, '</style' ) ) {

Right? That seems fine to me. To be more conservative, and only make it even looser if we find this is actually needed.

​The check I've proposed is a bit more complicated. It also covers text that ends in partial style close tags <, </, … </style. This should ensure that even when concatenated with other text in a style tag, it cannot produce a style close tag. I do believe that these customizer styles are concatenated with other styles by the global styles system, so this check may be worth having.

The HTML API should still protect against broken style tag contents, so I'm not sure whether this protection is worth bringing here as well.

<details>

<summary>code sample</summary>

protected function validate_custom_css( $css ) {
                $length = strlen( $css );
                for (
                        $at = strcspn( $css, '<' );
                        $at < $length;
                        $at += strcspn( $css, '<', ++$at )
                ) {
                        $remaining_strlen         = $length - $at;
                        $possible_style_close_tag = 0 === substr_compare( $css, '</style', $at, min( 7, $remaining_strlen ), true );
                        if ( $possible_style_close_tag ) {
                                if ( $remaining_strlen < 8 ) {
                                        return new WP_Error(
                                                'rest_custom_css_illegal_markup',
                                                sprintf(
                                                        /* translators: %s is the CSS that was provided. */
                                                        __( 'The CSS must not end in "%s".' ),
                                                        esc_html( substr( $css, $at ) )
                                                ),
                                                array( 'status' => 400 )
                                        );
                                }


                                if ( 1 === strspn( $css, " \t\f\r\n/>", $at + 7, 1 ) ) {
                                        return new WP_Error(
                                                'rest_custom_css_illegal_markup',
                                                sprintf(
                                                        /* translators: %s is the CSS that was provided. */
                                                        __( 'The CSS must not contain "%s".' ),
                                                        esc_html( substr( $css, $at, 8 ) )
                                                ),
                                                array( 'status' => 400 )
                                        );
                                }
                        }
                }


                return true;
        }

</details>

[sonaliprajapati]

Create a CSS file:/assets/css/custom.css

Enqueue it:

add_action('wp_enqueue_scripts', function () {

wp_enqueue_style(

'custom-css',
get_stylesheet_directory_uri() . '/assets/css/custom.css',
[],
'1.0'

);

});

Put your CSS inside custom.css:

@property --animate {

syntax: "<custom-ident>";
inherits: true;
initial-value: false;

}

[dmsnell]

@sonaliprajapati what are you trying to communicate with your comment? if you are attempting to suggest a workaround to this issue, that’s not what the report is about. specifically, the report is about the Additional CSS panel itself, and Core allowing legitimate values through in that.

[jonsurrell]

In 61486:

Customize: Allow arbitrary CSS in global styles custom CSS.

Relax Global Styles custom CSS filters to allow arbitrary CSS.

Escape HTML characters <>& in Global Styles data to prevent it from being mangled by post content filters. The data is JSON encoded and stored in post_content. Filters operating on post_content expect it to contain HTML. Some KSES filters would otherwise remove essential CSS features like the <custom-ident> CSS data type because they appear to be HTML tags.

[61418] changed STYLE tag generation to use the HTML API for improved safety.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/10641.

Props jonsurrell, dmsnell, westonruter, ramonopoly, oandregal, jorgefilipecosta, sabernhardt, soyebsalar01.
See #64418.

Merged in r61486.

I'm going to rework this based on what landed in r61486.

No, I'm not. This is ready to go, I'll land it.

[dlh]

@jonsurrell Belatedly, thanks for the ping re. the Customizer. I didn't realize that I was still listed as a maintainer of that component. I stepped away from it a couple years ago. At a glance, the change you mentioned seems OK, but I don't have any special expertise to lend at this point.

Can we get test coverage added for the example CSS in the PR description?

Some users may have the customize capability but not the unfiltered_html capability.

This may not be entirely relevant here because the Custom CSS requires the edit_css capability:

​https://github.com/WordPress/wordpress-develop/blob/98e8753b598d0193bf3005b88a6f784df1455e71/src/wp-includes/class-wp-customize-manager.php#L5758

And this is treated the same as unfiltered_html:

​https://github.com/WordPress/wordpress-develop/blob/1fdc11ad5c86ef38d16b7c88275e4192a131a15e/src/wp-includes/capabilities.php#L594-L595

Thanks for sending this one home @sirreal

I'm AFK for a while, would you mind double checking ​https://github.com/WordPress/gutenberg/pull/74371 and merging if you're happy with it?

I synced it a couple of days ago, but just to make sure we didn't miss anything.

Thank you!

Some users may have the customize capability but not the unfiltered_html capability.

I believe this is covered by edit_css capability as Weston mentioned ​https://github.com/WordPress/wordpress-develop/pull/10667#issuecomment-3757181382.

For those users I think it would be good to run urls through the kses protocol check, see this section of code in the safe css portion of kses.

I'm reluctant to add this if avoidable. If there are specific concerns I'm happy to work on addressing them. Do you know whether this filter was already being run on the content?

One concern I have is that these functions are working with HTML entities, indicating they're not designed to operate on CSS or the RAWTEXT content of STYLE tags. This type of content-type processing mismatch is why this change is necessary.

​https://github.com/WordPress/wordpress-develop/blob/bee33568389cc8dc534fc4c19f1488ded1059386/src/wp-includes/kses.php#L1995-L2007
​https://github.com/WordPress/wordpress-develop/blob/bee33568389cc8dc534fc4c19f1488ded1059386/src/wp-includes/kses.php#L2030-L2043

I'll take it over, thanks @ramonjd.

Thanks everyone! I believe all feedback is addressed.

After further discussion, it seemed prudent to restore the validation method and reject CSS that contains a closing style tag </style> or some prefix thereof in the same way that r61486 validates CSS content for global styles.

I've added that implementation and corresponding tests.

This may not be entirely relevant here because the Custom CSS requires the edit_css capability:

Sorry, wrong cap but it's still possible someone can edit css without using unfiltered HTML with something like:

// Untested so there may be a syntax error.
add_filter( 'map_meta_cap', function ( $cap, $caps ) {
        if ( 'unfiltered_html' === $cap ) {
                $caps[] = 'do_not_allow';
        }
        return $caps;
}, 10, 2 );

The above would place URL protocol restrictions on all users so I think it's needed for this too.

@sirreal Are such protocol restrictions being enforced in the Site Editor?

[jonsurrell]

There is a workaround for this limitation. CSS strings have Unicode escaping, and the syntax uses a string for the CSS type (like <custom-ident> or <angle>), the problematic < character can use CSS string Unicode escapes.

In the CSS type for syntax:, < should be replaced with \3C and > replaced with \3E as follows (white space is important):

/* From ticket description */
@property --animate {
  syntax: "\3C custom-ident\3E";
  inherits: true;
  initial-value: false;
}

/* Arbitrary example */
@property --rotation {
  syntax: "\3C angle\3E";
  inherits: true;
  initial-value: 45deg;
}

h1, h2, h3, h4, h5, h6 {
  transform: rotate( var(--rotation) );
}

The problematic <> characters no longer appear in the CSS. This passes the validation and is not mangled by KSES filters.

I spent a lot of time testing the behavior here. I set up a multisite install for testing testing network admins and site admins and performed the same tests on a single site install.

Multisite site admins do not have sufficient capabilities and their additional CSS edits are rejected. The additional CSS panel does not appear in customize for them and AJAX requests are rejected:

{
  "custom_css[twentyten]": {
    "unauthorized": {
      "message": "Unauthorized to modify setting due to capability.",
      "data": null
    }
  }
}

Multisite network (super) admins have access, just like regular admins on single site installs.

---

I added debugging inside the related filters and I _never_ saw them run when updating this setting. I believe that's because either the user has unfilitered_html/edit_css and the filters do not apply, or the user does not have the capability and the content is rejected before filters would be applied.

---

I've removed the filter changes from this PR because they don't seem to be necessary. I worked on this simultaneously with ​https://github.com/WordPress/wordpress-develop/pull/10641, and I may have changed the filters here after confusing test results on the two PRs.

All tests are passing on this PR _without any changes to filters_. This supports the idea that any user with sufficient capabilities to modify custom CSS will not have the CSS content filtered by KSES.

Discussion related to content filtering should no longer be relevant for this change.

[jonsurrell]

In 61527:

Customize: Allow arbitrary custom CSS.

Update custom CSS validation to allow any CSS except STYLE close tags. Previously, some valid CSS would be rejected for containing HTML syntax characters, like this example:

@property --animate {
  syntax: "<custom-ident>"; /* <-- Validation error on `<` */
  inherits: true;
  initial-value: false;
}

Developed in ​https://github.com/WordPress/wordpress-develop/pull/10667.

Follow-up to [61418], [61486].

Props jonsurrell, westonruter, peterwilsoncc, johnbillion, xknown, sabernhardt, dmsnell, soyebsalar01, dlh.
Fixes #64418.

[peterwilsoncc]

As I mentioned on the pull request, this introduces a security flaw in which users with the edit_css capability but without the unfiltered_html capability can use forbidden protocols in URLs included in the custom CSS.

This can be demonstrated with the plugin:

<?php
/**
 * Plugin name: Always filter html
 * Description: A plugin that removes the unfiltered_html capability from all users.
 */

add_filter( 'map_meta_cap', function ( $caps, $cap ) {
        if ( 'unfiltered_html' === $cap ) {
                $caps[] = 'do_not_allow';
        }
        return $caps;
}, 10, 2 );

I've uploaded a YouTube video ​demonstrating the security flaw.

Steps I took to reproduce the issue

1. Create an admin account
2. Install the plugin above
3. Activate the 2014 theme
4. Create a post
5. Switch to code mode
6. Enter the code <span style="background-image: url(bad://example.com);">Bad protocol</span>
7. Save the post
8. The style attribute will be removed by kses
9. Open the customizer custom CSS editor
10. Enter the code #selector {background-image: url(bad://example.com);}
11. Click the publish button
12. Navigate to the front end of the site
13. View source
14. Search for bad: in the markup
15. Observe the code has been saved.

[dmsnell]

this introduces a security flaw

Just to follow-up here, I believe this is mistaken. Without diving into security issues in this ticket, the reproduction steps are solid, but also unaffected by this patch. The steps reproduced the behavior long before this patch, and they work in the same way after the patch.

I was able to reproduce using the steps WordPress 6.5; I did not test prior to that. The snippets in the reproduction steps include content that doesn’t trigger any of the changed code paths in this ticket either.

If I have overlooked something, I apologize, though my testing has revealed with high confidence that this ticket/patch/changeset does not, as claimed, introduce this security flaw.

[jonsurrell]

I'm closing this as fixed. The behaviors described in in this comment are pre-existing and did not change as a result of this work. Any issues that may exist can be addressed independently.