From c4a74e87254572bd9e30e3b914842c6125b29cd5 Mon Sep 17 00:00:00 2001
From: Kay Joosten <kay.joosten@dawn.tech>
Date: Thu, 16 Jul 2026 13:00:32 +0200
Subject: [PATCH] Prevent session errors from swallowing API error responses
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Why is this change needed?
Prior to this change, ErrorReporter::reportError() only wrapped its
call to storeSessionFeedback() in a finally block, not a catch. On
stateless routes (e.g. the /api/connections metadata push endpoint),
starting the native PHP session while storing feedback info can fail
(e.g. "headers already sent"). That failure propagated out of
reportError(), out of the calling exception listener, and skipped
$event->setResponse() in ApiHttpExceptionListener — so the intended
400/500 JSON error response for a failed push was never set and the
client received an HTTP 200 instead.

How does it address the issue?
This change adds a catch (Throwable) around storeSessionFeedback(),
logging the secondary failure as a warning instead of letting it
escape. reportError() can no longer throw, so every exception
listener that calls it (API, feedback page, fallback) reliably
finishes and sets its intended error response.

https://github.com/OpenConext/OpenConext-engineblock/issues/2051
---
 src/OpenConext/EngineBlockBridge/ErrorReporter.php   |  5 +++++
 .../EngineBlockBridge/ErrorReporterTest.php          | 12 ++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/src/OpenConext/EngineBlockBridge/ErrorReporter.php b/src/OpenConext/EngineBlockBridge/ErrorReporter.php
index 85bd9cb2e..04a0a65bf 100644
--- a/src/OpenConext/EngineBlockBridge/ErrorReporter.php
+++ b/src/OpenConext/EngineBlockBridge/ErrorReporter.php
@@ -27,6 +27,7 @@
 use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface;
 use Psr\Log\LoggerInterface;
 use Symfony\Component\HttpFoundation\RequestStack;
+use Throwable;
 
 class ErrorReporter
 {
@@ -66,6 +67,10 @@ public function reportError(Exception $exception, string $messageSuffix): void
 
         try {
             $this->storeSessionFeedback($exception);
+        } catch (Throwable $sessionException) {
+            $this->logger->warning(
+                'Unable to store feedback info in session: ' . $sessionException->getMessage(),
+            );
         } finally {
             // flush all messages in queue, something went wrong!
             $this->engineBlockApplicationSingleton->flushLog('An error was caught');
diff --git a/tests/unit/OpenConext/EngineBlockBridge/ErrorReporterTest.php b/tests/unit/OpenConext/EngineBlockBridge/ErrorReporterTest.php
index dfe1e810b..fc390936c 100644
--- a/tests/unit/OpenConext/EngineBlockBridge/ErrorReporterTest.php
+++ b/tests/unit/OpenConext/EngineBlockBridge/ErrorReporterTest.php
@@ -142,4 +142,16 @@ public function it_stores_the_pep_policy_decision_in_the_session(): void
 
         self::assertSame($decision, $this->session->get('error_authorization_policy_decision'));
     }
+
+    #[Test]
+    public function it_does_not_propagate_exceptions_thrown_while_storing_feedback_in_the_session(): void
+    {
+        $this->feedbackInfoCollector
+            ->shouldReceive('collect')
+            ->andThrow(new Exception('Failed to start the session because headers have already been sent'));
+
+        $this->applicationSingleton->shouldReceive('flushLog')->with('An error was caught')->once();
+
+        $this->reporter->reportError(new Exception('test'), '');
+    }
 }