From 367e57e7bc22a6079d20762d9b1e9e43d4355016 Mon Sep 17 00:00:00 2001
From: Dave Long <dave@longwaveconsulting.com>
Date: Sun, 4 Feb 2024 11:56:13 +0000
Subject: [PATCH] Issue #2481349 by mfb, dagmar, jofitz, smustgrave, vasi,
neclimdul, ziomizar, quietone, _utsavsharma, littlepixiez, xjm, fgm, Todd
Zebert, hass, dawehner, heddn, webchick, catch: Prevent the use of
placeholders that cannot be converted into strings when creating logs
---
.../Drupal/Core/Logger/LogMessageParser.php | 11 +++-
.../dblog/tests/src/Kernel/DbLogTest.php | 15 ++++++
.../Core/Logger/LogMessageParserTest.php | 53 ++++++++++++++-----
3 files changed, 63 insertions(+), 16 deletions(-)
diff --git a/core/lib/Drupal/Core/Logger/LogMessageParser.php b/core/lib/Drupal/Core/Logger/LogMessageParser.php
index 3d94f6b77b6e..76fc951d01a1 100644
--- a/core/lib/Drupal/Core/Logger/LogMessageParser.php
+++ b/core/lib/Drupal/Core/Logger/LogMessageParser.php
@@ -29,9 +29,16 @@ public function parseMessagePlaceholders(&$message, array &$context) {
$key = '@' . $key;
}
}
+ // To be considered a valid placeholder, the key should be in
+ // \Drupal\Component\Render\FormattableMarkup style and the variable
+ // should be a string, number, or \Stringable object. For historical
+ // reasons, Boolean and NULL placeholders are also allowed; NULL
+ // placeholders are deprecated and may throw an error in the future.
+ // @see https://www.drupal.org/node/3318826
if (!empty($key) && ($key[0] === '@' || $key[0] === '%' || $key[0] === ':')) {
- // The key is now in \Drupal\Component\Render\FormattableMarkup style.
- $variables[$key] = $variable;
+ if (is_scalar($variable) || is_null($variable) || $variable instanceof \Stringable) {
+ $variables[$key] = $variable;
+ }
}
}
diff --git a/core/modules/dblog/tests/src/Kernel/DbLogTest.php b/core/modules/dblog/tests/src/Kernel/DbLogTest.php
index 59283e795f3f..a36cc29783eb 100644
--- a/core/modules/dblog/tests/src/Kernel/DbLogTest.php
+++ b/core/modules/dblog/tests/src/Kernel/DbLogTest.php
@@ -60,6 +60,21 @@ function (callable $hook, string $module) use (&$implementation_count) {
$this->assertEquals(1, $cron_count, "Cron added $cron_count of 1 new log entries");
}
+ /**
+ * Tests that only valid placeholders are stored in the variables column.
+ */
+ public function testInvalidPlaceholders() {
+ \Drupal::logger('my_module')->warning('Hello @string @array @object', ['@string' => '', '@array' => [], '@object' => new \stdClass()]);
+ $variables = \Drupal::database()
+ ->select('watchdog', 'w')
+ ->fields('w', ['variables'])
+ ->orderBy('wid', 'DESC')
+ ->range(0, 1)
+ ->execute()
+ ->fetchField();
+ $this->assertSame(serialize(['@string' => '']), $variables);
+ }
+
/**
* Runs cron and returns number of new log entries.
*
diff --git a/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php b/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php
index a9a71766c180..537d727fb6cc 100644
--- a/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php
+++ b/core/tests/Drupal/Tests/Core/Logger/LogMessageParserTest.php
@@ -4,6 +4,7 @@
namespace Drupal\Tests\Core\Logger;
+use Drupal\Component\Render\FormattableMarkup;
use Drupal\Core\Logger\LogMessageParser;
use Drupal\Tests\UnitTestCase;
@@ -40,38 +41,62 @@ public function testParseMessagePlaceholders(array $value, array $expected) {
*/
public function providerTestParseMessagePlaceholders() {
return [
- // PSR3 only message.
- [
+ 'PSR3-style placeholder' => [
['message' => 'User {username} created', 'context' => ['username' => 'Dries']],
['message' => 'User @username created', 'context' => ['@username' => 'Dries']],
],
- // PSR3 style mixed in a format_string style message.
- [
+ 'PSR3- and FormattableMarkup-style placeholders' => [
['message' => 'User {username} created @time', 'context' => ['username' => 'Dries', '@time' => 'now']],
['message' => 'User @username created @time', 'context' => ['@username' => 'Dries', '@time' => 'now']],
],
- // format_string style message only.
- [
+ 'FormattableMarkup-style placeholder' => [
['message' => 'User @username created', 'context' => ['@username' => 'Dries']],
['message' => 'User @username created', 'context' => ['@username' => 'Dries']],
],
- // Message without placeholders but wildcard characters.
- [
+ 'Wildcard characters' => [
['message' => 'User W-\\};~{&! created @', 'context' => ['' => '']],
['message' => 'User W-\\};~{&! created @', 'context' => []],
],
- // Message with double PSR3 style messages.
- [
+ 'Multiple PSR3-style placeholders' => [
['message' => 'Test {with} two {{encapsuled}} strings', 'context' => ['with' => 'together', 'encapsuled' => 'awesome']],
['message' => 'Test @with two {@encapsuled} strings', 'context' => ['@with' => 'together', '@encapsuled' => 'awesome']],
],
-
- // Test removal of unexpected placeholders like ! while allowed
- // placeholders beginning with @, % and : are preserved.
- [
+ 'Disallowed placeholder' => [
['message' => 'Test placeholder with :url and old !bang parameter', 'context' => [':url' => 'https://example.com', '!bang' => 'foo bar']],
['message' => 'Test placeholder with :url and old !bang parameter', 'context' => [':url' => 'https://example.com']],
],
+ 'Stringable object placeholder' => [
+ ['message' => 'object @b', 'context' => ['@b' => new FormattableMarkup('convertible', [])]],
+ ['message' => 'object @b', 'context' => ['@b' => 'convertible']],
+ ],
+ 'Non-placeholder context value' => [
+ ['message' => 'message', 'context' => ['not_a_placeholder' => new \stdClass()]],
+ ['message' => 'message', 'context' => []],
+ ],
+ 'Non-stringable array placeholder' => [
+ ['message' => 'array @a', 'context' => ['@a' => []]],
+ ['message' => 'array @a', 'context' => []],
+ ],
+ 'Non-stringable object placeholder' => [
+ ['message' => 'object @b', 'context' => ['@b' => new \stdClass()]],
+ ['message' => 'object @b', 'context' => []],
+ ],
+ 'Non-stringable closure placeholder' => [
+ ['message' => 'closure @c', 'context' => ['@c' => function () {}]],
+ ['message' => 'closure @c', 'context' => []],
+ ],
+ 'Non-stringable resource placeholder' => [
+ ['message' => 'resource @r', 'context' => ['@r' => fopen('php://memory', 'r+')]],
+ ['message' => 'resource @r', 'context' => []],
+ ],
+ 'Non-stringable placeholder is not the first placeholder' => [
+ ['message' => 'mixed @a @b @c', 'context' => ['@a' => 123, '@b' => [1], '@c' => TRUE]],
+ ['message' => 'mixed @a @b @c', 'context' => ['@a' => 123, '@c' => TRUE]],
+ ],
+ 'NULL and Boolean placeholders are considered stringable' => [
+ ['message' => 'mixed @a @b @c', 'context' => ['@a' => NULL, '@b' => TRUE, '@c' => FALSE]],
+ ['message' => 'mixed @a @b @c', 'context' => ['@a' => NULL, '@b' => TRUE, '@c' => FALSE]],
+ ],
];
}
--
GitLab