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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f00b55
[HttpKernel] Move conflicting origin IPs handling to catch block
nicolas-grekas Jun 29, 2016
db84101
[HttpKernel] Add listener that checks when request has both Forwarded…
magnusnordlander May 2, 2016
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
Next Next commit
[HttpKernel] Move conflicting origin IPs handling to catch block
  • Loading branch information
@nicolas-grekas
nicolas-grekas committed Jun 30, 2016
commit 1f00b553735f47f7614d98968cfdf2eb2f9c0b48
10 changes: 3 additions & 7 deletions 10 src/Symfony/Component/HttpKernel/HttpKernel.php
View file
Open in desktop
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 •
edited
Loading

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
18 changes: 6 additions & 12 deletions 18 src/Symfony/Component/HttpKernel/Tests/HttpKernelTest.php
View file
Open in desktop
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.