From ae439236444473612d3e581d9b607c2e6ea1ba1f Mon Sep 17 00:00:00 2001 From: Beau Simensen Date: Wed, 18 Oct 2023 11:31:27 -0500 Subject: [PATCH] [10.x] Do not bubble exceptions thrown rendering error view when debug is false (prevent infinite loops) (#48732) * Do not bubble exceptions thrown rendering error view when debug is false * Add failing tests * Only re-throw when debug mode it `true` * Manually report exception if debug mode is `false` * Remove unit tests * lint * Revert "Remove unit tests" This reverts commit f4222ef7648fcac2ceb9e41a1758aecf43ba5c4e. * Fix original unit tests * Fix paths for windows --------- Co-authored-by: Tim MacDonald --- .../Foundation/Exceptions/Handler.php | 14 ++++-- .../FoundationExceptionsHandlerTest.php | 45 ++++++++++++++++++- .../Foundation/ExceptionHandlerTest.php | 45 +++++++++++++++++++ .../MalformedErrorViews/errors/404.blade.php | 2 + .../MalformedErrorViews/errors/500.blade.php | 2 + 5 files changed, 103 insertions(+), 5 deletions(-) create mode 100644 tests/Integration/Foundation/Fixtures/MalformedErrorViews/errors/404.blade.php create mode 100644 tests/Integration/Foundation/Fixtures/MalformedErrorViews/errors/500.blade.php diff --git a/src/Illuminate/Foundation/Exceptions/Handler.php b/src/Illuminate/Foundation/Exceptions/Handler.php index 4d78a1a00f5d..498be561b7dc 100644 --- a/src/Illuminate/Foundation/Exceptions/Handler.php +++ b/src/Illuminate/Foundation/Exceptions/Handler.php @@ -719,10 +719,16 @@ protected function renderHttpException(HttpExceptionInterface $e) $this->registerErrorViewPaths(); if ($view = $this->getHttpExceptionView($e)) { - return response()->view($view, [ - 'errors' => new ViewErrorBag, - 'exception' => $e, - ], $e->getStatusCode(), $e->getHeaders()); + try { + return response()->view($view, [ + 'errors' => new ViewErrorBag, + 'exception' => $e, + ], $e->getStatusCode(), $e->getHeaders()); + } catch (Throwable $t) { + config('app.debug') && throw $t; + + $this->report($t); + } } return $this->convertExceptionToResponse($e); diff --git a/tests/Foundation/FoundationExceptionsHandlerTest.php b/tests/Foundation/FoundationExceptionsHandlerTest.php index a804ecab2f10..93c7cda2443a 100644 --- a/tests/Foundation/FoundationExceptionsHandlerTest.php +++ b/tests/Foundation/FoundationExceptionsHandlerTest.php @@ -49,6 +49,8 @@ class FoundationExceptionsHandlerTest extends TestCase protected $config; + protected $viewFactory; + protected $container; protected $handler; @@ -59,14 +61,18 @@ protected function setUp(): void { $this->config = m::mock(Config::class); + $this->viewFactory = m::mock(ViewFactory::class); + $this->request = m::mock(stdClass::class); $this->container = Container::setInstance(new Container); $this->container->instance('config', $this->config); + $this->container->instance(ViewFactory::class, $this->viewFactory); + $this->container->instance(ResponseFactoryContract::class, new ResponseFactory( - m::mock(ViewFactory::class), + $this->viewFactory, m::mock(Redirector::class) )); @@ -397,6 +403,43 @@ public function getErrorView($e) $this->assertNull($handler->getErrorView(new HttpException(404))); } + private function executeScenarioWhereErrorViewThrowsWhileRenderingAndDebugIs($debug) + { + $this->viewFactory->shouldReceive('exists')->once()->with('errors::404')->andReturn(true); + $this->viewFactory->shouldReceive('make')->once()->withAnyArgs()->andThrow(new Exception("Rendering this view throws an exception")); + + $this->config->shouldReceive('get')->with('app.debug', null)->andReturn($debug); + + $handler = new class($this->container) extends Handler + { + protected function registerErrorViewPaths() {} + + public function getErrorView($e) + { + return $this->renderHttpException($e); + } + }; + + $this->assertInstanceOf(SymfonyResponse::class, $handler->getErrorView(new HttpException(404))); + } + + public function testItDoesNotCrashIfErrorViewThrowsWhileRenderingAndDebugFalse() + { + // When debug is false, the exception thrown while rendering the error view + // should not bubble as this may trigger an infinite loop. + + } + + public function testItDoesNotCrashIfErrorViewThrowsWhileRenderingAndDebugTrue() + { + // When debug is true, it is OK to bubble the exception thrown while rendering + // the error view as the debug handler should handle this gracefully. + + $this->expectException(\Exception::class); + $this->expectExceptionMessage("Rendering this view throws an exception"); + $this->executeScenarioWhereErrorViewThrowsWhileRenderingAndDebugIs(true); + } + public function testAssertExceptionIsThrown() { $this->assertThrows(function () { diff --git a/tests/Integration/Foundation/ExceptionHandlerTest.php b/tests/Integration/Foundation/ExceptionHandlerTest.php index 622b4348d2e8..b6c71b96e321 100644 --- a/tests/Integration/Foundation/ExceptionHandlerTest.php +++ b/tests/Integration/Foundation/ExceptionHandlerTest.php @@ -4,9 +4,12 @@ use Illuminate\Auth\Access\AuthorizationException; use Illuminate\Auth\Access\Response; +use Illuminate\Contracts\Debug\ExceptionHandler; +use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Route; use Orchestra\Testbench\TestCase; use Symfony\Component\Process\PhpProcess; +use Throwable; class ExceptionHandlerTest extends TestCase { @@ -151,4 +154,46 @@ public static function exitCodesProvider() yield 'Throw exception' => [[Fixtures\Providers\ThrowUncaughtExceptionServiceProvider::class], false]; yield 'Do not throw exception' => [[Fixtures\Providers\ThrowExceptionServiceProvider::class], true]; } + + public function test_it_handles_malformed_error_views_in_production() + { + Config::set('view.paths', [__DIR__.'/Fixtures/MalformedErrorViews']); + Config::set('app.debug', false); + $reported = []; + $this->app[ExceptionHandler::class]->reportable(function (Throwable $e) use (&$reported) { + $reported[] = $e; + }); + + try { + $response = $this->get('foo'); + } catch (Throwable) { + $response ??= null; + } + + $this->assertCount(1, $reported); + $this->assertSame('Undefined variable $foo (View: '.__DIR__.DIRECTORY_SEPARATOR.'Fixtures'.DIRECTORY_SEPARATOR.'MalformedErrorViews'.DIRECTORY_SEPARATOR.'errors'.DIRECTORY_SEPARATOR.'404.blade.php)', $reported[0]->getMessage()); + $this->assertNotNull($response); + $response->assertStatus(404); + } + + public function test_it_handles_malformed_error_views_in_development() + { + Config::set('view.paths', [__DIR__.'/Fixtures/MalformedErrorViews']); + Config::set('app.debug', true); + $reported = []; + $this->app[ExceptionHandler::class]->reportable(function (Throwable $e) use (&$reported) { + $reported[] = $e; + }); + + try { + $response = $this->get('foo'); + } catch (Throwable) { + $response ??= null; + } + + $this->assertCount(1, $reported); + $this->assertSame('Undefined variable $foo (View: '.__DIR__.DIRECTORY_SEPARATOR.'Fixtures'.DIRECTORY_SEPARATOR.'MalformedErrorViews'.DIRECTORY_SEPARATOR.'errors'.DIRECTORY_SEPARATOR.'404.blade.php)', $reported[0]->getMessage()); + $this->assertNotNull($response); + $response->assertStatus(500); + } } diff --git a/tests/Integration/Foundation/Fixtures/MalformedErrorViews/errors/404.blade.php b/tests/Integration/Foundation/Fixtures/MalformedErrorViews/errors/404.blade.php new file mode 100644 index 000000000000..26b405f9717d --- /dev/null +++ b/tests/Integration/Foundation/Fixtures/MalformedErrorViews/errors/404.blade.php @@ -0,0 +1,2 @@ +My custom 404 +