Views feed doesn't encode embedded HTML anymore

I've added some very basic tests to ensure this doesn't regress again.

For now, I'm checkPlain'ing the <description> element. As far as I can tell, that's the only element that's supposed to have encoded HTML in it.

I have also tested this patch #2. its working fine.

• We should document, why we need it
• We need to change also the comment subclass of \Drupal\views\Plugin\views\row\RssPluginBase
• Given that the views render caching changes these lines and moves the drupal_render_call indirectly to the rss template, does that mean we need to manually escape it there?

Addressing issues from both #6 and #7:

We should add a very explicit comment why we have to do this. Because it must be encoded as it is being inlined into an XML document and shouldn't be treated as part of the structure.

Good call -- I wouldn't know why I made that change if I came across this two months from now! :)

Done.

We need to change also the comment subclass of \Drupal\views\Plugin\views\row\RssPluginBase

We also need to update the comment and aggregator (I think?) rss plugins in the same way

According to this, there are three classes that subclass RssPluginBase:

• core/modules/aggregator/src/Plugin/views/row/Rss.php
• core/modules/node/src/Plugin/views/row/Rss.php
• core/modules/comment/src/Plugin/views/row/Rss.php

The second was covered by the patch in #2. The attached patch takes care of the first and third.

Given that the views render caching changes these lines and moves the drupal_render_call indirectly to the rss template, does that mean we need to manually escape it there?

I don't understand this question... If you're asking "could we do this is the Twig template instead of in preprocess or in the plugin->render() function," then the answer is yes. But it felt cleaner to do it earlier in the render pipeline to provide fewer places for someone to accidentally undo the escaping. Also keeps the Twig template cleaner as we don't need to escape there (see core/modules/views/templates/views-view-row-rss.html.twig)

If not, sorry, I don't understand the question. :/

Finally, I don't have any tests in place for the newly added Comment and Aggregator module changes. The only thing in the current test suite for the Aggregator module that hits this code is in the pager test (testFeedPage). Comment module has RowRssTest, which at least is testing content, but only the publication date.

Is content verification something that needs to be added in the scope of this issue? Or can we push that into a followup?

And the interdiff.... Ooops.

And setting status.

It's Monday...

Updated issue summary.

The patch in #8 was broken by the changes introduced in #2381277: Make Views use render caching and remove Views' own "output caching". So now I'm taking a different approach since the description field is now a render array -- using a #post_render to call SafeMarkup::checkPlain.

This will not affect the newly added caching from the above issue. From the API docs, "if this element has #cache defined, the rendered output of this element is saved to Renderer::render()'s internal cache. This includes the changes made by #post_render." Woo-hoo! :)

Note: The interdiff is after rerolling #8 against the latest HEAD.

Added Beta eval.

+++ b/core/modules/aggregator/src/Plugin/views/row/Rss.php
@@ -50,8 +51,17 @@ public function render($row) {
+        // The description is the only place where we should find HTML.
+        // Escape it so that it appears as text rather than part of the DOM.
+        // @see https://validator.w3.org/feed/docs/rss2.html#hrelementsOfLtitemgt
+        $item->description = SafeMarkup::checkPlain($field->value);

Maybe a totally stupid question ...
What is the reason that it is not escaped as part of the template?

What is the reason that it is not escaped as part of the template?

No reason other than I figured it was better to do it earlier in the render pipeline. It does make for fewer code changes to do it in the template (two vs. four).

Thoughts?

@Berdir: I'm still seeing the original bug in the latest HEAD (ba5a3d2e178f1, which seems to be the same as beta13) and the test from #2 still fails unless the patch is applied. Post-patch, I get this:

Though this is killing me as well -- I just spent 10 minutes looking for escaped HTML instead of UN-escaped HTML while doing git bisect! :)

Goofed the image upload...

The contents of description don't always get sanitized.
For example with this setting:

Format: RSS Feed | Settings
Show: Content | Full content

Wouldn't it be better to sanitize the contents of description in the template preprocessor functions? That's how it's done for the feed's description field, I think it should be followed for feed rows too.

^ This is when applied on beta12.
Haven't tried beta13 yet.

mod: The bug still exists in 8.0.0-beta13

Since #12 patch relies heavily on SafeMarkup::checkPlain() I thought this might be relevant?

Issue #: Deprecate SafeMarkup::checkPlain() for Drupal 8.0.x

In #2545972: Remove all usages SafeMarkup::checkPlain() and rely more on Twig autoescaping we remove all usages in core. We should rely on Twig auto escape and completely remove SafeMarkup::checkPlain(). SafeMarkup::checkPlain() is dangerous because if the resulting string is modified in any way, for example nl2br(), the result will no longer be marked safe. In places where explicit escaping is needed, Html::escape() should be used.

This needs work based on #25.

@othermachines: Thanks for pointing that out. This issue had fallen off my radar...

Rerolling.

For now, pretty much swap of checkPlain() for Html::escape().

Is this still an issue now that #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping is in? If so, patch needs rerolling.

Verified this is still an issue, rerolling now.

Reroll of #28.

+++ b/core/modules/aggregator/src/Plugin/views/row/Rss.php
@@ -50,8 +51,17 @@ public function render($row) {
+        // Otherwise Twig handles it via auto-escape.
+        // @see core/modules/views/templates/views-view-row-rss.html.twig
+        $item->{$name} = $field->value;
+      }

Its a little bit odd to rely on autoescaping of HTML entities inside a xml document. Are we sure this is the right strategy?

I reread this issue and talked to @joelpittet at the PNWDS this weekend and we both agree that the current approach in this issue is wrong. Instead, we should be building render arrays in the various RSS plugins and rendering those as strings during preprocess so that Twig will autoescape them when passing them to the template.

I think the reason this issue appeared and disappeared over the months is that we're not clear when we are doing the escaping. Doing it as late as possible -- in the preprocess -- will hopefully clear things up.

This seems to be the correct approach as it ensures twig escaping will encode the the HTML every time and deals with the render arrays being passed along.

Thanks for tagging this for rc target triage! To have committers consider it for inclusion in RC, we should add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section <h3>Why this should be an RC target</h3> to the summary.

Updated issue summary. Also added RC target section as requested in #38.

Discussed with the committers and we agreed this was an rc target. The approach looks correct reading the link in the code and things like http://bit.ly/1GC66PE.

Given

Disruptions for this change are minimal. Contrib modules that provide RSS display or row plugins will need to ensure they are passing a render array for the description element. Core modules that provide those plugins (Comment and Aggregator) are updated with this patch.

I think we need a change record.

Working on the CR.

Draft CR: https://www.drupal.org/node/2601028

+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_display_feed.yml
@@ -90,10 +164,17 @@ display:
+      cacheable: false

@@ -115,14 +196,29 @@ display:
+      cacheable: false
...
+      cacheable: false

These are deprecated, we should not add them.

@Wim Leers, Thanks for the review. I've removed the cacheable entries and round-tripped the view through the latest D8 head to get the latest bits and pieces in the export.

Committed 28f5b70 and pushed to 8.0.x. Thanks!

I was testing #2552037-16: Aggregator RSS feed outputting empty feed items last night and noticed that the description elements weren't appearing in Aggregator's RSS feed. I backtracked the issue to here. This issue did not really update Aggregator's plugin, contrary to what's mentioned in the issue summary. Its plugin is not yet providing descriptions as render arrays. This makes me wonder whether Comment's RSS feed is having the same problem, but I haven't checked.

I'm not sure it's worth re-opening this issue to fix Aggregator. You can't even see the problem until #2552037: Aggregator RSS feed outputting empty feed items is fixed because the whole RSS feed is currently broken. A proper test can't be written for the other issue until the description is fixed, nor could a test be written for the description until the whole feed is fixed. Due to the catch-22, I'm going to propose that the description render array be added as part of the other issue, even though they really ought to be separate issues. Otherwise, there's just no way to test either of them.

Team,

This issue again happen latest drupal 8 versin. So I will reopen this issue.

Thanks,
Jabastin

https://www.drupal.org/project/drupal/issues/3001874#comment-13595732

Please open a new issue.