From d20ab50f9d480372a2b6489278f9876d1a85134c Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Sun, 25 Jun 2017 15:12:29 +1200 Subject: [PATCH] API Stronger Injector service unregistration BUG Fix up test regressions FIX director references to request object API Move all middlewares to common namespace API Implement RequestHandlerMiddlewareAdapter ENHANCEMENT Improve IP address parsing Fix up PHPDoc / psr2 linting BUG Fix property parsing in TrustedProxyMiddleware BUG Fix Director::is_https() --- .upgrade.yml | 2 +- _config/requestprocessors.yml | 11 +- _config/security.yml | 4 +- .../02_Controllers/05_Middlewares.md | 52 ++++-- src/Control/ContentNegotiator.php | 4 +- src/Control/Director.php | 164 ++++++++---------- src/Control/HTTPApplication.php | 6 +- src/Control/HTTPRequest.php | 16 +- .../Middleware/AllowedHostsMiddleware.php | 39 +++-- src/Control/Middleware/FlushMiddleware.php | 6 +- src/Control/Middleware/HTTPMiddleware.php | 5 +- .../Middleware/HTTPMiddlewareAware.php | 11 +- .../RequestHandlerMiddlewareAdapter.php | 59 +++++++ src/Control/Middleware/SessionMiddleware.php | 4 +- .../Middleware/TrustedProxyMiddleware.php | 141 ++++++++++----- src/Control/RequestFilter.php | 1 + src/Control/RequestHandler.php | 12 +- src/Control/RequestProcessor.php | 17 +- src/Control/Session.php | 13 +- src/Core/Flushable.php | 2 + src/Core/Injector/Injector.php | 11 +- .../Startup/ErrorControlChainMiddleware.php | 2 +- src/Security/AuthenticationMiddleware.php | 9 +- tests/php/Control/DirectorTest.php | 139 ++++++++++----- .../Control/DirectorTest/TestController.php | 7 + .../Control/DirectorTest/TestMiddleware.php | 2 +- tests/php/Control/HTTPRequestTest.php | 5 +- tests/php/Control/SessionTest.php | 4 +- tests/php/Core/Injector/InjectorTest.php | 33 +++- 29 files changed, 519 insertions(+), 262 deletions(-) create mode 100644 src/Control/Middleware/RequestHandlerMiddlewareAdapter.php diff --git a/.upgrade.yml b/.upgrade.yml index c04ad9e26..eb8ba58b0 100644 --- a/.upgrade.yml +++ b/.upgrade.yml @@ -261,7 +261,7 @@ mappings: Cookie_Backend: SilverStripe\Control\Cookie_Backend CookieJar: SilverStripe\Control\CookieJar Director: SilverStripe\Control\Director - FlushRequestFilter: SilverStripe\Control\FlushMiddleware + FlushRequestFilter: SilverStripe\Control\Middleware\FlushMiddleware HTTP: SilverStripe\Control\HTTP SS_HTTPRequest: SilverStripe\Control\HTTPRequest SS_HTTPResponse: SilverStripe\Control\HTTPResponse diff --git a/_config/requestprocessors.yml b/_config/requestprocessors.yml index 41704d179..59cce470a 100644 --- a/_config/requestprocessors.yml +++ b/_config/requestprocessors.yml @@ -5,12 +5,11 @@ SilverStripe\Core\Injector\Injector: SilverStripe\Control\Director: properties: Middlewares: - TrustedProxyMiddleware: '%$SilverStripe\Control\TrustedProxyMiddleware' - AllowedHostsMiddleware: '%$SilverStripe\Control\AllowedHostsMiddleware' - SessionMiddleware: '%$SilverStripe\Control\SessionMiddleware' - RequestProcessor: '%$SilverStripe\Control\RequestProcessor' - FlushMiddleware: '%$SilverStripe\Control\FlushMiddleware' - + TrustedProxyMiddleware: %$SilverStripe\Control\Middleware\TrustedProxyMiddleware + AllowedHostsMiddleware: %$SilverStripe\Control\Middleware\AllowedHostsMiddleware + SessionMiddleware: %$SilverStripe\Control\Middleware\SessionMiddleware + RequestProcessor: %$SilverStripe\Control\RequestProcessor + FlushMiddleware: %$SilverStripe\Control\Middleware\FlushMiddleware SilverStripe\Control\AllowedHostsMiddleware: properties: AllowedHosts: "`SS_ALLOWED_HOSTS`" diff --git a/_config/security.yml b/_config/security.yml index be0cf758f..f5af02dd2 100644 --- a/_config/security.yml +++ b/_config/security.yml @@ -18,12 +18,14 @@ SilverStripe\Core\Injector\Injector: alc: %$SilverStripe\Security\MemberAuthenticator\CookieAuthenticationHandler --- Name: coresecurity +After: + - requestprocessors --- SilverStripe\Core\Injector\Injector: SilverStripe\Control\Director: properties: Middlewares: - - %$SilverStripe\Security\AuthenticationMiddleware + AuthenticationMiddleware: %$SilverStripe\Security\AuthenticationMiddleware SilverStripe\Security\AuthenticationMiddleware: properties: AuthenticationHandler: %$SilverStripe\Security\AuthenticationHandler diff --git a/docs/en/02_Developer_Guides/02_Controllers/05_Middlewares.md b/docs/en/02_Developer_Guides/02_Controllers/05_Middlewares.md index a97a5d2f6..50c5015a0 100644 --- a/docs/en/02_Developer_Guides/02_Controllers/05_Middlewares.md +++ b/docs/en/02_Developer_Guides/02_Controllers/05_Middlewares.md @@ -8,8 +8,9 @@ authentication, logging, caching, request processing, and many other purposes. N replaces the SilverStripe 3 interface, [api:RequestFilter], which still works but is deprecated. To create a middleware class, implement `SilverStripe\Control\HTTPMiddleware` and define the -`process($request, $delegate)` method. You can do anything you like in this method, but to continue -normal execution, you should call `$response = $delegate($request)` at some point in this method. +`process(HTTPRequest $request, callbale $delegate)` method. You can do anything you like in this +method, but to continue normal execution, you should call `$response = $delegate($request)` +at some point in this method. In addition, you should return an HTTPResponse object. In normal cases, this should be the $response object returned by `$delegate`, perhaps with some modification. However, sometimes you @@ -20,7 +21,7 @@ will deliberately return a different response, e.g. an error response or a redir :::php enabled) { + if (static::config()->get('enabled')) { return true; } else { return (substr($response->getBody(), 0, 5) == '<' . '?xml'); @@ -106,7 +106,7 @@ class ContentNegotiator ); $q = array(); if (headers_sent()) { - $chosenFormat = static::config()->default_format; + $chosenFormat = static::config()->get('default_format'); } elseif (isset($_GET['forceFormat'])) { $chosenFormat = $_GET['forceFormat']; } else { diff --git a/src/Control/Director.php b/src/Control/Director.php index ed1a565d9..3921b8164 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -3,8 +3,10 @@ namespace SilverStripe\Control; use SilverStripe\CMS\Model\SiteTree; +use SilverStripe\Control\Middleware\HTTPMiddlewareAware; use SilverStripe\Core\Config\Configurable; use SilverStripe\Core\Environment; +use SilverStripe\Core\Injector\Injectable; use SilverStripe\Core\Injector\Injector; use SilverStripe\Core\Kernel; use SilverStripe\Dev\Deprecation; @@ -29,6 +31,7 @@ use SilverStripe\View\TemplateGlobalProvider; class Director implements TemplateGlobalProvider { use Configurable; + use Injectable; use HTTPMiddlewareAware; /** @@ -133,7 +136,7 @@ class Director implements TemplateGlobalProvider ) { return static::mockRequest( function (HTTPRequest $request) { - return Injector::inst()->get(Director::class)->handleRequest($request); + return Director::singleton()->handleRequest($request); }, $url, $postVars, @@ -315,6 +318,12 @@ class Director implements TemplateGlobalProvider }; foreach ($rules as $pattern => $controllerOptions) { + // Match pattern + $arguments = $request->match($pattern, true); + if ($arguments == false) { + continue; + } + // Normalise route rule if (is_string($controllerOptions)) { if (substr($controllerOptions, 0, 2) == '->') { @@ -323,57 +332,39 @@ class Director implements TemplateGlobalProvider $controllerOptions = array('Controller' => $controllerOptions); } } + $request->setRouteParams($controllerOptions); - // Match pattern - $arguments = $request->match($pattern, true); - if ($arguments !== false) { - $request->setRouteParams($controllerOptions); - // controllerOptions provide some default arguments - $arguments = array_merge($controllerOptions, $arguments); + // controllerOptions provide some default arguments + $arguments = array_merge($controllerOptions, $arguments); - // Pop additional tokens from the tokenizer if necessary - if (isset($controllerOptions['_PopTokeniser'])) { - $request->shift($controllerOptions['_PopTokeniser']); - } + // Pop additional tokens from the tokenizer if necessary + if (isset($controllerOptions['_PopTokeniser'])) { + $request->shift($controllerOptions['_PopTokeniser']); + } - // Handler for redirection - if (isset($arguments['Redirect'])) { - $handler = function () use ($arguments) { - // Redirection - $response = new HTTPResponse(); - $response->redirect(static::absoluteURL($arguments['Redirect'])); - return $response; - }; - break; - } - - // Find the controller name - $controller = $arguments['Controller']; - - // String = service name - if (is_string($controller)) { - $controllerObj = Injector::inst()->get($controller); - // Array = service spec - } elseif (is_array($controller)) { - $controllerObj = Injector::inst()->createFromSpec($controller); - } else { - throw new \LogicException("Invalid Controller value '$controller'"); - } - - // Handler for calling a controller - $handler = function ($request) use ($controllerObj) { - try { - // Apply the controller's middleware. We do this outside of handleRequest so that - // subclasses of handleRequest will be called after the middlware processing - return $controllerObj->callMiddleware($request, function ($request) use ($controllerObj) { - return $controllerObj->handleRequest($request); - }); - } catch (HTTPResponse_Exception $responseException) { - return $responseException->getResponse(); - } + // Handler for redirection + if (isset($arguments['Redirect'])) { + $handler = function () use ($arguments) { + // Redirection + $response = new HTTPResponse(); + $response->redirect(static::absoluteURL($arguments['Redirect'])); + return $response; }; break; } + + /** @var RequestHandler $controllerObj */ + $controllerObj = Injector::inst()->create($arguments['Controller']); + + // Handler for calling a controller + $handler = function (HTTPRequest $request) use ($controllerObj) { + try { + return $controllerObj->handleRequest($request); + } catch (HTTPResponse_Exception $responseException) { + return $responseException->getResponse(); + } + }; + break; } // Call the handler with the configured middlewares @@ -381,43 +372,11 @@ class Director implements TemplateGlobalProvider // Note that if a different request was previously registered, this will now be lost // In these cases it's better to use Kernel::nest() prior to kicking off a nested request - Injector::inst()->unregisterNamedObject(HTTPRequest::class); + Injector::inst()->unregisterNamedObject(HTTPRequest::class, true); return $response; } - /** - * Call the given request handler with the given middlewares - * Middlewares are specified as Injector service names - * - * @param $request The request to pass to the handler - * @param $middlewareNames The services names of the middlewares to apply - * @param $handler The request handler - */ - protected static function callWithMiddlewares(HTTPRequest $request, array $middlewareNames, callable $handler) - { - $next = $handler; - - if ($middlewareNames) { - $middlewares = array_map( - function ($name) { - return Injector::inst()->get($name); - }, - $middlewareNames - ); - - // Reverse middlewares - /** @var HTTPMiddleware $middleware */ - foreach (array_reverse($middlewares) as $middleware) { - $next = function ($request) use ($middleware, $next) { - return $middleware->process($request, $next); - }; - } - } - - return $next($request); - } - /** * Return the {@link SiteTree} object that is currently being viewed. If there is no SiteTree * object to return, then this will return the current controller. @@ -502,6 +461,7 @@ class Director implements TemplateGlobalProvider * - SERVER_NAME * - gethostname() * + * @param HTTPRequest $request * @return string */ public static function host(HTTPRequest $request = null) @@ -515,10 +475,8 @@ class Director implements TemplateGlobalProvider } } - if (!$request) { - $request = Injector::inst()->get(HTTPRequest::class, true, ['GET', '/']); - } - if ($request && $host = $request->getHeader('Host')) { + $request = static::currentRequest($request); + if ($request && ($host = $request->getHeader('Host'))) { return $host; } @@ -544,6 +502,7 @@ class Director implements TemplateGlobalProvider * Returns the domain part of the URL 'http://www.mysite.com'. Returns FALSE is this environment * variable isn't set. * + * @param HTTPRequest $request * @return bool|string */ public static function protocolAndHost(HTTPRequest $request = null) @@ -554,6 +513,7 @@ class Director implements TemplateGlobalProvider /** * Return the current protocol that the site is running under. * + * @param HTTPRequest $request * @return string */ public static function protocol(HTTPRequest $request = null) @@ -564,6 +524,7 @@ class Director implements TemplateGlobalProvider /** * Return whether the site is running as under HTTPS. * + * @param HTTPRequest $request * @return bool */ public static function is_https(HTTPRequest $request = null) @@ -578,11 +539,9 @@ class Director implements TemplateGlobalProvider } // Check the current request - if (!$request) { - $request = Injector::inst()->get(HTTPRequest::class, true, ['GET', '/']); - } - if ($request && $host = $request->getHeader('Host')) { - return $request->getScheme() === 'https'; + $request = static::currentRequest($request); + if ($request && ($scheme = $request->getScheme())) { + return $scheme === 'https'; } // Check default_base_url @@ -842,9 +801,10 @@ class Director implements TemplateGlobalProvider * Returns the Absolute URL of the site root, embedding the current basic-auth credentials into * the URL. * + * @param HTTPRequest|null $request * @return string */ - public static function absoluteBaseURLWithAuth() + public static function absoluteBaseURLWithAuth(HTTPRequest $request = null) { $login = ""; @@ -852,7 +812,7 @@ class Director implements TemplateGlobalProvider $login = "$_SERVER[PHP_AUTH_USER]:$_SERVER[PHP_AUTH_PW]@"; } - return Director::protocol() . $login . static::host() . Director::baseURL(); + return Director::protocol($request) . $login . static::host($request) . Director::baseURL(); } /** @@ -960,12 +920,14 @@ class Director implements TemplateGlobalProvider * Checks if the current HTTP-Request is an "Ajax-Request" by checking for a custom header set by * jQuery or whether a manually set request-parameter 'ajax' is present. * + * @param HTTPRequest $request * @return bool */ - public static function is_ajax() + public static function is_ajax(HTTPRequest $request = null) { - if (Controller::has_curr()) { - return Controller::curr()->getRequest()->isAjax(); + $request = self::currentRequest($request); + if ($request) { + return $request->isAjax(); } else { return ( isset($_REQUEST['ajax']) || @@ -1046,4 +1008,20 @@ class Director implements TemplateGlobalProvider 'BaseHref' => 'absoluteBaseURL', //@deprecated 3.0 ); } + + /** + * Helper to validate or check the current request object + * + * @param HTTPRequest $request + * @return HTTPRequest Request object if one is both current and valid + */ + protected static function currentRequest(HTTPRequest $request = null) + { + // Ensure we only use a registered HTTPRequest and don't + // incidentally construct a singleton + if (!$request && Injector::inst()->has(HTTPRequest::class)) { + $request = Injector::inst()->get(HTTPRequest::class); + } + return $request; + } } diff --git a/src/Control/HTTPApplication.php b/src/Control/HTTPApplication.php index 552ffba69..7f765feb7 100644 --- a/src/Control/HTTPApplication.php +++ b/src/Control/HTTPApplication.php @@ -2,9 +2,8 @@ namespace SilverStripe\Control; +use SilverStripe\Control\Middleware\HTTPMiddlewareAware; use SilverStripe\Core\Application; -use SilverStripe\Core\Injector\Injector; -use SilverStripe\Control\HTTPMiddleware; use SilverStripe\Core\Kernel; /** @@ -12,7 +11,6 @@ use SilverStripe\Core\Kernel; */ class HTTPApplication implements Application { - use HTTPMiddlewareAware; /** @@ -47,7 +45,7 @@ class HTTPApplication implements Application // Ensure boot is invoked return $this->execute($request, function (HTTPRequest $request) { - return Injector::inst()->get(Director::class)->handleRequest($request); + return Director::singleton()->handleRequest($request); }, $flush); } diff --git a/src/Control/HTTPRequest.php b/src/Control/HTTPRequest.php index b03369df1..6c54f8a2c 100644 --- a/src/Control/HTTPRequest.php +++ b/src/Control/HTTPRequest.php @@ -4,6 +4,7 @@ namespace SilverStripe\Control; use ArrayAccess; use BadMethodCallException; +use InvalidArgumentException; use SilverStripe\Core\ClassInfo; use SilverStripe\ORM\ArrayLib; @@ -783,12 +784,18 @@ class HTTPRequest implements ArrayAccess /** * Sets the client IP address which originated this request. + * Use setIPFromHeaderValue if assigning from header value. * * @param $ip string + * @return $this */ public function setIP($ip) { + if (!filter_var($ip, FILTER_VALIDATE_IP)) { + throw new InvalidArgumentException("Invalid ip $ip"); + } $this->ip = $ip; + return $this; } /** @@ -820,9 +827,11 @@ class HTTPRequest implements ArrayAccess /** * Return the URL scheme (e.g. "http" or "https"). * Equivalent to PSR-7 getUri()->getScheme() + * * @return string */ - public function getScheme() { + public function getScheme() + { return $this->scheme; } @@ -831,9 +840,12 @@ class HTTPRequest implements ArrayAccess * Equivalent to PSR-7 getUri()->getScheme(), * * @param string $scheme + * @return $this */ - public function setScheme($scheme) { + public function setScheme($scheme) + { $this->scheme = $scheme; + return $this; } /** diff --git a/src/Control/Middleware/AllowedHostsMiddleware.php b/src/Control/Middleware/AllowedHostsMiddleware.php index 533481762..f3e50826a 100644 --- a/src/Control/Middleware/AllowedHostsMiddleware.php +++ b/src/Control/Middleware/AllowedHostsMiddleware.php @@ -1,17 +1,25 @@ allowedHosts = $allowedHosts; + return $this; } /** @@ -31,13 +47,14 @@ class AllowedHostsMiddleware implements HTTPMiddleware */ public function process(HTTPRequest $request, callable $delegate) { - if ($this->allowedHosts && !Director::is_cli()) { - $allowedHosts = preg_split('/ *, */', $this->allowedHosts); + $allowedHosts = $this->getAllowedHosts(); - // check allowed hosts - if (!in_array($request->getHeader('Host'), $allowedHosts)) { - return new HTTPResponse('Invalid Host', 400); - } + // check allowed hosts + if ($allowedHosts + && !Director::is_cli() + && !in_array($request->getHeader('Host'), $allowedHosts) + ) { + return new HTTPResponse('Invalid Host', 400); } return $delegate($request); diff --git a/src/Control/Middleware/FlushMiddleware.php b/src/Control/Middleware/FlushMiddleware.php index e29e4448a..0d8f8b482 100644 --- a/src/Control/Middleware/FlushMiddleware.php +++ b/src/Control/Middleware/FlushMiddleware.php @@ -1,9 +1,10 @@ getVars())) { foreach (ClassInfo::implementorsOf(Flushable::class) as $class) { + /** @var Flushable|string $class */ $class::flush(); } } diff --git a/src/Control/Middleware/HTTPMiddleware.php b/src/Control/Middleware/HTTPMiddleware.php index a1be72d93..f8c8a1697 100644 --- a/src/Control/Middleware/HTTPMiddleware.php +++ b/src/Control/Middleware/HTTPMiddleware.php @@ -1,6 +1,9 @@ setRequestHandler($handler); + } + parent::__construct(); + } + + public function Link($action = null) + { + return $this->getRequestHandler()->Link($action); + } + + /** + * @return RequestHandler + */ + public function getRequestHandler() + { + return $this->requestHandler; + } + + /** + * @param RequestHandler $requestHandler + * @return $this + */ + public function setRequestHandler(RequestHandler $requestHandler) + { + $this->requestHandler = $requestHandler; + return $this; + } + + public function handleRequest(HTTPRequest $request) + { + return $this->callMiddleware($request, function (HTTPRequest $request) { + $this->setRequest($request); + return $this->getRequestHandler()->handleRequest($request); + }); + } +} diff --git a/src/Control/Middleware/SessionMiddleware.php b/src/Control/Middleware/SessionMiddleware.php index cd8c393c4..a1b788d5a 100644 --- a/src/Control/Middleware/SessionMiddleware.php +++ b/src/Control/Middleware/SessionMiddleware.php @@ -1,6 +1,8 @@ trustedProxyIPs = $trustedProxyIPs; + return $this; } /** - * Return the comma-separated list of headers from which to lookup the hostname + * Return the array of headers from which to lookup the hostname * - * @return string + * @return array */ public function getProxyHostHeaders() { @@ -55,19 +82,25 @@ class TrustedProxyMiddleware implements HTTPMiddleware } /** - * Set the comma-separated list of headers from which to lookup the hostname + * Set the array of headers from which to lookup the hostname + * Can also specify comma-separated list as a single string. * - * @param $proxyHostHeaders string + * @param array|string $proxyHostHeaders + * @return $this */ public function setProxyHostHeaders($proxyHostHeaders) { - $this->proxyHostHeaders = $proxyHostHeaders; + if (is_string($proxyHostHeaders)) { + $proxyHostHeaders = preg_split('/ *, */', $proxyHostHeaders); + } + $this->proxyHostHeaders = $proxyHostHeaders ?: []; + return $this; } /** - * Return the comma-separated list of headers from which to lookup the client IP + * Return the array of headers from which to lookup the client IP * - * @return string + * @return array */ public function getProxyIPHeaders() { @@ -75,19 +108,25 @@ class TrustedProxyMiddleware implements HTTPMiddleware } /** - * Set the comma-separated list of headers from which to lookup the client IP + * Set the array of headers from which to lookup the client IP + * Can also specify comma-separated list as a single string. * - * @param $proxyIPHeaders string + * @param array|string $proxyIPHeaders + * @return $this */ public function setProxyIPHeaders($proxyIPHeaders) { - $this->proxyIPHeaders = $proxyIPHeaders; + if (is_string($proxyIPHeaders)) { + $proxyIPHeaders = preg_split('/ *, */', $proxyIPHeaders); + } + $this->proxyIPHeaders = $proxyIPHeaders ?: []; + return $this; } /** - * Return the comma-separated list of headers from which to lookup the client scheme (http/https) + * Return the array of headers from which to lookup the client scheme (http/https) * - * @return string + * @return array */ public function getProxySchemeHeaders() { @@ -95,13 +134,19 @@ class TrustedProxyMiddleware implements HTTPMiddleware } /** - * Set the comma-separated list of headers from which to lookup the client scheme (http/https) + * Set array of headers from which to lookup the client scheme (http/https) + * Can also specify comma-separated list as a single string. * - * @param $proxySchemeHeaders string + * @param array|string $proxySchemeHeaders + * @return $this */ public function setProxySchemeHeaders($proxySchemeHeaders) { - $this->proxySchemeHeaders = $proxySchemeHeaders; + if (is_string($proxySchemeHeaders)) { + $proxySchemeHeaders = preg_split('/ *, */', $proxySchemeHeaders); + } + $this->proxySchemeHeaders = $proxySchemeHeaders ?: []; + return $this; } public function process(HTTPRequest $request, callable $delegate) @@ -109,29 +154,32 @@ class TrustedProxyMiddleware implements HTTPMiddleware // If this is a trust proxy if ($this->isTrustedProxy($request)) { // Replace host - foreach ($this->proxyHostHeaders as $header) { + foreach ($this->getProxyHostHeaders() as $header) { $hostList = $request->getHeader($header); if ($hostList) { - $request->setHeader('Host', strtok($hostList, ',')); + $request->addHeader('Host', strtok($hostList, ',')); break; } } // Replace scheme - foreach ($this->proxySchemeHeaders as $header) { - $scheme = $request->getHeader($header); - if ($scheme) { - $request->setScheme(strtolower($scheme)); + foreach ($this->getProxySchemeHeaders() as $header) { + $headerValue = $request->getHeader($header); + if ($headerValue) { + $request->setScheme(strtolower($headerValue)); break; } } // Replace IP foreach ($this->proxyIPHeaders as $header) { - $ipHeader = $this->getIPFromHeaderValue($request->getHeader($header)); - if ($ipHeader) { - $request->setIP($ipHeader); - break; + $headerValue = $request->getHeader($header); + if ($headerValue) { + $ipHeader = $this->getIPFromHeaderValue($headerValue); + if ($ipHeader) { + $request->setIP($ipHeader); + break; + } } } } @@ -142,12 +190,15 @@ class TrustedProxyMiddleware implements HTTPMiddleware /** * Determine if the current request is coming from a trusted proxy * - * @return boolean True if the request's source IP is a trusted proxy + * @param HTTPRequest $request + * @return bool True if the request's source IP is a trusted proxy */ - protected function isTrustedProxy($request) + protected function isTrustedProxy(HTTPRequest $request) { + $trustedIPs = $this->getTrustedProxyIPs(); + // Disabled - if (empty($this->trustedProxyIPs) || $trustedIPs === 'none') { + if (empty($trustedIPs) || $trustedIPs === 'none') { return false; } @@ -157,8 +208,9 @@ class TrustedProxyMiddleware implements HTTPMiddleware } // Validate IP address - if ($ip = $request->getIP()) { - return IPUtils::checkIP($ip, explode(',', $trustedIPs)); + $ip = $request->getIP(); + if ($ip) { + return IPUtils::checkIP($ip, preg_split('/ *, */', $trustedIPs)); } return false; @@ -173,19 +225,24 @@ class TrustedProxyMiddleware implements HTTPMiddleware */ protected function getIPFromHeaderValue($headerValue) { - if (strpos($headerValue, ',') !== false) { - //sometimes the IP from a load balancer could be "x.x.x.x, y.y.y.y, z.z.z.z" so we need to find the most - // likely candidate - $ips = explode(',', $headerValue); + // Sometimes the IP from a load balancer could be "x.x.x.x, y.y.y.y, z.z.z.z" + // so we need to find the most likely candidate + $ips = preg_split('/\s*,\s*/', $headerValue); + + // Prioritise filters + $filters = [ + FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE, + FILTER_FLAG_NO_PRIV_RANGE, + null + ]; + foreach ($filters as $filter) { + // Find best IP foreach ($ips as $ip) { - $ip = trim($ip); - if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) { + if (filter_var($ip, FILTER_VALIDATE_IP, $filter)) { return $ip; - } else { - return null; } } } - return $headerValue; + return null; } } diff --git a/src/Control/RequestFilter.php b/src/Control/RequestFilter.php index 0d64980a8..12b333f56 100644 --- a/src/Control/RequestFilter.php +++ b/src/Control/RequestFilter.php @@ -9,6 +9,7 @@ namespace SilverStripe\Control; * * @author marcus@silverstripe.com.au * @license BSD License http://silverstripe.org/bsd-license/ + * @deprecated 4.0..5.0 Use HTTPMiddleware instead */ interface RequestFilter { diff --git a/src/Control/RequestHandler.php b/src/Control/RequestHandler.php index ce76900e0..14c06ff53 100644 --- a/src/Control/RequestHandler.php +++ b/src/Control/RequestHandler.php @@ -2,17 +2,17 @@ namespace SilverStripe\Control; +use BadMethodCallException; +use Exception; use InvalidArgumentException; +use ReflectionClass; use SilverStripe\Core\ClassInfo; use SilverStripe\Core\Config\Config; use SilverStripe\Dev\Debug; -use SilverStripe\Security\Security; -use SilverStripe\Security\PermissionFailureException; use SilverStripe\Security\Permission; +use SilverStripe\Security\PermissionFailureException; +use SilverStripe\Security\Security; use SilverStripe\View\ViewableData; -use ReflectionClass; -use Exception; -use BadMethodCallException; /** * This class is the base class of any SilverStripe object that can be used to handle HTTP requests. @@ -47,8 +47,6 @@ use BadMethodCallException; class RequestHandler extends ViewableData { - use HTTPMiddlewareAware; - /** * Optional url_segment for this request handler * diff --git a/src/Control/RequestProcessor.php b/src/Control/RequestProcessor.php index fa6d319ae..c8f8a006a 100644 --- a/src/Control/RequestProcessor.php +++ b/src/Control/RequestProcessor.php @@ -2,12 +2,14 @@ namespace SilverStripe\Control; +use SilverStripe\Control\Middleware\HTTPMiddleware; use SilverStripe\Core\Injector\Injectable; use SilverStripe\Dev\Deprecation; /** * Middleware that provides back-support for the deprecated RequestFilter API. - * You should use HTTPMiddleware directly instead. + * + * @deprecated 4.0..5.0 Use HTTPMiddleware directly instead. */ class RequestProcessor implements HTTPMiddleware { @@ -16,10 +18,15 @@ class RequestProcessor implements HTTPMiddleware /** * List of currently assigned request filters * - * @var array + * @var RequestFilter[] */ private $filters = array(); + /** + * Construct new RequestFilter with a list of filter objects + * + * @param RequestFilter[] $filters + */ public function __construct($filters = array()) { $this->filters = $filters; @@ -28,11 +35,13 @@ class RequestProcessor implements HTTPMiddleware /** * Assign a list of request filters * - * @param array $filters + * @param RequestFilter[] $filters + * @return $this */ public function setFilters($filters) { $this->filters = $filters; + return $this; } /** @@ -42,7 +51,7 @@ class RequestProcessor implements HTTPMiddleware { if ($this->filters) { Deprecation::notice( - '4.0', + '5.0', 'Deprecated RequestFilters are in use. Apply HTTPMiddleware to Director.middlewares instead.' ); } diff --git a/src/Control/Session.php b/src/Control/Session.php index c734cbdb6..12db7c69a 100644 --- a/src/Control/Session.php +++ b/src/Control/Session.php @@ -143,9 +143,10 @@ class Session /** * Get user agent for this request * + * @param HTTPRequest $request * @return string */ - protected function userAgent($request) + protected function userAgent(HTTPRequest $request) { return $request->getHeader('User-Agent'); } @@ -167,6 +168,8 @@ class Session /** * Init this session instance before usage + * + * @param HTTPRequest $request */ public function init(HTTPRequest $request) { @@ -186,6 +189,8 @@ class Session /** * Destroy existing session and restart + * + * @param HTTPRequest $request */ public function restart(HTTPRequest $request) { @@ -206,7 +211,7 @@ class Session /** * Begin session * - * @param $request The request for which to start a session + * @param HTTPRequest $request The request for which to start a session */ public function start(HTTPRequest $request) { @@ -463,6 +468,8 @@ class Session /** * Set user agent key + * + * @param HTTPRequest $request */ public function finalize(HTTPRequest $request) { @@ -472,6 +479,8 @@ class Session /** * Save data to session * Only save the changes, so that anyone manipulating $_SESSION directly doesn't get burned. + * + * @param HTTPRequest $request */ public function save(HTTPRequest $request) { diff --git a/src/Core/Flushable.php b/src/Core/Flushable.php index fe605f398..7b30d3d32 100644 --- a/src/Core/Flushable.php +++ b/src/Core/Flushable.php @@ -2,6 +2,8 @@ namespace SilverStripe\Core; +use SilverStripe\Control\Middleware\FlushMiddleware; + /** * Provides an interface for classes to implement their own flushing functionality * whenever flush=1 is requested. diff --git a/src/Core/Injector/Injector.php b/src/Core/Injector/Injector.php index 8b68f5aef..02754bf46 100644 --- a/src/Core/Injector/Injector.php +++ b/src/Core/Injector/Injector.php @@ -851,11 +851,15 @@ class Injector implements ContainerInterface * by the inject * * @param string $name The name to unregister + * @param bool $flushSpecs Set to true to clear spec for this service * @return $this */ - public function unregisterNamedObject($name) + public function unregisterNamedObject($name, $flushSpecs = false) { unset($this->serviceCache[$name]); + if ($flushSpecs) { + unset($this->specs[$name]); + } return $this; } @@ -863,9 +867,10 @@ class Injector implements ContainerInterface * Clear out objects of one or more types that are managed by the injetor. * * @param array|string $types Base class of object (not service name) to remove + * @param bool $flushSpecs Set to true to clear spec for this service * @return $this */ - public function unregisterObjects($types) + public function unregisterObjects($types, $flushSpecs = false) { if (!is_array($types)) { $types = [ $types ]; @@ -879,7 +884,7 @@ class Injector implements ContainerInterface throw new InvalidArgumentException("Global unregistration is not allowed"); } if ($object instanceof $filterClass) { - unset($this->serviceCache[$key]); + $this->unregisterNamedObject($key, $flushSpecs); break; } } diff --git a/src/Core/Startup/ErrorControlChainMiddleware.php b/src/Core/Startup/ErrorControlChainMiddleware.php index e853ebd08..0e0d78d8c 100644 --- a/src/Core/Startup/ErrorControlChainMiddleware.php +++ b/src/Core/Startup/ErrorControlChainMiddleware.php @@ -7,7 +7,7 @@ use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; use SilverStripe\Control\HTTPResponse_Exception; use SilverStripe\Core\Application; -use SilverStripe\Control\HTTPMiddleware; +use SilverStripe\Control\Middleware\HTTPMiddleware; use SilverStripe\Security\Permission; use SilverStripe\Security\Security; diff --git a/src/Security/AuthenticationMiddleware.php b/src/Security/AuthenticationMiddleware.php index bb427ea2d..525b25277 100644 --- a/src/Security/AuthenticationMiddleware.php +++ b/src/Security/AuthenticationMiddleware.php @@ -4,8 +4,7 @@ namespace SilverStripe\Security; use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; -use SilverStripe\Control\HTTPResponse_Exception; -use SilverStripe\Control\HTTPMiddleware; +use SilverStripe\Control\Middleware\HTTPMiddleware; use SilverStripe\Core\Config\Configurable; use SilverStripe\ORM\ValidationException; @@ -40,8 +39,8 @@ class AuthenticationMiddleware implements HTTPMiddleware * Identify the current user from the request * * @param HTTPRequest $request - * @return bool|void - * @throws HTTPResponse_Exception + * @param callable $delegate + * @return HTTPResponse */ public function process(HTTPRequest $request, callable $delegate) { @@ -60,4 +59,4 @@ class AuthenticationMiddleware implements HTTPMiddleware return $delegate($request); } - } +} diff --git a/tests/php/Control/DirectorTest.php b/tests/php/Control/DirectorTest.php index a7095cf1a..e9fd2b741 100644 --- a/tests/php/Control/DirectorTest.php +++ b/tests/php/Control/DirectorTest.php @@ -7,12 +7,15 @@ use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPRequestBuilder; use SilverStripe\Control\HTTPResponse; use SilverStripe\Control\HTTPResponse_Exception; +use SilverStripe\Control\Middleware\HTTPMiddleware; +use SilverStripe\Control\Middleware\RequestHandlerMiddlewareAdapter; +use SilverStripe\Control\Middleware\TrustedProxyMiddleware; use SilverStripe\Control\RequestProcessor; use SilverStripe\Control\Tests\DirectorTest\TestController; +use SilverStripe\Core\Config\Config; use SilverStripe\Core\Injector\Injector; use SilverStripe\Core\Kernel; use SilverStripe\Dev\SapphireTest; -use SilverStripe\Core\Config\Config; /** * @todo test Director::alternateBaseFolder() @@ -535,63 +538,91 @@ class DirectorTest extends SapphireTest public function testIsHttps() { - if (!TRUSTED_PROXY) { - $this->markTestSkipped('Test cannot be run without trusted proxy'); - } + // Trust all IPs for this test + /** @var TrustedProxyMiddleware $trustedProxyMiddleware */ + $trustedProxyMiddleware + = Injector::inst()->get(TrustedProxyMiddleware::class); + $trustedProxyMiddleware->setTrustedProxyIPs('*'); + + // Clear alternate_base_url for this test + Director::config()->remove('alternate_base_url'); + // nothing available $headers = array( 'HTTP_X_FORWARDED_PROTOCOL', 'HTTPS', 'SSL' ); - - $origServer = $_SERVER; - foreach ($headers as $header) { if (isset($_SERVER[$header])) { unset($_SERVER['HTTP_X_FORWARDED_PROTOCOL']); } } - $this->assertFalse(Director::is_https()); + $this->assertEquals( + 'no', + Director::test('TestController/returnIsSSL')->getBody() + ); - $_SERVER['HTTP_X_FORWARDED_PROTOCOL'] = 'https'; - $this->assertTrue(Director::is_https()); + $this->assertEquals( + 'yes', + Director::test( + 'TestController/returnIsSSL', + null, + null, + null, + null, + [ 'X-Forwarded-Protocol' => 'https' ] + )->getBody() + ); - $_SERVER['HTTP_X_FORWARDED_PROTOCOL'] = 'http'; - $this->assertFalse(Director::is_https()); + $this->assertEquals( + 'no', + Director::test( + 'TestController/returnIsSSL', + null, + null, + null, + null, + [ 'X-Forwarded-Protocol' => 'http' ] + )->getBody() + ); - $_SERVER['HTTP_X_FORWARDED_PROTOCOL'] = 'ftp'; - $this->assertFalse(Director::is_https()); - - $_SERVER['HTTP_X_FORWARDED_PROTO'] = 'https'; - $this->assertTrue(Director::is_https()); - - $_SERVER['HTTP_X_FORWARDED_PROTO'] = 'http'; - $this->assertFalse(Director::is_https()); - - $_SERVER['HTTP_X_FORWARDED_PROTO'] = 'ftp'; - $this->assertFalse(Director::is_https()); - - $_SERVER['HTTP_FRONT_END_HTTPS'] = 'On'; - $this->assertTrue(Director::is_https()); - - $_SERVER['HTTP_FRONT_END_HTTPS'] = 'Off'; - $this->assertFalse(Director::is_https()); + $this->assertEquals( + 'no', + Director::test( + 'TestController/returnIsSSL', + null, + null, + null, + null, + [ 'X-Forwarded-Protocol' => 'ftp' ] + )->getBody() + ); // https via HTTPS $_SERVER['HTTPS'] = 'true'; - $this->assertTrue(Director::is_https()); + $this->assertEquals( + 'yes', + Director::test('TestController/returnIsSSL')->getBody() + ); $_SERVER['HTTPS'] = '1'; - $this->assertTrue(Director::is_https()); + $this->assertEquals( + 'yes', + Director::test('TestController/returnIsSSL')->getBody() + ); $_SERVER['HTTPS'] = 'off'; - $this->assertFalse(Director::is_https()); + $this->assertEquals( + 'no', + Director::test('TestController/returnIsSSL')->getBody() + ); // https via SSL $_SERVER['SSL'] = ''; - $this->assertTrue(Director::is_https()); - - $_SERVER = $origServer; + $this->assertEquals( + 'yes', + Director::test('TestController/returnIsSSL')->getBody() + ); } public function testTestIgnoresHashes() @@ -650,9 +681,7 @@ class DirectorTest extends SapphireTest public function testGlobalMiddleware() { $middleware = new DirectorTest\TestMiddleware; - - Injector::inst()->registerService($middleware, 'Middleware1'); - Config::modify()->set(Director::class, 'middlewares', [ 'Middleware1' ]); + Director::singleton()->setMiddlewares([$middleware]); $response = Director::test('some-dummy-url'); $this->assertEquals(404, $response->getStatusCode()); @@ -682,14 +711,30 @@ class DirectorTest extends SapphireTest public function testRouteSpecificMiddleware() { - $middleware = new DirectorTest\TestMiddleware; + // Inject adapter in place of controller $specificMiddleware = new DirectorTest\TestMiddleware; + Injector::inst()->registerService($specificMiddleware, 'SpecificMiddleware'); - Injector::inst()->registerService($middleware, 'Middleware1'); - Injector::inst()->registerService($specificMiddleware, 'Middleware2'); + // Register adapter as factory for creating this controller + Config::modify()->merge( + Injector::class, + 'ControllerWithMiddleware', + [ + 'class' => RequestHandlerMiddlewareAdapter::class, + 'constructor' => [ + '%$' . TestController::class + ], + 'properties' => [ + 'Middlewares' => [ + '%$SpecificMiddleware', + ], + ], + ] + ); // Global middleware - Config::modify()->set(Director::class, 'middlewares', [ 'Middleware1' ]); + $middleware = new DirectorTest\TestMiddleware; + Director::singleton()->setMiddlewares([ $middleware ]); // URL rules, one of which has a specific middleware Config::modify()->set( @@ -698,24 +743,22 @@ class DirectorTest extends SapphireTest [ 'url-one' => TestController::class, 'url-two' => [ - 'Controller' => TestController::class, - 'Middlewares' => [ 'Middleware2' ] - ] + 'Controller' => 'ControllerWithMiddleware', + ], ] ); // URL without a route-specific middleware - $response = Director::test('url-one'); + Director::test('url-one'); // Only the global middleware triggered $this->assertEquals(1, $middleware->preCalls); $this->assertEquals(0, $specificMiddleware->postCalls); - $response = Director::test('url-two'); + Director::test('url-two'); // Both triggered on the url with the specific middleware applied $this->assertEquals(2, $middleware->preCalls); $this->assertEquals(1, $specificMiddleware->postCalls); - } } diff --git a/tests/php/Control/DirectorTest/TestController.php b/tests/php/Control/DirectorTest/TestController.php index e3e4400be..d210ea4de 100644 --- a/tests/php/Control/DirectorTest/TestController.php +++ b/tests/php/Control/DirectorTest/TestController.php @@ -3,6 +3,7 @@ namespace SilverStripe\Control\Tests\DirectorTest; use SilverStripe\Control\Controller; +use SilverStripe\Control\Director; use SilverStripe\Dev\TestOnly; class TestController extends Controller implements TestOnly @@ -22,6 +23,7 @@ class TestController extends Controller implements TestOnly 'returnPostValue', 'returnRequestValue', 'returnCookieValue', + 'returnIsSSL', ); public function returnGetValue($request) @@ -55,4 +57,9 @@ class TestController extends Controller implements TestOnly } return null; } + + public function returnIsSSL() + { + return Director::is_https() ? 'yes': 'no'; + } } diff --git a/tests/php/Control/DirectorTest/TestMiddleware.php b/tests/php/Control/DirectorTest/TestMiddleware.php index d26e4a727..1390e8d48 100644 --- a/tests/php/Control/DirectorTest/TestMiddleware.php +++ b/tests/php/Control/DirectorTest/TestMiddleware.php @@ -4,7 +4,7 @@ namespace SilverStripe\Control\Tests\DirectorTest; use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; -use SilverStripe\Control\HTTPMiddleware; +use SilverStripe\Control\Middleware\HTTPMiddleware; use SilverStripe\Dev\TestOnly; class TestMiddleware implements HTTPMiddleware, TestOnly diff --git a/tests/php/Control/HTTPRequestTest.php b/tests/php/Control/HTTPRequestTest.php index 7947c2569..bade456b5 100644 --- a/tests/php/Control/HTTPRequestTest.php +++ b/tests/php/Control/HTTPRequestTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\Control\Tests; +use SilverStripe\Control\Middleware\TrustedProxyMiddleware; use SilverStripe\Dev\SapphireTest; use SilverStripe\Control\HTTPRequest; use ReflectionMethod; @@ -267,9 +268,9 @@ class HTTPRequestTest extends SapphireTest $this->assertEquals('home', $req->getURL()); } - public function testGetIPFromHeaderValue() + public function testSetIPFromHeaderValue() { - $req = new HTTPRequest('GET', '/'); + $req = new TrustedProxyMiddleware(); $reflectionMethod = new ReflectionMethod($req, 'getIPFromHeaderValue'); $reflectionMethod->setAccessible(true); diff --git a/tests/php/Control/SessionTest.php b/tests/php/Control/SessionTest.php index acc50c84a..7d92c28b5 100644 --- a/tests/php/Control/SessionTest.php +++ b/tests/php/Control/SessionTest.php @@ -109,7 +109,7 @@ class SessionTest extends SapphireTest { // Set a user agent $req1 = new HTTPRequest('GET', '/'); - $req1->setHeader('User-Agent', 'Test Agent'); + $req1->addHeader('User-Agent', 'Test Agent'); // Generate our session $s = new Session(array()); @@ -119,7 +119,7 @@ class SessionTest extends SapphireTest // Change our UA $req2 = new HTTPRequest('GET', '/'); - $req2->setHeader('User-Agent', 'Test Agent'); + $req2->addHeader('User-Agent', 'Fake Agent'); // Verify the new session reset our values $s2 = new Session($s); diff --git a/tests/php/Core/Injector/InjectorTest.php b/tests/php/Core/Injector/InjectorTest.php index 620df8875..bca527c10 100644 --- a/tests/php/Core/Injector/InjectorTest.php +++ b/tests/php/Core/Injector/InjectorTest.php @@ -24,6 +24,7 @@ use SilverStripe\Core\Tests\Injector\InjectorTest\TestObject; use SilverStripe\Core\Tests\Injector\InjectorTest\TestSetterInjections; use SilverStripe\Core\Tests\Injector\InjectorTest\TestStaticInjections; use SilverStripe\Dev\SapphireTest; +use SilverStripe\Dev\TestOnly; use stdClass; define('TEST_SERVICES', __DIR__ . '/AopProxyServiceTest'); @@ -802,10 +803,40 @@ class InjectorTest extends SapphireTest public function testNamedServices() { $injector = new Injector(); - $service = new stdClass(); + $service = new TestObject(); + $service->setSomething('injected'); + // Test registering with non-class name $injector->registerService($service, 'NamedService'); + $this->assertTrue($injector->has('NamedService')); $this->assertEquals($service, $injector->get('NamedService')); + + // Unregister by name only: New instance of the + // old class will be constructed + $injector->unregisterNamedObject('NamedService'); + $this->assertTrue($injector->has('NamedService')); + $this->assertNotEquals($service, $injector->get(TestObject::class)); + + // Unregister name and spec, injector forgets about this + // service spec altogether + $injector->unregisterNamedObject('NamedService', true); + $this->assertFalse($injector->has('NamedService')); + + // Test registered with class name + $injector->registerService($service); + $this->assertTrue($injector->has(TestObject::class)); + $this->assertEquals($service, $injector->get(TestObject::class)); + + // Unregister by name only: New instance of the + // old class will be constructed + $injector->unregisterNamedObject(TestObject::class); + $this->assertTrue($injector->has(TestObject::class)); + $this->assertNotEquals($service, $injector->get(TestObject::class)); + + // Unregister name and spec, injector forgets about this + // service spec altogether + $injector->unregisterNamedObject(TestObject::class, true); + $this->assertFalse($injector->has(TestObject::class)); } public function testCreateConfiggedObjectWithCustomConstructorArgs()