Skip to content

Commit

Permalink
[HttpKernel] fix trusted headers management in HttpCache and InlineFr…
Browse files Browse the repository at this point in the history
…agmentRenderer
  • Loading branch information
nicolas-grekas committed Aug 1, 2018
1 parent 6604978 commit 725dee4
Show file tree
Hide file tree
Showing 8 changed files with 350 additions and 92 deletions.
5 changes: 5 additions & 0 deletions src/Symfony/Component/HttpFoundation/Request.php
Expand Up @@ -1944,6 +1944,11 @@ private function getTrustedValues($type, $ip = null)
if (self::$trustedHeaders[self::HEADER_FORWARDED] && $this->headers->has(self::$trustedHeaders[self::HEADER_FORWARDED])) {
$forwardedValues = $this->headers->get(self::$trustedHeaders[self::HEADER_FORWARDED]);
$forwardedValues = preg_match_all(sprintf('{(?:%s)=(?:"?\[?)([a-zA-Z0-9\.:_\-/]*+)}', self::$forwardedParams[$type]), $forwardedValues, $matches) ? $matches[1] : array();
if (self::HEADER_CLIENT_PORT === $type) {
foreach ($forwardedValues as $k => $v) {
$forwardedValues[$k] = substr_replace($v, '0.0.0.0', 0, strrpos($v, ':'));
}
}
}

if (null !== $ip) {
Expand Down
Expand Up @@ -16,6 +16,7 @@
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Controller\ControllerReference;
use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent;
use Symfony\Component\HttpKernel\HttpCache\SubRequestHandler;
use Symfony\Component\HttpKernel\HttpKernelInterface;
use Symfony\Component\HttpKernel\KernelEvents;

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

$level = ob_get_level();
try {
return $this->kernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST, false);
return SubRequestHandler::handle($this->kernel, $subRequest, HttpKernelInterface::SUB_REQUEST, false);
} catch (\Exception $e) {
// we dispatch the exception event to trigger the logging
// the response that comes back is simply ignored
Expand Down Expand Up @@ -109,21 +110,6 @@ protected function createSubRequest($uri, Request $request)
$cookies = $request->cookies->all();
$server = $request->server->all();

// Override the arguments to emulate a sub-request.
// Sub-request object will point to localhost as client ip and real client ip
// will be included into trusted header for client ip
try {
if ($trustedHeaderName = Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP)) {
$currentXForwardedFor = $request->headers->get($trustedHeaderName, '');

$server['HTTP_'.$trustedHeaderName] = ($currentXForwardedFor ? $currentXForwardedFor.', ' : '').$request->getClientIp();
}
} catch (\InvalidArgumentException $e) {
// Do nothing
}

$server['REMOTE_ADDR'] = $this->resolveTrustedProxy();

unset($server['HTTP_IF_MODIFIED_SINCE']);
unset($server['HTTP_IF_NONE_MATCH']);

Expand All @@ -139,17 +125,6 @@ protected function createSubRequest($uri, Request $request)
return $subRequest;
}

private function resolveTrustedProxy()
{
if (!$trustedProxies = Request::getTrustedProxies()) {
return '127.0.0.1';
}

$firstTrustedProxy = reset($trustedProxies);

return false !== ($i = strpos($firstTrustedProxy, '/')) ? substr($firstTrustedProxy, 0, $i) : $firstTrustedProxy;
}

/**
* {@inheritdoc}
*/
Expand Down
21 changes: 1 addition & 20 deletions src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php
Expand Up @@ -468,27 +468,8 @@ protected function forward(Request $request, $catch = false, Response $entry = n
$this->surrogate->addSurrogateCapability($request);
}

// modify the X-Forwarded-For header if needed
$forwardedFor = $request->headers->get('X-Forwarded-For');
if ($forwardedFor) {
$request->headers->set('X-Forwarded-For', $forwardedFor.', '.$request->server->get('REMOTE_ADDR'));
} else {
$request->headers->set('X-Forwarded-For', $request->server->get('REMOTE_ADDR'));
}

// fix the client IP address by setting it to 127.0.0.1 as HttpCache
// is always called from the same process as the backend.
$request->server->set('REMOTE_ADDR', '127.0.0.1');

// make sure HttpCache is a trusted proxy
if (!\in_array('127.0.0.1', $trustedProxies = Request::getTrustedProxies())) {
$trustedProxies[] = '127.0.0.1';
Request::setTrustedProxies($trustedProxies);
}

// always a "master" request (as the real master request can be in cache)
$response = $this->kernel->handle($request, HttpKernelInterface::MASTER_REQUEST, $catch);
// FIXME: we probably need to also catch exceptions if raw === true
$response = SubRequestHandler::handle($this->kernel, $request, HttpKernelInterface::MASTER_REQUEST, $catch);

// we don't implement the stale-if-error on Requests, which is nonetheless part of the RFC
if (null !== $entry && \in_array($response->getStatusCode(), array(500, 502, 503, 504))) {
Expand Down
100 changes: 100 additions & 0 deletions src/Symfony/Component/HttpKernel/HttpCache/SubRequestHandler.php
@@ -0,0 +1,100 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\HttpKernel\HttpCache;

use Symfony\Component\HttpFoundation\IpUtils;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\HttpKernelInterface;

/**
* @author Nicolas Grekas <p@tchwork.com>
*
* @internal
*/
class SubRequestHandler
{
/**
* @return Response
*/
public static function handle(HttpKernelInterface $kernel, Request $request, $type, $catch)
{
// save global state related to trusted headers and proxies
$trustedProxies = Request::getTrustedProxies();
$trustedHeaders = array(
Request::HEADER_FORWARDED => Request::getTrustedHeaderName(Request::HEADER_FORWARDED),
Request::HEADER_CLIENT_IP => Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP),
Request::HEADER_CLIENT_HOST => Request::getTrustedHeaderName(Request::HEADER_CLIENT_HOST),
Request::HEADER_CLIENT_PROTO => Request::getTrustedHeaderName(Request::HEADER_CLIENT_PROTO),
Request::HEADER_CLIENT_PORT => Request::getTrustedHeaderName(Request::HEADER_CLIENT_PORT),
);

// remove untrusted values
$remoteAddr = $request->server->get('REMOTE_ADDR');
if (!IpUtils::checkIp($remoteAddr, $trustedProxies)) {
foreach (array_filter($trustedHeaders) as $name) {
$request->headers->remove($name);
}
}

// compute trusted values, taking any trusted proxies into account
$trustedIps = array();
$trustedValues = array();
foreach (array_reverse($request->getClientIps()) as $ip) {
$trustedIps[] = $ip;
$trustedValues[] = sprintf('for="%s"', $ip);
}
if ($ip !== $remoteAddr) {
$trustedIps[] = $remoteAddr;
$trustedValues[] = sprintf('for="%s"', $remoteAddr);
}

// set trusted values, reusing as much as possible the global trusted settings
if ($name = $trustedHeaders[Request::HEADER_FORWARDED]) {
$trustedValues[0] .= sprintf(';host="%s";proto=%s', $request->getHttpHost(), $request->getScheme());
$request->headers->set($name, implode(', ', $trustedValues));
}
if ($name = $trustedHeaders[Request::HEADER_CLIENT_IP]) {
$request->headers->set($name, implode(', ', $trustedIps));
}
if (!$name && !$trustedHeaders[Request::HEADER_FORWARDED]) {
$request->headers->set('X-Forwarded-For', implode(', ', $trustedIps));
Request::setTrustedHeaderName(Request::HEADER_CLIENT_IP, 'X_FORWARDED_FOR');
}

// fix the client IP address by setting it to 127.0.0.1,
// which is the core responsibility of this method
$request->server->set('REMOTE_ADDR', '127.0.0.1');

// ensure 127.0.0.1 is set as trusted proxy
if (!IpUtils::checkIp('127.0.0.1', $trustedProxies)) {
Request::setTrustedProxies(array_merge($trustedProxies, array('127.0.0.1')));
}

try {
$e = null;
$response = $kernel->handle($request, $type, $catch);
} catch (\Throwable $e) {
} catch (\Exception $e) {
}

// restore global state
Request::setTrustedHeaderName(Request::HEADER_CLIENT_IP, $trustedHeaders[Request::HEADER_CLIENT_IP]);
Request::setTrustedProxies($trustedProxies);

if (null !== $e) {
throw $e;
}

return $response;
}
}
Expand Up @@ -26,12 +26,16 @@ class InlineFragmentRendererTest extends TestCase

protected function setUp()
{
$this->originalTrustedHeaderName = Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP);
$this->originalTrustedHeaderNames = array(
Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP),
Request::getTrustedHeaderName(Request::HEADER_FORWARDED),
);
}

protected function tearDown()
{
Request::setTrustedHeaderName(Request::HEADER_CLIENT_IP, $this->originalTrustedHeaderName);
Request::setTrustedHeaderName(Request::HEADER_CLIENT_IP, $this->originalTrustedHeaderNames[0]);
Request::setTrustedHeaderName(Request::HEADER_FORWARDED, $this->originalTrustedHeaderNames[1]);
}

public function testRender()
Expand All @@ -55,7 +59,7 @@ public function testRenderWithObjectsAsAttributes()
$subRequest = Request::create('/_fragment?_path=_format%3Dhtml%26_locale%3Den%26_controller%3Dmain_controller');
$subRequest->attributes->replace(array('object' => $object, '_format' => 'html', '_controller' => 'main_controller', '_locale' => 'en'));
$subRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
$subRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
$subRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));

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

Expand Down Expand Up @@ -83,8 +87,12 @@ public function testRenderWithObjectsAsAttributesPassedAsObjectsInTheController(
public function testRenderWithTrustedHeaderDisabled()
{
Request::setTrustedHeaderName(Request::HEADER_CLIENT_IP, '');
Request::setTrustedHeaderName(Request::HEADER_FORWARDED, '');

$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest(Request::create('/')));
$expectedSubRequest = Request::create('/');
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));

$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
$this->assertSame('foo', $strategy->render('/', Request::create('/'))->getContent());
}

Expand Down Expand Up @@ -168,11 +176,10 @@ public function testESIHeaderIsKeptInSubrequest()
{
$expectedSubRequest = Request::create('/');
$expectedSubRequest->headers->set('Surrogate-Capability', 'abc="ESI/1.0"');

if (Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP)) {
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
}
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));

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

Expand All @@ -194,10 +201,8 @@ public function testESIHeaderIsKeptInSubrequestWithTrustedHeaderDisabled()
public function testHeadersPossiblyResultingIn304AreNotAssignedToSubrequest()
{
$expectedSubRequest = Request::create('/');
if (Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP)) {
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
}
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));

$strategy = new InlineFragmentRenderer($this->getKernelExpectingRequest($expectedSubRequest));
$request = Request::create('/', 'GET', array(), array(), array(), array('HTTP_IF_MODIFIED_SINCE' => 'Fri, 01 Jan 2016 00:00:00 GMT', 'HTTP_IF_NONE_MATCH' => '*'));
Expand All @@ -208,12 +213,9 @@ public function testFirstTrustedProxyIsSetAsRemote()
{
$expectedSubRequest = Request::create('/');
$expectedSubRequest->headers->set('Surrogate-Capability', 'abc="ESI/1.0"');
$expectedSubRequest->server->set('REMOTE_ADDR', '1.1.1.1');

if (Request::getTrustedHeaderName(Request::HEADER_CLIENT_IP)) {
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
}
$expectedSubRequest->server->set('REMOTE_ADDR', '127.0.0.1');
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));

Request::setTrustedProxies(array('1.1.1.1'));

Expand All @@ -230,9 +232,9 @@ public function testIpAddressOfRangedTrustedProxyIsSetAsRemote()
{
$expectedSubRequest = Request::create('/');
$expectedSubRequest->headers->set('Surrogate-Capability', 'abc="ESI/1.0"');
$expectedSubRequest->server->set('REMOTE_ADDR', '1.1.1.1');
$expectedSubRequest->server->set('REMOTE_ADDR', '127.0.0.1');
$expectedSubRequest->headers->set('x-forwarded-for', array('127.0.0.1'));
$expectedSubRequest->server->set('HTTP_X_FORWARDED_FOR', '127.0.0.1');
$expectedSubRequest->headers->set('forwarded', array('for="127.0.0.1";host="localhost";proto=http'));

Request::setTrustedProxies(array('1.1.1.1/24'));

Expand Down
54 changes: 30 additions & 24 deletions src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php
Expand Up @@ -1303,66 +1303,72 @@ public function testClientIpIsAlwaysLocalhostForForwardedRequests()
$this->setNextResponse();
$this->request('GET', '/', array('REMOTE_ADDR' => '10.0.0.1'));

$this->assertEquals('127.0.0.1', $this->kernel->getBackendRequest()->server->get('REMOTE_ADDR'));
$that = $this;
$this->kernel->assert(function ($backendRequest) use ($that) {
$that->assertSame('127.0.0.1', $backendRequest->server->get('REMOTE_ADDR'));
});
}

/**
* @dataProvider getTrustedProxyData
*/
public function testHttpCacheIsSetAsATrustedProxy(array $existing, array $expected)
public function testHttpCacheIsSetAsATrustedProxy(array $existing)
{
Request::setTrustedProxies($existing);

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

$this->assertEquals($expected, Request::getTrustedProxies());
$that = $this;
$existing = array_unique(array_merge($existing, array('127.0.0.1')));
$this->kernel->assert(function ($backendRequest) use ($existing, $that) {
$that->assertSame($existing, Request::getTrustedProxies());
$that->assertsame('10.0.0.1', $backendRequest->getClientIp());
});

Request::setTrustedProxies(array());
}

public function getTrustedProxyData()
{
return array(
array(array(), array('127.0.0.1')),
array(array('10.0.0.2'), array('10.0.0.2', '127.0.0.1')),
array(array('10.0.0.2', '127.0.0.1'), array('10.0.0.2', '127.0.0.1')),
array(array()),
array(array('10.0.0.2')),
array(array('10.0.0.2', '127.0.0.1')),
);
}

/**
* @dataProvider getXForwardedForData
* @dataProvider getForwardedData
*/
public function testXForwarderForHeaderForForwardedRequests($xForwardedFor, $expected)
public function testForwarderHeaderForForwardedRequests($forwarded, $expected)
{
$this->setNextResponse();
$server = array('REMOTE_ADDR' => '10.0.0.1');
if (false !== $xForwardedFor) {
$server['HTTP_X_FORWARDED_FOR'] = $xForwardedFor;
if (null !== $forwarded) {
Request::setTrustedProxies($server);
$server['HTTP_FORWARDED'] = $forwarded;
}
$this->request('GET', '/', $server);

$this->assertEquals($expected, $this->kernel->getBackendRequest()->headers->get('X-Forwarded-For'));
$that = $this;
$this->kernel->assert(function ($backendRequest) use ($expected, $that) {
$that->assertSame($expected, $backendRequest->headers->get('Forwarded'));
});

Request::setTrustedProxies(array());
}

public function getXForwardedForData()
public function getForwardedData()
{
return array(
array(false, '10.0.0.1'),
array('10.0.0.2', '10.0.0.2, 10.0.0.1'),
array('10.0.0.2, 10.0.0.3', '10.0.0.2, 10.0.0.3, 10.0.0.1'),
array(null, 'for="10.0.0.1";host="localhost";proto=http'),
array('for=10.0.0.2', 'for="10.0.0.2";host="localhost";proto=http, for="10.0.0.1"'),
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"'),
);
}

public function testXForwarderForHeaderForPassRequests()
{
$this->setNextResponse();
$server = array('REMOTE_ADDR' => '10.0.0.1');
$this->request('POST', '/', $server);

$this->assertEquals('10.0.0.1', $this->kernel->getBackendRequest()->headers->get('X-Forwarded-For'));
}

public function testEsiCacheRemoveValidationHeadersIfEmbeddedResponses()
{
$time = \DateTime::createFromFormat('U', time());
Expand Down

0 comments on commit 725dee4

Please sign in to comment.