check for common XSS mistakes with drupal_set_title and drupal_set_message

This patch isn't perfect - it doesn't have knowledge of, for example:

But I think that's pretty uncommon anyway and is worth a second look by the maintainer.

However, perhaps the comment about the security test that "What it catches is good" should be changed if this patch is accepted.

chx pointed out that it's more important to have t( than it is to have an array. So, I added the t( check to the #never.

The patch needs a bit more work as it is incorrectly flagging lines like drupal_set_title(check_plain($title));. I've attached a new version of the patch, which doesn't fix this issue, but adds in the beginnings of a series of tests for these new rules. It might be an idea to add more of these tests - it should help you verify the rules work in all scenarios.

Cheers,
Stella

And here's an updated patch.

My feeling on this is that coder will never be able to properly categorize all the weaknesses here and so we should err on the side of showing some false positives that require human investigation rather than having false negatives (i.e. real weaknesses that are not identified as such). The current code will not identify a lot of things that are actually vulnerabilities, but it should be pretty good about not having false positives.

Here's a slightly modified version of the patch. I've added in the following changes:

• Additional simple tests for both testing drupal_set_message() and drupal_set_title() - we need one for every scenario we can think (both good and bad) for both functions.
• Added in format_plural() into list of allowed functions.
• Changed regex so lines like function drupal_set_message(...) aren't matched.
• Changed regex so the use of the t() ! placeholder gives a warning - we should alert on this if we're going to alert on drupal_set_title($message);
• Changed regex so in addition to allowing t(), we also allow their counterparts st() and $t() ($t() is set by get_t())

Cheers,
Stella