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
(name of function/class::method that calls it) 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.
Use of the functionality itself was removed in #1851414: Convert Views to use the abstracted dialog modal and is now dead code, so it can be removed.
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)
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 |
---|---|---|---|
#20 | 2501933-20.patch | 1.01 KB | YesCT |
#11 | Source_of__http___d8_dev_admin_structure_views_view_content_-_before.png | 25.59 KB | joelpittet |
#11 | Source_of__http___d8_dev_admin_structure_views_view_content.png | 23.44 KB | joelpittet |
#6 | interdiff-2501933-2-6.txt | 566 bytes | ashutoshsngh |
#6 | remove_safemarkup_set-2501933-6.patch | 643 bytes | ashutoshsngh |
Comments
Comment #1
leslieg CreditAttribution: leslieg as a volunteer commentedWorking on removing SafeMarkup::set in ViewsUI::getDefaultAJAXMessage() as part of NHDevDays2
Comment #2
leslieg CreditAttribution: leslieg as a volunteer commentedchanged SafeMarkup::set to SafeMarkup::format
Comment #3
star-szrWe lost the t()! Need to get t() back :)
Comment #4
leslieg CreditAttribution: leslieg as a volunteer commentedFixed issue with missing t that @cottser found
Comment #6
ashutoshsngh CreditAttribution: ashutoshsngh at Srijan | A Material+ Company commentedComment #7
star-szr@leslieg - Just need to move the
"
one character to the left here.@ashutoshsngh thanks for the patch. Normally when someone is working on an issue we give them a bit of time to respond to reviewers, and it's nice to say in your comment what you are changing. Although the interdiff is helpful there!
I think I like the approach in #4 better but could go either way.
Comment #8
star-szrComment #9
ashutoshsngh CreditAttribution: ashutoshsngh at Srijan | A Material+ Company commented@Cottser will keep in mind your suggestion from next time.I worked because #4 failed testing.
Comment #10
joelpittetThis does the trick, thank you.
Comment #11
joelpittetSome screenshots:
Before:
After:
Comment #12
xjmThanks folks for taking this on.
So in general this is a pattern I think we should discourage as much as possible. We're essentially calling
SafeMarkup::format()
twice on the same string (and therefore doing the entire sanitization twice and using the internalSafeMarkup::set()
twice, adding variants of the same string to the SafeMarkup list).It would be better to either (a) put the markup within one
t()
call if translators are the audience that might alter the markup, or (b) use a template, inline template, or render array if translators should not have to deal with the markup and/or themers are the ones who should control it. In this case, given that it's a div with a specific class and referencing the guidelines for HTML in translatable strings, I think the second option is correct. I think an inline template would be the minimum fix here if we can make it handle a render array, and refactoring for actual themeable output would be better still. Let's look at the usage of this method in the Views UI and see what we can do.Comment #13
joelpitteta) I'd rather this than put the markup in
t()
. Translators have a enough to worry about let alone breaking markup. I'd take this patch 100 times more than putting the markup in thet()
.b) Inline template feels a bit like an overkill from DX considering we don't need any template logic.
Comment #14
xjmBut why should Views a div and class that themers can't control? We're essentially just hacking around Views doing a not-best-practice thing.
After all: https://api.drupal.org/api/drupal/8/search/status-messages.html.twig
Comment #15
xjmOh, I should have mentioned that I talked to @alexpott about this issue briefly this morning and he seemed to agree (though we didn't dig into what Views was doing at the time).
But my perspective is that part of the original "contract" of the SafeMarkup issue is that we would remove the technical debt by doing less concatenating of hardcoded HTML in PHP and convert more of it to the render and theme system. So this is a case to me where that at least seems possible. If we decide it's too much work and disruption to get the critical done, I'd be up for filing a postponed followup, but I'd like to have at least tried it and know what the change would entail in the Views UI.
Comment #16
xjm(Adding @joelpittet and @Cottser to the suggested commit message for their review and feedback here.)
Comment #17
dawehnerThe best solution for this issue is to remove that function and its single usage.
The only usage of that has been in javascript, though, got removed as part of #1851414: Convert Views to use the abstracted dialog modal long time ago,
see
Comment #18
xjm@dawehner, that sounds even better yet.
Comment #19
xjmTechnically we should consider whether we just deprecate it, but this might be a scenario where removing it is better if it's already essentially mostly dead code and you'd only use it if, what, you were overriding the Views UI? GLHF. :)
Comment #20
YesCT CreditAttribution: YesCT commentedok. I see in #1851414: Convert Views to use the abstracted dialog modal
- Drupal.AjaxCommands.prototype.viewsSetForm({}, {'title': '', 'output': drupalSettings.views.ajax.defaultForm});
So I removed the function, and also the place where drupalSettings.views.ajax.defaultForm was set.
Comment #21
dawehnerThis is why we need to declare what we define as API and what not.
IMHO this is certainly not API
The fix for itself looks perfect
Comment #22
xjmExcellent! I confirmed that these were the only uses of that AJAX element and method.
Comment #23
xjmSince this code is not used, it made me wonder how this
SafeMarkup
set got added in the first place, since it doesn't appear to have test coverage that would have failed and there was no manually testable. It got added in the original issue in #1825952-99: Turn on twig autoescape by default by chx with no specific explanation of why, but it was following a large merge/change.I don't think this needs a change record.
This issue helps solve a required part of a critical and reduces fragility by removing dead code, , so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x.
Comment #25
xjmOh, I also checked for whether we could remove the
use
statement, but it is used other times in the file.Comment #26
olli CreditAttribution: olli commentedFiled #2505965: Remove dead code related to old views ui modal to remove also views-ajax-body, -title and -popup.