Skip to content

Commit 04caacb

Browse files
nicolas-grekasnicolas-grekas
committed
[HttpFoundation] Fix missing handling of for/host/proto info from "Forwarded" header
1 parent e1ab801 commit 04caacb

File tree

4 files changed

+120
-58
lines changed

4 files changed

+120
-58
lines changed

‎src/Symfony/Component/HttpFoundation/Request.php

+68-55
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,15 @@ class Request
206206

207207
protected static $requestFactory;
208208

209+
private $isForwardedValid = true;
210+
211+
private static $forwardedParams = array(
212+
self::HEADER_CLIENT_IP => 'for',
213+
self::HEADER_CLIENT_HOST => 'host',
214+
self::HEADER_CLIENT_PROTO => 'proto',
215+
self::HEADER_CLIENT_PORT => 'host',
216+
);
217+
209218
/**
210219
* Constructor.
211220
*
@@ -806,41 +815,13 @@ public function setSession(SessionInterface $session)
806815
*/
807816
public function getClientIps()
808817
{
809-
$clientIps = array();
810818
$ip = $this->server->get('REMOTE_ADDR');
811819

812820
if (!$this->isFromTrustedProxy()) {
813821
return array($ip);
814822
}
815823

816-
$hasTrustedForwardedHeader = self::$trustedHeaders[self::HEADER_FORWARDED] && $this->headers->has(self::$trustedHeaders[self::HEADER_FORWARDED]);
817-
$hasTrustedClientIpHeader = self::$trustedHeaders[self::HEADER_CLIENT_IP] && $this->headers->has(self::$trustedHeaders[self::HEADER_CLIENT_IP]);
818-
819-
if ($hasTrustedForwardedHeader) {
820-
$forwardedHeader = $this->headers->get(self::$trustedHeaders[self::HEADER_FORWARDED]);
821-
preg_match_all('{(for)=("?\[?)([a-z0-9\.:_\-/]*)}', $forwardedHeader, $matches);
822-
$forwardedClientIps = $matches[3];
823-
824-
$forwardedClientIps = $this->normalizeAndFilterClientIps($forwardedClientIps, $ip);
825-
$clientIps = $forwardedClientIps;
826-
}
827-
828-
if ($hasTrustedClientIpHeader) {
829-
$xForwardedForClientIps = array_map('trim', explode(',', $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_IP])));
830-
831-
$xForwardedForClientIps = $this->normalizeAndFilterClientIps($xForwardedForClientIps, $ip);
832-
$clientIps = $xForwardedForClientIps;
833-
}
834-
835-
if ($hasTrustedForwardedHeader && $hasTrustedClientIpHeader && $forwardedClientIps !== $xForwardedForClientIps) {
836-
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.');
837-
}
838-
839-
if (!$hasTrustedForwardedHeader && !$hasTrustedClientIpHeader) {
840-
return $this->normalizeAndFilterClientIps(array(), $ip);
841-
}
842-
843-
return $clientIps;
824+
return $this->getTrustedValues(self::HEADER_CLIENT_IP, $ip) ?: array($ip);
844825
}
845826

846827
/**
@@ -966,31 +947,25 @@ public function getScheme()
966947
*/
967948
public function getPort()
968949
{
969-
if ($this->isFromTrustedProxy()) {
970-
if (self::$trustedHeaders[self::HEADER_CLIENT_PORT] && $port = $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_PORT])) {
971-
return (int) $port;
972-
}
973-
974-
if (self::$trustedHeaders[self::HEADER_CLIENT_PROTO] && 'https' === $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_PROTO], 'http')) {
975-
return 443;
976-
}
950+
if ($this->isFromTrustedProxy() && $host = $this->getTrustedValues(self::HEADER_CLIENT_PORT)) {
951+
$host = $host[0];
952+
} elseif ($this->isFromTrustedProxy() && $host = $this->getTrustedValues(self::HEADER_CLIENT_HOST)) {
953+
$host = $host[0];
954+
} elseif (!$host = $this->headers->get('HOST')) {
955+
return $this->server->get('SERVER_PORT');
977956
}
978957

979-
if ($host = $this->headers->get('HOST')) {
980-
if ($host[0] === '[') {
981-
$pos = strpos($host, ':', strrpos($host, ']'));
982-
} else {
983-
$pos = strrpos($host, ':');
984-
}
985-
986-
if (false !== $pos) {
987-
return (int) substr($host, $pos + 1);
988-
}
958+
if ($host[0] === '[') {
959+
$pos = strpos($host, ':', strrpos($host, ']'));
960+
} else {
961+
$pos = strrpos($host, ':');
962+
}
989963

990-
return 'https' === $this->getScheme() ? 443 : 80;
964+
if (false !== $pos) {
965+
return (int) substr($host, $pos + 1);
991966
}
992967

993-
return $this->server->get('SERVER_PORT');
968+
return 'https' === $this->getScheme() ? 443 : 80;
994969
}
995970

996971
/**
@@ -1190,8 +1165,8 @@ public function getQueryString()
11901165
*/
11911166
public function isSecure()
11921167
{
1193-
if ($this->isFromTrustedProxy() && self::$trustedHeaders[self::HEADER_CLIENT_PROTO] && $proto = $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_PROTO])) {
1194-
return in_array(strtolower(current(explode(',', $proto))), array('https', 'on', 'ssl', '1'));
1168+
if ($this->isFromTrustedProxy() && $proto = $this->getTrustedValues(self::HEADER_CLIENT_PROTO)) {
1169+
return in_array(strtolower($proto[0]), array('https', 'on', 'ssl', '1'), true);
11951170
}
11961171

11971172
$https = $this->server->get('HTTPS');
@@ -1216,10 +1191,8 @@ public function isSecure()
12161191
*/
12171192
public function getHost()
12181193
{
1219-
if ($this->isFromTrustedProxy() && self::$trustedHeaders[self::HEADER_CLIENT_HOST] && $host = $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_HOST])) {
1220-
$elements = explode(',', $host, 2);
1221-
1222-
$host = $elements[0];
1194+
if ($this->isFromTrustedProxy() && $host = $this->getTrustedValues(self::HEADER_CLIENT_HOST)) {
1195+
$host = $host[0];
12231196
} elseif (!$host = $this->headers->get('HOST')) {
12241197
if (!$host = $this->server->get('SERVER_NAME')) {
12251198
$host = $this->server->get('SERVER_ADDR', '');
@@ -1948,8 +1921,48 @@ private function isFromTrustedProxy()
19481921
return self::$trustedProxies && IpUtils::checkIp($this->server->get('REMOTE_ADDR'), self::$trustedProxies);
19491922
}
19501923

1924+
private function getTrustedValues($type, $ip = null)
1925+
{
1926+
$clientValues = array();
1927+
$forwardedValues = array();
1928+
1929+
if (self::$trustedHeaders[$type] && $this->headers->has(self::$trustedHeaders[$type])) {
1930+
foreach (explode(',', $this->headers->get(self::$trustedHeaders[$type])) as $v) {
1931+
$clientValues[] = (self::HEADER_CLIENT_PORT === $type ? '0.0.0.0:' : '').trim($v);
1932+
}
1933+
}
1934+
1935+
if (self::$trustedHeaders[self::HEADER_FORWARDED] && $this->headers->has(self::$trustedHeaders[self::HEADER_FORWARDED])) {
1936+
$forwardedValues = $this->headers->get(self::$trustedHeaders[self::HEADER_FORWARDED]);
1937+
$forwardedValues = preg_match_all(sprintf('{(?:%s)=(?:"?\[?)([a-zA-Z0-9\.:_\-/]*+)}', self::$forwardedParams[$type]), $forwardedValues, $matches) ? $matches[1] : array();
1938+
}
1939+
1940+
if (null !== $ip) {
1941+
$clientValues = $this->normalizeAndFilterClientIps($clientValues, $ip);
1942+
$forwardedValues = $this->normalizeAndFilterClientIps($forwardedValues, $ip);
1943+
}
1944+
1945+
if ($forwardedValues === $clientValues || !$clientValues) {
1946+
return $forwardedValues;
1947+
}
1948+
1949+
if (!$forwardedValues) {
1950+
return $clientValues;
1951+
}
1952+
1953+
if (!$this->isForwardedValid) {
1954+
return null !== $ip ? array('0.0.0.0', $ip) : array();
1955+
}
1956+
$this->isForwardedValid = false;
1957+
1958+
throw new ConflictingHeadersException(sprintf('The request has both a trusted "%s" header and a trusted "%s" header, conflicting with each other. You should either configure your proxy to remove one of them, or configure your project to distrust the offending one.', self::$trustedHeaders[self::HEADER_FORWARDED], self::$trustedHeaders[$type]));
1959+
}
1960+
19511961
private function normalizeAndFilterClientIps(array $clientIps, $ip)
19521962
{
1963+
if (!$clientIps) {
1964+
return array();
1965+
}
19531966
$clientIps[] = $ip; // Complete the IP chain with the IP the request actually came from
19541967
$firstTrustedIp = null;
19551968

‎src/Symfony/Component/HttpFoundation/Tests/RequestTest.php

+50-1
Original file line numberDiff line numberDiff line change
@@ -1631,7 +1631,7 @@ private function getRequestInstanceForClientIpsForwardedTests($remoteAddr, $http
16311631
return $request;
16321632
}
16331633

1634-
public function testTrustedProxies()
1634+
public function testTrustedProxiesXForwardedFor()
16351635
{
16361636
$request = Request::create('http://example.com/');
16371637
$request->server->set('REMOTE_ADDR', '3.3.3.3');
@@ -1714,6 +1714,55 @@ public function testTrustedProxies()
17141714
Request::setTrustedHeaderName(Request::HEADER_CLIENT_PROTO, 'X_FORWARDED_PROTO');
17151715
}
17161716

1717+
public function testTrustedProxiesForwarded()
1718+
{
1719+
$request = Request::create('http://example.com/');
1720+
$request->server->set('REMOTE_ADDR', '3.3.3.3');
1721+
$request->headers->set('FORWARDED', 'for=1.1.1.1, host=foo.example.com:8080, proto=https, for=2.2.2.2, host=real.example.com:8080');
1722+
1723+
// no trusted proxies
1724+
$this->assertEquals('3.3.3.3', $request->getClientIp());
1725+
$this->assertEquals('example.com', $request->getHost());
1726+
$this->assertEquals(80, $request->getPort());
1727+
$this->assertFalse($request->isSecure());
1728+
1729+
// disabling proxy trusting
1730+
Request::setTrustedProxies(array());
1731+
$this->assertEquals('3.3.3.3', $request->getClientIp());
1732+
$this->assertEquals('example.com', $request->getHost());
1733+
$this->assertEquals(80, $request->getPort());
1734+
$this->assertFalse($request->isSecure());
1735+
1736+
// request is forwarded by a non-trusted proxy
1737+
Request::setTrustedProxies(array('2.2.2.2'));
1738+
$this->assertEquals('3.3.3.3', $request->getClientIp());
1739+
$this->assertEquals('example.com', $request->getHost());
1740+
$this->assertEquals(80, $request->getPort());
1741+
$this->assertFalse($request->isSecure());
1742+
1743+
// trusted proxy via setTrustedProxies()
1744+
Request::setTrustedProxies(array('3.3.3.3', '2.2.2.2'));
1745+
$this->assertEquals('1.1.1.1', $request->getClientIp());
1746+
$this->assertEquals('foo.example.com', $request->getHost());
1747+
$this->assertEquals(8080, $request->getPort());
1748+
$this->assertTrue($request->isSecure());
1749+
1750+
// trusted proxy via setTrustedProxies()
1751+
Request::setTrustedProxies(array('3.3.3.4', '2.2.2.2'));
1752+
$this->assertEquals('3.3.3.3', $request->getClientIp());
1753+
$this->assertEquals('example.com', $request->getHost());
1754+
$this->assertEquals(80, $request->getPort());
1755+
$this->assertFalse($request->isSecure());
1756+
1757+
// check various X_FORWARDED_PROTO header values
1758+
Request::setTrustedProxies(array('3.3.3.3', '2.2.2.2'));
1759+
$request->headers->set('FORWARDED', 'proto=ssl');
1760+
$this->assertTrue($request->isSecure());
1761+
1762+
$request->headers->set('FORWARDED', 'proto=https, proto=http');
1763+
$this->assertTrue($request->isSecure());
1764+
}
1765+
17171766
/**
17181767
* @expectedException \InvalidArgumentException
17191768
*/

‎src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestListenerTest.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ public function testListenerThrowsWhenMasterRequestHasInconsistentClientIps()
3232
$request = new Request();
3333
$request->setTrustedProxies(array('1.1.1.1'));
3434
$request->server->set('REMOTE_ADDR', '1.1.1.1');
35-
$request->headers->set('FORWARDED', '2.2.2.2');
35+
$request->headers->set('FORWARDED', 'for=2.2.2.2');
3636
$request->headers->set('X_FORWARDED_FOR', '3.3.3.3');
3737

3838
$dispatcher->addListener(KernelEvents::REQUEST, array(new ValidateRequestListener(), 'onKernelRequest'));

‎src/Symfony/Component/HttpKernel/Tests/HttpKernelTest.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,7 @@ public function testInconsistentClientIpsOnMasterRequests()
287287
$request = new Request();
288288
$request->setTrustedProxies(array('1.1.1.1'));
289289
$request->server->set('REMOTE_ADDR', '1.1.1.1');
290-
$request->headers->set('FORWARDED', '2.2.2.2');
290+
$request->headers->set('FORWARDED', 'for=2.2.2.2');
291291
$request->headers->set('X_FORWARDED_FOR', '3.3.3.3');
292292

293293
$kernel->handle($request, $kernel::MASTER_REQUEST, false);

0 commit comments

Comments
 (0)