Skip to content

Commit dccfcca

Browse files
pattonwebzclaude
pattonwebz
and
claude
committed
refactor: rebuild the SVG sanitizer allow-list as one shared attribute 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>
1 parent 2412052 commit dccfcca

2 files changed

Lines changed: 109 additions & 215 deletions

File tree

‎includes/helper-functions.php‎

Lines changed: 59 additions & 213 deletions
Original file line numberDiff line numberDiff line change
@@ -808,225 +808,83 @@ function edac_svg_markup_to_data_uri( $svg_markup ): string {
808808

809809
/**
810810
* Allowed tags/attributes for edac_sanitize_scanned_html() - wp_kses_allowed_html( 'post' )
811-
* plus the non-scripting SVG vocabulary actually likely to appear in a
812-
* flagged icon/logo/decorative graphic: structure, shapes, text, and basic
813-
* gradients/patterns/masks. Deliberately excludes <script>, <foreignObject>,
814-
* <image>, <a> (SVG's own href-based link element - ordinary post-content
815-
* links are still allowed via the 'post' base list), SMIL animation
816-
* elements, and filter primitives - none of which this plugin's real-world
817-
* content needs, and never lists any on* attribute for anything.
811+
* plus the minimal SVG vocabulary a flagged icon/logo/decorative graphic
812+
* actually uses: container, grouping, basic shapes, gradients, text, and the
813+
* accessible-name elements (<title>/<desc>). Deliberately excludes <script>,
814+
* <foreignObject>, <image>, <a> (SVG's own href-based link element -
815+
* ordinary post-content links are still allowed via the 'post' base list),
816+
* SMIL animation, filter primitives, and rarely-used structural extras
817+
* (<pattern>, <mask>, <marker>, <switch>, <textPath>) - none of which this
818+
* plugin's real-world content needs. Never lists any on* attribute for
819+
* anything.
820+
*
821+
* Every allowed SVG element shares one attribute set: wp_kses() only needs
822+
* attribute names allow-listed, not semantically scoped per tag, and a
823+
* geometry/paint attribute on a tag that ignores it is harmless. The only
824+
* exception is href/xlink:href, which stays scoped to <use> (the icon-sprite
825+
* pattern) and is protocol-validated - see edac_sanitize_scanned_html().
818826
*
819827
* @since x.x.x
820828
*
821829
* @return array<string, array<string, bool>>
822830
*/
823831
function edac_scanned_html_allowed_tags(): array {
824-
$allowed = wp_kses_allowed_html( 'post' );
825-
826-
// Shared by (almost) every SVG element - core presentation attributes
827-
// plus the accessibility attributes this plugin most cares about.
828-
$common = [
829-
'id' => true,
830-
'class' => true,
831-
'style' => true,
832-
'transform' => true,
833-
'role' => true,
834-
'aria-hidden' => true,
835-
'aria-label' => true,
836-
'aria-labelledby' => true,
837-
'aria-describedby' => true,
838-
'focusable' => true,
839-
'tabindex' => true,
840-
'lang' => true,
841-
'xml:lang' => true,
842-
'xml:space' => true,
843-
];
844-
845-
// Paint/stroke/fill presentation attributes - valid on most shape and
846-
// container elements per the SVG spec; wp_kses doesn't need them to be
847-
// semantically scoped per tag, only explicitly allow-listed.
848-
$paint = [
849-
'fill' => true,
850-
'fill-rule' => true,
851-
'fill-opacity' => true,
852-
'stroke' => true,
853-
'stroke-width' => true,
854-
'stroke-linecap' => true,
855-
'stroke-linejoin' => true,
856-
'stroke-dasharray' => true,
857-
'stroke-dashoffset' => true,
858-
'stroke-opacity' => true,
859-
'stroke-miterlimit' => true,
860-
'opacity' => true,
861-
'color' => true,
862-
'clip-path' => true,
863-
'clip-rule' => true,
864-
'mask' => true,
865-
'filter' => true,
866-
'marker-start' => true,
867-
'marker-mid' => true,
868-
'marker-end' => true,
869-
'vector-effect' => true,
870-
'shape-rendering' => true,
871-
];
872-
873-
$common_paint = $common + $paint;
832+
$identity = [ 'id', 'class', 'style', 'transform', 'role', 'focusable' ];
833+
$aria = [ 'aria-hidden', 'aria-label', 'aria-labelledby', 'aria-describedby' ];
834+
$root = [ 'xmlns', 'xmlns:xlink', 'version', 'viewbox', 'preserveaspectratio' ];
835+
$geometry = [ 'x', 'y', 'width', 'height', 'cx', 'cy', 'r', 'rx', 'ry', 'x1', 'y1', 'x2', 'y2', 'fx', 'fy', 'd', 'points', 'dx', 'dy' ];
836+
$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' ];
837+
$gradient = [ 'gradientunits', 'gradienttransform', 'spreadmethod', 'offset', 'stop-color', 'stop-opacity' ];
838+
$text = [ 'text-anchor', 'font-family', 'font-size', 'font-weight', 'font-style' ];
839+
840+
$svg_attributes = array_fill_keys(
841+
array_merge( $identity, $aria, $root, $geometry, $paint, $gradient, $text ),
842+
true
843+
);
874844

875-
// href/xlink:href only ever added to elements whose sole use is a local
876-
// same-document fragment reference (#id) - edac_sanitize_scanned_html()
877-
// registers xlink:href with wp_kses_uri_attributes() for the duration
878-
// of the call, so both get the same bad-protocol/URI validation core
879-
// already applies to the plain 'href' attribute.
880-
$href = [
881-
'href' => true,
882-
'xlink:href' => true,
845+
$svg_elements = [
846+
'svg',
847+
'g',
848+
'defs',
849+
'symbol',
850+
'use',
851+
'path',
852+
'rect',
853+
'circle',
854+
'ellipse',
855+
'line',
856+
'polyline',
857+
'polygon',
858+
'lineargradient',
859+
'radialgradient',
860+
'stop',
861+
'clippath',
862+
'text',
863+
'tspan',
864+
'title',
865+
'desc',
883866
];
884867

885-
$svg_tags = [
886-
'svg' => $common + [
887-
'xmlns' => true,
888-
'xmlns:xlink' => true,
889-
'version' => true,
890-
'viewbox' => true,
891-
'width' => true,
892-
'height' => true,
893-
'preserveaspectratio' => true,
894-
],
895-
'g' => $common_paint,
896-
'defs' => $common,
897-
'symbol' => $common + [
898-
'viewbox' => true,
899-
'preserveaspectratio' => true,
900-
],
901-
'switch' => $common,
902-
'use' => $common_paint + $href + [
903-
'x' => true,
904-
'y' => true,
905-
'width' => true,
906-
'height' => true,
907-
],
908-
'path' => $common_paint + [ 'd' => true ],
909-
'rect' => $common_paint + [
910-
'x' => true,
911-
'y' => true,
912-
'width' => true,
913-
'height' => true,
914-
'rx' => true,
915-
'ry' => true,
916-
],
917-
'circle' => $common_paint + [
918-
'cx' => true,
919-
'cy' => true,
920-
'r' => true,
921-
],
922-
'ellipse' => $common_paint + [
923-
'cx' => true,
924-
'cy' => true,
925-
'rx' => true,
926-
'ry' => true,
927-
],
928-
'line' => $common_paint + [
929-
'x1' => true,
930-
'y1' => true,
931-
'x2' => true,
932-
'y2' => true,
933-
],
934-
'polyline' => $common_paint + [ 'points' => true ],
935-
'polygon' => $common_paint + [ 'points' => true ],
936-
'text' => $common_paint + [
937-
'x' => true,
938-
'y' => true,
939-
'dx' => true,
940-
'dy' => true,
941-
'text-anchor' => true,
942-
'font-family' => true,
943-
'font-size' => true,
944-
'font-weight' => true,
945-
'font-style' => true,
946-
],
947-
'tspan' => $common_paint + [
948-
'x' => true,
949-
'y' => true,
950-
'dx' => true,
951-
'dy' => true,
952-
],
953-
'textpath' => $common_paint + $href + [
954-
'startoffset' => true,
955-
'method' => true,
956-
'spacing' => true,
957-
'side' => true,
958-
],
959-
'lineargradient' => $common + $href + [
960-
'x1' => true,
961-
'y1' => true,
962-
'x2' => true,
963-
'y2' => true,
964-
'gradientunits' => true,
965-
'gradienttransform' => true,
966-
'spreadmethod' => true,
967-
],
968-
'radialgradient' => $common + $href + [
969-
'cx' => true,
970-
'cy' => true,
971-
'r' => true,
972-
'fx' => true,
973-
'fy' => true,
974-
'fr' => true,
975-
'gradientunits' => true,
976-
'gradienttransform' => true,
977-
'spreadmethod' => true,
978-
],
979-
'stop' => $common + [
980-
'offset' => true,
981-
'stop-color' => true,
982-
'stop-opacity' => true,
983-
],
984-
'pattern' => $common + $href + [
985-
'x' => true,
986-
'y' => true,
987-
'width' => true,
988-
'height' => true,
989-
'patternunits' => true,
990-
'patterncontentunits' => true,
991-
'patterntransform' => true,
992-
'viewbox' => true,
993-
],
994-
'clippath' => $common + [ 'clippathunits' => true ],
995-
'mask' => $common + [
996-
'x' => true,
997-
'y' => true,
998-
'width' => true,
999-
'height' => true,
1000-
'maskunits' => true,
1001-
'maskcontentunits' => true,
1002-
],
1003-
'marker' => $common + [
1004-
'markerwidth' => true,
1005-
'markerheight' => true,
1006-
'markerunits' => true,
1007-
'refx' => true,
1008-
'refy' => true,
1009-
'orient' => true,
1010-
'viewbox' => true,
1011-
'preserveaspectratio' => true,
1012-
],
1013-
'title' => $common,
1014-
'desc' => $common,
1015-
];
868+
$svg = array_fill_keys( $svg_elements, $svg_attributes );
1016869

1017-
foreach ( $svg_tags as $tag => $attrs ) {
1018-
$allowed[ $tag ] = $attrs;
1019-
}
870+
// Local same-document fragment reference (#id) for the icon-sprite
871+
// pattern. edac_sanitize_scanned_html() registers xlink:href with
872+
// wp_kses_uri_attributes() for the duration of the call, so both get
873+
// the same bad-protocol/URI validation core already applies to the
874+
// plain 'href' attribute.
875+
$svg['use']['href'] = true;
876+
$svg['use']['xlink:href'] = true;
1020877

1021-
return $allowed;
878+
return array_merge( wp_kses_allowed_html( 'post' ), $svg );
1022879
}
1023880

1024881
/**
1025882
* SVG attribute names are case-sensitive per spec (e.g. viewBox,
1026883
* gradientTransform), but wp_kses() - built for case-insensitive HTML -
1027884
* lowercases every attribute name it outputs. Left alone, that silently
1028885
* breaks otherwise-safe SVGs (a lowercased viewbox is simply ignored by
1029-
* browsers). Keyed by the lowercased name wp_kses() produces.
886+
* browsers). Keyed by the lowercased name wp_kses() produces. Must stay in
887+
* sync with the camelCase attributes in edac_scanned_html_allowed_tags().
1030888
*
1031889
* @since x.x.x
1032890
*
@@ -1039,18 +897,6 @@ function edac_svg_case_sensitive_attributes(): array {
1039897
'gradientunits' => 'gradientUnits',
1040898
'gradienttransform' => 'gradientTransform',
1041899
'spreadmethod' => 'spreadMethod',
1042-
'patternunits' => 'patternUnits',
1043-
'patterncontentunits' => 'patternContentUnits',
1044-
'patterntransform' => 'patternTransform',
1045-
'clippathunits' => 'clipPathUnits',
1046-
'maskunits' => 'maskUnits',
1047-
'maskcontentunits' => 'maskContentUnits',
1048-
'markerwidth' => 'markerWidth',
1049-
'markerheight' => 'markerHeight',
1050-
'markerunits' => 'markerUnits',
1051-
'refx' => 'refX',
1052-
'refy' => 'refY',
1053-
'startoffset' => 'startOffset',
1054900
];
1055901
}
1056902

‎tests/phpunit/helper-functions/SanitizeScannedHtmlTest.php‎

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,9 @@ public function test_preserves_safe_svg_content() {
6363
$svg = '<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" role="img" aria-label="Warning">'
6464
. '<title>Warning</title>'
6565
. '<defs><linearGradient id="g1" x1="0" y1="0" x2="1" y2="1">'
66-
. '<stop offset="0%" stop-color="#fff" /><stop offset="100%" stop-color="#000" /></linearGradient></defs>'
67-
. '<g fill="url(#g1)" stroke="#333" stroke-width="1">'
66+
. '<stop offset="0%" stop-color="#fff" /><stop offset="100%" stop-color="#000" /></linearGradient>'
67+
. '<clipPath id="c1"><rect x="0" y="0" width="24" height="24" /></clipPath></defs>'
68+
. '<g fill="url(#g1)" stroke="#333" stroke-width="1" clip-path="url(#c1)">'
6869
. '<circle cx="12" cy="12" r="10" />'
6970
. '<path d="M12 6v8" />'
7071
. '</g>'
@@ -79,13 +80,60 @@ public function test_preserves_safe_svg_content() {
7980
$this->assertStringContainsString( '<linearGradient', $sanitized );
8081
$this->assertStringContainsString( '<stop', $sanitized );
8182
$this->assertStringContainsString( 'stop-color="#fff"', $sanitized );
83+
$this->assertStringContainsString( '<clipPath', $sanitized );
84+
$this->assertStringContainsString( 'clip-path="url(#c1)"', $sanitized );
8285
$this->assertStringContainsString( '<circle', $sanitized );
8386
$this->assertStringContainsString( 'cx="12"', $sanitized );
8487
$this->assertStringContainsString( '<path', $sanitized );
8588
$this->assertStringContainsString( 'd="M12 6v8"', $sanitized );
8689
$this->assertStringContainsString( 'xlink:href="#g1"', $sanitized );
8790
}
8891

92+
/**
93+
* Tests that every case-sensitive SVG attribute still on the allow-list
94+
* comes back out with its correct camelCase name even though wp_kses()
95+
* lowercases attribute names internally.
96+
*/
97+
public function test_restores_case_sensitive_svg_attribute_names() {
98+
$svg = '<svg viewBox="0 0 10 10" preserveAspectRatio="xMidYMid meet">'
99+
. '<linearGradient id="g" gradientUnits="userSpaceOnUse" gradientTransform="rotate(45)" spreadMethod="pad">'
100+
. '<stop offset="0" stop-color="#fff" /></linearGradient>'
101+
. '<rect width="10" height="10" fill="url(#g)" />'
102+
. '</svg>';
103+
104+
$sanitized = edac_sanitize_scanned_html( $svg );
105+
106+
$this->assertStringContainsString( 'viewBox=', $sanitized );
107+
$this->assertStringContainsString( 'preserveAspectRatio=', $sanitized );
108+
$this->assertStringContainsString( 'gradientUnits=', $sanitized );
109+
$this->assertStringContainsString( 'gradientTransform=', $sanitized );
110+
$this->assertStringContainsString( 'spreadMethod=', $sanitized );
111+
}
112+
113+
/**
114+
* Tests that structural SVG elements deliberately left off the allow-list
115+
* (pattern, mask, marker, switch, textPath) are stripped as tags - kses
116+
* removes the tag itself while keeping any benign child shapes.
117+
*/
118+
public function test_strips_svg_elements_outside_the_allow_list() {
119+
$svg = '<svg viewBox="0 0 10 10">'
120+
. '<pattern id="p"><circle r="1" /></pattern>'
121+
. '<mask id="m"><rect width="10" height="10" /></mask>'
122+
. '<marker id="k"><path d="M0 0" /></marker>'
123+
. '<switch><text x="0" y="0">Hi</text></switch>'
124+
. '<text><textPath href="#p">curved</textPath></text>'
125+
. '</svg>';
126+
127+
$sanitized = edac_sanitize_scanned_html( $svg );
128+
129+
$this->assertStringNotContainsString( '<pattern', $sanitized );
130+
$this->assertStringNotContainsString( '<mask', $sanitized );
131+
$this->assertStringNotContainsString( '<marker', $sanitized );
132+
$this->assertStringNotContainsString( '<switch', $sanitized );
133+
$this->assertStringNotContainsString( '<textPath', $sanitized );
134+
$this->assertStringNotContainsString( '<textpath', $sanitized );
135+
}
136+
89137
/**
90138
* Tests that a local same-document fragment reference (the common,
91139
* legitimate icon-sprite pattern) is preserved on both href and

0 commit comments

Comments
 (0)