Skip to content

Commit

Permalink
Removed logic that tried to avoid double-escaping
Browse files Browse the repository at this point in the history
Because that's just not possible (have a look at the unit tests to see all possibilities
-- as you will notice, there is no way we can determine the context and whether the
data are already escaped or not).

So, we always escape data, which means that sometimes, we will try to escape already
escaped data. This is not a problem for everything except strings. That's because
strings are not wrapped with an object like everything else (for performance reason).

This means that all escapers must be able to avoid double-escaping (that's the case
for the default escapers as both htmlspecialchars() and htmlentities() have a flag
that does just this).
  • Loading branch information
fabpot committed Oct 28, 2010
1 parent 3eee458 commit 13f36b1
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 15 deletions.
15 changes: 2 additions & 13 deletions src/Symfony/Bundle/FrameworkBundle/Templating/Engine.php
Expand Up @@ -27,7 +27,6 @@ class Engine extends BaseEngine
{
protected $container;
protected $escaper;
protected $level;

/**
* Constructor.
Expand All @@ -39,7 +38,6 @@ class Engine extends BaseEngine
*/
public function __construct(ContainerInterface $container, LoaderInterface $loader, array $renderers = array(), $escaper = false)
{
$this->level = 0;
$this->container = $container;
$this->escaper = $escaper;

Expand All @@ -61,8 +59,6 @@ public function __construct(ContainerInterface $container, LoaderInterface $load

public function render($name, array $parameters = array())
{
++$this->level;

list(, $options) = $this->splitTemplateName($name);

$renderer = $options['renderer'];
Expand All @@ -73,17 +69,10 @@ public function render($name, array $parameters = array())
}

if ('php' === $renderer) {
// escape only once
if (1 === $this->level && !isset($parameters['_data'])) {
$parameters = $this->escapeParameters($parameters);
}
$parameters = $this->escapeParameters($parameters);
}

$content = parent::render($name, $parameters);

--$this->level;

return $content;
return parent::render($name, $parameters);
}

/**
Expand Down
41 changes: 41 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Tests/Templating/EngineTest.php
Expand Up @@ -13,9 +13,50 @@

use Symfony\Bundle\FrameworkBundle\Tests\TestCase;
use Symfony\Bundle\FrameworkBundle\Templating\Engine;
use Symfony\Component\Templating\Storage\StringStorage;
use Symfony\Component\Templating\Storage\Storage;
use Symfony\Component\Templating\Renderer\PhpRenderer;
use Symfony\Component\OutputEscaper\Escaper;

// simulate the rendering of another controller
function foo($engine)
{
return $engine->render('FooBundle:Foo:tpl1.php', array('foo' => 'foo <br />'));
}

class EngineTest extends TestCase
{
public function testRenderEscaping()
{
$templates = array(
'tpl1' => '<?php echo $foo ?>',
'tpl2' => '<?php echo $foo.$view->render("FooBundle:Foo:tpl1.php", array("foo" => $foo)) ?>',
'tpl3' => '<?php echo $foo.$view->render("FooBundle:Foo:tpl1.php", array("foo" => "foo <br />")) ?>',
'tpl4' => '<?php echo $foo.Symfony\Bundle\FrameworkBundle\Tests\Templating\foo($view) ?>',
);

$loader = $this->getMock('Symfony\Component\Templating\Loader\LoaderInterface');
$loader->expects($this->exactly(4))
->method('load')
->with($this->anything(), $this->anything())
->will($this->onConsecutiveCalls(
new StringStorage($templates['tpl1']),
new StringStorage($templates['tpl2']),
new StringStorage($templates['tpl3']),
new StringStorage($templates['tpl4'])
))
;

$engine = new Engine($this->getContainerMock(), $loader, array('php' => new PhpRenderer()), 'htmlspecialchars');

$this->assertEquals('foo &lt;br /&gt;', $engine->render('FooBundle:Foo:tpl1.php', array('foo' => 'foo <br />')));
$this->assertEquals('foo &lt;br /&gt;', $engine->render('FooBundle:Foo:tpl1.php', array('foo' => 'foo <br />')));

$this->assertEquals('foo &lt;br /&gt;foo &lt;br /&gt;', $engine->render('FooBundle:Foo:tpl2.php', array('foo' => 'foo <br />')));
$this->assertEquals('foo &lt;br /&gt;foo &lt;br /&gt;', $engine->render('FooBundle:Foo:tpl3.php', array('foo' => 'foo <br />')));
$this->assertEquals('foo &lt;br /&gt;foo &lt;br /&gt;', $engine->render('FooBundle:Foo:tpl4.php', array('foo' => 'foo <br />')));
}

/**
* @dataProvider getSplitTemplateNameTests
*/
Expand Down
7 changes: 5 additions & 2 deletions src/Symfony/Component/OutputEscaper/Escaper.php
Expand Up @@ -206,6 +206,9 @@ static public function getCharset()
/**
* Adds a named escaper.
*
* Warning: An escaper must be able to deal with
* double-escaping correctly.
*
* @param string $name The escaper name
* @param mixed $escaper A PHP callable
*/
Expand Down Expand Up @@ -262,7 +265,7 @@ function ($value)
{
// Numbers and boolean values get turned into strings which can cause problems
// with type comparisons (e.g. === or is_int() etc).
return is_string($value) ? htmlspecialchars($value, ENT_QUOTES, Escaper::getCharset()) : $value;
return is_string($value) ? htmlspecialchars($value, ENT_QUOTES, Escaper::getCharset(), false) : $value;
},

'entities' =>
Expand All @@ -276,7 +279,7 @@ function ($value)
{
// Numbers and boolean values get turned into strings which can cause problems
// with type comparisons (e.g. === or is_int() etc).
return is_string($value) ? htmlentities($value, ENT_QUOTES, Escaper::getCharset()) : $value;
return is_string($value) ? htmlentities($value, ENT_QUOTES, Escaper::getCharset(), false) : $value;
},

'raw' =>
Expand Down

3 comments on commit 13f36b1

@marijn
Copy link

@marijn marijn commented on 13f36b1 Oct 28, 2010

Choose a reason for hiding this comment

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

At the risk of saying something stupid: why not test for is_object && class_implements("BaseEscaper") when looping over the parameters while escaping..?

@fabpot
Copy link
Member Author

@fabpot fabpot commented on 13f36b1 Oct 28, 2010

Choose a reason for hiding this comment

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

I don't understand your suggestion. Can you be a bit more explicit?

@marijn
Copy link

@marijn marijn commented on 13f36b1 Oct 28, 2010

Choose a reason for hiding this comment

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

Never mind, it turned out I did say something stupid...
I was under the impression that every kind of value would be decorated with a BaseEscaper object.
If that were the case you could (as you do already for the ArrayDecorator and ObjectDecorator) test if the value that should be escaped wasn't decorated already.

But as it turns out, that's not the case here :-)

Please sign in to comment.