Problem/Motivation

AllowedTagsXssTrait::fieldFilterXss() calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

Similar code comment as #2501403: Document SafeMarkup::set in Xss::filter to be added.

  • Remove the call by refactoring the code.
  • If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.

Remaining tasks

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
  2. Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.

Manual testing steps (for XSS and double escaping)

Not necessary, we are only adding documentation.

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#12 document-2501441-12.patch1020 bytescilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,902 pass(es). View
#8 safemarkup-set-for-filterxss-2501441-8.patch682 bytesmlncn
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,395 pass(es). View
#6 safemarkup-set-for-filterxss-2501441-5.patch523 bytesjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,164 pass(es). View
#6 interdiff.txt743 bytesjoelpittet
#5 safemarkup-set-for-filterxss-2501441-3.patch522 bytesmlncn
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,714 pass(es). View
#3 safemarkup-set-for-filterxss-2501441-3.patch522 bytesmlncn
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch safemarkup-set-for-filterxss-2501441-3.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

Cottser’s picture

Issue summary: View changes
mlncn’s picture

Assigned: Unassigned » mlncn
mlncn’s picture

Status: Active » Needs review
FileSize
522 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch safemarkup-set-for-filterxss-2501441-3.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, 3: safemarkup-set-for-filterxss-2501441-3.patch, failed testing.

mlncn’s picture

Status: Needs work » Needs review
FileSize
522 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,714 pass(es). View

With a patch that's not backwards this time.

joelpittet’s picture

Assigned: mlncn » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
743 bytes
523 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,164 pass(es). View

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

xjm’s picture

Status: Reviewed & tested by the community » Needs work

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.

mlncn’s picture

Status: Needs work » Needs review
FileSize
682 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,395 pass(es). View

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.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

This seems clearer now and still succinct.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs followup

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?

cilefen’s picture

I am looking at this at the NJ sprint.

cilefen’s picture

mlncn’s picture

Status: Needs review » Reviewed & tested by the community

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 :-)

xjm’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • xjm committed 48d0043 on 8.0.x
    Issue #2501441 by mlncn, joelpittet, cilefen: Document SafeMarkup::set...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.