[BUGFIX] Mitigate exceptions in `LogDataTrait::formatLogDetailsStatic`

`LogDataTrait::formatLogDetailsStatic()` simple used `vsprintf()`
in case that the string contains any `%` characters and throws
either a `\ValueError` or `\ArgumentCountError` depending if no
value exists or the count does not match.

It is not reliable and implement additional lexing here to avoid
the exception by not calling `vsprintf()` upfront.

This change calls `vsprintf()` and handles the mentioned exceptions
and use the fallback handling in case exception has been thrown as
a simplefied approach. Tests are added to cover legacy, mixed and
new log format option along with static and instanced method calls.

Note that a minor inline PHP DocBlock annotation has been added
in `LogDataTrait` to mitigate detection issues of PHPStan, which
allows now to remove baseline ignore patterns in the same run.

Used command(s):

> Build/Scripts/runTests.sh -s phpstanGenerateBaseline

Resolves: #103711
Releases: main, 12.4
Change-Id: I7b23eccdc944d2cb068f584b01b52835aca938e9
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/84004


Tested-by: core-ci <typo3@b13.com>
Tested-by: Garvin Hicking <gh@faktor-e.de>
Tested-by: Stefan Bürk <stefan@buerk.tech>
Reviewed-by: Garvin Hicking <gh@faktor-e.de>
Reviewed-by: Stefan Bürk <stefan@buerk.tech>

Build/phpstan/phpstan-baseline.neon

@@ -20,11 +20,6 @@ parameters:
 			count: 1
 			path: ../../typo3/sysext/backend/Classes/Controller/EditDocumentController.php
 
-		-
-			message: "#^Offset 1 on array\\{string, non\\-empty\\-string\\} on left side of \\?\\? always exists and is not nullable\\.$#"
-			count: 1
-			path: ../../typo3/sysext/backend/Classes/EventListener/FailedLoginAttemptNotification.php
-
 		-
 			message: "#^Offset 'eval' on array on left side of \\?\\? always exists and is not nullable\\.$#"
 			count: 1
@@ -180,16 +175,6 @@ parameters:
 			count: 1
 			path: ../../typo3/sysext/backend/Classes/Utility/BackendUtility.php
 
-		-
-			message: "#^Offset 1 on array\\{string, non\\-empty\\-string\\} on left side of \\?\\? always exists and is not nullable\\.$#"
-			count: 1
-			path: ../../typo3/sysext/belog/Classes/Domain/Model/LogEntry.php
-
-		-
-			message: "#^Offset 1 on array\\{string, non\\-empty\\-string\\} on left side of \\?\\? always exists and is not nullable\\.$#"
-			count: 1
-			path: ../../typo3/sysext/belog/Classes/ViewHelpers/FormatDetailsViewHelper.php
-
 		-
 			message: "#^Property TYPO3\\\\CMS\\\\Core\\\\Authentication\\\\AbstractUserAuthentication\\:\\:\\$writeAttemptLog \\(bool\\) on left side of \\?\\? is not nullable\\.$#"
 			count: 1
@@ -280,11 +265,6 @@ parameters:
 			count: 1
 			path: ../../typo3/sysext/core/Classes/Crypto/HashService.php
 
-		-
-			message: "#^Offset 1 on array\\{string, non\\-empty\\-string\\} on left side of \\?\\? always exists and is not nullable\\.$#"
-			count: 1
-			path: ../../typo3/sysext/core/Classes/DataHandling/DataHandler.php
-
 		-
 			message: "#^Method TYPO3\\\\CMS\\\\Core\\\\DataHandling\\\\Localization\\\\DataMapProcessor\\:\\:fetchDependencies\\(\\) should return array\\<array\\<TYPO3\\\\CMS\\\\Core\\\\DataHandling\\\\Localization\\\\DataMapItem\\>\\> but returns array\\<int\\|string, array\\<string, array\\<int, TYPO3\\\\CMS\\\\Core\\\\DataHandling\\\\Localization\\\\DataMapItem\\>\\>\\>\\.$#"
 			count: 1
@@ -725,11 +705,6 @@ parameters:
 			count: 1
 			path: ../../typo3/sysext/core/Tests/Functional/Cache/Backend/RedisBackendTest.php
 
-		-
-			message: "#^Offset 1 on array\\{string, non\\-empty\\-string\\} on left side of \\?\\? always exists and is not nullable\\.$#"
-			count: 1
-			path: ../../typo3/sysext/core/Tests/Functional/DataScenarios/AbstractDataHandlerActionTestCase.php
-
 		-
 			message: "#^Parameter \\#2 \\$data of method TYPO3\\\\CMS\\\\Core\\\\Cache\\\\Backend\\\\Typo3DatabaseBackend\\:\\:set\\(\\) expects string, array\\<int, string\\> given\\.$#"
 			count: 1
@@ -1630,16 +1605,6 @@ parameters:
 			count: 1
 			path: ../../typo3/sysext/install/Classes/Updates/DatabaseRowsUpdateWizard.php
 
-		-
-			message: "#^Offset 1 on array\\{string, non\\-empty\\-string\\} on left side of \\?\\? always exists and is not nullable\\.$#"
-			count: 1
-			path: ../../typo3/sysext/install/Classes/Updates/SysLogSerializationUpdate.php
-
-		-
-			message: "#^Offset 1 on array\\{string, non\\-empty\\-string\\} on left side of \\?\\? always exists and is not nullable\\.$#"
-			count: 1
-			path: ../../typo3/sysext/lowlevel/Classes/Command/ListSysLogCommand.php
-
 		-
 			message: "#^Negated boolean expression is always true\\.$#"
 			count: 1
@@ -1654,8 +1619,3 @@ parameters:
 			message: "#^Strict comparison using \\=\\=\\= between non\\-falsy\\-string and '' will always evaluate to false\\.$#"
 			count: 1
 			path: ../../typo3/sysext/scheduler/Classes/CronCommand/NormalizeCommand.php
-
-		-
-			message: "#^Offset 1 on array\\{string, non\\-empty\\-string\\} on left side of \\?\\? always exists and is not nullable\\.$#"
-			count: 1
-			path: ../../typo3/sysext/workspaces/Classes/Controller/Remote/RemoteServer.php

typo3/sysext/core/Classes/Log/LogDataTrait.php

@@ -55,17 +55,21 @@ trait LogDataTrait
      */
     protected static function formatLogDetailsStatic(string $detailString, array $substitutes): string
     {
-        // Handle legacy "%s" placeholders
-        if (str_contains($detailString, '%')) {
+        // Handles placeholders with "%" first
+        try {
             $detailString = vsprintf($detailString, $substitutes);
-        } elseif ($substitutes !== []) {
-            // Handles placeholders with "{myPlaceholder}"
-            $detailString = preg_replace_callback('/{([A-z]+)}/', static function ($matches) use ($substitutes) {
-                // $matches[0] contains the unsubstituted placeholder
-                return $substitutes[$matches[1] ?? null] ?? $matches[0];
-            }, $detailString);
+        } catch (\ValueError|\ArgumentCountError) {
+            // Ignore if $substitutes doesn't contain the number of "%" found in $detailString
         }
-        // Remove possible pending other %s
+
+        // Handles placeholders with "{myPlaceholder}"
+        $detailString = preg_replace_callback('/{([A-z]+)}/', static function (array $matches) use ($substitutes) {
+            // $matches[0] contains the unsubstituted placeholder
+            /** @var array{0: string, 1?: string} $matches added to mitigate false-positives PHPStan reportings */
+            return $substitutes[$matches[1] ?? null] ?? $matches[0];
+        }, $detailString);
+
+        // Remove possible pending %s
         return str_replace('%s', '', (string)$detailString);
     }
 }

typo3/sysext/core/Tests/Unit/Log/Fixtures/LogDataTraitTestAccessor.php

+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+namespace TYPO3\CMS\Core\Tests\Unit\Log\Fixtures;
+
+use TYPO3\CMS\Core\Log\LogDataTrait;
+
+final class LogDataTraitTestAccessor
+{
+    use LogDataTrait;
+
+    public function callUnserializeLogData(mixed $logData): ?array
+    {
+        return $this->unserializeLogData($logData);
+    }
+
+    public function callFormatLogDetails(string $detailString, mixed $substitutes): string
+    {
+        return $this->formatLogDetails($detailString, $substitutes);
+    }
+
+    public static function callFormatLogDetailsStatic(string $detailString, array $substitutes): string
+    {
+        return self::formatLogDetailsStatic($detailString, $substitutes);
+    }
+}

typo3/sysext/core/Tests/Unit/Log/LogDataTraitTest.php

+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the TYPO3 CMS project.
+ *
+ * It is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License, either version 2
+ * of the License, or any later version.
+ *
+ * For the full copyright and license information, please read the
+ * LICENSE.txt file that was distributed with this source code.
+ *
+ * The TYPO3 project - inspiring people to share!
+ */
+
+namespace TYPO3\CMS\Core\Tests\Unit\Log;
+
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\Attributes\Test;
+use TYPO3\CMS\Core\Tests\Unit\Log\Fixtures\LogDataTraitTestAccessor;
+use TYPO3\TestingFramework\Core\Unit\UnitTestCase;
+
+final class LogDataTraitTest extends UnitTestCase
+{
+    public static function formatLogDetailsLegacyStringsDataProvider(): \Generator
+    {
+        yield 'String with percent and empty argument %' => [
+            'detailString' => 'String with percent and empty argument %',
+            'substitutes' => [],
+            'expectedResult' => 'String with percent and empty argument %',
+        ];
+        yield 'String with string %s and empty argument' => [
+            'detailString' => 'String with string %s and empty argument',
+            'substitutes' => [],
+            'expectedResult' => 'String with string  and empty argument',
+        ];
+        yield 'String with decimal %d and empty argument' => [
+            'detailString' => 'String with decimal %d and empty argument',
+            'substitutes' => [],
+            'expectedResult' => 'String with decimal %d and empty argument',
+        ];
+        yield 'String with percent and single argument %' => [
+            'detailString' => 'String with percent and single argument %',
+            'substitutes' => [0 => 'a argument'],
+            'expectedResult' => 'String with percent and single argument %',
+        ];
+        yield 'String with string %s and single argument' => [
+            'detailString' => 'String with string %s and single argument',
+            'substitutes' => [0 => 'this is a string'],
+            'expectedResult' => 'String with string this is a string and single argument',
+        ];
+        yield 'String with decimal %d and single argument' => [
+            'detailString' => 'String with decimal %d and single argument',
+            'substitutes' => [0 => 42],
+            'expectedResult' => 'String with decimal 42 and single argument',
+        ];
+        yield 'String with percent and multiple arguments %' => [
+            'detailString' => 'String with percent and multiple arguments %',
+            'substitutes' => [0 => 'a argument', 1 => 'another argument'],
+            'expectedResult' => 'String with percent and multiple arguments %',
+        ];
+        yield 'String with string %s and multiple arguments %s' => [
+            'detailString' => 'String with string %s and multiple arguments %s',
+            'substitutes' => [0 => 'this is a string', 1 => 'another string'],
+            'expectedResult' => 'String with string this is a string and multiple arguments another string',
+        ];
+        yield 'String with decimal %d and multiple arguments %s' => [
+            'detailString' => 'String with decimal %d and multiple arguments %s',
+            'substitutes' => [0 => 42, 1 => 'another string'],
+            'expectedResult' => 'String with decimal 42 and multiple arguments another string',
+        ];
+        yield 'String with percent and to many arguments %' => [
+            'detailString' => 'String with percent and to many arguments %',
+            'substitutes' => [0 => 'a argument', 1 => 'another argument', 2 => 'a third string'],
+            'expectedResult' => 'String with percent and to many arguments %',
+        ];
+        yield 'String with string %s and to many arguments %s' => [
+            'detailString' => 'String with string %s and to many arguments %s',
+            'substitutes' => [0 => 'this is a string', 1 => 'another string', 2 => 'a third string'],
+            'expectedResult' => 'String with string this is a string and to many arguments another string',
+        ];
+        yield 'String with decimal %d and to many arguments %s' => [
+            'detailString' => 'String with decimal %d and to many arguments %s',
+            'substitutes' => [0 => 42, 1 => 'another string', 2 => 'a third string'],
+            'expectedResult' => 'String with decimal 42 and to many arguments another string',
+        ];
+        // %s is special since it's replaced with empty string now matter what
+        yield 'String with string %s %s and empty argument' => [
+            'detailString' => 'String with string %s %s and empty argument',
+            'substitutes' => [],
+            'expectedResult' => 'String with string   and empty argument',
+        ];
+        yield 'String with string %s %s and to few arguments' => [
+            'detailString' => 'String with string %s %s and to few arguments',
+            'substitutes' => [0 => 'astring'],
+            'expectedResult' => 'String with string   and to few arguments',
+        ];
+        yield 'String with string %s %s and to many arguments' => [
+            'detailString' => 'String with string %s %s and to many arguments',
+            'substitutes' => [0 => 'astring', 1 => 'another string', 2 => 'a third string'],
+            'expectedResult' => 'String with string astring another string and to many arguments',
+        ];
+    }
+
+    #[DataProvider('formatLogDetailsLegacyStringsDataProvider')]
+    #[Test]
+    public function formatLogDetailsStaticLegacyStrings(string $detailString, array $substitutes, string $expectedResult): void
+    {
+        self::assertSame($expectedResult, LogDataTraitTestAccessor::callFormatLogDetailsStatic($detailString, $substitutes));
+    }
+
+    #[DataProvider('formatLogDetailsLegacyStringsDataProvider')]
+    #[Test]
+    public function formatLogDetailsLegacyStrings(string $detailString, array $substitutes, string $expectedResult): void
+    {
+        self::assertSame($expectedResult, (new LogDataTraitTestAccessor())->callFormatLogDetails($detailString, $substitutes));
+    }
+
+    #[DataProvider('formatLogDetailsLegacyStringsDataProvider')]
+    #[Test]
+    public function formatLogDetailsLegacyStringsWithSerializedSubstitutes(string $detailString, array $substitutes, string $expectedResult): void
+    {
+        self::assertSame($expectedResult, (new LogDataTraitTestAccessor())->callFormatLogDetails($detailString, serialize($substitutes)));
+    }
+
+    #[DataProvider('formatLogDetailsLegacyStringsDataProvider')]
+    #[Test]
+    public function formatLogDetailsLegacyStringsWithJsonEncodedSubstitutes(string $detailString, array $substitutes, string $expectedResult): void
+    {
+        self::assertSame($expectedResult, (new LogDataTraitTestAccessor())->callFormatLogDetails($detailString, json_encode($substitutes)));
+    }
+
+    public static function formatLogDetailsNewFormatStringsDataProvider(): \Generator
+    {
+        yield 'Empty arguments {aPlaceholder}' => [
+            'detailString' => 'Empty arguments {aPlaceholder}',
+            'substitutes' => [],
+            'expectedResult' => 'Empty arguments {aPlaceholder}',
+        ];
+        yield 'Non existing arguments {aPlaceholder}' => [
+            'detailString' => 'Non existing arguments {aPlaceholder}',
+            'substitutes' => ['non-existing-argument' => 'non-existing-argument'],
+            'expectedResult' => 'Non existing arguments {aPlaceholder}',
+        ];
+        yield 'Single argument {myPlaceholder}' => [
+            'detailString' => 'Single argument {myPlaceholder}',
+            'substitutes' => ['myPlaceholder' => 'replacedPlacerHolder'],
+            'expectedResult' => 'Single argument replacedPlacerHolder',
+        ];
+        yield 'Multiple argument {myPlaceholder} {anotherPlaceholder}' => [
+            'detailString' => 'Multiple argument {myPlaceholder} {anotherPlaceholder}',
+            'substitutes' => ['myPlaceholder' => 'replacedPlacerHolder', 'anotherPlaceholder' => 'replacedAnotherPlaceholder'],
+            'expectedResult' => 'Multiple argument replacedPlacerHolder replacedAnotherPlaceholder',
+        ];
+        yield 'Multiple argument {myPlaceholder} {anotherPlaceholder} to many arguments' => [
+            'detailString' => 'Multiple argument {myPlaceholder} {anotherPlaceholder} to many arguments',
+            'substitutes' => ['non-existing-argument' => 'non-existing-argument', 'myPlaceholder' => 'replacedPlacerHolder', 'anotherPlaceholder' => 'replacedAnotherPlaceholder'],
+            'expectedResult' => 'Multiple argument replacedPlacerHolder replacedAnotherPlaceholder to many arguments',
+        ];
+    }
+
+    #[DataProvider('formatLogDetailsNewFormatStringsDataProvider')]
+    #[Test]
+    public function formatLogDetailsStaticNewFormatStrings(string $detailString, array $substitutes, string $expectedResult): void
+    {
+        self::assertSame($expectedResult, LogDataTraitTestAccessor::callFormatLogDetailsStatic($detailString, $substitutes));
+    }
+
+    #[DataProvider('formatLogDetailsNewFormatStringsDataProvider')]
+    #[Test]
+    public function formatLogDetailsNewFormatStrings(string $detailString, array $substitutes, string $expectedResult): void
+    {
+        self::assertSame($expectedResult, (new LogDataTraitTestAccessor())->callFormatLogDetails($detailString, $substitutes));
+    }
+
+    #[DataProvider('formatLogDetailsNewFormatStringsDataProvider')]
+    #[Test]
+    public function formatLogDetailsNewFormatStringsWithSerializedSubstitutes(string $detailString, array $substitutes, string $expectedResult): void
+    {
+        self::assertSame($expectedResult, (new LogDataTraitTestAccessor())->callFormatLogDetails($detailString, serialize($substitutes)));
+    }
+
+    #[DataProvider('formatLogDetailsNewFormatStringsDataProvider')]
+    #[Test]
+    public function formatLogDetailsNewFormatStringsWithJsonEncodedSubstitutes(string $detailString, array $substitutes, string $expectedResult): void
+    {
+        self::assertSame($expectedResult, (new LogDataTraitTestAccessor())->callFormatLogDetails($detailString, json_encode($substitutes)));
+    }
+
+    public static function formatLogDetailsMixedFormatStringsDataProvider(): \Generator
+    {
+        yield 'Mixed empty arguments % %s {aPlaceholder}' => [
+            'detailString' => 'Mixed empty arguments % %s {aPlaceholder}',
+            'substitutes' => [],
+            'expectedResult' => 'Mixed empty arguments %  {aPlaceholder}',
+        ];
+        yield 'Mixed non existing arguments % %s {aPlaceholder}' => [
+            'detailString' => 'Mixed non existing arguments % %s {aPlaceholder}',
+            'substitutes' => ['non-existing-argument' => 'non-existing-argument'],
+            'expectedResult' => 'Mixed non existing arguments  {aPlaceholder}',
+        ];
+        yield 'Mixed Single argument % %s {myPlaceholder}' => [
+            'detailString' => 'Mixed Single argument % %s {myPlaceholder}',
+            'substitutes' => ['myPlaceholder' => 'replacedPlacerHolder'],
+            'expectedResult' => 'Mixed Single argument  replacedPlacerHolder',
+        ];
+        yield 'Mixed multiple argument % %s {myPlaceholder} {anotherPlaceholder}' => [
+            'detailString' => 'Mixed multiple argument % %s {myPlaceholder} {anotherPlaceholder}',
+            'substitutes' => ['myPlaceholder' => 'replacedPlacerHolder', 'anotherPlaceholder' => 'replacedAnotherPlaceholder'],
+            'expectedResult' => 'Mixed multiple argument  replacedPlacerHolder replacedAnotherPlaceholder',
+        ];
+        yield 'Mixed multiple argument % %s {myPlaceholder} {anotherPlaceholder} to many arguments' => [
+            'detailString' => 'Mixed multiple argument % %s {myPlaceholder} {anotherPlaceholder} to many arguments',
+            'substitutes' => ['non-existing-argument' => 'non-existing-argument', 'myPlaceholder' => 'replacedPlacerHolder', 'anotherPlaceholder' => 'replacedAnotherPlaceholder'],
+            'expectedResult' => 'Mixed multiple argument  replacedPlacerHolder replacedAnotherPlaceholder to many arguments',
+        ];
+    }
+
+    #[DataProvider('formatLogDetailsMixedFormatStringsDataProvider')]
+    #[Test]
+    public function formatLogDetailsStaticMixedFormatStrings(string $detailString, array $substitutes, string $expectedResult): void
+    {
+        self::assertSame($expectedResult, LogDataTraitTestAccessor::callFormatLogDetailsStatic($detailString, $substitutes));
+    }
+
+    #[DataProvider('formatLogDetailsMixedFormatStringsDataProvider')]
+    #[Test]
+    public function formatLogDetailsMixedFormatStrings(string $detailString, array $substitutes, string $expectedResult): void
+    {
+        self::assertSame($expectedResult, (new LogDataTraitTestAccessor())->callFormatLogDetails($detailString, $substitutes));
+    }
+
+    #[DataProvider('formatLogDetailsMixedFormatStringsDataProvider')]
+    #[Test]
+    public function formatLogDetailsMixedFormatStringsWithSerializedSubstitutes(string $detailString, array $substitutes, string $expectedResult): void
+    {
+        self::assertSame($expectedResult, (new LogDataTraitTestAccessor())->callFormatLogDetails($detailString, serialize($substitutes)));
+    }
+
+    #[DataProvider('formatLogDetailsMixedFormatStringsDataProvider')]
+    #[Test]
+    public function formatLogDetailsMixedFormatStringsWithJsonEncodedSubstitutes(string $detailString, array $substitutes, string $expectedResult): void
+    {
+        self::assertSame($expectedResult, (new LogDataTraitTestAccessor())->callFormatLogDetails($detailString, json_encode($substitutes)));
+    }
+}