Skip to content

fix: sanitize SVG without HTML reparsing#3707

Open
Arukuen wants to merge 2 commits into
developfrom
fix/3705-custom-SVG
Open

fix: sanitize SVG without HTML reparsing#3707
Arukuen wants to merge 2 commits into
developfrom
fix/3705-custom-SVG

Conversation

@Arukuen
Copy link
Copy Markdown
Contributor

@Arukuen Arukuen commented May 19, 2026

fixes #3705

safeHTML() is incompatible here because it sanitizes by parsing the input as HTML, not as SVG/XML.

It transforms:

<path />
<path />

into:

<path>
	<path></path>
</path>

which makes it invalid. What we did is to get the the safeHTML code from wordpress/dom and only mimic the removal of script (already done) and on. For reference, this is the original implementation:

/**
 * Internal dependencies
 */
import remove from './remove';

/**
 * Strips scripts and on* attributes from HTML.
 *
 * @param {string} html HTML to sanitize.
 *
 * @return {string} The sanitized HTML.
 */
export default function safeHTML( html ) {
	const { body } = document.implementation.createHTMLDocument( '' );
	body.innerHTML = html;
	const elements = body.getElementsByTagName( '*' );
	let elementIndex = elements.length;

	while ( elementIndex-- ) {
		const element = elements[ elementIndex ];

		if ( element.tagName === 'SCRIPT' ) {
			remove( element );
		} else {
			let attributeIndex = element.attributes.length;

			while ( attributeIndex-- ) {
				const { name: key } = element.attributes[ attributeIndex ];

				if ( key.startsWith( 'on' ) ) {
					element.removeAttribute( key );
				}
			}
		}
	}

	return body.innerHTML;
}

Summary by CodeRabbit

  • Bug Fixes
    • Improved security of page icon SVG handling by strengthening sanitization of inline content and attributes to prevent potential vulnerabilities when processing icon data.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

Warning

Rate limit exceeded

@Arukuen has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 52 minutes and 56 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 48147d19-df8a-4cfb-8924-e70129cb6c9e

📥 Commits

Reviewing files that changed from the base of the PR and between 3f34fd5 and cfefd3b.

📒 Files selected for processing (1)
  • src/plugins/page-icons/page-icons.js
📝 Walkthrough

Walkthrough

The PR refactors SVG sanitization in the page-icons plugin by removing the @wordpress/dom safeHTML dependency. Instead, parseSVGString extends regex-based sanitization to explicitly strip inline event-handler attributes (on*) and updates attribute filtering to skip attributes starting with on, returning the regex-sanitized SVG directly.

Changes

SVG Sanitization Refactoring

Layer / File(s) Summary
Replace safeHTML with comprehensive regex sanitization
src/plugins/page-icons/page-icons.js
parseSVGString removes the safeHTML import, adds regex to strip inline event-handler attributes (onload, onclick, etc.) in double-quoted, single-quoted, and unquoted forms, removes the safeHTML(rawInnerSVG) call, and updates attribute extraction to also skip attributes starting with on.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A rabbit scrubbed the SVG clean,
With regex patterns sharp and keen,
No more safeHTML—just patterns tight,
Event handlers stripped from sight,
Sanitized markup, secure and bright!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: sanitize SVG without HTML reparsing' is fully related to the main change, clearly summarizing the primary modification to avoid HTML reparsing during SVG sanitization.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/3705-custom-SVG

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 19, 2026

🤖 Pull request artifacts

file commit
pr3707-stackable-3707-merge.zip cfefd3b

github-actions Bot added a commit that referenced this pull request May 19, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/plugins/page-icons/page-icons.js`:
- Around line 45-48: The regexes that scrub on* attributes from rawInnerSVG only
match when '=' immediately follows the attribute name, so attributes like
`onload = "..."` slip through; update the three replace calls that operate on
rawInnerSVG (the ones currently matching /\son[\w-]+="[^"]*"/gi,
/\son[\w-]+='[^']*'/gi, and /\son[\w-]+=[^\s"'=<>`]+/gi) to allow optional
whitespace around the '=' (e.g. match `\s*=\s*`), and apply the same change to
the identical scrub at the later occurrence around line 84 to ensure attributes
with spaces are removed consistently.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8952b20b-83ae-45ee-b6a6-d5a568bbdc64

📥 Commits

Reviewing files that changed from the base of the PR and between 2438d9e and 3f34fd5.

📒 Files selected for processing (1)
  • src/plugins/page-icons/page-icons.js

Comment thread src/plugins/page-icons/page-icons.js Outdated
github-actions Bot added a commit that referenced this pull request May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Icon Block - custom SVG icon not visible in editor

1 participant