Sanitize scanned object HTML at save time - #1833
Conversation
…wp_kses allow-list Adds ingestion-side defense-in-depth alongside the existing render-time fix: edac_sanitize_scanned_html() runs the incoming object HTML through wp_kses() with a wide allow-list covering ordinary post content plus a broad, non-scripting SVG vocabulary (shapes, gradients, filters, text, animation elements) - built to strip only script tags, on* handlers, and <foreignObject>, not to mangle legitimate SVGs. Wired into Insert_Rule_Data::insert(), the single storage choke point for both the REST-submitted JS violation path and the deprecated legacy insert function, ahead of the existing esc_attr() call. xlink:href is registered with wp_kses_uri_attributes() for the duration of the call so it gets the same bad-protocol validation core already applies to the plain href attribute, closing the javascript:-via-xlink:href vector specifically. wp_kses() lowercases attribute names (it's built for case-insensitive HTML), which would otherwise silently break case-sensitive SVG attributes like viewBox or gradientTransform - added edac_restore_svg_attribute_case() to fix the casing back up after sanitizing, so real, safe SVGs survive unmodified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…sanitizer allow-list fe* filter primitives (~20 tags: blur, lighting, turbulence, etc.) and SMIL animation elements (animate/animateTransform/animateMotion/set/mpath) covered effects this plugin's real content - icons, logos, decorative graphics flagged by an accessibility rule - essentially never uses. Dropping them roughly halves the allow-list. A real SVG that happens to use one of these loses that specific effect (the shape/icon itself still renders); that's an acceptable, silent degradation for a rare case, not a functional regression worth the added surface area. Trimmed edac_svg_case_sensitive_attributes() to match (removed entries for attributes that no longer appear in the allow-list at all). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e set edac_scanned_html_allowed_tags() goes from ~200 lines of bespoke per-tag attribute arrays to ~49: one shared attribute set (identity, aria, root, geometry, paint, gradient, text groups) applied uniformly across a flat list of 20 elements via array_fill_keys(). wp_kses() only needs attribute names allow-listed, not semantically scoped per tag, so this is safe - a geometry attribute on a tag that ignores it is inert, not a vulnerability. Dropped <pattern>, <mask>, <marker>, <switch>, and <textPath> - none of which show up in the icon/logo/decorative graphics this plugin's rules actually flag. kses strips the tag but keeps benign child shapes, so the cost of a real SVG using one of these is losing that specific structural wrapper, not its visible content. href/xlink:href narrowed to just <use> (the icon-sprite pattern), still protocol-validated via the existing wp_kses_uri_attributes registration. edac_svg_case_sensitive_attributes() trimmed from 17 entries to the 5 that still matter (viewBox, preserveAspectRatio, gradientUnits, gradientTransform, spreadMethod). Extended the realistic-icon test with clipPath, and added dedicated tests for case-restoration and for confirming the five dropped elements are actually stripped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review feedback from PR #1832 (split into this branch): the key named 'style expression' actually holds an onmouseover event-handler payload. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…_data filter The post-filter re-sanitization block only ran esc_attr() over the object field, so a filter callback could persist markup that bypassed edac_sanitize_scanned_html(). Every other field gets fully re-sanitized there; now the object does too. Idempotent for unfiltered data - kses sees no tags in the already-encoded value and esc_attr() does not double-encode existing entities. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Correct the allow-list docblock: <a> is not excluded - wp_kses() has no namespace awareness, so the 'post' base list's entry also matches <a> inside <svg>. Safe: no on* attributes, protocol-validated href. - Reword the case-restoration map docblock - the allow-list stores lowercased names, not camelCase ones. - Make dangerous-construct test assertions case-insensitive so a lowercased <foreignobject> regression could not slip past them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a custom HTML/SVG allowlist sanitizer, applies it before storing scanned rule objects, restores approved SVG attribute casing and fragment references, and adds PHPUnit coverage for malicious, benign, encoded, non-SVG, and invalid inputs. ChangesScanned HTML/SVG Sanitization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Insert_Rule_Data
participant edac_sanitize_scanned_html
participant wp_kses
participant StoredRuleObject
Insert_Rule_Data->>edac_sanitize_scanned_html: Submit untrusted object markup
edac_sanitize_scanned_html->>wp_kses: Decode entities and sanitize with SVG allowlist
wp_kses-->>edac_sanitize_scanned_html: Return filtered markup
edac_sanitize_scanned_html-->>Insert_Rule_Data: Restore SVG attribute casing
Insert_Rule_Data->>StoredRuleObject: Escape and insert sanitized object
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a sanitization mechanism for scanned HTML and SVG content to prevent XSS vulnerabilities when storing rule data. It adds the edac_sanitize_scanned_html helper function, which leverages wp_kses with a custom SVG-aware allow-list, restores case-sensitive SVG attributes, and validates xlink:href URIs. Comprehensive unit tests are also added. Feedback suggests optimizing the case-restoration regex replacement into a single pass using preg_replace_callback and adding a null coalescing fallback for the object key in $rule_data to prevent potential undefined key warnings.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@admin/class-insert-rule-data.php`:
- Line 166: The INSERT path in the rule data construction re-sanitizes the
already escaped value assigned to `$rule_data['object']`, causing double
encoding unlike UPDATE. In the relevant insert logic, use the existing sanitized
`$rule_data['object']` directly, or decode it before sanitizing only if the
filter can inject raw markup, ensuring INSERT and UPDATE store the same
single-escaped value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e978aad-84f2-48a4-aacb-a370cd26fb61
📒 Files selected for processing (3)
admin/class-insert-rule-data.phpincludes/helper-functions.phptests/phpunit/helper-functions/SanitizeScannedHtmlTest.php
edac_sanitize_scanned_html() ran wp_kses() on the raw input, but the stored value is html_entity_decode()d again by every display consumer (edac_parse_html_for_media, Frontend_Highlight). Entity-encoded markup such as `<img src=x onerror=alert(1)>` is inert text at kses time, so it survived sanitization and revived into a live tag on that downstream decode - a stored-XSS bypass, confirmed against real WP and at multiple encoding depths. Fix: fully decode entities to a fixed point before wp_kses() runs, so it sees the exact markup a browser will ultimately parse. Multiply-encoded payloads (&lt;...) collapse too; html_entity_decode only contracts, so there is no expansion risk. Also addresses review feedback on this PR: - Insert path guards `object` with ?? '' in case a filter unset it (Gemini). - edac_restore_svg_attribute_case() now restores all case-sensitive attributes in a single outer pass over tags instead of one preg_replace per attribute (Gemini). The naive flat-alternation form only fixes the first attribute per tag because it consumes the opening '<'. - Adds tests for single/double/triple/numeric entity-encoded payloads and the full store -> display-decode round trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Security fix pushed (86ce4ae) — entity-smuggling bypass closedAdversarial review found (and I reproduced against real WordPress) that the sanitizer could be bypassed with entity-encoded markup: At Fix: Added tests cover single/double/triple/numeric-entity payloads and the full store → display-decode round trip (the actual exploit path), asserting no live Also in this commit: the two Gemini items ( Note: this branch is still output-agnostic; in a release it will sit behind PR #1832's data-URI rendering. The sanitizer is now a genuine save-time boundary rather than relying on safe output alone. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/phpunit/helper-functions/SanitizeScannedHtmlTest.php (1)
30-40: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCover all security-sensitive SVG exclusions.
The
onbegin animatecase only asserts thatonbegindisappears, so<animate>could remain undetected. The suite also lacks regression cases for the explicitly excluded<image>,<filter>, and filter primitive elements. Add vectors asserting those tag names are absent.Suggested additional cases
+ 'animate element' => [ '<svg><animate attributeName="x" /></svg>', '<animate' ], + 'image element' => [ '<svg><image href="image.svg" /></svg>', '<image' ], + 'filter element' => [ '<svg><filter id="f"><feGaussianBlur stdDeviation="1" /></filter></svg>', '<filter' ], + 'filter primitive' => [ '<svg><filter id="f"><feGaussianBlur stdDeviation="1" /></filter></svg>', '<feGaussianBlur' ],Also applies to: 113-135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/phpunit/helper-functions/SanitizeScannedHtmlTest.php` around lines 30 - 40, Expand the malicious_svg_data() provider to verify removal of security-sensitive SVG elements, not just attributes: update the onbegin animate case to assert the animate tag is absent, and add regression vectors for excluded image, filter, and filter primitive elements, asserting each corresponding tag name is absent from sanitized output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/phpunit/helper-functions/SanitizeScannedHtmlTest.php`:
- Around line 30-40: Expand the malicious_svg_data() provider to verify removal
of security-sensitive SVG elements, not just attributes: update the onbegin
animate case to assert the animate tag is absent, and add regression vectors for
excluded image, filter, and filter primitive elements, asserting each
corresponding tag name is absent from sanitized output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 797754ec-bc88-4e00-b53e-9835ff36e7fe
📒 Files selected for processing (3)
admin/class-insert-rule-data.phpincludes/helper-functions.phptests/phpunit/helper-functions/SanitizeScannedHtmlTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- admin/class-insert-rule-data.php
- includes/helper-functions.php
Add regression vectors for the allow-list exclusions that carry a security or external-fetch rationale - raster <image>, the <filter> pipeline and its primitives (<feGaussianBlur>, <feImage>), and SMIL <animate> - asserting the tag names themselves are gone, not just their attributes. Also strengthens the animate case to check the whole tag is removed rather than only its onbegin handler. Addresses CodeRabbit test-coverage feedback on PR #1833. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the test-coverage suggestion in 2e52bec. Added regression vectors asserting the excluded elements are stripped as tags — raster |
1 participant
Summary
edac_sanitize_scanned_html()and applies it inInsert_Rule_Datawhen persisting an issue's scannedobjectHTML/SVG. The allow-list iswp_kses_allowed_html( 'post' )plus a non-scripting SVG vocabulary (shapes, gradients, text,<title>/<desc>);<script>,<foreignObject>,<image>, SMIL animation, filter primitives, and allon*attributes are stripped.href/xlink:hrefon<use>get core's bad-protocol validation via a scopedwp_kses_uri_attributesfilter.viewBoxetc.) thatwp_kses()lowercases, so safe SVGs are not visually broken by sanitization.objectfield after theedac_filter_insert_rule_datafilter runs, closing the one write path where a filter callback could persist unsanitized markup.Notes for testing
wp_kses()normalizes formatting (attribute whitespace, entities), so stored objects may differ byte-wise from pre-sanitization captures. Known interaction: Pro's global-ignore matching (edacp_check_global_ignore_on_insert_rule) comparesobjectexactly, so pre-existing global ignores whose snippets kses reformats may stop matching and need a migration or normalized comparison.Test plan
npm run test:php— 894 tests, 0 failures (includes newSanitizeScannedHtmlTest)./vendor/bin/phpcsclean on changed files🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests