RSS Style plugin escapes feed title twice

Where is the first check plain?

For example the display_block runs a filter_xss_admin as well.

Looks fine from my perspective.

Assign to merlinofchaos for a last review.

Default title is not escaped with last patch, which breaks whole feed in the worst case.

I'm seeing this now even in node titles when pulled out in a page or block view (6.x-2.12) - and it shows fine elsewhere (so Fall '05 is shown as Fall '05)

If you look at the node itself (as a page view) it's fine - only a problem when the title is displayed in the view.

Jason

I also have this problem, where I am using a taxonomy term name converted to id.
The view produces an RSS feed.

The term name has an ampersand in it, so the ampersand is in the url.
I then take the taxonomy name url argument and use it as part of the page title.

This is where the check_plain() in #3 causes a problem.
The problem does happen on the actual page (RSS feed).
The title should say Faculty & Staff Feed and not Faculty & Staff Feed

The above patch does solve the problem, but given what #5 says, I am hesitant to use the patch.

Marking as duplicate of this:
#1955072: Node/feed item titles are double escaped

This patch is fixing the double-escaping problem for me. The patch un-escapes the title before the generation of the RSS file, during which everything gets escaped. Basically, with this patch, it undoes an escaping that is not needed.

However, it seems to me that it would be better to not do this unnecessary escaping at all. Look at the patch. If I dsm($this); above the changed line, the title that I see is not escaped. If I dsm($item->title); below the changed line, it is escaped. So, $this->get_field() must be doing the escaping. I have not been able to find where this happens. Any insight would be appreciated.

It seems to be, as you wrote before, that we first figure out what causes the encoding.

I have been working through the code tracing back where the escaping happens. In views_handler_field.inc, there is a theme() method of views_handler_field object. theme() just calls the regular Drupal theme() function with particular parameters. If I add dsm() calls in the theme() method, I can see that the double escaping is happening inside the Drupal theme() function. I have not been able to trace it back further. It is very hard to usefully put dsm() calls inside theme() because theme() is called all over the place and a huge amount of content gets spit out.

The code which escapes the title seem to be part of template_preprocess_views_view_row_rss ?

That's it. This patch makes the needed change.

Does this solution look good to you?

We could all sleep better if there would be a test, which ensures that the output is as expected (so it's escaped once, and really just once). At least for Drupal 8 this is required.

In trying to write test I have discovered that this is not the problem. The title is getting escaped somewhere else, but I don't know where.

This might be caused by #2011918: Titles are often double-escaped (including in the content attribute of the dc:title meta element for nodes).

I am no longer sure of what I wrote in #16. I can't get tests to work because, on two different machines, when I run the views tests as they come, I get an "Base table or view not found" error. Are you able to run tests without errors?

Steps to reproduce the problem on a fresh D7 install:

1. Publish an article with title "Liam's Test title" and body "Liam's Test body".
2. Enable Views UI.
3. Enable "Front page" view.
4. Edit the Feed display of that view.
5. Under Format, Show, click on Content, set it to Fields.
6. Click cancel on "Feed: Row style options" page.
7. Under Fields, click Add.
8. Add "Content: Body" and "Content: Title".
9. For each, uncheck "Create a label" and "Link this field to the original piece of content".
10. Under Format, Show, click on Settings.
11. Assign each field. For some use "Content: Body" and others "Content: Title".
12. Look in the preview. Observe that instances of "Liam's Test title" will be double-escaped while instances of "Liam's Test body" are not.

While my patch in #13 seemed to solve my problem, it doesn't make sense. Everything else is being escaped at that point in the code, so it seems to me that the title should be too. The title must be getting escaped before it gets to the RSS generation code. It is that previous escaping that should be removed.

When the View Format is set to "Show: Content", check_plain() needs to be run on $item->title in template_preprocess_views_view_row_rss() because it is not already escaped.

When the View Format is set to "Show: Fields", "Content: Title" has already been escaped and "Content: Body" has not been escaped. Why are these different?

Thank you for the fix. It works for me.

Drupal 7.42 / Wetkit 4.5

I created a view of content, tagged with a Vocabulary Term.
The view is drawing the feed Title from a contextual filter for the tid.

This resulted in the same double encoding issue found in the function template_preprocess_views_view_row_rss()

Can somebody confirm the issue and recreate the views_1189550_escape_rss_title.patch found in #10?

So... This is what I did to resolve it on row 885 in \theme\theme.inc.

function template_preprocess_views_view_rss(&$vars) {
  global $base_url;
  global $language;

  $view     = &$vars['view'];
  $options  = &$vars['options'];
  $items    = &$vars['rows'];

  $style    = &$view->style_plugin;

  // The RSS 2.0 "spec" doesn't indicate HTML can be used in the description.
  // We strip all HTML tags, but need to prevent double encoding from properly
  // escaped source data (such as &amp becoming &).
  $vars['description'] = check_plain(decode_entities(strip_tags($style->get_description())));

  if ($view->display_handler->get_option('sitename_title')) {
    $title = variable_get('site_name', 'Drupal');
    if ($slogan = variable_get('site_slogan', '')) {
      $title .= ' - ' . $slogan;
    }
  }
  else {
    $title = $view->get_title();
  }
-  $vars['title'] = check_plain($title);
+  $vars['title'] = $title;
  // Figure out which display which has a path we're using for this feed. If there isn't
  // one, use the global $base_url

Adding attribution.

Does this work both when View Format is set to "Show: Content" and when it is set to "Show: Fields"?

Yes...
In my environment this seems to work if I choose

- Format / Show / Fields
or
- Format / Show / Content

Just ran into this bug. No negative impacts found with this patch.

The patch is the solution to the problem we've encountered. Thanks!

Marked this as reviewed too.

As talked with drupol on IRC we need to ensure that we escape at least once. This should be at least documented in the issue summary or maybe better also as part of the patch.
While a double escaping is bad, having no escaping by accident at all is orders of magnitude worse, of course.

Hi,

Here's the backtrace of the display of a feed using the Field row plugin.

1. views_plugin_display_feed->render(): calls $this->view->style_plugin->render($this->view->result);
2. views_plugin_style_rss->render(): calls $rows .= $this->row_plugin->render($row);
3. views_plugin_row_rss_fields->render(): calls $item->title = $this->get_field($row_index, $this->options['title_field']);
4. views_plugin_row_rss_fields->get_field(): calls return $this->view->style_plugin->get_field($index, $field_id);
5. views_plugin_style_rss->get_field(): calls views_plugin_style->get_field()
6. views_plugin_style->get_field(): calls $this->render_fields($this->view->result);
7. views_plugin_style->render_fields(): calls $this->rendered_fields[$count][$id] = $this->view->field[$id]->theme($row);
8. views_handler_field_node->theme(): calls views_handler_field->theme()
9. template_preprocess_views_view_field(): calls $vars['output'] = $vars['field']->advanced_render($vars['row']);
10. $vars['field']->advanced_render($vars['row']): calls views_handler_field_node->advanced_render()
11. views_handler_field_node->advanced_render(): calls views_handler_field->advancer_render()
12. views_handler_field->advancer_render(): calls $value = $this->render($values);
13. $this->render(): calls views_handler_field_node->render()
14. views_handler_field_node->render(): calls return $this->render_link($this->sanitize_value($value), $values);
15. $this->sanitize_value($value): calls views_handler->sanitize_values()
16. views_handler->sanitize_values(): calls $value = check_plain($value);

This is where the first check_plain() is done, so, there is no need to have a second one.

This explains maybe the case changed in template_preprocess_views_view_row_rss , but does it also for template_preprocess_views_view_rss ?

Hi,

Here's the backtrace of the display of content using RSS Feed style and Content row plugin:

1. views_plugin_display_feed->render(): calls $this->view->style_plugin->render($this->view->result);
2. views_plugin_style_rss->render(): calls $rows .= $this->row_plugin->render($row);
3. views_plugin_row_node_rss->render(): calls return theme($this->theme_functions());
4. return theme($this->theme_functions());: calls template_preprocess_views_view_row_rss()
5. template_preprocess_views_view_row_rss(): calls $vars['title'] = check_plain($item->title);

This is where the first check_plain() is done, and here it make sense to keep it.

Here's an updated patch. I moved the check_plain from the template_preprocess_views_view_row_rss() to method views_plugin_row_node_rss::render(), this way there are no more double check_plain() calls.

Here's an updated patch. I moved the check_plain from the template_preprocess_views_view_row_rss() to method views_plugin_row_node_rss::render(), this way there are no more double check_plain() calls.

Does that mean that every row plugin would have to be adapted? This sounds like a no go to be honest. We really don't want to introduce any chance of an XSS.
Here is an alternative idea:

• Introduce \Drupal\Component\Render\MarkupInterface from D8
• Implement that once for views, see \Drupal\views\Render\ViewsRenderPipelineMarkup
• Use this object in the places which call check_plain()
• Check in the preprocess methods, whether the title implements that interface. In case it doesn't, call check_plain() on it.

Hi @dawehner,

Something like this ?

Updated patch.

Sorry,

I've included the staged files in the patch.

Updated patch, makes more sense now.

Updated patch, makes more sense now.

+++ b/plugins/views_plugin_row_rss_fields.inc
@@ -122,25 +122,28 @@ class views_plugin_row_rss_fields extends views_plugin_row {
+    $item->link = view_markup::create(url($this->get_field($row_index, $this->options['link_field']), array('absolute' => TRUE)));

+++ b/theme/theme.inc
@@ -927,9 +927,9 @@ function template_preprocess_views_view_row_rss(&$vars) {
+  $vars['link'] = ($item->link instanceof MarkupInterface) ? $item->link : check_url($item->link);

URLs are special, maybe we should choose its own object for them?

I agree, but should it be taken in account in this issue ? It's a bit out of scope no ?

Feel free to skip the changes for the link. Yeah ideally just maybe fix the title double escaping here?

Yes that's the idea.

[took the liberty of fixing the issue description to actually show the double escaped title]

Rerolling patch against latest (7.x-3.29).

Helps to upload the correct file.
Rerolling patch against latest (7.x-3.29).