Some problems with double? encoding/decoding of HTML entities

This is caused by filter_xss:

// Summary ' is converted in filter_xss to '
// but the named conversion turns it back in ' the decimal conversion then turns it in & #039;

  // Defuse all HTML entities, results in '
  $string = str_replace('&', '&', $string);
  // Change back only well-formed entities in our whitelist

  // Named entities, results in '
  $string = preg_replace('/&([A-Za-z][A-Za-z0-9]*;)/', '&\1', $string);

  // Decimal numeric entities, oops, results in & #039;
  $string = preg_replace('/&#([0-9]+;)/', '&#\1', $string);

//  (idem)
  // Hexadecimal numeric entities
  $string = preg_replace('/&#[Xx]0*((?:[0-9A-Fa-f]{2})+;)/', '&#x\1', $string);

Requires extremely thorough review.

Bumping all the way to D7.

This isn't right either.

Subscribing.

Slept on it, still makes sense.

I think it looks good. Now neither of the first two preg_replace() calls can generate a substring that - together with any prefix or suffix - matches the following preg_replace patterns.

As an alternative approach, the lines visible in the patch can be replaced by a single preg_replace that uses a negative look-ahead assertion (the pattern for hexadecimal entities is modified to also allow characters with an odd number of digits, e.g. Օ):
$string = preg_replace('/&(?![a-z][a-z0-9]*;|#[0-9]+;|#x[0-9a-f]+;)/i', '&', $string);

Here's the patch from #4 plus some unit testing of filter_xss(). The third test/assert currently fails, but is fixed by the patch. I can't think of any things that should break by this patch (or I would've tested them :)), nor can I think of any XSS flaws that should be introduced by this. I'm not confident enough to RTBC it though. :)

(Also, the tests probably need some touching up, so please do that. It would also be good with some more strings to test. E.g. with some HTML and stuff.)

D'oh. And the patch. -.-

There's a bunch of XSS examples on this issue which never made it into the committed patch if that helps with test fodder - http://drupal.org/node/354812

I do not see anything immediately wrong in here... the code looks ok, the tests pass.

After looking at the function for a while, I can't see anything wrong with this either. Committed it.

Patch from #4 should apply to Drupal 6 with a 15 line offset.

Sorry, needs review

Committed to Drupal 6 too, thanks. Bug applies to Drupal 5 too as said above.

Marking as won't fix as d5 is end of life.