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
_batch_test_finished_helper() calls SafeMarkup::set() which is meant to be for internal use only.
Pattern is Imploding/concatenating in a loop
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
Code should be refactored for removal of SafeMarkup::set()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.The SafeMarkup::Set() already occurs inside of an automated test, so an additional one is not necessary.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: Manual testing should not be necessary since the SafeMarkup::set() occurs inside an automated test. The test(s) should fail if string is improperly escaped.
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 |
---|---|---|---|
#21 | 2550985_set_message_after.jpg | 54.96 KB | justAChris |
#21 | 2550985_set_message_before.jpg | 50.12 KB | justAChris |
#18 | sm_set_batch_test-2550985-18.patch | 3.39 KB | justAChris |
#18 | 2550985-interdiff-17-18.txt | 1.86 KB | justAChris |
#18 | 2550985-interdiff-6-18.txt | 3.59 KB | justAChris |
Comments
Comment #2
akalata CreditAttribution: akalata commentedPostponed on #2505931: Remove SafeMarkup::set in ViewListBuilder. Whatever pattern we find works there can most likely be used here as well.
Comment #3
akalata CreditAttribution: akalata commentedIf we're asserting all test markup (per #2550965-7: Remove SafeMarkup::set() in ValidationTest::assertErrorMessages()), then this can also be worked on.
Comment #4
justAChris CreditAttribution: justAChris as a volunteer commentedNot sure the same pattern as #2550965: Remove SafeMarkup::set() in ValidationTest::assertErrorMessages() can be applied here, the implode is occurring in a drupal_set_message():
It also doesn't seem like this will have the same pattern as #2505931: Remove SafeMarkup::set in ViewListBuilder. Still trying to figure out the manual test steps (if any), have't gotten any further. Will pick up again tomorrow.
Comment #5
justAChris CreditAttribution: justAChris as a volunteer commentedManual tests should not be necessary here since the SafeMarkup::set() occurs inside of an automated test. These tests should fail if strings are improperly escaped.
This is confirmed by:
drupal_set_message(SafeMarkup::set(implode('<br>', $messages)));
ProcessingTest::assertBatchMessages() does the assertions in these tests, which asserts static text(s) against the raw content (output). The raw content include the drupal_set_message() output found in _batch_test_finished_helper().
Note that the assertion text is provided by ProcessingTest::_resultMessages() as simple strings.
Comment #6
justAChris CreditAttribution: justAChris as a volunteer commentedSince the automated test sets both the message (via drupal_set_message) and the assertion text, then we should simplify both and not have to worry about escaping the line break properly.
Comment #7
dawehner@justAChris
I try to figure out why we can't wait on #2505931: Remove SafeMarkup::set in ViewListBuilder and just use that?
Comment #8
justAChris CreditAttribution: justAChris as a volunteer commented@dawehner, that is the other option. I've tested using the code in that patch and it seems to work fine. However, do we really need that overhead in this case?
We're setting up the message in one location in the test and the asserting it elsewhere. From what I can tell, the message text can contain anything as long as we're matching it with the assertion. While using #2505931: Remove SafeMarkup::set in ViewListBuilder would provide a cleaner output in the drupal_set_message() via the inline_list, does it matter if only the test bot sees it?
Comment #9
dawehnerLet's be clear we talk about a test here, and for tests we should optimize for readability and long term maintainability.
Well, under this criterias both solutions seems to be the same, but I guess using the other new feature would reduce the patch size?
Comment #11
justAChris CreditAttribution: justAChris as a volunteer commentedNot disagreeing. Thanks @dawehner for the information on tests. Either solution would work, but since I don't know why the messages were formatted the way they are, perhaps it is best leave them outputting as is and use the inline_list. Postponed on #2505931: Remove SafeMarkup::set in ViewListBuilder, which hopefully shouldn't be too long from now.
Comment #12
stefan.r CreditAttribution: stefan.r commentedWhy does this have to be a comma/
<br>
delimited list necessarily?Can't we make this a bullet point list, we already have a template for this (
item-list.html.twig
) -- then we could just do a renderPlain() in the drupal_set_message().Comment #13
justAChris CreditAttribution: justAChris as a volunteer commented@stefan.r
Nope, as you mention it should have been an item-list when the tests were created. I was trying minimize the number of lines changing in this patch, but seeming that 'br' will no longer be supported in inline-list, this is no longer postponed.
Not setting to needs review on #6 since the messages in drupal_set_message() should be in a better format using item-list.
Comment #14
stefan.r CreditAttribution: stefan.r commentedMay be worth a try to convert this into an item-list (except the first element) if we don't consider that to be out of scope...
Comment #16
xjmYep, since it's already an array of messages, it'd be quite easy to use the strategy from #2501835: Remove SafeMarkup::set in Drupal\Core\Database\Install\Tasks::runTasks() and ensure multiple messages are printed here. That seems fine to me since it's a test module.
Comment #17
justAChris CreditAttribution: justAChris as a volunteer commentedUpdate for message, removing SafeMarkup::set and utilizing item_list inside of an inline template.
Associated test assertions still need to be updated, thus marking this do not test. Will probably return to this later this evening unless somebody else wants to get at it.
Comment #18
justAChris CreditAttribution: justAChris as a volunteer commentedUpdated test assertions, this should be ready for review now.
Comment #19
Wim LeersPatch looks great. But I think it'd be better if somebody who was closely involved in #2501835: Remove SafeMarkup::set in Drupal\Core\Database\Install\Tasks::runTasks() and ensure multiple messages are printed would review/RTBC this.
Comment #20
stefan.r CreditAttribution: stefan.r commentedThis doesn't seem right even if this is only a test module.
Mixing up an inline template with an
item_list
theme call passing along items that have content that is has the markup for an item list hardcoded in HTML just seems a bit confusing, as developers may refer to core code as documentation on how to implement something themselves. Maybe we could clean that up a little bit?Also, should we do before and after screenshots of the drupal_set_message, just so we can skip the manual testing?
Comment #21
justAChris CreditAttribution: justAChris as a volunteer commentedScreenshots of the drupal_set_message in these tests before and after patch #18 since the format has changed.
Before Patch
After Patch
Note that the messages in the following function are for the test assertions:
Comment #22
stefan.r CreditAttribution: stefan.r commentedAh, didn't see those were just test assertions! This looks ready now.
Nit: needs to be indented by another space.
Comment #23
alexpottCommitted b077607 and pushed to 8.0.x. Thanks!
Fixed on commit.