From fa8f4f8137444d267c087a404bc5532b14f10463 Mon Sep 17 00:00:00 2001 From: Christophe Coevoet Date: Tue, 2 Sep 2014 02:43:20 +0200 Subject: [PATCH 1/4] Forced all fragment uris to be signed, even for ESI --- EventListener/FragmentListener.php | 13 ++---- Fragment/EsiFragmentRenderer.php | 22 +++++++-- Tests/EventListener/FragmentListenerTest.php | 13 ------ Tests/Fragment/EsiFragmentRendererTest.php | 48 +++++++++++++++++++- 4 files changed, 71 insertions(+), 25 deletions(-) diff --git a/EventListener/FragmentListener.php b/EventListener/FragmentListener.php index ef3fad3d4c..3b0e3a9425 100644 --- a/EventListener/FragmentListener.php +++ b/EventListener/FragmentListener.php @@ -12,7 +12,6 @@ namespace Symfony\Component\HttpKernel\EventListener; use Symfony\Component\HttpFoundation\Request; -use Symfony\Component\HttpFoundation\IpUtils; use Symfony\Component\HttpKernel\Event\GetResponseEvent; use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; @@ -77,13 +76,6 @@ protected function validateRequest(Request $request) throw new AccessDeniedHttpException(); } - // does the Request come from a trusted IP? - $trustedIps = array_merge($this->getLocalIpAddresses(), $request->getTrustedProxies()); - $remoteAddress = $request->server->get('REMOTE_ADDR'); - if (IpUtils::checkIp($remoteAddress, $trustedIps)) { - return; - } - // is the Request signed? // we cannot use $request->getUri() here as we want to work with the original URI (no query string reordering) if ($this->signer->check($request->getSchemeAndHttpHost().$request->getBaseUrl().$request->getPathInfo().(null !== ($qs = $request->server->get('QUERY_STRING')) ? '?'.$qs : ''))) { @@ -93,6 +85,11 @@ protected function validateRequest(Request $request) throw new AccessDeniedHttpException(); } + /** + * @deprecated Deprecated since 2.3.19, to be removed in 3.0. + * + * @return string[] + */ protected function getLocalIpAddresses() { return array('127.0.0.1', 'fe80::1', '::1'); diff --git a/Fragment/EsiFragmentRenderer.php b/Fragment/EsiFragmentRenderer.php index e36e459506..8ff0e6ad44 100644 --- a/Fragment/EsiFragmentRenderer.php +++ b/Fragment/EsiFragmentRenderer.php @@ -15,6 +15,7 @@ use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Controller\ControllerReference; use Symfony\Component\HttpKernel\HttpCache\Esi; +use Symfony\Component\HttpKernel\UriSigner; /** * Implements the ESI rendering strategy. @@ -25,6 +26,7 @@ class EsiFragmentRenderer extends RoutableFragmentRenderer { private $esi; private $inlineStrategy; + private $signer; /** * Constructor. @@ -34,11 +36,13 @@ class EsiFragmentRenderer extends RoutableFragmentRenderer * * @param Esi $esi An Esi instance * @param FragmentRendererInterface $inlineStrategy The inline strategy to use when ESI is not supported + * @param UriSigner $signer */ - public function __construct(Esi $esi, FragmentRendererInterface $inlineStrategy) + public function __construct(Esi $esi, FragmentRendererInterface $inlineStrategy, UriSigner $signer = null) { $this->esi = $esi; $this->inlineStrategy = $inlineStrategy; + $this->signer = $signer; } /** @@ -61,12 +65,12 @@ public function render($uri, Request $request, array $options = array()) } if ($uri instanceof ControllerReference) { - $uri = $this->generateFragmentUri($uri, $request); + $uri = $this->generateSignedFragmentUri($uri, $request); } $alt = isset($options['alt']) ? $options['alt'] : null; if ($alt instanceof ControllerReference) { - $alt = $this->generateFragmentUri($alt, $request); + $alt = $this->generateSignedFragmentUri($alt, $request); } $tag = $this->esi->renderIncludeTag($uri, $alt, isset($options['ignore_errors']) ? $options['ignore_errors'] : false, isset($options['comment']) ? $options['comment'] : ''); @@ -81,4 +85,16 @@ public function getName() { return 'esi'; } + + private function generateSignedFragmentUri($uri, Request $request) + { + if (null === $this->signer) { + throw new \LogicException('You must use a URI when using the ESI rendering strategy or set a URL signer.'); + } + + // we need to sign the absolute URI, but want to return the path only. + $fragmentUri = $this->signer->sign($this->generateFragmentUri($uri, $request, true)); + + return substr($fragmentUri, strlen($request->getSchemeAndHttpHost())); + } } diff --git a/Tests/EventListener/FragmentListenerTest.php b/Tests/EventListener/FragmentListenerTest.php index 153d8a44ff..75562f7098 100644 --- a/Tests/EventListener/FragmentListenerTest.php +++ b/Tests/EventListener/FragmentListenerTest.php @@ -54,19 +54,6 @@ public function testAccessDeniedWithNonSafeMethods() $listener->onKernelRequest($event); } - /** - * @expectedException \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException - */ - public function testAccessDeniedWithNonLocalIps() - { - $request = Request::create('http://example.com/_fragment', 'GET', array(), array(), array(), array('REMOTE_ADDR' => '10.0.0.1')); - - $listener = new FragmentListener(new UriSigner('foo')); - $event = $this->createGetResponseEvent($request); - - $listener->onKernelRequest($event); - } - /** * @expectedException \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException */ diff --git a/Tests/Fragment/EsiFragmentRendererTest.php b/Tests/Fragment/EsiFragmentRendererTest.php index 1a63c4df6f..0c3ed8a2fb 100644 --- a/Tests/Fragment/EsiFragmentRendererTest.php +++ b/Tests/Fragment/EsiFragmentRendererTest.php @@ -15,6 +15,7 @@ use Symfony\Component\HttpKernel\Fragment\EsiFragmentRenderer; use Symfony\Component\HttpKernel\HttpCache\Esi; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpKernel\UriSigner; class EsiFragmentRendererTest extends \PHPUnit_Framework_TestCase { @@ -48,7 +49,52 @@ public function testRender() $this->assertEquals('', $strategy->render('/', $request)->getContent()); $this->assertEquals("\n", $strategy->render('/', $request, array('comment' => 'This is a comment'))->getContent()); $this->assertEquals('', $strategy->render('/', $request, array('alt' => 'foo'))->getContent()); - $this->assertEquals('', $strategy->render(new ControllerReference('main_controller', array(), array()), $request, array('alt' => new ControllerReference('alt_controller', array(), array())))->getContent()); + } + + public function testRenderControllerReference() + { + $signer = new UriSigner('foo'); + $strategy = new EsiFragmentRenderer(new Esi(), $this->getInlineStrategy(), $signer); + + $request = Request::create('/'); + $request->setLocale('fr'); + $request->headers->set('Surrogate-Capability', 'ESI/1.0'); + + $reference = new ControllerReference('main_controller', array(), array()); + $altReference = new ControllerReference('alt_controller', array(), array()); + + $this->assertEquals( + '', + $strategy->render($reference, $request, array('alt' => $altReference))->getContent() + ); + } + + /** + * @expectedException \LogicException + */ + public function testRenderControllerReferenceWithoutSignerThrowsException() + { + $strategy = new EsiFragmentRenderer(new Esi(), $this->getInlineStrategy()); + + $request = Request::create('/'); + $request->setLocale('fr'); + $request->headers->set('Surrogate-Capability', 'ESI/1.0'); + + $strategy->render(new ControllerReference('main_controller'), $request); + } + + /** + * @expectedException \LogicException + */ + public function testRenderAltControllerReferenceWithoutSignerThrowsException() + { + $strategy = new EsiFragmentRenderer(new Esi(), $this->getInlineStrategy()); + + $request = Request::create('/'); + $request->setLocale('fr'); + $request->headers->set('Surrogate-Capability', 'ESI/1.0'); + + $strategy->render('/', $request, array('alt' => new ControllerReference('alt_controller'))); } private function getInlineStrategy($called = false) From f3e0f0eb4c8fca174b5743e8811c12f8323b7fea Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 3 Sep 2014 10:09:50 +0200 Subject: [PATCH 2/4] [HttpKernel] fixed internal fragment handling --- EventListener/FragmentListener.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/EventListener/FragmentListener.php b/EventListener/FragmentListener.php index 3b0e3a9425..6f45c3b129 100644 --- a/EventListener/FragmentListener.php +++ b/EventListener/FragmentListener.php @@ -16,6 +16,7 @@ use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException; use Symfony\Component\HttpKernel\UriSigner; +use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\EventDispatcher\EventSubscriberInterface; /** @@ -24,8 +25,8 @@ * All URL paths starting with /_fragment are handled as * content fragments by this listener. * - * If the request does not come from a trusted IP, it throws an - * AccessDeniedHttpException exception. + * If throws an AccessDeniedHttpException exception if the request + * is not signed or if it is not an internal sub-request. * * @author Fabien Potencier */ @@ -61,7 +62,9 @@ public function onKernelRequest(GetResponseEvent $event) return; } - $this->validateRequest($request); + if (HttpKernelInterface::MASTER_REQUEST === $event->getRequestType()) { + $this->validateRequest($request); + } parse_str($request->query->get('_path', ''), $attributes); $request->attributes->add($attributes); From 2fa91bdacdf0eb0241f3c5dd2c80aa6ddb45b468 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 3 Sep 2014 10:44:56 +0200 Subject: [PATCH 3/4] [HttpKernel] simplified code --- EventListener/FragmentListener.php | 2 +- EventListener/SessionListener.php | 2 +- EventListener/TestSessionListener.php | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/EventListener/FragmentListener.php b/EventListener/FragmentListener.php index 6f45c3b129..1a198a55f6 100644 --- a/EventListener/FragmentListener.php +++ b/EventListener/FragmentListener.php @@ -62,7 +62,7 @@ public function onKernelRequest(GetResponseEvent $event) return; } - if (HttpKernelInterface::MASTER_REQUEST === $event->getRequestType()) { + if ($event->isMasterRequest()) { $this->validateRequest($request); } diff --git a/EventListener/SessionListener.php b/EventListener/SessionListener.php index 2fe5c0356c..4c10418d6e 100644 --- a/EventListener/SessionListener.php +++ b/EventListener/SessionListener.php @@ -25,7 +25,7 @@ abstract class SessionListener implements EventSubscriberInterface { public function onKernelRequest(GetResponseEvent $event) { - if (HttpKernelInterface::MASTER_REQUEST !== $event->getRequestType()) { + if (!$event->isMasterRequest()) { return; } diff --git a/EventListener/TestSessionListener.php b/EventListener/TestSessionListener.php index 4d3232324a..794d45e720 100644 --- a/EventListener/TestSessionListener.php +++ b/EventListener/TestSessionListener.php @@ -30,7 +30,7 @@ abstract class TestSessionListener implements EventSubscriberInterface { public function onKernelRequest(GetResponseEvent $event) { - if (HttpKernelInterface::MASTER_REQUEST !== $event->getRequestType()) { + if (!$event->isMasterRequest()) { return; } @@ -55,7 +55,7 @@ public function onKernelRequest(GetResponseEvent $event) */ public function onKernelResponse(FilterResponseEvent $event) { - if (HttpKernelInterface::MASTER_REQUEST !== $event->getRequestType()) { + if (!$event->isMasterRequest()) { return; } From 9a071a677e5bada5b4cb31527305e1f318c77409 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 3 Sep 2014 10:50:47 +0200 Subject: [PATCH 4/4] [HttpKernel] fixed some unit tests for 2.4 (signature now uses SHA256 instead of MD5) --- Tests/Fragment/EsiFragmentRendererTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Fragment/EsiFragmentRendererTest.php b/Tests/Fragment/EsiFragmentRendererTest.php index 2b9016d082..90768f9dac 100644 --- a/Tests/Fragment/EsiFragmentRendererTest.php +++ b/Tests/Fragment/EsiFragmentRendererTest.php @@ -57,7 +57,7 @@ public function testRenderControllerReference() $altReference = new ControllerReference('alt_controller', array(), array()); $this->assertEquals( - '', + '', $strategy->render($reference, $request, array('alt' => $altReference))->getContent() ); }