Skip to content
Browse files

restricted to only URIs the first argument of the render tag

  • Loading branch information...
1 parent 2c9083a commit 64d43c806bc1c00dda536305565d8aaa3d07b699 @fabpot fabpot committed Dec 17, 2012
View
18 UPGRADE-2.2.md
@@ -1,6 +1,24 @@
UPGRADE FROM 2.1 to 2.2
=======================
+### TwigBridge
+
+ * The `render` tag signature and arguments changed.
+
+ Before:
+
+ ```
+ {% render 'BlogBundle:Post:list' with { 'limit': 2 }, { 'alt': 'BlogBundle:Post:error' } %}
+ ```
+
+ After:
+
+ ```
+ {% render url('post_list', { 'limit': 2 }), { 'alt': 'BlogBundle:Post:error' } %}
+ ```
+
+ where `post_list` is the route name for the `BlogBundle:Post:list` controller.
+
### HttpFoundation
* The MongoDbSessionHandler default field names and timestamp type have changed.
View
1 src/Symfony/Bridge/Twig/CHANGELOG.md
@@ -4,6 +4,7 @@ CHANGELOG
2.2.0
-----
+ * [BC BREAK] restricted the `render` tag to only accept URIs as reference (the signature changed)
* added a render function to render a request
* The `app` global variable is now injected even when using the twig service directly.
View
4 src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
@@ -4,6 +4,10 @@ CHANGELOG
2.2.0
-----
+ * [BC BREAK] restricted the `Symfony\Bundle\FrameworkBundle\HttpKernel::render()` method to only accept URIs as reference
+ * `Symfony\Bundle\FrameworkBundle\HttpKernel::render()` method signature changed and the first argument
+ must now be a URI (the `generateInternalUri()` method was removed)
+ * The internal routes have been removed (`Resources/config/routing/internal.xml`)
* replaced Symfony\Bundle\FrameworkBundle\Controller\TraceableControllerResolver by Symfony\Component\HttpKernel\Controller\TraceableControllerResolver
* replaced Symfony\Component\HttpKernel\Debug\ContainerAwareTraceableEventDispatcher by Symfony\Component\HttpKernel\Debug\TraceableEventDispatcher
* added Client::enableProfiler()
View
75 src/Symfony/Bundle/FrameworkBundle/Controller/InternalController.php
@@ -1,75 +0,0 @@
-<?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\FrameworkBundle\Controller;
-
-use Symfony\Component\DependencyInjection\ContainerAware;
-use Symfony\Component\HttpFoundation\Response;
-
-/**
- * InternalController.
- *
- * @author Fabien Potencier <fabien@symfony.com>
- */
-class InternalController extends ContainerAware
-{
- /**
- * Forwards to the given controller with the given path.
- *
- * @param string $path The path
- * @param string $controller The controller name
- *
- * @return Response A Response instance
- */
- public function indexAction($path, $controller)
- {
- // safeguard
- if (!is_string($controller)) {
- throw new \RuntimeException('A Controller must be a string.');
- }
-
- // check that the controller looks like a controller
- if (false === strpos($controller, '::')) {
- $count = substr_count($controller, ':');
- if (2 == $count) {
- // the convention already enforces the Controller suffix
- } elseif (1 == $count) {
- // controller in the service:method notation
- list($service, $method) = explode(':', $controller, 2);
- $class = get_class($this->container->get($service));
-
- if (!preg_match('/Controller$/', $class)) {
- throw new \RuntimeException('A Controller class name must end with Controller.');
- }
- } else {
- throw new \LogicException('Unable to parse the Controller name.');
- }
- } else {
- list($class, $method) = explode('::', $controller, 2);
-
- if (!preg_match('/Controller$/', $class)) {
- throw new \RuntimeException('A Controller class name must end with Controller.');
- }
- }
-
- $request = $this->container->get('request');
- $attributes = $request->attributes;
-
- $attributes->remove('path');
- $attributes->remove('controller');
- if ('none' !== $path) {
- parse_str($path, $tmp);
- $attributes->add($tmp);
- }
-
- return $this->container->get('http_kernel')->forward($controller, $attributes->all(), $request->query->all());
- }
-}
View
94 src/Symfony/Bundle/FrameworkBundle/HttpKernel.php
@@ -84,54 +84,40 @@ public function forward($controller, array $attributes = array(), array $query =
*
* Available options:
*
- * * attributes: An array of request attributes (only when the first argument is a controller)
- * * query: An array of request query parameters (only when the first argument is a controller)
* * ignore_errors: true to return an empty string in case of an error
- * * alt: an alternative controller to execute in case of an error (can be a controller, a URI, or an array with the controller, the attributes, and the query arguments)
+ * * alt: an alternative URI to execute in case of an error
* * standalone: whether to generate an esi:include tag or not when ESI is supported
* * comment: a comment to add when returning an esi:include tag
*
- * @param string $controller A controller name to execute (a string like BlogBundle:Post:index), or a relative URI
- * @param array $options An array of options
+ * @param string $uri A URI
+ * @param array $options An array of options
*
* @return string The Response content
*
* @throws \RuntimeException
* @throws \Exception
*/
- public function render($controller, array $options = array())
+ public function render($uri, array $options = array())
{
+ $request = $this->container->get('request');
+
$options = array_merge(array(
- 'attributes' => array(),
- 'query' => array(),
'ignore_errors' => !$this->container->getParameter('kernel.debug'),
- 'alt' => array(),
+ 'alt' => null,
'standalone' => false,
'comment' => '',
'default' => null,
), $options);
- if (!is_array($options['alt'])) {
- $options['alt'] = array($options['alt']);
- }
-
if (null === $this->esiSupport) {
- $this->esiSupport = $this->container->has('esi') && $this->container->get('esi')->hasSurrogateEsiCapability($this->container->get('request'));
+ $this->esiSupport = $this->container->has('esi') && $this->container->get('esi')->hasSurrogateEsiCapability($request);
}
if ($this->esiSupport && (true === $options['standalone'] || 'esi' === $options['standalone'])) {
- $uri = $this->generateInternalUri($controller, $options['attributes'], $options['query']);
-
- $alt = '';
- if ($options['alt']) {
- $alt = $this->generateInternalUri($options['alt'][0], isset($options['alt'][1]) ? $options['alt'][1] : array(), isset($options['alt'][2]) ? $options['alt'][2] : array());
- }
-
- return $this->container->get('esi')->renderIncludeTag($uri, $alt, $options['ignore_errors'], $options['comment']);
+ return $this->container->get('esi')->renderIncludeTag($uri, $options['alt'], $options['ignore_errors'], $options['comment']);
}
if ('js' === $options['standalone']) {
- $uri = $this->generateInternalUri($controller, $options['attributes'], $options['query'], false);
$defaultContent = null;
$templating = $this->container->get('templating');
@@ -149,29 +135,9 @@ public function render($controller, array $options = array())
return $this->renderHIncludeTag($uri, $defaultContent);
}
- $request = $this->container->get('request');
-
- // controller or URI or path?
- if (0 === strpos($controller, 'http://') || 0 === strpos($controller, 'https://')) {
- $subRequest = Request::create($controller, 'get', array(), $request->cookies->all(), array(), $request->server->all());
- if ($session = $request->getSession()) {
- $subRequest->setSession($session);
- }
- } elseif (0 === strpos($controller, '/')) {
- $subRequest = Request::create($request->getUriForPath($controller), 'get', array(), $request->cookies->all(), array(), $request->server->all());
- if ($session = $request->getSession()) {
- $subRequest->setSession($session);
- }
- } else {
- $options['attributes']['_controller'] = $controller;
-
- if (!isset($options['attributes']['_format'])) {
- $options['attributes']['_format'] = $request->getRequestFormat();
- }
-
- $options['attributes']['_route'] = '_internal';
- $subRequest = $request->duplicate($options['query'], null, $options['attributes']);
- $subRequest->setMethod('GET');
+ $subRequest = Request::create($uri, 'get', array(), $request->cookies->all(), array(), $request->server->all());
+ if ($session = $request->getSession()) {
+ $subRequest->setSession($session);
}
$level = ob_get_level();
@@ -191,10 +157,8 @@ public function render($controller, array $options = array())
if ($options['alt']) {
$alt = $options['alt'];
unset($options['alt']);
- $options['attributes'] = isset($alt[1]) ? $alt[1] : array();
- $options['query'] = isset($alt[2]) ? $alt[2] : array();
- return $this->render($alt[0], $options);
+ return $this->render($alt, $options);
}
if (!$options['ignore_errors']) {
@@ -209,38 +173,6 @@ public function render($controller, array $options = array())
}
/**
- * Generates an internal URI for a given controller.
- *
- * This method uses the "_internal" route, which should be available.
- *
- * @param string $controller A controller name to execute (a string like BlogBundle:Post:index), or a relative URI
- * @param array $attributes An array of request attributes
- * @param array $query An array of request query parameters
- * @param boolean $secure
- *
- * @return string An internal URI
- */
- public function generateInternalUri($controller, array $attributes = array(), array $query = array(), $secure = true)
- {
- if (0 === strpos($controller, '/')) {
- return $controller;
- }
-
- $path = http_build_query($attributes, '', '&');
- $uri = $this->container->get('router')->generate($secure ? '_internal' : '_internal_public', array(
- 'controller' => $controller,
- 'path' => $path ?: 'none',
- '_format' => $this->container->get('request')->getRequestFormat(),
- ));
-
- if ($queryString = http_build_query($query, '', '&')) {
- $uri .= '?'.$queryString;
- }
-
- return $uri;
- }
-
- /**
* Renders an HInclude tag.
*
* @param string $uri A URI
View
18 src/Symfony/Bundle/FrameworkBundle/Resources/config/routing/internal.xml
@@ -1,18 +0,0 @@
-<?xml version="1.0" encoding="UTF-8" ?>
-
-<routes xmlns="http://symfony.com/schema/routing"
- xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
- xsi:schemaLocation="http://symfony.com/schema/routing http://symfony.com/schema/routing/routing-1.0.xsd">
-
- <route id="_internal" pattern="/secure/{controller}/{path}.{_format}">
- <default key="_controller">FrameworkBundle:Internal:index</default>
- <requirement key="path">.+</requirement>
- <requirement key="_format">[^.]+</requirement>
- </route>
-
- <route id="_internal_public" pattern="/{controller}/{path}.{_format}">
- <default key="_controller">FrameworkBundle:Internal:index</default>
- <requirement key="path">.+</requirement>
- <requirement key="_format">[^.]+</requirement>
- </route>
-</routes>
View
55 src/Symfony/Bundle/FrameworkBundle/Tests/HttpKernelTest.php
@@ -116,47 +116,6 @@ public function testHandleRestoresThePreviousRequestOnException($type)
}
}
- public function testGenerateInternalUriHandlesNullValues()
- {
- $request = new Request();
-
- $router = $this->getMock('Symfony\\Component\\Routing\\RouterInterface');
- $container = $this->getMock('Symfony\\Component\\DependencyInjection\\ContainerInterface');
- $container
- ->expects($this->at(0))
- ->method('get')
- ->with($this->equalTo('router'))
- ->will($this->returnValue($router))
- ;
- $container
- ->expects($this->at('1'))
- ->method('get')
- ->with($this->equalTo('request'))
- ->will($this->returnValue($request))
- ;
-
- $controller = 'AController';
- $attributes = array('anAttribute' => null);
- $query = array('aQueryParam' => null);
-
- $expectedPath = 'none';
-
- $routeParameters = array('controller' => $controller, 'path' => $expectedPath, '_format' => 'html');
- $router
- ->expects($this->once())
- ->method('generate')
- ->with($this->equalTo('_internal'), $this->equalTo($routeParameters))
- ->will($this->returnValue('GENERATED_URI'))
- ;
-
- $dispatcher = new EventDispatcher();
- $resolver = $this->getMock('Symfony\\Component\\HttpKernel\\Controller\\ControllerResolverInterface');
- $kernel = new HttpKernel($dispatcher, $container, $resolver);
-
- $uri = $kernel->generateInternalUri($controller, $attributes, $query);
- $this->assertEquals('GENERATED_URI', $uri);
- }
-
public function getProviderTypes()
{
return array(
@@ -172,22 +131,22 @@ public function testExceptionInSubRequestsDoesNotMangleOutputBuffers()
$container = $this->getMock('Symfony\\Component\\DependencyInjection\\ContainerInterface');
$container
->expects($this->at(0))
+ ->method('get')
+ ->with($this->equalTo('request'))
+ ->will($this->returnValue($request))
+ ;
+ $container
+ ->expects($this->at(1))
->method('getParameter')
->with($this->equalTo('kernel.debug'))
->will($this->returnValue(false))
;
$container
- ->expects($this->at(1))
+ ->expects($this->at(2))
->method('has')
->with($this->equalTo('esi'))
->will($this->returnValue(false))
;
- $container
- ->expects($this->at(2))
- ->method('get')
- ->with($this->equalTo('request'))
- ->will($this->returnValue($request))
- ;
$dispatcher = new EventDispatcher();
$resolver = $this->getMock('Symfony\\Component\\HttpKernel\\Controller\\ControllerResolverInterface');
View
13 src/Symfony/Bundle/TwigBundle/Extension/ActionsExtension.php
@@ -34,17 +34,16 @@ public function __construct(ContainerInterface $container)
}
/**
- * Returns the Response content for a given controller or URI.
+ * Returns the Response content for a given URI.
*
- * @param string $controller A controller name to execute (a string like BlogBundle:Post:index), or a relative URI
- * @param array $attributes An array of request attributes
- * @param array $options An array of options
+ * @param string $uri A URI
+ * @param array $options An array of options
*
* @see Symfony\Bundle\FrameworkBundle\Controller\ControllerResolver::render()
*/
- public function renderAction($controller, array $attributes = array(), array $options = array())
+ public function renderUri($uri, array $options = array())
{
- return $this->container->get('templating.helper.actions')->render($controller, $attributes, $options);
+ return $this->container->get('templating.helper.actions')->render($uri, $options);
}
/**
@@ -55,7 +54,7 @@ public function renderAction($controller, array $attributes = array(), array $op
public function getTokenParsers()
{
return array(
- // {% render 'BlogBundle:Post:list' with { 'limit': 2 }, { 'alt': 'BlogBundle:Post:error' } %}
+ // {% render url('post_list', { 'limit': 2 }), { 'alt': 'BlogBundle:Post:error' } %}
new RenderTokenParser(),
);
}
View
8 src/Symfony/Bundle/TwigBundle/Node/RenderNode.php
@@ -18,9 +18,9 @@
*/
class RenderNode extends \Twig_Node
{
- public function __construct(\Twig_Node_Expression $expr, \Twig_Node_Expression $attributes, \Twig_Node_Expression $options, $lineno, $tag = null)
+ public function __construct(\Twig_Node_Expression $expr, \Twig_Node_Expression $options, $lineno, $tag = null)
{
- parent::__construct(array('expr' => $expr, 'attributes' => $attributes, 'options' => $options), array(), $lineno, $tag);
+ parent::__construct(array('expr' => $expr, 'options' => $options), array(), $lineno, $tag);
}
/**
@@ -32,11 +32,9 @@ public function compile(\Twig_Compiler $compiler)
{
$compiler
->addDebugInfo($this)
- ->write("echo \$this->env->getExtension('actions')->renderAction(")
+ ->write("echo \$this->env->getExtension('actions')->renderUri(")
->subcompile($this->getNode('expr'))
->raw(', ')
- ->subcompile($this->getNode('attributes'))
- ->raw(', ')
->subcompile($this->getNode('options'))
->raw(");\n")
;
View
17 src/Symfony/Bundle/TwigBundle/TokenParser/RenderTokenParser.php
@@ -31,29 +31,16 @@ public function parse(\Twig_Token $token)
{
$expr = $this->parser->getExpressionParser()->parseExpression();
- // attributes
- if ($this->parser->getStream()->test(\Twig_Token::NAME_TYPE, 'with')) {
- $this->parser->getStream()->next();
-
- $hasAttributes = true;
- $attributes = $this->parser->getExpressionParser()->parseExpression();
- } else {
- $hasAttributes = false;
- $attributes = new \Twig_Node_Expression_Array(array(), $token->getLine());
- }
-
// options
- if ($hasAttributes && $this->parser->getStream()->test(\Twig_Token::PUNCTUATION_TYPE, ',')) {
+ if ($this->parser->getStream()->test(\Twig_Token::PUNCTUATION_TYPE, ',')) {
$this->parser->getStream()->next();
$options = $this->parser->getExpressionParser()->parseExpression();
- } else {
- $options = new \Twig_Node_Expression_Array(array(), $token->getLine());
}
$this->parser->getStream()->expect(\Twig_Token::BLOCK_END_TYPE);
- return new RenderNode($expr, $attributes, $options, $token->getLine(), $this->getTag());
+ return new RenderNode($expr, $options, $token->getLine(), $this->getTag());
@benja-M-1
benja-M-1 added a note Dec 20, 2012

If the condition is not true, $option is not defined which produces a PHP Notice "Notice: Undefined variable: options in [...]/vendor/symfony/symfony/src/Symfony/Bundle/TwigBundle/TokenParser/RenderTokenParser.php line 43"

@fabpot
Symfony member
fabpot added a note Dec 20, 2012

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
/**

11 comments on commit 64d43c8

@stof
Symfony member
stof commented on 64d43c8 Dec 20, 2012

@fabpot what is the reason for removing this feature, without even deprecating it in 2.2 before removing it ?

@fabpot
Symfony member
fabpot commented on 64d43c8 Dec 20, 2012
@leevigraham

The previous implementation of this allowed you to call controller methods that were never meant to be accesible via a URL pattern. For example a recent list of articles widget which returned a html snippet.

This implementation requires a route to be defined and all routes require the pattern parameter. Doesn't this mean that all snippets will be available via a URL?

@leevigraham

This commit also breaks the Doctrine section of the Web Profiler. See issue: #6448

@fabpot
Symfony member
fabpot commented on 64d43c8 Dec 21, 2012

@leevigraham: You are right. But as the vulnerability is major, we have no other choices. The article on the blog tries to explain that in details.

@asm89
asm89 commented on 64d43c8 Dec 21, 2012

With the firewall fixed, why is the complete _internal feature removed? It is possible to restrict access to the _internal routes only to reverse proxies? Or maybe some signing mechanism for _internal routes that add a kind of CSRF token to the _internal routes?

@asm89
asm89 commented on 64d43c8 Dec 21, 2012

@fabpot If we were to generate a route like /_internal/...html and then create a signature with something like: sha1('%framework.secret%' . $generatedUrl); then the receiving controller can validate that the url was generated by the framework?

If something like signing the url could indeed be a fix to the issue the upgrade would require no code changes for users.

Am I missing something?

@sukei
sukei commented on 64d43c8 Dec 21, 2012

What's about creating a new tag to handle ESI (and HInclude)? Something like:

{% esi path('posts_list', { page: 2 }) , { standalone: true } %}

So we can let the render block as partial renderer.

@weaverryan
Symfony member

In my experience, the vast majority of people use render as a "controller include" and never for ESI or HInclude. Requiring most users to worry about (a) creating a route for the controller and (b) properly securing that path is a lot of unfortunate developer overhead. Additionally, this opens up the real potential that many "normal" users will create routes for their controllers, but never actually secure them properly.

So, I don't know if it's possible, and maybe it's not really a big deal, but I'd also it to continue to be possible to render a controller via its logical name.

Thanks!

@lsmith77

see #6463

Please sign in to comment.
Something went wrong with that request. Please try again.