docs/tests: address second-round review feedback on the sanitizer

pattonwebz · claude · pattonwebz · commit 299796398307 · 2026-07-10T15:39:15.000+01:00
- Correct the allow-list docblock: &lt;a&gt; is not excluded - wp_kses() has
  no namespace awareness, so the 'post' base list's entry also matches
  &lt;a&gt; inside &lt;svg&gt;. 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 &lt;foreignobject&gt; regression could not slip past them.

Co-Authored-By: Claude Fable 5 &lt;noreply@anthropic.com&gt;
diff --git a/includes/helper-functions.php b/includes/helper-functions.php
@@ -793,12 +793,13 @@ function edac_parse_html_for_media( $html ) {
  * 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>, <a> (SVG's own href-based link element -
- * ordinary post-content links are still allowed via the 'post' base list),
- * SMIL animation, filter primitives, and rarely-used structural extras
- * (<pattern>, <mask>, <marker>, <switch>, <textPath>) - none of which this
- * plugin's real-world content needs. Never lists any on* attribute for
- * anything.
+ * <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
@@ -865,8 +866,9 @@ function edac_scanned_html_allowed_tags(): array {
  * 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. Must stay in
- * sync with the camelCase attributes in edac_scanned_html_allowed_tags().
+ * 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
  *
diff --git a/tests/phpunit/helper-functions/SanitizeScannedHtmlTest.php b/tests/phpunit/helper-functions/SanitizeScannedHtmlTest.php
@@ -21,7 +21,7 @@ class SanitizeScannedHtmlTest extends WP_UnitTestCase {
 	public function test_strips_dangerous_constructs( $svg, $must_not_contain ) {
 		$sanitized = edac_sanitize_scanned_html( $svg );
 
-		$this->assertStringNotContainsString( $must_not_contain, $sanitized );
+		$this->assertStringNotContainsStringIgnoringCase( $must_not_contain, $sanitized );
 	}
 
 	/**
@@ -48,10 +48,10 @@ 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->assertStringNotContainsString( 'onload', $sanitized );
-		$this->assertStringNotContainsString( '<script', $sanitized );
-		$this->assertStringNotContainsString( 'onclick', $sanitized );
-		$this->assertStringNotContainsString( 'foreignObject', $sanitized );
+		$this->assertStringNotContainsStringIgnoringCase( 'onload', $sanitized );
+		$this->assertStringNotContainsStringIgnoringCase( '<script', $sanitized );
+		$this->assertStringNotContainsStringIgnoringCase( 'onclick', $sanitized );
+		$this->assertStringNotContainsStringIgnoringCase( 'foreignObject', $sanitized );
 	}
 
 	/**
