inconsistent use of filter_xss_admin() on $site_slogan and $site_name

Here's the HEAD and 6.x-dev patches.

Looks good, footer and mission are gone in D7 but this looks like a backport to D6 as well.

The last submitted patch failed testing.

Setting to previous status - testbot was broken.

Committed to CVS HEAD. Moving to D6.

The D6 patch was included as well and the changes are identical. Ready for commit?

Committed to Drupal 6, thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

whoops... we missed a few spots in $head_title. Follow-up patch is attached. BTW this seems to be the cause of a number of issues with using an ampersand in site_name.

#535240: Ampersands in site name are not escaped in title tag
#527776: Cannot display ampersand in site title
#167176: ampersand in site title not transformed to HTML leads to invalid html code
#371975: &s and other entities
#258137: $head_title construction lacks check_plain calls

Applied the patch and my D7 HEAD did still function. I added some &%$§ to the head and it works. From the code I'd say its okay.

Cool, let's get some tests for this please.

Okay the test seems quiet easy to me. I just enter some markup for the title like "###< / title > ###". Without the patch the html is broken. With this patch I can check for the escaped string inside the real < title > and everything is fine. Done this by hand and it testable.

But I have a question before I go coding this as I am new to writing tests for core:

Where should it go to?

Is there already a testcase it can be added or should this go in a new file to setup where?

And one more thing just appeared to me:

I thought slogan is now a block? If I activate "slogan" in Garland as this is patched by this patch as well it is added to the site like it was in D6? I mean this is a problem about writing the test. Should I add the slogan block or just activate garlands settings?

Thanks to stBorchert I figured that I should put this into system.test. So I made a little test to check if site name and slogan are escaped correctly. And I reworked the patch to latest HEAD.

The last submitted patch failed testing.

wrong path...

Looks good to me. Thanks for the tests.

The last submitted patch failed testing.

Reroll against latest HEAD.

looks good.

I believe we want filter_xss_admin() instead, since these values are entered by the administrator.

Also, could you please add a line of PHPDoc to:

webchick: The reason filter_xss_admin isn't used is that tags aren't allowed in the <title>. My patch in #9 actually used

. It looks like Kars-T removed the array(). I hadn't noticed that, so I'm glad you caught it.

Now that raises the obvious question, why not just use check_plain, or strip_tags?

Check plain escapes tags, so isn't suitable if we allow HTML in site_name. (I think a good argument can be made that HTML shouldn't be allowed at all, since site_name is used in e-mail and other places.) However past versions (at least back to 4.7 I think) have allowed HTML, so I expect some users would see this as a regression.

Ok, so what about strip_tags? strip_tags won't entify ampersands, which is part of what got this whole issue started. If you put &amp; directly in your site_name, then you'll get email like "An administrator created an account for you at stuff &amp; things." (Which I suppose is an argument against allowing HTML here).

So, #9 was intended as a compromise for to accommodate both situations. What do you think? Should we go to check_plain?

filter_xss_admin() for slogan and check_plain() for the site_name? If Garland did this before for site_name and it did workout why don't keep this behavior?
Otherwise I fear we would need more context where and why site_name is used or split it up in different variables. Both not really desirable to me.

filter_xss_admin() for slogan and check_plain() for the site_name?

Fair enough. Kars-T, if you're willing to update the patch I'll do another review. Also, regarding the test:
You could probably shrink it a bit by using variable_set('site_name', 'foo'). I don't think you really need to go through the admin form. Could you also add a test that includes an ampersand in the site name, since there are a number of bug reports for that specifically? Thanks!

Sure will do probably tomorrow :)

I currently can't test the slogan with garland.
#602372: $site_slogan no longer used in page.tpl.php

I don't know if changing the default theme to stark during the test is an appreciated thing to do. Should tests always use garland?

D6 patch attached.

Garrett: can we keep the backport tag until the version is changed (i.e. the 7.x fix is committed) ? Also per #23 I think we're going ahead with check_plain for site_name.

Kars-T: I would go ahead with the patch, and don't worry about changing the theme. If slogan isn't testable I think we can live with that.

Okay, here's a patch that uses check_plain() for $site_name.

Incidentally, I ran into this problem when I ran into the ampersand issue as in #9.

New Patch against latest HEAD.

As discussed site_name is check_plain() and slogan filter_xss_admin(). The test for the slogan is escaped right now. And I did add a short comment.

Why is this tagged with Minelli as it just adds CSS and no tpl files or do I miss something?