From 51f59d6f62bb4264e982eb7e8919e5013887c60b Mon Sep 17 00:00:00 2001 From: Roland Franssen Date: Fri, 24 Jun 2016 15:39:41 +0000 Subject: [PATCH 1/2] [Console] Decouple SymfonyStyle from TableCell --- .../Component/Console/Style/SymfonyStyle.php | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/Symfony/Component/Console/Style/SymfonyStyle.php b/src/Symfony/Component/Console/Style/SymfonyStyle.php index 03550836cf..e9932d1f87 100644 --- a/src/Symfony/Component/Console/Style/SymfonyStyle.php +++ b/src/Symfony/Component/Console/Style/SymfonyStyle.php @@ -17,7 +17,6 @@ use Symfony\Component\Console\Helper\Helper; use Symfony\Component\Console\Helper\ProgressBar; use Symfony\Component\Console\Helper\SymfonyQuestionHelper; use Symfony\Component\Console\Helper\Table; -use Symfony\Component\Console\Helper\TableCell; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\BufferedOutput; use Symfony\Component\Console\Output\OutputInterface; @@ -218,21 +217,13 @@ class SymfonyStyle extends OutputStyle */ public function table(array $headers, array $rows) { - array_walk_recursive($headers, function (&$value) { - if ($value instanceof TableCell) { - $value = new TableCell(sprintf('%s', $value), array( - 'colspan' => $value->getColspan(), - 'rowspan' => $value->getRowspan(), - )); - } else { - $value = sprintf('%s', $value); - } - }); + $style = clone Table::getStyleDefinition('symfony-style-guide'); + $style->setCellHeaderFormat('%s'); $table = new Table($this); $table->setHeaders($headers); $table->setRows($rows); - $table->setStyle('symfony-style-guide'); + $table->setStyle($style); $table->render(); $this->newLine(); From ee8842fedbc1dcab12fc1557cb6c3fbf9e7211dd Mon Sep 17 00:00:00 2001 From: Magnus Nordlander Date: Mon, 2 May 2016 14:34:55 +0200 Subject: [PATCH 2/2] [HttpFoundation] Warning when request has both Forwarded and X-Forwarded-For --- .../FrameworkBundle/Resources/config/web.xml | 4 + .../Exception/ConflictingHeadersException.php | 23 ++++++ .../Component/HttpFoundation/Request.php | 81 ++++++++++++------- .../HttpFoundation/Tests/RequestTest.php | 68 ++++++++++++++++ .../EventListener/ValidateRequestListener.php | 56 +++++++++++++ .../HttpKernel/Profiler/Profiler.php | 7 +- .../ValidateRequestListenerTest.php | 67 +++++++++++++++ 7 files changed, 277 insertions(+), 29 deletions(-) create mode 100644 src/Symfony/Component/HttpFoundation/Exception/ConflictingHeadersException.php create mode 100644 src/Symfony/Component/HttpKernel/EventListener/ValidateRequestListener.php create mode 100644 src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestListenerTest.php diff --git a/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml b/src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml index 9b2f3cb3a4..c1f73e5610 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/Exception/ConflictingHeadersException.php b/src/Symfony/Component/HttpFoundation/Exception/ConflictingHeadersException.php new file mode 100644 index 0000000000..fa5f1c7873 --- /dev/null +++ b/src/Symfony/Component/HttpFoundation/Exception/ConflictingHeadersException.php @@ -0,0 +1,23 @@ + + * + * 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 da6244ddb9..8b23b92aa9 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; /** @@ -805,41 +806,34 @@ class Request 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) { $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])) { - $clientIps = array_map('trim', explode(',', $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_IP]))); + $forwardedClientIps = $matches[3]; + + $forwardedClientIps = $this->normalizeAndFilterClientIps($forwardedClientIps, $ip); + $clientIps = $forwardedClientIps; } - $clientIps[] = $ip; // Complete the IP chain with the IP the request actually came from - $firstTrustedIp = null; + if ($hasTrustedClientIpHeader) { + $xForwardedForClientIps = array_map('trim', explode(',', $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_IP]))); - 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; - } - } + $xForwardedForClientIps = $this->normalizeAndFilterClientIps($xForwardedForClientIps, $ip); + $clientIps = $xForwardedForClientIps; } - // Now the IP chain contains only untrusted proxies and the client IP - return $clientIps ? array_reverse($clientIps) : array($firstTrustedIp); + 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.'); + } + + if (!$hasTrustedForwardedHeader && !$hasTrustedClientIpHeader) { + return $this->normalizeAndFilterClientIps(array(), $ip); + } + + return $clientIps; } /** @@ -1930,4 +1924,35 @@ class Request { return self::$trustedProxies && IpUtils::checkIp($this->server->get('REMOTE_ADDR'), self::$trustedProxies); } + + private function normalizeAndFilterClientIps(array $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 89c4eb2d42..1edc48c1b6 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php @@ -923,6 +923,74 @@ class RequestTest extends \PHPUnit_Framework_TestCase ); } + /** + * @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(); diff --git a/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestListener.php b/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestListener.php new file mode 100644 index 0000000000..6316b77ffe --- /dev/null +++ b/src/Symfony/Component/HttpKernel/EventListener/ValidateRequestListener.php @@ -0,0 +1,56 @@ + + * + * 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\BadRequestHttpException; +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 ValidateRequestListener implements EventSubscriberInterface +{ + /** + * Performs the validation. + * + * @param GetResponseEvent $event + */ + public function onKernelRequest(GetResponseEvent $event) + { + if ($event->isMasterRequest()) { + try { + // This will throw an exception if the headers are inconsistent. + $event->getRequest()->getClientIps(); + } catch (ConflictingHeadersException $e) { + throw new BadRequestHttpException('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/Profiler/Profiler.php b/src/Symfony/Component/HttpKernel/Profiler/Profiler.php index 864f624729..35d3a8f1b4 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 @@ class Profiler $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()); diff --git a/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestListenerTest.php b/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestListenerTest.php new file mode 100644 index 0000000000..0f6db8d880 --- /dev/null +++ b/src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestListenerTest.php @@ -0,0 +1,67 @@ + + * + * 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\ValidateRequestListener; +use Symfony\Component\HttpKernel\Event\GetResponseEvent; +use Symfony\Component\HttpKernel\HttpKernelInterface; +use Symfony\Component\HttpKernel\KernelEvents; + +class ValidateRequestListenerTest extends \PHPUnit_Framework_TestCase +{ + public function testListenerThrowsWhenMasterRequestHasInconsistentClientIps() + { + $dispatcher = new EventDispatcher(); + $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); + $listener = new ValidateRequestListener(); + $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::MASTER_REQUEST); + + $this->setExpectedException('Symfony\Component\HttpKernel\Exception\BadRequestHttpException'); + $dispatcher->dispatch(KernelEvents::REQUEST, $event); + } + + public function testListenerDoesNothingOnValidRequests() + { + $dispatcher = new EventDispatcher(); + $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); + $listener = new ValidateRequestListener(); + $request = $this->getMock('Symfony\Component\HttpFoundation\Request'); + $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 ValidateRequestListener(); + $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); + } +}