Term names are double-HTML-encoded in RSS feed titles and <category> elements, and also in the feed links on taxonomy/term/* pages (both the <link> element in the HTML head and the feed icon in the HTML body). You can reproduce this by creating a term with an ampersand such as AT&T. The term name is incorrectly rendered as AT&amp;T (rather than AT&T) in both XML and HTML.
Attached patch is for Drupal 6 (I'm about to check if a patch is needed for Drupal 7).
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | taxonomy-term-encode.patch | 1.38 KB | mfb |
| taxonomy-term-encode.patch | 1.5 KB | mfb |
Comments
Comment #1
mfbSlightly different patch for Drupal 7, as drupal_set_title() now takes care of the check_plain() for us.
Comment #2
mrfelton commentedI can confirm that this patch fixes the said issue. Although, shouldn't we be running some kind of xss filter? I don't think we should be printing unfiltered user data like this, but agreed that check_plain is not the right filter to use.
Comment #3
mfbBut, there is still a check_plain() before the HTML and XML are rendered. You can test with the patch applied that < becomes
<and & becomes&etc.That's also all we do in case, for example, someone puts a <script> tag in their title: the title is simply HTML-encoded as
<script>, not cleansed of potentially unwanted markup. I don't see why term name should be treated any differently from node title.Comment #4
mrfelton commentedok, fair enough... I just wasn't sure of the policy with filtering data like this, and I didn't know that check_plain was run before the render also. Well, the patch certainly does fix the issue at hand then.
Comment #5
dries commentedGood catch. I committed this to CVS HEAD, and I'm updating the version to D6 for Gabor to review.
Comment #6
gábor hojtsymrfelton: as for the taxonomy_rss_item() implementation, it is an implementation of hook_nodeapi(rss item) - is called from taxonomy_nodeapi(). That hooks is used from node_feed() and the data from there is passed on to http://api.drupal.org/api/function/format_rss_item/6 which calls http://api.drupal.org/api/function/format_xml_elements/6 on these extra values, which check_plain()s 'value' and calls drupal_attributes() on any attribute which also does check_plain(). So looks like a good change to remove it from here.
The other change makes $title unescaped for a http://api.drupal.org/api/function/drupal_add_feed/6 call, which again uses drupal_attributes() which does the escaping. Also, the second change makes the $title unescaped for setting up as suffix to $channel['title']. That is then passed on to node_feed() which calls http://api.drupal.org/api/function/format_rss_channel/6 at the end where it gets escaped.
So all looks good and safe, committed to Drupal 6!
Comment #7
mrfelton commentedGábor Hojtsy: Thanks for the explanation... I've been developing modules for Drupal for quite some time, but this is the first attempt I've made at helping out with D7 and I'm not 100% familiar with the inner workings of Drupal in areas other than those that have concerned modules I develop/maintain. Hopefully I'll be able to give more input over the coming months.
Comment #8
gábor hojtsyWell, I was not deeply knowledgeable in this area either, therefore it helped that I went through the calls in each function one-by-one. Api.drupal.org can be used for that, you see I linked in several functions I've checked. A debugger shows one actual code path but might let you get past other code paths, which might still be important, so looking at actual code is better in this case. Anyway, keep up the work! :)