Formatting: Allow CSS transform and SVG shape functions in safecss_filter_attr() - #12942
Formatting: Allow CSS transform and SVG shape functions in safecss_filter_attr()#12942HasnainAshfaq wants to merge 2 commits into
Conversation
…lter_attr(). #65457 added SVG presentation properties (transform, clip-path, fill, stroke, mask, marker-*) to the KSES CSS allowlist, but the two structures that govern their accepted value forms were not updated. This meant: - transform: rotate(45deg) — dropped (( character was rejected) - clip-path: url(#myClipper) — dropped (url() not validated for clip-path) - fill: url(#gradient1) — dropped (url() not validated for fill) Two changes: 1. Expand $css_url_data_types to include the SVG properties that accept url() references (clip-path, fill, stroke, mask, marker-*). This routes their values through the existing URL-validation path that already protects against javascript: and other bad protocols. 2. Extend the CSS function strip regex (the one that removes var(), calc(), etc. before the backslash/paren safety check) to also cover CSS transform functions (rotate, translate, scale, matrix, skew*, perspective) and CSS shape functions used in clip-path (inset, circle, ellipse, polygon, path). These are purely geometric/visual values with no script execution risk; the precedent is calc() (#46197), min/max (#55966), and CSS custom properties (#56353). Adds test cases for: - transform: rotate(), translate(), scale(), matrix(), skewX(), skewY() - Chained transform functions - clip-path: inset(), circle(), ellipse(), polygon() - clip-path: url(), fill: url(), mask: url(), marker-start/end: url() - Security regression: javascript: URLs in SVG url() references blocked Fixes #65832. Follow-up to #65457.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
irozum
left a comment
irozum
left a comment
There was a problem hiding this comment.
This looks like a clean, well-targeted fix — it directly implements the fix that ticket #65832 itself proposed, completing the two structures ($css_url_data_types and the function-strip regex) that #65457/#12169 left unsynced with the new SVG allowlist.
Checked out the branch and ran the full kses group locally (462 tests, 1537 assertions, all green), plus phpcs lint:errors on both changed files (clean). Also reproduced the bug by hand via wp_kses_post(): on trunk, <p style="transform:rotate(45deg)">x</p> has its entire style attribute stripped; with this patch it's preserved, while fill:url(javascript:alert(1)) still correctly resolves to an empty declaration. I also grepped the other core callers of safecss_filter_attr() (block supports, WP_Theme_JSON, several core blocks) — the change only loosens previously fail-closed values, so there's no back-compat concern for existing callers.
One small non-blocking note: stroke and bare marker/marker-mid are added to $css_url_data_types but don't get a dedicated url() test case (only fill, clip-path, mask, marker-start, marker-end do). The code path is shared and clearly correct from the other cases, so this isn't blocking — just worth rounding out for completeness.
There was a problem hiding this comment.
Pull request overview
Updates safecss_filter_attr() (KSES safe inline-style sanitizer) so that newly allowlisted SVG/CSS presentation properties can retain common functional values, specifically transform/shape functions and url() references, and adds PHPUnit coverage for these cases.
Changes:
- Route additional SVG presentation properties through the existing
url()validation logic by expanding$css_url_data_types. - Extend the CSS function-stripping regex to include transform and clip-path shape functions so balanced parentheses don’t cause declarations to be dropped.
- Add new PHPUnit data-provider cases covering transform functions, clip-path shapes, and SVG
url()references (includingjavascript:regressions).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/wp-includes/kses.php |
Adds SVG properties to URL-validated list and expands the function-stripping regex in safecss_filter_attr(). |
tests/phpunit/tests/kses.php |
Adds new data_safecss_filter_attr() cases for transforms, clip-path shapes, and SVG url() references. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| */ | ||
| $css_test_string = preg_replace( | ||
| '/\b(?:var|calc|min|max|minmax|clamp|repeat)(\((?:[^()]|(?1))*\))/', | ||
| '/\b(?:var|calc|min|max|minmax|clamp|repeat|rotate|rotateX|rotateY|rotateZ|rotate3d|translate|translateX|translateY|translateZ|translate3d|scale|scaleX|scaleY|scaleZ|scale3d|skew|skewX|skewY|matrix|matrix3d|perspective|inset|circle|ellipse|polygon|path)(\((?:[^()]|(?1))*\))/', |
There was a problem hiding this comment.
The observation is correct, but I'm not sure it should be addressed here. The pattern has always been case-sensitive. Whether to accept uppercase function names is out of scope for this PR and would be better handled in its own ticket.
There was a problem hiding this comment.
Agreed that it's a separate issue and shouldn't block this one, but before merging, let's make sure this is raised as a separate ticket. Solution may be simple (just adding the i regex flag) but we still need to add some tests and ensure it doesn't break anything.
t-hamano
left a comment
t-hamano
left a comment
There was a problem hiding this comment.
Thanks for the follow-up! It looks good to me.
| * @since 6.5.0 Added support for `background-repeat`. | ||
| * @since 6.6.0 Added support for `grid-column`, `grid-row`, and `container-type`. | ||
| * @since 6.9.0 Added support for `white-space`. | ||
| * @since 7.1.0 Extended gradient support to allow any single-level nested function. |
There was a problem hiding this comment.
| * @since 7.1.0 Extended gradient support to allow any single-level nested function. | |
| * Added support for transform functions, `clip-path` basic shapes, | |
| * and URLs in the SVG element reference properties. |
Let's update the docblock.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| */ | ||
| $css_test_string = preg_replace( | ||
| '/\b(?:var|calc|min|max|minmax|clamp|repeat)(\((?:[^()]|(?1))*\))/', | ||
| '/\b(?:var|calc|min|max|minmax|clamp|repeat|rotate|rotateX|rotateY|rotateZ|rotate3d|translate|translateX|translateY|translateZ|translate3d|scale|scaleX|scaleY|scaleZ|scale3d|skew|skewX|skewY|matrix|matrix3d|perspective|inset|circle|ellipse|polygon|path)(\((?:[^()]|(?1))*\))/', |
There was a problem hiding this comment.
Some of the functions I don't see here are: rect(), xywh(), and shape(). Should we add those too? Sounds like we should have a more thorough lookup to ensure we're not missing any others.
Follow-up to #65457 / #12169. Fixes #65832.
The problem
#65457added 20+ SVG presentation properties to the KSES CSS allowlist, but left two internal structures unchanged. This means the new properties are allowlisted in name only — their functional values are still stripped:transform: rotate(45deg)transform: translate(10px, 20px)clip-path: url(#myClipper)fill: url(#gradient1)mask: url(#myMask)transform: nonefill: #ff0000The failure mode is the
(character check atkses.php:3015. After stripping known-safe functions (var,calc, etc.), any remaining(causes the whole declaration to be dropped. Transform functions likerotate(45deg)were never added to that strip list.The second failure:
clip-path: url(#id)andfill: url(#gradient)need the URL-validation code path, which only runs for properties listed in$css_url_data_types. Those SVG properties were never added there either.The fix
1. Expand
$css_url_data_types(9 lines added) to includeclip-path,fill,stroke,mask,marker,marker-start,marker-mid,marker-end. This routes theirurl()values through the existing URL validator — which already blocksjavascript:,vbscript:, and other bad protocols viawp_kses_bad_protocol(). No new security surface; the same gate that protectsbackground-image: url(...)now protects these too.2. Extend the CSS function strip regex to cover:
rotate,rotateX/Y/Z,rotate3d,translate,translateX/Y/Z,translate3d,scale,scaleX/Y/Z,scale3d,skew,skewX,skewY,matrix,matrix3d,perspectiveclip-path):inset,circle,ellipse,polygon,pathThese are purely geometric/visual — no script execution risk. Precedent:
calc()added in #46197 (5.8),min()/max()in #55966 (6.1), CSS custom properties in #56353 (6.1).Tests
Added 20 cases to
data_safecss_filter_attr():rotate(),translate(),scale(),matrix(),skewX(),skewY()rotate(45deg) scale(1.5)transform: none(function-free value, was already passing)clip-pathshapes:inset(),circle(),ellipse(),polygon()url()references:clip-path: url(#id),fill: url(#id),mask: url(#id),marker-start: url(#id),marker-end: url(#id)fill: url(javascript:alert(1))→"",clip-path: url(javascript:alert(1))→""Full kses group: 462 tests, 1537 assertions — all passing.
Trac ticket: https://core.trac.wordpress.org/ticket/65832
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.