bug #23684 [Debug] Missing escape in debug output (c960657)

This PR was merged into the 2.7 branch.

Discussion
----------

[Debug] Missing escape in debug output

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

When pretty-printing an exception, the debug handler does not properly escape array keys.

The problem only occurs when debug output is enabled, so this is not considered a [security issue](http://symfony.com/doc/current/contributing/code/security.html) (according to @fabpot), because the debug tools [should not be used in production](https://symfony.com/doc/current/components/debug.html#usage).

A test for this is included in my patch for #18722.

Commits
-------

636777d [Debug] HTML-escape array key

BufferingLogger.php

@@ -1,37 +0,0 @@
-<?php
-
-/*
- * This file is part of the Symfony package.
- *
- * (c) Fabien Potencier <fabien@symfony.com>
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
-namespace Symfony\Component\Debug;
-
-use Psr\Log\AbstractLogger;
-
-/**
- * A buffering logger that stacks logs for later.
- *
- * @author Nicolas Grekas <p@tchwork.com>
- */
-class BufferingLogger extends AbstractLogger
-{
-    private $logs = array();
-
-    public function log($level, $message, array $context = array())
-    {
-        $this->logs[] = array($level, $message, $context);
-    }
-
-    public function cleanLogs()
-    {
-        $logs = $this->logs;
-        $this->logs = array();
-
-        return $logs;
-    }
-}

CHANGELOG.md

@@ -1,13 +1,6 @@
 CHANGELOG
 =========
 
-2.8.0
------
-
-* added BufferingLogger for errors that happen before a proper logger is configured
-* allow throwing from `__toString()` with `return trigger_error($e, E_USER_ERROR);`
-* deprecate ExceptionHandler::createResponse
-
 2.7.0
 -----
 

Debug.php

@@ -52,10 +52,9 @@ class Debug
             // CLI - display errors only if they're not already logged to STDERR
             ini_set('display_errors', 1);
         }
-        if ($displayErrors) {
-            ErrorHandler::register(new ErrorHandler(new BufferingLogger()));
-        } else {
-            ErrorHandler::register()->throwAt(0, true);
+        $handler = ErrorHandler::register();
+        if (!$displayErrors) {
+            $handler->throwAt(0, true);
         }
 
         DebugClassLoader::enable();

DebugClassLoader.php

@@ -223,26 +223,9 @@ class DebugClassLoader
                         @trigger_error(sprintf('The %s class extends %s that is deprecated %s', $name, $parent, self::$deprecated[$parent]), E_USER_DEPRECATED);
                     }
 
-                    $parentInterfaces = array();
-                    $deprecatedInterfaces = array();
-                    if ($parent) {
-                        foreach (class_implements($parent) as $interface) {
-                            $parentInterfaces[$interface] = 1;
-                        }
-                    }
-
-                    foreach ($refl->getInterfaceNames() as $interface) {
-                        if (isset(self::$deprecated[$interface]) && strncmp($ns, $interface, $len)) {
-                            $deprecatedInterfaces[] = $interface;
-                        }
-                        foreach (class_implements($interface) as $interface) {
-                            $parentInterfaces[$interface] = 1;
-                        }
-                    }
-
-                    foreach ($deprecatedInterfaces as $interface) {
-                        if (!isset($parentInterfaces[$interface])) {
-                            @trigger_error(sprintf('The %s %s %s that is deprecated %s', $name, $refl->isInterface() ? 'interface extends' : 'class implements', $interface, self::$deprecated[$interface]), E_USER_DEPRECATED);
+                    foreach (class_implements($class) as $interface) {
+                        if (isset(self::$deprecated[$interface]) && strncmp($ns, $interface, $len) && !is_subclass_of($parent, $interface)) {
+                            @trigger_error(sprintf('The %s %s %s that is deprecated %s', $name, interface_exists($class) ? 'interface extends' : 'class implements', $interface, self::$deprecated[$interface]), E_USER_DEPRECATED);
                         }
                     }
                 }

ErrorHandler.php

@@ -97,12 +97,10 @@ class ErrorHandler
     private $isRecursive = 0;
     private $isRoot = false;
     private $exceptionHandler;
-    private $bootstrappingLogger;
 
     private static $reservedMemory;
     private static $stackedErrors = array();
     private static $stackedErrorLevels = array();
-    private static $toStringException = null;
     private static $exitCode = 0;
 
     /**
@@ -160,14 +158,6 @@ class ErrorHandler
         return $handler;
     }
 
-    public function __construct(BufferingLogger $bootstrappingLogger = null)
-    {
-        if ($bootstrappingLogger) {
-            $this->bootstrappingLogger = $bootstrappingLogger;
-            $this->setDefaultLogger($bootstrappingLogger);
-        }
-    }
-
     /**
      * Sets a logger to non assigned errors levels.
      *
@@ -181,7 +171,7 @@ class ErrorHandler
 
         if (is_array($levels)) {
             foreach ($levels as $type => $logLevel) {
-                if (empty($this->loggers[$type][0]) || $replace || $this->loggers[$type][0] === $this->bootstrappingLogger) {
+                if (empty($this->loggers[$type][0]) || $replace) {
                     $loggers[$type] = array($logger, $logLevel);
                 }
             }
@@ -190,7 +180,7 @@ class ErrorHandler
                 $levels = E_ALL | E_STRICT;
             }
             foreach ($this->loggers as $type => $log) {
-                if (($type & $levels) && (empty($log[0]) || $replace || $log[0] === $this->bootstrappingLogger)) {
+                if (($type & $levels) && (empty($log[0]) || $replace)) {
                     $log[0] = $logger;
                     $loggers[$type] = $log;
                 }
@@ -213,7 +203,6 @@ class ErrorHandler
     {
         $prevLogged = $this->loggedErrors;
         $prev = $this->loggers;
-        $flush = array();
 
         foreach ($loggers as $type => $log) {
             if (!isset($prev[$type])) {
@@ -232,24 +221,9 @@ class ErrorHandler
                 throw new \InvalidArgumentException('Invalid logger provided');
             }
             $this->loggers[$type] = $log + $prev[$type];
-
-            if ($this->bootstrappingLogger && $prev[$type][0] === $this->bootstrappingLogger) {
-                $flush[$type] = $type;
-            }
         }
         $this->reRegister($prevLogged | $this->thrownErrors);
 
-        if ($flush) {
-            foreach ($this->bootstrappingLogger->cleanLogs() as $log) {
-                $type = $log[2]['type'];
-                if (!isset($flush[$type])) {
-                    $this->bootstrappingLogger->log($log[0], $log[1], $log[2]);
-                } elseif ($this->loggers[$type][0]) {
-                    $this->loggers[$type][0]->log($this->loggers[$type][1], $log[1], $log[2]);
-                }
-            }
-        }
-
         return $prev;
     }
 
@@ -423,10 +397,7 @@ class ErrorHandler
         }
 
         if ($throw) {
-            if (null !== self::$toStringException) {
-                $throw = self::$toStringException;
-                self::$toStringException = null;
-            } elseif ($scope && class_exists('Symfony\Component\Debug\Exception\ContextErrorException')) {
+            if ($scope && class_exists('Symfony\Component\Debug\Exception\ContextErrorException')) {
                 // Checking for class existence is a work around for https://bugs.php.net/42098
                 $throw = new ContextErrorException($this->levels[$type].': '.$message, 0, $type, $file, $line, $context);
             } else {
@@ -441,47 +412,6 @@ class ErrorHandler
                 $throw->errorHandlerCanary = new ErrorHandlerCanary();
             }
 
-            if (E_USER_ERROR & $type) {
-                $backtrace = $backtrace ?: $throw->getTrace();
-
-                for ($i = 1; isset($backtrace[$i]); ++$i) {
-                    if (isset($backtrace[$i]['function'], $backtrace[$i]['type'], $backtrace[$i - 1]['function'])
-                        && '__toString' === $backtrace[$i]['function']
-                        && '->' === $backtrace[$i]['type']
-                        && !isset($backtrace[$i - 1]['class'])
-                        && ('trigger_error' === $backtrace[$i - 1]['function'] || 'user_error' === $backtrace[$i - 1]['function'])
-                    ) {
-                        // Here, we know trigger_error() has been called from __toString().
-                        // HHVM is fine with throwing from __toString() but PHP triggers a fatal error instead.
-                        // A small convention allows working around the limitation:
-                        // given a caught $e exception in __toString(), quitting the method with
-                        // `return trigger_error($e, E_USER_ERROR);` allows this error handler
-                        // to make $e get through the __toString() barrier.
-
-                        foreach ($context as $e) {
-                            if (($e instanceof \Exception || $e instanceof \Throwable) && $e->__toString() === $message) {
-                                if (1 === $i) {
-                                    // On HHVM
-                                    $throw = $e;
-                                    break;
-                                }
-                                self::$toStringException = $e;
-
-                                return true;
-                            }
-                        }
-
-                        if (1 < $i) {
-                            // On PHP (not on HHVM), display the original error message instead of the default one.
-                            $this->handleException($throw);
-
-                            // Stop the process by giving back the error to the native handler.
-                            return false;
-                        }
-                    }
-                }
-            }
-
             throw $throw;
         }
 

ExceptionHandler.php

@@ -39,8 +39,6 @@ class ExceptionHandler
     public function __construct($debug = true, $charset = null, $fileLinkFormat = null)
     {
         if (false !== strpos($charset, '%')) {
-            @trigger_error('Providing $fileLinkFormat as second argument to '.__METHOD__.' is deprecated since version 2.8 and will be unsupported in 3.0. Please provide it as third argument, after $charset.', E_USER_DEPRECATED);
-
             // Swap $charset and $fileLinkFormat for BC reasons
             $pivot = $fileLinkFormat;
             $fileLinkFormat = $charset;
@@ -166,7 +164,6 @@ class ExceptionHandler
             $response = $this->createResponse($exception);
             $response->sendHeaders();
             $response->sendContent();
-            @trigger_error(sprintf("The %s::createResponse method is deprecated since 2.8 and won't be called anymore when handling an exception in 3.0.", $reflector->class), E_USER_DEPRECATED);
 
             return;
         }
@@ -180,7 +177,7 @@ class ExceptionHandler
      * This method uses plain PHP functions like header() and echo to output
      * the response.
      *
-     * @param \Exception|FlattenException $exception An \Exception or FlattenException instance
+     * @param \Exception|FlattenException $exception An \Exception instance
      */
     public function sendPhpResponse($exception)
     {
@@ -202,37 +199,17 @@ class ExceptionHandler
     /**
      * Creates the error Response associated with the given Exception.
      *
-     * @param \Exception|FlattenException $exception An \Exception or FlattenException instance
+     * @param \Exception|FlattenException $exception An \Exception instance
      *
      * @return Response A Response instance
-     *
-     * @deprecated since 2.8, to be removed in 3.0.
      */
     public function createResponse($exception)
     {
-        @trigger_error('The '.__METHOD__.' method is deprecated since version 2.8 and will be removed in 3.0.', E_USER_DEPRECATED);
-
         if (!$exception instanceof FlattenException) {
             $exception = FlattenException::create($exception);
         }
 
-        return Response::create($this->getHtml($exception), $exception->getStatusCode(), $exception->getHeaders())->setCharset($this->charset);
-    }
-
-    /**
-     * Gets the full HTML content associated with the given exception.
-     *
-     * @param \Exception|FlattenException $exception An \Exception or FlattenException instance
-     *
-     * @return string The HTML content as a string
-     */
-    public function getHtml($exception)
-    {
-        if (!$exception instanceof FlattenException) {
-            $exception = FlattenException::create($exception);
-        }
-
-        return $this->decorate($this->getContent($exception), $this->getStylesheet($exception));
+        return Response::create($this->decorate($this->getContent($exception), $this->getStylesheet($exception)), $exception->getStatusCode(), $exception->getHeaders())->setCharset($this->charset);
     }
 
     /**
@@ -442,7 +419,7 @@ EOF;
                 $formattedValue = str_replace("\n", '', var_export($this->escapeHtml((string) $item[1]), true));
             }
 
-            $result[] = is_int($key) ? $formattedValue : sprintf("'%s' => %s", $key, $formattedValue);
+            $result[] = is_int($key) ? $formattedValue : sprintf("'%s' => %s", $this->escapeHtml($key), $formattedValue);
         }
 
         return implode(', ', $result);

Tests/DebugClassLoaderTest.php

@@ -203,28 +203,6 @@ class DebugClassLoaderTest extends TestCase
         );
     }
 
-    public function testInterfaceExtendsDeprecatedInterface()
-    {
-        set_error_handler(function () { return false; });
-        $e = error_reporting(0);
-        trigger_error('', E_USER_NOTICE);
-
-        class_exists('Test\\'.__NAMESPACE__.'\\NonDeprecatedInterfaceClass', true);
-
-        error_reporting($e);
-        restore_error_handler();
-
-        $lastError = error_get_last();
-        unset($lastError['file'], $lastError['line']);
-
-        $xError = array(
-            'type' => E_USER_NOTICE,
-            'message' => '',
-        );
-
-        $this->assertSame($xError, $lastError);
-    }
-
     public function testDeprecatedSuperInSameNamespace()
     {
         set_error_handler(function () { return false; });
@@ -311,8 +289,6 @@ class ClassLoader
             eval('namespace Test\\'.__NAMESPACE__.'; class DeprecatedParentClass extends \\'.__NAMESPACE__.'\Fixtures\DeprecatedClass {}');
         } elseif ('Test\\'.__NAMESPACE__.'\DeprecatedInterfaceClass' === $class) {
             eval('namespace Test\\'.__NAMESPACE__.'; class DeprecatedInterfaceClass implements \\'.__NAMESPACE__.'\Fixtures\DeprecatedInterface {}');
-        } elseif ('Test\\'.__NAMESPACE__.'\NonDeprecatedInterfaceClass' === $class) {
-            eval('namespace Test\\'.__NAMESPACE__.'; class NonDeprecatedInterfaceClass implements \\'.__NAMESPACE__.'\Fixtures\NonDeprecatedInterface {}');
         } elseif ('Test\\'.__NAMESPACE__.'\Float' === $class) {
             eval('namespace Test\\'.__NAMESPACE__.'; class Float {}');
         }

Tests/ErrorHandlerTest.php

@@ -14,7 +14,6 @@ namespace Symfony\Component\Debug\Tests;
 use PHPUnit\Framework\TestCase;
 use Psr\Log\LogLevel;
 use Symfony\Component\Debug\ErrorHandler;
-use Symfony\Component\Debug\BufferingLogger;
 use Symfony\Component\Debug\Exception\ContextErrorException;
 
 /**
@@ -270,33 +269,6 @@ class ErrorHandlerTest extends TestCase
         }
     }
 
-    public function testHandleUserError()
-    {
-        try {
-            $handler = ErrorHandler::register();
-            $handler->throwAt(0, true);
-
-            $e = null;
-            $x = new \Exception('Foo');
-
-            try {
-                $f = new Fixtures\ToStringThrower($x);
-                $f .= ''; // Trigger $f->__toString()
-            } catch (\Exception $e) {
-            }
-
-            $this->assertSame($x, $e);
-
-            restore_error_handler();
-            restore_exception_handler();
-        } catch (\Exception $e) {
-            restore_error_handler();
-            restore_exception_handler();
-
-            throw $e;
-        }
-    }
-
     public function testHandleDeprecation()
     {
         $that = $this;
@@ -401,49 +373,6 @@ class ErrorHandlerTest extends TestCase
         }
     }
 
-    public function testBootstrappingLogger()
-    {
-        $bootLogger = new BufferingLogger();
-        $handler = new ErrorHandler($bootLogger);
-
-        $loggers = array(
-            E_DEPRECATED => array($bootLogger, LogLevel::INFO),
-            E_USER_DEPRECATED => array($bootLogger, LogLevel::INFO),
-            E_NOTICE => array($bootLogger, LogLevel::WARNING),
-            E_USER_NOTICE => array($bootLogger, LogLevel::WARNING),
-            E_STRICT => array($bootLogger, LogLevel::WARNING),
-            E_WARNING => array($bootLogger, LogLevel::WARNING),
-            E_USER_WARNING => array($bootLogger, LogLevel::WARNING),
-            E_COMPILE_WARNING => array($bootLogger, LogLevel::WARNING),
-            E_CORE_WARNING => array($bootLogger, LogLevel::WARNING),
-            E_USER_ERROR => array($bootLogger, LogLevel::CRITICAL),
-            E_RECOVERABLE_ERROR => array($bootLogger, LogLevel::CRITICAL),
-            E_COMPILE_ERROR => array($bootLogger, LogLevel::CRITICAL),
-            E_PARSE => array($bootLogger, LogLevel::CRITICAL),
-            E_ERROR => array($bootLogger, LogLevel::CRITICAL),
-            E_CORE_ERROR => array($bootLogger, LogLevel::CRITICAL),
-        );
-
-        $this->assertSame($loggers, $handler->setLoggers(array()));
-
-        $handler->handleError(E_DEPRECATED, 'Foo message', __FILE__, 123, array());
-        $expectedLog = array(LogLevel::INFO, 'Foo message', array('type' => E_DEPRECATED, 'file' => __FILE__, 'line' => 123, 'level' => error_reporting()));
-
-        $logs = $bootLogger->cleanLogs();
-        unset($logs[0][2]['stack']);
-
-        $this->assertSame(array($expectedLog), $logs);
-
-        $bootLogger->log($expectedLog[0], $expectedLog[1], $expectedLog[2]);
-
-        $mockLogger = $this->getMockBuilder('Psr\Log\LoggerInterface')->getMock();
-        $mockLogger->expects($this->once())
-            ->method('log')
-            ->with(LogLevel::WARNING, 'Foo message', $expectedLog[2]);
-
-        $handler->setLoggers(array(E_DEPRECATED => array($mockLogger, LogLevel::WARNING)));
-    }
-
     public function testHandleFatalError()
     {
         try {

Tests/Fixtures/NonDeprecatedInterface.php

@@ -1,7 +0,0 @@
-<?php
-
-namespace Symfony\Component\Debug\Tests\Fixtures;
-
-interface NonDeprecatedInterface extends DeprecatedInterface
-{
-}

Tests/Fixtures/ToStringThrower.php

@@ -1,24 +0,0 @@
-<?php
-
-namespace Symfony\Component\Debug\Tests\Fixtures;
-
-class ToStringThrower
-{
-    private $exception;
-
-    public function __construct(\Exception $e)
-    {
-        $this->exception = $e;
-    }
-
-    public function __toString()
-    {
-        try {
-            throw $this->exception;
-        } catch (\Exception $e) {
-            // Using user_error() here is on purpose so we do not forget
-            // that this alias also should work alongside with trigger_error().
-            return user_error($e, E_USER_ERROR);
-        }
-    }
-}

composer.json

@@ -23,8 +23,8 @@
         "symfony/http-kernel": ">=2.3,<2.3.24|~2.4.0|>=2.5,<2.5.9|>=2.6,<2.6.2"
     },
     "require-dev": {
-        "symfony/class-loader": "~2.2|~3.0.0",
-        "symfony/http-kernel": "~2.3.24|~2.5.9|^2.6.2|~3.0.0"
+        "symfony/class-loader": "~2.2",
+        "symfony/http-kernel": "~2.3.24|~2.5.9|^2.6.2"
     },
     "autoload": {
         "psr-4": { "Symfony\\Component\\Debug\\": "" },
@@ -35,7 +35,7 @@
     "minimum-stability": "dev",
     "extra": {
         "branch-alias": {
-            "dev-master": "2.8-dev"
+            "dev-master": "2.7-dev"
         }
     }
 }