Problem/Motivation

\Drupal\dblog\Tests\Views\ViewsIntegrationTest::testIntegration calls SafeMarkup::set() which is meant to be for internal use only.

This is an automated test, which is testing (among other things) that unsafe strings passed as the link in a watchdog entry will be filtered automatically. So, it itself is the test (per #2 under "Remaining tasks")!

It's possible to test this while not using SafeMarkup::set() which is what the patch does.

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)

N/A. This change is in an automated test, so there isn't anything to manually test!

User interface changes

N/A

API changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

Status: Active » Needs review
FileSize
751 bytes
cilefen’s picture

I am reviewing this at the NJ sprint.

cilefen’s picture

I have just gone over this patch for review.

  • I have become familiar with the parent META issue.
  • I executed ViewsIntegrationTest with and without this patch applied. The rendered log message is the same in both cases, so this patch eliminates a call SafeMarkup::set() in one of the allowed ways, SafeMarkup::format(), and does not introduce new problems. For that reason, this could be RTBC.

That said, I offer for consideration not replacing SafeMarkup::set() with SafeMarkup::format() in this issue. The purpose of the code in question is to deliberately pass unsafe content to l() to prove that XSS:filterAdmin() later strips it. In other words, a deliberate misuse of SafeMarkup::set() is not much different than a misuse of SafeMarkup::format() and is appropriate, with accompanying comments as to the reason.

cilefen’s picture

Re #3.

cilefen’s picture

+++ b/core/modules/dblog/src/Tests/Views/ViewsIntegrationTest.php
@@ -72,7 +72,8 @@ public function testIntegration() {
+      // Creates a link with a unsafe content which we wrongly mark as safe with

That should be "containing unsafe content" if we go this route.

davidhernandez’s picture

I think cilefen right that in this case it doesn't seem necessary to change to ::format. I fixed the comment a bit.

YesCT’s picture

I'll review this.

YesCT’s picture

Status: Needs review » Needs work

here it is with more context:

    // Setup a watchdog entry with two tokens.
    $entries[] = array(
      'message' => '@token1 !token2',
      // Creates a link containing unsafe content which we deliberately, wrongly
      // mark as safe with SafeMarkup::set() to verify that it is later filtered
      // and made safe by \Drupal\Component\Utility\Xss::filterAdmin().
      'variables' => array(
        '@token1' => $this->randomMachineName(),
        '!token2' => $this->randomMachineName(),
        'link' => \Drupal::l(SafeMarkup::set('<object>Link</object>'), new Url('<front>')),
      ),
    );

strictly speaking the SafeMarkup::set() should have the namespace on it also

https://www.drupal.org/coding-standards/docs#classes

hm. that says "If you use a namespace on a class anywhere in documentation, always make sure it is a fully-qualified namespace (beginning with a backslash)" but I thought it was always use the full namespace...

I think that comment is just about the link element in the array, not the whole variable array, so the comment should probably go directly before the
'link' => \Drupal::l(SafeMarkup::set('<object>Link</object>'), new Url('<front>')),
line.

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
1.07 KB

.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

@cilefen Thanks. That addresses both points.

xjm’s picture

Title: Remove SafeMarkup::set in core/modules/dblog/src/Tests/Views/ViewsIntegrationTest.php » Document SafeMarkup::set() in core/modules/dblog/src/Tests/Views/ViewsIntegrationTest.php
xjm’s picture

Title: Document SafeMarkup::set() in core/modules/dblog/src/Tests/Views/ViewsIntegrationTest.php » Remove SafeMarkup::set() in core/modules/dblog/src/Tests/Views/ViewsIntegrationTest.php
Status: Reviewed & tested by the community » Needs review
FileSize
731 bytes

Thanks @cilefen for considering the intent of the test. We should do this every time we look at a usage in tests.

The test was added in #1874534: Introduce a dblog / watchdog views integration to ensure the object tag was sanitized, before the introduction of SafeMarkup and Twig autoescaping. The SafeMarkup::set() was added in #2273923: Remove html => TRUE option from l() and link generator.. From the summary of that issue:

A literal containing HTML and text
There should be no example of this in non-test code, because all literal text should pass through t(), at which point it's no longer a literal and is covered by the "literal HTML with a variable" example above. Within test code, however, this can occur, such as within Drupal\dblog\Tests\Views\ViewsIntegrationTest::testIntegration(), but within test code, it's okay to use SafeMarkup::set() despite its warning of being for internal use only.

From #190 of that issue, commenting on an earlier version of the patch that used an inline template:

+++ b/core/modules/dblog/src/Tests/Views/ViewsIntegrationTest.php
@@ -77,7 +77,10 @@ public function testIntegration() {
-        'link' => \Drupal::l('<object>Link</object>', new Url('<front>')),
+        'link' => \Drupal::l(array(
+          '#type' => 'inline_template',
+          '#template' => '<object>Link</object>',
+        ), new Url('<front>')),

This is a test, and we have a literal, so I think \Drupal::l(SafeMarkup::set('

Link

'), ...) would be fine. In general, I don't see any problem with using SafeMarkup::set() on a literal, even within non-test code; it's only the embedding of variables where there's a risk.

So I don't actually think it was added to ensure that it was escaped even if SafeMarkup::set() is called. I think it was added as part of a mass conversion, without considering the previous intent of the test. It also seems like it is not the responsibility of this test to test the interaction between SafeMarkup::set() and Xss::filterAdmin().

So I think the attached is the correct patch.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Yep this SafeMarkup::set() use looks totally spurious. Any safe marking should be done by what is under test, unless we're testing how something is rendered under different safe markup states. That is not the case here.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

I don't think we want to remove this entirely. I think what the test in HEAD is intended to check is that if the log entry is written with unsafe markup (we don't always escape/filter everything when saving to storage, which is why we must filter/escape when printing to browser), that Views still displays it with proper filtering. So we don't want to checkPlain() on write in the test (which is what ends up happening if we call l() on a string that isn't marked safe). Perhaps we shouldn't call l() here though, and instead write an explicit anchor tag to make it clearer what the test is checking.

dsnopek’s picture

@effulgentsia: Is this what you had in mind?

dsnopek’s picture

Actually, I've been thinking about this a little more. If that fails (or actually even if it passes) we should try it again with the SafeMarkup::set() and see if the result is different. Because if the result is the same both with and without it, then that probably isn't really testing anything... Right?

Or is the idea that we want to make sure that that text gets filtered regardless of whether SafeMarkup::set() is used?

YesCT’s picture

Status: Needs review » Needs work

Looked at the assert which is later, and I think in order to actually check that getField() is filtering a string not marked safe (which has a html link tag), we need to additionally assert that the result of getfield() is different than the value we start with for the link.

dsnopek’s picture

@YesCT: Thanks for the review! I added the requested assert. :-)

Status: Needs review » Needs work

The last submitted patch, 18: drupal8-dblog-safemarkup-test-2502025-18.patch, failed testing.

The last submitted patch, 18: drupal8-dblog-safemarkup-test-2502025-18.patch, failed testing.

dsnopek’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
851 bytes

Hrm. Not sure why that's failing, but let's try this!

Status: Needs review » Needs work

The last submitted patch, 21: drupal8-dblog-safemarkup-test-2502025-21.patch, failed testing.

geertvd’s picture

Well I guess that only the 1 link containing unsafe markup should be equal.

geertvd’s picture

Status: Needs work » Needs review
dsnopek’s picture

@geertvd: Ooooh, that's totally it, thanks! I'm not sure how I missed that. It's only the 3rd entry that has any unsafe markup that will be filtered.

Here's a slight variation on the patch that puts that into the loop and adjusts the comment a little because I think it's a bit easier to see what's happening and why.

YesCT’s picture

needs an issue summary update
1) to fill out the steps to manually test
2) evaluate (and document in the summary) if there is test coverage

dsnopek’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

@YesCT: I updated the issue summary as requested! However, I think "Manual testing steps" are N/A in this case, because his change is in an automated test, so there isn't anything to manually test!

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

@dsnopek That makes sense.

I read the whole patch, and it makes sense to me, and stays within scope.
All the remaining tasks in the issue summary are done, and there are no remaining "needs" tags on this issue.

rtbc.

Thanks!

alexpott’s picture

Assigned: Unassigned » xjm

Assigning to @xjm to have a final look at the patch. fwiw I think this is good to go.

  • xjm committed 3b3c82e on 8.0.x
    Issue #2502025 by dsnopek, cilefen, geertvd, joelpittet, xjm,...
xjm’s picture

Assigned: xjm » Unassigned
Status: Reviewed & tested by the community » Fixed

Yep, the updated patch makes sense to me too. Sorry for the delay in reviewing. Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

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