wp_kses makes HTML comment HTML uncommented

[kkmuffme]

asd <!-- <a href="other-page.com" class="hello">world</a> --> asd

when calling wp_kses(_post) on it, this commented HTML gets uncommented and you get a link displayed on the page.

asd &lt;!-- <a href="other-page.com" class="hello">world</a> --&gt; asd

If that commented code contains some unsanitized stuff (as is often the case since people assume it's commented and thus can be ignored - including phpcs won't report errors for commented stuff afaik) this could be a security issue.

Generally though, it's just an unexpected display issue.

Trac ticket: https://core.trac.wordpress.org/ticket/61246

I also added tests for the original ticket that introduced the deprecated function https://core.trac.wordpress.org/ticket/4409, since there were none.

Additionally, the fix also improves the existing logic by combining the escaping logic for <> into a single regex (ensuring it cannot be partially broken by removing of a filter and can be relied upon in 100% of cases).

Furthermore, a side effect is that it fixes broken behavior that kses would leave the last single or double quote unescaped, while escaping everything else unnecessarily.
see xssAttacks.xml "Remote Stylesheet 3" it will unnecessarily escape the " but will leave the last " unescaped
This seems to be a general issue that the last ' or " won't be escaped, e.g. also in
jQuery('#abc').append('<iframe src="xyz.com"></iframe>');
you'll end up with
jQuery('#abc').append('');

which itself was unsafe as the '); delimits the string now and can probably somehow abused (not too familiar with that tbf, so feedback welcome)

[jamieblomerus]

I have taken a look at this and can confirm that this is indeed a bug I could reproduce from the trunk. I have also tested the patch and it seems to address this issue accordingly. Good job!

[hellofromTonya]

The patch is not changing the code introduced in the 6.6 cycle, i.e. changed via r58418 and its Trac ticket #61009. For now, I'm removing 6.6 as the Version.

@dmsnell worked on KSES during the 6.6 cycle in #61009.

I wonder ... given the changes, has this bug been resolved? or does it still persist?

[dmsnell]

Thanks @hellofromTonya - the bug is still present in trunk

<?php

var_dump( wp_kses_post( 'asd <!-- <a href="other-page.com" class="hello">world</a> --> asd' ) );

string(71) "asd &lt;!-- <a href="other-page.com" class="hello">world</a> --&gt; asd"

It is a very complicated issue though, particularly since comment handling is recursive and a fix in one spot might cascade into others.

One big question to answer is whether fixing this in this function would expose new issues in other places that assume the content coming out of wp_kses() is a certain way. I'd love to see this entire stack of functions be replaced with a single pass with the Tag Processor, but I don't yet have all the answers to those questions.