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()