Skip to content

Commit db2c65f

Browse files
pattonwebzclaude
pattonwebz
and
claude
committed
fix: harden edac_svg_markup_to_data_uri() against non-string input, trim docblock
Address review feedback on PR #1832 (CodeRabbit + Gemini Code Assist): add a return type declaration, tighten the docblock down to the non-obvious "why", and cover a combined-vector payload plus non-string inputs in tests. The parameter itself stays untyped: a strict `string` type hint would turn a wrong-type caller into a fatal TypeError. Instead the function checks is_string() itself and always returns a valid (if payload-less) data: URI, even on bad input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 2e8827d commit db2c65f

2 files changed

Lines changed: 59 additions & 17 deletions

File tree

‎includes/helper-functions.php‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -789,22 +789,20 @@ function edac_parse_html_for_media( $html ) {
789789
}
790790

791791
/**
792-
* Convert raw SVG markup into a data: URI safe for use as an <img> src.
793-
*
794-
* The source markup comes from scanned page code stored in the issues
795-
* table, which can contain arbitrary attributes/children (e.g. <script>,
796-
* on* handlers, <foreignObject>). Encoding it into a data URI and only
797-
* ever placing that URI in an <img> src means the browser treats it as an
798-
* image resource, not as markup to parse for scripting - the same
799-
* technique already used for untrusted SVGs in the issue modal's image
800-
* finder (see src/issueModal/components/IssueImage.js).
792+
* Convert raw SVG markup into a data: URI, safe as an <img> src - browsers
793+
* don't execute scripts or event handlers in SVGs loaded as images. Returns
794+
* a bare (payload-less) data URI if given anything other than a string.
801795
*
802796
* @since x.x.x
803797
*
804-
* @param string $svg_markup Raw SVG markup.
805-
* @return string Data URI string (unescaped - callers must esc_url()/esc_attr() it before output).
798+
* @param mixed $svg_markup Raw SVG markup - expected to be a string.
799+
* @return string Unescaped data URI - callers must esc_url() it before output.
806800
*/
807-
function edac_svg_markup_to_data_uri( $svg_markup ) {
801+
function edac_svg_markup_to_data_uri( $svg_markup ): string {
802+
if ( ! is_string( $svg_markup ) ) {
803+
return 'data:image/svg+xml,';
804+
}
805+
808806
return 'data:image/svg+xml,' . rawurlencode( $svg_markup );
809807
}
810808

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

Lines changed: 49 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -51,11 +51,55 @@ public function test_strips_no_bytes_but_leaves_no_raw_markup_characters( $svg )
5151
*/
5252
public function malicious_svg_data() {
5353
return [
54-
'onload handler' => [ '<svg onload="alert(document.cookie)"></svg>' ],
55-
'script child' => [ '<svg><script>alert(1)</script></svg>' ],
56-
'foreignObject' => [ '<svg><foreignObject><img src=x onerror="alert(1)"></foreignObject></svg>' ],
57-
'javascript: xlink' => [ '<svg><a xlink:href="javascript:alert(1)"><text>click</text></a></svg>' ],
58-
'animate onbegin' => [ '<svg><animate onbegin="alert(1)" attributeName="x" /></svg>' ],
54+
'onload handler' => [ '<svg onload="alert(document.cookie)"></svg>' ],
55+
'script child' => [ '<svg><script>alert(1)</script></svg>' ],
56+
'foreignObject' => [ '<svg><foreignObject><img src=x onerror="alert(1)"></foreignObject></svg>' ],
57+
'javascript: xlink' => [ '<svg><a xlink:href="javascript:alert(1)"><text>click</text></a></svg>' ],
58+
'animate onbegin' => [ '<svg><animate onbegin="alert(1)" attributeName="x" /></svg>' ],
59+
'combined all-in-one' => [
60+
'<svg onload="alert(1)"><script>alert(2)</script><a onclick="alert(3)"><foreignObject><body>hi</body></foreignObject></a></svg>',
61+
],
62+
];
63+
}
64+
65+
/**
66+
* Tests that the combined-vector SVG's dangerous constructs survive
67+
* encoding intact (this function encodes, it does not sanitize - the
68+
* caller's <img src> context is what neutralizes them, not this string).
69+
*/
70+
public function test_combined_vector_payload_is_preserved_not_stripped() {
71+
$svg = '<svg onload="alert(1)"><script>alert(2)</script><a onclick="alert(3)"><foreignObject><body>hi</body></foreignObject></a></svg>';
72+
$decoded = rawurldecode( substr( edac_svg_markup_to_data_uri( $svg ), strlen( 'data:image/svg+xml,' ) ) );
73+
74+
$this->assertSame( $svg, $decoded );
75+
$this->assertStringContainsString( 'onload="alert(1)"', $decoded );
76+
$this->assertStringContainsString( '<script>alert(2)</script>', $decoded );
77+
$this->assertStringContainsString( 'onclick="alert(3)"', $decoded );
78+
$this->assertStringContainsString( '<foreignObject>', $decoded );
79+
}
80+
81+
/**
82+
* Tests that non-string input never fatals and always yields a bare,
83+
* payload-less data URI rather than attempting to encode it.
84+
*
85+
* @dataProvider non_string_data
86+
*
87+
* @param mixed $value A non-string value.
88+
*/
89+
public function test_non_string_input_returns_bare_data_uri( $value ) {
90+
$this->assertSame( 'data:image/svg+xml,', edac_svg_markup_to_data_uri( $value ) );
91+
}
92+
93+
/**
94+
* Data provider of non-string values.
95+
*/
96+
public function non_string_data() {
97+
return [
98+
'null' => [ null ],
99+
'array' => [ [ '<svg onload="alert(1)"></svg>' ] ],
100+
'int' => [ 42 ],
101+
'bool' => [ true ],
102+
'object' => [ (object) [ 'markup' => '<svg></svg>' ] ],
59103
];
60104
}
61105
}

0 commit comments

Comments
 (0)