When a site title contains an & it is rendered as &

The page processing uses filter_xss_admin() to protect the site title from XSS attacks. This ensures that any bare &s are converted to &. The protected string is then used with l() to form a link to the home page. As the 'html' option flag is not set to TRUE the string is passed through check_plain(), which uses htmlentities(). That blindly converts all &s to & with the result that the string now contains & instead of & (or &).

The basic solution is to set the 'html' flag. (See: basic.diff)

filter_xss_admin() is actually over-permissive in this case as you shouldn't (IMHO) be using complex markup in the title, although I can see why you might want to include some elements to help with styling. I've included an alternative patch that also restricts the allowed markup to inline elements. (See: extended.diff)

CommentFileSizeAuthor
extended.diff670 bytesslite
basic.diff317 bytesslite

Comments

slite’s picture

Don't you just love the consistency. The issue system is over-zealous about converting & back into bare &. Note to self: Always remember to preview 8-)

8<----8<----
When a site title contains an & it is rendered as &amp;

The page processing uses filter_xss_admin() to protect the site title from XSS attacks. This ensures that any bare &s are converted to &amp;. The protected string is then used with l() to form a link to the home page. As the 'html' option flag is not set to TRUE the string is passed through check_plain(), which uses htmlentities(). That blindly converts all &s to &amp; with the result that the string now contains &amp;amp; instead of & (or &amp).
8<----8<----

Jeff Burnz’s picture

Status: Active » Needs work
+++ inc/template.process.inc	2011-04-20 12:24:05.215766764 +0100
@@ -120,11 +120,12 @@
+  $sitename = filter_xss(variable_get('site_name', 'Drupal'), array('b', 'big','br', 'em', 'i', 'small', 'span', 'strong', 'sub', 'sup'));

Maybe it makes more sense to use check_markup(variable_get('site_name'), 1) (assuming id #1 is filtered html, which I havent actually checked :) This shit always confuses the crap out of me, so I probably am being over zealous with security.

Powered by Dreditor.

heine’s picture

Just like an individual's name, a sitename is plaintext and should be displayed as is.

To do so, it needs to be check_plained, which is what l() already does.

slite’s picture

The real issue for me isn't that check_plain() is being used, but that it is being used as part of a combination which breaks perfectly reasonable site titles. Feeding the site title directly into l() would be a perfectly good solution (as far as I'm concerned 8-) but the patch was on the principle of least breakage.

Personally I don't care whether tags are allowed in the site name or not, although if they *aren't* allowed then IMHO this should be enforced by Drupal core. (This isn't the only theme where problems occur because of the confusion/inconsistent usage of this.) I accounted for them simply because they *are* currently allowed.

jmlane’s picture

I spent a bit of time digging around the Drupal 7 theme API trying to get a sense of where the $site_name variable in page.tpl.php is being set and how it is potentially being filtered. It seems that variable_get(site_name, "Drupal") is considered to be HTML content and thus the call to filter_xss_admin() which strips all non-whitelisted tags from the output. From this brief analysis, I'd say it has something to with the l() call wrapping $sitename that is then assigned to $variables['site_name']. I'll have to debug it a bit when I have some more time.

This seems to be the main issue for the #461938: Core should consistently filter_xss_admin() on $site_slogan and check_plain $site_name issue that is still unresolved for both Drupal 7 and 8 HEAD.

Jeff Burnz’s picture

Version: 7.x-1.0 » 7.x-3.x-dev
Component: Code » CSS/HTML
Status: Needs work » Fixed

Fixed in 7.x-3.x, need to check if I changed this in older versions, now we just run with Heine's original view to just use l()

Jeff Burnz’s picture

Status: Fixed » Closed (fixed)