KSES: Add test cases for "bogus comment" syntax not currently detected. #6833
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. |
dmsnell
force-pushed
the
kses/fix-missing-invalid-comment-detection
branch
2 times, most recently
from
ba487d9 to
1c7befa
Compare
tests/phpunit/tests/kses.php
Outdated
| $this->assertFalse( | ||
| current_user_can( 'unfiltered_html' ), | ||
| 'Test needs to verify behavior without "unfiltered_html" capability.' | ||
| ); |
There was a problem hiding this comment.
wp_kses itself does not check user caps right? So this is not needed?
There was a problem hiding this comment.
I think that's right, but the test is somewhat useless if that capability is available. it was working properly already, but I didn't know for sure. maybe it's best to nix this part. thank you.
In [58418] a test was added without the `test_` prefix in its function name, and because of that, it wasn't run in the test suite. The prefix has been added to ensure that it runs. In the original patch, due to a logical bug, a recursive loop to transform the inside contents of the bogus comments was never run more than once. This has been fixed. This patch also includes one more case where `kses` wasn't properly detecting the bogus comment state, and adds a test case to cover this. It limits itself to some but not all constructions of invalid markup declaration so that it doesn't conflict with existing behaviors around those and other kinds of invalid comments. Props ellatrix, dmsnell. See #61009. Follow-up to [58418]. Co-authored-by: ellatrix <[email protected]>
dmsnell
force-pushed
the
kses/fix-missing-invalid-comment-detection
branch
from
1c7befa to
800c254
Compare
| @@ -988,6 +988,9 @@ function wp_kses_split( $content, $allowed_html, $allowed_protocols ) { | |||
| (<!--.*?(-->|$)) # - Normative HTML comments. | |||
| | | |||
| </[^a-zA-Z][^>]*> # - Closing tags with invalid tag names. | |||
| | | |||
| <![^>]*> # - Invalid markup declaration nodes. Not all invalid nodes | |||
There was a problem hiding this comment.
Should we make the same [a-z] check here?
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| do { | ||
| $prev = $content; | ||
| $content = wp_kses( $content, $allowed_html, $allowed_protocols ); | ||
| } while ( $prev !== $content ); |
There was a problem hiding this comment.
Is there a test case that fails in trunk and now passes in the PR as a result of this fixed loop?
There was a problem hiding this comment.
no. I think it's because we don't have a test case showing how one form of sanitization adds a new thing needing sanitization. given that this is copying the existing behavior I don't want to touch that, and I'm not even sure what the scenario would be where it would need to trigger this.
There was a problem hiding this comment.
For anyone else looking at this, this is just copying https://github.com/WordPress/wordpress-develop/pull/6833/files#diff-13e853d5729978a0e7936543d65ad1a3e120b994702c9559bf99c3ad857f5066R1153, but had to be rewritten to make the linter happy.
In [58418] a test was added without the
test_prefix in its functionname, and because of that, it wasn't run in the test suite.
The prefix has been added to ensure that it runs.
In the original patch, due to a logical bug, a recursive loop to
transform the inside contents of the bogus comments was never run
more than once. This has been fixed.
This patch also includes one more case where
kseswasn'tproperly detecting the bogus comment state, and adds a test case
to cover this. It limits itself to some but not all constructions
of invalid markup declaration so that it doesn't conflict with
existing behaviors around those and other kinds of invalid comments.
See also #6840