Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[HttpKernel] Move handling of conflicting origin IPs to catch block #19233

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 30, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions 4 src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,9 @@
<argument type="service" id="request_stack" />
<tag name="kernel.event_subscriber" />
</service>

<service id="validate_request_listener" class="Symfony\Component\HttpKernel\EventListener\ValidateRequestListener">
<tag name="kernel.event_subscriber" />
</service>
</services>
</container>
4 changes: 2 additions & 2 deletions 4 src/Symfony/Bundle/FrameworkBundle/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@
"symfony/config": "~2.4",
"symfony/event-dispatcher": "~2.5",
"symfony/finder": "~2.0,>=2.0.5",
"symfony/http-foundation": "~2.4.9|~2.5,>=2.5.4",
"symfony/http-kernel": "~2.7",
"symfony/http-foundation": "~2.7",
"symfony/http-kernel": "~2.7.15|~2.8.8",
"symfony/filesystem": "~2.3",
"symfony/routing": "~2.6,>2.6.4",
"symfony/security-core": "~2.6.13|~2.7.9|~2.8",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<?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\EventListener;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\GetResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;

/**
* Validates that the headers and other information indicating the
* client IP address of a request are consistent.
*
* @author Magnus Nordlander <magnus@fervo.se>
*/
class ValidateRequestListener implements EventSubscriberInterface
{
/**
* Performs the validation.
*
* @param GetResponseEvent $event
*/
public function onKernelRequest(GetResponseEvent $event)
{
if (!$event->isMasterRequest()) {
return;
}
$request = $event->getRequest();

if ($request::getTrustedProxies()) {
// This will throw an exception if the headers are inconsistent.
$request->getClientIps();
}
}

/**
* {@inheritdoc}
*/
public static function getSubscribedEvents()
{
return array(
KernelEvents::REQUEST => array(
array('onKernelRequest', 256),
),
);
}
}
10 changes: 3 additions & 7 deletions 10 src/Symfony/Component/HttpKernel/HttpKernel.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
try {
return $this->handleRaw($request, $type);
} catch (\Exception $e) {
if ($e instanceof ConflictingHeadersException) {
Copy link
Member Author

@nicolas-grekas nicolas-grekas Jun 29, 2016

Choose a reason for hiding this comment

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

To me, this belongs to HttpKernel: it's a low level HttpFoundation exception (the only one), and HttpKernel job's is to map HttpFoundation to event's world, and deal with any kind of events, exceptions included. It's HttpKernel's responsibility to convert HttpFoundation exceptions to HttpKernel's world.
It's not optional at all, and existing HttpKernel users (e.g. Drupal) need to get a BadRequestHttpException that they already know how do deal with. Not this ConflictingHeadersException that comes from lower levels.

The listener below is a nice to have, that adds an early check in the request management process. Totally optional and a smooth feature for the FrameworkBundle.

Thus the split I made here.
That's only my humble opinion and maybe I miss the thing, so I'll move to the listener if you're sure it's the right place.

Copy link
Member

Choose a reason for hiding this comment

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

That's indeed much better now. 👍

$e = new BadRequestHttpException('The request headers contain conflicting information regarding the origin of this request.', $e);
}
if (false === $catch) {
$this->finishRequest($request, $type);

Expand Down Expand Up @@ -115,13 +118,6 @@ public function terminateWithException(\Exception $exception)
*/
private function handleRaw(Request $request, $type = self::MASTER_REQUEST)
{
if (self::MASTER_REQUEST === $type && $request::getTrustedProxies()) {
try {
$request->getClientIps();
} catch (ConflictingHeadersException $e) {
throw new BadRequestHttpException('The request headers contain conflicting information regarding the origin of this request.', $e);
}
}
$this->requestStack->push($request);

// request
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<?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\Tests\EventListener;

use Symfony\Component\EventDispatcher\EventDispatcher;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\EventListener\ValidateRequestListener;
use Symfony\Component\HttpKernel\Event\GetResponseEvent;
use Symfony\Component\HttpKernel\HttpKernelInterface;
use Symfony\Component\HttpKernel\KernelEvents;

class ValidateRequestListenerTest extends \PHPUnit_Framework_TestCase
Copy link
Member Author

Choose a reason for hiding this comment

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

Btw, this could be moved as a feature on master now.

{
/**
* @expectedException Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException
*/
public function testListenerThrowsWhenMasterRequestHasInconsistentClientIps()
{
$dispatcher = new EventDispatcher();
$kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface');

$request = new Request();
$request->setTrustedProxies(array('1.1.1.1'));
$request->server->set('REMOTE_ADDR', '1.1.1.1');
$request->headers->set('FORWARDED', '2.2.2.2');
$request->headers->set('X_FORWARDED_FOR', '3.3.3.3');

$dispatcher->addListener(KernelEvents::REQUEST, array(new ValidateRequestListener(), 'onKernelRequest'));
$event = new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST);

$dispatcher->dispatch(KernelEvents::REQUEST, $event);
}
}
18 changes: 6 additions & 12 deletions 18 src/Symfony/Component/HttpKernel/Tests/HttpKernelTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -276,26 +276,20 @@ public function testVerifyRequestStackPushPopDuringHandle()
*/
public function testInconsistentClientIpsOnMasterRequests()
{
$kernel = new HttpKernel(new EventDispatcher(), $this->getResolver());
$request = new Request();
$request->setTrustedProxies(array('1.1.1.1'));
$request->server->set('REMOTE_ADDR', '1.1.1.1');
$request->headers->set('FORWARDED', '2.2.2.2');
$request->headers->set('X_FORWARDED_FOR', '3.3.3.3');
$dispatcher = new EventDispatcher();
$dispatcher->addListener(KernelEvents::REQUEST, function ($event) {
$event->getRequest()->getClientIp();
});

$kernel->handle($request, $kernel::MASTER_REQUEST, false);
}
$kernel = new HttpKernel($dispatcher, $this->getResolver());

public function testInconsistentClientIpsOnSubRequests()
{
$kernel = new HttpKernel(new EventDispatcher(), $this->getResolver());
$request = new Request();
$request->setTrustedProxies(array('1.1.1.1'));
$request->server->set('REMOTE_ADDR', '1.1.1.1');
$request->headers->set('FORWARDED', '2.2.2.2');
$request->headers->set('X_FORWARDED_FOR', '3.3.3.3');

$this->assertInstanceOf('Symfony\Component\HttpFoundation\Response', $kernel->handle($request, $kernel::SUB_REQUEST, false));
$kernel->handle($request, $kernel::MASTER_REQUEST, false);
}

protected function getResolver($controller = null)
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.