Skip to content

Commit

Permalink
made HTTP headers coming from proxies non-trusted by default
Browse files Browse the repository at this point in the history
  • Loading branch information
fabpot committed Jul 5, 2011
1 parent cf1714c commit 932cd10
Show file tree
Hide file tree
Showing 6 changed files with 45 additions and 5 deletions.
12 changes: 12 additions & 0 deletions UPDATE.md
Expand Up @@ -6,8 +6,20 @@ one. It only discusses changes that need to be done when using the "public"
API of the framework. If you "hack" the core, you should probably follow the
timeline closely anyway.

RC4 to RC5
----------

* To avoid security issues, HTTP headers coming from proxies are not trusted
anymore by default (like `HTTP_X_FORWARDED_FOR`, `X_FORWARDED_PROTO`, and
`X_FORWARDED_HOST`). If your application is behind a reverse proxy, add the
following configuration:

framework:
proxy: true

RC3 to RC4
----------

* Annotation classes must be annotated with @Annotation
(see the validator constraints for examples)

Expand Down
Expand Up @@ -47,6 +47,7 @@ public function getConfigTreeBuilder()
$rootNode
->children()
->scalarNode('charset')->end()
->scalarNode('proxy')->defaultFalse()->end()
->scalarNode('secret')->isRequired()->end()
->scalarNode('exception_controller')->defaultValue('Symfony\\Bundle\\FrameworkBundle\\Controller\\ExceptionController::showAction')->end()
->scalarNode('ide')->defaultNull()->end()
Expand Down
Expand Up @@ -66,6 +66,8 @@ public function load(array $configs, ContainerBuilder $container)
$container->setParameter('kernel.secret', $config['secret']);
$container->setParameter('exception_listener.controller', $config['exception_controller']);

$container->setParameter('kernel.proxy', $config['proxy']);

if (!empty($config['test'])) {
$loader->load('test.xml');
}
Expand Down
9 changes: 8 additions & 1 deletion src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php
Expand Up @@ -24,7 +24,7 @@
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Compiler\PassConfig;
use Symfony\Component\DependencyInjection\Scope;
use Symfony\Component\HttpFoundation\File\File;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Bundle\Bundle;

/**
Expand All @@ -34,6 +34,13 @@
*/
class FrameworkBundle extends Bundle
{
public function boot()
{
if ($this->container->getParameter('kernel.proxy')) {
Request::trustProxyData();
}
}

public function build(ContainerBuilder $container)
{
parent::build($container);
Expand Down
21 changes: 17 additions & 4 deletions src/Symfony/Component/HttpFoundation/Request.php
Expand Up @@ -20,6 +20,8 @@
*/
class Request
{
static protected $trustProxy = false;

/**
* @var \Symfony\Component\HttpFoundation\ParameterBag
*/
Expand Down Expand Up @@ -322,6 +324,17 @@ public function overrideGlobals()
$_REQUEST = array_merge($_GET, $_POST);
}

/**
* Trusts $_SERVER entries coming from proxies.
*
* You should only call this method if your application
* is hosted behind a reverse proxy that you manage.
*/
static public function trustProxyData()
{
self::$trustProxy = true;
}

/**
* Gets a "parameter" value.
*
Expand Down Expand Up @@ -397,7 +410,7 @@ public function getClientIp($proxy = false)
if ($proxy) {
if ($this->server->has('HTTP_CLIENT_IP')) {
return $this->server->get('HTTP_CLIENT_IP');
} elseif ($this->server->has('HTTP_X_FORWARDED_FOR')) {
} elseif (self::$trustProxy && $this->server->has('HTTP_X_FORWARDED_FOR')) {
return $this->server->get('HTTP_X_FORWARDED_FOR');
}
}
Expand Down Expand Up @@ -600,9 +613,9 @@ public function isSecure()
return (
(strtolower($this->server->get('HTTPS')) == 'on' || $this->server->get('HTTPS') == 1)
||
(strtolower($this->headers->get('SSL_HTTPS')) == 'on' || $this->headers->get('SSL_HTTPS') == 1)
(self::$trustProxy && strtolower($this->headers->get('SSL_HTTPS')) == 'on' || $this->headers->get('SSL_HTTPS') == 1)
||
(strtolower($this->headers->get('X_FORWARDED_PROTO')) == 'https')
(self::$trustProxy && strtolower($this->headers->get('X_FORWARDED_PROTO')) == 'https')
);
}

Expand All @@ -613,7 +626,7 @@ public function isSecure()
*/
public function getHost()
{
if ($host = $this->headers->get('X_FORWARDED_HOST')) {
if (self::$trustProxy && $host = $this->headers->get('X_FORWARDED_HOST')) {
$elements = explode(',', $host);

$host = trim($elements[count($elements) - 1]);
Expand Down
5 changes: 5 additions & 0 deletions tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php
Expand Up @@ -21,6 +21,11 @@

class RequestTest extends \PHPUnit_Framework_TestCase
{
public function setUp()
{
Request::trustProxyData();
}

/**
* @covers Symfony\Component\HttpFoundation\Request::__construct
*/
Expand Down

3 comments on commit 932cd10

@Seldaek
Copy link
Member

@Seldaek Seldaek commented on 932cd10 Jul 6, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good change, but I think the name proxy isn't explicit enough for something that has security relevance. allow_proxy_headers or trust_proxy_headers would maybe be more explicit as to the weight that value carries.

@fabpot
Copy link
Member Author

@fabpot fabpot commented on 932cd10 Jul 6, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@staabm
Copy link
Contributor

@staabm staabm commented on 932cd10 Oct 7, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this change also affect the "in app reverse proxy" (httpCache) which is built into symfony, or only a "real" proxy?

Please sign in to comment.