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']
Comment | File | Size | Author |
---|---|---|---|
#18 | term-name-sanitized-1751054-18.patch | 1.29 KB | David_Rothstein |
#14 | drupal-taxonomy_term_tpl-1751054-12.patch | 652 bytes | vomiand |
#11 | term-name-documentation-1751054-11.patch | 652 bytes | Berdir |
#8 | drupal-taxonomy-term-1751054-7.patch | 1.24 KB | borisbaldinger |
#1 | drupal-taxonomy-term-1751054.patch | 1.24 KB | borisbaldinger |
Comments
Comment #1
borisbaldinger CreditAttribution: borisbaldinger commentedI 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.
Comment #2
borisbaldinger CreditAttribution: borisbaldinger commentedComment #3
Schnitzel CreditAttribution: Schnitzel commented+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 :)Comment #4
BerdirSecond that, agree that this should be removed. There's no need to have $name and $term_name/label.
Comment #5
Schnitzel CreditAttribution: Schnitzel commentedadding tags, because:
* - $name: the (sanitized) name of the term.
is actually wrong! it is not sanitized.
Comment #6
catch#1: drupal-taxonomy-term-1751054.patch queued for re-testing.
Comment #8
borisbaldinger CreditAttribution: borisbaldinger commentedRewrote the patch on the actual codebase of drupal 8.x
Comment #9
Schnitzel CreditAttribution: Schnitzel commentedreroll looks good. RTBC
Comment #10
catchCommitted/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.
Comment #11
BerdirI 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.
Comment #12
xjmThis 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.)
Comment #13
xjmComment #14
vomiand CreditAttribution: vomiand commentedHi
I fixed issue from comment #12.
Please review.
I hope this helps.
Comment #15
BerdirLooks good.
Comment #16
xjmThanks @vomiand!
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedWow, 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
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commentedYou 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.
Comment #19
xjmYeah, that seems worthwhile to me. Two questions, though:
Comment #20
xjmComment #21
David_Rothstein CreditAttribution: David_Rothstein commentedHm, 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 :)
Comment #22
BerdirHm, 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.
Comment #23
xjmAlright, 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.Comment #24
babruix CreditAttribution: babruix commentedCould someone provide a bit more clear info of what exactly should be tested?
Comment #25
dcam CreditAttribution: dcam commentedhttp://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.
Comment #25.0
dcam CreditAttribution: dcam commentedUpdated issue summary.
Comment #25.1
joshy333 CreditAttribution: joshy333 commentedAttended on DrupalCon Prague 13 mentored sprint and picked up the last comment and attempted to update and fix the issue summary.
Comment #26
inders CreditAttribution: inders commentedAs 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).
Comment #26.0
inders CreditAttribution: inders commentedUpdated issue summary.
Comment #26.1
inders CreditAttribution: inders commentedUpdated issue summary.
Comment #26.2
inders CreditAttribution: inders commentedAdding steps to reproduce & Problem/Motivation
Comment #26.3
inders CreditAttribution: inders commentedUpdated issue summary.
Comment #27
mgifford18: term-name-sanitized-1751054-18.patch queued for re-testing.
Comment #30
stefan.r CreditAttribution: stefan.r commentedMarking addition of tests as a novice task