8000 Merge branch '2.8' into 3.4 · alex-dev/symfony@bcf5897 · GitHub
[go: up one dir, main page]
Skip to content

Commit bcf5897

Browse files
nicolas-grekasnicolas-grekas
committed
Merge branch '2.8' into 3.4
* 2.8: [HttpKernel] fix trusted headers management in HttpCache and InlineFragmentRenderer
2 parents 768abbf + 0f7667d commit bcf5897

File tree

8 files changed

+317
-87
lines changed

8 files changed

+317
-87
lines changed

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2087,6 +2087,11 @@ private function getTrustedValues($type, $ip = null)
20872087
if (self::$trustedHeaders[self::HEADER_FORWARDED] && $this->headers->has(self::$trustedHeaders[self::HEADER_FORWARDED])) {
20882088
$forwardedValues = $this->headers->get(self::$trustedHeaders[self::HEADER_FORWARDED]);
20892089
$forwardedValues = preg_match_all(sprintf('{(?:%s)=(?:"?\[?)([a-zA-Z0-9\.:_\-/]*+)}', self::$forwardedParams[$type]), $forwardedValues, $matches) ? $matches[1] : array();
2090+
if (self::HEADER_CLIENT_PORT === $type) {
2091+
foreach ($forwardedValues as $k => $v) {
2092+
$forwardedValues[$k] = substr_replace($v, '0.0.0.0', 0, strrpos($v, ':'));
2093+
}
2094+
}
20902095
}
20912096

20922097
if (null !== $ip) {

‎src/Symfony/Component/HttpKernel/Fragment/InlineFragmentRenderer.php

Lines changed: 2 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use Symfony\Component\HttpFoundation\Response;
1717
use Symfony\Component\HttpKernel\Controller\ControllerReference;
1818
use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent;
19+
use Symfony\Component\HttpKernel\HttpCache\SubRequestHandler;
1920
use Symfony\Component\HttpKernel\HttpKernelInterface;
2021
use Symfony\Component\HttpKernel\KernelEvents;
2122

@@ -76,7 +77,7 @@ public function render($uri, Request $request, array $options = array())
7677

7778
$level = ob_get_level();
7879
try {
79-
return $this->kernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST, false);
80+
return SubRequestHandler::handle($this->kernel, $subRequest, HttpKernelInterface::SUB_REQUEST, false);
8081
} catch (\Exception $e) {
8182
// we dispatch the exception event to trigger the logging
8283
// the response that comes back is simply ignored
@@ -109,25 +110,6 @@ protected function createSubRequest($uri, Request $request)
109110
$cookies = $request->cookies->all();
110111
$server = $request->server->all();
111112

112-
// Override the arguments to emulate a sub-request.
113-
// Sub-request object will point to localhost as client ip and real client ip
114-
// will be included into trusted header for client ip
115-
try {
116-
if (Request::HEADER_X_FORWARDED_FOR & Request::getTrustedHeaderSet()) {
117-
$currentXForwardedFor = $request->headers->get('X_FORWARDED_FOR', '');
118-
119-
$server['HTTP_X_FORWARDED_FOR'] = ($currentXForwardedFor ? $currentXForwardedFor.', ' : '').$request->getClientIp();
120-
} elseif (method_exists(Request::class, 'getTrustedHeaderName') && $trustedHeaderName = Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP, false)) {
121-
$currentXForwardedFor = $request->headers->get($trustedHeaderName, '');
122-
123-
$server['HTTP_'.$trustedHeaderName] = ($currentXForwardedFor ? $currentXForwardedFor.', ' : '').$request->getClientIp();
124-
}
125-
} catch (\InvalidArgumentException $e) {
126-
// Do nothing
127-
}
128-
129-
$server['REMOTE_ADDR'] = $this->resolveTrustedProxy();
130-
131113
unset($server['HTTP_IF_MODIFIED_SINCE']);
132114
unset($server['HTTP_IF_NONE_MATCH']);
133115

@@ -143,17 +125,6 @@ protected function createSubRequest($uri, Request $request)
143125
return $subRequest;
144126
}
145127

146-
private function resolveTrustedProxy()
147-
{
148-
if (!$trustedProxies = Request::getTrustedProxies()) {
149-
return '127.0.0.1';
150-
}
151-
152-
$firstTrustedProxy = reset($trustedProxies);
153-
154-
return false !== ($i = strpos($firstTrustedProxy, '/')) ? substr($firstTrustedProxy, 0, $i) : $firstTrustedProxy;
155-
}
156-
157128
/**
158129
* {@inheritdoc}
159130
*/

‎src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -444,27 +444,8 @@ protected function forward(Request $request, $catch = false, Response $entry = n
444444
$this->surrogate->addSurrogateCapability($request);
445445
}
446446

447-
// modify the X-Forwarded-For header if needed
448-
$forwardedFor = $request->headers->get('X-Forwarded-For');
449-
if ($forwardedFor) {
450-
$request->headers->set('X-Forwarded-For', $forwardedFor.', '.$request->server->get('REMOTE_ADDR'));
451-
} else {
452-
$request->headers->set('X-Forwarded-For', $request->server->get('REMOTE_ADDR'));
453-
}
454-
455-
// fix the client IP address by setting it to 127.0.0.1 as HttpCache
456-
// is always called from the same process as the backend.
457-
$request->server->set('REMOTE_ADDR', '127.0.0.1');
458-
459-
// make sure HttpCache is a trusted proxy
460-
if (!\in_array('127.0.0.1', $trustedProxies = Request::getTrustedProxies())) {
461-
$trustedProxies[] = '127.0.0.1';
462-
Request::setTrustedProxies($trustedProxies, Request::HEADER_X_FORWARDED_ALL);
463-
}
464-
465447
// always a "master" request (as the real master request can be in cache)
466-
$response = $this->kernel->handle($request, HttpKernelInterface::MASTER_REQUEST, $catch);
467-
// FIXME: we probably need to also catch exceptions if raw === true
448+
$response = SubRequestHandler::handle($this->kernel, $request, HttpKernelInterface::MASTER_REQUEST, $catch);
468449

469450
// we don't implement the stale-if-error on Requests, which is nonetheless part of the RFC
470451
if (null !== $entry && \in_array($response->getStatusCode(), array(500, 502, 503, 504))) {

‎src/Symfony/Component/HttpKernel/HttpCache/SubRequestHandler.php

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\HttpKernel\HttpCache;
13+
14+
use Symfony\Component\HttpFoundation\IpUtils;
15+
use Symfony\Component\HttpFoundation\Request;
16+
use Symfony\Component\HttpFoundation\Response;
17+
use Symfony\Component\HttpKernel\HttpKernelInterface;
18+
19+
/**
20+
* @author Nicolas Grekas <p@tchwork.com>
21+
*
22+
* @internal
23+
*/
24+
class SubRequestHandler
25+
{
26+
/**
27+
* @return Response
28+
*/
29+
public static function handle(HttpKernelInterface $kernel, Request $request, $type, $catch)
30+
{
31+
// save global state related to trusted headers and proxies
32+
$trustedProxies = Request::getTrustedProxies();
33+
$trustedHeaderSet = Request::getTrustedHeaderSet();
34+
if (\method_exists(Request::class, 'getTrustedHeaderName')) {
35+
Request::setTrustedProxies($trustedProxies, -1);
36+
$trustedHeaders = array(
37+
Request::HEADER_FORWARDED => Request::getTrustedHeaderName(Request::HEADER_FORWARDED, false),
38+
Request::HEADER_X_FORWARDED_FOR => Request::getTrustedHeaderName(Request::HEADER_X_FORWARDED_FOR, false),
39+
Request::HEADER_X_FORWARDED_HOST => Request::getTrustedHeaderName(Request::HEADER_X_FORWARDED_HOST, false),
40+
Request::HEADER_X_FORWARDED_PROTO => Request::getTrustedHeaderName(Request::HEADER_X_FORWARDED_PROTO, false),
41+
Request::HEADER_X_FORWARDED_PORT => Request::getTrustedHeaderName(Request::HEADER_X_FORWARDED_PORT, false),
42+
);
43+
Request::setTrustedProxies($trustedProxies, $trustedHeaderSet);
44+
} else {
45+
$trustedHeaders = array(
46+
Request::HEADER_FORWARDED => 'FORWARDED',
47+
Request::HEADER_X_FORWARDED_FOR => 'X_FORWARDED_FOR',
48+
Request::HEADER_X_FORWARDED_HOST => 'X_FORWARDED_HOST',
49+
Request::HEADER_X_FORWARDED_PROTO => 'X_FORWARDED_PROTO',
50+
Request::HEADER_X_FORWARDED_PORT => 'X_FORWARDED_PORT',
51+
);
52+
}
53+
54+
// remove untrusted values
55+
$remoteAddr = $request->server->get('REMOTE_ADDR');
56+
if (!IpUtils::checkIp($remoteAddr, $trustedProxies)) {
57+
foreach ($trustedHeaders as $key => $name) {
58+
if ($trustedHeaderSet & $key) {
59+
$request->headers->remove($name);
60+
}
61+
}
62+
}
63+
64+
// compute trusted values, taking any trusted proxies into account
65+
$trustedIps = array();
66+
$trustedValues = array();
67+
foreach (array_reverse($request->getClientIps()) as $ip) {
68+
$trustedIps[] = $ip;
69+
$trustedValues[] = sprintf('for="%s"', $ip);
70+
}
71+
if ($ip !== $remoteAddr) {
72+
$trustedIps[] = $remoteAddr;
73+
$trustedValues[] = sprintf('for="%s"', $remoteAddr);
74+
}
75+
76+
// set trusted values, reusing as much as possible the global trusted settings
77+
if (Request::HEADER_FORWARDED & $trustedHeaderSet) {
78+
$trustedValues[0] .= sprintf(';host="%s";proto=%s', $request->getHttpHost(), $request->getScheme());
79+
$request->headers->set($trustedHeaders[Request::HEADER_FORWARDED], implode(', ', $trustedValues));
80+
}
81+
if (Request::HEADER_X_FORWARDED_FOR & $trustedHeaderSet) {
82+
$request->headers->set($trustedHeaders[Request::HEADER_X_FORWARDED_FOR], implode(', ', $trustedIps));
83+
} elseif (!(Request::HEADER_FORWARDED & $trustedHeaderSet)) {
84+
Request::setTrustedProxies($trustedProxies, $trustedHeaderSet | Request::HEADER_X_FORWARDED_FOR);
85+
$request->headers->set($trustedHeaders[Request::HEADER_X_FORWARDED_FOR], implode(', ', $trustedIps));
86+
}
87+
88+
// fix the client IP address by setting it to 127.0.0.1,
89+
// which is the core responsibility of this method
90+
$request->server->set('REMOTE_ADDR', '127.0.0.1');
91+
92+
// ensure 127.0.0.1 is set as trusted proxy
93+
if (!IpUtils::checkIp('127.0.0.1', $trustedProxies)) {
94+
Request::setTrustedProxies(array_merge($trustedProxies, array('127.0.0.1')), Request::getTrustedHeaderSet());
95+
}
96+
97+
try {
98+
return $kernel->handle($request, $type, $catch);
99+
} finally {
100+
// restore global state
101+
Request::setTrustedProxies($trustedProxies, $trustedHeaderSet);
102+
}
103+
}
104+
}

‎src/Symfony/Component/HttpKernel/Tests/Fragment/InlineFragmentRendererTest.php

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public function testRenderWithObjectsAsAttributes()
4646
$subRequest = Request::create('/_fragment?_path=_format%3Dhtml%26_locale%3Den%26_controller%3Dmain_controller');
4747
$subRequest->attributes->replace(array('object' => $object, '_format' => 'html', '_controller' => 'main_controller', '_locale' => 'en'));
4848
$subRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
49-
$subRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
49+
$subRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
5050

5151
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($subRequest));
5252

@@ -99,7 +99,10 @@ public function testRenderWithTrustedHeaderDisabled()
9999
{
100100
Request::setTrustedProxies(array(), 0);
101101

102-
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest(Request::create('/')));
102+
$expectedSubRequest = Request::create('/');
103+
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
104+
105+
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
103106
$this->assertSame('foo', $strategy->render('/', Request::create('/'))->getContent());
104107

105108
Request::setTrustedProxies(array(), -1);
@@ -190,8 +193,8 @@ public function testESIHeaderIsKeptInSubrequest()
190193

191194
if (Request::HEADER_X_FORWARDED_FOR & Request::getTrustedHeaderSet()) {
192195
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
193-
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
194196
}
197+
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
195198

196199
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
197200

@@ -202,7 +205,7 @@ public function testESIHeaderIsKeptInSubrequest()
202205

203206
public function testESIHeaderIsKeptInSubrequestWithTrustedHeaderDisabled()
204207
{
205-
Request::setTrustedProxies(array(), 0);
208+
Request::setTrustedProxies(array(), Request::HEADER_FORWARDED);
206209

207210
$this->testESIHeaderIsKeptInSubrequest();
208211

@@ -213,7 +216,7 @@ public function testHeadersPossiblyResultingIn304AreNotAssignedToSubrequest()
213216
{
214217
$expectedSubRequest = Request::create('/');
215218
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
216-
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
219+
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
217220

218221
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
219222
$request = Request::create('/', 'GET', array(), array(), array(), array('HTTP_IF_MODIFIED_SINCE' => 'Fri, 01 Jan 2016 00:00:00 GMT', 'HTTP_IF_NONE_MATCH' => '*'));
@@ -226,9 +229,9 @@ public function testFirstTrustedProxyIsSetAsRemote()
226229

227230
$expectedSubRequest = Request::create('/');
228231
$expectedSubRequest->headers->set('Surrogate-Capability', 'abc="ESI/1.0"');
229-
$expectedSubRequest->server->set('REMOTE_ADDR', '1.1.1.1');
232+
$expectedSubRequest->server->set('REMOTE_ADDR', '127.0.0.1');
230233
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
231-
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
234+
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
232235

233236
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
234237

@@ -243,9 +246,9 @@ public function testIpAddressOfRangedTrustedProxyIsSetAsRemote()
243246
{
244247
$expectedSubRequest = Request::create('/');
245248
$expectedSubRequest->headers->set('Surrogate-Capability', 'abc="ESI/1.0"');
246-
$expectedSubRequest->server->set('REMOTE_ADDR', '1.1.1.1');
249+
$expectedSubRequest->server->set('REMOTE_ADDR', '127.0.0.1');
247250
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
248-
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
251+
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
249252

250253
Request::setTrustedProxies(array('1.1.1.1/24'), -1);
251254

‎src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php

Lines changed: 27 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1338,66 +1338,69 @@ public function testClientIpIsAlwaysLocalhostForForwardedRequests()
13381338
$this->setNextResponse();
13391339
$this->request('GET', '/', array('REMOTE_ADDR' => '10.0.0.1'));
13401340

1341-
$this->assertEquals('127.0.0.1', $this->kernel->getBackendRequest()->server->get('REMOTE_ADDR'));
1341+
$this->kernel->assert(function ($backendRequest) {
1342+
$this->assertSame('127.0.0.1', $backendRequest->server->get('REMOTE_ADDR'));
1343+
});
13421344
}
13431345

13441346
/**
13451347
* @dataProvider getTrustedProxyData
13461348
*/
1347-
public function testHttpCacheIsSetAsATrustedProxy(array $existing, array $expected)
1349+
public function testHttpCacheIsSetAsATrustedProxy(array $existing)
13481350
{
13491351
Request::setTrustedProxies($existing, Request::HEADER_X_FORWARDED_ALL);
13501352

13511353
$this->setNextResponse();
13521354
$this->request('GET', '/', array('REMOTE_ADDR' => '10.0.0.1'));
1355+
$this->assertSame($existing, Request::getTrustedProxies());
13531356

1354-
$this->assertEquals($expected, Request::getTrustedProxies());
1357+
$existing = array_unique(array_merge($existing, array('127.0.0.1')));
1358+
$this->kernel->assert(function ($backendRequest) use ($existing) {
1359+
$this->assertSame($existing, Request::getTrustedProxies());
1360+
$this->assertsame('10.0.0.1', $backendRequest->getClientIp());
1361+
});
13551362

13561363
Request::setTrustedProxies(array(), -1);
13571364
}
13581365

13591366
public function getTrustedProxyData()
13601367
{
13611368
return array(
1362-
array(array(), array('127.0.0.1')),
1363-
array(array('10.0.0.2'), array('10.0.0.2', '127.0.0.1')),
1364-
array(array('10.0.0.2', '127.0.0.1'), array('10.0.0.2', '127.0.0.1')),
1369+
array(array()),
1370+
array(array('10.0.0.2')),
1371+
array(array('10.0.0.2', '127.0.0.1')),
13651372
);
13661373
}
13671374

13681375
/**
1369-
* @dataProvider getXForwardedForData
1376+
* @dataProvider getForwardedData
13701377
*/
1371-
public function testXForwarderForHeaderForForwardedRequests($xForwardedFor, $expected)
1378+
public function testForwarderHeaderForForwardedRequests($forwarded, $expected)
13721379
{
13731380
$this->setNextResponse();
13741381
$server = array('REMOTE_ADDR' => '10.0.0.1');
1375-
if (false !== $xForwardedFor) {
1376-
$server['HTTP_X_FORWARDED_FOR'] = $xForwardedFor;
1382+
if (null !== $forwarded) {
1383+
Request::setTrustedProxies($server, -1);
1384+
$server['HTTP_FORWARDED'] = $forwarded;
13771385
}
13781386
$this->request('GET', '/', $server);
13791387

1380-
$this->assertEquals($expected, $this->kernel->getBackendRequest()->headers->get('X-Forwarded-For'));
1388+
$this->kernel->assert(function ($backendRequest) use ($expected) {
1389+
$this->assertSame($expected, $backendRequest->headers->get('Forwarded'));
1390+
});
1391+
1392+
Request::setTrustedProxies(array(), -1);
13811393
}
13821394

1383-
public function getXForwardedForData()
1395+
public function getForwardedData()
13841396
{
13851397
return array(
1386-
array(false, '10.0.0.1'),
1387-
array('10.0.0.2', '10.0.0.2, 10.0.0.1'),
1388-
array('10.0.0.2, 10.0.0.3', '10.0.0.2, 10.0.0.3, 10.0.0.1'),
1398+
array(null, 'for="10.0.0.1";host="localhost";proto=http'),
1399+
array('for=10.0.0.2', 'for="10.0.0.2";host="localhost";proto=http, for="10.0.0.1"'),
1400+
array('for=10.0.0.2, for=10.0.0.3', 'for="10.0.0.2";host="localhost";proto=http, for="10.0.0.3", for="10.0.0.1"'),
13891401
);
13901402
}
13911403

1392-
public function testXForwarderForHeaderForPassRequests()
1393-
{
1394-
$this->setNextResponse();
1395-
$server = array('REMOTE_ADDR' => '10.0.0.1');
1396-
$this->request('POST', '/', $server);
1397-
1398-
$this->assertEquals('10.0.0.1', $this->kernel->getBackendRequest()->headers->get('X-Forwarded-For'));
1399-
}
1400-
14011404
public function testEsiCacheRemoveValidationHeadersIfEmbeddedResponses()
14021405
{
14031406
$time = \DateTime::createFromFormat('U', time());

0 commit comments

Comments
 (0)
0