Problem/Motivation

calls to SafeMarkup::set() are meant to be for internal use only.

\Drupal\Core\Utility\Error::renderExceptionSafe()
has a SafeMarkup::set() right after a static::formatBacktrace($backtrace)

DefaultExceptionSubscriber::onHtml() has a SafeMarkup::escape() to deal with formatting a backtrace
but no call to SafeMarkup::set()

_drupal_log_error in core/includes/errors.inc
has a Error::formatBacktrace($backtrace) call also
and SafeMarkup::set() on line 254 near the end of _drupal_log_error()

Proposed resolution

  • (done) in Error::renderExceptionSafe(): Remove the call to SafeMarkup::set() by refactoring the code to use SafeMarkup::format()
  • in Error::renderExceptionSafe(): If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required. (refactoring is possible)
  • (done) improve consistency with other places where formatting backtraces are done.
  • in _drupal_log_error(): Remove the call to SafeMarkup::set() by refactoring the code to use SafeMarkup::format()
  • in _drupal_log_error(): If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required. (refactoring is possible)

Remaining tasks

  1. in Error::renderExceptionSafe(): Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123 DONE, it can be refactored to use SafeMarkup::format
  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. DONE, there is no coverage
  3. in Error::renderExceptionSafe(): If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented. DONE, it can be refactored
  4. in _drupal_log_error(): Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123 DONE, it can be refactored
  5. in _drupal_log_error(): 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. DONE, there is no coverage
  6. in _drupal_log_error(): If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented. DONE, it can be refactored
  7. security review asked for in #22 DONE in #24 by @pfrenssen
  8. Add automated test coverage for the sanitization of the string in Error::renderExceptionSafe()
  9. Add automated test coverage for the sanitization of the string in _drupal_log_error()

Manual testing steps (for XSS and double escaping)

Do these steps both with HEAD and with the patch applied:

  1. Clean install of Drupal 8.
  2. Enabled all error messages on /admin/config/development/logging Change to "All messages, with backtrace information"
  3. Throw an new exception somewhere in code with a script tag in the message (or use the custom module attached in comment [2501319-53])
  4. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
  5. If there is any user or calling code input in the string, submit
    alert('XSS');

    and ensure that it is sanitized.

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#90 remove-2501319-90.patch28.34 KBnlisgo
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,196 pass(es). View
#90 interdiff-2501319-88-90.txt741 bytesnlisgo
#88 remove-2501319-88.patch28.33 KBnlisgo
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,216 pass(es). View
#88 interdiff-2501319-86-88.txt540 bytesnlisgo
#86 remove-2501319-86.patch28.35 KBnlisgo
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,224 pass(es). View
#86 interdiff-2501319-82-86.txt651 bytesnlisgo
#82 remove-2501319-82.patch28.1 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,210 pass(es), 1 fail(s), and 0 exception(s). View
#82 interdiff.txt2.04 KBjoelpittet
#81 interdiff.txt2.94 KBjoelpittet
#81 remove-2501319-81.patch28.1 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,207 pass(es), 1 fail(s), and 0 exception(s). View
#79 interdiff.txt2.74 KBjoelpittet
#79 remove-2501319-79.patch27.29 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,181 pass(es), 1 fail(s), and 0 exception(s). View
#76 interdiff-76-77.txt2 KBstefan.r
#76 remove-2501319-77.patch27.44 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,185 pass(es). View
#72 remove-2501319-72.patch27.44 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,179 pass(es), 1 fail(s), and 0 exception(s). View
#72 interdiff-70-72.txt5.27 KBjoelpittet
#70 2501319-70.patch26.58 KBstefan.r
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,536 pass(es). View
#70 interdiff-63-70.txt4.98 KBstefan.r
#68 2501319-68.patch26.59 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,537 pass(es), 2 fail(s), and 0 exception(s). View
#68 interdiff-63-68.txt5 KBstefan.r
#63 interdiff-54-63.txt2.93 KBstefan.r
#63 2501319-63.patch24.47 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,446 pass(es), 2 fail(s), and 2 exception(s). View
#63 2501319-54-reroll.patch23.37 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,469 pass(es), 1 fail(s), and 2 exception(s). View
#54 interdiff.txt1.88 KBjoelpittet
#54 remove_safemarkup_set-2501319-54.patch23.5 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,827 pass(es). View
#53 custom.zip2.91 KBjoelpittet
#50 2501319-50.patch22.08 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,677 pass(es). View
#50 interdiff.2501319.49.50.txt1012 bytesYesCT
#49 2501319-49.patch22.09 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,686 pass(es). View
#49 interdiff.2501319.47.49.txt4.91 KBYesCT
#47 2501319-47.patch21.79 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,677 pass(es). View
#47 interdiff.2501319.38.47.txt2.19 KBYesCT
#38 remove_safemarkup_set-2501319-38.patch21.69 KBcmanalansan
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,806 pass(es). View
#30 2501319-25-reroll.patch21.7 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2501319-25-reroll.patch. Unable to apply patch. See the log in the details link for more information. View
#25 interdiff.txt3.41 KBCottser
#25 remove_safemarkup_set-2501319-25.patch22.07 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,061 pass(es). View
#19 Error___Site-Install_-_after.png286.74 KBjoelpittet
#19 Error___Site-Install_-_before.png283.82 KBjoelpittet
#18 interdiff.txt1.41 KBCottser
#18 remove_safemarkup_set-2501319-18.patch22.05 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,852 pass(es). View
#17 remove_safemarkup_set-2501319-17.patch22.13 KBcwells
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,716 pass(es). View
#14 interdiff-14.txt4.36 KBcwells
#14 remove_or_document-2501319-14.patch22.51 KBcwells
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,706 pass(es). View
#11 remove_or_document-2501319-11.patch23.76 KBcwells
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,681 pass(es). View
#8 remove_or_document-2501319-8.patch23.02 KBcwells
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,724 pass(es), 1 fail(s), and 0 exception(s). View
#6 remove_or_document-2501319-6.patch22 KBcwells
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch remove_or_document-2501319-6.patch. Unable to apply patch. See the log in the details link for more information. View
#1 remove_or_document-2501319-1.patch2.64 KBcwells
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,629 pass(es), 9 fail(s), and 3 exception(s). View

Comments

cwells’s picture

Status: Active » Needs review
FileSize
2.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,629 pass(es), 9 fail(s), and 3 exception(s). View

The reason for the SafeMarkup::set was largely because the message and the backtrace could contain HTML. Now we use @ to format the message and the backtrace, and have moved the backtrace's pre tags to inside the string itself.

cwells’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 1: remove_or_document-2501319-1.patch, failed testing.

Cottser’s picture

cwells’s picture

Title: Remove or document SafeMarkup::set in _drupal_log_error » Remove or document SafeMarkup::set in _drupal_log_error, DefaultExceptionSubscriber::onHtml

The same general logic is used in DefaultExceptionSubscriber as is in _drupal_log_error(), so combining those two instances into this issue.

cwells’s picture

Status: Needs work » Needs review
FileSize
22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch remove_or_document-2501319-6.patch. Unable to apply patch. See the log in the details link for more information. View

Turns out this patch needs to sweep into a few more areas, including catching another instance of SafeMarkup::set. Many places were found to depend on !message coming back from an Exception (which is now @message).

Status: Needs review » Needs work

The last submitted patch, 6: remove_or_document-2501319-6.patch, failed testing.

cwells’s picture

Status: Needs work » Needs review
FileSize
23.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,724 pass(es), 1 fail(s), and 0 exception(s). View

Actually just needed a re-roll from when I pulled this morning!

Status: Needs review » Needs work

The last submitted patch, 8: remove_or_document-2501319-8.patch, failed testing.

Cottser’s picture

So close! Awesome work @cwells.

cwells’s picture

Status: Needs work » Needs review
FileSize
23.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,681 pass(es). View

Update NodeCreationTest because it was counting on !message to pull out of the watchdog to make sure it rolled back an exception. Had to change to @message.

joelpittet’s picture

Status: Needs review » Needs work
+++ b/sites/default/default.services.yml
@@ -25,9 +25,9 @@ parameters:
-    # full domain name used to access the site. This mechanism is sufficient
-    # for most use-cases, including multi-site deployments. However, if it is
-    # desired that a session can be reused across different subdomains, the
+    # full domain name used to access the site. This mechanism is sufficent for
+    # most use-cases, including multi-site deployments. However, if it is
+    # desired that a session can be reused accross different subdomains, the

This shouldn't be in this patch.

joelpittet’s picture

A couple more things to help make this patch as clear as we can around the comments that were added:

  1. +++ b/core/includes/errors.inc
    @@ -205,12 +205,16 @@ function _drupal_log_error($error, $fatal = FALSE) {
    -      // Should not translate the string to avoid errors producing more errors.
    ...
    +      // We call SafeMarkup::format directly here, rather than use t() since
    +      // we are in the middle of error handling, and we don't want t() to
    +      // cause further errors.
    
    @@ -218,11 +222,16 @@ function _drupal_log_error($error, $fatal = FALSE) {
    +        $message = SafeMarkup::format('%type: @message in %function (line %line of %file). <pre class="backtrace">@backtrace</pre>', $error);
    ...
    +        $message = SafeMarkup::format('%type: @message in %function (line %line of %file).', $error);
    

    This is a longer version of this sentence refering to the ::format method. Please move the format call below the if statement because it happens on both code path and the comment will be in context.

  2. +++ b/core/includes/errors.inc
    @@ -205,12 +205,16 @@ function _drupal_log_error($error, $fatal = FALSE) {
    +      // With verbose logging, we will append a backtrace. To keep our string
    +      // safe, we need to include the <pre> tags inside the string to be
    +      // formatted, and not included as part of the placeholder.
    

    Remove the second sentence. It's only useful with this diff context.

  3. +++ b/core/includes/errors.inc
    @@ -218,11 +222,16 @@ function _drupal_log_error($error, $fatal = FALSE) {
    -        drupal_set_message(SafeMarkup::set($message), $class, TRUE);
    +        drupal_set_message($message, $class, TRUE);
    

    Sweet meat!

  4. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php
    @@ -104,12 +104,18 @@ protected function onHtml(GetResponseForExceptionEvent $event) {
    +      // We call SafeMarkup::format directly here, rather than use t() since
    +      // we are in the middle of error handling, and we don't want t() to
    +      // cause further errors.
    
    @@ -120,12 +126,16 @@ protected function onHtml(GetResponseForExceptionEvent $event) {
    +        $message = SafeMarkup::format('%type: @message in %function (line %line of %file). <pre class="backtrace">@backtrace</pre>', $error);
    ...
    +        $message = SafeMarkup::format('%type: @message in %function (line %line of %file).', $error);
    

    ditto on this to bring them together for the comment in context.

cwells’s picture

Title: Remove or document SafeMarkup::set in _drupal_log_error, DefaultExceptionSubscriber::onHtml » Remove or document SafeMarkup::set in _drupal_log_error, DefaultExceptionSubscriber::onHtml, Error::renderExceptionSafe
Status: Needs work » Needs review
FileSize
22.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,706 pass(es). View
4.36 KB

Incorporate feedback from 12 and 13.

cwells’s picture

Title: Remove or document SafeMarkup::set in _drupal_log_error, DefaultExceptionSubscriber::onHtml, Error::renderExceptionSafe » Remove SafeMarkup::set in _drupal_log_error, DefaultExceptionSubscriber::onHtml, Error::renderExceptionSafe
joelpittet’s picture

Status: Needs review » Needs work

Sorry @cwells a few things left in here I didn't spot the first review.

  1. +++ b/core/includes/errors.inc
    @@ -205,12 +205,11 @@ function _drupal_log_error($error, $fatal = FALSE) {
    +      // With verbose logging, we will append a backtrace.
    
    @@ -218,11 +217,21 @@ function _drupal_log_error($error, $fatal = FALSE) {
    +        $error['@backtrace'] = Error::formatBacktrace($backtrace);
    +        $error_message_format = '%type: @message in %function (line %line of %file). <pre class="backtrace">@backtrace</pre>';
    

    May be better to put this comment.

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php
    @@ -104,12 +104,13 @@ protected function onHtml(GetResponseForExceptionEvent $event) {
    -      if ($this->getErrorLevel() == ERROR_REPORTING_DISPLAY_VERBOSE) {
    +      $error_level = _drupal_get_error_level();
    ...
    +      if ($error_level == ERROR_REPORTING_DISPLAY_VERBOSE) {
    

    Why was this changed?

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php
    @@ -120,12 +121,21 @@ protected function onHtml(GetResponseForExceptionEvent $event) {
    +      ¶
    

    Extra space left (phpstorm will remove these automatically but not if you hit save on that line :S)

  4. +++ b/core/modules/system/src/Tests/System/ErrorHandlerTest.php
    @@ -180,15 +180,16 @@ function testExceptionHandler() {
    +    debug($message);
    

    Whoops left in there.

cwells’s picture

Status: Needs work » Needs review
FileSize
22.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,716 pass(es). View

Incorporate feedback from #16.

Cottser’s picture

FileSize
22.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,852 pass(es). View
1.41 KB

Two super minor things so just adding them here. I also pair programmed #1 with @cwells yesterday so this is my sneaky way to get commit credit :)

  1. +++ b/core/includes/errors.inc
    @@ -218,11 +219,18 @@ function _drupal_log_error($error, $fatal = FALSE) {
    +      // We call SafeMarkup::format directly here, rather than use t() since
    

    Minor: SafeMarkup::format()

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php
    @@ -120,12 +121,18 @@ protected function onHtml(GetResponseForExceptionEvent $event) {
    -
    -        // Generate a backtrace containing only scalar argument values. Make
    -        // sure the backtrace is escaped as it can contain user submitted data.
    -        $message .= '<pre class="backtrace">' . SafeMarkup::escape(Error::formatBacktrace($backtrace)) . '</pre>';
    +        // Generate a backtrace containing only scalar argument values.
    +        $error['@backtrace'] = Error::formatBacktrace($backtrace);
    +        $error_message_format .= ' <pre class="backtrace">@backtrace</pre>';
    

    Minor: Probably shouldn't remove this blank line.

joelpittet’s picture

Assigned: cwells » Unassigned
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
283.82 KB
286.74 KB

We went through this patch thoroughly to make sure we didn't miss anything.

I've manually tested this:

Before:

After:

Wim Leers’s picture

I think @pfrenssen worked on fixing the original security issue here, so it'd be great to get a review from him of this.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/includes/errors.inc
@@ -218,11 +219,18 @@ function _drupal_log_error($error, $fatal = FALSE) {
+      // We call SafeMarkup::format() directly here, rather than use t() since
+      // we are in the middle of error handling, and we don't want t() to
+      // cause further errors.
+      $message = SafeMarkup::format($error_message_format, $error);
+

+++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php
@@ -121,11 +122,18 @@ protected function onHtml(GetResponseForExceptionEvent $event) {
+      // We call SafeMarkup::format directly here, rather than use t() since
+      // we are in the middle of error handling, and we don't want t() to
+      // cause further errors.
+      $message = SafeMarkup::format($error_message_format, $error);
+
+      // Do not translate the string to avoid errors producing more errors.
+      drupal_set_message($message, $class, TRUE);

So, using t() indeed adds risk and coupling to other things that could produce further errors and make debugging more difficult. However, using SafeMarkup::format() also carries some of this risk. I discussed this issue briefly with @alexpott a couple days back and he had similar concerns.

What's in this patch is definitely safer in terms of sanitization, though. Setting NR for more feedback on that point, as well as for @pfrenssen's feedback. (He's not in the maintainer list so I can't assign it to him, but Wim pinged him already on Twitter.)

xjm’s picture

Issue tags: +needs security review

And probably this.

alexpott’s picture

I did have reservations about using the risk of SafeMarkup::format() and further exceptions - but I can't see how to get around that and the risk is minimal especially compared with t(). SafeMarkup is all (for better or worse) all static methods and not dependent on anything other than code.

pfrenssen’s picture

Thanks @WimLeers for bringing this to my attention :)

The issue that fixed the XSS vulnerability in displaying exception messages is #2371141: XSS vulnerability when displaying exception backtrace. In that issue a test was added to check if exception messages are properly escaped. Since the last patch is green we can rest assured that the messages are still safe.

I only had a quick look at the patch but the approach looks fine to me.

Cottser’s picture

FileSize
22.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,061 pass(es). View
3.41 KB
dawehner’s picture

I'd recommend this issue to really wait on #2509898: Additional uncaught exception thrown while handling exception after service changes as this changes a LOT in here.

dawehner’s picture

Status: Needs review » Postponed
alexpott’s picture

dawehner’s picture

This certainly needs a reroll.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
21.7 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2501319-25-reroll.patch. Unable to apply patch. See the log in the details link for more information. View

Here's the re-roll.

We have #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests going that does some of the bulk of this patch as well.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

No remaining taks left beside needs security review tag. Tested this manually and it seems to be still working.

dawehner’s picture

MH, I'm not entirely convinced by that. This should use something like String::format() which does not do the escaping. At this time Safemarkup will not be used any longer.

Cottser’s picture

@dawehner not sure what you mean by will not be used any longer?

Does your comment have anything to do with #2514044: Do not use SafeMarkup::format in exceptions by chance? Just reaching and guessing because I'm confused.

Edit: If I'm not mistaken String::format doesn't exist anymore.

dawehner’s picture

@dawehner not sure what you mean by will not be used any longer?

The point is that on those returns we don't use twig anymoger at all, it is just plan printing out of the strings. So SafeMarkup won't be used.

Edit: If I'm not mistaken String::format doesn't exist anymore.

yeah, but maybe we should have it

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: 2501319-25-reroll.patch, failed testing.

lauriii’s picture

Issue tags: +Needs reroll
cmanalansan’s picture

At DrupalGovCon, working on the reroll.

cmanalansan’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
21.69 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,806 pass(es). View
alimac’s picture

jrearick’s picture

I'm at DrupalCorn Camp sprint and will be trying to Identify whether there are 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.

jrearick’s picture

I did not find _drupal_log_error() referenced in the tests in core or in core modules.

# grep -r "_drupal_log_error(" .
./includes/bootstrap.inc:    _drupal_log_error(Error::decodeException($exception), TRUE);
./includes/errors.inc:    _drupal_log_error(array(
./includes/errors.inc:function _drupal_log_error($error, $fatal = FALSE) {
./lib/Drupal/Core/DrupalKernel.php:      // @todo: _drupal_log_error() and thus _drupal_exception_handler() prints
./lib/Drupal/Core/Utility/Error.php:   *   An error in the format expected by _drupal_log_error().
./modules/simpletest/src/WebTestBase.php:   * @see _drupal_log_error().
./modules/simpletest/src/WebTestBase.php:    // generated by _drupal_log_error() in the exact form required

I attempted to see if the methods that call _drupal_log_error() ( _drupal_error_handler_real() and _drupal_exception_handler() ) have tests, but then realized that future development in core may end up using this.

Perhaps a test should be created to test _drupal_log_error()

YesCT’s picture

Issue summary: View changes

added security review to the remaining tasks... but marked it as done cause @pfrenssen's feedback in #24 did that.

YesCT’s picture

Issue summary: View changes
Issue tags: -needs security review
YesCT’s picture

There are changes to tests in the patch... but I'm not sure if that proves we have test coverage. they are mostly changing ! to @

I'm going to try making _drupal_log_error() return something unexpected and incorrect and see which tests fail. #2544380-1: test issue for _drupal_log_error, DefaultExceptionSubscriber::onHtml, Error::renderExceptionSafe

@joelpittet Can you add the steps you did for manual testing to the issue summary? (I think we should repeat the manual testing, because there have been changes since it was done in #19)

YesCT’s picture

Title: Remove SafeMarkup::set in _drupal_log_error, DefaultExceptionSubscriber::onHtml, Error::renderExceptionSafe » Remove SafeMarkup::set DefaultExceptionSubscriber::onHtml and improve backtrace formatting consistancy in _drupal_log_error, Error::renderExceptionSafe, and DefaultExceptionSubscriber::onHtml
Issue summary: View changes

I'm trying to understand how the other classes got added to the issue and what the title should be.

"_drupal_log_error() calls SafeMarkup::set() which is meant to be for internal use only." seems in accurate.

there is no ::set() in _drupal_log_error().

There is no ::set() in DefaultExceptionSubscriber::onHtml
but there is a ::escape() for a backtrace

there is a ::formatBacktrace() and a SafeMarkup::set()
in core/lib/Drupal/Core/Utility/Error.php in renderExceptionSafe

so, retitling. Please see if this accurately explains things here. also please check updated motivation in the issue summary.

YesCT’s picture

Title: Remove SafeMarkup::set DefaultExceptionSubscriber::onHtml and improve backtrace formatting consistancy in _drupal_log_error, Error::renderExceptionSafe, and DefaultExceptionSubscriber::onHtml » Remove SafeMarkup::set in DefaultExceptionSubscriber::onHtml and improve backtrace formatting consistancy in _drupal_log_error, Error::renderExceptionSafe, and DefaultExceptionSubscriber::onHtml
YesCT’s picture

FileSize
2.19 KB
21.79 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,677 pass(es). View
+++ b/core/includes/errors.inc
@@ -208,12 +208,16 @@ function _drupal_log_error($error, $fatal = FALSE) {
+      $message = SafeMarkup::format('%type: @message in %function (line %line of %file).', $error);
+
+      // With verbose logging, we will append a backtrace.

@@ -221,7 +225,8 @@ function _drupal_log_error($error, $fatal = FALSE) {
-        $message .= '<pre class="backtrace">' . Error::formatBacktrace($backtrace) . '</pre>';
+        $error['@backtrace'] = Error::formatBacktrace($backtrace);
+        $message = SafeMarkup::format('%type: @message in %function (line %line of %file). <pre class="backtrace">@backtrace</pre>', $error);

we call format on a fragment used in the message.

and then if we attach a backtrace, we call format on a larger string which contains that fragment.

maybe we dont have to call format on the fragment if we are going to append a backtrace.

So how about something like this.

note interdiff has more context lines so it makes more sense. made with: git diff -U12

YesCT’s picture

Title: Remove SafeMarkup::set in DefaultExceptionSubscriber::onHtml and improve backtrace formatting consistancy in _drupal_log_error, Error::renderExceptionSafe, and DefaultExceptionSubscriber::onHtml » Remove SafeMarkup::set in Error::renderExceptionSafe and improve backtrace formatting consistancy in _drupal_log_error, Error::renderExceptionSafe, and DefaultExceptionSubscriber::onHtml

ug.

YesCT’s picture

FileSize
4.91 KB
22.09 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,686 pass(es). View
  1. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php
    @@ -91,11 +91,15 @@ protected function onHtml(GetResponseForExceptionEvent $event) {
    +      // With verbose logging, we will append a backtrace.
           if ($this->getErrorLevel() == ERROR_REPORTING_DISPLAY_VERBOSE) {
    

    making the same change in DefaultExceptionSubscriber.

    and... changing the comments (in both places) to make more sense with the way the code is now (we are not "appending").

  2. so to explain a little more my reasoning for making this an if/else:

    it is because SafeString::format() adds the string to the list of safe strings (see the comments on the @return for format:

       * @return string
       *   The formatted string, which is marked as safe unless sanitization of an
       *   unsafe argument was suppressed (see above).
    

    and without the if part, we would in some cases pollute the list of safe strings with a string we would not use.

YesCT’s picture

FileSize
1012 bytes
22.08 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,677 pass(es). View
+++ b/core/lib/Drupal/Core/Utility/Error.php
@@ -94,15 +94,14 @@ public static function renderExceptionSafe($exception) {
     // Remove 'main()'.
     array_shift($backtrace);
+    $decode['@backtrace'] = $backtrace;
...
-    $output = SafeMarkup::format('%type: !message in %function (line %line of %file).', $decode);
...
-    $output .= '<pre>' . static::formatBacktrace($backtrace) . '</pre>';
-    return SafeMarkup::set($output);
+    return SafeMarkup::format('%type: @message in %function (line %line of %file). <pre class="backtrace">@backtrace</pre>', $decode);

why did we move up the backtrace replacement and take out the formatBacktrace() ?

trying to put it back.

YesCT’s picture

Issue summary: View changes

so, there is still a call to SafeMarkup::set in _drupal_log_error in core/includes/errors.inc

we need to handle that.
added tasks to issue summary.

YesCT’s picture

Title: Remove SafeMarkup::set in Error::renderExceptionSafe and improve backtrace formatting consistancy in _drupal_log_error, Error::renderExceptionSafe, and DefaultExceptionSubscriber::onHtml » Remove SafeMarkup::set in Error::renderExceptionSafe and _drupal_log_error And improve backtrace formatting consistancy in _drupal_log_error, Error::renderExceptionSafe, and DefaultExceptionSubscriber::onHtml
joelpittet’s picture

Issue summary: View changes
FileSize
2.91 KB

Re @YesCT in #44 about how we did #19.

We had a custom module (see attached)

/admin/config/development/logging
Change to "All messages, with backtrace information"

OR in your settings.php

$config['system.logging']['error_level'] = 'verbose';

One thing to note since that was done, the Exception messages are no longer styled #2509898: Additional uncaught exception thrown while handling exception after service changes

joelpittet’s picture

FileSize
23.5 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,827 pass(es). View
1.88 KB

Just the ensure !message related to errors always are treated the same as @message.

YesCT’s picture

@joelpittet Thanks so much for those manual testing details!

If we want a test to ensure !message are the same as @message, then we should get the result of !message and @message and assert they are the same?

Actually I could use a general explanation of why this issue is changing so many of the !message to @message... and do we need to update some docs somewhere that say that is the "right way"?

YesCT’s picture

About the manual testing steps in the issue summary,
do we need step 5, the last step: If there is any user or calling code input in the string, submit alert('XSS'); and ensure that it is sanitized.
Seems like no, or that we have to find a way to make the message contain user input?

YesCT’s picture

Status: Needs review » Needs work

This is still needs work to remove the remaining call to SafeMarkup::set in _drupal_log_error in core/includes/errors.inc

alexpott’s picture

+++ b/core/includes/errors.inc
@@ -68,7 +68,7 @@ function _drupal_error_handler_real($error_level, $message, $filename, $line, $c
-      '!message' => Xss::filterAdmin($message),
+      '@message' => Xss::filterAdmin($message),

This doesn't look right. Xss::filterAdmin() no longer marks stuff as safe so the @ will result in unexpected escaping. SafeMarkup::xssFilter() and use Xss::getAdminTags(). This also points to missing test coverage.

joelpittet’s picture

@alexpott and @YesCT regarding the !message to @message

Both now escape values that are not marked as safe. This is the issue that did that #2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument. @xjm and I are a still bit dubious about this change and we preemptively created: #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand and #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests

The only difference now for Twig rendering is that !message if passed in a variable to t() will mark entire output of SafeMarkup::format()/t() as unsafe, where @message in the same context will only escape the message. And the other difference (that I know of) is that when outputting to non-html (AKA most cases the command line) @message will always escape unsafe code and !message will not but will still mark the output as unsafe.

Does that clear things up? Make you more confused.

So the real question, at least to me, is where is _drupal_error_handler_real() outputting it's content?

In 99% of our code @something is the right way™ to do all things.

xjm’s picture

@joelpittet, but see #2506195: Remove SafeMarkup::set() from Xss::filter(). Xss::filter() no longer marks things as safe. (I had the same reaction you did when @alexpott said this aloud.) But that's what @alexpott is getting at -- the current patch probably should have started failing after #2506195: Remove SafeMarkup::set() from Xss::filter() and the fact that it didn't implies missing test coverage. I think.

joelpittet’s picture

Issue tags: +Needs tests

@xjm Yeah this needs some tests for sure. I see his point, just wanted to clarify the other side of things. the Xss::filterAdmin didn't change, and in that case depending on where it's output will dictate how the unfiltered output it will be escaped.

stefan.r’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
23.37 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,469 pass(es), 1 fail(s), and 2 exception(s). View
24.47 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,446 pass(es), 2 fail(s), and 2 exception(s). View
2.93 KB

Added a test for double escaping along with the fix suggested in #59.

@joelpittet: as to where _drupal_error_handler_real() outputs its content, I think that'd usually be in the messages area (or on a maintentance theme page). We recommend turning off error display on production sites, but some people forget :)

Status: Needs review » Needs work

The last submitted patch, 63: 2501319-63.patch, failed testing.

The last submitted patch, 63: 2501319-54-reroll.patch, failed testing.

stefan.r’s picture

The test failure in UncaughtExceptionTest seems to be about error.log not being created (also in the original patch now) and the one in SimpleTestErrorCollectorTest still looks for the outdated "Drupal is awesome" string.

+++ b/core/includes/errors.inc
@@ -68,7 +68,7 @@ function _drupal_error_handler_real($error_level, $message, $filename, $line, $c
       // The standard PHP error handler considers that the error messages
       // are HTML. We mimick this behavior here.
-      '!message' => Xss::filterAdmin($message),
+      '@message' => SafeMarkup::xssFilter($message, Xss::getAdminTagList()),

@@ -109,9 +109,9 @@ function error_displayable($error = NULL) {
- *   with the exception of !message, which needs to be a safe HTML string, and
+ *   with the exception of @message, which needs to be an HTML string, and

what kind of HTML do we actually want to show in the message, maybe we could do stricter filtering than merely admin tags?

xjm’s picture

Title: Remove SafeMarkup::set in Error::renderExceptionSafe and _drupal_log_error And improve backtrace formatting consistancy in _drupal_log_error, Error::renderExceptionSafe, and DefaultExceptionSubscriber::onHtml » Remove SafeMarkup::set() in Error::renderExceptionSafe() and _drupal_log_error() and improve backtrace formatting consistency in _drupal_log_error(), Error::renderExceptionSafe(), and DefaultExceptionSubscriber::onHtml ()
stefan.r’s picture

Status: Needs work » Needs review
FileSize
5 KB
26.59 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,537 pass(es), 2 fail(s), and 0 exception(s). View

Just to answer to my previous question, considering errors are supposed to be turned off on production environments I think XSS filtering should be enough.

Status: Needs review » Needs work

The last submitted patch, 68: 2501319-68.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
4.98 KB
26.58 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,536 pass(es). View
josephdpurcell’s picture

Reviewing now.

joelpittet’s picture

FileSize
5.27 KB
27.44 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,179 pass(es), 1 fail(s), and 0 exception(s). View
stefan.r’s picture

@joelpittet changes look good!

According to its docs SafeString is supposed to be @internal to the render system, so maybe we can introduce something like XssFilteredErrorString for safe error messages?

josephdpurcell’s picture

Issue summary: View changes

Update summary proposed resolution to reflect that refactoring *is* possible.

Status: Needs review » Needs work

The last submitted patch, 72: remove-2501319-72.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
27.44 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,185 pass(es). View
2 KB
josephdpurcell’s picture

Issue summary: View changes
Status: Needs review » Needs work

I reviewed the remaining tasks and updated their statuses accordingly.

There are two remaining tasks:

  • Add automated test coverage for the sanitization of the string in Error::renderExceptionSafe()
  • Add automated test coverage for the sanitization of the string in _drupal_log_error()

Setting to needs work for adding tests for Error:renderExceptionSafe.

I don't see any tests for core/includes/errors.inc. Is that something we want to add now?

stefan.r’s picture

Thanks! We also need to address #76

joelpittet’s picture

Status: Needs work » Needs review
FileSize
27.29 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,181 pass(es), 1 fail(s), and 0 exception(s). View
2.74 KB

Moved the line back up and used an & instead because it's easier to test for double escaping as &amp;amp;

Tried to remove the necessity to mark safe and move it to where it's passed in instead of where we are checking.

Status: Needs review » Needs work

The last submitted patch, 79: remove-2501319-79.patch, failed testing.

joelpittet’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
28.1 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,207 pass(es), 1 fail(s), and 0 exception(s). View
2.94 KB

Resolved #77 remaining issues around adding checks for escaping and double escaping for error and fatals.

And should resolve that remaining failure as well.

joelpittet’s picture

FileSize
2.04 KB
28.1 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,210 pass(es), 1 fail(s), and 0 exception(s). View

Uno mas

The last submitted patch, 81: remove-2501319-81.patch, failed testing.

josephdpurcell’s picture

I've looked at the tests and I believe they are sufficiently testing the changes made. There are no more remaining tasks, which means that I believe this is ready to be RTBC if the tests pass.

Status: Needs review » Needs work

The last submitted patch, 82: remove-2501319-82.patch, failed testing.

nlisgo’s picture

Status: Needs work » Needs review
FileSize
651 bytes
28.35 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,224 pass(es). View

'Drupal is awesome' was still being displayed instead of 'Drupal & awesome' so the test was failing.

alexpott’s picture

+++ b/core/includes/errors.inc
@@ -252,7 +262,7 @@ function _drupal_log_error($error, $fatal = FALSE) {
-        drupal_set_message(SafeMarkup::set($message), $class, TRUE);
+        drupal_set_message(SafeString::create($message), $class, TRUE);

I don't think the SafeString::create() is necessary here since I think at this point $message is always the result of a SafeMarkup::format().

nlisgo’s picture

FileSize
540 bytes
28.33 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,216 pass(es). View

Address feedback in #87.

akalata’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/src/Tests/System/ErrorHandlerTest.php
    @@ -181,16 +187,16 @@ function testExceptionHandler() {
    +    $this->assertRaw($message, format_string('Found error message: @message.', array('@message' => $message)));
    

    We should be asserting the specific text we expect in this test, rather than passing it through format_string.

  2. +++ b/core/modules/system/src/Tests/System/ErrorHandlerTest.php
    @@ -181,16 +187,16 @@ function testExceptionHandler() {
    +    $this->assertNoRaw($message, format_string('Did not find error message: @message.', array('@message' => $message)));
    

    And here.

  3. +++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
    @@ -94,11 +94,11 @@ function testArgumentDefaultNoOptions() {
    +    $message = t('%type: @message in %function', $error);
    +    $this->assertNoRaw($message, t('Did not find error message: @message.', array('@message' => $message)));
    

    Here as well.

nlisgo’s picture

Status: Needs work » Needs review
FileSize
741 bytes
28.34 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,196 pass(es). View

@akalata, unless I'm misunderstanding you the text that we are asserting is not passed through format_string. In each case it is the variable $message.

Your comment prompted me to read the documentation for the assertRaw and assertNoRaw methods and in both cases it encourages the use of format_string for values passed as the second parameter but to avoid t(). So I am addressing that in this patch.

joelpittet’s picture

Thanks for resolving my last failure @nlisgo.

stefan.r’s picture

I think this looks good now...

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

yep. looks good to me also.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I think @akalata's point was that doing format_string(), t() or SafeMarkup::format() in tests is wrong cause we should just test for the expected output - i.e. hard code the string. That said -the use of format_string / SafeMarkup::format() is all over tests so I'm going to commit this anyway as it is only test code and we have an issue somewhere to address this.

Committed bb2d279 and pushed to 8.0.x. Thanks!

diff --git a/core/includes/errors.inc b/core/includes/errors.inc
index 0752aea..215b6cb 100644
--- a/core/includes/errors.inc
+++ b/core/includes/errors.inc
@@ -168,7 +168,7 @@ function _drupal_log_error($error, $fatal = FALSE) {
     if ($fatal) {
       // When called from CLI, simply output a plain text message.
       // Should not translate the string to avoid errors producing more errors.
-      $response->setContent(html_entity_decode(strip_tags(format_string('%type: @message in %function (line %line of %file).', $error))). "\n");
+      $response->setContent(html_entity_decode(strip_tags(SafeMarkup::format('%type: @message in %function (line %line of %file).', $error))). "\n");
       $response->send();
       exit;
     }
@@ -179,7 +179,7 @@ function _drupal_log_error($error, $fatal = FALSE) {
       if (error_displayable($error)) {
         // When called from JavaScript, simply output the error message.
         // Should not translate the string to avoid errors producing more errors.
-        $response->setContent(format_string('%type: @message in %function (line %line of %file).', $error));
+        $response->setContent(SafeMarkup::format('%type: @message in %function (line %line of %file).', $error));
         $response->send();
       }
       exit;

I've swapped these two runtime usages of format_string() since the context switching between SafeMarkup::format() and format_string() in the same piece of code makes for unnecessary confusion.

  • alexpott committed bb2d279 on 8.0.x
    Issue #2501319 by joelpittet, cwells, stefan.r, YesCT, nlisgo, Cottser,...

Status: Fixed » Closed (fixed)

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