Log messages should be XSS filtered on display

The markup for dblog messages is being worked on in #2345779: Fix double-escaping due to Twig autoescape in dblog event "operations" and includes XSS filtering for messages, so this can possible be closed when the related issue is committed.

This is already considered and tested here:

http://cgit.drupalcode.org/drupal/tree/core/modules/dblog/src/Tests/DbLo...

Thanks, dagmar. Moving back to 7.x in case anyone wants to backport that change.

This is still reproducible in Drupal 8 - just add code somewhere that does \Drupal::logger('my_module')->notice('<script type="text/javascript">alert("xss");</script>') and then go to the admin page to view the message.

Note that the issue (for both Drupal 7 and 8) only occurs when you click through to the actual log message, not when it's displayed on the overview page at admin/reports/dblog. The test linked above is for admin/reports/dblog so it isn't relevant here.

The test linked above is for admin/reports/dblog so it isn't relevant here.

I guess I shouldn't say it isn't relevant - it is (and the test is probably worth backporting too). But it's not a complete test for this issue since log messages are displayed elsewhere too.

Thanks @David_Rothstein. I will take this issue.

Here is the patch.

Actually this could be simplified.

@klausi I agree but, in this case apply the sanitizing function inside formatMessage will double escape the link generation in the listing See http://cgit.drupalcode.org/drupal/tree/core/modules/dblog/src/Controller/DbLogController.php#n182.

Based on this could you reconsider your comment? Or I need to refactor the entire formatMessage method?

Thanks @klausi this works fine and my tests introduced in #10 are confirming this.

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Looks that needs follow-up and proper status

+++ b/core/modules/dblog/src/Controller/DbLogController.php
@@ -346,11 +347,11 @@ public function formatMessage($row) {
       // Messages without variables or user specified text.
       if ($row->variables === 'N;') {
...
+        $message = Xss::filterAdmin($row->message);
...
       // Message to translate with injected variables.
       else {
...
+        $message = $this->t(Xss::filterAdmin($row->message), unserialize($row->variables));

Why message translated only in second case?

Following the approach of the patch #13, I created a patch for D7. I'd like someone to review this. Thank you in advance. Here are points:

- I added the filter_xss_admin() (D7 version of Xss::filterAdmin()) to filter the message output.
- Also I revised the helper method generateLogEntries() in the class DBLogTestCase to match it with the D8 version and use it in the test case added.

@andypost,

> Why message translated only in second case?

I believe that this is because messages without any variable may be very case specific and should not be added into translation texts. 'N;' equals to serialize(NULL).

Hi,
I tested the patch in #20 and it works like a charm.

It would be great to get this committed.

Looks like it probably just needs a simple reroll.

Reroll patch against last 7.x

Just a quick look at the patch right now... but:

-      $output = t($event->message, unserialize($event->variables));
+      $output = t(filter_xss_admin($event->message), unserialize($event->variables));

Does putting the filter_xss_admin() inside of the t() have the potential to break translations (since it can alter the string)?

Because I don't think this (from above) is quite true:

Xss::filterAdmin() does not escape characters, it only strips dangerous HTML tags.

At least testing with Drupal 7:

$string = 'test < & >';
print filter_xss_admin($string);

Results in test < & > as output.

I think it is smart enough not to double-escape maybe (if it is run twice), but even a single escape like the above looks to me like it might break translations for any string that has those kinds of characters in it.

@David_Rothstein, initially I had done a quick xss test (when I set the RTBC), then I just rerolled the patch; however, due to your last comment; I did some more testing and it looks to be working as expected: no xss, strings looks ok in WD overview and WD detail.

This is the code that I've used to test:

watchdog('custom', '<script type="text/javascript">alert(1);</script>');
watchdog('custom', '< & > @test', array('@test' => 'test'));
watchdog('custom', '<script type="text/javascript">alert(1);</script>  @test', array('@test' => 'test'));

If you have more advices; I'll be really glad and I'll try to apply them.

Thanks,

Thanks.

What I'd be curious about based on my above comment is if the string in the second example ('< & > @test') has a translation into a different language, what happens? My guess is that before the patch the translated version would appear in the logs (to an administrator viewing the log messages page in a non-English language) but afterwards the non-translated version would appear.

I tested the translation with the sample code #27.

watchdog('custom', '< & > @test', array('@test' => 'test'));
watchdog('custom', '<script type="text/javascript">alert(1);</script>  @test', array('@test' => 'test'));

The result is that, exactly as David_Rothstein told,

My guess is that before the patch the translated version would appear in the logs (to an administrator viewing the log messages page in a non-English language) but afterwards the non-translated version would appear.

tranlation source texts changed with the patch #25. So #25 is not appropriate.

I tried creating a new patch for D7 to avoid this translation break. The diff is so small.

Again, as for D8, should we create a new patch for D8 or not? The patch #13 was already committed and I created a new one as a starting point. Even with this patch, there can be double escapes for the log list page. Should we fix that now? (This double escape problem is not seen in D7, I believe.)

The patch #29 for D7 failed to apply the latest dev because other commit changed the same file... I've recreated it and am going to retry it first.

As for D8 patch, my local test was not enough, I'm sorry.

I failed to delete one line and adjusted it.

The new approach makes more sense to me. It does mean you can't display HTML tags in the logs that filter_xss_admin() doesn't allow (even if you pass that via a !placeholder to t()) but that's probably OK. I doubt most people want lots of HTML in their log messages anyway.

I just noticed the code right above this (visible in the patch file) is using filter_xss() but this new code is using filter_xss_admin(). We should probably reconcile that (or at least add a code comment)... maybe it makes some sense if the former is going into a link and the latter isn't. Overall I think filter_xss_admin() is better here, to be as permissive as possible.

What is the reason for making all those changes to the generateLogEntries() method in the tests? Since we're just trying to test a specific log message here, I would think having the new test call watchdog() directly would be better.

I guess we should create a separate Drupal 8 issue to possibly fix this as a followup there.

I created #2760031: Log messages won't appear translated if they have special characters in them as a related issue for fixing that problem in Drupal 8.

@David_Rothstein, thank you for your detailed feedback. I created a new patch.

1.

I just noticed the code right above this (visible in the patch file) is using filter_xss() but this new code is using filter_xss_admin(). We should probably reconcile that (or at least add a code comment)... maybe it makes some sense if the former is going into a link and the latter isn't. Overall I think filter_xss_admin() is better here, to be as permissive as possible.

I see. I saw the corresponding D8 code and found that the D8 method formatMessage() which replaces theme_dblog_message() of D7 doesn't add the link. Instead, page callback DbLogContoller::overview() add a link after generating the message text from formatMessage() method.

a code in DbLogController::overview() of D8:

    foreach ($result as $dblog) {
      $message = $this->formatMessage($dblog);
      if ($message && isset($dblog->wid)) {
        $title = Unicode::truncate(Html::decodeEntities(strip_tags($message)), 256, TRUE, TRUE);
        $log_text = Unicode::truncate($title, 56, TRUE, TRUE);
        // The link generator will escape any unsafe HTML entities in the final
        // text.
        $message = $this->l($log_text, new Url('dblog.event', array('event_id' => $dblog->wid), array(
          'attributes' => array(
            // Provide a title for the link for useful hover hints. The
            // Attribute object will escape any unsafe HTML entities in the
            // final text.
            'title' => $title,
          ),
        )));
      }

After seeing this, I'd like to take the same approach as D8 does in D7 if it's acceptable. I mean, I'd like to simplify the theme_dbglog_mesasge() to make it create a text without link option. Then, the complexity in theme_dblog_message() that uses filter_xss() and filter_xss_admin() is cleared.

But, this change can be considered as an API change (since theme_dbglog_mesasge() will not use link parameter any more.). Shouldn't we do this?

2.

What is the reason for making all those changes to the generateLogEntries() method in the tests? Since we're just trying to test a specific log message here, I would think having the new test call watchdog() directly would be better.

I see. Exactly. I replaced generateLogEntries() into a simpler process without calling that method.

3.

This point is not a bug and a new proposal. If possible, I'd like to add a title attribute to each link to improve UI in this patch as following:

    $message = theme('dblog_message', array('event' => $dblog));
    // Add a link to the message.
    if ($message && isset($dblog->wid)) {
      // Escape all the tags and special characters in message.
      $title = truncate_utf8(filter_xss($message, array()), 256, TRUE, TRUE);
      $log_text = truncate_utf8($title, 56, TRUE, TRUE);
      $message = l($log_text, 'admin/reports/event/' . $dblog->wid, array(
        // Provide a title for the link for useful hover hints.
        'attributes' => array('title' => $title),
      ));
    }

This is exactly how D8 does.

Here's a revised patch and I'd like it to be reviewed.

Or should we wait for #2760031: Log messages won't appear translated if they have special characters in them to be fixed first before going ahead on this issue?

hmm. There was a new failed test in #38. I may have changed theme_dblog_message() too much... I'd like someone to review this approach. Anyway, I fixed the test failure.

I'm sorry to mess up the thread. I'll fix the point that causes the error.

I went a wrong way between #38 and #43... I'd like to restart from the point #35 and #36.

I updated the patch #35 following David_Rothstein's feedback.

I just noticed the code right above this (visible in the patch file) is using filter_xss() but this new code is using filter_xss_admin(). We should probably reconcile that (or at least add a code comment)... maybe it makes some sense if the former is going into a link and the latter isn't. Overall I think filter_xss_admin() is better here, to be as permissive as possible.

I investigated this point. As David_Rothstein told, the following code strips all the tags from $output and make it a link.

      $output = truncate_utf8(filter_xss($output, array()), 56, TRUE, TRUE);
      $output = l($output, 'admin/reports/event/' . $event->wid, array('html' => TRUE));

This pattern is used in the dblog overview (list) page where links are attached to the messages.

(filter_xss($output, array()) filters out all the tags and special characters as the empty second parameter array() means there's no allowed tag.)

So I added a new comment to explain this above the if statement for link.

What is the reason for making all those changes to the generateLogEntries() method in the tests? Since we're just trying to test a specific log message here, I would think having the new test call watchdog() directly would be better.

I see. Surely we don't need to use generateLogEntries() here. I replaced it to a simpler test code. And I stopped to change generateLogEntries() because this change is not necessary any more at this point.

As a result, the patch became much simpler. I'd like someone to review this.

RTBC - looks good to me!

Re-reviewed with Fabianx, and committed and pushed to 7.x, thanks!