Sanitize scanned object HTML at save time

admin/class-insert-rule-data.php

@@ -65,7 +65,11 @@ public function insert( object $post, string $rule, string $ruletype, string $ru
 			'xpath'             => $selectors['xpath'][0] ?? null,
 			'rule'              => $rule,
 			'ruletype'          => $ruletype,
-			'object'            => esc_attr( $rule_obj ),
+			// Sanitize before esc_attr(): the scanned object HTML/SVG is
+			// untrusted (submitted by any user who can edit the post, via
+			// the JS scan-results REST route) and is later html_entity_decode()d
+			// and re-parsed for display - see edac_sanitize_scanned_html().
+			'object'            => esc_attr( edac_sanitize_scanned_html( $rule_obj ) ),
 			'recordcheck'       => 1,
 			'user'              => get_current_user_id(),
 			'ignre'             => 0,
@@ -159,7 +163,7 @@ public function insert( object $post, string $rule, string $ruletype, string $ru
 				'xpath'             => sanitize_text_field( $rule_data['xpath'] ?? '' ),
 				'rule'              => sanitize_text_field( $rule_data['rule'] ),
 				'ruletype'          => sanitize_text_field( $rule_data['ruletype'] ),
-				'object'            => esc_attr( $rule_data['object'] ),
+				'object'            => esc_attr( edac_sanitize_scanned_html( $rule_data['object'] ) ),
 				'recordcheck'       => absint( $rule_data['recordcheck'] ),
 				'user'              => absint( $rule_data['user'] ),
 				'ignre'             => absint( $rule_data['ignre'] ),

includes/helper-functions.php

@@ -788,6 +788,161 @@ function edac_parse_html_for_media( $html ) {
 	];
 }
 
+/**
+ * Allowed tags/attributes for edac_sanitize_scanned_html() - wp_kses_allowed_html( 'post' )
+ * plus the minimal SVG vocabulary a flagged icon/logo/decorative graphic
+ * actually uses: container, grouping, basic shapes, gradients, text, and the
+ * accessible-name elements (<title>/<desc>). Deliberately excludes <script>,
+ * <foreignObject>, <image>, SMIL animation, filter primitives, and
+ * rarely-used structural extras (<pattern>, <mask>, <marker>, <switch>,
+ * <textPath>) - none of which this plugin's real-world content needs. Note
+ * that <a> IS allowed, via the 'post' base list: wp_kses() has no namespace
+ * awareness, so that entry also matches <a> inside <svg> markup. That's safe
+ * because no on* attribute is ever allowed on anything and href values get
+ * core's bad-protocol validation.
+ *
+ * Every allowed SVG element shares one attribute set: wp_kses() only needs
+ * attribute names allow-listed, not semantically scoped per tag, and a
+ * geometry/paint attribute on a tag that ignores it is harmless. The only
+ * exception is href/xlink:href, which stays scoped to <use> (the icon-sprite
+ * pattern) and is protocol-validated - see edac_sanitize_scanned_html().
+ *
+ * @since x.x.x
+ *
+ * @return array<string, array<string, bool>>
+ */
+function edac_scanned_html_allowed_tags(): array {
+	$identity = [ 'id', 'class', 'style', 'transform', 'role', 'focusable' ];
+	$aria     = [ 'aria-hidden', 'aria-label', 'aria-labelledby', 'aria-describedby' ];
+	$root     = [ 'xmlns', 'xmlns:xlink', 'version', 'viewbox', 'preserveaspectratio' ];
+	$geometry = [ 'x', 'y', 'width', 'height', 'cx', 'cy', 'r', 'rx', 'ry', 'x1', 'y1', 'x2', 'y2', 'fx', 'fy', 'd', 'points', 'dx', 'dy' ];
+	$paint    = [ 'fill', 'fill-rule', 'fill-opacity', 'stroke', 'stroke-width', 'stroke-linecap', 'stroke-linejoin', 'stroke-dasharray', 'stroke-dashoffset', 'stroke-opacity', 'stroke-miterlimit', 'opacity', 'clip-path', 'clip-rule' ];
+	$gradient = [ 'gradientunits', 'gradienttransform', 'spreadmethod', 'offset', 'stop-color', 'stop-opacity' ];
+	$text     = [ 'text-anchor', 'font-family', 'font-size', 'font-weight', 'font-style' ];
+
+	$svg_attributes = array_fill_keys(
+		array_merge( $identity, $aria, $root, $geometry, $paint, $gradient, $text ),
+		true
+	);
+
+	$svg_elements = [
+		'svg',
+		'g',
+		'defs',
+		'symbol',
+		'use',
+		'path',
+		'rect',
+		'circle',
+		'ellipse',
+		'line',
+		'polyline',
+		'polygon',
+		'lineargradient',
+		'radialgradient',
+		'stop',
+		'clippath',
+		'text',
+		'tspan',
+		'title',
+		'desc',
+	];
+
+	$svg = array_fill_keys( $svg_elements, $svg_attributes );
+
+	// Local same-document fragment reference (#id) for the icon-sprite
+	// pattern. edac_sanitize_scanned_html() registers xlink:href with
+	// wp_kses_uri_attributes() for the duration of the call, so both get
+	// the same bad-protocol/URI validation core already applies to the
+	// plain 'href' attribute.
+	$svg['use']['href']       = true;
+	$svg['use']['xlink:href'] = true;
+
+	return array_merge( wp_kses_allowed_html( 'post' ), $svg );
+}
+
+/**
+ * SVG attribute names are case-sensitive per spec (e.g. viewBox,
+ * gradientTransform), but wp_kses() - built for case-insensitive HTML -
+ * lowercases every attribute name it outputs. Left alone, that silently
+ * breaks otherwise-safe SVGs (a lowercased viewbox is simply ignored by
+ * browsers). Keyed by the lowercased name wp_kses() produces. Every SVG
+ * attribute with a case-sensitive canonical spelling that appears (lowercased)
+ * in edac_scanned_html_allowed_tags() must have an entry here.
+ *
+ * @since x.x.x
+ *
+ * @return array<string, string>
+ */
+function edac_svg_case_sensitive_attributes(): array {
+	return [
+		'viewbox'             => 'viewBox',
+		'preserveaspectratio' => 'preserveAspectRatio',
+		'gradientunits'       => 'gradientUnits',
+		'gradienttransform'   => 'gradientTransform',
+		'spreadmethod'        => 'spreadMethod',
+	];
+}
+
+/**
+ * Restore case-sensitive SVG attribute names that wp_kses() lowercased.
+ *
+ * @since x.x.x
+ *
+ * @param string $sanitized_html HTML already run through wp_kses().
+ * @return string
+ */
+function edac_restore_svg_attribute_case( string $sanitized_html ): string {
+	if ( false === stripos( $sanitized_html, '<svg' ) ) {
+		// Cheap early exit - none of these attributes mean anything outside SVG.
+		return $sanitized_html;
+	}
+
+	foreach ( edac_svg_case_sensitive_attributes() as $lowercased => $correct ) {
+		$sanitized_html = preg_replace(
+			'/(<[^>]*?[\s])' . preg_quote( $lowercased, '/' ) . '(\s*=)/i',
+			'$1' . $correct . '$2',
+			$sanitized_html
+		);
+	}
+
+	return $sanitized_html;
+}
+
+/**
+ * Sanitize an HTML/SVG snippet before it's stored as an issue's "object"
+ * (the scanned element's outerHTML) - strips constructs that could execute
+ * as script (script tags, on* event handler attributes, <foreignObject>)
+ * while leaving ordinary post-content HTML and non-scripting SVG markup
+ * (shapes, gradients, text) unmodified.
+ *
+ * @since x.x.x
+ *
+ * @param mixed $html Raw HTML snippet - expected to be a string.
+ * @return string Sanitized HTML, or '' if given anything other than a string.
+ */
+function edac_sanitize_scanned_html( $html ): string {
+	if ( ! is_string( $html ) || '' === $html ) {
+		return '';
+	}
+
+	// xlink:href isn't in core's wp_kses_uri_attributes() list (that list
+	// predates SVG/XLink awareness), so without this filter a value like
+	// xlink:href="javascript:alert(1)" would pass through unchecked even
+	// though the identical case for the plain 'href' attribute is already
+	// protocol-validated by core. Scoped to this call only.
+	$add_xlink_href = static function ( array $uri_attributes ): array {
+		$uri_attributes[] = 'xlink:href';
+		return $uri_attributes;
+	};
+
+	add_filter( 'wp_kses_uri_attributes', $add_xlink_href );
+	$sanitized = wp_kses( $html, edac_scanned_html_allowed_tags() );
+	remove_filter( 'wp_kses_uri_attributes', $add_xlink_href );
+
+	return edac_restore_svg_attribute_case( $sanitized );
+}
+
 /**
  * Remove corrected posts
  *

tests/phpunit/helper-functions/SanitizeScannedHtmlTest.php

@@ -0,0 +1,204 @@
+<?php
+/**
+ * Class SanitizeScannedHtmlTest
+ *
+ * @package Accessibility_Checker
+ */
+
+/**
+ * Test cases for edac_sanitize_scanned_html() function.
+ */
+class SanitizeScannedHtmlTest extends WP_UnitTestCase {
+
+	/**
+	 * Tests that dangerous constructs are stripped from otherwise-valid SVG markup.
+	 *
+	 * @dataProvider malicious_svg_data
+	 *
+	 * @param string $svg           The malicious SVG markup.
+	 * @param string $must_not_contain A substring that must not survive sanitization.
+	 */
+	public function test_strips_dangerous_constructs( $svg, $must_not_contain ) {
+		$sanitized = edac_sanitize_scanned_html( $svg );
+
+		$this->assertStringNotContainsStringIgnoringCase( $must_not_contain, $sanitized );
+	}
+
+	/**
+	 * Data provider of SVG markup containing common XSS vectors.
+	 */
+	public function malicious_svg_data() {
+		return [
+			'onload handler'      => [ '<svg onload="alert(document.cookie)"><circle r="5" /></svg>', 'onload' ],
+			'script child'        => [ '<svg><script>alert(1)</script></svg>', '<script' ],
+			'foreignObject'       => [ '<svg><foreignObject><img src=x onerror="alert(1)"></foreignObject></svg>', 'foreignObject' ],
+			'onclick handler'     => [ '<svg><a onclick="alert(1)"><circle r="5" /></a></svg>', 'onclick' ],
+			'javascript: xlink'   => [ '<svg><use xlink:href="javascript:alert(1)" /></svg>', 'javascript:' ],
+			'javascript: href'    => [ '<svg><use href="javascript:alert(1)" /></svg>', 'javascript:' ],
+			'onbegin animate'     => [ '<svg><animate onbegin="alert(1)" attributeName="x" /></svg>', 'onbegin' ],
+			'onmouseover handler' => [ '<svg onmouseover="alert(1)"><rect width="10" height="10" /></svg>', 'onmouseover' ],
+		];
+	}
+
+	/**
+	 * Tests the combined all-in-one payload (script + onload + onclick +
+	 * foreignObject together) has every dangerous construct removed.
+	 */
+	public function test_combined_vector_payload_is_fully_stripped() {
+		$svg       = '<svg onload="alert(1)"><script>alert(2)</script><a onclick="alert(3)"><foreignObject><body>hi</body></foreignObject></a></svg>';
+		$sanitized = edac_sanitize_scanned_html( $svg );
+
+		$this->assertStringNotContainsStringIgnoringCase( 'onload', $sanitized );
+		$this->assertStringNotContainsStringIgnoringCase( '<script', $sanitized );
+		$this->assertStringNotContainsStringIgnoringCase( 'onclick', $sanitized );
+		$this->assertStringNotContainsStringIgnoringCase( 'foreignObject', $sanitized );
+	}
+
+	/**
+	 * Tests that a realistic, benign icon-style SVG survives sanitization
+	 * with its meaningful content intact - the whole point of a wide
+	 * allow-list is not mangling ordinary safe SVGs.
+	 */
+	public function test_preserves_safe_svg_content() {
+		$svg = '<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" role="img" aria-label="Warning">'
+			. '<title>Warning</title>'
+			. '<defs><linearGradient id="g1" x1="0" y1="0" x2="1" y2="1">'
+			. '<stop offset="0%" stop-color="#fff" /><stop offset="100%" stop-color="#000" /></linearGradient>'
+			. '<clipPath id="c1"><rect x="0" y="0" width="24" height="24" /></clipPath></defs>'
+			. '<g fill="url(#g1)" stroke="#333" stroke-width="1" clip-path="url(#c1)">'
+			. '<circle cx="12" cy="12" r="10" />'
+			. '<path d="M12 6v8" />'
+			. '</g>'
+			. '<use xlink:href="#g1" />'
+			. '</svg>';
+
+		$sanitized = edac_sanitize_scanned_html( $svg );
+
+		$this->assertStringContainsString( '<svg', $sanitized );
+		$this->assertStringContainsString( 'viewBox="0 0 24 24"', $sanitized );
+		$this->assertStringContainsString( '<title>Warning</title>', $sanitized );
+		$this->assertStringContainsString( '<linearGradient', $sanitized );
+		$this->assertStringContainsString( '<stop', $sanitized );
+		$this->assertStringContainsString( 'stop-color="#fff"', $sanitized );
+		$this->assertStringContainsString( '<clipPath', $sanitized );
+		$this->assertStringContainsString( 'clip-path="url(#c1)"', $sanitized );
+		$this->assertStringContainsString( '<circle', $sanitized );
+		$this->assertStringContainsString( 'cx="12"', $sanitized );
+		$this->assertStringContainsString( '<path', $sanitized );
+		$this->assertStringContainsString( 'd="M12 6v8"', $sanitized );
+		$this->assertStringContainsString( 'xlink:href="#g1"', $sanitized );
+	}
+
+	/**
+	 * Tests that every case-sensitive SVG attribute still on the allow-list
+	 * comes back out with its correct camelCase name even though wp_kses()
+	 * lowercases attribute names internally.
+	 */
+	public function test_restores_case_sensitive_svg_attribute_names() {
+		$svg = '<svg viewBox="0 0 10 10" preserveAspectRatio="xMidYMid meet">'
+			. '<linearGradient id="g" gradientUnits="userSpaceOnUse" gradientTransform="rotate(45)" spreadMethod="pad">'
+			. '<stop offset="0" stop-color="#fff" /></linearGradient>'
+			. '<rect width="10" height="10" fill="url(#g)" />'
+			. '</svg>';
+
+		$sanitized = edac_sanitize_scanned_html( $svg );
+
+		$this->assertStringContainsString( 'viewBox=', $sanitized );
+		$this->assertStringContainsString( 'preserveAspectRatio=', $sanitized );
+		$this->assertStringContainsString( 'gradientUnits=', $sanitized );
+		$this->assertStringContainsString( 'gradientTransform=', $sanitized );
+		$this->assertStringContainsString( 'spreadMethod=', $sanitized );
+	}
+
+	/**
+	 * Tests that structural SVG elements deliberately left off the allow-list
+	 * (pattern, mask, marker, switch, textPath) are stripped as tags - kses
+	 * removes the tag itself while keeping any benign child shapes.
+	 */
+	public function test_strips_svg_elements_outside_the_allow_list() {
+		$svg = '<svg viewBox="0 0 10 10">'
+			. '<pattern id="p"><circle r="1" /></pattern>'
+			. '<mask id="m"><rect width="10" height="10" /></mask>'
+			. '<marker id="k"><path d="M0 0" /></marker>'
+			. '<switch><text x="0" y="0">Hi</text></switch>'
+			. '<text><textPath href="#p">curved</textPath></text>'
+			. '</svg>';
+
+		$sanitized = edac_sanitize_scanned_html( $svg );
+
+		$this->assertStringNotContainsString( '<pattern', $sanitized );
+		$this->assertStringNotContainsString( '<mask', $sanitized );
+		$this->assertStringNotContainsString( '<marker', $sanitized );
+		$this->assertStringNotContainsString( '<switch', $sanitized );
+		$this->assertStringNotContainsString( '<textPath', $sanitized );
+		$this->assertStringNotContainsString( '<textpath', $sanitized );
+	}
+
+	/**
+	 * Tests that a local same-document fragment reference (the common,
+	 * legitimate icon-sprite pattern) is preserved on both href and
+	 * xlink:href.
+	 */
+	public function test_preserves_local_fragment_references() {
+		$svg = '<svg><use href="#icon-check" /><use xlink:href="#icon-check" /></svg>';
+
+		$sanitized = edac_sanitize_scanned_html( $svg );
+
+		$this->assertStringContainsString( 'href="#icon-check"', $sanitized );
+		$this->assertStringContainsString( 'xlink:href="#icon-check"', $sanitized );
+	}
+
+	/**
+	 * Tests that ordinary (non-SVG) post-content HTML - the kind most
+	 * flagged elements actually are - passes through unaffected.
+	 */
+	public function test_preserves_ordinary_html_snippet() {
+		$html = '<a href="https://example.com"><strong>Click here</strong></a>';
+
+		$this->assertSame( $html, edac_sanitize_scanned_html( $html ) );
+	}
+
+	/**
+	 * Tests that a plain empty-link snippet (no SVG involved at all) is untouched.
+	 */
+	public function test_preserves_empty_link_snippet() {
+		$html = '<a href="#"><img src="icon.png" alt="Icon"></a>';
+
+		$sanitized = edac_sanitize_scanned_html( $html );
+
+		$this->assertStringContainsString( 'href="#"', $sanitized );
+		$this->assertStringContainsString( 'src="icon.png"', $sanitized );
+	}
+
+	/**
+	 * Tests that non-string input returns an empty string rather than
+	 * throwing or emitting a PHP warning.
+	 *
+	 * @dataProvider non_string_data
+	 *
+	 * @param mixed $value A non-string value.
+	 */
+	public function test_non_string_input_returns_empty_string( $value ) {
+		$this->assertSame( '', edac_sanitize_scanned_html( $value ) );
+	}
+
+	/**
+	 * Data provider of non-string values.
+	 */
+	public function non_string_data() {
+		return [
+			'null'   => [ null ],
+			'array'  => [ [ '<svg onload="alert(1)"></svg>' ] ],
+			'int'    => [ 42 ],
+			'bool'   => [ true ],
+			'object' => [ (object) [ 'markup' => '<svg></svg>' ] ],
+		];
+	}
+
+	/**
+	 * Tests that an empty string input returns an empty string.
+	 */
+	public function test_empty_string_input_returns_empty_string() {
+		$this->assertSame( '', edac_sanitize_scanned_html( '' ) );
+	}
+}