Updated: Comment #26

Problem/Motivation

$name is not sanitized and can cause possible security problems if not handled properly in template.

Steps to reproduce

  • Add a new taxonomy term with code in its name:
     <script>alert("script");</script>
     
  • Print $name in taxonomy-term.tpl.php . Alert window will appear.

Proposed resolution

  • In D7 $name is not sanitized inside (taxonomy-term.tpl.php) and needs to be sanitized to prevent possible security problems.
  • Allow basic tags such i,span,b,em etc inside term name to prevent breaking existing sites.

Remaining tasks

  • Sanitize $name variable using filter_xss() or other function . We can still allow users to use basic tags (Someone may be using i,span tags inside term name)

Original report by borisbaldinger

In template_preprocess_taxonomy_term() the $variables array and the $term object are merged:

 $variables = array_merge((array) $term, $variables);

This is not really required because the whole term is available within the variables: $variables['term']

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

borisbaldinger’s picture

I removed the merge of the template variables array and the term. I think we don't need to backport this to D7 because many people are using the flattened variables.
Also i removed the $name from the comments in the template file because this variable isn't available anymore because it came from the merger of the term with the variables array.

borisbaldinger’s picture

Status: Active » Needs review
Schnitzel’s picture

+1 from me, found this during working on #1642070: Make use of entity labels in templates because it is like an defacto standard, that variables which are non arrays are "secure", and if an themer uses $term->something he is more carefull :)

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Second that, agree that this should be removed. There's no need to have $name and $term_name/label.

Schnitzel’s picture

Issue tags: +Needs backport to D7

adding tags, because:
* - $name: the (sanitized) name of the term.
is actually wrong! it is not sanitized.

catch’s picture

Issue tags: -Needs backport to D7

#1: drupal-taxonomy-term-1751054.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, drupal-taxonomy-term-1751054.patch, failed testing.

borisbaldinger’s picture

Status: Needs work » Needs review
FileSize
1.24 KB

Rewrote the patch on the actual codebase of drupal 8.x

Schnitzel’s picture

Status: Needs review » Reviewed & tested by the community

reroll looks good. RTBC

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.x.

I'm not sure if this can be backported, but I'll leave that to webchick/David as to balancing the bug fix vs. API change. Whether it is or not we'll need a change notification I think.

Berdir’s picture

Status: Patch (to be ported) » Needs review
FileSize
652 bytes

I think we just want to fix the documentation here, everything else would be a API change.

Here's a try, not sure if this is correct in regards to coding/documentation standards.

xjm’s picture

Issue tags: +Novice
+++ b/modules/taxonomy/taxonomy-term.tpl.phpundefined
@@ -5,7 +5,8 @@
+ * - $name: (deprecated) the unsanitized name of the term, use $term_name
+ *   instead.

This looks reasonable to me. Two small fixes: "The" should be capitalized, and it should be two sentences rather than joined with a comma. (This is an example of what prescriptive English teachers call a comma splice.)

xjm’s picture

Assigned: borisbaldinger » Unassigned
Status: Needs review » Needs work
vomiand’s picture

Status: Needs work » Needs review
FileSize
652 bytes

Hi

I fixed issue from comment #12.

Please review.

I hope this helps.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

xjm’s picture

Thanks @vomiand!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.22 release notes

Wow, that's a pretty serious documentation bug (worthy of a release notes mention)...

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/ba9b60d

David_Rothstein’s picture

Title: Taxonomy template variables are getting merged with the term itself » Taxonomy template variables are getting merged with the term itself (security aspects of $name variable were documented wrong)
Priority: Normal » Major
Status: Fixed » Needs review
Issue tags: +Security improvements
FileSize
1.29 KB

You know, the fact that that documentation mistake has been there for so long makes me think we should really just sanitize it. Too much chance people out there have security holes in their themes and are being led by the documentation to believe they don't.

xjm’s picture

Issue tags: -Security improvements

Yeah, that seems worthwhile to me. Two questions, though:

  1. Do we risk breaking sites that intentionally put markup in term names, or something? (Maybe it's worth it given the security implications.)
  2. Do we want a test for D7 confirming that it's sanitized? (D8 has a bunch of these already).
xjm’s picture

Issue tags: +Security improvements
David_Rothstein’s picture

Hm, you are right, $term_name is run through check_plain() so you can't have any markup in there at all.

I don't know... Deliberately putting HTML in a taxonomy term name sounds like an unusual thing to try to do, but it's possible someone is doing it. We could run $name through filter_xss() or something instead, although that seems wasteful for a variable that very few people are likely to even be using.

Tests sound like a good idea also :)

Berdir’s picture

Hm, given that term names are already check_plain()'d everywhere (see e.g. taxonomy_field_formatter_view()), it seems highly unlikely that someone has been using tags in them :)

+1 from me.

xjm’s picture

Issue tags: +Needs tests

Alright, let's just do this then.

core/modules/system/lib/Drupal/system/Tests/Theme/EntityFilteringThemeTest.php in D8 might be a starting point for ideas on how to test it.

babruix’s picture

Could someone provide a bit more clear info of what exactly should be tested?

dcam’s picture

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

dcam’s picture

Issue summary: View changes

Updated issue summary.

joshy333’s picture

Issue summary: View changes

Attended on DrupalCon Prague 13 mentored sprint and picked up the last comment and attempted to update and fix the issue summary.

inders’s picture

As the wrong comment has been there for so long might be worth to sanitize the variable and left the comment there because this makes it true for D7. In D7 $name in unsanitized. Inside (taxonomy-term.tpl.php).

We can sanitize $name variable using filter_xss() or other function to prevent possible security problems. We can still allow users to use basic tags (Someone may be using i,span tags inside term name).

inders’s picture

Issue summary: View changes

Updated issue summary.

inders’s picture

Issue summary: View changes

Updated issue summary.

inders’s picture

Issue summary: View changes

Adding steps to reproduce & Problem/Motivation

inders’s picture

Issue summary: View changes

Updated issue summary.

mgifford’s picture

  • catch committed e6bfe4f on 8.3.x
    Issue #1751054 by borisbaldinger: Fixed Taxonomy template variables are...

  • catch committed e6bfe4f on 8.3.x
    Issue #1751054 by borisbaldinger: Fixed Taxonomy template variables are...
stefan.r’s picture

Marking addition of tests as a novice task

  • catch committed e6bfe4f on 8.4.x
    Issue #1751054 by borisbaldinger: Fixed Taxonomy template variables are...

  • catch committed e6bfe4f on 8.4.x
    Issue #1751054 by borisbaldinger: Fixed Taxonomy template variables are...

  • catch committed e6bfe4f on 9.1.x
    Issue #1751054 by borisbaldinger: Fixed Taxonomy template variables are...