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

  1. 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()
  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. The SafeMarkup::Set() already occurs inside of an automated test, so an additional one is not necessary.
  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)

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.

  1. Clean install of Drupal 8.
  2. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
  3. 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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

justAChris created an issue. See original summary.

akalata’s picture

Status: Active » Postponed

Postponed on #2505931: Remove SafeMarkup::set in ViewListBuilder. Whatever pattern we find works there can most likely be used here as well.

akalata’s picture

Status: Postponed » Active

If we're asserting all test markup (per #2550965-7: Remove SafeMarkup::set() in ValidationTest::assertErrorMessages()), then this can also be worked on.

justAChris’s picture

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

if (!$success) {
    // A fatal error occurred during the processing.
    $error_operation = reset($operations);
    $messages[] = t('An error occurred while processing @op with arguments:<br />@args', array('@op' => $error_operation[0], '@args' => print_r($error_operation[1], TRUE)));
  }

  drupal_set_message(SafeMarkup::set(implode('<br>', $messages)));

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.

justAChris’s picture

Title: Remove or document SafeMarkup::set in _batch_test_finished_helper() » Remove SafeMarkup::set in _batch_test_finished_helper()
Issue summary: View changes

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

  • Removing the SafeMarkup::set() from the following line without replacement, the line break should no longer be escaped: drupal_set_message(SafeMarkup::set(implode('<br>', $messages)));
  • Running Tests: \Drupal\system\Tests\Batch\ProcessingTest

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.

justAChris’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
3.04 KB

Since 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.

dawehner’s picture

@justAChris
I try to figure out why we can't wait on #2505931: Remove SafeMarkup::set in ViewListBuilder and just use that?

justAChris’s picture

@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?

dawehner’s picture

@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?

Let'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?

Status: Needs review » Needs work

The last submitted patch, 6: sm_set_batch_test-2550985-6.patch, failed testing.

justAChris’s picture

Status: Needs work » Postponed

Not 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.

stefan.r’s picture

Why 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().

justAChris’s picture

Status: Postponed » Needs work

@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.

stefan.r’s picture

May 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...

Status: Needs work » Needs review
xjm’s picture

Yep, 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.

justAChris’s picture

Update 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.

justAChris’s picture

Updated test assertions, this should be ready for review now.

Wim Leers’s picture

Patch 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.

stefan.r’s picture

+++ b/core/modules/system/src/Tests/Batch/ProcessingTest.php
@@ -257,28 +257,28 @@ function _resultMessages($id) {
+        $messages[] = 'results for batch 0<div class="item-list"><ul><li>none</li></ul></div>';

This 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?

justAChris’s picture

Screenshots of the drupal_set_message in these tests before and after patch #18 since the format has changed.

Before Patch
Test drupal_set_message before
After Patch
Test drupal_set_message after

Note that the messages in the following function are for the test assertions:

+++ b/core/modules/system/src/Tests/Batch/ProcessingTest.php
@@ -257,28 +257,28 @@ function _resultMessages($id) {
+        $messages[] = 'results for batch 0<div class="item-list"><ul><li>none</li></ul></div>';
stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

Ah, didn't see those were just test assertions! This looks ready now.

+++ b/core/modules/system/tests/modules/batch_test/batch_test.callbacks.inc
@@ -104,7 +103,20 @@ function _batch_test_finished_helper($batch_id, $success, $results, $operations)
+       '#theme' => 'item_list',
+       '#items' => $messages,

Nit: needs to be indented by another space.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b077607 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/system/tests/modules/batch_test/batch_test.callbacks.inc b/core/modules/system/tests/modules/batch_test/batch_test.callbacks.inc
index 7dd8f1f..461219f 100644
--- a/core/modules/system/tests/modules/batch_test/batch_test.callbacks.inc
+++ b/core/modules/system/tests/modules/batch_test/batch_test.callbacks.inc
@@ -110,8 +110,8 @@ function _batch_test_finished_helper($batch_id, $success, $results, $operations)
     '#context' => [
       'batch_id' => $batch_id,
       'errors' => [
-       '#theme' => 'item_list',
-       '#items' => $messages,
+        '#theme' => 'item_list',
+        '#items' => $messages,
       ],
     ],
   ];

Fixed on commit.

  • alexpott committed b077607 on 8.0.x
    Issue #2550985 by justAChris, stefan.r: Remove SafeMarkup::set in...

Status: Fixed » Closed (fixed)

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