Skip to content

Commit 653a13a

Browse files
authored
Prevent session errors from swallowing API error responses (#2059)
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. #2051
1 parent 49265fd commit 653a13a

2 files changed

Lines changed: 17 additions & 0 deletions

File tree

src/OpenConext/EngineBlockBridge/ErrorReporter.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
use OpenConext\EngineBlock\Service\FeedbackStateHelperInterface;
2828
use Psr\Log\LoggerInterface;
2929
use Symfony\Component\HttpFoundation\RequestStack;
30+
use Throwable;
3031

3132
class ErrorReporter
3233
{
@@ -66,6 +67,10 @@ public function reportError(Exception $exception, string $messageSuffix): void
6667

6768
try {
6869
$this->storeSessionFeedback($exception);
70+
} catch (Throwable $sessionException) {
71+
$this->logger->warning(
72+
'Unable to store feedback info in session: ' . $sessionException->getMessage(),
73+
);
6974
} finally {
7075
// flush all messages in queue, something went wrong!
7176
$this->engineBlockApplicationSingleton->flushLog('An error was caught');

tests/unit/OpenConext/EngineBlockBridge/ErrorReporterTest.php

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,4 +142,16 @@ public function it_stores_the_pep_policy_decision_in_the_session(): void
142142

143143
self::assertSame($decision, $this->session->get('error_authorization_policy_decision'));
144144
}
145+
146+
#[Test]
147+
public function it_does_not_propagate_exceptions_thrown_while_storing_feedback_in_the_session(): void
148+
{
149+
$this->feedbackInfoCollector
150+
->shouldReceive('collect')
151+
->andThrow(new Exception('Failed to start the session because headers have already been sent'));
152+
153+
$this->applicationSingleton->shouldReceive('flushLog')->with('An error was caught')->once();
154+
155+
$this->reporter->reportError(new Exception('test'), '');
156+
}
145157
}

0 commit comments

Comments
 (0)