silverstripe-framework: [CONTROL] Logging before httpError will cause HTML to be added and output as plaintext

Affected Version

SilverStripe 4.1 composer info --direct

silverstripe-themes/simple dev-master 4d546a4 The SilverStripe simple theme (default SilverStripe 3 theme)
silverstripe/recipe-cms    1.1.0              SilverStripe recipe for fully featured page and asset content editing
silverstripe/recipe-plugin 1.2.0              Helper plugin to install SilverStripe recipes

Description

Calling httpError with an error message will (magically and at an unknown location) prefix the message with HTML Dom and then send the entire thing to the browser, which causes conforming browsers to display the complete HTML instead of only the error message, as the Content-Type Header is rightfully set to text/plain during the Exception handling for security reasons.

Steps to Reproduce

Install Base SS 4.1 composer create-project --no-dev silverstripe/installer . ^4.1 and add the following code to a custom page action and load the appropriate page.

    Injector::inst()->get(LoggerInterface::class)->error("This causes bad stuff to happen"); // Removing this line will remove the HTML and just leave the browser outputting "foo"
    $this->httpError(400, "foo"); // Will raise an HTTPResponse_Exception, setting the content-type header to text/plain

For example edit mysite/code/PageController.php and add the above to a new function crash(), add ‘crash’ to the allowedActions array and add the necessary use lines at the top.

Complete PageController.php:

use SilverStripe\CMS\Controllers\ContentController;
use SilverStripe\Core\Injector\Injector;
use Psr\Log\LoggerInterface;

class PageController extends ContentController
{
    private static $allowed_actions = ['crash'];

    protected function init()
    {
        parent::init();
    }

    public function crash() {
        Injector::inst()->get(LoggerInterface::class)->error("This causes bad stuff to happen"); // Removing this line will remove the HTML and just leave the browser outputting "foo"
        $this->httpError(400, "foo"); // Will raise an HTTPResponse_Exception, setting the content-type header to text/plain
    }
}

Result

Instead of displaying a text “foo”, you will see a bunch of HTML source code and at the very end the “foo” we were expecting. Comment out the Injector::inst() line, reload the page and see that you’re only getting the expected “foo”

The content-type header is set to text/plain for valid security concerns and this is a good thing, however when doing so I would expect there to be no HTML present at all.

Fix Approach (by sminnee)

• MonologErrorHandler is refactored to accept an array of loggers as as well as a single logger.
  • In cases where an array is provided, it constructs a LoggerList decorator that dispatches log messages to each child LoggerInterface (maybe someone on packagist already has implemented this?)
• The default handler send messages to 2 services: %$Psr\Log\LoggerInterface and %$Psr\Log\LoggerInterface.errorhandler (in that order)
• %$Psr\Log\LoggerInterface.errorhandler is given the display error logic
• %$Psr\Log\LoggerInterface by default is just a Monolog\Logger with no handlers attached.

We should test that instructions for setting up, say, logging to a file remain working without modification, to ensure that we haven’t broken any APIs in doing this.