Views Aggregrator plugin blocking html tags allowd in Aggregator module [#713078]

This is in reference to http://drupal.org/node/369867 Images are being blocked in Views because the aggregator plugin is running them through filter_xss. The views_handler_field_xss class, defined in views/handlers/views_handler_field.inc does not check to see if Aggregator is allowing HTML tags in the content of feed items as defined in /admin/content/aggregator/settings

The attached patch checks to see if the variable aggregator_allowed_html_tags has been set . If not, it can accept a set of tags at the time the $views_handler_field_xss->render() function is called. By default, it will stip all html tags when calling filter_xss()

Comment	File	Size	Author
#12	713078-aggregator_xss.patch	2.59 KB	dawehner
#12
#9	713078-aggregator_xss.patch	2.59 KB	dawehner
#9
	views_handler_field.patch	639 bytes	achaux

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

merlinofchaos CreditAttribution: merlinofchaos commented 12 February 2010 at 23:42

Status:

Active

» Needs review

Setting proper status. Hopeful we can get a review on this, the explanation makes a lot of sense.

Comment #2

achaux CreditAttribution: achaux commented 15 February 2010 at 19:33

I was searching through some older issues and came across this: http://drupal.org/node/627402 It is definitely related, but it seem that the patch did not fix the problem of displaying allowed HTML entities from the RSS feeds. I just added this for historical context, does not necessarily change the status of this bug report.

Comment #3

dawehner

German

CreditAttribution: dawehner commented 15 February 2010 at 21:44

Status:

Needs review

» Needs work

+    return filter_xss($value, preg_split('/\s+|<|>/', variable_get('aggregator_allowed_html_tags', $allowed_html_tags), -1, PREG_SPLIT_NO_EMPTY));

For me this makes absolute no sense for me, because this is filter_xss handler and not a aggregator specific handler.

Comment #4

jfable CreditAttribution: jfable commented 16 February 2010 at 15:24

Status:

Needs work

» Needs review

For me it makes perfect sense. I would make the assumption that an Aggregator View would utilize the same allowed values tags as set in /admin/content/aggregator/settings. The fact that it doesn't is counter-intuitive.

Currently there is no way to pass allowed tags to the filter_xss function in views/aggregator. I would call that a bug. This seems to be a very reasonable fix to me.

Comment #5

achaux CreditAttribution: achaux commented 16 February 2010 at 15:40

Correct, filter_xss is not an aggregator specific handler. The patch does not change the filter_xss handler, only what is passed to the filter_xss handler.

Comment #6

dawehner

German

CreditAttribution: dawehner commented 22 February 2010 at 17:01

Status:

Needs review

» Needs work

But still

1) + function render($values, $allowed_html_tags = FALSE) {
I don't think that the signature of the render function should be changed.
2) I don't think that we should use the settings of aggregator+ function render($values, $allowed_html_tags = FALSE) {
for every filter_xss thing. You could better make a new handler for it.

Thx for finding this bug.

Comment #7

achaux CreditAttribution: achaux commented 23 February 2010 at 17:54

Thanks for the reply dereine. I want to keep this thread going because I think Views should respect the settings set/defined by Aggregator, especially since Views has an "Aggregator item" view type. Since the only way to generate a view of RSS/Atom feeds through Views GUI is by using the Aggregator item view type, I think that it is very important. I'll look into adding a new Aggregator handler. Any suggestions are welcome.

Comment #8

merlinofchaos CreditAttribution: merlinofchaos commented 23 February 2010 at 18:02

Ok, the answer is that we need an aggregator specific handler that has its own render that does essentially what this patch does. I agree that the generic _xss should not do this, because that would force aggregator settings onto non-aggregator uses. However, I do agree, that aggregator specific output should.

Summary:

A new handler class, views_handler_field_aggregator_xss extends views_handler_field
Handlers in aggregator.views.inc currently using field_xss should be evaluated to see if they should use the generic xss or the aggregator specific (only data from an external source should use the aggregator specific one)

A clean patch that does the above seems very likely to be accepted.

Comment #9

dawehner

German

CreditAttribution: dawehner commented 23 June 2010 at 22:25

Status:

Needs work

» Needs review

File	Size
713078-aggregator_xss.patch	2.59 KB

Here is a general patch.

Comment #10

merlinofchaos CreditAttribution: merlinofchaos commented 14 October 2010 at 20:14

This looks good. Patch does not apply. Reroll and mark RTBC?

Comment #11

dawehner

German

CreditAttribution: dawehner commented 14 October 2010 at 21:06

Assigned:

Unassigned

» dawehner

Comment #12

dawehner

German

CreditAttribution: dawehner commented 16 October 2010 at 13:54

Status:

Needs review

» Reviewed & tested by the community

File	Size
713078-aggregator_xss.patch	2.59 KB

I love cvs/git *arg*
After just 1,5hours of work i have a working patch file.

// $Id: views_handler_field_aggregator_item_description.inc,v 1.1.2.1 2010-01-05 01:16:58 merlinofchaos Exp $
// $Id: views_handler_field_aggregator_item_description.inc,v 1.1.2.1 2010/01/05 01:16:58 merlinofchaos Exp $

The second line is the current cvs, the previous is old version of cvs/git checkout of the cvs. WTH