Problem/Motivation
todo
Steps to reproduce
Todo
Proposed resolution
todo
Remaining tasks
Update issue summary.
Review code
Manual test
Commit
User interface changes
Translation on log messages page
API changes
NA
Data model changes
NA
Release notes snippet
NA
Original Post
I haven't tested this, but @hgoto has done some equivalent testing on a Drupal 7 patch that would have done what Drupal 8 does now (see #2459339: Log messages should be XSS filtered on display) and the problem occurred then.
Because https://api.drupal.org/api/drupal/core%21modules%21dblog%21src%21Control... does this:
$message = $this->t(Xss::filterAdmin($row->message), unserialize($row->variables));
It suggests that if the message has any characters which get altered by Xss:filterAdmin() (such as <
, >
or &
) then the message won't be properly translated, and the entire message will appear in English when viewing the logs even when the site is being viewed in a different language.
The solution of running Xss::filterAdmin() on the result of $this->t() instead was discussed in the above issue. It was rejected at the time, but I'm not sure the reasons for rejecting it are relevant anymore. Doing that should fix this.
@hgoto has an initial Drupal 8 patch for this at #2459339-29: Log messages should be XSS filtered on display
Comment | File | Size | Author |
---|---|---|---|
#95 | 2760031-95.patch | 7.8 KB | Abhisheksingh27 |
| |||
#94 | 2760031-94.patch | 7.52 KB | Abhisheksingh27 |
#92 | 2760031-92.patch | 8.01 KB | smustgrave |
#92 | interdiff-83-92.txt | 1.16 KB | smustgrave |
#91 | Screenshot from 2022-09-05 18-11-38.png | 115.46 KB | Anjali Rathod |
Comments
Comment #3
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented"Needs work" since there is a patch, which can be copied here.
Adding credit for @hgoto.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #5
hgoto CreditAttribution: hgoto as a volunteer commented@David_Rothstein, thank you for organizing the information and creating the issue.
I created a new patch for this issue. I revised some points from the previous patch #29 of #2459339: Log messages should be XSS filtered on display.
1. Changed the following line to avoid double escaping. For example,
<script>alert('foo');</script> <strong>Lorem ipsum</strong>
is shown asalert('foo'); <strong>Lorem ipsum</strong>
in the detailed page table if we don't change this line.2. Changed back the
@return
comment of the methodformatMessage()
.3. Splitted the test from the
testOverviewLinks()
and created a dedicated test casetestLogEventPageMessageEscaped()
.As for the point 1 above, I'm not confident that this approach is the best. I'd like someone to review this considering twig and table template.
Comment #6
hgoto CreditAttribution: hgoto as a volunteer commentedI'msorry, the patch #5 doesn't consider David_Rothstein's guide in comment #36 on #2459339: Log messages should be XSS filtered on display.
Surely,
generateLogEntries()
is not necessary to use here and I revised the test case to make it simpler.Comment #7
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedHmm, would the data => #markup change not always escape the thing with XSS::filterAdmin() again?
I think it would be better to always return a Markup object instead similar to how https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21... would do it.
Comment #8
hgoto CreditAttribution: hgoto as a volunteer commented@Fabianx, thank you for the review and assist.
I see. I changed the return value of
formatMessage()
to a Markup object. Surely it looks better.And
data => #markup
was necessary to avoid double escape in the patch #6 but it's no longer needed after replacing the return value offormatMessage()
.Comment #9
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC - looks good to me.
Comment #11
hgoto CreditAttribution: hgoto as a volunteer commentedThe auto test succeeded once and failed later with CI error. So I believe it's OK to make this status RTBC again.
Maybe this is related to #2749955: Random fails in UpdatePathTestBase tests
Comment #12
xjmYou can retest patches by clicking the "Add new test" link in tiny characters underneath the last test (not obvious, I know). :)
The CI error is actually a problem with the infrastructure itself, not random fails. So usually when that happens the patch should just be retested. Doing so now.
Comment #13
xjmThanks for working on this issue.
Why are we moving almost exactly the same test from one place to the other, without adding anything? Unless I missed something, what this accomplishes is making testbot install the site an extra time with the same coverage. That said...
So I guess that the idea here is that we want to XSS filter after the message is translated, rather than before.
It still seems a bit overcomplicated and also (in HEAD as well) a misuse of
t()
... however, before I recommend changes, let's get some test coverage. If the bug is that the messages do not get translated when they are supposed to be, then let's add tests for that -- for messages that should be translated and would not be in HEAD. We would upload a failing test-only patch for the translation, and a passing combined one.Once the test coverage is fixed, then we can ensure that any changes we make to the code itself also do what they are supposed to.
Thanks!
Comment #14
hgoto CreditAttribution: hgoto as a volunteer commented@xjm, thank you for your detailed review and comments.
As you saw, this doesn't change the coverage. We overused the method
generateLogEntries()
in the commit http://cgit.drupalcode.org/drupal/commit/?id=a2a4051 and we're going to fix that on D7 in #2459339: Log messages should be XSS filtered on display. I thought it's better to fix it also in D8 and that's why I changed the test without adding coverage. The comment https://www.drupal.org/node/2459339#comment-11363781 may be helpful.So I guess that the idea here is that we want to XSS filter after the message is translated, rather than before.
I see. If I understand that, we should create a test to confirm there's a bug in HEAD, right.
As the first step, I tried creating a new test-only patch. It's a little complicated and I'd like to upload another patch with working code to compare the results.
I didn't understand the misuse of t() and I'll tackle that point later.
If my understanding is wrong or not enough, please tell me. Thank you.
Comment #15
hgoto CreditAttribution: hgoto as a volunteer commentedComment #19
dagmarThanks! @hgoto
You can use ->notice().
Could you use the 3 types of placeholders here? We have another issue that needs tests regarding this topic. #2617330: LogMessageParser::parseMessagePlaceholders() needs to switch bang placeholder to colon placeholder
We need to use
[]
now.Comment #20
hgoto CreditAttribution: hgoto as a volunteer commented@dagmar thank you. Following the review #19, I revised the patch with 3 points improved. I'd like this to be reviewed.
I think the changes including the following is enough for that but if I don't understand it correctly, please let me know.
Tests in this patch needs the patch #2617330-2: LogMessageParser::parseMessagePlaceholders() needs to switch bang placeholder to colon placeholder committed in order to pass. (I tested it in my environment.)
Also, I'm sorry, I couldn't create an interdiff. Probably it's because the code around the patch changed since then.
Comment #21
hgoto CreditAttribution: hgoto as a volunteer commentedComment #24
dagmar@hgoto thanks. I marked #2617330: LogMessageParser::parseMessagePlaceholders() needs to switch bang placeholder to colon placeholder as a duplicate of this one.
Could you include the fix from #2617330 in your patch?
Comment #25
hgoto CreditAttribution: hgoto as a volunteer and at Studio Umi commented@dagmar thanks!
I merged the fix in #2617330 into the last patch. There was another
log(RfcLogLevel::NOTICE, ...)
left and I also replaced it tonotice(...)
. Auto Tests should pass...Comment #27
dagmarGreat, green. Almost there.
I forgot this, the use of REQUEST_TIME is deprecated. https://www.drupal.org/node/2785211
I would prefer to keep this lines here. The patch is adding 'locale' module as a dependency, that we could abstract into a new test case in the future because it seems only necessary for the new test. Lets keep this 6 lines of code unmodified.
Comment #28
dagmarGreat, green. Almost there.
I forgot this, the use of REQUEST_TIME is deprecated. https://www.drupal.org/node/2785211
I would prefer to keep this lines here. The patch is adding 'locale' module as a dependency, that we could abstract into a new test case in the future because it seems only necessary for the new test. Lets keep this 6 lines of code unmodified.
Comment #29
yogeshmpawarUpdated patch as per comment #27 & also added interdiff.
Comment #30
hgoto CreditAttribution: hgoto as a volunteer and at Studio Umi commented@dagmar thank you for your review.
1. Thank you. I didn't notice that. So I believe it's better to replace all
REQUEST_TIME
in this file, right?2. That test is covered in
testLogEventPageMessageEscaped()
and we can remove it. The test method name istestOverviewLinks()
and these lines shouldn't be there originally, I think. Please see comment #13 and #14. (If we keep it, it's better to change the method name, isn't it?)Comment #33
dagmarLets skip the replacement of REQUEST_TIME for now, several other issues has shown this change is not required as part of this patch.
2. That test is covered in testLogEventPageMessageEscaped() and we can remove it. The test method name is testOverviewLinks() and these lines shouldn't be there originally, I think. Please see comment #13 and #14. (If we keep it, it's better to change the method name, isn't it?)
I guess you are right. #29 claims it included in the interdiff but actually that's not true.
So the remaining thing for this patch is, rollaback the REQUEST_TIME related changes. and see if the testbot still pass. This will probably need a reroll. Tagging as novice since the remaining changes seems quite simple to implement.
Comment #34
yogeshmpawarComment #35
yogeshmpawarModified the patch as per comment #33 & also this patch failed to apply on 8.5.x branch because of "core/modules/dblog/src/Tests/DbLogTest.php" changed to "core/modules/dblog/tests/src/Functional/DbLogTest.php".
Comment #36
dagmarCool, Thanks @Yogesh Pawar!
There is one more thing I would like to see in the test coverage. According to this #2952091: Show translatable db log message on the main view, the logs messages are not translated in views. So adding the same checkes that we have now for detail page logs in the logs view should expose the failure.
The approach then could be, take this portion of code and abstract it into a protected method (perhaps
assertTranslations
?)And then use it twice, one for
and other for
Hopefully this will fail only for views.
Comment #37
dagmarComment #38
yogeshmpawarComment #39
yogeshmpawarChanges addressed as per comment #36, also added an interdiff.
Comment #41
MerryHamster CreditAttribution: MerryHamster at Skilld commentedComment #42
MerryHamster CreditAttribution: MerryHamster at Skilld commentedonly reroll path #39 for 8.6.x
Comment #43
MerryHamster CreditAttribution: MerryHamster at Skilld commentedComment #45
dagmarThanks for the patches. I'm afraid my comments in #36 were not addressed properly.
An assert* method should assert "something", and currently is returning a
FormattableMarkup
which is not ideal.So more code should be inside the new method and should run the same assertions.
Let me know if there is something else to explain there, I will be glad to help.
Comment #46
yogeshmpawarComment #47
yogeshmpawarThanks @dagmar, As per my understanding i have updated a patch. Can you please look into the patch & tell me what i am missing or is it good to go.
Comment #48
dagmarAs I mentioned in #36. I think the
assertTranslations
method should make this assertions:And the
assertTranslations
should be ran twice, one for a single event admin/reports/dblog/1234 and other for the full list admin/reports/dblogComment #49
gawaksh CreditAttribution: gawaksh at OpenSense Labs for DrupalFit commentedThis Patch should solve the purpose
Comment #51
dagmarThanks @gawaksh.
I recommend you start applying the patch from #47 and make the modifications from there. You introduced some syntax errors in your patch and removed some useful function docs.
Comment #53
izus CreditAttribution: izus commentedHi,
here is a patch simply rerolling #47 as it doesn't apply no more.
Comment #54
izus CreditAttribution: izus commentedHi,
and here is a new patch based on #53 to adress #48
Thanks
Comment #59
anushrikumari CreditAttribution: anushrikumari at OpenSense Labs commentedSince patch #54 failed for 9.1.x so I re-rolled the patch for the same.
Comment #60
anushrikumari CreditAttribution: anushrikumari at OpenSense Labs commentedpatch after resolving the error
Comment #62
anushrikumari CreditAttribution: anushrikumari at OpenSense Labs commentedComment #63
naresh_bavaskarComment #65
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRe-roll patch for 9.2
Comment #66
abhisekmazumdarComment #67
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedPlease ignore
Comment #68
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedPrevious issues were fixed.
Comment #69
sanjayk CreditAttribution: sanjayk as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed #68 test cases.
Comment #70
volkswagenchickTagging for Florida DrupalCamp which is happening today, Feb 10, 12-4pm ET
Thanks
Comment #72
mindbet CreditAttribution: mindbet as a volunteer commentedTested successfully on Drupal 9.3 (see #69).
Reviewed the issue summary and reviewed the patch.
All looks correct, RTBC
Comment #73
larowlannit: this needs to break at 80 chars
Other than that, this looks good to me
Comment #74
renatogPatch with fixes on #73
Comment #75
dagmar@RenatoG thanks for your patch. There is an extra file in the patch that doesnt seems related to this issue.
Comment #76
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedAddressed #75
Comment #77
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedComment #78
dagmarThanks @KapilV patch looks good now.
Comment #80
mindbet CreditAttribution: mindbet as a volunteer commentedSetting back to RTBC.
The test failure in #79 was a glitch; re-run of test (see #76) passed.
Comment #81
alexpottThe change is functionally correct but the comment change is incorrect.
Instead of doing this here we could use the render system and do
in \Drupal\dblog\Controller\DbLogController::topLogMessages() and \Drupal\dblog\Controller\DbLogController::eventDetails()
That'd save us a Markup::create() and preserve the return type and do the admin XSS filtering consistently.
Alternatively I think we might be able to remove it entirely as HTML in the messages apart from the
Comment #82
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedDone with the changes mentioned in #81.1 ad #81.2, Please have a look and advise, if it is in the right direction.
Comment #83
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedFixed custom command fail issue.
Comment #87
smustgrave CreditAttribution: smustgrave at Mobomo commentedRerolled #83 for 9.5.x
Cleaned up some of the file displays for this ticket also
Tried adding an interdiff but some reason it failed to generate but I did tweak DbLogController.php line 442 to use
$rows[] = [$dblog->count, $message];
vs
$rows[] = [$dblog->count, ['#markup' => $message]];
The latter was preventing messages from appearing on the page not found page
Comment #89
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #90
smustgrave CreditAttribution: smustgrave at Mobomo commentedFixed CI failure
Comment #91
Anjali RathodPatch applied successfully, but the log messages are still not translated. attaching screenshots below.
Comment #92
smustgrave CreditAttribution: smustgrave at Mobomo commentedPlease ignore patches #90, #89, and #87 I introduced some out of scope changes.
@Anjali Rathod thanks for testing. For next time it's not necessary to upload a screenshot of the patch applying.
Also please rename the screenshots to be more meaningful for what it's displaying.
So going back to #83 I rerolled and applied some tweaks.
This will need an IS update. Specifically
What is being fixed?
What is the proposed solution?
Step to reproduce?
before/after screenshots.
Remaining tasks.
Got it started
Comment #94
Abhisheksingh27 CreditAttribution: Abhisheksingh27 at OpenSense Labs for DrupalFit commentedAdding reroll for 10.1.x.
please review
Comment #95
Abhisheksingh27 CreditAttribution: Abhisheksingh27 at OpenSense Labs for DrupalFit commentedReuploading the previous patch after removing unnecessary whitespaces
Comment #96
smustgrave CreditAttribution: smustgrave at Mobomo commentedIssue summary is full of todos which need to be addressed