merged branch fabpot/twig-url-escaping (PR #7850)

This PR was merged into the master branch.

Discussion
----------

[2.3] [TwigBridge] save auto-escaping of generated URLs when possible

| Q             | A
| ------------- | ---
| Bug fix?      | [no]
| New feature?  | [yes: optimization]
| BC breaks?    | [no]
| Deprecations? | [no]
| Tests pass?   | [yes]
| Fixed tickets | #7088
| License       | MIT
| Doc PR        | [-]

Determines at compile time whether the generated URL will be safe and thus
saving the unneeded automatic escaping for performance reasons.

The URL generation process percent encodes non-alphanumeric characters. So there is no risk
that malicious/invalid characters are part of the URL. The only character within an URL that
must be escaped in html is the ampersand ("&") which separates query params. So we cannot mark
the URL generation as always safe, but only when we are sure there won't be multiple query
params. This is the case when there are none or only one constant parameter given.
E.g. we know beforehand this will be safe:
- path('route')
- path('route', {'param': 'value'})

But the following may not:
- path('route', var)
- path('route', {'param': ['val1', 'val2'] }) // a sub-array
- path('route', {'param1': 'value1', 'param2': 'value2'})

If param1 and param2 reference placeholder in the route, it would still be safe. But we don't know.

Commits
-------

725568b [TwigBridge] added some unit test for the previous commit
0721ff8 save auto-escaping of generated URLs when possible for performance reasons

src/Symfony/Bridge/Twig/Extension/RoutingExtension.php

@@ -35,8 +35,8 @@ class RoutingExtension extends \Twig_Extension
     public function getFunctions()
     {
         return array(
-            'url'  => new \Twig_Function_Method($this, 'getUrl'),
-            'path' => new \Twig_Function_Method($this, 'getPath'),
+            'url'  => new \Twig_Function_Method($this, 'getUrl', array('is_safe_callback' => array($this, 'isUrlGenerationSafe'))),
+            'path' => new \Twig_Function_Method($this, 'getPath', array('is_safe_callback' => array($this, 'isUrlGenerationSafe'))),
         );
     }
 
@@ -50,6 +50,44 @@ class RoutingExtension extends \Twig_Extension
         return $this->generator->generate($name, $parameters, $schemeRelative ? UrlGeneratorInterface::NETWORK_PATH : UrlGeneratorInterface::ABSOLUTE_URL);
     }
 
+    /**
+     * Determines at compile time whether the generated URL will be safe and thus
+     * saving the unneeded automatic escaping for performance reasons.
+     *
+     * The URL generation process percent encodes non-alphanumeric characters. So there is no risk
+     * that malicious/invalid characters are part of the URL. The only character within an URL that
+     * must be escaped in html is the ampersand ("&") which separates query params. So we cannot mark
+     * the URL generation as always safe, but only when we are sure there won't be multiple query
+     * params. This is the case when there are none or only one constant parameter given.
+     * E.g. we know beforehand this will be safe:
+     * - path('route')
+     * - path('route', {'param': 'value'})
+     * But the following may not:
+     * - path('route', var)
+     * - path('route', {'param': ['val1', 'val2'] }) // a sub-array
+     * - path('route', {'param1': 'value1', 'param2': 'value2'})
+     * If param1 and param2 reference placeholder in the route, it would still be safe. But we don't know.
+     *
+     * @param \Twig_Node $argsNode The arguments of the path/url function
+     *
+     * @return array An array with the contexts the URL is safe
+     */
+    public function isUrlGenerationSafe(\Twig_Node $argsNode)
+    {
+        // support named arguments
+        $paramsNode = $argsNode->hasNode('parameters') ? $argsNode->getNode('parameters') : (
+            $argsNode->hasNode(1) ? $argsNode->getNode(1) : null
+        );
+
+        if (null === $paramsNode || $paramsNode instanceof \Twig_Node_Expression_Array && count($paramsNode) <= 2 &&
+            (!$paramsNode->hasNode(1) || $paramsNode->getNode(1) instanceof \Twig_Node_Expression_Constant)
+        ) {
+            return array('html');
+        }
+
+        return array();
+    }
+
     /**
      * Returns the name of the extension.
      *

src/Symfony/Bridge/Twig/Tests/Extension/HttpKernelExtensionTest.php

@@ -20,6 +20,8 @@ class HttpKernelExtensionTest extends TestCase
 {
     protected function setUp()
     {
+        parent::setUp();
+
         if (!class_exists('Symfony\Component\HttpKernel\HttpKernel')) {
             $this->markTestSkipped('The "HttpKernel" component is not available');
         }

src/Symfony/Bridge/Twig/Tests/Extension/RoutingExtensionTest.php

@@ -0,0 +1,60 @@
+<?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\Bridge\Twig\Tests\Extension;
+
+use Symfony\Bridge\Twig\Extension\RoutingExtension;
+use Symfony\Bridge\Twig\Tests\TestCase;
+
+class RoutingExtensionTest extends TestCase
+{
+    protected function setUp()
+    {
+        parent::setUp();
+
+        if (!class_exists('Symfony\Component\Routing\Route')) {
+            $this->markTestSkipped('The "Routing" component is not available');
+        }
+    }
+
+    /**
+     * @dataProvider getEscapingTemplates
+     */
+    public function testEscaping($template, $mustBeEscaped)
+    {
+        $twig = new \Twig_Environment(null, array('debug' => true, 'cache' => false, 'autoescape' => true, 'optimizations' => 0));
+        $twig->addExtension(new RoutingExtension($this->getMock('Symfony\Component\Routing\Generator\UrlGeneratorInterface')));
+
+        $nodes = $twig->parse($twig->tokenize($template));
+
+        $this->assertSame($mustBeEscaped, $nodes->getNode('body')->getNode(0)->getNode('expr') instanceof \Twig_Node_Expression_Filter);
+    }
+
+    public function getEscapingTemplates()
+    {
+        return array(
+            array('{{ path("foo") }}', false),
+            array('{{ path("foo", {}) }}', false),
+            array('{{ path("foo", { foo: "foo" }) }}', false),
+            array('{{ path("foo", foo) }}', true),
+            array('{{ path("foo", { foo: foo }) }}', true),
+            array('{{ path("foo", { foo: ["foo", "bar"] }) }}', true),
+            array('{{ path("foo", { foo: "foo", bar: "bar" }) }}', true),
+
+            array('{{ path(name = "foo", parameters = {}) }}', false),
+            array('{{ path(name = "foo", parameters = { foo: "foo" }) }}', false),
+            array('{{ path(name = "foo", parameters = foo) }}', true),
+            array('{{ path(name = "foo", parameters = { foo: ["foo", "bar"] }) }}', true),
+            array('{{ path(name = "foo", parameters = { foo: foo }) }}', true),
+            array('{{ path(name = "foo", parameters = { foo: "foo", bar: "bar" }) }}', true),
+        );
+    }
+}

src/Symfony/Bridge/Twig/Tests/Extension/TranslationExtensionTest.php

@@ -21,6 +21,8 @@ class TranslationExtensionTest extends TestCase
 {
     protected function setUp()
     {
+        parent::setUp();
+
         if (!class_exists('Symfony\Component\Translation\Translator')) {
             $this->markTestSkipped('The "Translation" component is not available');
         }