Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Xss::filter() calls SafeMarkup::set() which is meant to be for internal use only.
Proposed resolution
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 changing documentation.
Do these steps both with HEAD and with the patch applied:
- Clean install of Drupal 8.
- Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
- If there is any user or calling code input in the string, submit
alert('XSS');and ensure that it is sanitized.
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#10 | interdiff.txt | 1.13 KB | star-szr |
#10 | document-2501403-9.patch | 875 bytes | star-szr |
Comments
Comment #1
star-szrWorking on this at NHDevDays 2.
Comment #2
star-szrComment #4
star-szrReal time feedback from @cwells, new patch.
Comment #5
dawehnerSure, this is fair
Comment #6
xjmSo, technically, if someone passes
<script>
in the$html_tags
when they call this, then the XSS has been filtered. I think this is actually more like #2273925-256: Ensure #markup is XSS escaped in Renderer::doRender(). Also, I think we should emphasize why callingSafeMarkup::set()
is correct rather than just acceptable.Also, this is minor, but the regex is still doing some of the sanitization, so "by now" is slightly misleading I think.
So something like:
Or something along those lines. Try to improve it from that probably. :)
Comment #7
xjmOr something like:
Comment #8
star-szrThanks @xjm, I added some more to your suggestion in #7.
Comment #10
star-szrRunning out of brain steam ;)
Comment #11
pwolanin CreditAttribution: pwolanin at Acquia commentedLooks ok to me, though we should make it harder for people to allow stupid things like SCRIPT, IFRAME, or OBJECT in the UI
Comment #12
joelpittetPerfect, very thorough.
@pwolanin Maybe you could make a follow up but I believe we should let people shoot themselves in the foot sometimes and not mollycoddle them.
Comment #13
xjmGreat, thanks @joelpittet and @pwolanin!
The use is internal to Drupal's sanitization APIs, and we've had a member of the sec team check over this patch, so I think this is good as one of the "Document" issues. We could possibly use a followup about Xss more generally for #11.
This issue only changes documentation, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. It's also part of resolving a critical issue. Committed and pushed to 8.0.x.
Comment #15
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedSee #275811: Warn about potentially insecure filter configurations