symfony · javiereguiluz · Oct 4, 2024 · Oct 7, 2024 · Oct 9, 2024 · Oct 9, 2024
diff --git a/src/Symfony/Bridge/Twig/ErrorRenderer/TwigErrorRenderer.php b/src/Symfony/Bridge/Twig/ErrorRenderer/TwigErrorRenderer.php
@@ -25,15 +25,15 @@
 */
 class TwigErrorRenderer implements ErrorRendererInterface
 {
-    private HtmlErrorRenderer $fallbackErrorRenderer;
+    private ErrorRendererInterface $fallbackErrorRenderer;
    private \Closure|bool $debug;

    /**
     * @param bool|callable $debug The debugging mode as a boolean or a callable that should return it
     */
    public function __construct(
        private Environment $twig,
-        ?HtmlErrorRenderer $fallbackErrorRenderer = null,
+        ?ErrorRendererInterface $fallbackErrorRenderer = null,
        bool|callable $debug = false,
    ) {
        $this->fallbackErrorRenderer = $fallbackErrorRenderer ?? new HtmlErrorRenderer();

diff --git a/src/Symfony/Bridge/Twig/Tests/ErrorRenderer/TwigErrorRendererTest.php b/src/Symfony/Bridge/Twig/Tests/ErrorRenderer/TwigErrorRendererTest.php
@@ -13,44 +13,40 @@

 use PHPUnit\Framework\TestCase;
 use Symfony\Bridge\Twig\ErrorRenderer\TwigErrorRenderer;
-use Symfony\Component\ErrorHandler\ErrorRenderer\HtmlErrorRenderer;
+use Symfony\Component\ErrorHandler\ErrorRenderer\CliErrorRenderer;
 use Symfony\Component\ErrorHandler\Exception\FlattenException;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 use Twig\Environment;
 use Twig\Loader\ArrayLoader;

 class TwigErrorRendererTest extends TestCase
 {
-    public function testFallbackToNativeRendererIfDebugOn()
+    public function tesFallbackRenderer()
    {
-        $exception = new \Exception();
-
        $twig = $this->createMock(Environment::class);
-        $nativeRenderer = $this->createMock(HtmlErrorRenderer::class);
-        $nativeRenderer
-            ->expects($this->once())
-            ->method('render')
-            ->with($exception)
-        ;
-
-        (new TwigErrorRenderer($twig, $nativeRenderer, true))->render(new \Exception());
+        $customRenderer = new class implements ErrorRendererInterface {
+            public function render(\Throwable $exception): FlattenException
+            {
+                return 'This is a custom error renderer.';
+            }
+        };
+
+        $this->assertSame('This is a custom error renderer.', (new TwigErrorRenderer($twig, $customRenderer, true))->render(new \Exception()));
    }

-    public function testFallbackToNativeRendererIfCustomTemplateNotFound()
+    public function testCliRenderer()
    {
        $exception = new NotFoundHttpException();
-
        $twig = new Environment(new ArrayLoader([]));

-        $nativeRenderer = $this->createMock(HtmlErrorRenderer::class);
-        $nativeRenderer
-            ->expects($this->once())
-            ->method('render')
-            ->with($exception)
-            ->willReturn(FlattenException::createFromThrowable($exception))
-        ;
+        $exception = (new TwigErrorRenderer($twig, new CliErrorRenderer(), false))->render($exception);
+
+        $exceptionHeaders = $exception->getHeaders();
+        if (isset($exceptionHeaders['Content-Type'])) {
+            $this->assertSame('text/plain', $exceptionHeaders['Content-Type'], 'The exception does not return HTML contents (to prevent potential XSS vulnerabilities)');
+        }

-        (new TwigErrorRenderer($twig, $nativeRenderer, false))->render($exception);
+        $this->assertStringNotContainsString('<!DOCTYPE html>', $exception->getAsString(), 'The exception does not include elements of the HTML exception page');
    }

    public function testRenderCustomErrorTemplate()

@@ -0,0 +1,33 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\TwigBundle\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\ErrorHandler\ErrorRenderer\CliErrorRenderer;
+
+/**
+ * @author Javier Eguiluz <javier.eguiluz@gmail.com>
+ * @internal
+ */
+class ErrorRendererPass implements CompilerPassInterface
+{
+    public function process(ContainerBuilder $container): void
+    {
+        // in the 'test' environment, use the CLI error renderer as the default one
+        if ($container->hasDefinition('test.client')) {
+            $container->getDefinition('twig.error_renderer.html')
+                ->setArgument(1, new Definition(CliErrorRenderer::class));
+        }
+    }
+}
@@ -11,6 +11,7 @@

 namespace Symfony\Bundle\TwigBundle;

+use Symfony\Bundle\TwigBundle\DependencyInjection\Compiler\ErrorRendererPass;
 use Symfony\Bundle\TwigBundle\DependencyInjection\Compiler\ExtensionPass;
 use Symfony\Bundle\TwigBundle\DependencyInjection\Compiler\RuntimeLoaderPass;
 use Symfony\Bundle\TwigBundle\DependencyInjection\Compiler\TwigEnvironmentPass;
@@ -36,6 +37,7 @@ public function build(ContainerBuilder $container): void
        $container->addCompilerPass(new TwigEnvironmentPass());
        $container->addCompilerPass(new TwigLoaderPass());
        $container->addCompilerPass(new RuntimeLoaderPass(), PassConfig::TYPE_BEFORE_REMOVING);
+        $container->addCompilerPass(new ErrorRendererPass());
    }

    public function registerCommands(Application $application): void

@@ -20,7 +20,7 @@
        "composer-runtime-api": ">=2.1",
        "symfony/config": "^6.4|^7.0",
        "symfony/dependency-injection": "^6.4|^7.0",
-        "symfony/twig-bridge": "^6.4|^7.0",
+        "symfony/twig-bridge": "^7.2",
        "symfony/http-foundation": "^6.4|^7.0",
        "symfony/http-kernel": "^6.4|^7.0",
        "twig/twig": "^3.12|^4.0"

@@ -40,7 +40,7 @@ protected function supportsColors(): bool
            }
        };

-        return FlattenException::createFromThrowable($exception)
+        return FlattenException::createFromThrowable($exception, headers: ['Content-Type' => 'text/plain'])
            ->setAsString($dumper->dump($cloner->cloneVar($exception), true));
    }
 }
-Original file line number
+Diff line change
@@ -0,0 +1,33 @@
+    <?php
+    /*
+     * This file is part of the Symfony package.
+     *
+     * (c) Fabien Potencier <fabien@symfony.com>
+     *
+     * For the full copyright and license information, please view the LICENSE
+     * file that was distributed with this source code.
+     */
+    namespace Symfony\Bundle\TwigBundle\DependencyInjection\Compiler;
+    use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+    use Symfony\Component\DependencyInjection\ContainerBuilder;
+    use Symfony\Component\DependencyInjection\Definition;
+    use Symfony\Component\ErrorHandler\ErrorRenderer\CliErrorRenderer;
+    /**
+     * @author Javier Eguiluz <javier.eguiluz@gmail.com>
+     * @internal
+     */
+    class ErrorRendererPass implements CompilerPassInterface
                   javiereguiluz marked this conversation as resolved.
              

          
            Show resolved
            Hide resolved
+    {
+        public function process(ContainerBuilder $container): void
+        {
+            // in the 'test' environment, use the CLI error renderer as the default one
+            if ($container->hasDefinition('test.client')) {
+                $container->getDefinition('twig.error_renderer.html')
+                    ->setArgument(1, new Definition(CliErrorRenderer::class));
                   javiereguiluz marked this conversation as resolved.
              

          
            Show resolved
            Hide resolved
        

                
            
  


  


    
    
      
  
    
    
              
          

          
        

                


  
    
        
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            stof
  

      

      

      


        Oct 9, 2024


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      wouldn't this break if the error-handler component is not installed (which is not a dependency of the bundle right now) ?
    
  
  


    

        
      
  
    
  
  
  
    
    Sorry, something went wrong.
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



  
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            stof
  

      

      

      


        Oct 9, 2024


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      and maybe we need a conflict with older versions of the component to ensure we don't inject a version that does not properly report a plaintext content type (and so is subject to XSS)
    
  
  


    

        
      
  
    
  
  
  
    
    Sorry, something went wrong.
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    






    

  





  


    
    
      
  
    
    
              
          

          
        

                


  
    
        
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            yceruto
  

      

      

      


        Oct 9, 2024


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      I think the decision of which fallback error renderer is the best does not depend on the environment but on the SAPI value, as you did at the beginning. What about creating a service factory that injects the proper error renderer depending on the SAPI value?
    
  
  


    

        
      
  
    
  
  
  
    
    Sorry, something went wrong.
  


          
      
  
    
    
      
        
            
    All reactions
+            }
+        }
+    }