Repository navigation
Conversation
github-project-automation
Bot
moved this to 🔎 Needs Review
in WordPress 7.0 Editor Tasks
t-hamano
added
[Type] Experimental
e537f45 to
7bdd4c1
Compare
| '<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24" class="icon" aria-hidden="true"><path d="M0 0" fill="currentColor" /></svg>', | ||
| '<svg xmlns="http://www.w3.org/2000/svg" viewbox="0 0 24 24" width="24" height="24" class="icon" aria-hidden="true"><path d="M0 0" fill="currentColor" /></svg>', |
There was a problem hiding this comment.
As far as I've tested, lowercase viewbox is automatically recognized as viewBox by major browsers, so as long as we're using inline SVG as HTML content, we shouldn't have any problems.
<!DOCTYPE html>
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Document</title>
</head>
<body>
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100" width="100">
<rect x="0" y="0" width="100%" height="100%" />
<circle cx="50%" cy="50%" r="4" fill="white" />
</svg>
<svg xmlns="http://www.w3.org/2000/svg" viewbox="0 0 100 100" width="100">
<rect x="0" y="0" width="100%" height="100%" />
<circle cx="50%" cy="50%" r="4" fill="white" />
</svg>
</body>
</html>
There was a problem hiding this comment.
yes, when the SVG is embedded within HTML, those attributes are identical across case variations. the HTML parser will convert a subset of attributes to their normalized camelCase form, but when parsing they are identical.
one of the security concerns with SVG revolves around subtleties like this, because embedded and external SVG parse differently, which can be a problem when plugins inline external files.
t-hamano
changed the title
mcsf
changed the title
|
@t-hamano this seems like a good improvement since it retains the structure of the existing sanitization while adding extra attributes. did you verify that none of the newly-allowed attributes allows for script execution? one note here is that SVG is complicated to parse, and even the search for $svg = '';
$processor = WP_HTML_Processor::create_fragment( $icon_content );
if ( ! $processor->next_token() || 'SVG' !== $processor->get_tag() ) {
return '';
}
$svg .= $processor->serialize_token();
$svg_depth = $processor->get_current_depth();
while ( $processor->next_token() && $processor->get_current_depth() > $svg_depth ) {
$svg .= $processor->serialize_token();
}
// Parsing has stopped even though there might be additional content.
$filtered = wp_kses( ... )This could also be done after filtering with Unfortunately there could be issues with self-closing tags, which exist within the SVG content but otherwise not in the HTML. In HTML, sticking with for gradual improvement I think this is good as long as we have a thorough review of the attributes and tags we’re allowing. |
| * @return string The sanitized icon SVG content. | ||
| */ | ||
| private function sanitize_icon_content( $icon_content ) { | ||
| // Core attributes applicable to most elements. |
There was a problem hiding this comment.
This appears to be using the elements and attributes from my earlier comment, can you confirm?
Would be nice to validate those, as I haven't gone in-depth in that, and there are plenty of elements and attributes that SVGs can support. Also we know that any KSES-related changes shouldn't be taken lightly 😉
|
Sorry for the late reply. I'm working with @mcsf to build the infrastructure to allow the Icon API to be extended on the Gutenberg plugin. If that works out, I'll come back to this PR. |
t-hamano
added
[Type] Enhancement
t-hamano
added
[Feature] Icons
t-hamano
moved this from 🔎 Needs Review
to 🦵 Punted to 7.1
in WordPress 7.0 Editor Tasks
7bdd4c1 to
26a51db
Compare
The HTML parser honors XML self-closing syntax in foreign content, so `<svg />` has no descendants and emits no closing tag. The descendant scan stopped at a depth lower than the root, which for such a root is only reached after consuming the following sibling: `<svg /><svg>…</svg>` was returned as both roots concatenated plus an unbalanced `</svg>`, and the multiple-root check that follows never saw the sibling because the cursor had already passed it. Skip the scan and the closing tag unless the root `expects_closer()`. Leaving the cursor on the root is what lets the multiple-root check find the sibling — narrowing the depth comparison instead would stop the loop on the sibling token, which `next_tag()` would then skip past. Co-Authored-By: Claude <noreply@anthropic.com>
|
Punting this to 7.2 as the 7.1 RC1 release is approaching. |
t-hamano
moved this from 🔎 Needs Review
to 🦵 Punted to 7.2
in WordPress 7.1 Editor Tasks
|
Update: Let's consider addressing this in the 7.1 minor release if possible. |
t-hamano
moved this from 🦵 Punted to 7.2
to 🐛 Punted to 7.1.1
in WordPress 7.1 Editor Tasks
github-project-automation
Bot
moved this to 🔎 Needs Review
in WordPress 7.1.x Editor Tasks
github-project-automation
Bot
moved this to 🔎 Needs Review
in WordPress 7.2 Editor Tasks
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
🤖 PR meta 🤖🎉 PropsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. Updated as activity occurs, without notifying anyone named here. Add the ⚡ PerformanceShow the resultsClient side metrics exclude the server response time. front-end-block-theme
front-end-classic-theme
media-processing
media-upload
post-editor
site-editor
|
| * @param non-empty-string ...$attribute_names Attribute names to allow. | ||
| * @return array<non-empty-string, true> Attribute names mapped to true. |
There was a problem hiding this comment.
| * @param non-empty-string ...$attribute_names Attribute names to allow. | |
| * @return array<non-empty-string, true> Attribute names mapped to true. | |
| * @param non-falsy-string ...$attribute_names Attribute names to allow. | |
| * @return array<non-falsy-string, true> Attribute names mapped to true. |
There was a problem hiding this comment.
is this saying that code calling this function should perform this chec Before calling it?
return array_any( $attribute_names, fn ( $n ) => is_string( $n ) && '' !== $n && '0' !== $n )
? get_allowed_attribute_list( ...$attribute_names )
: array();that will be faster generally, but also exports some of the internal behavior from the function onto the callers, which can be valuable, but is often quite messy and error-prone.
maybe I’m misunderstanding the type annotations?
should the function crash if someone sends '' or '0' as any attribute name?
| * @param string $html_containing_svg HTML fragment containing the SVG to sanitize. | ||
| * @return string The sanitized SVG, or an empty string when no valid SVG is found. | ||
| */ | ||
| private function sanitize_inline_svg( $html_containing_svg ) { |
There was a problem hiding this comment.
| private function sanitize_inline_svg( $html_containing_svg ) { | |
| private function sanitize_inline_svg( string $html_containing_svg ): string { |
| // An external `.svg` file is XML, but sanitize_inline_svg() expects an | ||
| // inline HTML fragment. A dedicated XML sanitizer should handle this | ||
| // in the future. | ||
| $content = $this->sanitize_inline_svg( file_get_contents( $icon_path ) ); |
There was a problem hiding this comment.
To guard against failures in loading the file:
| $content = $this->sanitize_inline_svg( file_get_contents( $icon_path ) ); | |
| $content = file_get_contents( $icon_path ); | |
| if ( is_string( $content ) ) { | |
| $content = $this->sanitize_inline_svg( $content ); | |
| } else { | |
| $content = false; | |
| } |
| * @return array<non-empty-string, array{ input: string, expected: string }> | ||
| */ | ||
| public function data_sanitize_inline_svg() { |
There was a problem hiding this comment.
| * @return array<non-empty-string, array{ input: string, expected: string }> | |
| */ | |
| public function data_sanitize_inline_svg() { | |
| * @return array<non-falsy-string, array{ input: non-falsy-string, expected: non-falsy-string }> | |
| */ | |
| public function data_sanitize_inline_svg(): array { |
There was a problem hiding this comment.
Fixed in f278e66. I kept string rather than non-falsy-string for input and expected, because some data sets use an empty string: 'returns empty for empty string' has '' as its input, and every rejection case (e.g. 'rejects multiple top-level svg elements') has '' as its expected output.
| * @param string $input The icon content to sanitize. | ||
| * @param string $expected The expected sanitized output. | ||
| */ | ||
| public function test_sanitize_inline_svg( $input, $expected ) { |
There was a problem hiding this comment.
| * @param string $input The icon content to sanitize. | |
| * @param string $expected The expected sanitized output. | |
| */ | |
| public function test_sanitize_inline_svg( $input, $expected ) { | |
| * @param non-falsy-string $input The icon content to sanitize. | |
| * @param non-falsy-string $expected The expected sanitized output. | |
| */ | |
| public function test_sanitize_inline_svg( string $input, string $expected ) { |
There was a problem hiding this comment.
Fixed in fdc9db8. For the same reason as in the data provider, I kept string in the @param tags: both $input and $expected can be an empty string.
# Conflicts: # phpunit/class-wp-icons-registry-gutenberg-test.php
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…provider Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What?
In the experimental icon registry, any string can be registered as SVG content. Unfortunately, there is still no core function to properly sanitize SVGs, so we've enhanced the private methods for sanitization and defined as many SVG-friendly tags and attributes as possible.
Testing Instructions
Please refer to the unit test to see what the function expects.