On the browse projects by category page (for example http://drupal.org/project/Modules), the links to the various categories do not have the title attribute set, and thus there are no pop up tooltips with the description of the category. I don't see any reason why we wouldn't want these to display, and when the links are created in theme_project_term_list() the description field is already part of the $terms object. The attached patch changes one line in the function to add the title attributes to the link.

CommentFileSizeAuthor
project_browse_term_description_0.txt661 bytesaclight

Comments

hunmonk’s picture

hunmonk commented
Status: Needs review » Needs work

hunmonk: chx: is strip_tags basically a check_plain that doesn't do replacement but removal?
chx: strip_tags is a piece of shit
chx: non unicode safe
chx: you want filter_xss($string, array())

aclight’s picture

aclight commented
Status: Needs work » Needs review

hunmonk: chx: is strip_tags basically a check_plain that doesn't do replacement but removal?

I think that's exactly what we want. Administrators are allowed to place HTML tags in the description field of a taxonomy term, but we don't want those HTML tags to show up in the title attribute of the link.

The patch I provided does the same thing that taxonomy_link() does in taxonomy.module:

function taxonomy_link($type, $node = NULL) {
  if ($type == 'taxonomy terms' && $node != NULL) {
    $links = array();
    if (array_key_exists('taxonomy', $node)) {
      foreach ($node->taxonomy as $term) {
        $links['taxonomy_term_'. $term->tid] = array(
          'title' => $term->name,
          'href' => taxonomy_term_path($term),
          'attributes' => array('rel' => 'tag', 'title' => strip_tags($term->description))     <------
        );
      }
    }
...
dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

@aclight: hunmonk's paste from chx's IRC session is still relevant. strip_tags() is crap. filter_xss() does exactly what you want: strip out any potentially malicous HTML tags but still allow harmess HTML in the output.

dww’s picture

dww
we/he/they
commented

sorry, i wasn't entirely clear... you mean inside the title attribute, where we don't want any tags at all. that's why you want to pass the empty array() as the 2nd argument, which defines what tags you want to leave in. in your case, you want none, hence, the empty array. make sense?

aclight’s picture

aclight commented

The problem with using filter_xss is that is also "Defuse[s] all HTML entities". For example,:

& --> &amp;
< --> &lt;
> --> &gt;

Firefox (and probably other browsers) do not treat the title attribute text as HTML and therefore don't display these sanitized HTML entities as they should be displayed. So a title might look like

A &lt; sign is used &amp; it looks bad

where the desired display would be

A < sign is used & it looks bad

Is it even possible to create XSS vulnerabilities in title attributes? I'm guessing that even if there was naughty javascript or something there that it wouldn't be parsed by the browser anyway.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Actually, because links are passed through the l() function, which in turn check_plains on both the link title and any attributes (via the drupal_attributes()) function, I think you're safe to put the description in directly.

aclight’s picture

aclight commented

@webchick: I tried removing strip_tags and just putting the description in unchecked. The title attribute of the link on the rendered page still had HTML tags in it, and special characters were not encoded as HTML entities. I'm not quite sure why, because looking at the code it seems that you should be right. But for some reason that's not what is actually happening.

aclight’s picture

aclight commented
Status: Needs work » Needs review

I'm setting this back to cnr because I am still not convinced that filter_xss is the appropriate thing to use here, given my comments in #5.

Let me reiterate that core also uses strip_tags in the function taxonomy_link(). I know that core isn't perfect, but I couldn't find any issues/bug reports/etc. suggesting that filter_xss should be used in the taxonomy_link() function instead of strip_tags().

Furthermore, if using strip_tags() is some kind of vulnerability, any site using taxonomy will already be subject to any problems that could result, so using strip_tags() in theme_project_term_list() shouldn't add any NEW problems.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Fixed

Ok, I'm convinced. Tested and then committed to DRUPAL-4-7, DRUPAL-4-7--2, and HEAD. Installed on d.o. Now we need an infra issue about giving more of those terms a description. ;)

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)