[HttpFoundation] [Request] Throw dedicated exceptions. #20389

SpacePossum · 2016-11-02T14:20:03Z

Q	A
Branch?	master (but the lower the better ;) )
Bug fix?	no
New feature?	yes
BC breaks?	not really
Deprecations?	no
Tests pass?	yes
License	MIT

I'm trying to deal with invalid requests being sent to one of my sites.
While this is a Silex site I think this PR will help dealing with such requests and will be useful for SF developers as well.

The problem is that sometimes a request is send with host data like "my.test.com/".
The trailing slash will cause the Request class to throw an exception when asking for the host (for example through getHost()).

In order to deal with these kind of errors I setup something like:

// bootstrap.php

$app = new Application(); // this is a Silex app

// more bootstrap stuff

$app->register(new MonologServiceProvider(), [/**/]);
$app['monolog.listener'] = function () use ($app) {
    return new class($app['logger'], $app['folders.log']) extends LogListener {
        protected function logException(\Exception $e)
        {
            if ($e instanceof HttpExceptionInterface) {
                if ($e->getStatusCode() >= 500) { // don't log 400 errors
                    $this->logger->critical($message, ['exception' => $e]);
                }
            } else {
                // FIXME deal with host problem here
                $this->logger->critical($message, ['exception' => $e]);
            }		
        }
    }
};

$app->error(function (\Exception $e, Request $request, $code = null) use ($app) {
    // FIXME deal with host problem here
}

// more bootstrap stuff

$app->run();

Dealing with the exception at these points is hard because the Request class throws an SPL \UnexpectedValueException exception.
At the point of handling it is hard to see this exception comes from the Request class and not from somewhere else in the system (for example a controller dealing with the Request object).

It is not possible to deal with exception within the controllers and/or other part of the application on user level.
This is because the exception is thrown before the Request is given to a controller.

Example trace:

#0 /var/www/digital-shop-intermediate-site/vendor/symfony/http-foundation/Request.php(1032): Symfony\Component\HttpFoundation\Request->getHost()
#1 /var/www/digital-shop-intermediate-site/vendor/symfony/http-foundation/Request.php(1062): Symfony\Component\HttpFoundation\Request->getHttpHost()
#2 /var/www/digital-shop-intermediate-site/vendor/symfony/http-foundation/Request.php(1716): Symfony\Component\HttpFoundation\Request->getSchemeAndHttpHost()
#3 /var/www/digital-shop-intermediate-site/vendor/symfony/http-foundation/Request.php(1046): Symfony\Component\HttpFoundation\Request->prepareRequestUri()
#4 /var/www/digital-shop-intermediate-site/vendor/symfony/http-foundation/Request.php(1768): Symfony\Component\HttpFoundation\Request->getRequestUri()
#5 /var/www/digital-shop-intermediate-site/vendor/symfony/http-foundation/Request.php(924): Symfony\Component\HttpFoundation\Request->prepareBaseUrl()
#6 /var/www/digital-shop-intermediate-site/vendor/symfony/routing/RequestContext.php(73): Symfony\Component\HttpFoundation\Request->getBaseUrl()
#7 /var/www/digital-shop-intermediate-site/vendor/symfony/http-kernel/EventListener/RouterListener.php(71): Symfony\Component\Routing\RequestContext->fromRequest(Object(Symfony\Component\HttpFoundation\Request))
#8 /var/www/digital-shop-intermediate-site/vendor/symfony/http-kernel/EventListener/RouterListener.php(90): Symfony\Component\HttpKernel\EventListener\RouterListener->setCurrentRequest(Object(Symfony\Component\HttpFoundation\Request))
#9 [internal function]: Symfony\Component\HttpKernel\EventListener\RouterListener->onKernelRequest(Object(Symfony\Component\HttpKernel\Event\GetResponseEvent), 'kernel.request', Object(Symfony\Component\EventDispatcher\EventDispatcher))
#10 /var/www/digital-shop-intermediate-site/vendor/symfony/event-dispatcher/EventDispatcher.php(174): call_user_func(Array, Object(Symfony\Component\HttpKernel\Event\GetResponseEvent), 'kernel.request', Object(Symfony\Component\EventDispatcher\EventDispatcher))
#11 /var/www/digital-shop-intermediate-site/vendor/symfony/event-dispatcher/EventDispatcher.php(43): Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(Array, 'kernel.request', Object(Symfony\Component\HttpKernel\Event\GetResponseEvent))
#12 /var/www/digital-shop-intermediate-site/vendor/symfony/http-kernel/HttpKernel.php(129): Symfony\Component\EventDispatcher\EventDispatcher->dispatch('kernel.request', Object(Symfony\Component\HttpKernel\Event\GetResponseEvent))
#13 /var/www/digital-shop-intermediate-site/vendor/symfony/http-kernel/HttpKernel.php(68): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#14 /var/www/digital-shop-intermediate-site/vendor/silex/silex/src/Silex/Application.php(496): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#15 /var/www/digital-shop-intermediate-site/vendor/silex/silex/src/Silex/Application.php(477): Silex\Application->handle(Object(Symfony\Component\HttpFoundation\Request))
#16 /var/www/digital-shop-intermediate-site/web/index.php(13): Silex\Application->run()

It will not only be a lot of work modifying all components dealing with the host of a Request (either by getHost or getHttpHost) before the Request is routed to a controller, it will also be hard to make sure when new components are added (or existing modified) deal with the exception correctly.

Therefor I propose to let the Request class throw exceptions of meaningful (/dedicated) classes. That way people can deal with these exception as they want.

GuilhemN · 2016-11-03T18:10:04Z

src/Symfony/Component/HttpFoundation/Exception/InvalidHostException.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpFoundation\Exception;


In other components, exceptions implement an interface specific to the component.

Which interface would be appropriate in this case?
For example currently the class throws exceptions of the class ConflictingHeadersException but that doesn't implement any interface.

There was only one exception but if the component contains more exceptions, they should implement an ExceptionInterface.

SpacePossum · 2016-11-04T09:03:23Z

reworked, @Ener-Getick please have a look :)

chalasr · 2016-11-04T09:10:49Z

src/Symfony/Component/HttpFoundation/Request.php

@@ -1220,7 +1226,7 @@ public function getHost()
        // check that it does not contain forbidden characters (see RFC 952 and RFC 2181)
        // use preg_replace() instead of preg_match() to prevent DoS attacks with long host names
        if ($host && '' !== preg_replace('/(?:^\[)?[a-zA-Z0-9-:\]_]+\.?/', '', $host)) {
-            throw new \UnexpectedValueException(sprintf('Invalid Host "%s"', $host));
+            throw new InvalidHostException($host, sprintf('Invalid host "%s".', $host));


Couldn't we just pass the message as it contains the host, ~~such as done for UntrustedHostException~~?

I would like to get the host that triggered the exception to be thrown when I'm dealing with the exception. Relying on the message (format) of an exception is never a good idea IMHO.

GuilhemN · 2016-11-04T19:35:09Z

src/Symfony/Component/HttpFoundation/Request.php

     */
    public static function getTrustedHeaderName($key)
    {
        if (!array_key_exists($key, self::$trustedHeaders)) {
-            throw new \InvalidArgumentException(sprintf('Unable to get the trusted header name for key "%s".', $key));
+            throw new InvalidTrustedHeaderException($key, null, sprintf('Unable to get the trusted header name for key "%s".', $key));


Isn't an InvalidArgumentException localized in the component enough and more appropriate?

Happy to revert :)

GuilhemN · 2016-11-04T19:41:15Z

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

@@ -1936,7 +1937,7 @@ public function testHostValidity($host, $isValid, $expectedHost = null, $expecte
                $this->assertSame($expectedPort, $request->getPort());
            }
        } else {
-            $this->setExpectedException('UnexpectedValueException', 'Invalid Host');
+            $this->setExpectedException('Symfony\Component\HttpFoundation\Exception\InvalidHostException', 'Invalid host');


You can use the short syntax here Exception::class

ah SF upped requirements to PHP 5.5, good point will update

GuilhemN · 2016-11-04T19:42:44Z

src/Symfony/Component/HttpFoundation/Exception/InvalidHostException.php

+ *
+ * @author SpacePossum
+ */
+final class InvalidHostException extends AbstractHostException


I think this one should be internal.

👍 agreed

On second thought, I don't think so, people want to deal with this exception by catching it. Marking it as internal will cause IDE's to complain about using an internal class.

Indeed sorry I didn't commented on the right class... I was thinking about AbstractHostException.

👍 I've updated that class.

ro0NL · 2016-11-05T08:37:08Z

src/Symfony/Component/HttpFoundation/Exception/AbstractHostException.php

+     * @param string $host
+     * @param string $message
+     */
+    public function __construct($host, $message)


Why not making $message optional (if null, default to Invalid host "'.$host.'"), so you can simply throw new InvalidHostException($host).

Making it null able will break with the design of the base exception class itself, which is IIRC something people don't want in the SF project.
Besides there is no way this class can generate a useful/correct message based on the value of $host alone, the logic that figured out the $host value is incorrect and throws the exception knows what is wrong and can therefor set a meaningful message only.

ro0NL · 2016-11-05T08:37:51Z

src/Symfony/Component/HttpFoundation/Exception/InvalidTrustedHeaderException.php

+     * @param string|null $value
+     * @param string      $message
+     */
+    public function __construct($header, $value, $message)


Same.. make message optional?

ro0NL · 2016-11-05T08:38:58Z

src/Symfony/Component/HttpFoundation/Exception/InvalidTrustedHeaderException.php

+    /**
+     * @var string
+     */
+    private $value;


Having getHeader() / getValue() would be nice? Right now these are basically useless.

thank, indeed, however as opt here I will remove the class all together.

ro0NL · 2016-11-05T09:27:04Z

src/Symfony/Component/HttpFoundation/Exception/AbstractHostException.php

+abstract class AbstractHostException extends \UnexpectedValueException implements ExceptionInterface
+{
+    /**
+     * @var string


I believe unnecessary docs can be removed.. same for the getter.

these do no harm and will become of use when doing SCA, refactoring and/or updating to PHP 7 when the PHPDocs can be used to generate the type hints on method signatures.

They are useless, they can be inferred from constructor's docs and most tools support this.

hhamon · 2016-11-10T08:50:53Z

If this PR is accepted, it will only go into Symfony 4.0 only for BC reasons I suppose. You should also trigger some deprecation errors before every new exception types is thrown.

ogizanagi · 2016-11-10T09:00:44Z

@hhamon : Why ? There is no BC break, as the new exceptions extends \UnexpectedValueException which was thrown before, right ?

ogizanagi · 2016-11-10T09:03:43Z

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

-        } catch (\UnexpectedValueException $e) {
-            $this->assertEquals('Untrusted Host "evil.com"', $e->getMessage());
+        } catch (UntrustedHostException $e) {
+            $this->assertEquals('Untrusted host "evil.com".', $e->getMessage());


I think host was capitalized referencing the Host header. I'm not sure this has to be changed (same for other occurrences).

alright 👍 updated back to Host

SpacePossum · 2016-11-10T09:40:13Z

If this PR is accepted, it will only go into Symfony 4.0 only for BC reasons I suppose.

I agree with @ogizanagi here, there is no real BC break. The only way this can break is people rely on the exception being an instance of \UnexpectedValueException but not inherits from it.

You should also trigger some deprecation errors before every new exception types is thrown.

no, the logic of the exception being thrown is not deprecated and so it should not trigger such warnings.

dunglas · 2016-11-10T15:45:06Z

👍

SpacePossum · 2016-11-16T11:40:54Z

@dunglas I've removed the PHPDoc's as requested, anything else you want me to change to get the PR into the status that is ready and marked as reviewed?

dunglas · 2016-11-16T15:45:55Z

It's ok for me, you now need to wait that at least another core team member review it and agree to merge it.

Status: reviewed

xabbuh · 2016-11-16T19:26:37Z

👍 for me (but for 3.3)

ro0NL · 2016-11-16T19:34:08Z

src/Symfony/Component/HttpFoundation/Request.php

@@ -1940,7 +1945,12 @@ private static function createRequestFromFactory(array $query = array(), array $
            $request = call_user_func(self::$requestFactory, $query, $request, $attributes, $cookies, $files, $server, $content);

            if (!$request instanceof self) {
-                throw new \LogicException('The Request factory must return an instance of Symfony\Component\HttpFoundation\Request.');
+                throw new \UnexpectedValueException(


BC break :) although this is the better exception :(

reverted, I don't care to much about that one anyway so ;)

SpacePossum · 2016-11-25T09:32:28Z

but for 3.3

does this mean I've to retarget this PR?

fabpot

Closing in favor of #20962

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

carsonbot added Status: Needs Review HttpFoundation Feature labels Nov 2, 2016

GuilhemN reviewed Nov 3, 2016

View reviewed changes

Request: throw dedicted exceptions.

9807f06

SpacePossum force-pushed the master_Request_dedicated_exceptions branch from 8b34ded to 9807f06 Compare November 4, 2016 08:58

Update Request.php

45e439f

chalasr reviewed Nov 4, 2016

View reviewed changes

GuilhemN reviewed Nov 4, 2016

View reviewed changes

ro0NL reviewed Nov 5, 2016

View reviewed changes

process feedbacl

e750da0

ro0NL reviewed Nov 5, 2016

View reviewed changes

Update Request.php

9ca7de7

ogizanagi reviewed Nov 10, 2016

View reviewed changes

SpacePossum added 2 commits November 10, 2016 10:46

Update RequestTest.php

b99a3f9

Update Request.php

183d59d

SpacePossum added 2 commits November 11, 2016 10:21

Update AbstractHostException.php

c25ff57

Update AbstractHostException.php

c721ecc

carsonbot added Status: Reviewed and removed Status: Needs Review labels Nov 16, 2016

ro0NL reviewed Nov 16, 2016

View reviewed changes

Update Request.php

8c9dbb8

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

fabpot mentioned this pull request Dec 16, 2016

Request exceptions #20962

Merged

fabpot closed this Dec 16, 2016

SpacePossum deleted the master_Request_dedicated_exceptions branch December 16, 2016 16:30

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

Uh oh!

[HttpFoundation] [Request] Throw dedicated exceptions. #20389

[HttpFoundation] [Request] Throw dedicated exceptions. #20389

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!