Repository navigation
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
c784058 to
fdc5e96
Compare
bd15b3e to
38470b2
Compare
07d03c9 to
038c1c9
Compare
|
I'm supportive of this direction overall. I'll leave some feedback on different points. I started this review at 0b1535e but it moved while I was reviewing, so some of this review may be stale by the time I post. General things to debug: echo wp_kses( '<template>foo</template>', [ 'template' => true ] );
// Prints: </template>echo wp_kses( '<div style>x</div>', [ 'div' => ['style' => true] ] );
// Prints: <div style="1">x</div>echo wp_kses( '<div style="color:red">x</div>', [ 'div' => [ 'style' => true ] ] );
// Before (trunk): <div>x</div>
// After : <div style="color:red">x</div>This is an interesting one, the block delimiter is preserved (improvement) but for some reason echo wp_kses( '<!-- wp:image {"alt":"<script>x</script>"} -->', [] );
// Before: <!-- wp:image {"alt":"x"} -->
// After: <!-- wp:core/image {"alt":""} -->Incomplete tokens are interesting. KSES would leave something like a trailing That means that partial tokens like the unclosed echo wp_kses( '<p>keep me</p><b this is lost', ['p'=>[],'b'=>[]] ) . "\n";
echo wp_kses( '<p>keep me</p><title>this is lost', ['p'=>[],'title'=>[]] ) . "\n";
echo wp_kses( '<p>keep me</p><style>*{content:"this is lost";}', ['p'=>[],'title'=>[]] ) . "\n";
echo wp_kses( '<p>keep me</p><style>*{content:"this is lost";}', ['p'=>[],'style'=>[]] ) . "\n";
echo wp_kses( '<p>keep me', ['p'=>[]] ) . "\n";Before/after diff of output: diff --git 1/tmp/before.txt 2/tmp/after.txt
index 5122acd7a7..d87549cc15 100644
--- 1/tmp/before.txt
+++ 2/tmp/after.txt
@@ -1,5 +1,5 @@
-<p>keep me</p><b this is lost
+<p>keep me</p>
-<p>keep me</p><title>this is lost
+<p>keep me</p>
-<p>keep me</p>*{content:"this is lost";}
+<p>keep me</p>
-<p>keep me</p><style>*{content:"this is lost";}
+<p>keep me</p>
<p>keep meThis is worth considering carefully. The HTML snippets KSES sees don't necessarily align with coherent boundaries. In particular We could align with the previous behavior, assume that the incomplete tokens are HTML text, and escape them as such. There may be negative implications to that, it's worth some careful consideration. Foreign content is difficult, is this a case of integration points bailing because they're risky? echo wp_kses( '<math><mtext>x</mtext></math>', ['math'=>[],'mtext'=>[]] );
// (no output)
echo wp_kses( '</br>', [ 'br' => true ] );
// Warning: foreach() argument must be of type array|object, true given in /var/www/html/wp-includes/kses.php on line 1499
// Warning: foreach() argument must be of type array|object, null given in /var/www/html/wp-includes/kses.php on line 1536
// Prints: <br>First warning is the allowed tags. I used This change produces a warning where before it did not: wordpress-develop/src/wp-includes/kses.php Line 1499 in 0b1535e Maybe wordpress-develop/src/wp-includes/kses.php Lines 1533 to 1537 in 049287a Either way, it's filterable and important to to follow whatever KSES did previously. I think if the value is Second warning is here: wordpress-develop/src/wp-includes/kses.php Line 1536 in 0b1535e As it is now, this needs a |
|
@sirreal this is great. thanks for the awesome review and findings.
|
I was confused here because I expected it to happen across core blocks, but I didn't observe the same behavior with the paragraph block. I may not have been testing appropriately.
I called this out just to make sure we know what's different and understand the implications. Was this passed through
I mostly agree that removing it is the right thing to do. I think it would be appropriate to escape the incomplete token as HTML in sometimes and drop it others, but that would require a defining where the incomplete token parsing and a set of rules for how to deal with each case. Defining those rules will require some arbitrary decisions that are likely to be surprising as often as not. One important change that's worth worth documenting clearly is that some elements must be properly closed, while other elements do not need to be closed. This is especially important since we've seen examples where the KSES input starts with end tags. That implies that other KSES input may ends with unclosed tags. Again, I think on the whole these are probably good to remove, but may still be surprising. An example that lists the "atomic elements" defined by the tag processor and a few examples should help with this, e.g. |
sirreal
left a comment
sirreal
left a comment
There was a problem hiding this comment.
I have a few more findings to consider.
A major concern is that MathML is extremely restricted, most MathML markup I've tested is stripped because it quickly encounters integration points. I think this needs a more permissive solution because as-is it disallows a lot of potentially valid and conforming content that would previously pass.
<template> is always removed, is that correct? The Interactivity API often relies on template elements. It may be unusual for those to come from untrusted sources, but I'm concerned about blocking it entirely.
HTML character references are normalized. Often this is fine, but it can produce shortcode syntax where HTML character references avoided it. For example, in the following snippet plain HTML text becomes a potential shortcode: echo wp_kses( '[PR 13271]', [] );
-[PR 13271]
+[PR 13271]wp_remove_unwanted_c0_controls() seems to run on text nodes but not the contents of atomic elements. Those are mostly the same category so it's worth considering whether it should be applied there or not.
There were clearly some bugs with the way <object> was either removed or not in KSES. I think this PR implements the intention in a less buggy way. One thing I'd consider is to completely remove known void HTML elements that don't satisfy the required attributes. That seems to align with the intended behavior.
demo
$u = wp_parse_url( wp_upload_dir( null, false )['url'] );
$host = $u['host'];
$port = $u['port'] ? ":{$u['port']}" : '';
$pdf_file = "https://{$host}{$port}/ex.pdf";
echo wp_kses( '<object type="application/pdf" data="'.$pdf_file.'">after', 'post' ) . "\n";
echo wp_kses( '<object type="application/pdf">after', 'post' ) . "\n";
echo wp_kses( '<object type="application/pdf" />after', 'post' ) . "\n";
$example_tag_allow = ['example-tag'=>['r'=>['required'=>true,'values'=>['yes']]]];
echo wp_kses( '<example-tag r="yes">after', $example_tag_allow ) . "\n";
echo wp_kses( '<example-tag r="yes" />after', $example_tag_allow ) . "\n";
echo wp_kses( '<example-tag r="no">after', $example_tag_allow ) . "\n";
echo wp_kses( '<example-tag r="no" />after', $example_tag_allow ) . "\n"; <object data="https://localhost:8888/ex.pdf" type="application/pdf">after
+<object>after
-<object>after
+<object>after
-after
+<object>after
-<example-tag r="yes">after
+<example-tag r="yes">after
-<example-tag r="yes" />after
+<example-tag r="yes">after
-<example-tag>after
+<example-tag>after
-after
+<example-tag>afterThis should inject newlines into some HTML elements similar to r61747 and r61754. Possible test:
public function test_pre_leading_newline_is_preserved_and_idempotent(): void {
$input = "<pre>\n\ncode</pre>";
$once = wp_kses( $input, 'post' );
$twice = wp_kses( $once, 'post' );
$this->assertSame( $once, $twice, 'wp_kses() must be idempotent for <pre> content.' );
$this->assertSame( $input, $once, 'wp_kses() must not consume newlines inside <pre>.' );
}There seem to be two problems with block delimiters. I mentioned that the block name may change for core blocks. This seems to be triggered by sanitization:
echo wp_kses( '<!-- wp:image {"alt":"<script>"} -->', 'post' );
// <!-- wp:core/image {"alt":""} -->
echo wp_kses( '<!-- wp:paragraph {"note":"<script>"} --><p>hi</p><!-- /wp:paragraph -->', 'post' );
// <!-- wp:core/paragraph {"note":""} --><p>hi</p><!-- /wp:paragraph -->In both cases, I'd expect the block name to remain unchanged. This is likely limited to Core blocks.
There may be problems with block attributes. Existing KSES seems broken in this example (it escapes the comment <!-- to <!--) so I'm not sure what the baseline behavior should be here.
$x = '<!-- wp:more {"customText":"<more>"} --><!--more <more>--><!-- /wp:more -->';
echo "1: " . wp_kses( $x, ['more'=>[]] ) . "\n";
echo "2: " . wp_kses( wp_kses( $x, ['more'=>[]] ), ['more'=>[]] );1: <!-- wp:more {"customText":"<more>"} --><!--more <more>--><!-- /wp:more -->
2: <!-- wp:core/more {"customText":"\u0026lt;more\u0026gt;"} --><!--more <more>--><!-- /wp:more -->
This block is confusing at many levels (duplicated data, HTML encoded data in comments, etc). It seems to work regardless of the escaping, but I think we can do better here. I'd expect the attribute values to be correctly encoded on the first pass to a fixed point, preserving the intended value and becoming idempotent (line breaks added for clarity):
<!-- wp:more {"customText":"\u003cmore\u003e"} -->
<!--more <more>-->
<!-- /wp:more -->
This raises an additional concern with < escaping in comments, it will cause this to get out of sync 😕 That's the situation right now on trunk, more block with <b> will drift and break for users without unfiltered_html because of the < rewriting to <.
I’ve normalized by removing the
I’ve re-gated this to only block when
That’s something we can iterate on. Fundamentally it’s only about how much accounting we duplicate here. I’d rather start more-locked-down and relax the rules as we can.
Seems reasonable enough to escape, but also, the appropriate way to “escape” shortcodes is with the double-bracket syntax, like
I really don’t like this. It gets really confusing when we start applying it to special atomic elements, which have their own unique escaping rules. I still think this was probably the wrong layer at which to address the problem: if we can audit XML generation we should be free to remove C0-controls-removal as it was originally intended to prevent generating broken XML.
This is what it’s supposed to be doing already, and I think it does. Are you seeing it fail to do that? The problem with the existing behaviors and tests is that If you change your test/example code to a void or special atomic element, or a self-closing foreign element, then it should fully remove it.
Do you think this should be an easy thing to resolve? Just ensure that the modifiable text always starts with a newline? Or is the problem that we’re pushing out the decoded text and what we should be doing is building a custom
This is technically true; the serialization changes, but the block name remains unchanged. I’ve added the aesthetic change, but they are semantically equivalent. I understand the desire for aesthetics, but I’m not scoping this to require byte-identical idempotence.
This is also the scope-level for merging this work. There are lots of things I don’t like about pushing updates in bfedf65 which address your points unless stated otherwise. |
sirreal
left a comment
sirreal
left a comment
There was a problem hiding this comment.
I've left a few comments about comment handling. I'd recommend stronger langauge to make it clear that the section is designed to handle normative comments only.
| if ( WP_HTML_Tag_Processor::COMMENT_AS_HTML_COMMENT !== $this->get_comment_type() ) { | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
Comment handling currently depends on only matching normative HTML comments. If the line above were to change, for example to recognize more comments, there's a risk of not getting the full comment text.
This could use $text = $this->get_full_comment_text(). The result should be identical for normative comments but correct for other types of comments.
There was a problem hiding this comment.
why did we not handle this in set_modifiable_text()? was it to preserve the malformed comment type?
There was a problem hiding this comment.
I'm don't understand your question. get_full_comment_text() should always return what would appear between <!-- and --> when the comment is parsed. get_modifiable_text() has different behaviors for things like PI-lookalike and other kinds of bogus comments because parts of their "full comment text" are not modifiable text because modification could change the HTML structure. For example, <?xml not allowed in html> is a comment where it's "full" and "modifiable" text are different.
This is all covered at set_mofiable_text() because it only allows normative comments, PI nodes, and certain HTML elements.
There was a problem hiding this comment.
I was just thinking, if we had ?xml not allowed in html and changed it with set_modifiable_text() to something else, why don’t we also change the comment type?
honestly I just can’t remember, because I am sure we debated this. maybe it would cause issues for repeated calls to set_modifiable_text()?
| * | ||
| * For the sake of sanitization, remove the comment entirely. | ||
| */ | ||
| $was_incorrectly_closed = '!' === $comment[ strlen( $comment ) - 2 ]; |
There was a problem hiding this comment.
Similar to my other comment, this assumes a normative comment. <!> is an example of a non-normative comment that would satisfy this. It's arguably incorrectly closed as well 🙂
There was a problem hiding this comment.
it doesn’t assume because of the check above to abort for non-normative comments. <!> has a comment type of COMMENT_AS_INVALID_HTML, which gets ignored by the code.
I can see your point about risk for future changes to this code, but as written we don’t allow these and we have no known intentions to widen this later.
we might consider adding behavioral tests, but we’re also in kind of a pickle while wp_kses() botches things, as this is supposed to replace it. I’d like to follow-up after the replacement with further hardening and improvements, once we have a foundation on which to discuss rules vs. emergent behaviors of the interlinked KSES code.
anyway, I appreciate your comment and I think it’s good, but I want to be careful about delaying this work, which brings substantial uplift in reliability over wp_kses() now, by planning for and guarding against a future we want to avoid and for supporting features we actively don’t want.
There was a problem hiding this comment.
the thought crossed my mind that maybe we just ignore all non-normative comments and also those which end in --!>. this is untrusted input, so it might seem reasonable to do that
| $block_type = str_starts_with( $block_type, 'core/' ) | ||
| ? substr( $block_type, /* 'core/' */ 5 ) | ||
| : $block_type; | ||
|
|
||
| $filtered_attributes = filter_block_kses_value( | ||
| $original_attributes, | ||
| $this->allowed_html, | ||
| $this->allowed_protocols, | ||
| array( 'blockName' => $block_type ) | ||
| ); | ||
|
|
||
| if ( $original_attributes !== $filtered_attributes ) { | ||
| $serialized_attributes = serialize_block_attributes( $filtered_attributes ); | ||
| $voider = WP_Block_Processor::VOID === $block_processor->get_delimiter_type() ? '/' : ''; | ||
| $text = " wp:{$block_type} {$serialized_attributes} {$voider}"; |
There was a problem hiding this comment.
The full normalized version of $block_type (including core/ prefix) must happen after filter_block_kses_value because that expects to be passed the complete block type including the core/ prefix. See:
wordpress-develop/src/wp-includes/blocks.php
Lines 2196 to 2198 in 88f2c9e
It's essential to remove the core/ implicit prefix for the serialization, but not before:
| $block_type = str_starts_with( $block_type, 'core/' ) | |
| ? substr( $block_type, /* 'core/' */ 5 ) | |
| : $block_type; | |
| $filtered_attributes = filter_block_kses_value( | |
| $original_attributes, | |
| $this->allowed_html, | |
| $this->allowed_protocols, | |
| array( 'blockName' => $block_type ) | |
| ); | |
| if ( $original_attributes !== $filtered_attributes ) { | |
| $serialized_attributes = serialize_block_attributes( $filtered_attributes ); | |
| $voider = WP_Block_Processor::VOID === $block_processor->get_delimiter_type() ? '/' : ''; | |
| $text = " wp:{$block_type} {$serialized_attributes} {$voider}"; | |
| $filtered_attributes = filter_block_kses_value( | |
| $original_attributes, | |
| $this->allowed_html, | |
| $this->allowed_protocols, | |
| array( 'blockName' => $block_type ) | |
| ); | |
| if ( $original_attributes !== $filtered_attributes ) { | |
| $serialized_attributes = serialize_block_attributes( $filtered_attributes ); | |
| $block_type = str_starts_with( $block_type, 'core/' ) | |
| ? substr( $block_type, /* 'core/' */ 5 ) | |
| : $block_type; | |
| $voider = WP_Block_Processor::VOID === $block_processor->get_delimiter_type() ? '/' : ''; | |
| $text = " wp:{$block_type} {$serialized_attributes} {$voider}"; |
There was a problem hiding this comment.
already saw that but hadn’t pushed it. thank you!
A few elements ignore a first newline: TEXTAREA, PRE, LISTING. TEXTAREA is already covered by wordpress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php Lines 4191 to 4200 in f27699d It should be covered on wordpress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php Lines 3867 to 3879 in f27699d I think the best thing to do here for PRE and LISTING is to always inject a leading newline after their open tag, something like this: $output .= $tag_maker->get_updated_html();
if ( 'html' === $namespace && 'PRE' === $tag_name || 'LISTING' === $tag_name ) {
$output .= "\n";
}I shared a potential test for this, it could probably be added to the idempotency test data set: public function test_pre_leading_newline_is_preserved_and_idempotent(): void {
$input = "<pre>\n\ncode</pre>";
$once = wp_kses( $input, 'post' );
$twice = wp_kses( $once, 'post' );
$this->assertSame( $once, $twice, 'wp_kses() must be idempotent for <pre> content.' );
$this->assertSame( $input, $once, 'wp_kses() must not consume newlines inside <pre>.' );
} |
|
There are some behavioral changes around unclosed blocks. Before, KSES would close them (either as void or with a closing delimiter):
|
This patch introduces a new version of `wp_kses()`, temporarily living alongside the legacy implementation in a new function named `wp_sanitize_html_kses()`. This new implementation is based on the HTML API, which provides more reliable parsing of HTML inputs. - A new filter, `wp_kses_force_legacy_parser`, offers an opt-out for the new code, and for testing side-by-side. - Many tests have been updated because of hard-coded values which result from historic HTML parsing oddities. They have been updated to reflect the newer parsing, the spec-compliant parsing. There should be no changes necessary to calling function with this change. Co-Authored-By: Jon Surrell <jonsurrel@git.wordpress.org>
2 participants
Trac ticket: Core-66208
Trac ticket: Core-65984
Replaces #6577
Description
Rewrites
wp_kses()to rely on the HTML API for structural and reliable application of sanitization rules, normalizing the output for improved downstream parsing.Notables
wp_kses_force_legacy_parserprovides the choice of whether to use this new parser or stick with the legacy code.Fixes
wp_kses_split()calls.wp_kses().wp_kses()turns SCRIPT and STYLE content into renderable text.wp_kses()and HTML disagree on the set of named character references.parse_blocks()called unnecessarily.wp_kses()un-comments HTML comments.wp_kses_post()incorrectly escapes "<" attributes values.Todo
Merge after #13273, which accounts for three of the failing tests.wp_kses(), it’s possible to simply wait until an opened element is closed based on depth, and skip that closing element if it exists.wp_kses()with intentionally-incomplete input, for example, a wrapper opening tag with part of the content, separately from the closer. closing open elements does a good job of isolating content, but legacy behaviors depend too much on the more procedural use ofwp_kses()so isolation cannot be reasonably added without mangling websites.pre_ksesfilters but then callpre_ksesserialize_to_xml()instead).xmlcontext, it only leaves the five syntax characters as names, and decodes everything else. This is correlated with the default behavior for the new implementation for XML and HTML.wp_kses()is entirely inappropriate for XML inputs or XML outputs. That requiresserialize_to_xml().[and]should remain escaped through sanitization.Notes