diff --git a/src/Infrastructure/Functions.php b/src/Infrastructure/Functions.php index 2365ba670..65ba81d0e 100644 --- a/src/Infrastructure/Functions.php +++ b/src/Infrastructure/Functions.php @@ -174,7 +174,10 @@ function processException(Throwable $exception): void sprintf( "(P) %s\n%s", __($previous->getMessage()), - $previous->getTraceAsString() + // The same formatter the line above uses, and for the same reason: + // `getTraceAsString()` prints argument values, so a secret passed to any frame on + // the way to the throw point lands in the log file. + formatStackTrace($previous) ), 'EXCEPTION' ); diff --git a/src/Infrastructure/Log/Providers/DatabaseHandler.php b/src/Infrastructure/Log/Providers/DatabaseHandler.php index bc7db5381..d3314ffdc 100644 --- a/src/Infrastructure/Log/Providers/DatabaseHandler.php +++ b/src/Infrastructure/Log/Providers/DatabaseHandler.php @@ -38,6 +38,7 @@ use SP\Application\Security\Ports\EventlogService; use Throwable; +use function SP\formatStackTrace; use function SP\processException; /** @@ -90,7 +91,30 @@ public function update(Event $event): void if ($source instanceof Throwable) { $properties['level'] = 'ERROR'; - $properties['description'] = (string)$source; + + // PHP's default `Exception::__toString()` embeds `getTraceAsString()`, which prints + // each frame's **argument values** rather than their types — the first 15 characters of + // every string on the stack. The chains that throw into this sink include the crypt and + // database layers and the LDAP providers, so a master password, an account password or + // a bind credential can be an argument on the way to the throw point. This row is + // readable by anyone whose profile has `isEvl()`, and the event log can be searched and + // exported. + // + // The header is kept as each exception renders it and only the trace is replaced, with + // `formatStackTrace()` — the same trace reduced to argument *types*, which + // `processException()` has always used for exactly this reason. + // + // Worth knowing while reading this: `SPException::__toString()` overrides PHP's and + // emits no trace at all, so the application's own exception type was never the leaky + // one. What reaches here carrying a trace is a `RuntimeException`, a `PDOException`, a + // `TypeError` or a library's own — which is precisely the set that fails inside crypt + // and database calls. + $rendered = (string)$source; + [$head] = explode("\nStack trace:\n", $rendered, 2); + + $properties['description'] = $head === $rendered + ? $rendered + : sprintf("%s\n%s", $head, formatStackTrace($source)); } else { $properties['description'] = $event->getEventMessage()?->composeText(); } diff --git a/tests/Unit/Infrastructure/Log/Providers/DatabaseHandlerTest.php b/tests/Unit/Infrastructure/Log/Providers/DatabaseHandlerTest.php index 486b4a38e..0b36067d0 100644 --- a/tests/Unit/Infrastructure/Log/Providers/DatabaseHandlerTest.php +++ b/tests/Unit/Infrastructure/Log/Providers/DatabaseHandlerTest.php @@ -50,6 +50,8 @@ #[AllowMockObjectsWithoutExpectations] class DatabaseHandlerTest extends UnitaryTestCase { + private const A_SECRET = 'SuperSecretMasterPassword123'; + private MockObject|EventlogService $eventLogService; private MockObject|LanguageInterface $language; private DatabaseHandler $databaseHandler; @@ -164,6 +166,66 @@ public function testUpdateWithSPExceptionMessage() $this->databaseHandler->update($event); } + /** + * A logged exception records what went wrong, and none of the values that were on the stack. + * + * The row used to be `(string)$source`, and `Exception::__toString()` embeds + * `getTraceAsString()`, which prints each frame's **argument values** — the first 15 characters + * of every string. The chains that throw into this sink include the crypt and database layers + * and the LDAP providers, so a master password, an account password or a bind credential can be + * an argument on the way to the throw point; the row is readable by anyone whose profile has + * `isEvl()`, and the event log can be searched and exported. + * + * `formatStackTrace()` is the same trace with every argument reduced to its type, and + * `processException()` has always used it for exactly this reason. + * + * Whether a trace carries arguments at all is an ini setting that differs between a development + * build and a production one, so it is pinned here rather than assumed — `FunctionsTest` does + * the same, and without it this passes locally and proves nothing wherever the production ini + * is in force. + */ + public function testALoggedExceptionCarriesNoArgumentValues() + { + $ignoreArgs = ini_get('zend.exception_ignore_args'); + ini_set('zend.exception_ignore_args', '0'); + + $description = null; + + $this->eventLogService + ->expects($this->once()) + ->method('create') + ->willReturnCallback( + static function (Eventlog $eventlog) use (&$description): int { + $description = $eventlog->getDescription(); + + return 1; + } + ); + + try { + $throw = static function (string $masterPassword, string $accountKey): void { + throw new RuntimeException('could not decrypt'); + }; + + try { + $throw(self::A_SECRET, 'an-account-key'); + } catch (RuntimeException $e) { + $this->databaseHandler->update(new Event('test_a.update', $e)); + } + } finally { + ini_set('zend.exception_ignore_args', (string)$ignoreArgs); + } + + self::assertIsString($description); + self::assertStringNotContainsString(substr(self::A_SECRET, 0, 15), $description); + self::assertStringNotContainsString('an-account-key', $description); + + // ...and it is still an account of what happened, or withholding the arguments would have + // been achieved just as well by logging nothing. + self::assertStringContainsString('could not decrypt', $description); + self::assertStringContainsString('String', $description, 'arguments are recorded by type'); + } + /** * @throws InvalidClassException */