Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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
Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123- 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.
- 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
Comment | File | Size | Author |
---|---|---|---|
#12 | document-2501441-12.patch | 1020 bytes | cilefen |
#8 | safemarkup-set-for-filterxss-2501441-8.patch | 682 bytes | mlncn |
#6 | safemarkup-set-for-filterxss-2501441-5.patch | 523 bytes | joelpittet |
#6 | interdiff.txt | 743 bytes | joelpittet |
#5 | safemarkup-set-for-filterxss-2501441-3.patch | 522 bytes | mlncn |
Comments
Comment #1
star-szrComment #2
mlncn CreditAttribution: mlncn at Agaric commentedComment #3
mlncn CreditAttribution: mlncn at Agaric commentedFollowed 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.
Comment #5
mlncn CreditAttribution: mlncn at Agaric commentedWith a patch that's not backwards this time.
Comment #6
joelpittetJust added a \ to the namespace because that is more common in core. But otherwise it's RTBC. Thanks @mlncn
Comment #7
xjmThanks @mlncn and @joelpittet.
@mlncn's comment helps explain why this is safe:
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.Comment #8
mlncn CreditAttribution: mlncn at Agaric commentedBetter 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.
Comment #9
joelpittetThis seems clearer now and still succinct.
Comment #10
xjmThanks @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:
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?
Comment #11
cilefen CreditAttribution: cilefen commentedI am looking at this at the NJ sprint.
Comment #12
cilefen CreditAttribution: cilefen commentedComment #13
mlncn CreditAttribution: mlncn at Agaric commentedI 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 :-)
Comment #14
xjmLooks 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.