Double-escaped log message

<em class="placeholder">Warning</em>: DateTime::createFromFormat(): Invalid date.timezone value '@@TIMEZONE@@', we selected the timezone 'UTC' for now. in <em class="placeholder">Drupal\Core\EventSubscriber\FinishResponseSubscriber-&gt;setExpiresNoCache()</em> (line <em class="placeholder">263</em> of <em class="placeholder">/Users/webchick/Sites/8.x/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php</em>).

Relevant code is in DbLogController::eventDetails().

This fixes it, but not sure it's the right fix. I wanted to try and fix it in formatMessage() but that didn't seem to take effect.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

This solves the problem, but I think formatMessage() would be a better place to solve it like you mentioned.

This works as well.

subhojit777’s picture

Status: Needs review » Closed (duplicate)

There is already an issue for this #2345779: Fix double-escaping due to Twig autoescape in dblog event "operations", see comment. We should close this one.

cburschka’s picture

Issue summary: View changes
Status: Closed (duplicate) » Needs review
FileSize
11.49 KB

Sorry, the other issue did not fix this. That patch targeted the "operations" links; the message itself is still escaped.

Screenshot from an install based on the latest source (commit c83565f6).

Edit: Based on looking at some of the rejected patches, it seems that some of them did try to fix the message too, but after some iterations the one that ultimately got committed only got the link.

cburschka’s picture

Last patch still applies with small offset, and still fixes the problem.

subhojit777’s picture

Patch looks good. I would have RTBC'd it, but not sure whether it needs tests.

ruscoe’s picture

Had to make a small change to the patch from #1 to get this working for me.

I wrapped the formatted message in a call to SafeMarkup::set to bring DbLogController::formatMessage in line with how DbLogController::overview displays the log message.

dawehner’s picture

Before we change that we should be 100% ensure that we have a test to ensure that we escape once here.

pwolanin’s picture

Status: Needs review » Needs work

We should not be adding any uses of SafeMarkup::set()

Instead, we possibly want to use SafeMarkup::checkAdminXss($string), or the suggested inline code inside that method.

cburschka’s picture

What would be the correct approach here?

Edit: ah

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

subhojit777’s picture

Not using SafeMarkup::checkAdminXss($string) and using SafeMarkup::xssFilter($string) because the former function will be deprecated soon.

subhojit777’s picture

Assigned: subhojit777 » Unassigned
subhojit777’s picture

Status: Needs review » Needs work

oops, looks like something's wrong. This patch exception_log_messages-2409881-12-only-test.patch should have failed.

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

looks like the test works with <script> tag, and fails with <em> tag.

Status: Needs review » Needs work

The last submitted patch, 16: exception_log_messages-2409881-16-only-test.patch, failed testing.

subhojit777’s picture

Status: Needs work » Needs review

The last submitted patch, 16: exception_log_messages-2409881-16.patch, failed testing.

dawehner’s picture

Status: Needs review » Needs work

So yeah the patch no longer works as expected ...

pwolanin’s picture

Status: Needs work » Fixed

This seems to be fixed in HEAD - I cannot reproduce it and the text is not double escaped.

Status: Fixed » Closed (fixed)

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