From a75a1c62163be8d5241c00cbac94b7bd5993ceed Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 17 Sep 2026 03:26:26 +0200 Subject: [PATCH] fix: a log that cannot be written does not fail the request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LoggerBase::update() — the file/syslog receiver — had no guard around its body, and it is the one receiver attached on every request, before the install check and regardless of any config flag. Its three siblings all catch and hand to processException(), and two of them are config-gated as well. Monolog's StreamHandler throws when var/syspass.log cannot be opened or appended to, and notify() is always called after the work it describes, so that exception propagated out of an operation that had already completed. Router turns it into a generic error response, which means an administrator is told a master-password rotation failed when it had already finished — the one outcome that must never be ambiguous. unsetAppLocales() was skipped on that path too. The body moves into a private writeEvent() and update() wraps it the way the siblings do, with a finally so the locale is restored either way. Throwable rather than the siblings' Exception, because a stream failure can surface as an Error and processException() accepts either; and processException() is safe to call from here because logger() writes with a suppressed file_put_contents() and falls back to error_log(), so it does not come back through Monolog. That this receiver is attached unconditionally is left alone: the file log is the one that has to work before the database and the config are usable. The defect was that it could not fail safely, not that it runs. --- .../Log/Providers/LoggerBase.php | 35 +++++++++++++++++-- .../Log/Providers/LogHandlerTest.php | 27 ++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/src/Infrastructure/Log/Providers/LoggerBase.php b/src/Infrastructure/Log/Providers/LoggerBase.php index d410f9967..ff76d4a6c 100644 --- a/src/Infrastructure/Log/Providers/LoggerBase.php +++ b/src/Infrastructure/Log/Providers/LoggerBase.php @@ -27,6 +27,7 @@ namespace SP\Infrastructure\Log\Providers; use Exception; +use Throwable; use Psr\Log\LoggerInterface; use SP\Application\Application; use SP\Domain\Core\Events\Event; @@ -37,6 +38,7 @@ use SP\Domain\Core\LanguageInterface; use SP\Domain\Http\Ports\RequestService; +use function SP\processException; use function SP\__; use function SP\getLastCaller; @@ -78,6 +80,37 @@ public function update(Event $event): void { $this->language->setAppLocales(); + try { + $this->writeEvent($event); + } catch (Throwable $e) { + // A log that cannot be written must not fail the thing it was reporting on. + // + // This receiver is attached on every request, before the install check and regardless + // of any config flag, and it was the only one of the four with no guard — + // `DatabaseHandler`, `MailEvent` and `NotificationEvent` all catch and hand to + // `processException()`. Monolog's `StreamHandler` throws when `var/syspass.log` cannot + // be opened or appended to, and `notify()` is always called *after* the work it + // describes, so a full disk turned a completed operation into an error response: the + // administrator is told a master-password rotation failed when it had already + // finished, which is the one thing that must never be ambiguous. + // + // `processException()` is safe to call from here: `logger()` writes with a suppressed + // `file_put_contents()` and falls back to `error_log()`, so it does not come back + // through Monolog. + // + // `Throwable` rather than the siblings' `Exception`, because a stream failure can + // surface as an `Error`; `processException()` accepts either. + processException($e); + } finally { + $this->language->unsetAppLocales(); + } + } + + /** + * @throws InvalidClassException + */ + private function writeEvent(Event $event): void + { $eventName = $event->getName(); $userLogin = 'N/A'; @@ -115,8 +148,6 @@ public function update(Event $event): void ) ); } - - $this->language->unsetAppLocales(); } /** diff --git a/tests/Unit/Infrastructure/Log/Providers/LogHandlerTest.php b/tests/Unit/Infrastructure/Log/Providers/LogHandlerTest.php index e362829cd..d0198b5c4 100644 --- a/tests/Unit/Infrastructure/Log/Providers/LogHandlerTest.php +++ b/tests/Unit/Infrastructure/Log/Providers/LogHandlerTest.php @@ -131,6 +131,33 @@ public function testUpdateWithNoMessage() $this->logHandler->update($event); } + /** + * A log that cannot be written does not fail the thing it was reporting on. + * + * This receiver is attached on every request — before the install check, and regardless of any + * config flag — and it was the only one of the four with no guard around `update()`: + * `DatabaseHandler`, `MailEvent` and `NotificationEvent` all catch and hand to + * `processException()`. Monolog's `StreamHandler` throws when `var/syspass.log` cannot be + * opened or appended to, and `notify()` is always called *after* the work it describes, so a + * full disk turned a completed operation into an error response — the administrator is told a + * master-password rotation failed when it had already finished. + */ + public function testAFailingLoggerDoesNotFailTheRequest() + { + $this->logger + ->expects($this->once()) + ->method('debug') + ->willThrowException(new RuntimeException('could not write to var/syspass.log')); + + // And the locales are still put back, which a plain try/catch around the call would miss. + $this->language->expects($this->once())->method('setAppLocales'); + $this->language->expects($this->once())->method('unsetAppLocales'); + + $this->logHandler->update(new Event('test.event', $this)); + + self::assertTrue(true, 'update() returned rather than propagating'); + } + /** * @throws InvalidClassException */