8000 feature #20962 Request exceptions (thewilkybarkid, fabpot) · symfony/symfony@4009280 · GitHub
[go: up one dir, main page]
Skip to content

Commit 4009280

Browse files
fabpotfabpot
committed
feature #20962 Request exceptions (thewilkybarkid, fabpot)
This PR was merged into the 3.3-dev branch. Discussion ---------- Request exceptions | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20389, #20615, #20662 | License | MIT | Doc PR | n/a Replaces #20389 and #20662. The idea is to generically manage 400 responses when an exception implements `RequestExceptionInterface`. The "weird" caches on the request for the host and the clients IPs allows to correctly manage exceptions in an exception listener/controller (as we are duplicating the request there, but we don't want to throw an exception there). Commits ------- 32ec288 [HttpFoundation] refactored Request exceptions d876809 Return a 400 response for suspicious operations
2 parents db9a008 + 32ec288 commit 4009280

File tree

11 files changed

+115
-19
lines changed

11 files changed

+115
-19
lines changed

‎src/Symfony/Component/Debug/Exception/FlattenException.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
namespace Symfony\Component\Debug\Exception;
1313

14+
use Symfony\Component\HttpFoundation\Exception\RequestExceptionInterface;
1415
use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
1516

1617
/**
@@ -41,6 +42,8 @@ public static function create(\Exception $exception, $statusCode = null, array $
4142
if ($exception instanceof HttpExceptionInterface) {
4243
$statusCode = $exception->getStatusCode();
4344
$headers = array_merge($headers, $exception->getHeaders());
45+
} elseif ($exception instanceof RequestExceptionInterface) {
46+
$statusCode = 400;
4447
}
4548

4649
if (null === $statusCode) {

‎src/Symfony/Component/Debug/Tests/Exception/FlattenExceptionTest.php

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace Symfony\Component\Debug\Tests\Exception;
1313

1414
use Symfony\Component\Debug\Exception\FlattenException;
15+
use Symfony\Component\HttpFoundation\Exception\SuspiciousOperationException;
1516
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
1617
use Symfony\Component\HttpKernel\Exception\MethodNotAllowedHttpException;
1718
use Symfony\Component\HttpKernel\Exception\UnauthorizedHttpException;
@@ -78,6 +79,11 @@ public function testStatusCode()
7879

7980
$flattened = FlattenException::create(new UnsupportedMediaTypeHttpException());
8081
$this->assertEquals('415', $flattened->getStatusCode());
82+
83+
if (class_exists(SuspiciousOperationException::class)) {
84+
$flattened = FlattenException::create(new SuspiciousOperationException());
85+
$this->assertEquals('400', $flattened->getStatusCode());
86+
}
8187
}
8288

8389
public function testHeadersForHttpException()

‎src/Symfony/Component/HttpFoundation/Exception/ConflictingHeadersException.php

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,8 @@
1414
/**
1515
* The HTTP request contains headers with conflicting information.
1616
*
17-
* This exception should trigger an HTTP 400 response in your application code.
18-
*
1917
* @author Magnus Nordlander <magnus@fervo.se>
2018
*/
21-
class ConflictingHeadersException extends \RuntimeException
19+
class ConflictingHeadersException extends \UnexpectedValueException implements RequestExceptionInterface
2220
{
2321
}

‎src/Symfony/Component/HttpFoundation/Exception/RequestExceptionInterface.php

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
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\HttpFoundation\Exception;
13+
14+
/**
15+
* Interface for Request exceptions.
16+
*
17+
* Exceptions implementing this interface should trigger an HTTP 400 response in the application code.
18+
*/
19+
interface RequestExceptionInterface
20+
{
21+
}

‎src/Symfony/Component/HttpFoundation/Exception/SuspiciousOperationException.php

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
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\HttpFoundation\Exception;
13+
14+
/**
15+
* Raised when a user has performed an operation that should be considered
16+
* suspicious from a security perspective.
17+
*/
18+
class SuspiciousOperationException extends \UnexpectedValueException implements RequestExceptionInterface
19+
{
20+
}

‎src/Symfony/Component/HttpFoundation/Request.php

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace Symfony\Component\HttpFoundation;
1313

1414
use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException;
15+
use Symfony\Component\HttpFoundation\Exception\SuspiciousOperationException;
1516
use Symfony\Component\HttpFoundation\Session\SessionInterface;
1617

1718
/**
@@ -206,6 +207,9 @@ class Request
206207

207208
protected static $requestFactory;
208209

210+
private $isHostValid = true;
211+
private $isClientIpsValid = true;
212+
209213
/**
210214
* Constructor.
211215
*
@@ -819,7 +823,12 @@ public function getClientIps()
819823
}
820824

821825
if ($hasTrustedForwardedHeader && $hasTrustedClientIpHeader && $forwardedClientIps !== $xForwardedForClientIps) {
822-
throw new ConflictingHeadersException('The request has both a trusted Forwarded header and a trusted Client IP header, conflicting with each other with regards to the originating IP addresses of the request. This is the result of a misconfiguration. You should either configure your proxy only to send one of these headers, or configure Symfony to distrust one of them.');
826+
if (!$this->isClientIpsValid) {
827+
return array('0.0.0.0');
828+
}
829+
$this->isClientIpsValid = false;
830+
831+
throw new ConflictingHeadersException('The request has both a trusted Forwarded header and a trusted Client IP header, conflicting with each other with regards to the originating IP addresses of the request. This is the result of a misconfiguration. You should either configure your proxy only to send one of these headers, or configure your project to distrust one of them.');
823832
}
824833

825834
if (!$hasTrustedForwardedHeader && !$hasTrustedClientIpHeader) {
@@ -1198,7 +1207,7 @@ public function isSecure()
11981207
*
11991208
* @return string
12001209
*
1201-
* @throws \UnexpectedValueException when the host name is invalid
1210+
* @throws SuspiciousOperationException when the host name is invalid or not trusted
12021211
*/
12031212
public function getHost()
12041213
{
@@ -1220,7 +1229,12 @@ public function getHost()
12201229
// check that it does not contain forbidden characters (see RFC 952 and RFC 2181)
12211230
// use preg_replace() instead of preg_match() to prevent DoS attacks with long host names
12221231
if ($host && '' !== preg_replace('/(?:^\[)?[a-zA-Z0-9-:\]_]+\.?/', '', $host)) {
1223-
throw new \UnexpectedValueException(sprintf('Invalid Host "%s"', $host));
1232+
if (!$this->isHostValid) {
1233+
return '';
1234+
}
1235+
$this->isHostValid = false;
1236+
1237+
throw new SuspiciousOperationException(sprintf('Invalid Host "%s".', $host));
12241238
}
12251239

12261240
if (count(self::$trustedHostPatterns) > 0) {
@@ -1238,7 +1252,12 @@ public function getHost()
12381252
}
12391253
}
12401254

1241-
throw new \UnexpectedValueException(sprintf('Untrusted Host "%s"', $host));
1255+
if (!$this->isHostValid) {
1256+
return '';
1257+
}
1258+
$this->isHostValid = false;
1259+
1260+
throw new SuspiciousOperationException(sprintf('Untrusted Host "%s".', $host));
12421261
}
12431262

12441263
return $host;

‎src/Symfony/Component/HttpFoundation/Tests/RequestTest.php

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
namespace Symfony\Component\HttpFoundation\Tests;
1313

14+
use Symfony\Component\HttpFoundation\Exception\SuspiciousOperationException;
1415
use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage;
1516
use Symfony\Component\HttpFoundation\Session\Session;
1617
use Symfony\Component\HttpFoundation\Request;
@@ -1871,8 +1872,8 @@ public function testTrustedHosts()
18711872
try {
18721873
$request->getHost();
18731874
$this->fail('Request::getHost() should throw an exception when host is not trusted.');
1874-
} catch (\UnexpectedValueException $e) {
1875-
$this->assertEquals('Untrusted Host "evil.com"', $e->getMessage());
1875+
} catch (SuspiciousOperationException $e) {
1876+
$this->assertEquals('Untrusted Host "evil.com".', $e->getMessage());
18761877
}
18771878

18781879
// trusted hosts
@@ -1935,7 +1936,7 @@ public function testHostValidity($host, $isValid, $expectedHost = null, $expecte
19351936
$this->assertSame($expectedPort, $request->getPort());
19361937
}
19371938
} else {
1938-
$this->setExpectedException('UnexpectedValueException', 'Invalid Host');
1939+
$this->setExpectedException(SuspiciousOperationException::class, 'Invalid Host');
19391940
$request->getHost();
19401941
}
19411942
}

‎src/Symfony/Component/HttpKernel/EventListener/ValidateRequestListener.php

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,7 @@
1616
use Symfony\Component\HttpKernel\KernelEvents;
1717

1818
/**
19-
* Validates that the headers and other information indicating the
20-
* client IP address of a request are consistent.
19+
* Validates Requests.
2120
*
2221
* @author Magnus Nordlander <magnus@fervo.se>
2322
*/
@@ -36,9 +35,10 @@ public function onKernelRequest(GetResponseEvent $event)
3635
$request = $event->getRequest();
3736

3837
if ($request::getTrustedProxies()) {
39-
// This will throw an exception if the headers are inconsistent.
4038
$request->getClientIps();
4139
}
40+
41+
$request->getHost();
4242
}
4343

4444
/**

‎src/Symfony/Component/HttpKernel/HttpKernel.php

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
use Symfony\Component\HttpKernel\Event\GetResponseForControllerResultEvent;
2626
use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent;
2727
use Symfony\Component\HttpKernel\Event\PostResponseEvent;
28-
use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException;
28+
use Symfony\Component\HttpFoundation\Exception\RequestExceptionInterface;
2929
use Symfony\Component\HttpFoundation\Request;
3030
use Symfony\Component\HttpFoundation\RequestStack;
3131
use Symfony\Component\HttpFoundation\Response;
@@ -67,8 +67,8 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
6767
try {
6868
return $this->handleRaw($request, $type);
6969
} catch (\Exception $e) {
70-
if ($e instanceof ConflictingHeadersException) {
71-
$e = new BadRequestHttpException('The request headers contain conflicting information regarding the origin of this request.', $e);
70+
if ($e instanceof RequestExceptionInterface) {
71+
$e = new BadRequestHttpException($e->getMessage(), $e);
7272
}
7373
if (false === $catch) {
7474
$this->finishRequest($request, $type);

‎src/Symfony/Component/HttpKernel/Tests/EventListener/RouterListenerTest.php

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,17 @@
99
* file that was distributed with this source code.
1010
*/
1111

12-
namespace Symfony\Component\HttpKernel\Tests\EventListener;
13-
12+
use Symfony\Component\EventDispatcher\EventDispatcher;
1413
use Symfony\Component\HttpFoundation\Request;
14+
use Symfony\Component\HttpFoundation\RequestStack;
15+
use Symfony\Component\HttpFoundation\Response;
16+
use Symfony\Component\HttpKernel\Controller\ArgumentResolver;
17+
use Symfony\Component\HttpKernel\Controller\ControllerResolver;
18+
use Symfony\Component\HttpKernel\EventListener\ExceptionListener;
1519
use Symfony\Component\HttpKernel\EventListener\RouterListener;
20+
use Symfony\Component\HttpKernel\EventListener\ValidateRequestListener;
1621
use Symfony\Component\HttpKernel\HttpKernelInterface;
22+
use Symfony\Component\HttpKernel\HttpKernel;
1723
use Symfony\Component\HttpKernel\Event\GetResponseEvent;
1824
use Symfony\Component\Routing\RequestContext;
1925

@@ -154,4 +160,26 @@ public function getLoggingParameterData()
154160
array(array(), 'Matched route "{route}".', array('route' => 'n/a', 'route_parameters' => array(), 'request_uri' => 'http://localhost/', 'method' => 'GET')),
155161
);
156162
}
163+
164+
public function testWithBadRequest()
165+
{
166+
$requestStack = new RequestStack();
167+
168+
$requestMatcher = $this->getMock('Symfony\Component\Routing\Matcher\RequestMatcherInterface');
169+
$requestMatcher->expects($this->never())->method('matchRequest');
170+
171+
$dispatcher = new EventDispatcher();
172+
$dispatcher->addSubscriber(new ValidateRequestListener());
173+
$dispatcher->addSubscriber(new RouterListener($requestMatcher, $requestStack, new RequestContext()));
174+
$dispatcher->addSubscriber(new ExceptionListener(function () {
175+
return new Response('Exception handled', 400);
176+
}));
177+
178+
$kernel = new HttpKernel($dispatcher, new ControllerResolver(), $requestStack, new ArgumentResolver());
179+
180+
$request = Request::create('http://localhost/');
181+
$request->headers->set('host', '###');
182+
$response = $kernel->handle($request);
183+
$this->assertSame(400, $response->getStatusCode());
184+
}
157185
}

‎src/Symfony/Component/HttpKernel/composer.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
"require": {
1919
"php": ">=5.5.9",
2020
"symfony/event-dispatcher": "~2.8|~3.0",
21-
"symfony/http-foundation": "~2.8.13|~3.1.6|~3.2",
21+
"symfony/http-foundation": "~3.3",
2222
"symfony/debug": "~2.8|~3.0",
2323
"psr/log": "~1.0"
2424
},

0 commit comments

Comments
 (0)
0