Issue #2481349 by mfb, dagmar, jofitz, smustgrave, vasi, neclimdul, ziomizar,...

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

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;
+        }
       }
     }
 

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.
    *

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]],
+      ],
     ];
   }