Make WordPress Core

Opened 4 weeks ago

Closed 4 weeks ago

Last modified 4 weeks ago

#65832 closed defect (bug) (fixed)

KSES: SVG presentation properties added in 7.1 are allowlisted but their canonical values are still dropped

Reported by: courane01 Owned by: wildworks
Priority: normal Milestone: 7.1
Component: Formatting Version: 7.1
Severity: normal Keywords: has-patch has-unit-tests has-test-info dev-reviewed
Cc: Focuses:

Description

Follow-up to #65457 / wordpress-develop#12169, which added SVG presentation attributes to safe_style_css.

The property names were added to the allowlist, but neither structure that governs their values was extended to match, so the canonical value form of the new properties is still stripped.

The mechanism

Two structures in safecss_filter_attr() were left unchanged:

  • $css_url_data_types (wp-includes/kses.php:2900-2909) — the properties permitted to contain url(...). Still only background, background-image, cursor, filter, list-style, list-style-image.
  • The function-strip list (kses.php:3007) — var|calc|min|max|minmax|clamp|repeat. No transform or SVG shape functions.

So the gate at kses.php:3021 still rejects the declaration:

$allow_css = 0 === preg_match( '%[\\\(&=}]|/\*%', $css_test_string );

Steps to reproduce

As a user without unfiltered_html (Author on single site; any non-super-admin on multisite):

wp_kses_post( '<p style="transform:rotate(45deg)">x</p>' );
// actual:   <p>x</p>   — the whole style attribute is removed
// expected: <p style="transform:rotate(45deg)">x</p>

The same happens through any save path, since wp_filter_post_kses hangs on content_save_pre (kses.php:2546-2565).

Expected vs actual

Measured on a live 7.1-RC2 install (single site, PHP 8.3.32, Twenty Twenty-Five):

Declaration Result Note
transform: rotate(45deg) DROP allowlisted at kses.php:2881
transform: translate(10px, 20px) DROP
transform: scale(2) DROP
clip-path: url(#c) DROP allowlisted at :2849
clip-path: inset(10px) DROP
mask: url(#m) DROP allowlisted at :2851
fill: url(#grad1) DROP allowlisted at :2822
transform: none KEEP control — function-free
transform-origin: 50% 50% KEEP control
fill: #ff0000 KEEP control
stroke-width: 2 KEEP control
color: var(--wp--preset--color--primary) KEEP control — var is on the strip list
width: clamp(1rem, 2vw, 3rem) KEEP control — clamp is on the strip list

The passing controls matter: the filter is not over-blocking, and these same properties work with function-free values. It is specifically the functional form that cannot get through.

transform is the highest-impact case — it is not SVG-specific, it is common, and rotate()/translate()/scale() are essentially its only real values. clip-path: url(#id) and mask: url(#id) are the most self-contradictory, since referencing an SVG element by fragment is the reason to allowlist those properties at all.

Not a regression

All of these properties are absent from the 7.0.3 allowlist and present in 7.1, so no previously-working content breaks. This is feature-incomplete rather than a regression, and there is no security implication — the failure direction is fail-closed.

Why this looks unintentional

PR 12169 touches only src/wp-includes/kses.php and tests/phpunit/tests/kses.php; neither $css_url_data_types nor the strip regex appears in the diff. It adds 20+ properties but 9 test cases, and no test value contains a function call:

fill: none          fill-rule: evenodd     stroke: red
stroke-width: 2     stroke-linecap: round  paint-order: stroke
vector-effect: non-scaling-stroke          clip-rule: evenodd
text-anchor: middle

transform, clip-path, mask and marker-* shipped with no tests at all, and fill is tested only as fill: none.

Suggested fix

  1. Add the url-bearing SVG properties (fill, stroke, clip-path, mask, marker-start, marker-mid, marker-end) to $css_url_data_types.
  2. Add the transform and shape functions (rotate, translate, scale, matrix, skew, inset, circle, ellipse, polygon, path) to the strip list at kses.php:3007.

#24157 (safecss_filter_attr doesn't allow rgb() in inline styles) is the same mechanism on a different property set, and extending the strip list would address both. Prior instances of this shape were each accepted and fixed the same way: #46197 (calc(), 5.8), #55966 (min(), 6.1), #56353 (CSS custom properties, 6.1). #53815 (min()/max()/minmax()) is still open.

Attachments (1)

Before after.png (66.4 KB ) - added by mdridipu 4 weeks ago.

Download all attachments as: .zip

Change History (14)

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


4 weeks ago
#1

  • Keywords has-patch has-unit-tests added

Follow-up to #65457 / #12169. Fixes #65832.

## The problem

#65457 added 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:

Declaration Before this PR Expected
transform: rotate(45deg) stripped preserved
transform: translate(10px, 20px) stripped preserved
clip-path: url(#myClipper) stripped preserved
fill: url(#gradient1) stripped preserved
mask: url(#myMask) stripped preserved
transform: none preserved ✓ preserved ✓
fill: #ff0000 preserved ✓ preserved ✓

The failure mode is the ( character check at kses.php:3015. After stripping known-safe functions (var, calc, etc.), any remaining ( causes the whole declaration to be dropped. Transform functions like rotate(45deg) were never added to that strip list.

The second failure: clip-path: url(#id) and fill: 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 include clip-path, fill, stroke, mask, marker, marker-start, marker-mid, marker-end. This routes their url() values through the existing URL validator — which already blocks javascript:, vbscript:, and other bad protocols via wp_kses_bad_protocol(). No new security surface; the same gate that protects background-image: url(...) now protects these too.

2. Extend the CSS function strip regex to cover:

  • CSS transform functions: rotate, rotateX/Y/Z, rotate3d, translate, translateX/Y/Z, translate3d, scale, scaleX/Y/Z, scale3d, skew, skewX, skewY, matrix, matrix3d, perspective
  • CSS shape functions (for clip-path): inset, circle, ellipse, polygon, path

These 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():

  • Transform functions: rotate(), translate(), scale(), matrix(), skewX(), skewY()
  • Chained: rotate(45deg) scale(1.5)
  • Regression control: transform: none (function-free value, was already passing)
  • clip-path shapes: inset(), circle(), ellipse(), polygon()
  • SVG url() references: clip-path: url(#id), fill: url(#id), mask: url(#id), marker-start: url(#id), marker-end: url(#id)
  • Security regressions: fill: url(javascript:alert(1))"", clip-path: url(javascript:alert(1))""

Full kses group: 462 tests, 1537 assertions — all passing.

---

---

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.

#2 @hasnainashfaq
4 weeks ago

I've opened a PR for this: https://github.com/WordPress/wordpress-develop/pull/12942

The fix is two targeted changes to safecss_filter_attr() in kses.php:

$css_url_data_types - adds clip-path, fill, stroke, mask, marker, marker-start, marker-mid, marker-end. This routes their url() values through the existing URL-validation path that already blocks javascript: and other bad protocols via wp_kses_bad_protocol().

CSS function strip regex - extends the existing var|calc|min|max|... pattern to also strip CSS transform functions (rotate, translate, scale, matrix, skew*, perspective, and their 3D variants) and CSS shape functions for clip-path (inset, circle, ellipse, polygon, path). Once stripped, the remaining test string has no ( characters and passes the safety check. The actual CSS value in the output is untouched.

Both changes are strictly additive - no existing behaviour changes. The security model is unchanged: fill: url(javascript:alert(1)) still produces "".

Adds 20 test cases covering transform functions (including chained), clip-path shapes, SVG url() references for fill/mask/marker, and the two javascript: security regressions. Full kses group: 462 tests, 1537 assertions, all passing.

Last edited 4 weeks ago by hasnainashfaq (previous) (diff)

#3 @wildworks
4 weeks ago

  • Milestone Awaiting Review7.1

Thanks for the report. Let's try to fix this in 7.1.

@mdridipu
4 weeks ago

#4 @mdridipu
4 weeks ago

Tested: WordPress 7.1-RC2 on Local (Windows) Patch #12942

Before patch: wp_kses_post() returned <p>x</p>, removing style="transform:rotate(45deg)".
After applying the patch: The same test returned <p style="transform:rotate(45deg)">x</p>.

Result: Resolved after applying the patch.


#5 @khokansardar
4 weeks ago

  • Keywords has-test-info added

Patch testing report

Patch / PR tested

Environment

WordPress: 7.1-RC2-63095-src
PHP: 8.2.18 (Docker)
MySQL: 8.0.36
OS: macOS 26.5.2
Local wordpress-develop @ http://localhost:8889

Steps

  1. Ran a 53-declaration matrix through safecss_filter_attr() against both states — patched, then with kses.php reverted to trunk — and diffed the results to classify each case.
  2. Ran the ticket's verbatim repro, and published a post as an Author (no unfiltered_html) through content_save_pre in both states.
  3. Ran the new PHPUnit cases against unpatched source, then the full kses group patched. PHPCS, PHPStan, regex benchmark.

Results

  • Ticket repro on trunk: reproduced — wp_kses_post( '<p style="transform:rotate(45deg)">x</p>' ) returned <p>x</p>.
  • Ticket repro with patch: pass (fixes) — returned <p style="transform:rotate(45deg)">x</p>.
  • Save path as Author: pass (fixes) — stored content <p>x</p> on trunk → <p style="transform:rotate(45deg)">x</p> patched, and it renders on the front end.
  • All 7 DROP rows of the ticket's table: pass (fixes) — transform: rotate(45deg), transform: translate(10px, 20px), transform: scale(2), clip-path: url(#c), clip-path: inset(10px), mask: url(#m), fill: url(#grad1) each returned '' on trunk, now preserved.
  • All 6 KEEP controls: pass (guard) — identical before and after. The ticket's predicted trunk behaviour reproduced row for row.
  • Rest of the patch surface: pass (fixes) — 25 further declarations went '' → preserved (matrix(), skewX/Y(), rotate3d(), perspective(), chained transforms, circle(), ellipse(), polygon(), path(), rect(), xywh(), shape(), and url() on stroke/marker-*).
  • Hostile protocols: pass (guard) — javascript:, vbscript:, data:, quoted and escaped-paren variants on fill/clip-path/mask/stroke/marker-start all returned '' before and after.
  • New PHPUnit cases against unpatched source: 24 of 27 fail. The 3 that pass either way are transform: none and the two javascript: cases — deliberate regression guards.
  • Full kses group patched: 470 tests, 1545 assertions, OK. PHPCS and PHPStan clean.
  • Performance: 0.104 µs → 0.126 µs per declaration; oversized input still fails closed via the existing null === $css_test_string guard.

Conclusion

PR #12942 resolves the ticket. Every DROP row is fixed, every KEEP control is untouched, and the fix holds through the real save path. The change is minimal and additive: all eight properties added to $css_url_data_types are already in the safe_style_css allowlist, the set is complete, and no signature, hook, return shape or default behaviour changes. url() values route through the same wp_kses_bad_protocol() gate that already protects background-image.

Two notes, neither a blocker. The diff goes past the function list the ticket suggests (axis/3D transform variants, perspective(), rect()/shape()/xywh()) — justified, since a partial list would recreate the same inconsistency this ticket reports. And because the strip list is global rather than per-property, color: rect(expression(alert(1))), --x: rect(url(javascript:alert(1))) and transform: -webkit-rotate(45deg) are now preserved; I verified each is pre-existing in kind, as trunk already preserves the identical calc() and -webkit-calc() forms. This widens an accepted design rather than introducing a new bypass.

Recommend commit.

@wildworks commented on PR #12942:


4 weeks ago
#6

@tyxla, do you think this PR is ready for a commit?

@tyxla commented on PR #12942:


4 weeks ago
#7

@tyxla, do you think this PR is ready for a commit?

Yup, looking good from my end. Thanks 🙌

#8 @wildworks
4 weeks ago

  • Owner set to wildworks
  • Resolutionfixed
  • Status newclosed

In 63180:

KSES: Allow functional CSS values in inline styles.

The presentation properties allowlisted in [62530] were accepted in name only: any declaration whose value contained a function call was dropped in full, so for users without the unfiltered_html capability those properties remained unusable in their canonical form. URL references are routed through the existing protocol validation, so the security model is unchanged.

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

Follow-up to [62530].

Props courane01, hasnainashfaq, irozum, khokansardar, mdridipu, tyxla.
Fixes #65832.

#9 @wildworks
4 weeks ago

  • Keywords dev-feedback added
  • Resolution fixed
  • Status closedreopened

Reopening #65832 to request backporting [63180] to 7.1 branch

#10 @wildworks
4 weeks ago

Reopening #65832 to request backporting [63180] to 7.1 branch

@tyxla @westonruter @mcsf, would you mind reviewing this if you have time?

#11 @joedolson
4 weeks ago

  • Keywords dev-reviewed added; dev-feedback removed

Approving for backport to 7.1.

#12 @joedolson
4 weeks ago

  • Resolutionfixed
  • Status reopenedclosed

In 63229:

KSES: Allow functional CSS values in inline styles.

The presentation properties allowlisted in [62530] were accepted in name only: any declaration whose value contained a function call was dropped in full, so for users without the unfiltered_html capability those properties remained unusable in their canonical form. URL references are routed through the existing protocol validation, so the security model is unchanged.

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

Follow-up to [62530].

Reviewed by joedolson
Merges [63180] to the 7.1 branch.

Props courane01, hasnainashfaq, irozum, khokansardar, mdridipu, tyxla.
Fixes #65832.

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


4 weeks ago

Note: See TracTickets for help on using tickets.