Problem/Motivation

ValidationTest::assertErrorMessages 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

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
  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.
  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:

  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

josephdpurcell created an issue. See original summary.

josephdpurcell’s picture

josephdpurcell’s picture

Issue summary: View changes
josephdpurcell’s picture

Issue summary: View changes
akalata’s picture

akalata’s picture

Status: Active » Postponed
Related issues: +#2505931: Remove SafeMarkup::set in ViewListBuilder

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

xjm’s picture

Status: Postponed » Active

Actually, this is a test, so we should assert the expected output instead, similarly to #2550961: Remove or document SafeMarkup::set in UserBlocksTest::testUserLoginBlock. Thanks!

akalata’s picture

@xjm that might make the test difficult to maintain, since the assertErrorMessages() is a helper function that is generating the text we expect to see based on the $messages that we are testing for. We're not specifically testing the creation of the error message itself.

kgoel’s picture

working on this

kgoel’s picture

Status: Active » Needs review
FileSize
1.37 KB
Anonymous’s picture

I don't see any issues with the code and tests pass locally.

akalata’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/src/Tests/Form/ValidationTest.php
@@ -292,11 +291,10 @@ protected function assertErrorMessages($messages) {
-    $top_message = \Drupal::translation()->formatPlural(count($error_links), '1 error has been found: !errors', '@count errors have been found: !errors', [
-      '!errors' => SafeMarkup::set(implode(', ', $error_links))
-    ]);
-    $this->assertRaw($top_message);
-    $this->assertNoText(t('An illegal choice has been detected. Please contact the site administrator.'));
+    foreach ($error_links as $links) {
+      $this->assertRaw(count($links), '1 error has been found:', $links);
+      $this->assertNoText(t('An illegal choice has been detected. Please contact the site administrator.'));
+    }

I don't think the two asserts should be within the foreach.

If my understanding of xjm's concern is correct, we should not be passing $links as a variable to the assert; instead we should specify the HTML of what $links should contain.

kgoel’s picture

I think I clearly misunderstood and so going to read test class.

kgoel’s picture

Status: Needs work » Needs review
FileSize
1.38 KB
1.04 KB

Status: Needs review » Needs work

The last submitted patch, 14: 2550965-14.patch, failed testing.

joelpittet’s picture

I think this is the exception:

+++ b/core/modules/system/src/Tests/Form/ValidationTest.php
@@ -292,11 +291,11 @@ protected function assertErrorMessages($messages) {
+    $this->assertRaw('1 error has been found:', $error_links);

The second argument is not needed, error_links is an array.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
1.45 KB

Let's see if this passes, I leave the formatPlural in but removed the errors from it.

stefan.r’s picture

+++ b/core/modules/system/src/Tests/Form/ValidationTest.php
@@ -292,11 +291,12 @@ protected function assertErrorMessages($messages) {
-    $this->assertNoText(t('An illegal choice has been detected. Please contact the site administrator.'));
+    $this->assertNoText('An illegal choice has been detected. Please contact the site administrator.');

unrelated change

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Novice

.

stefan.r’s picture

Maybe we can also find a solution that maintains the original translation string and asserts the errors appear in the right format/place (as before?)

stefan.r’s picture

We also still have #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages() open, should this be postponed on that issue?

joelpittet’s picture

Was hoping to avoid translating string outside of translation tests and also adding to the SafeMarkup list by way of t(). That's why I kept that in.

stefan.r’s picture

Yes, I wasn't aware we want to avoid t() in tests anyway. There is also no need for this to be blocked on the other issue as the outpit won't change.

Per #20 we probably want to keep the implode() though and assert for the full string (message + counts appended).

joelpittet’s picture

I'm trying to see what we'd be losing with the current patch. To me it's a space and some comma separation and order. Hmm

stefan.r’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
1.44 KB

Correct, so let's keep testing for that :)

joelpittet’s picture

stefan.r’s picture

Yes that will also work ;)

For me that's RTBC if green.

stefan.r’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Title: Remove or document SafeMarkup::set in ValidationTest::assertErrorMessages » Remove SafeMarkup::set() in ValidationTest::assertErrorMessages()
ssingla6c’s picture

last patch seems good towards double escape bugs and also removes SafeMarkup:set() call.so, it is good to be in RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed ee6c2d5 and pushed to 8.0.x. Thanks!

  • alexpott committed ee6c2d5 on 8.0.x
    Issue #2550965 by joelpittet, kgoel, stefan.r, josephdpurcell, akalata:...

Status: Fixed » Closed (fixed)

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