From 0b5eece0b87d7d3466b1c645c943e9034daba925 Mon Sep 17 00:00:00 2001 From: Beau Simensen Date: Sat, 14 Oct 2023 19:03:00 -0500 Subject: [PATCH 1/9] Do not bubble exceptions thrown rendering error view when debug is false --- .../Foundation/Exceptions/Handler.php | 12 +++-- .../FoundationExceptionsHandlerTest.php | 48 ++++++++++++++++++- 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/src/Illuminate/Foundation/Exceptions/Handler.php b/src/Illuminate/Foundation/Exceptions/Handler.php index 301918317dd7..4ca020c52b2b 100644 --- a/src/Illuminate/Foundation/Exceptions/Handler.php +++ b/src/Illuminate/Foundation/Exceptions/Handler.php @@ -719,10 +719,14 @@ 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; + } } return $this->convertExceptionToResponse($e); diff --git a/tests/Foundation/FoundationExceptionsHandlerTest.php b/tests/Foundation/FoundationExceptionsHandlerTest.php index a804ecab2f10..a2b8335adafe 100644 --- a/tests/Foundation/FoundationExceptionsHandlerTest.php +++ b/tests/Foundation/FoundationExceptionsHandlerTest.php @@ -39,6 +39,7 @@ use stdClass; use Symfony\Component\HttpFoundation\Exception\SuspiciousOperationException; use Symfony\Component\HttpFoundation\File\UploadedFile; +use Symfony\Component\HttpFoundation\Response as SymfonyResponse; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; use Symfony\Component\HttpKernel\Exception\HttpException; @@ -49,6 +50,8 @@ class FoundationExceptionsHandlerTest extends TestCase protected $config; + protected $viewFactory; + protected $container; protected $handler; @@ -59,14 +62,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 +404,45 @@ 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. + + $this->expectException(\Exception::class); + $this->expectExceptionMessage("Rendering this view throws an exception"); + + $this->executeScenarioWhereErrorViewThrowsWhileRenderingAndDebugIs(false); + } + + 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->executeScenarioWhereErrorViewThrowsWhileRenderingAndDebugIs(true); + } + public function testAssertExceptionIsThrown() { $this->assertThrows(function () { From ecf17f6fb727510aff96701d060fdb0738fb3c66 Mon Sep 17 00:00:00 2001 From: Tim MacDonald Date: Tue, 17 Oct 2023 11:20:34 +1100 Subject: [PATCH 2/9] Add failing tests --- .../Foundation/ExceptionHandlerTest.php | 46 +++++++++++++++++++ .../MalformedErrorViews/errors/404.blade.php | 2 + .../MalformedErrorViews/errors/500.blade.php | 2 + 3 files changed, 50 insertions(+) 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/tests/Integration/Foundation/ExceptionHandlerTest.php b/tests/Integration/Foundation/ExceptionHandlerTest.php index 622b4348d2e8..00692a104a8f 100644 --- a/tests/Integration/Foundation/ExceptionHandlerTest.php +++ b/tests/Integration/Foundation/ExceptionHandlerTest.php @@ -4,9 +4,13 @@ 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 Illuminate\Support\Facades\View; use Orchestra\Testbench\TestCase; use Symfony\Component\Process\PhpProcess; +use Throwable; class ExceptionHandlerTest extends TestCase { @@ -151,4 +155,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__.'/Fixtures/MalformedErrorViews/errors/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__.'/Fixtures/MalformedErrorViews/errors/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 + Date: Tue, 17 Oct 2023 11:25:37 +1100 Subject: [PATCH 3/9] Only re-throw when debug mode it `true` --- src/Illuminate/Foundation/Exceptions/Handler.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Illuminate/Foundation/Exceptions/Handler.php b/src/Illuminate/Foundation/Exceptions/Handler.php index 4ca020c52b2b..b209900311b8 100644 --- a/src/Illuminate/Foundation/Exceptions/Handler.php +++ b/src/Illuminate/Foundation/Exceptions/Handler.php @@ -725,7 +725,7 @@ protected function renderHttpException(HttpExceptionInterface $e) 'exception' => $e, ], $e->getStatusCode(), $e->getHeaders()); } catch (Throwable $t) { - config('app.debug') || throw $t; + config('app.debug') && throw $t; } } From 9dd6490d63ff0fed8993bb6732c01bc7386e4936 Mon Sep 17 00:00:00 2001 From: Tim MacDonald Date: Tue, 17 Oct 2023 11:25:57 +1100 Subject: [PATCH 4/9] Manually report exception if debug mode is `false` --- src/Illuminate/Foundation/Exceptions/Handler.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Illuminate/Foundation/Exceptions/Handler.php b/src/Illuminate/Foundation/Exceptions/Handler.php index b209900311b8..d113b51a3776 100644 --- a/src/Illuminate/Foundation/Exceptions/Handler.php +++ b/src/Illuminate/Foundation/Exceptions/Handler.php @@ -726,6 +726,8 @@ protected function renderHttpException(HttpExceptionInterface $e) ], $e->getStatusCode(), $e->getHeaders()); } catch (Throwable $t) { config('app.debug') && throw $t; + + $this->report($t); } } From f4222ef7648fcac2ceb9e41a1758aecf43ba5c4e Mon Sep 17 00:00:00 2001 From: Tim MacDonald Date: Tue, 17 Oct 2023 12:07:10 +1100 Subject: [PATCH 5/9] Remove unit tests --- .../FoundationExceptionsHandlerTest.php | 39 ------------------- 1 file changed, 39 deletions(-) diff --git a/tests/Foundation/FoundationExceptionsHandlerTest.php b/tests/Foundation/FoundationExceptionsHandlerTest.php index a2b8335adafe..106c63b9be0f 100644 --- a/tests/Foundation/FoundationExceptionsHandlerTest.php +++ b/tests/Foundation/FoundationExceptionsHandlerTest.php @@ -404,45 +404,6 @@ 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. - - $this->expectException(\Exception::class); - $this->expectExceptionMessage("Rendering this view throws an exception"); - - $this->executeScenarioWhereErrorViewThrowsWhileRenderingAndDebugIs(false); - } - - 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->executeScenarioWhereErrorViewThrowsWhileRenderingAndDebugIs(true); - } - public function testAssertExceptionIsThrown() { $this->assertThrows(function () { From a23380e00bdc7a8cca0b89a666342764aa082fb2 Mon Sep 17 00:00:00 2001 From: Tim MacDonald Date: Tue, 17 Oct 2023 12:11:31 +1100 Subject: [PATCH 6/9] lint --- tests/Foundation/FoundationExceptionsHandlerTest.php | 1 - tests/Integration/Foundation/ExceptionHandlerTest.php | 1 - 2 files changed, 2 deletions(-) diff --git a/tests/Foundation/FoundationExceptionsHandlerTest.php b/tests/Foundation/FoundationExceptionsHandlerTest.php index 106c63b9be0f..af8f9c5b68d3 100644 --- a/tests/Foundation/FoundationExceptionsHandlerTest.php +++ b/tests/Foundation/FoundationExceptionsHandlerTest.php @@ -39,7 +39,6 @@ use stdClass; use Symfony\Component\HttpFoundation\Exception\SuspiciousOperationException; use Symfony\Component\HttpFoundation\File\UploadedFile; -use Symfony\Component\HttpFoundation\Response as SymfonyResponse; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; use Symfony\Component\HttpKernel\Exception\HttpException; diff --git a/tests/Integration/Foundation/ExceptionHandlerTest.php b/tests/Integration/Foundation/ExceptionHandlerTest.php index 00692a104a8f..cb3d45ed0bc2 100644 --- a/tests/Integration/Foundation/ExceptionHandlerTest.php +++ b/tests/Integration/Foundation/ExceptionHandlerTest.php @@ -7,7 +7,6 @@ use Illuminate\Contracts\Debug\ExceptionHandler; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Route; -use Illuminate\Support\Facades\View; use Orchestra\Testbench\TestCase; use Symfony\Component\Process\PhpProcess; use Throwable; From 954dc1de3237c18ea0d14391e57871206b46af20 Mon Sep 17 00:00:00 2001 From: Tim MacDonald Date: Tue, 17 Oct 2023 12:19:55 +1100 Subject: [PATCH 7/9] Revert "Remove unit tests" This reverts commit f4222ef7648fcac2ceb9e41a1758aecf43ba5c4e. --- .../FoundationExceptionsHandlerTest.php | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/Foundation/FoundationExceptionsHandlerTest.php b/tests/Foundation/FoundationExceptionsHandlerTest.php index af8f9c5b68d3..b929595a1881 100644 --- a/tests/Foundation/FoundationExceptionsHandlerTest.php +++ b/tests/Foundation/FoundationExceptionsHandlerTest.php @@ -403,6 +403,45 @@ 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. + + $this->expectException(\Exception::class); + $this->expectExceptionMessage("Rendering this view throws an exception"); + + $this->executeScenarioWhereErrorViewThrowsWhileRenderingAndDebugIs(false); + } + + 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->executeScenarioWhereErrorViewThrowsWhileRenderingAndDebugIs(true); + } + public function testAssertExceptionIsThrown() { $this->assertThrows(function () { From a6c101f331d9954b42f8cfb092e9cee17b91449a Mon Sep 17 00:00:00 2001 From: Tim MacDonald Date: Tue, 17 Oct 2023 12:25:04 +1100 Subject: [PATCH 8/9] Fix original unit tests --- tests/Foundation/FoundationExceptionsHandlerTest.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/Foundation/FoundationExceptionsHandlerTest.php b/tests/Foundation/FoundationExceptionsHandlerTest.php index b929595a1881..93c7cda2443a 100644 --- a/tests/Foundation/FoundationExceptionsHandlerTest.php +++ b/tests/Foundation/FoundationExceptionsHandlerTest.php @@ -428,10 +428,6 @@ 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. - $this->expectException(\Exception::class); - $this->expectExceptionMessage("Rendering this view throws an exception"); - - $this->executeScenarioWhereErrorViewThrowsWhileRenderingAndDebugIs(false); } public function testItDoesNotCrashIfErrorViewThrowsWhileRenderingAndDebugTrue() @@ -439,6 +435,8 @@ 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); } From 170b55171452a4c59aaeb4a1276782dffefbe3c7 Mon Sep 17 00:00:00 2001 From: Tim MacDonald Date: Tue, 17 Oct 2023 12:30:16 +1100 Subject: [PATCH 9/9] Fix paths for windows --- tests/Integration/Foundation/ExceptionHandlerTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Integration/Foundation/ExceptionHandlerTest.php b/tests/Integration/Foundation/ExceptionHandlerTest.php index cb3d45ed0bc2..b6c71b96e321 100644 --- a/tests/Integration/Foundation/ExceptionHandlerTest.php +++ b/tests/Integration/Foundation/ExceptionHandlerTest.php @@ -171,7 +171,7 @@ public function test_it_handles_malformed_error_views_in_production() } $this->assertCount(1, $reported); - $this->assertSame('Undefined variable $foo (View: '.__DIR__.'/Fixtures/MalformedErrorViews/errors/404.blade.php)', $reported[0]->getMessage()); + $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); } @@ -192,7 +192,7 @@ public function test_it_handles_malformed_error_views_in_development() } $this->assertCount(1, $reported); - $this->assertSame('Undefined variable $foo (View: '.__DIR__.'/Fixtures/MalformedErrorViews/errors/404.blade.php)', $reported[0]->getMessage()); + $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); }