8000 [HttpFoundation] [Request] Throw dedicated exceptions. by SpacePossum · Pull Request #20389 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

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

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

Closed
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<?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\HttpFoundation\Exception;

/**
* The HTTP request contains incorrect host data.
*
* @internal
*
* @author SpacePossum
*/
abstract class AbstractHostException extends \UnexpectedValueException implements ExceptionInterface
{
private $host;

/**
* @param string $host
* @param string $message
*/
public function __construct($host, $message)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

{
parent::__construct($message);
$this->host = $host;
}

public function getHost()
{
return $this->host;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@
*
* @author Magnus Nordlander <magnus@fervo.se>
*/
class ConflictingHeadersException extends \RuntimeException
class ConflictingHeadersException extends \UnexpectedValueException implements ExceptionInterface
{
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?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\HttpFoundation\Exception;

/**
* Base ExceptionInterface for the Request component.
*
* @author SpacePossum
*/
interface ExceptionInterface
{
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?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\HttpFoundation\Exception;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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


/**
* The HTTP request contains invalid host data.
*
* @author SpacePossum
*/
final class InvalidHostException extends AbstractHostException
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one should be internal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I've updated that class.

{
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?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\HttpFoundation\Exception;

/**
* The HTTP request contains host data which is not trusted.
*
* @author SpacePossum
*/
final class UntrustedHostException extends AbstractHostException
{
}
22 changes: 16 additions & 6 deletions src/Symfony/Component/HttpFoundation/Request.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
namespace Symfony\Component\HttpFoundation;

use Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException;
use Symfony\Component\HttpFoundation\Exception\InvalidHostException;
use Symfony\Component\HttpFoundation\Exception\UntrustedHostException;
use Symfony\Component\HttpFoundation\Session\SessionInterface;

/**
Expand Down Expand Up @@ -788,6 +790,8 @@ public function setSession(SessionInterface $session)
*
* @return array The client IP addresses
*
* @throws ConflictingHeadersException
*
* @see getClientIp()
*/
public function getClientIps()
Expand Down Expand Up @@ -819,7 +823,7 @@ public function getClientIps()
}

if ($hasTrustedForwardedHeader && $hasTrustedClientIpHeader && $forwardedClientIps !== $xForwardedForClientIps) {
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.');
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.');
}

if (!$hasTrustedForwardedHeader && !$hasTrustedClientIpHeader) {
Expand Down Expand Up @@ -1198,7 +1202,8 @@ public function isSecure()
*
* @return string
*
* @throws \UnexpectedValueException when the host name is invalid
* @throws InvalidHostException when the host name is invalid
* @throws UntrustedHostException when the host is not trusted
*/
public function getHost()
{
Expand All @@ -1220,7 +1225,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));
}

if (count(self::$trustedHostPatterns) > 0) {
Expand All @@ -1238,7 +1243,7 @@ public function getHost()
}
}

throw new \UnexpectedValueException(sprintf('Untrusted Host "%s"', $host));
throw new UntrustedHostException($host, sprintf('Untrusted Host "%s".', $host));
}

return $host;
Expand Down Expand Up @@ -1515,7 +1520,7 @@ public function getContent($asResource = false)
{
$currentContentIsResource = is_resource($this->content);
if (PHP_VERSION_ID < 50600 && false === $this->content) {
throw new \LogicException('getContent() can only be called once when using the resource return type and PHP below 5.6.');
throw new \LogicException(sprintf('Method %s can only be called once when using the resource return type and PHP below 5.6.', __METHOD__));
}

if (true === $asResource) {
Expand Down Expand Up @@ -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 \LogicException(
sprintf(
'The Request factory must return an instance of %s. Got %s.',
self::class, is_object($request) ? get_class($request) : (null === $request ? 'null' : gettype($request))
)
);
}

return $request;
Expand Down
8 changes: 5 additions & 3 deletions src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

namespace Symfony\Component\HttpFoundation\Tests;

use Symfony\Component\HttpFoundation\Exception\InvalidHostException;
use Symfony\Component\HttpFoundation\Exception\UntrustedHostException;
use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage;
use Symfony\Component\HttpFoundation\Session\Session;
use Symfony\Component\HttpFoundation\Request;
Expand Down Expand Up @@ -1872,8 +1874,8 @@ public function testTrustedHosts()
try {
$request->getHost();
$this->fail('Request::getHost() should throw an exception when host is not trusted.');
} catch (\UnexpectedValueException $e) {
$this->assertEquals('Untrusted Host "evil.com"', $e->getMessage());
} catch (UntrustedHostException $e) {
$this->assertEquals('Untrusted Host "evil.com".', $e->getMessage());
}

// trusted hosts
Expand Down Expand Up @@ -1936,7 +1938,7 @@ public function testHostValidity($host, $isValid, $expectedHost = null, $expecte
$this->assertSame($expectedPort, $request->getPort());
}
} else {
$this->setExpectedException('UnexpectedValueException', 'Invalid Host');
$this->setExpectedException(InvalidHostException::class, 'Invalid Host');
$request->getHost();
}
}
Expand Down
0