From 4c262d4aedd0242929eb1d92f8e14d8ef52146b6 Mon Sep 17 00:00:00 2001 From: Magnus Nordlander Date: Mon, 2 May 2016 14:34:55 +0200 Subject: [PATCH 01/11] Emit a warning when a request has both a trusted Forwarded header and a trusted X-Forwarded-For header, as this is most likely a misconfiguration which causes security flaws. --- src/Symfony/Component/HttpFoundation/Request.php | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index da6244ddb917c..4b3826fdcb0d1 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -805,11 +805,18 @@ public function getClientIps() return array($ip); } - if (self::$trustedHeaders[self::HEADER_FORWARDED] && $this->headers->has(self::$trustedHeaders[self::HEADER_FORWARDED])) { + $hasTrustedForwardedHeader = self::$trustedHeaders[self::HEADER_FORWARDED] && $this->headers->has(self::$trustedHeaders[self::HEADER_FORWARDED]); + $hasTrustedClientIpHeader = self::$trustedHeaders[self::HEADER_CLIENT_IP] && $this->headers->has(self::$trustedHeaders[self::HEADER_CLIENT_IP]); + + if ($hasTrustedForwardedHeader && $hasTrustedClientIpHeader) { + trigger_error("The request has both a trusted Forwarded header and a trusted Client IP header. This is likely a misconfiguration. You should either configure your proxy only to send one of these headers, or configure Symfony to distrust one of them. When both headers are set and trusted, this method returns only IPs from the Forwarded header.", E_USER_WARNING); + } + + if ($hasTrustedForwardedHeader) { $forwardedHeader = $this->headers->get(self::$trustedHeaders[self::HEADER_FORWARDED]); preg_match_all('{(for)=("?\[?)([a-z0-9\.:_\-/]*)}', $forwardedHeader, $matches); $clientIps = $matches[3]; - } elseif (self::$trustedHeaders[self::HEADER_CLIENT_IP] && $this->headers->has(self::$trustedHeaders[self::HEADER_CLIENT_IP])) { + } elseif ($hasTrustedClientIpHeader) { $clientIps = array_map('trim', explode(',', $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_IP]))); } From da4b6c5fc610c66b3a2e58f7eb9499562245a6ae Mon Sep 17 00:00:00 2001 From: Magnus Nordlander Date: Tue, 3 May 2016 14:26:36 +0200 Subject: [PATCH 02/11] Updated according to fabbot review --- src/Symfony/Component/HttpFoundation/Request.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index 4b3826fdcb0d1..34b4e9d8c4429 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -809,7 +809,7 @@ public function getClientIps() $hasTrustedClientIpHeader = self::$trustedHeaders[self::HEADER_CLIENT_IP] && $this->headers->has(self::$trustedHeaders[self::HEADER_CLIENT_IP]); if ($hasTrustedForwardedHeader && $hasTrustedClientIpHeader) { - trigger_error("The request has both a trusted Forwarded header and a trusted Client IP header. This is likely a misconfiguration. You should either configure your proxy only to send one of these headers, or configure Symfony to distrust one of them. When both headers are set and trusted, this method returns only IPs from the Forwarded header.", E_USER_WARNING); + trigger_error('The request has both a trusted Forwarded header and a trusted Client IP header. This is likely a misconfiguration. You should either configure your proxy only to send one of these headers, or configure Symfony to distrust one of them. When both headers are set and trusted, this method returns only IPs from the Forwarded header.', E_USER_WARNING); } if ($hasTrustedForwardedHeader) { From f0c3d15b5743f438a6be338ee9bdfd4a6775b683 Mon Sep 17 00:00:00 2001 From: Magnus Nordlander Date: Thu, 5 May 2016 13:30:22 +0200 Subject: [PATCH 03/11] Changed warning to exception, and only throws when headers disagree. --- .../Exception/ConflictingHeadersException.php | 24 ++++++ .../Component/HttpFoundation/Request.php | 76 +++++++++++-------- .../HttpFoundation/Tests/RequestTest.php | 68 +++++++++++++++++ 3 files changed, 138 insertions(+), 30 deletions(-) create mode 100644 src/Symfony/Component/HttpFoundation/Exception/ConflictingHeadersException.php diff --git a/src/Symfony/Component/HttpFoundation/Exception/ConflictingHeadersException.php b/src/Symfony/Component/HttpFoundation/Exception/ConflictingHeadersException.php new file mode 100644 index 0000000000000..233b848cbf7e0 --- /dev/null +++ b/src/Symfony/Component/HttpFoundation/Exception/ConflictingHeadersException.php @@ -0,0 +1,24 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpFoundation\Exception; + +/** + * The HTTP request contains headers with conflicting information. + * + * This exception should trigger an HTTP 400 response in your application code. + * + * @author Magnus Nordlander + */ +class ConflictingHeadersException extends \RuntimeException +{ + +} diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index 34b4e9d8c4429..c9f7c51ee54d7 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -11,6 +11,7 @@ namespace Symfony\Component\HttpFoundation; +use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException; use Symfony\Component\HttpFoundation\Session\SessionInterface; /** @@ -808,45 +809,29 @@ public function getClientIps() $hasTrustedForwardedHeader = self::$trustedHeaders[self::HEADER_FORWARDED] && $this->headers->has(self::$trustedHeaders[self::HEADER_FORWARDED]); $hasTrustedClientIpHeader = self::$trustedHeaders[self::HEADER_CLIENT_IP] && $this->headers->has(self::$trustedHeaders[self::HEADER_CLIENT_IP]); - if ($hasTrustedForwardedHeader && $hasTrustedClientIpHeader) { - trigger_error('The request has both a trusted Forwarded header and a trusted Client IP header. This is likely a misconfiguration. You should either configure your proxy only to send one of these headers, or configure Symfony to distrust one of them. When both headers are set and trusted, this method returns only IPs from the Forwarded header.', E_USER_WARNING); - } - if ($hasTrustedForwardedHeader) { $forwardedHeader = $this->headers->get(self::$trustedHeaders[self::HEADER_FORWARDED]); preg_match_all('{(for)=("?\[?)([a-z0-9\.:_\-/]*)}', $forwardedHeader, $matches); - $clientIps = $matches[3]; - } elseif ($hasTrustedClientIpHeader) { - $clientIps = array_map('trim', explode(',', $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_IP]))); - } - - $clientIps[] = $ip; // Complete the IP chain with the IP the request actually came from - $firstTrustedIp = null; + $forwardedClientIps = $matches[3]; - foreach ($clientIps as $key => $clientIp) { - // Remove port (unfortunately, it does happen) - if (preg_match('{((?:\d+\.){3}\d+)\:\d+}', $clientIp, $match)) { - $clientIps[$key] = $clientIp = $match[1]; - } + $forwardedClientIps = $this->normalizeAndFilterClientIps($forwardedClientIps, $ip); + $clientIps = $forwardedClientIps; + } - if (!filter_var($clientIp, FILTER_VALIDATE_IP)) { - unset($clientIps[$key]); + if ($hasTrustedClientIpHeader) { + $xForwardedForClientIps = array_map('trim', explode(',', $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_IP]))); - continue; - } - - if (IpUtils::checkIp($clientIp, self::$trustedProxies)) { - unset($clientIps[$key]); + $xForwardedForClientIps = $this->normalizeAndFilterClientIps($xForwardedForClientIps, $ip); + $clientIps = $xForwardedForClientIps; + } - // Fallback to this when the client IP falls into the range of trusted proxies - if (null === $firstTrustedIp) { - $firstTrustedIp = $clientIp; - } - } + if ($hasTrustedForwardedHeader && $hasTrustedClientIpHeader && $forwardedClientIps !== $xForwardedForClientIps) { + throw new ConflictingHeadersException('The request has both a trusted Forwarded header and a trusted Client IP header, conflicting with each other with regards to the originating IP addresses of the request. This is the result of a misconfiguration. You should either configure your proxy only to send one of these headers, or configure Symfony to distrust one of them.'); + } elseif (!$hasTrustedForwardedHeader && !$hasTrustedClientIpHeader) { + return $this->normalizeAndFilterClientIps(array(), $ip); } - // Now the IP chain contains only untrusted proxies and the client IP - return $clientIps ? array_reverse($clientIps) : array($firstTrustedIp); + return $clientIps; } /** @@ -1937,4 +1922,35 @@ private function isFromTrustedProxy() { return self::$trustedProxies && IpUtils::checkIp($this->server->get('REMOTE_ADDR'), self::$trustedProxies); } + + private function normalizeAndFilterClientIps($clientIps, $ip) + { + $clientIps[] = $ip; // Complete the IP chain with the IP the request actually came from + $firstTrustedIp = null; + + foreach ($clientIps as $key => $clientIp) { + // Remove port (unfortunately, it does happen) + if (preg_match('{((?:\d+\.){3}\d+)\:\d+}', $clientIp, $match)) { + $clientIps[$key] = $clientIp = $match[1]; + } + + if (!filter_var($clientIp, FILTER_VALIDATE_IP)) { + unset($clientIps[$key]); + + continue; + } + + if (IpUtils::checkIp($clientIp, self::$trustedProxies)) { + unset($clientIps[$key]); + + // Fallback to this when the client IP falls into the range of trusted proxies + if (null === $firstTrustedIp) { + $firstTrustedIp = $clientIp; + } + } + } + + // Now the IP chain contains only untrusted proxies and the client IP + return $clientIps ? array_reverse($clientIps) : array($firstTrustedIp); + } } diff --git a/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php b/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php index 89c4eb2d4272c..1edc48c1b62db 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php @@ -923,6 +923,74 @@ public function testGetClientIpsProvider() ); } + /** + * @expectedException \Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException + * @dataProvider testGetClientIpsWithConflictingHeadersProvider + */ + public function testGetClientIpsWithConflictingHeaders($httpForwarded, $httpXForwardedFor) + { + $request = new Request(); + + $server = array( + 'REMOTE_ADDR' => '88.88.88.88', + 'HTTP_FORWARDED' => $httpForwarded, + 'HTTP_X_FORWARDED_FOR' => $httpXForwardedFor, + ); + + Request::setTrustedProxies(array('88.88.88.88')); + + $request->initialize(array(), array(), array(), array(), array(), $server); + + $request->getClientIps(); + } + + public function testGetClientIpsWithConflictingHeadersProvider() + { + // $httpForwarded $httpXForwardedFor + return array( + array('for=87.65.43.21', '192.0.2.60'), + array('for=87.65.43.21, for=192.0.2.60', '192.0.2.60'), + array('for=192.0.2.60', '192.0.2.60,87.65.43.21'), + array('for="::face", for=192.0.2.60', '192.0.2.60,192.0.2.43'), + array('for=87.65.43.21, for=192.0.2.60', '192.0.2.60,87.65.43.21'), + ); + } + + /** + * @dataProvider testGetClientIpsWithAgreeingHeadersProvider + */ + public function testGetClientIpsWithAgreeingHeaders($httpForwarded, $httpXForwardedFor) + { + $request = new Request(); + + $server = array( + 'REMOTE_ADDR' => '88.88.88.88', + 'HTTP_FORWARDED' => $httpForwarded, + 'HTTP_X_FORWARDED_FOR' => $httpXForwardedFor, + ); + + Request::setTrustedProxies(array('88.88.88.88')); + + $request->initialize(array(), array(), array(), array(), array(), $server); + + $request->getClientIps(); + + Request::setTrustedProxies(array()); + } + + public function testGetClientIpsWithAgreeingHeadersProvider() + { + // $httpForwarded $httpXForwardedFor + return array( + array('for="192.0.2.60"', '192.0.2.60'), + array('for=192.0.2.60, for=87.65.43.21', '192.0.2.60,87.65.43.21'), + array('for="[::face]", for=192.0.2.60', '::face,192.0.2.60'), + array('for="192.0.2.60:80"', '192.0.2.60'), + array('for=192.0.2.60;proto=http;by=203.0.113.43', '192.0.2.60'), + array('for="[2001:db8:cafe::17]:4711"', '2001:db8:cafe::17'), + ); + } + public function testGetContentWorksTwiceInDefaultMode() { $req = new Request(); From 2eea0f1bb2e08b37d983972da373451d0a8b57e6 Mon Sep 17 00:00:00 2001 From: Magnus Nordlander Date: Thu, 5 May 2016 13:35:31 +0200 Subject: [PATCH 04/11] CS fix --- .../HttpFoundation/Exception/ConflictingHeadersException.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Symfony/Component/HttpFoundation/Exception/ConflictingHeadersException.php b/src/Symfony/Component/HttpFoundation/Exception/ConflictingHeadersException.php index 233b848cbf7e0..fa5f1c7873270 100644 --- a/src/Symfony/Component/HttpFoundation/Exception/ConflictingHeadersException.php +++ b/src/Symfony/Component/HttpFoundation/Exception/ConflictingHeadersException.php @@ -20,5 +20,4 @@ */ class ConflictingHeadersException extends \RuntimeException { - } From c0172900e5a069d6b26b4f498fbdeb3055853bb1 Mon Sep 17 00:00:00 2001 From: Magnus Nordlander Date: Sat, 25 Jun 2016 11:29:22 +0200 Subject: [PATCH 05/11] Added a listener to validate requests --- .../FrameworkBundle/Resources/config/web.xml | 4 ++ .../Component/HttpFoundation/Request.php | 8 +-- .../ValidateRequestClientIpListener.php | 54 +++++++++++++++++++ .../ValidateRequestClientIpTest.php | 53 ++++++++++++++++++ 4 files changed, 116 insertions(+), 3 deletions(-) create mode 100644 src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php create mode 100644 src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml index 9b2f3cb3a4373..b27d468c91383 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml +++ b/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml @@ -46,5 +46,9 @@ + + + + diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index c9f7c51ee54d7..8b23b92aa9319 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -827,7 +827,9 @@ public function getClientIps() if ($hasTrustedForwardedHeader && $hasTrustedClientIpHeader && $forwardedClientIps !== $xForwardedForClientIps) { throw new ConflictingHeadersException('The request has both a trusted Forwarded header and a trusted Client IP header, conflicting with each other with regards to the originating IP addresses of the request. This is the result of a misconfiguration. You should either configure your proxy only to send one of these headers, or configure Symfony to distrust one of them.'); - } elseif (!$hasTrustedForwardedHeader && !$hasTrustedClientIpHeader) { + } + + if (!$hasTrustedForwardedHeader && !$hasTrustedClientIpHeader) { return $this->normalizeAndFilterClientIps(array(), $ip); } @@ -1923,7 +1925,7 @@ private function isFromTrustedProxy() return self::$trustedProxies && IpUtils::checkIp($this->server->get('REMOTE_ADDR'), self::$trustedProxies); } - private function normalizeAndFilterClientIps($clientIps, $ip) + private function normalizeAndFilterClientIps(array $clientIps, $ip) { $clientIps[] = $ip; // Complete the IP chain with the IP the request actually came from $firstTrustedIp = null; @@ -1944,7 +1946,7 @@ private function normalizeAndFilterClientIps($clientIps, $ip) unset($clientIps[$key]); // Fallback to this when the client IP falls into the range of trusted proxies - if (null === $firstTrustedIp) { + if (null === $firstTrustedIp) { $firstTrustedIp = $clientIp; } } diff --git a/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php b/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php new file mode 100644 index 0000000000000..35306384bfe21 --- /dev/null +++ b/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php @@ -0,0 +1,54 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpKernel\EventListener; + +use Symfony\Component\EventDispatcher\EventSubscriberInterface; +use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException; +use Symfony\Component\HttpKernel\Event\GetResponseEvent; +use Symfony\Component\HttpKernel\Exception\HttpException; +use Symfony\Component\HttpKernel\KernelEvents; + +/** + * Validates that the headers and other information indicating the + * client IP address of a request are consistent. + * + * @author Magnus Nordlander + */ +class ValidateRequestClientIpListener implements EventSubscriberInterface +{ + /** + * Performs the validation + * + * @param GetResponseEvent $event + */ + public function onKernelRequest(GetResponseEvent $event) + { + try { + // This will throw an exception if the headers are inconsistent. + $event->getRequest()->getClientIps(); + } catch (ConflictingHeadersException $e) { + throw new HttpException(400, "The request headers contain conflicting information regarding the origin of this request.", $e); + } + } + + /** + * {@inheritdoc} + */ + public static function getSubscribedEvents() + { + return array( + KernelEvents::REQUEST => array( + array('onKernelRequest', 256), + ), + ); + } +} diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php new file mode 100644 index 0000000000000..8027860957094 --- /dev/null +++ b/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php @@ -0,0 +1,53 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpKernel\Tests\EventListener; + +use Symfony\Component\EventDispatcher\EventDispatcher; +use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpKernel\EventListener\ValidateRequestClientIpListener; +use Symfony\Component\HttpKernel\Event\GetResponseEvent; +use Symfony\Component\HttpKernel\HttpKernelInterface; +use Symfony\Component\HttpKernel\KernelEvents; + +class ValidateRequestClientIpTest extends \PHPUnit_Framework_TestCase +{ + public function testListenerThrowsOnInconsistentRequests() + { + $dispatcher = new EventDispatcher(); + $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); + $listener = new ValidateRequestClientIpListener(); + $request = $this->getMock('Symfony\Component\HttpFoundation\Request'); + $request->method('getClientIps') + ->will($this->throwException(new ConflictingHeadersException)); + + $dispatcher->addListener(KernelEvents::REQUEST, array($listener, 'onKernelRequest')); + $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST); + + $this->setExpectedException('Symfony\Component\HttpKernel\Exception\HttpException'); + $dispatcher->dispatch(KernelEvents::REQUEST, $event); + } + + public function testListenerDoesNothingOnConsistenRequests() + { + $dispatcher = new EventDispatcher(); + $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); + $listener = new ValidateRequestClientIpListener(); + $request = $this->getMock('Symfony\Component\HttpFoundation\Request'); + $request->method('getClientIps') + ->willReturn(['127.0.0.1']); + + $dispatcher->addListener(KernelEvents::REQUEST, array($listener, 'onKernelRequest')); + $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST); + $dispatcher->dispatch(KernelEvents::REQUEST, $event); + } +} From 99b072e19f63689626e69a211d18a7043e748647 Mon Sep 17 00:00:00 2001 From: Magnus Nordlander Date: Sat, 25 Jun 2016 11:59:51 +0200 Subject: [PATCH 06/11] Changed listener to only throw on master requests (to preserve exception handling) --- .../ValidateRequestClientIpListener.php | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php b/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php index 35306384bfe21..34383c7fee308 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php @@ -15,6 +15,7 @@ use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException; use Symfony\Component\HttpKernel\Event\GetResponseEvent; use Symfony\Component\HttpKernel\Exception\HttpException; +use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\HttpKernel\KernelEvents; /** @@ -32,12 +33,15 @@ class ValidateRequestClientIpListener implements EventSubscriberInterface */ public function onKernelRequest(GetResponseEvent $event) { - try { - // This will throw an exception if the headers are inconsistent. - $event->getRequest()->getClientIps(); - } catch (ConflictingHeadersException $e) { - throw new HttpException(400, "The request headers contain conflicting information regarding the origin of this request.", $e); + if ($event->getRequestType() == HttpKernelInterface::MASTER_REQUEST) { + try { + // This will throw an exception if the headers are inconsistent. + $event->getRequest()->getClientIps(); + } catch (ConflictingHeadersException $e) { + throw new HttpException(400, "The request headers contain conflicting information regarding the origin of this request.", $e); + } } + } /** From 41d2603c0e246a1896c259c897551d4fbea75e13 Mon Sep 17 00:00:00 2001 From: Magnus Nordlander Date: Sat, 25 Jun 2016 12:03:54 +0200 Subject: [PATCH 07/11] Added fabbot CS fixes --- .../EventListener/ValidateRequestClientIpListener.php | 5 ++--- .../Tests/EventListener/ValidateRequestClientIpTest.php | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php b/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php index 34383c7fee308..8e9b393b9fdbb 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php @@ -27,7 +27,7 @@ class ValidateRequestClientIpListener implements EventSubscriberInterface { /** - * Performs the validation + * Performs the validation. * * @param GetResponseEvent $event */ @@ -38,10 +38,9 @@ public function onKernelRequest(GetResponseEvent $event) // This will throw an exception if the headers are inconsistent. $event->getRequest()->getClientIps(); } catch (ConflictingHeadersException $e) { - throw new HttpException(400, "The request headers contain conflicting information regarding the origin of this request.", $e); + throw new HttpException(400, 'The request headers contain conflicting information regarding the origin of this request.', $e); } } - } /** diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php index 8027860957094..30d2c29e18e80 100644 --- a/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php @@ -28,7 +28,7 @@ public function testListenerThrowsOnInconsistentRequests() $listener = new ValidateRequestClientIpListener(); $request = $this->getMock('Symfony\Component\HttpFoundation\Request'); $request->method('getClientIps') - ->will($this->throwException(new ConflictingHeadersException)); + ->will($this->throwException(new ConflictingHeadersException())); $dispatcher->addListener(KernelEvents::REQUEST, array($listener, 'onKernelRequest')); $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST); @@ -44,7 +44,7 @@ public function testListenerDoesNothingOnConsistenRequests() $listener = new ValidateRequestClientIpListener(); $request = $this->getMock('Symfony\Component\HttpFoundation\Request'); $request->method('getClientIps') - ->willReturn(['127.0.0.1']); + ->willReturn(array('127.0.0.1')); $dispatcher->addListener(KernelEvents::REQUEST, array($listener, 'onKernelRequest')); $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST); From ea691e70b1c95cd0306dcdd1c956314484a1d07e Mon Sep 17 00:00:00 2001 From: Magnus Nordlander Date: Sat, 25 Jun 2016 12:09:49 +0200 Subject: [PATCH 08/11] Made the profiler handle unknown client IPs. --- src/Symfony/Component/HttpKernel/Profiler/Profiler.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Component/HttpKernel/Profiler/Profiler.php b/src/Symfony/Component/HttpKernel/Profiler/Profiler.php index 864f624729d54..35d3a8f1b4713 100644 --- a/src/Symfony/Component/HttpKernel/Profiler/Profiler.php +++ b/src/Symfony/Component/HttpKernel/Profiler/Profiler.php @@ -11,6 +11,7 @@ namespace Symfony\Component\HttpKernel\Profiler; +use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\DataCollector\DataCollectorInterface; @@ -200,9 +201,13 @@ public function collect(Request $request, Response $response, \Exception $except $profile = new Profile(substr(hash('sha256', uniqid(mt_rand(), true)), 0, 6)); $profile->setTime(time()); $profile->setUrl($request->getUri()); - $profile->setIp($request->getClientIp()); $profile->setMethod($request->getMethod()); $profile->setStatusCode($response->getStatusCode()); + try { + $profile->setIp($request->getClientIp()); + } catch (ConflictingHeadersException $e) { + $profile->setIp('Unknown'); + } $response->headers->set('X-Debug-Token', $profile->getToken()); From a9f864d1176a29792d7a8cfbc11ac1e897a2eacd Mon Sep 17 00:00:00 2001 From: Magnus Nordlander Date: Sat, 25 Jun 2016 12:23:10 +0200 Subject: [PATCH 09/11] Updated tests --- .../ValidateRequestClientIpTest.php | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php index 30d2c29e18e80..c1ff7fedcd202 100644 --- a/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php @@ -21,7 +21,7 @@ class ValidateRequestClientIpTest extends \PHPUnit_Framework_TestCase { - public function testListenerThrowsOnInconsistentRequests() + public function testListenerThrowsOnInconsistentMasterRequests() { $dispatcher = new EventDispatcher(); $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); @@ -31,13 +31,13 @@ public function testListenerThrowsOnInconsistentRequests() ->will($this->throwException(new ConflictingHeadersException())); $dispatcher->addListener(KernelEvents::REQUEST, array($listener, 'onKernelRequest')); - $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST); + $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST); $this->setExpectedException('Symfony\Component\HttpKernel\Exception\HttpException'); $dispatcher->dispatch(KernelEvents::REQUEST, $event); } - public function testListenerDoesNothingOnConsistenRequests() + public function testListenerDoesNothingOnConsistentRequests() { $dispatcher = new EventDispatcher(); $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); @@ -46,6 +46,20 @@ public function testListenerDoesNothingOnConsistenRequests() $request->method('getClientIps') ->willReturn(array('127.0.0.1')); + $dispatcher->addListener(KernelEvents::REQUEST, array($listener, 'onKernelRequest')); + $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST); + $dispatcher->dispatch(KernelEvents::REQUEST, $event); + } + + public function testListenerDoesNothingOnSubrequests() + { + $dispatcher = new EventDispatcher(); + $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); + $listener = new ValidateRequestClientIpListener(); + $request = $this->getMock('Symfony\Component\HttpFoundation\Request'); + $request->method('getClientIps') + ->will($this->throwException(new ConflictingHeadersException())); + $dispatcher->addListener(KernelEvents::REQUEST, array($listener, 'onKernelRequest')); $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST); $dispatcher->dispatch(KernelEvents::REQUEST, $event); From 66f19f12cb450f22d7092ee085587bdd2371abf7 Mon Sep 17 00:00:00 2001 From: Magnus Nordlander Date: Tue, 28 Jun 2016 15:53:43 +0200 Subject: [PATCH 10/11] Renamed listener and CS fixes. --- .../Bundle/FrameworkBundle/Resources/config/web.xml | 2 +- ...tClientIpListener.php => ValidateRequestListener.php} | 9 ++++----- ...tClientIpTest.php => ValidateRequestListenerTest.php} | 7 +++---- 3 files changed, 8 insertions(+), 10 deletions(-) rename src/Symfony/Component/HttpKernel/EventListener/{ValidateRequestClientIpListener.php => ValidateRequestListener.php} (76%) rename src/Symfony/Component/HttpKernel/Tests/EventListener/{ValidateRequestClientIpTest.php => ValidateRequestListenerTest.php} (90%) diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml index b27d468c91383..c1f73e561038a 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml +++ b/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml @@ -47,7 +47,7 @@ - + diff --git a/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php b/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestListener.php similarity index 76% rename from src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php rename to src/Symfony/Component/HttpKernel/EventListener/ValidateRequestListener.php index 8e9b393b9fdbb..6316b77ffed2c 100644 --- a/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php +++ b/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestListener.php @@ -14,8 +14,7 @@ use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException; use Symfony\Component\HttpKernel\Event\GetResponseEvent; -use Symfony\Component\HttpKernel\Exception\HttpException; -use Symfony\Component\HttpKernel\HttpKernelInterface; +use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; use Symfony\Component\HttpKernel\KernelEvents; /** @@ -24,7 +23,7 @@ * * @author Magnus Nordlander */ -class ValidateRequestClientIpListener implements EventSubscriberInterface +class ValidateRequestListener implements EventSubscriberInterface { /** * Performs the validation. @@ -33,12 +32,12 @@ class ValidateRequestClientIpListener implements EventSubscriberInterface */ public function onKernelRequest(GetResponseEvent $event) { - if ($event->getRequestType() == HttpKernelInterface::MASTER_REQUEST) { + if ($event->isMasterRequest()) { try { // This will throw an exception if the headers are inconsistent. $event->getRequest()->getClientIps(); } catch (ConflictingHeadersException $e) { - throw new HttpException(400, 'The request headers contain conflicting information regarding the origin of this request.', $e); + throw new BadRequestHttpException('The request headers contain conflicting information regarding the origin of this request.', $e); } } } diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestListenerTest.php similarity index 90% rename from src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php rename to src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestListenerTest.php index c1ff7fedcd202..b582ae6d52dce 100644 --- a/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestClientIpTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestListenerTest.php @@ -14,14 +14,13 @@ use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException; use Symfony\Component\HttpFoundation\Request; -use Symfony\Component\HttpKernel\EventListener\ValidateRequestClientIpListener; use Symfony\Component\HttpKernel\Event\GetResponseEvent; use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\HttpKernel\KernelEvents; -class ValidateRequestClientIpTest extends \PHPUnit_Framework_TestCase +class ValidateRequestListenerTest extends \PHPUnit_Framework_TestCase { - public function testListenerThrowsOnInconsistentMasterRequests() + public function testListenerThrowsWhenMasterRequestHasInconsistentClientIps() { $dispatcher = new EventDispatcher(); $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); @@ -37,7 +36,7 @@ public function testListenerThrowsOnInconsistentMasterRequests() $dispatcher->dispatch(KernelEvents::REQUEST, $event); } - public function testListenerDoesNothingOnConsistentRequests() + public function testListenerDoesNothingOnValidRequests() { $dispatcher = new EventDispatcher(); $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); From d52d098db8636cdb3620e1a347b5a47db217ae04 Mon Sep 17 00:00:00 2001 From: Magnus Nordlander Date: Tue, 28 Jun 2016 15:55:36 +0200 Subject: [PATCH 11/11] Fixed test failures --- .../Tests/EventListener/ValidateRequestListenerTest.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestListenerTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestListenerTest.php index b582ae6d52dce..0f6db8d88036d 100644 --- a/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestListenerTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestListenerTest.php @@ -14,6 +14,7 @@ use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpKernel\EventListener\ValidateRequestListener; use Symfony\Component\HttpKernel\Event\GetResponseEvent; use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\HttpKernel\KernelEvents; @@ -24,7 +25,7 @@ public function testListenerThrowsWhenMasterRequestHasInconsistentClientIps() { $dispatcher = new EventDispatcher(); $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); - $listener = new ValidateRequestClientIpListener(); + $listener = new ValidateRequestListener(); $request = $this->getMock('Symfony\Component\HttpFoundation\Request'); $request->method('getClientIps') ->will($this->throwException(new ConflictingHeadersException())); @@ -32,7 +33,7 @@ public function testListenerThrowsWhenMasterRequestHasInconsistentClientIps() $dispatcher->addListener(KernelEvents::REQUEST, array($listener, 'onKernelRequest')); $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST); - $this->setExpectedException('Symfony\Component\HttpKernel\Exception\HttpException'); + $this->setExpectedException('Symfony\Component\HttpKernel\Exception\BadRequestHttpException'); $dispatcher->dispatch(KernelEvents::REQUEST, $event); } @@ -40,7 +41,7 @@ public function testListenerDoesNothingOnValidRequests() { $dispatcher = new EventDispatcher(); $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); - $listener = new ValidateRequestClientIpListener(); + $listener = new ValidateRequestListener(); $request = $this->getMock('Symfony\Component\HttpFoundation\Request'); $request->method('getClientIps') ->willReturn(array('127.0.0.1')); @@ -54,7 +55,7 @@ public function testListenerDoesNothingOnSubrequests() { $dispatcher = new EventDispatcher(); $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); - $listener = new ValidateRequestClientIpListener(); + $listener = new ValidateRequestListener(); $request = $this->getMock('Symfony\Component\HttpFoundation\Request'); $request->method('getClientIps') ->will($this->throwException(new ConflictingHeadersException()));