Aggregator xss fields should be using Field/Entity formatters

Its a bit odd, ideally we should be able to just drop that file and all its uses, but for some reason aggregator has its own specialized xss filter.

function aggregator_filter_xss($value) {
  return Xss::filter($value, preg_split('/\s+|<|>/', \Drupal::config('aggregator.settings')->get('items.allowed_html'), -1, PREG_SPLIT_NO_EMPTY));
}

Does anyone know why?

this function is ancient and will be there forever unless this issue is fixed

Thanks a ton @ParisLiakos for your feedback!

Yeah that thing is weird. Would have been nice to get rid of it, but not sure we should still try.

We're talking about field access and translatability of feed and feed items. At least by default in core, those two entity types are not translatable and have no data table. And field access on feed/item titles? I challenge you do provide a use case where that would make sense ;)

Should this *really* be a critical release blocker? I'm worried that we use this meta issue to push through all kinds of changes that would otherwise no longer be possible, resulting in more API and database/config changes. 8.0.0 will not be perfect, we have to accept that or it will never happen.

Well that is the problem, everyone can theoretically swap out the implementation and boom we have a security problem, unless views uses the proper mechanisms,
so I think we have to do it.

Well, entity field access is technically possible, my points are...

a) it's the entity label (Drupal's UI's usually fall apart completely if you can't see node titles, taxonomy terms or feed titles but have access to the entity)
b) it's feeds and feed items, I see no practical use of field level access, it's for 99% of the cases aggregation of content that is publicly availalbe elsewhere.
c) The default UI for aggregator.module is still custom code and not provided by views. And that custom code doesn't care about entity field access in any way. So why would supporting it in the views integration be a critical release-blocker?

I'm not against fixing it. And I've been trying to fix #2149845: Convert the description field of the 'aggregator_item' entity to a text field a long, long time ago. But it didn't happen, and IMHO, we have to live with that now. This is similar to the user signature case, it's custom data/formatter handling, we would have to change storage to support this, which be one more thing that breaks existing D8 sites (I do have those, although I'm not using aggregator). Which is fine if it's worth it, my point is that IMHO, in this case, it is not.

I guess we could convert the custom views field plugins to custom field formatters, that just apply to those specific entity types/fields, but not sure what we gain with that exactly.

Anyway, what I care about is getting D8 out asap, and with as few API changes as possible (and as many as required). That said, I'm not actually using aggregator.module at the moment, so I don't care too much about this specific issue. I tried to make my point, won't continue to argue.

c) The default UI for aggregator.module is still custom code and not provided by views. And that custom code doesn't care about entity field access in any way. So why would supporting it in the views integration be a critical release-blocker?

That is a strong point.

Also neither aggregator feed nor aggregator item is translatable, so the other part of the meta doesn't apply here.
I think for this particular field we can indeed move the issue to be "just" major. Does someone strongly disagree?

So to this point:

it's feeds and feed items, I see no practical use of field level access, it's for 99% of the cases aggregation of content that is publicly available elsewhere

I've used Feeds in Drupal 6 to import data from a feed on a restricted local network onto a site, which then displayed some of the content on a publicly-accessible intranet, but with restricted access on some fields. If I built a view of that content and my access rules were ignored when they were followed everywhere else, wouldn't that be an information disclosure that merited a private security issue? Obviously my particular scenario wouldn't happen since those were configurable fields, but isn't something similar possible?

It's obviously not critical the way that (say) the user email field was, but at the least, I think we need a big caveat in the Views UI when you add those fields. I'd like a second opinion from a sec team member, but I think if we had that, we could indeed downgrade this or even wontfix it if so desired.

Well, won't fix is certainly the wrong thing, given that it adds value to convert the beside of "just" security.

@dawehner clarified in IRC that the edgecase disclosures I describe in #15 would also happen in HEAD with aggregator itself currently, which didn't quite click before. So agreed with this being major.

- just wanted to remove the hashtag from the issue tag ... yannickoo was faster -

Yeah, without #2149845: Convert the description field of the 'aggregator_item' entity to a text field (which would have added a text format in aggregator.module that it would be using), we can't use the default formatters and need a custom one.

Is there any test coverage of this?

I'm not sure, why the test did not fail. I now removed all references to aggregator_xss also from the config.

Test coverage is okay I think, the fields are rendered as before.

oops

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 5f987ee and pushed to 8.0.x. Thanks!