Document SafeMarkup::set in AllowedTagsXssTrait::fieldFilterXss

Followed up on #2501403 to note that when we run something marked safe from Xss::filter() it should still be marked safe when all we do is put it through HTML::normalize() without combining with any unsafe input.

As a documentation-only change, not adding test. We could repeat Drupal\Component\Utility\Xss tests in core/tests/Drupal/Tests/Core/Field/ but that would be needlessly redundant.

With a patch that's not backwards this time.

Just added a \ to the namespace because that is more common in core. But otherwise it's RTBC. Thanks @mlncn

Thanks @mlncn and @joelpittet.

@mlncn's comment helps explain why this is safe:

when we run something marked safe from Xss::filter() it should still be marked safe when all we do is put it through HTML::normalize() without combining with any unsafe input.

However, the comment in the patch itself isn't quite as clear. :) Can we spell it out a little more as to why normalizing filtered HTML should also be added to the safe list? In general, I'd also like us to document not only why the SafeMarkup use is indeed safe, but why it is necessary and appropriate.

If the HTML output isn't already in the normal form, this single line of code is adding not one but two potentially lengthy entries to the SafeMarkup list: one in Xss::filter() itself, and then a variant of it that's normalized. See #2488538: Add SafeMarkup::remove() to free memory from marked strings when they're printed and #2295823: Ensure that we don't store excessive lists of safe strings for why this is potentially a concern. In this case since it's internal to filtering/sanitization APIs, we might decide that it's fine, or at least any working around it would be needless disruption or overhead. But I'd like to at least raise the question. :)

Also, a minor note: the method name should be followed by () so it gets linked properly on api.d.o and such.

Better explanation and parenthesis for the methods! Thanks @xjm

This is a place where i suppose we could safemarkup::remove() the now-obsolete string (filterXss without the HTML normalize). Perhaps we could do a follow-up for that if #2488538 lands and we want to extend that approach.

This seems clearer now and still succinct.

Thanks @mlncn, that's much clearer. This is most likely committable as it is right now, but I have an additional suggestion. Iterating on @mlncn's patch:

All known XSS vectors are filtered out by \Drupal\Component\Utility\Xss::filter(), all tags in the markup are allowed intentionally by the trait, and no danger is added in by \Drupal\Component\Utility\HTML::normalize(). Since the normalized value is essentially the same markup, designate this string as safe as well. This method is an internal part of field sanitization, so the resultant, sanitized string should be printable as is.

The intent there is to additionally make it clear that other code should not use this as a pattern, because it's a truly internal call.

And then I think maybe follow it with an @todo referencing #2488538: Add SafeMarkup::remove() to free memory from marked strings when they're printed and/or #2450993: Rendered Cache Metadata created during the main controller request gets lost suchlike is in order -- actually, I think we should file a new followup that's postponed on one or both of those.

What say ye?

I am looking at this at the NJ sprint.

I would say that nails it! Very good call @xjm on the new follow-up issue and thank you @cilefen for making it; looking forward to the follow-ups and removing the @todo :-)

Looks great, thanks!

This issue is a required part of a critical task and is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x.