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
\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
Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123Identify 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)
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
Comment | File | Size | Author |
---|---|---|---|
#25 | interdiff.txt | 1.25 KB | dsnopek |
#25 | drupal8-dblog-safemarkup-test-2502025-25.patch | 1.94 KB | dsnopek |
Comments
Comment #1
joelpittetComment #2
cilefen CreditAttribution: cilefen commentedI am reviewing this at the NJ sprint.
Comment #3
cilefen CreditAttribution: cilefen commentedI have just gone over this patch for review.
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.
Comment #4
cilefen CreditAttribution: cilefen commentedRe #3.
Comment #5
cilefen CreditAttribution: cilefen commentedThat should be "containing unsafe content" if we go this route.
Comment #6
davidhernandezI think cilefen right that in this case it doesn't seem necessary to change to ::format. I fixed the comment a bit.
Comment #7
YesCT CreditAttribution: YesCT commentedI'll review this.
Comment #8
YesCT CreditAttribution: YesCT commentedhere it is with more context:
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.
Comment #9
cilefen CreditAttribution: cilefen commented.
Comment #10
YesCT CreditAttribution: YesCT commented@cilefen Thanks. That addresses both points.
Comment #11
xjmComment #12
xjmThanks @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:From #190 of that issue, commenting on an earlier version of the patch that used an inline template:
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 betweenSafeMarkup::set()
andXss::filterAdmin()
.So I think the attached is the correct patch.
Comment #13
alexpottYep 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.Comment #14
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #15
dsnopek@effulgentsia: Is this what you had in mind?
Comment #16
dsnopekActually, 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?
Comment #17
YesCT CreditAttribution: YesCT commentedLooked 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.
Comment #18
dsnopek@YesCT: Thanks for the review! I added the requested assert. :-)
Comment #21
dsnopekHrm. Not sure why that's failing, but let's try this!
Comment #23
geertvd CreditAttribution: geertvd at XIO commentedWell I guess that only the 1 link containing unsafe markup should be equal.
Comment #24
geertvd CreditAttribution: geertvd at XIO commentedComment #25
dsnopek@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.
Comment #26
YesCT CreditAttribution: YesCT commentedneeds 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
Comment #27
dsnopek@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!
Comment #28
YesCT CreditAttribution: YesCT commented@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!
Comment #29
alexpottAssigning to @xjm to have a final look at the patch. fwiw I think this is good to go.
Comment #31
xjmYep, the updated patch makes sense to me too. Sorry for the delay in reviewing. Committed and pushed to 8.0.x. Thanks!