[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 f4222ef.

* Fix original unit tests

* Fix paths for windows

---------

Co-authored-by: Tim MacDonald <[email protected]>

Author: simensen

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);

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 () {

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);
+    }
 }

tests/Integration/Foundation/Fixtures/MalformedErrorViews/errors/404.blade.php

@@ -0,0 +1,2 @@
+My custom 404
+<?php $foo();

tests/Integration/Foundation/Fixtures/MalformedErrorViews/errors/500.blade.php

@@ -0,0 +1,2 @@
+My custom 500
+<?php $bar();