8000 Merge branch '3.4' into 4.0 · symfony/http-kernel@d2e26c5 · GitHub
[go: up one dir, main page]
Skip to content

Commit d2e26c5

Browse files
nicolas-grekasnicolas-grekas
committed
Merge branch '3.4' into 4.0
* 3.4: [HttpKernel] fix trusted headers management in HttpCache and InlineFragmentRenderer
2 parents e4c47ed + 19c8e1e commit d2e26c5

File tree

7 files changed

+295
-76
lines changed

7 files changed

+295
-76
lines changed

‎Fragment/InlineFragmentRenderer.php

Lines changed: 2 additions & 20 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,14 +110,6 @@ protected function createSubRequest($uri, Request $request)
109110
$cookies = $request->cookies->all();
110111
$server = $request->server->all();
111112

112-
if (Request::HEADER_X_FORWARDED_FOR & Request::getTrustedHeaderSet()) {
113-
$currentXForwardedFor = $request->headers->get('X_FORWARDED_FOR', '');
114-
115-
$server['HTTP_X_FORWARDED_FOR'] = ($currentXForwardedFor ? $currentXForwardedFor.', ' : '').$request->getClientIp();
116-
}
117-
118-
$server['REMOTE_ADDR'] = $this->resolveTrustedProxy();
119-
120113
unset($server['HTTP_IF_MODIFIED_SINCE']);
121114
unset($server['HTTP_IF_NONE_MATCH']);
122115

@@ -132,17 +125,6 @@ protected function createSubRequest($uri, Request $request)
132125
return $subRequest;
133126
}
134127

135-
private function resolveTrustedProxy()
136-
{
137-
if (!$trustedProxies = Request::getTrustedProxies()) {
138-
return '127.0.0.1';
139-
}
140-
141-
$firstTrustedProxy = reset($trustedProxies);
142-
143-
return false !== ($i = strpos($firstTrustedProxy, '/')) ? substr($firstTrustedProxy, 0, $i) : $firstTrustedProxy;
144-
}
145-
146128
/**
147129
* {@inheritdoc}
148130
*/

‎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))) {

‎HttpCache/SubRequestHandler.php

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
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+
public static function handle< EF5E /span>(HttpKernelInterface $kernel, Request $request, $type, $catch): Response
27+
{
28+
// save global state related to trusted headers and proxies
29+
$trustedProxies = Request::getTrustedProxies();
30+
$trustedHeaderSet = Request::getTrustedHeaderSet();
31+
32+
// remove untrusted values
33+
$remoteAddr = $request->server->get('REMOTE_ADDR');
34+
if (!IpUtils::checkIp($remoteAddr, $trustedProxies)) {
35+
$trustedHeaders = array(
36+
'FORWARDED' => $trustedHeaderSet & Request::HEADER_FORWARDED,
37+
'X_FORWARDED_FOR' => $trustedHeaderSet & Request::HEADER_X_FORWARDED_FOR,
38+
'X_FORWARDED_HOST' => $trustedHeaderSet & Request::HEADER_X_FORWARDED_HOST,
39+
'X_FORWARDED_PROTO' => $trustedHeaderSet & Request::HEADER_X_FORWARDED_PROTO,
40+
'X_FORWARDED_PORT' => $trustedHeaderSet & Request::HEADER_X_FORWARDED_PORT,
41+
);
42+
foreach (array_filter($trustedHeaders) as $name => $key) {
43+
$request->headers->remove($name);
44+
}
45+
}
46+
47+
// compute trusted values, taking any trusted proxies into account
48+
$trustedIps = array();
49+
$trustedValues = array();
50+
foreach (array_reverse($request->getClientIps()) as $ip) {
51+
$trustedIps[] = $ip;
52+
$trustedValues[] = sprintf('for="%s"', $ip);
53+
}
54+
if ($ip !== $remoteAddr) {
55+
$trustedIps[] = $remoteAddr;
56+
$trustedValues[] = sprintf('for="%s"', $remoteAddr);
57+
}
58+
59+
// set trusted values, reusing as much as possible the global trusted settings
60+
if (Request::HEADER_FORWARDED & $trustedHeaderSet) {
61+
$trustedValues[0] .= sprintf(';host="%s";proto=%s', $request->getHttpHost(), $request->getScheme());
62+
$request->headers->set('Forwarded', implode(', ', $trustedValues));
63+
}
64+
if (Request::HEADER_X_FORWARDED_FOR & $trustedHeaderSet) {
65+
$request->headers->set('X-Forwarded-For', implode(', ', $trustedIps));
66+
} elseif (!(Request::HEADER_FORWARDED & $trustedHeaderSet)) {
67+
Request::setTrustedProxies($trustedProxies, $trustedHeaderSet | Request::HEADER_X_FORWARDED_FOR);
68+
$request->headers->set('X-Forwarded-For', implode(', ', $trustedIps));
69+
}
70+
71+
// fix the client IP address by setting it to 127.0.0.1,
72+
// which is the core responsibility of this method
73+
$request->server->set('REMOTE_ADDR', '127.0.0.1');
74+
75+
// ensure 127.0.0.1 is set as trusted proxy
76+
if (!IpUtils::checkIp('127.0.0.1', $trustedProxies)) {
77+
Request::setTrustedProxies(array_merge($trustedProxies, array('127.0.0.1')), Request::getTrustedHeaderSet());
78+
}
79+
80+
try {
81+
return $kernel->handle($request, $type, $catch);
82+
} finally {
83+
// restore global state
84+
Request::setTrustedProxies($trustedProxies, $trustedHeaderSet);
85+
}
86+
}
87+
}

‎Tests/Fragment/InlineFragmentRendererTest.php

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

4949
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($subRequest));
5050

@@ -55,7 +55,10 @@ public function testRenderWithTrustedHeaderDisabled()
5555
{
5656
Request::setTrustedProxies(array(), 0);
5757

58-
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest(Request::create('/')));
58+
$expectedSubRequest = Request::create('/');
59+
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
60+
61+
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
5962
$this->assertSame('foo', $strategy->render('/', Request::create('/'))->getContent());
6063

6164
Request::setTrustedProxies(array(), -1);
@@ -146,8 +149,8 @@ public function testESIHeaderIsKeptInSubrequest()
146149

147150
if (Request::HEADER_X_FORWARDED_FOR & Request::getTrustedHeaderSet()) {
148151
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
149-
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
150152
}
153+
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
151154

152155
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
153156

@@ -158,7 +161,7 @@ public function testESIHeaderIsKeptInSubrequest()
158161

159162
public function testESIHeaderIsKeptInSubrequestWithTrustedHeaderDisabled()
160163
{
161-
Request::setTrustedProxies(array(), 0);
164+
Request::setTrustedProxies(array(), Request::HEADER_FORWARDED);
162165

163166
$this->testESIHeaderIsKeptInSubrequest();
164167

@@ -169,7 +172,7 @@ public function testHeadersPossiblyResultingIn304AreNotAssignedToSubrequest()
169172
{
170173
$expectedSubRequest = Request::create('/');
171174
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
172-
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
175+
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
173176

174177
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
175178
$request = Request::create('/', 'GET', array(), array(), array(), array('HTTP_IF_MODIFIED_SINCE' => 'Fri, 01 Jan 2016 00:00:00 GMT', 'HTTP_IF_NONE_MATCH' => '*'));
@@ -182,9 +185,9 @@ public function testFirstTrustedProxyIsSetAsRemote()
182185

183186
$expectedSubRequest = Request::create('/');
184187
$expectedSubRequest->headers->set('Surrogate-Capability', 'abc="ESI/1.0"');
185-
$expectedSubRequest->server->set('REMOTE_ADDR', '1.1.1.1');
188+
$expectedSubRequest->server->set('REMOTE_ADDR', '127.0.0.1');
186189
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
187-
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
190+
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
188191

189192
$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
190193

@@ -199,9 +202,9 @@ public function testIpAddressOfRangedTrustedProxyIsSetAsRemote()
199202
{
200203
$expectedSubRequest = Request::create('/');
201204
$expectedSubRequest->headers->set('Surrogate-Capability', 'abc="ESI/1.0"');
202-
$expectedSubRequest->server->set('REMOTE_ADDR', '1.1.1.1');
205+
$expectedSubRequest->server->set('REMOTE_ADDR', '127.0.0.1');
203206
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
204-
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
207+
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));
205208

206209
Request::setTrustedProxies(array('1.1.1.1/24'), -1);
207210

‎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