From 254b11062e7580f40c782bd3c9c74272fd01f7a5 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Tue, 27 Nov 2012 19:09:41 +0100 Subject: [PATCH 1/6] removed the non-standard Client-IP HTTP header --- .../Component/HttpFoundation/Request.php | 4 +--- .../Component/HttpFoundation/RequestTest.php | 21 +++++++------------ 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index 03c7e03a5f..99970efe27 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -450,9 +450,7 @@ class Request public function getClientIp($proxy = false) { if ($proxy) { - if ($this->server->has('HTTP_CLIENT_IP')) { - return $this->server->get('HTTP_CLIENT_IP'); - } elseif (self::$trustProxy && $this->server->has('HTTP_X_FORWARDED_FOR')) { + if (self::$trustProxy && $this->server->has('HTTP_X_FORWARDED_FOR')) { $clientIp = explode(',', $this->server->get('HTTP_X_FORWARDED_FOR'), 2); return isset($clientIp[0]) ? trim($clientIp[0]) : ''; diff --git a/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php b/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php index 43e083aadd..3042c5cfcd 100644 --- a/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php +++ b/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php @@ -496,16 +496,13 @@ class RequestTest extends \PHPUnit_Framework_TestCase /** * @dataProvider testGetClientIpProvider */ - public function testGetClientIp($expected, $proxy, $remoteAddr, $httpClientIp, $httpForwardedFor) + public function testGetClientIp($expected, $proxy, $remoteAddr, $httpForwardedFor) { $request = new Request; $this->assertEquals('', $request->getClientIp()); $this->assertEquals('', $request->getClientIp(true)); $server = array('REMOTE_ADDR' => $remoteAddr); - if (null !== $httpClientIp) { - $server['HTTP_CLIENT_IP'] = $httpClientIp; - } if (null !== $httpForwardedFor) { $server['HTTP_X_FORWARDED_FOR'] = $httpForwardedFor; } @@ -517,15 +514,13 @@ class RequestTest extends \PHPUnit_Framework_TestCase public function testGetClientIpProvider() { return array( - array('88.88.88.88', false, '88.88.88.88', null, null), - array('127.0.0.1', false, '127.0.0.1', '88.88.88.88', null), - array('88.88.88.88', true, '127.0.0.1', '88.88.88.88', null), - array('127.0.0.1', false, '127.0.0.1', null, '88.88.88.88'), - array('88.88.88.88', true, '127.0.0.1', null, '88.88.88.88'), - array('::1', false, '::1', null, null), - array('2620:0:1cfe:face:b00c::3', true, '::1', '2620:0:1cfe:face:b00c::3', null), - array('2620:0:1cfe:face:b00c::3', true, '::1', null, '2620:0:1cfe:face:b00c::3, ::1'), - array('88.88.88.88', true, '123.45.67.89', null, '88.88.88.88, 87.65.43.21, 127.0.0.1'), + array('88.88.88.88', false, '88.88.88.88', null), + array('127.0.0.1', false, '127.0.0.1', null), + array('127.0.0.1', false, '127.0.0.1', '88.88.88.88'), + array('88.88.88.88', true, '127.0.0.1', '88.88.88.88'), + array('::1', false, '::1', null), + array('2620:0:1cfe:face:b00c::3', true, '::1', '2620:0:1cfe:face:b00c::3, ::1'), + array('88.88.88.88', true, '123.45.67.89', '88.88.88.88, 87.65.43.21, 127.0.0.1'), ); } From b45873a3f6e958fa02e34a415f7077a3d31e58a6 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Tue, 27 Nov 2012 22:21:23 +0100 Subject: [PATCH 2/6] fixed algorithm used to determine the trusted client IP --- .../Component/HttpFoundation/Request.php | 66 ++++++++++++++----- .../Component/HttpFoundation/RequestTest.php | 41 ++++++------ 2 files changed, 71 insertions(+), 36 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index 99970efe27..34249c624a 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -20,7 +20,9 @@ namespace Symfony\Component\HttpFoundation; */ class Request { - protected static $trustProxy = false; + protected static $trustProxyData = false; + + protected static $trustedProxies = array(); /** * @var \Symfony\Component\HttpFoundation\ParameterBag @@ -357,14 +359,26 @@ class Request /** * Trusts $_SERVER entries coming from proxies. * - * You should only call this method if your application - * is hosted behind a reverse proxy that you manage. - * - * @api + * @deprecated Deprecated since version 2.0, to be removed in 2.3. Use setTrustedProxies instead. */ public static function trustProxyData() { - self::$trustProxy = true; + self::$trustProxyData = true; + } + + /** + * Sets a list of trusted proxies. + * + * You should only list the reverse proxies that you manage directly. + * + * @param array $proxies A list of trusted proxies + * + * @api + */ + public static function setTrustedProxies(array $proxies) + { + self::$trustedProxies = $proxies; + self::$trustProxyData = $proxies ? true : false; } /** @@ -441,23 +455,41 @@ class Request /** * Returns the client IP address. * - * @param Boolean $proxy Whether the current request has been made behind a proxy or not + * This method can read the client IP address from the "X-Forwarded-For" header + * when trusted proxies were set via "setTrustedProxies()". The "X-Forwarded-For" + * header value is a comma+space separated list of IP addresses, the left-most + * being the original client, and each successive proxy that passed the request + * adding the IP address where it received the request from. + * + * @param Boolean $proxy Whether the current request has been made behind a proxy or not (deprecated) * * @return string The client IP address * + * @see http://en.wikipedia.org/wiki/X-Forwarded-For + * + * @deprecated The proxy argument is deprecated since version 2.0 and will be removed in 2.3. Use setTrustedProxies instead. + * * @api */ public function getClientIp($proxy = false) { - if ($proxy) { - if (self::$trustProxy && $this->server->has('HTTP_X_FORWARDED_FOR')) { - $clientIp = explode(',', $this->server->get('HTTP_X_FORWARDED_FOR'), 2); + $ip = $this->server->get('REMOTE_ADDR'); - return isset($clientIp[0]) ? trim($clientIp[0]) : ''; - } + if (!$proxy && !self::$trustProxyData) { + return $ip; } - return $this->server->get('REMOTE_ADDR'); + if (!$this->server->has('HTTP_X_FORWARDED_FOR')) { + return $ip; + } + + $clientIps = array_map('trim', explode(',', $this->server->get('HTTP_X_FORWARDED_FOR'))); + $clientIps[] = $ip; + + $trustedProxies = ($proxy || self::$trustProxyData) && !self::$trustedProxies ? array($ip) : self::$trustedProxies; + $clientIps = array_diff($clientIps, $trustedProxies); + + return array_pop($clientIps); } /** @@ -560,7 +592,7 @@ class Request */ public function getPort() { - if (self::$trustProxy && $this->headers->has('X-Forwarded-Port')) { + if (self::$trustProxyData && $this->headers->has('X-Forwarded-Port')) { return $this->headers->get('X-Forwarded-Port'); } @@ -683,9 +715,9 @@ class Request return ( (strtolower($this->server->get('HTTPS')) == 'on' || $this->server->get('HTTPS') == 1) || - (self::$trustProxy && strtolower($this->headers->get('SSL_HTTPS')) == 'on' || $this->headers->get('SSL_HTTPS') == 1) + (self::$trustProxyData && strtolower($this->headers->get('SSL_HTTPS')) == 'on' || $this->headers->get('SSL_HTTPS') == 1) || - (self::$trustProxy && strtolower($this->headers->get('X_FORWARDED_PROTO')) == 'https') + (self::$trustProxyData && strtolower($this->headers->get('X_FORWARDED_PROTO')) == 'https') ); } @@ -698,7 +730,7 @@ class Request */ public function getHost() { - if (self::$trustProxy && $host = $this->headers->get('X_FORWARDED_HOST')) { + if (self::$trustProxyData && $host = $this->headers->get('X_FORWARDED_HOST')) { $elements = explode(',', $host); $host = trim($elements[count($elements) - 1]); diff --git a/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php b/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php index 3042c5cfcd..c9b164581e 100644 --- a/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php +++ b/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php @@ -19,11 +19,6 @@ use Symfony\Component\HttpFoundation\Request; class RequestTest extends \PHPUnit_Framework_TestCase { - public function setUp() - { - Request::trustProxyData(); - } - /** * @covers Symfony\Component\HttpFoundation\Request::__construct */ @@ -430,15 +425,17 @@ class RequestTest extends \PHPUnit_Framework_TestCase $request->initialize(array(), array(), array(), array(), array(), array('HTTP_HOST' => 'www.exemple.com')); $this->assertEquals('www.exemple.com', $request->getHost(), '->getHost() from Host Header'); - // Host header with port number. + // Host header with port number $request->initialize(array(), array(), array(), array(), array(), array('HTTP_HOST' => 'www.exemple.com:8080')); $this->assertEquals('www.exemple.com', $request->getHost(), '->getHost() from Host Header with port number'); - // Server values. + // Server values $request->initialize(array(), array(), array(), array(), array(), array('SERVER_NAME' => 'www.exemple.com')); $this->assertEquals('www.exemple.com', $request->getHost(), '->getHost() from server name'); - // X_FORWARDED_HOST. + Request::setTrustedProxies(array('1.1.1.1')); + + // X_FORWARDED_HOST $request->initialize(array(), array(), array(), array(), array(), array('HTTP_X_FORWARDED_HOST' => 'www.exemple.com')); $this->assertEquals('www.exemple.com', $request->getHost(), '->getHost() from X_FORWARDED_HOST'); @@ -458,6 +455,8 @@ class RequestTest extends \PHPUnit_Framework_TestCase $request->initialize(array(), array(), array(), array(), array(), array('SERVER_NAME' => 'www.exemple.com', 'HTTP_HOST' => 'www.host.com')); $this->assertEquals('www.host.com', $request->getHost(), '->getHost() value from Host header has priority over SERVER_NAME '); + + Request::setTrustedProxies(array()); } /** @@ -496,17 +495,19 @@ class RequestTest extends \PHPUnit_Framework_TestCase /** * @dataProvider testGetClientIpProvider */ - public function testGetClientIp($expected, $proxy, $remoteAddr, $httpForwardedFor) + public function testGetClientIp($expected, $proxy, $remoteAddr, $httpForwardedFor, $trustedProxies) { - $request = new Request; - $this->assertEquals('', $request->getClientIp()); - $this->assertEquals('', $request->getClientIp(true)); + $request = new Request(); $server = array('REMOTE_ADDR' => $remoteAddr); if (null !== $httpForwardedFor) { $server['HTTP_X_FORWARDED_FOR'] = $httpForwardedFor; } + if ($proxy || $trustedProxies) { + Request::setTrustedProxies(null === $trustedProxies ? array($remoteAddr) : $trustedProxies); + } + $request->initialize(array(), array(), array(), array(), array(), $server); $this->assertEquals($expected, $request->getClientIp($proxy)); } @@ -514,13 +515,15 @@ class RequestTest extends \PHPUnit_Framework_TestCase public function testGetClientIpProvider() { return array( - array('88.88.88.88', false, '88.88.88.88', null), - array('127.0.0.1', false, '127.0.0.1', null), - array('127.0.0.1', false, '127.0.0.1', '88.88.88.88'), - array('88.88.88.88', true, '127.0.0.1', '88.88.88.88'), - array('::1', false, '::1', null), - array('2620:0:1cfe:face:b00c::3', true, '::1', '2620:0:1cfe:face:b00c::3, ::1'), - array('88.88.88.88', true, '123.45.67.89', '88.88.88.88, 87.65.43.21, 127.0.0.1'), + array('88.88.88.88', false, '88.88.88.88', null, null), + array('127.0.0.1', false, '127.0.0.1', null, null), + array('::1', false, '::1', null, null), + array('127.0.0.1', false, '127.0.0.1', '88.88.88.88', null), + array('88.88.88.88', true, '127.0.0.1', '88.88.88.88', null), + array('2620:0:1cfe:face:b00c::3', true, '::1', '2620:0:1cfe:face:b00c::3', null), + array('88.88.88.88', true, '123.45.67.89', '127.0.0.1, 87.65.43.21, 88.88.88.88', null), + array('87.65.43.21', true, '123.45.67.89', '127.0.0.1, 87.65.43.21, 88.88.88.88', array('123.45.67.89', '88.88.88.88')), + array('87.65.43.21', false, '123.45.67.89', '127.0.0.1, 87.65.43.21, 88.88.88.88', array('123.45.67.89', '88.88.88.88')), ); } From 67e12f3ecbc6622b20dcc73014a597bacfd07e34 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 28 Nov 2012 08:25:47 +0100 Subject: [PATCH 3/6] added a way to configure the X-Forwarded-XXX header names and a way to disable trusting them --- .../Component/HttpFoundation/Request.php | 79 +++++++++++++++++-- .../Component/HttpFoundation/RequestTest.php | 41 +++++++++- 2 files changed, 111 insertions(+), 9 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index 34249c624a..adbff82f24 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -24,6 +24,20 @@ class Request protected static $trustedProxies = array(); + /** + * Names for headers that can be trusted when + * using trusted proxies. + * + * The default names are non-standard, but widely used + * by popular reverse proxies (like Apache mod_proxy or Amazon EC2). + */ + protected static $trustedHeaders = array( + 'client_ip' => 'X_FORWARDED_FOR', + 'client_host' => 'X_FORWARDED_HOST', + 'client_proto' => 'X_FORWARDED_PROTO', + 'client_port' => 'X_FORWARDED_PORT', + ); + /** * @var \Symfony\Component\HttpFoundation\ParameterBag * @@ -381,6 +395,30 @@ class Request self::$trustProxyData = $proxies ? true : false; } + /** + * Sets the name for trusted headers. + * + * The following header keys are supported: + * + * * client_ip: defaults to X-Forwarded-For (see getClientIp()) + * * client_host: defaults to X-Forwarded-Host (see getClientHost()) + * * client_port: defaults to X-Forwarded-Port (see getClientPort()) + * * client_proto: defaults to X-Forwarded-Proto (see getScheme() and isSecure()) + * + * Setting an empty value allows to disable the trusted header for the given key. + * + * @param string $key The header key + * @param string $value The header name + */ + public static function setTrustedHeaderName($key, $value) + { + if (!array_key_exists($key, self::$trustedHeaders)) { + throw new \InvalidArgumentException(sprintf('Unable to set the trusted header name for key "%s".', $key)); + } + + self::$trustedHeaders[$key] = $value; + } + /** * Gets a "parameter" value. * @@ -461,6 +499,10 @@ class Request * being the original client, and each successive proxy that passed the request * adding the IP address where it received the request from. * + * If your reverse proxy uses a different header name than "X-Forwarded-For", + * ("Client-Ip" for instance), configure it via "setTrustedHeaderName()" with + * the "client-ip" key. + * * @param Boolean $proxy Whether the current request has been made behind a proxy or not (deprecated) * * @return string The client IP address @@ -479,11 +521,11 @@ class Request return $ip; } - if (!$this->server->has('HTTP_X_FORWARDED_FOR')) { + if (!self::$trustedHeaders['client_ip'] || !$this->headers->has(self::$trustedHeaders['client_ip'])) { return $ip; } - $clientIps = array_map('trim', explode(',', $this->server->get('HTTP_X_FORWARDED_FOR'))); + $clientIps = array_map('trim', explode(',', $this->headers->get(self::$trustedHeaders['client_ip']))); $clientIps[] = $ip; $trustedProxies = ($proxy || self::$trustProxyData) && !self::$trustedProxies ? array($ip) : self::$trustedProxies; @@ -586,14 +628,22 @@ class Request /** * Returns the port on which the request is made. * + * This method can read the client port from the "X-Forwarded-Port" header + * when trusted proxies were set via "setTrustedProxies()". + * + * The "X-Forwarded-Port" header must contain the client port. + * + * If your reverse proxy uses a different header name than "X-Forwarded-Port", + * configure it via "setTrustedHeaderName()" with the "client-port" key. + * * @return string * * @api */ public function getPort() { - if (self::$trustProxyData && $this->headers->has('X-Forwarded-Port')) { - return $this->headers->get('X-Forwarded-Port'); + if (self::$trustProxyData && self::$trustedHeaders['client_port'] && $port = $this->headers->get(self::$trustedHeaders['client_port'])) { + return $port; } return $this->server->get('SERVER_PORT'); @@ -706,6 +756,15 @@ class Request /** * Checks whether the request is secure or not. * + * This method can read the client port from the "X-Forwarded-Proto" header + * when trusted proxies were set via "setTrustedProxies()". + * + * The "X-Forwarded-Proto" header must contain the protocol: "https" or "http". + * + * If your reverse proxy uses a different header name than "X-Forwarded-Proto" + * ("SSL_HTTPS" for instance), configure it via "setTrustedHeaderName()" with + * the "client-proto" key. + * * @return Boolean * * @api @@ -717,20 +776,28 @@ class Request || (self::$trustProxyData && strtolower($this->headers->get('SSL_HTTPS')) == 'on' || $this->headers->get('SSL_HTTPS') == 1) || - (self::$trustProxyData && strtolower($this->headers->get('X_FORWARDED_PROTO')) == 'https') + (self::$trustProxyData && self::$trustedHeaders['client_proto'] && strtolower($this->headers->get(self::$trustedHeaders['client_proto'])) == 'https') ); } /** * Returns the host name. * + * This method can read the client port from the "X-Forwarded-Host" header + * when trusted proxies were set via "setTrustedProxies()". + * + * The "X-Forwarded-Host" header must contain the client host name. + * + * If your reverse proxy uses a different header name than "X-Forwarded-Host", + * configure it via "setTrustedHeaderName()" with the "client-host" key. + * * @return string * * @api */ public function getHost() { - if (self::$trustProxyData && $host = $this->headers->get('X_FORWARDED_HOST')) { + if (self::$trustProxyData && self::$trustedHeaders['client_host'] && $host = $this->headers->get(self::$trustedHeaders['client_host'])) { $elements = explode(',', $host); $host = trim($elements[count($elements) - 1]); diff --git a/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php b/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php index c9b164581e..c6e11f05cd 100644 --- a/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php +++ b/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php @@ -433,6 +433,9 @@ class RequestTest extends \PHPUnit_Framework_TestCase $request->initialize(array(), array(), array(), array(), array(), array('SERVER_NAME' => 'www.exemple.com')); $this->assertEquals('www.exemple.com', $request->getHost(), '->getHost() from server name'); + $request->initialize(array(), array(), array(), array(), array(), array('SERVER_NAME' => 'www.exemple.com', 'HTTP_HOST' => 'www.host.com')); + $this->assertEquals('www.host.com', $request->getHost(), '->getHost() value from Host header has priority over SERVER_NAME '); + Request::setTrustedProxies(array('1.1.1.1')); // X_FORWARDED_HOST @@ -453,10 +456,22 @@ class RequestTest extends \PHPUnit_Framework_TestCase $request->initialize(array(), array(), array(), array(), array(), array('SERVER_NAME' => 'www.exemple.com', 'HTTP_X_FORWARDED_HOST' => 'www.forward.com')); $this->assertEquals('www.forward.com', $request->getHost(), '->getHost() value from X_FORWARDED_HOST has priority over SERVER_NAME '); - $request->initialize(array(), array(), array(), array(), array(), array('SERVER_NAME' => 'www.exemple.com', 'HTTP_HOST' => 'www.host.com')); - $this->assertEquals('www.host.com', $request->getHost(), '->getHost() value from Host header has priority over SERVER_NAME '); + // custom X_FORWARDED_HOST header name + Request::setTrustedHeaderName('client_host', 'X_MY_HOST'); + $request->initialize(array(), array(), array(), array(), array(), array('SERVER_NAME' => 'www.exemple.com', 'HTTP_X_MY_HOST' => 'www.forward.com')); + $this->assertEquals('www.forward.com', $request->getHost(), '->getHost() value from custom header name has priority over SERVER_NAME '); + // X_FORWARDED_HOST ignored when custom header name is empty + Request::setTrustedHeaderName('client_host', null); + $request->initialize(array(), array(), array(), array(), array(), array('SERVER_NAME' => 'www.exemple.com', 'HTTP_X_FORWARDED_HOST' => 'www.forward.com')); + $this->assertEquals('www.exemple.com', $request->getHost(), '->getHost() value from X_FORWARDED_HOST has priority over SERVER_NAME '); + + Request::setTrustedHeaderName('client_host', 'X_FORWARDED_HOST'); Request::setTrustedProxies(array()); + + // X_FORWARDED_HOST ignored when no trusted proxies + $request->initialize(array(), array(), array(), array(), array(), array('SERVER_NAME' => 'www.exemple.com', 'HTTP_X_FORWARDED_HOST' => 'www.forward.com')); + $this->assertEquals('www.exemple.com', $request->getHost(), '->getHost() value from X_FORWARDED_HOST has priority over SERVER_NAME '); } /** @@ -510,6 +525,8 @@ class RequestTest extends \PHPUnit_Framework_TestCase $request->initialize(array(), array(), array(), array(), array(), $server); $this->assertEquals($expected, $request->getClientIp($proxy)); + + Request::setTrustedProxies(array()); } public function testGetClientIpProvider() @@ -619,7 +636,9 @@ class RequestTest extends \PHPUnit_Framework_TestCase $request->headers->set('X_FORWARDED_PROTO', 'https'); + Request::setTrustedProxies(array('1.1.1.1')); $this->assertTrue($request->isSecure()); + Request::setTrustedProxies(array()); $request->overrideGlobals(); @@ -813,12 +832,28 @@ class RequestTest extends \PHPUnit_Framework_TestCase public function testForwardedSecure() { - $request = new Request(); + $request = Request::create('http://test.com/'); $request->headers->set('X-Forwarded-Proto', 'https'); $request->headers->set('X-Forwarded-Port', 443); + $this->assertFalse($request->isSecure()); + $this->assertEquals(80, $request->getPort()); + + Request::setTrustedProxies(array('1.1.1.1')); + $this->assertTrue($request->isSecure()); $this->assertEquals(443, $request->getPort()); + + // custom header names + Request::setTrustedHeaderName('client_proto', 'X-My-Proto'); + Request::setTrustedHeaderName('client_port', 'X-My-Port'); + $request->headers->set('X-My-Proto', 'http'); + $request->headers->set('X-My-Port', 81); + + $this->assertFalse($request->isSecure()); + $this->assertEquals(81, $request->getPort()); + + Request::setTrustedProxies(array()); } public function testHasSession() From 6a3ba52858f4979874e48f0ef15817344f0c5edb Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 28 Nov 2012 08:37:24 +0100 Subject: [PATCH 4/6] fixed the logic in Request::isSecure() (if the information comes from a source that we trust, don't check other ones) --- src/Symfony/Component/HttpFoundation/Request.php | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index adbff82f24..0f6e467823 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -771,13 +771,11 @@ class Request */ public function isSecure() { - return ( - (strtolower($this->server->get('HTTPS')) == 'on' || $this->server->get('HTTPS') == 1) - || - (self::$trustProxyData && strtolower($this->headers->get('SSL_HTTPS')) == 'on' || $this->headers->get('SSL_HTTPS') == 1) - || - (self::$trustProxyData && self::$trustedHeaders['client_proto'] && strtolower($this->headers->get(self::$trustedHeaders['client_proto'])) == 'https') - ); + if (self::$trustProxyData && self::$trustedHeaders['client_proto'] && $proto = $this->headers->get(self::$trustedHeaders['client_proto'])) { + return in_array(strtolower($proto), array('https', 'on', '1')); + } + + return 'on' == strtolower($this->server->get('HTTPS')) || 1 == $this->server->get('HTTPS'); } /** From f5d8cca25ddac0bc522726e6a38d8f7796029e37 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 28 Nov 2012 11:49:12 +0100 Subject: [PATCH 5/6] refactored tests for Request --- .../Component/HttpFoundation/RequestTest.php | 127 +++++++++--------- 1 file changed, 64 insertions(+), 63 deletions(-) diff --git a/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php b/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php index c6e11f05cd..2eb4ba49a2 100644 --- a/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php +++ b/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php @@ -435,43 +435,6 @@ class RequestTest extends \PHPUnit_Framework_TestCase $request->initialize(array(), array(), array(), array(), array(), array('SERVER_NAME' => 'www.exemple.com', 'HTTP_HOST' => 'www.host.com')); $this->assertEquals('www.host.com', $request->getHost(), '->getHost() value from Host header has priority over SERVER_NAME '); - - Request::setTrustedProxies(array('1.1.1.1')); - - // X_FORWARDED_HOST - $request->initialize(array(), array(), array(), array(), array(), array('HTTP_X_FORWARDED_HOST' => 'www.exemple.com')); - $this->assertEquals('www.exemple.com', $request->getHost(), '->getHost() from X_FORWARDED_HOST'); - - // X_FORWARDED_HOST - $request->initialize(array(), array(), array(), array(), array(), array('HTTP_X_FORWARDED_HOST' => 'www.exemple.com, www.second.com')); - $this->assertEquals('www.second.com', $request->getHost(), '->getHost() value from X_FORWARDED_HOST use last value'); - - // X_FORWARDED_HOST with port number - $request->initialize(array(), array(), array(), array(), array(), array('HTTP_X_FORWARDED_HOST' => 'www.exemple.com, www.second.com:8080')); - $this->assertEquals('www.second.com', $request->getHost(), '->getHost() value from X_FORWARDED_HOST with port number'); - - $request->initialize(array(), array(), array(), array(), array(), array('HTTP_HOST' => 'www.exemple.com', 'HTTP_X_FORWARDED_HOST' => 'www.forward.com')); - $this->assertEquals('www.forward.com', $request->getHost(), '->getHost() value from X_FORWARDED_HOST has priority over Host'); - - $request->initialize(array(), array(), array(), array(), array(), array('SERVER_NAME' => 'www.exemple.com', 'HTTP_X_FORWARDED_HOST' => 'www.forward.com')); - $this->assertEquals('www.forward.com', $request->getHost(), '->getHost() value from X_FORWARDED_HOST has priority over SERVER_NAME '); - - // custom X_FORWARDED_HOST header name - Request::setTrustedHeaderName('client_host', 'X_MY_HOST'); - $request->initialize(array(), array(), array(), array(), array(), array('SERVER_NAME' => 'www.exemple.com', 'HTTP_X_MY_HOST' => 'www.forward.com')); - $this->assertEquals('www.forward.com', $request->getHost(), '->getHost() value from custom header name has priority over SERVER_NAME '); - - // X_FORWARDED_HOST ignored when custom header name is empty - Request::setTrustedHeaderName('client_host', null); - $request->initialize(array(), array(), array(), array(), array(), array('SERVER_NAME' => 'www.exemple.com', 'HTTP_X_FORWARDED_HOST' => 'www.forward.com')); - $this->assertEquals('www.exemple.com', $request->getHost(), '->getHost() value from X_FORWARDED_HOST has priority over SERVER_NAME '); - - Request::setTrustedHeaderName('client_host', 'X_FORWARDED_HOST'); - Request::setTrustedProxies(array()); - - // X_FORWARDED_HOST ignored when no trusted proxies - $request->initialize(array(), array(), array(), array(), array(), array('SERVER_NAME' => 'www.exemple.com', 'HTTP_X_FORWARDED_HOST' => 'www.forward.com')); - $this->assertEquals('www.exemple.com', $request->getHost(), '->getHost() value from X_FORWARDED_HOST has priority over SERVER_NAME '); } /** @@ -830,32 +793,6 @@ class RequestTest extends \PHPUnit_Framework_TestCase $this->assertEquals('foo', $request->getRequestFormat(null)); } - public function testForwardedSecure() - { - $request = Request::create('http://test.com/'); - $request->headers->set('X-Forwarded-Proto', 'https'); - $request->headers->set('X-Forwarded-Port', 443); - - $this->assertFalse($request->isSecure()); - $this->assertEquals(80, $request->getPort()); - - Request::setTrustedProxies(array('1.1.1.1')); - - $this->assertTrue($request->isSecure()); - $this->assertEquals(443, $request->getPort()); - - // custom header names - Request::setTrustedHeaderName('client_proto', 'X-My-Proto'); - Request::setTrustedHeaderName('client_port', 'X-My-Port'); - $request->headers->set('X-My-Proto', 'http'); - $request->headers->set('X-My-Port', 81); - - $this->assertFalse($request->isSecure()); - $this->assertEquals(81, $request->getPort()); - - Request::setTrustedProxies(array()); - } - public function testHasSession() { $request = new Request; @@ -908,6 +845,70 @@ class RequestTest extends \PHPUnit_Framework_TestCase array('text/html,application/xhtml+xml', array('application/xhtml+xml' => 1, 'text/html' => 1)), ); } + + public function testTrustedProxies() + { + $request = Request::create('http://example.com/'); + $request->server->set('REMOTE_ADDR', '3.3.3.3'); + $request->headers->set('X_FORWARDED_FOR', '1.1.1.1, 2.2.2.2'); + $request->headers->set('X_FORWARDED_HOST', 'foo.example.com, real.example.com:8080'); + $request->headers->set('X_FORWARDED_PROTO', 'https'); + $request->headers->set('X_FORWARDED_PORT', 443); + $request->headers->set('X_MY_FOR', '3.3.3.3, 4.4.4.4'); + $request->headers->set('X_MY_HOST', 'my.example.com'); + $request->headers->set('X_MY_PROTO', 'http'); + $request->headers->set('X_MY_PORT', 81); + + // no trusted proxies + $this->assertEquals('3.3.3.3', $request->getClientIp()); + $this->assertEquals('example.com', $request->getHost()); + $this->assertEquals(80, $request->getPort()); + $this->assertFalse($request->isSecure()); + + // trusted proxy via deprecated trustProxyData() + Request::trustProxyData(); + $this->assertEquals('2.2.2.2', $request->getClientIp()); + $this->assertEquals('real.example.com', $request->getHost()); + $this->assertEquals(443, $request->getPort()); + $this->assertTrue($request->isSecure()); + + // disabling proxy trusting + Request::setTrustedProxies(array()); + $this->assertEquals('3.3.3.3', $request->getClientIp()); + $this->assertEquals('example.com', $request->getHost()); + $this->assertEquals(80, $request->getPort()); + $this->assertFalse($request->isSecure()); + + // trusted proxy via setTrustedProxies() + Request::setTrustedProxies(array('3.3.3.3', '2.2.2.2')); + $this->assertEquals('1.1.1.1', $request->getClientIp()); + $this->assertEquals('real.example.com', $request->getHost()); + $this->assertEquals(443, $request->getPort()); + $this->assertTrue($request->isSecure()); + + // custom header names + Request::setTrustedHeaderName('client_ip', 'X_MY_FOR'); + Request::setTrustedHeaderName('client_host', 'X_MY_HOST'); + Request::setTrustedHeaderName('client_port', 'X_MY_PORT'); + Request::setTrustedHeaderName('client_proto', 'X_MY_PROTO'); + $this->assertEquals('4.4.4.4', $request->getClientIp()); + $this->assertEquals('my.example.com', $request->getHost()); + $this->assertEquals(81, $request->getPort()); + $this->assertFalse($request->isSecure()); + + // disabling via empty header names + Request::setTrustedHeaderName('client_ip', null); + Request::setTrustedHeaderName('client_host', null); + Request::setTrustedHeaderName('client_port', null); + Request::setTrustedHeaderName('client_proto', null); + $this->assertEquals('3.3.3.3', $request->getClientIp()); + $this->assertEquals('example.com', $request->getHost()); + $this->assertEquals(80, $request->getPort()); + $this->assertFalse($request->isSecure()); + + // reset + Request::setTrustedProxies(array()); + } } class RequestContentProxy extends Request From e5536f0fe10421da7ebbe0071343e94d039dfb97 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Wed, 28 Nov 2012 11:53:55 +0100 Subject: [PATCH 6/6] replaced magic strings by proper constants --- .../Component/HttpFoundation/Request.php | 31 +++++++++++-------- .../Component/HttpFoundation/RequestTest.php | 16 +++++----- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Request.php b/src/Symfony/Component/HttpFoundation/Request.php index 0f6e467823..f655c0e129 100644 --- a/src/Symfony/Component/HttpFoundation/Request.php +++ b/src/Symfony/Component/HttpFoundation/Request.php @@ -20,6 +20,11 @@ namespace Symfony\Component\HttpFoundation; */ class Request { + const HEADER_CLIENT_IP = 'client_ip'; + const HEADER_CLIENT_HOST = 'client_host'; + const HEADER_CLIENT_PROTO = 'client_proto'; + const HEADER_CLIENT_PORT = 'client_port'; + protected static $trustProxyData = false; protected static $trustedProxies = array(); @@ -32,10 +37,10 @@ class Request * by popular reverse proxies (like Apache mod_proxy or Amazon EC2). */ protected static $trustedHeaders = array( - 'client_ip' => 'X_FORWARDED_FOR', - 'client_host' => 'X_FORWARDED_HOST', - 'client_proto' => 'X_FORWARDED_PROTO', - 'client_port' => 'X_FORWARDED_PORT', + self::HEADER_CLIENT_IP => 'X_FORWARDED_FOR', + self::HEADER_CLIENT_HOST => 'X_FORWARDED_HOST', + self::HEADER_CLIENT_PROTO => 'X_FORWARDED_PROTO', + self::HEADER_CLIENT_PORT => 'X_FORWARDED_PORT', ); /** @@ -400,10 +405,10 @@ class Request * * The following header keys are supported: * - * * client_ip: defaults to X-Forwarded-For (see getClientIp()) - * * client_host: defaults to X-Forwarded-Host (see getClientHost()) - * * client_port: defaults to X-Forwarded-Port (see getClientPort()) - * * client_proto: defaults to X-Forwarded-Proto (see getScheme() and isSecure()) + * * Request::HEADER_CLIENT_IP: defaults to X-Forwarded-For (see getClientIp()) + * * Request::HEADER_CLIENT_HOST: defaults to X-Forwarded-Host (see getClientHost()) + * * Request::HEADER_CLIENT_PORT: defaults to X-Forwarded-Port (see getClientPort()) + * * Request::HEADER_CLIENT_PROTO: defaults to X-Forwarded-Proto (see getScheme() and isSecure()) * * Setting an empty value allows to disable the trusted header for the given key. * @@ -521,11 +526,11 @@ class Request return $ip; } - if (!self::$trustedHeaders['client_ip'] || !$this->headers->has(self::$trustedHeaders['client_ip'])) { + if (!self::$trustedHeaders[self::HEADER_CLIENT_IP] || !$this->headers->has(self::$trustedHeaders[self::HEADER_CLIENT_IP])) { return $ip; } - $clientIps = array_map('trim', explode(',', $this->headers->get(self::$trustedHeaders['client_ip']))); + $clientIps = array_map('trim', explode(',', $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_IP]))); $clientIps[] = $ip; $trustedProxies = ($proxy || self::$trustProxyData) && !self::$trustedProxies ? array($ip) : self::$trustedProxies; @@ -642,7 +647,7 @@ class Request */ public function getPort() { - if (self::$trustProxyData && self::$trustedHeaders['client_port'] && $port = $this->headers->get(self::$trustedHeaders['client_port'])) { + if (self::$trustProxyData && self::$trustedHeaders[self::HEADER_CLIENT_PORT] && $port = $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_PORT])) { return $port; } @@ -771,7 +776,7 @@ class Request */ public function isSecure() { - if (self::$trustProxyData && self::$trustedHeaders['client_proto'] && $proto = $this->headers->get(self::$trustedHeaders['client_proto'])) { + if (self::$trustProxyData && self::$trustedHeaders[self::HEADER_CLIENT_PROTO] && $proto = $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_PROTO])) { return in_array(strtolower($proto), array('https', 'on', '1')); } @@ -795,7 +800,7 @@ class Request */ public function getHost() { - if (self::$trustProxyData && self::$trustedHeaders['client_host'] && $host = $this->headers->get(self::$trustedHeaders['client_host'])) { + if (self::$trustProxyData && self::$trustedHeaders[self::HEADER_CLIENT_HOST] && $host = $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_HOST])) { $elements = explode(',', $host); $host = trim($elements[count($elements) - 1]); diff --git a/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php b/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php index 2eb4ba49a2..9956b87e8d 100644 --- a/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php +++ b/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php @@ -887,20 +887,20 @@ class RequestTest extends \PHPUnit_Framework_TestCase $this->assertTrue($request->isSecure()); // custom header names - Request::setTrustedHeaderName('client_ip', 'X_MY_FOR'); - Request::setTrustedHeaderName('client_host', 'X_MY_HOST'); - Request::setTrustedHeaderName('client_port', 'X_MY_PORT'); - Request::setTrustedHeaderName('client_proto', 'X_MY_PROTO'); + Request::setTrustedHeaderName(Request::HEADER_CLIENT_IP, 'X_MY_FOR'); + Request::setTrustedHeaderName(Request::HEADER_CLIENT_HOST, 'X_MY_HOST'); + Request::setTrustedHeaderName(Request::HEADER_CLIENT_PORT, 'X_MY_PORT'); + Request::setTrustedHeaderName(Request::HEADER_CLIENT_PROTO, 'X_MY_PROTO'); $this->assertEquals('4.4.4.4', $request->getClientIp()); $this->assertEquals('my.example.com', $request->getHost()); $this->assertEquals(81, $request->getPort()); $this->assertFalse($request->isSecure()); // disabling via empty header names - Request::setTrustedHeaderName('client_ip', null); - Request::setTrustedHeaderName('client_host', null); - Request::setTrustedHeaderName('client_port', null); - Request::setTrustedHeaderName('client_proto', null); + Request::setTrustedHeaderName(Request::HEADER_CLIENT_IP, null); + Request::setTrustedHeaderName(Request::HEADER_CLIENT_HOST, null); + Request::setTrustedHeaderName(Request::HEADER_CLIENT_PORT, null); + Request::setTrustedHeaderName(Request::HEADER_CLIENT_PROTO, null); $this->assertEquals('3.3.3.3', $request->getClientIp()); $this->assertEquals('example.com', $request->getHost()); $this->assertEquals(80, $request->getPort());