diff --git a/core/lib/Drupal/Core/Logger/LogMessageParser.php b/core/lib/Drupal/Core/Logger/LogMessageParser.php index 8a67df1..fa223d6 100644 --- a/core/lib/Drupal/Core/Logger/LogMessageParser.php +++ b/core/lib/Drupal/Core/Logger/LogMessageParser.php @@ -29,7 +29,7 @@ public function parseMessagePlaceholders(&$message, array &$context) { $key = '@' . $key; } } - if (!empty($key) && ($key[0] === '@' || $key[0] === '%' || $key[0] === '!')) { + if (!empty($key) && ($key[0] === '@' || $key[0] === '%' || $key[0] === ':')) { // The key is now in \Drupal\Component\Utility\SafeMarkup::format() style. $variables[$key] = $variable; } diff --git a/core/modules/dblog/src/Logger/DbLog.php b/core/modules/dblog/src/Logger/DbLog.php index 97b0b7b..4106f22 100644 --- a/core/modules/dblog/src/Logger/DbLog.php +++ b/core/modules/dblog/src/Logger/DbLog.php @@ -9,6 +9,7 @@ use Drupal\Core\DependencyInjection\DependencySerializationTrait; use Drupal\Core\Logger\LogMessageParserInterface; use Drupal\Core\Logger\RfcLoggerTrait; +use Psr\Log\InvalidArgumentException; use Psr\Log\LoggerInterface; /** @@ -51,6 +52,32 @@ public function __construct(Connection $connection, LogMessageParserInterface $p } /** + * Ensure that all placeholder values are strings. + * + * @param array &$placeholders + * The context to be logged. + */ + protected function stringifyPlaceholders(array &$context) { + foreach ($context as $key => &$value) { + $stringable = is_null($value) || is_scalar($value); + if (is_resource($value)) { + // is_scalar() is documented to possibly return TRUE for resources. + $stringable = FALSE; + } + if (is_object($value) && method_exists($value, '__toString')) { + $stringable = TRUE; + } + + if ($stringable) { + $value = (string)$value; + } + else { + throw new InvalidArgumentException('Log context contains non-string value for key "' . $key . '"'); + } + } + } + + /** * {@inheritdoc} */ public function log($level, $message, array $context = []) { @@ -60,6 +87,7 @@ public function log($level, $message, array $context = []) { // Convert PSR3-style messages to SafeMarkup::format() style, so they can be // translated too in runtime. $message_placeholders = $this->parser->parseMessagePlaceholders($message, $context); + $this->stringifyPlaceholders($message_placeholders); try { $this->connection diff --git a/core/modules/dblog/tests/src/Unit/DbLogTest.php b/core/modules/dblog/tests/src/Unit/DbLogTest.php new file mode 100644 index 0000000..ea5a219 --- /dev/null +++ b/core/modules/dblog/tests/src/Unit/DbLogTest.php @@ -0,0 +1,152 @@ +conn = $this->getMockBuilder('\Drupal\Core\Database\Connection') + ->disableOriginalConstructor() + ->setMethods(['insert', 'fields', 'execute']) + ->getMockForAbstractClass(); + // Return self, so we can chain calls. + $this->conn->expects($this->any())->method($this->anything())->willReturnSelf(); + // Store the variables field in an instance var. This may seem like it's + // abusing internals, but we actually vend the entire contents of this + // table to the outside world in DBLogResource. + $this->conn->method('fields')->will($this->returnCallback(function($fields) { + $this->variables = unserialize($fields['variables']); + return $this->conn; + })); + + $parser = new LogMessageParser(); + $this->dblog = new DbLog($this->conn, $parser); + + $this->channel = new LoggerChannel('test'); + $this->channel->addLogger($this->dblog); + } + + /** + * Test that we enforce string-ish placeholder substitutions. + */ + public function testPlaceholderValidation() { + foreach ($this->methods as $method) { + $this->assertLogThrows(FALSE, $method, 'no parameters'); + $this->assertLogThrows(FALSE, $method, ':h href parameter', + [':h' => 'string']); + $this->assertLogThrows(FALSE, $method, '%em int em parameter', + ['%em' => 123]); + $this->assertSame('123', $this->variables['%em'], + "Parameter was converted to string"); + $this->assertNotSame(123, $this->variables['%em'], + "Parameter does not remain an int"); + $this->assertLogThrows(FALSE, $method, '@gen float general parameter', + ['@gen' => -5.4]); + $this->assertLogThrows(FALSE, $method, + 'multiple convertible params: @a :b %c', + ['@a' => NULL, ':b' => TRUE, '%c' => 'foo']); + $this->assertSame('', $this->variables['@a'], + "Null was converted to string"); + $this->assertLogThrows(FALSE, $method, 'psr {param}', + ['param' => 'foo']); + $this->assertLogThrows(FALSE, $method, 'convertible @object', + ['@object' => new StringConvertible()]); + $this->assertSame('convertible', $this->variables['@object'], + "Object was converted to string"); + + $this->assertLogThrows(TRUE, $method, 'array @a', ['@a' => []]); + $this->assertLogThrows(TRUE, $method, 'obj :b', + [':b' => new NotStringConvertible()]); + $this->assertLogThrows(TRUE, $method, 'closure %c', + ['%c' => function() { }]); + $this->assertLogThrows(TRUE, $method, 'resource @a', + ['@a' => fopen('php://memory', 'r+')]); + $this->assertLogThrows(TRUE, $method, 'mixed @a @b @c', + ['@a' => 123, '@b' => [1], '@c' => TRUE]); + } + } + + /** + * Assert that logging something does (or does not) throw an exception. + */ + protected function assertLogThrows($throws, $method, $message, $context = array()) { + try { + $this->channel->$method($message, $context); + $this->assertFalse($throws, "Logging '$message' does not throw"); + } + catch (InvalidArgumentException $e) { + $this->assertTrue($throws, "Logging '$message'' throws"); + } + } + +}