Make WordPress Core

Opened 7 weeks ago

Last modified 6 weeks ago

#65738 new enhancement

Introduce a CSS tokenizer.

Reported by: dmsnell Owned by:
Priority: normal Milestone: Future Release
Component: General Version:
Severity: normal Keywords: dev-feedback
Cc: Focuses:

Description (last modified by westonruter)

Many operations within WordPress need to process CSS content, whether inline style elements with their list of declarations, STYLE element CSS, or theme.json-like CSS-inside-JSON.

Currently, code tends to run naive PCRE patterns against the CSS. This presents a number of common problems related to string values, basic tokenization, and the like.

WordPress could use at least a CSS lexer which would allow code to speak at the level of language tokens instead of repeating the same overly-simplific parsers in different places, leaving opportunity for parsing differentials or misparses.

Related issues

  • #46197 allow calc() in safecss_filter_attr()
  • #46498 allow var() in safecss_filter_attr()

Change History (2)

#1 @khokansardar
7 weeks ago

  • Keywords dev-feedback added

Thanks for opening this, @dmsnell. I spent some time auditing the current state of CSS handling in trunk (7.1-beta3), confirmed the problem, and quantified it. There are twelve ad-hoc CSS parsers in src/ today, and I was able to reproduce misparses in four of them against trunk (7.1-beta3).

safecss_filter_attr() (source:trunk/src/wp-includes/kses.php#L2646) splits on ; with explode(), so any semicolon inside a string or url() cuts a declaration in half. Running the real function, not a re-implementation:

font-family:foo\;bar               => bar
background-image:url("a;b.png")    => b.png")
font-family:"Foo;bar:baz",serif    => font-family:"Foo
COLOR:red                          => ''
color:/* hi */red                  => ''
font-family:"Ben & Jerry"          => ''
color:red;this-is-not-css-at-all   => color:red;this-is-not-css-at-all

The first two are the worst: the property name is discarded and a fragment of the value survives as though it were a declaration. Cases three and four are the inverse — valid CSS rejected, because property names are compared case-sensitively and the /\* in the character blacklist kills any declaration containing a comment.

The last case has a structural cause at source:trunk/src/wp-includes/kses.php#L2934 — when a fragment contains no colon, $found is set to true unconditionally and the safe_style_css allowlist is never consulted. That is also why the third case truncates rather than erroring: the tail is admitted unvalidated.

That third case matters beyond truncation. font-family:"Foo is an unterminated string, and source:trunk/src/wp-includes/blocks/post-featured-image.php#L49 concatenates four separately-filtered values, so an unterminated string from one can swallow the next. It is the same concatenation hazard already documented for </style in WP_REST_Global_Styles_Controller::validate_custom_css() (source:trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php#L683), which is the one place in core that scans rather than pattern-matches.

WP_Theme_JSON::process_blocks_custom_css() (source:trunk/src/wp-includes/class-wp-theme-json.php#L2048) splits nested CSS with explode( '&' ) and then explode( '{' ), which breaks on an ampersand in a string, a url(), or a comment:

content: "a & b"; color: red;    => :root :where(.wp-x){content: "a}:root :where(.wp-x){b"; color: red;}
/* & */ color: red;              => :root :where(.wp-x){/*}:root :where(.wp-x){*/ color: red;}
@media (min-width:600px){…}      => :root :where(.wp-x@media (min-width: 600px) ){color: red;}
& p { & b { color: red } }       => :root :where(.wp-x p){}:root :where(.wp-x b){color: red;}

The count( $part ) !== 2 guard means a second level of nesting is silently dropped, and an at-rule is folded into the selector rather than rejected.

Unrelated live bug found while auditing: WP_Interactivity_API::merge_style_property() (source:trunk/src/wp-includes/interactivity-api/class-wp-interactivity-api.php#L1180) does list( $name, $value ) = explode( ':', $style_assignment ), so every URL value is truncated at the scheme colon — background-image:url(https://example.com/a.png) merges to background-image:url(https;…. A colon-less fragment also emits an Undefined array key 1 warning. Happy to open that as its own ticket; it stands independent of how this one resolves.

On scoping: the twelve call sites split cleanly in two, which I think bears on how much of the stack this ticket should cover. Six need nothing more than a token stream - safecss_filter_attr(), the Interactivity API merge above, the %[\\\(&=}]|/\*% blacklist copied out of kses into source:trunk/src/wp-includes/block-supports/layout.php#L89, the font size preg_replace in source:trunk/src/wp-includes/block-supports/typography.php#L339, the var() matcher at source:trunk/src/wp-includes/class-wp-theme-json.php#L5795, and source:trunk/src/wp-includes/blocks/gallery.php#L73, whose comment states outright that its regex was "borrowed from safecss_filter_attr". The other six need {} structure on top: process_blocks_custom_css(), the preg_match( '#</?\w+#' ) guard at source:trunk/src/wp-includes/block-supports/custom-css.php#L53, both custom-CSS validators, and the selector handling in states.php and WP_Style_Engine_CSS_Rule.

A lexer alone resolves the first six outright and is a prerequisite for the rest, so "at least a CSS lexer" reads to me as the right decomposition rather than a compromise.

Worth noting that the two related tickets, #46197 and #46498, were both closed in 5.8 by adding regex rather than removing it — the recursive \b(?:var|calc|min|max|minmax|clamp|repeat)(\((?:[^()]|(?1))*\)) pattern now in safecss_filter_attr(). Every new CSS function costs another pattern, and each one widens the gap between what core thinks it parsed and what a browser will.

Two things worth settling before any code exists. romainmenke/css-tokenizer-tests, the obvious conformance corpus, declares no license on GitHub; the PHP Toolkit ships a ~90KB derived css-test-cases.json, so it would be good to know how that provenance was cleared before core depends on it. And separately, fixing the COLOR:red, color:/* hi */red and "Ben & Jerry" cases means admitting CSS that safecss_filter_attr() currently
rejects — a behaviour change on a security-adjacent function that likely deserves its own
review rather than riding along with the tokenizer.

Proposed scope:

  1. A read-only WP_CSS_Tokenizer — a cursor over CSS Syntax Level 3 tokens, with no setters and no get_updated_css(). With no mutation surface it cannot corrupt a document, and it keeps parsing separate from policy: no sanitize(), no validate(), no allowlists, no at-rule semantics. That also makes the name honest and leaves …_Processor free for a rewriting layer if and when real consumers need one.
  2. Emit comments as tokens even though the spec discards them, and defer the CR/FF/CRLF and NUL/surrogate normalization so byte offsets stay valid for any later rewriting consumer.
  3. Expose escape-decoded and raw-byte accessors separately — that is what fixes the foo\;bar and foo\}bar cases.
  4. Convert the six token-only call sites one ticket at a time, starting with safecss_filter_attr() against the existing 22 assertions in tests/phpunit/tests/kses.php, so a parsing regression stays bisectable to one consumer.

Open questions I would want answered before starting: whether v1 stops at tokens or also lands a component-value/block layer for the second group; whether url("…") tokenizing as function-token plus string-token rather than url-token warrants a helper so consumers do not each rediscover it; and whether non-UTF-8 input should yield replacement characters or refuse to construct.

Happy to open a PR upon clarification.

#2 @westonruter
6 weeks ago

  • Description modified (diff)
  • Milestone Awaiting ReviewFuture Release
Note: See TracTickets for help on using tickets.