From 26b9bf11edcc2cb5a27a681ab52ad0f8299df81a Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 23 Jun 2017 11:15:51 +1200 Subject: [PATCH 01/18] =?UTF-8?q?NEW:=20Allow=20=E2=80=9C%$=E2=80=9D=20pre?= =?UTF-8?q?fix=20in=20Injector::get()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Injector::get() looks up services by name. In yaml config it can make things clearer to prefix service names by %$, which is how they must be prefixed when referencing nested services within service definitions. This change means that any other system referencing services will support an optional prefix without needing to specifically code support in themselves. --- src/Core/Injector/Injector.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Core/Injector/Injector.php b/src/Core/Injector/Injector.php index 6b3f28abd..8b68f5aef 100644 --- a/src/Core/Injector/Injector.php +++ b/src/Core/Injector/Injector.php @@ -929,6 +929,11 @@ class Injector implements ContainerInterface */ protected function getNamedService($name, $asSingleton = true, $constructorArgs = []) { + // Allow service names of the form "%$ServiceName" + if (substr($name, 0, 2) == '%$') { + $name = substr($name, 2); + } + // Normalise service / args list($name, $constructorArgs) = $this->normaliseArguments($name, $constructorArgs); From b30f410ea02b2330eef871a2eb59ad022fdfa102 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 23 Jun 2017 11:17:30 +1200 Subject: [PATCH 02/18] API: Deprecate RequestFilter. NEW: Allow application of HTTPMiddleware to Director. Director can now use the same HTTPMiddleware objects as the app object. They can be applied either globally or pre-rule. --- _config/routes.yml | 2 + src/Control/Director.php | 99 ++++++++++++++++++++++++-------- src/Control/RequestProcessor.php | 31 ++++++---- 3 files changed, 98 insertions(+), 34 deletions(-) diff --git a/_config/routes.yml b/_config/routes.yml index 86c35b233..f15949722 100644 --- a/_config/routes.yml +++ b/_config/routes.yml @@ -2,6 +2,8 @@ Name: rootroutes --- SilverStripe\Control\Director: + middlewares: + RequestProcessor: 'SilverStripe\Control\RequestProcessor' rules: '': SilverStripe\Control\Controller --- diff --git a/src/Control/Director.php b/src/Control/Director.php index f5f2d920f..59fb66efd 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -131,24 +131,12 @@ class Director implements TemplateGlobalProvider } } - // Pre-request - $output = RequestProcessor::singleton()->preRequest($request); - if ($output === false) { - return new HTTPResponse(_t(__CLASS__.'.INVALID_REQUEST', 'Invalid request'), 400); - } - // Generate output $result = static::handleRequest($request); // Save session data. Note that save() will start/resume the session if required. $request->getSession()->save(); - // Post-request handling - $postRequest = RequestProcessor::singleton()->postRequest($request, $result); - if ($postRequest === false) { - return new HTTPResponse(_t(__CLASS__ . '.REQUEST_ABORTED', 'Request aborted'), 500); - } - // Return return $result; } @@ -351,6 +339,14 @@ class Director implements TemplateGlobalProvider { $rules = Director::config()->uninherited('rules'); + // Get global middlewares + $middlewares = Director::config()->uninherited('middlewares') ?: []; + + // Default handler - mo URL rules matched, so return a 404 error. + $handler = function () { + return new HTTPResponse('No URL rule was matched', 404); + }; + foreach ($rules as $pattern => $controllerOptions) { // Normalise route rule if (is_string($controllerOptions)) { @@ -361,6 +357,18 @@ class Director implements TemplateGlobalProvider } } + // Add controller-specific middlewares + if (isset($controllerOptions['Middlewares'])) { + // Force to array + if (!is_array($controllerOptions['Middlewares'])) { + $controllerOptions['Middlewares'] = [$controllerOptions['Middlewares']]; + } + $middlewares = array_merge($middlewares, $controllerOptions['Middlewares']); + } + + // Remove null middlewares (may be included due to limitatons of config yml) + $middlewares = array_filter($middlewares); + // Match pattern $arguments = $request->match($pattern, true); if ($arguments !== false) { @@ -373,28 +381,71 @@ class Director implements TemplateGlobalProvider $request->shift($controllerOptions['_PopTokeniser']); } - // Handle redirection + // Handler for redirection if (isset($arguments['Redirect'])) { - // Redirection - $response = new HTTPResponse(); - $response->redirect(static::absoluteURL($arguments['Redirect'])); - return $response; + $handler = function () use ($arguments) { + // Redirection + $response = new HTTPResponse(); + $response->redirect(static::absoluteURL($arguments['Redirect'])); + return $response; + }; + break; } // Find the controller name $controller = $arguments['Controller']; $controllerObj = Injector::inst()->create($controller); - try { - return $controllerObj->handleRequest($request); - } catch (HTTPResponse_Exception $responseException) { - return $responseException->getResponse(); - } + // Handler for calling a controller + $handler = function ($request) use ($controllerObj) { + try { + return $controllerObj->handleRequest($request); + } catch (HTTPResponse_Exception $responseException) { + return $responseException->getResponse(); + } + }; + break; } } - // No URL rules matched, so return a 404 error. - return new HTTPResponse('No URL rule was matched', 404); + // Call the handler with the given middlewares + return self::callWithMiddlewares( + $request, + $middlewares, + $handler + ); + } + + /** + * 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); } /** diff --git a/src/Control/RequestProcessor.php b/src/Control/RequestProcessor.php index a1e0cad1a..fa6d319ae 100644 --- a/src/Control/RequestProcessor.php +++ b/src/Control/RequestProcessor.php @@ -3,11 +3,13 @@ namespace SilverStripe\Control; use SilverStripe\Core\Injector\Injectable; +use SilverStripe\Dev\Deprecation; /** - * Represents a request processer that delegates pre and post request handling to nested request filters + * Middleware that provides back-support for the deprecated RequestFilter API. + * You should use HTTPMiddleware directly instead. */ -class RequestProcessor implements RequestFilter +class RequestProcessor implements HTTPMiddleware { use Injectable; @@ -33,25 +35,34 @@ class RequestProcessor implements RequestFilter $this->filters = $filters; } - public function preRequest(HTTPRequest $request) + /** + * @inheritdoc + */ + public function process(HTTPRequest $request, callable $delegate) { + if ($this->filters) { + Deprecation::notice( + '4.0', + 'Deprecated RequestFilters are in use. Apply HTTPMiddleware to Director.middlewares instead.' + ); + } + foreach ($this->filters as $filter) { $res = $filter->preRequest($request); if ($res === false) { - return false; + return new HTTPResponse(_t(__CLASS__.'.INVALID_REQUEST', 'Invalid request'), 400); } } - return null; - } - public function postRequest(HTTPRequest $request, HTTPResponse $response) - { + $response = $delegate($request); + foreach ($this->filters as $filter) { $res = $filter->postRequest($request, $response); if ($res === false) { - return false; + return new HTTPResponse(_t(__CLASS__ . '.REQUEST_ABORTED', 'Request aborted'), 500); } } - return null; + + return $response; } } From c482cd673ef121aa22180b9e4a647d031fcfa819 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 23 Jun 2017 11:43:19 +1200 Subject: [PATCH 03/18] DOC: Documentation and upgrade notes for director middleware --- .../02_Controllers/05_Middlewares.md | 99 +++++++++++++++++++ .../02_Controllers/05_RequestFilters.md | 57 ----------- docs/en/04_Changelogs/4.0.0.md | 4 +- 3 files changed, 102 insertions(+), 58 deletions(-) create mode 100644 docs/en/02_Developer_Guides/02_Controllers/05_Middlewares.md delete mode 100644 docs/en/02_Developer_Guides/02_Controllers/05_RequestFilters.md diff --git a/docs/en/02_Developer_Guides/02_Controllers/05_Middlewares.md b/docs/en/02_Developer_Guides/02_Controllers/05_Middlewares.md new file mode 100644 index 000000000..a97a5d2f6 --- /dev/null +++ b/docs/en/02_Developer_Guides/02_Controllers/05_Middlewares.md @@ -0,0 +1,99 @@ +title: HTTP Middlewares +summary: Create objects for modifying request and response objects across controllers. + +# HTTP Middlewares + +HTTP Middlewares allow you to put code that will run before or after. These might be used for +authentication, logging, caching, request processing, and many other purposes. Note this interface +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. + +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 +will deliberately return a different response, e.g. an error response or a redirection. + +**mysite/code/CustomMiddleware.php** + + :::php + getHeader('X-Special-Header') !== $this->Secret) { + return new HTTPResponse('You missed the special header', 400); + } + + // You can modify the request before + // For example, this might force JSON responses + $request->addHeader('Accept', 'application/json'); + + // If you want normal behaviour to occur, make sure you call $delegate($request) + $response = $delegate($request); + + // You can modify the response after it has been generated + $response->addHeader('X-Middleware-Applied', 'CustomMiddleware') + + // Don't forget to the return the response! + return $response; + } + } + +Once you have created your middleware class, you must attach it to the Director config to make +use of it. + +## Global middleware + +By adding the service or class name to the SilverStripe\Control\Director.middlewares array, a +middleware will be executed on every request: + +**mysite/_config/app.yml** + + :::yml + SilverStripe\Control\Director: + middlewares: + - %$CustomMiddleware + +Because these are service names, you can configure properties into a custom service if you would +like: + +**mysite/_config/app.yml** + + :::yml + SilverStripe\Control\Director: + middlewares: + - %$ConfiguredMiddleware + SilverStripe\Core\Injector\Injector: + ConfiguredMiddleware: + class: $CustomMiddleware + properties: + Secret: "DIFFERENT-ONE" + +## Route-specific middleware + +Alternatively, you can apply middlewares to a specific route. These will be processed after the +global middlewares. You do this by specifying the "Middlewares" property of the route rule: + +**mysite/_config/app.yml** + + :::yml + SilverStripe\Control\Director: + rules: + special\section: + Controller: SpecialSectionController + Middlewares: + - %$CustomMiddleware + +## API Documentation + +* [api:SilverStripe\Control\HTTPMiddleware] + diff --git a/docs/en/02_Developer_Guides/02_Controllers/05_RequestFilters.md b/docs/en/02_Developer_Guides/02_Controllers/05_RequestFilters.md deleted file mode 100644 index 4761064da..000000000 --- a/docs/en/02_Developer_Guides/02_Controllers/05_RequestFilters.md +++ /dev/null @@ -1,57 +0,0 @@ -title: Request Filters -summary: Create objects for modifying request and response objects across controllers. - -# Request Filters - -[api:RequestFilter] is an interface that provides two key methods. `preRequest` and `postRequest`. These methods are -executed before and after a request occurs to give developers a hook to modify any global state, add request tracking or -perform operations wrapped around responses and request objects. A `RequestFilter` is defined as: - -**mysite/code/CustomRequestFilter.php** - - :::php - General and Core Removed API From 95a266c6b97c82588a3342eebfac61fc23932891 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 23 Jun 2017 12:20:39 +1200 Subject: [PATCH 04/18] FIX: Add tests for middleware --- tests/php/Control/DirectorTest.php | 74 ++++++++++++++++++- .../Control/DirectorTest/TestMiddleware.php | 40 ++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 tests/php/Control/DirectorTest/TestMiddleware.php diff --git a/tests/php/Control/DirectorTest.php b/tests/php/Control/DirectorTest.php index cb2e94b62..a7095cf1a 100644 --- a/tests/php/Control/DirectorTest.php +++ b/tests/php/Control/DirectorTest.php @@ -1,5 +1,4 @@ assertEquals(2, $filter->postCalls); } + + public function testGlobalMiddleware() + { + $middleware = new DirectorTest\TestMiddleware; + + Injector::inst()->registerService($middleware, 'Middleware1'); + Config::modify()->set(Director::class, 'middlewares', [ 'Middleware1' ]); + + $response = Director::test('some-dummy-url'); + $this->assertEquals(404, $response->getStatusCode()); + + // Both triggered + $this->assertEquals(1, $middleware->preCalls); + $this->assertEquals(1, $middleware->postCalls); + + $middleware->failPost = true; + + $response = Director::test('some-dummy-url'); + $this->assertEquals(500, $response->getStatusCode()); + + // Both triggered + $this->assertEquals(2, $middleware->preCalls); + $this->assertEquals(2, $middleware->postCalls); + + $middleware->failPre = true; + + $response = Director::test('some-dummy-url'); + $this->assertEquals(400, $response->getStatusCode()); + + // Pre triggered, post not + $this->assertEquals(3, $middleware->preCalls); + $this->assertEquals(2, $middleware->postCalls); + } + + public function testRouteSpecificMiddleware() + { + $middleware = new DirectorTest\TestMiddleware; + $specificMiddleware = new DirectorTest\TestMiddleware; + + Injector::inst()->registerService($middleware, 'Middleware1'); + Injector::inst()->registerService($specificMiddleware, 'Middleware2'); + + // Global middleware + Config::modify()->set(Director::class, 'middlewares', [ 'Middleware1' ]); + + // URL rules, one of which has a specific middleware + Config::modify()->set( + Director::class, + 'rules', + [ + 'url-one' => TestController::class, + 'url-two' => [ + 'Controller' => TestController::class, + 'Middlewares' => [ 'Middleware2' ] + ] + ] + ); + + // URL without a route-specific middleware + $response = Director::test('url-one'); + + // Only the global middleware triggered + $this->assertEquals(1, $middleware->preCalls); + $this->assertEquals(0, $specificMiddleware->postCalls); + + $response = 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/TestMiddleware.php b/tests/php/Control/DirectorTest/TestMiddleware.php new file mode 100644 index 000000000..d26e4a727 --- /dev/null +++ b/tests/php/Control/DirectorTest/TestMiddleware.php @@ -0,0 +1,40 @@ +preCalls++; + if ($this->failPre) { + return new HTTPResponse('Fail pre', 400); + } + + $response = $delegate($request); + + $this->postCalls++; + if ($this->failPost) { + return new HTTPResponse('Fail post', 500); + } + + return $response; + } + + public function reset() + { + $this->preCalls = 0; + $this->postCalls = 0; + } +} From e85562289045cdc7c5c0c619811444e4b1f4dcd9 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 23 Jun 2017 12:29:11 +1200 Subject: [PATCH 05/18] NEW: Replace FlushRequestFilter with FlushMiddleware --- .upgrade.yml | 6 +++--- _config/requestprocessors.yml | 11 ++++------- _config/routes.yml | 2 -- .../16_Execution_Pipeline/01_Flushable.md | 2 +- .../{FlushRequestFilter.php => FlushMiddleware.php} | 13 ++++++------- src/Core/Flushable.php | 2 +- ...equestFilterTest.php => FlushMiddlewareTest.php} | 4 ++-- .../TestFlushable.php | 2 +- 8 files changed, 18 insertions(+), 24 deletions(-) rename src/Control/{FlushRequestFilter.php => FlushMiddleware.php} (62%) rename tests/php/Control/{FlushRequestFilterTest.php => FlushMiddlewareTest.php} (74%) rename tests/php/Control/{FlushRequestFilterTest => FlushMiddlewareTest}/TestFlushable.php (79%) diff --git a/.upgrade.yml b/.upgrade.yml index 74caf9ebc..c04ad9e26 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\FlushRequestFilter + FlushRequestFilter: SilverStripe\Control\FlushMiddleware HTTP: SilverStripe\Control\HTTP SS_HTTPRequest: SilverStripe\Control\HTTPRequest SS_HTTPResponse: SilverStripe\Control\HTTPResponse @@ -496,8 +496,8 @@ mappings: TestRequestFilter: SilverStripe\Control\Tests\DirectorTest\TestRequestFilter DirectorTestRequest_Controller: SilverStripe\Control\Tests\DirectorTest\TestController FakeController: SilverStripe\Control\Tests\FakeController - FlushRequestFilterTest: SilverStripe\Control\Tests\FlushRequestFilterTest - FlushRequestFilterTest_Flushable: SilverStripe\Control\Tests\FlushRequestFilterTest\TestFlushable + FlushRequestFilterTest: SilverStripe\Control\Tests\FlushMiddlewareTest + FlushRequestFilterTest_Flushable: SilverStripe\Control\Tests\FlushMiddlewareTest\TestFlushable HTTPRequestTest: SilverStripe\Control\Tests\HTTPRequestTest HTTPResponseTest: SilverStripe\Control\Tests\HTTPResponseTest HTTPTest: SilverStripe\Control\Tests\HTTPTest diff --git a/_config/requestprocessors.yml b/_config/requestprocessors.yml index 66846879b..5c53964b1 100644 --- a/_config/requestprocessors.yml +++ b/_config/requestprocessors.yml @@ -1,11 +1,8 @@ --- Name: requestprocessors --- -SilverStripe\Core\Injector\Injector: - FlushRequestFilter: - class: SilverStripe\Control\FlushRequestFilter - SilverStripe\Control\RequestProcessor: - properties: - filters: - - '%$FlushRequestFilter' +SilverStripe\Control\Director: + middlewares: + RequestProcessor: 'SilverStripe\Control\RequestProcessor' + FlushMiddleware: '%$SilverStripe\Control\FlushMiddleware' diff --git a/_config/routes.yml b/_config/routes.yml index f15949722..86c35b233 100644 --- a/_config/routes.yml +++ b/_config/routes.yml @@ -2,8 +2,6 @@ Name: rootroutes --- SilverStripe\Control\Director: - middlewares: - RequestProcessor: 'SilverStripe\Control\RequestProcessor' rules: '': SilverStripe\Control\Controller --- diff --git a/docs/en/02_Developer_Guides/16_Execution_Pipeline/01_Flushable.md b/docs/en/02_Developer_Guides/16_Execution_Pipeline/01_Flushable.md index a6cd93302..c3b88d14c 100644 --- a/docs/en/02_Developer_Guides/16_Execution_Pipeline/01_Flushable.md +++ b/docs/en/02_Developer_Guides/16_Execution_Pipeline/01_Flushable.md @@ -6,7 +6,7 @@ summary: Allows a class to define it's own flush functionality. ## Introduction Allows a class to define it's own flush functionality, which is triggered when `flush=1` is requested in the URL. -[api:FlushRequestFilter] is run before a request is made, calling `flush()` statically on all +[api:FlushMiddleware] is run before a request is made, calling `flush()` statically on all implementors of [api:Flushable]. ## Usage diff --git a/src/Control/FlushRequestFilter.php b/src/Control/FlushMiddleware.php similarity index 62% rename from src/Control/FlushRequestFilter.php rename to src/Control/FlushMiddleware.php index 2f7c0a694..e29e4448a 100644 --- a/src/Control/FlushRequestFilter.php +++ b/src/Control/FlushMiddleware.php @@ -8,20 +8,19 @@ use SilverStripe\Core\ClassInfo; /** * Triggers a call to flush() on all implementors of Flushable. */ -class FlushRequestFilter implements RequestFilter +class FlushMiddleware implements HTTPMiddleware { - public function preRequest(HTTPRequest $request) + /** + * @inheritdoc + */ + public function process(HTTPRequest $request, callable $delegate) { if (array_key_exists('flush', $request->getVars())) { foreach (ClassInfo::implementorsOf(Flushable::class) as $class) { $class::flush(); } } - return true; - } - public function postRequest(HTTPRequest $request, HTTPResponse $response) - { - return true; + return $delegate($request); } } diff --git a/src/Core/Flushable.php b/src/Core/Flushable.php index 94b6674f8..fe605f398 100644 --- a/src/Core/Flushable.php +++ b/src/Core/Flushable.php @@ -14,7 +14,7 @@ interface Flushable * parameter has been set. Each class that implements Flushable implements * this function which looks after it's own specific flushing functionality. * - * @see FlushRequestFilter + * @see FlushMiddleware */ public static function flush(); } diff --git a/tests/php/Control/FlushRequestFilterTest.php b/tests/php/Control/FlushMiddlewareTest.php similarity index 74% rename from tests/php/Control/FlushRequestFilterTest.php rename to tests/php/Control/FlushMiddlewareTest.php index 8644cc014..df141f1dd 100644 --- a/tests/php/Control/FlushRequestFilterTest.php +++ b/tests/php/Control/FlushMiddlewareTest.php @@ -2,10 +2,10 @@ namespace SilverStripe\Control\Tests; -use SilverStripe\Control\Tests\FlushRequestFilterTest\TestFlushable; +use SilverStripe\Control\Tests\FlushMiddlewareTest\TestFlushable; use SilverStripe\Dev\FunctionalTest; -class FlushRequestFilterTest extends FunctionalTest +class FlushMiddlewareTest extends FunctionalTest { /** * Assert that classes that implement flushable are called diff --git a/tests/php/Control/FlushRequestFilterTest/TestFlushable.php b/tests/php/Control/FlushMiddlewareTest/TestFlushable.php similarity index 79% rename from tests/php/Control/FlushRequestFilterTest/TestFlushable.php rename to tests/php/Control/FlushMiddlewareTest/TestFlushable.php index c867696f0..7a28aa6ba 100644 --- a/tests/php/Control/FlushRequestFilterTest/TestFlushable.php +++ b/tests/php/Control/FlushMiddlewareTest/TestFlushable.php @@ -1,6 +1,6 @@ Date: Fri, 23 Jun 2017 12:32:43 +1200 Subject: [PATCH 06/18] NEW: Replace AuthenticationRequestFilter with AuthenticationMiddleware --- _config/security.yml | 9 ++-- ...ilter.php => AuthenticationMiddleware.php} | 41 +++++++------------ src/Security/Member.php | 2 +- 3 files changed, 20 insertions(+), 32 deletions(-) rename src/Security/{AuthenticationRequestFilter.php => AuthenticationMiddleware.php} (57%) diff --git a/_config/security.yml b/_config/security.yml index 8930f15bb..03dbf19ea 100644 --- a/_config/security.yml +++ b/_config/security.yml @@ -19,14 +19,13 @@ SilverStripe\Core\Injector\Injector: --- Name: coresecurity --- +SilverStripe\Control\Director: + middlewares: + - %$SilverStripe\Security\AuthenticationMiddleware SilverStripe\Core\Injector\Injector: - SilverStripe\Security\AuthenticationRequestFilter: + SilverStripe\Security\AuthenticationMiddleware: properties: AuthenticationHandler: %$SilverStripe\Security\AuthenticationHandler - SilverStripe\Control\RequestProcessor: - properties: - filters: - - %$SilverStripe\Security\AuthenticationRequestFilter SilverStripe\Security\Security: properties: Authenticators: diff --git a/src/Security/AuthenticationRequestFilter.php b/src/Security/AuthenticationMiddleware.php similarity index 57% rename from src/Security/AuthenticationRequestFilter.php rename to src/Security/AuthenticationMiddleware.php index c276e8174..bb427ea2d 100644 --- a/src/Security/AuthenticationRequestFilter.php +++ b/src/Security/AuthenticationMiddleware.php @@ -5,11 +5,11 @@ namespace SilverStripe\Security; use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; use SilverStripe\Control\HTTPResponse_Exception; -use SilverStripe\Control\RequestFilter; +use SilverStripe\Control\HTTPMiddleware; use SilverStripe\Core\Config\Configurable; use SilverStripe\ORM\ValidationException; -class AuthenticationRequestFilter implements RequestFilter +class AuthenticationMiddleware implements HTTPMiddleware { use Configurable; @@ -43,32 +43,21 @@ class AuthenticationRequestFilter implements RequestFilter * @return bool|void * @throws HTTPResponse_Exception */ - public function preRequest(HTTPRequest $request) + public function process(HTTPRequest $request, callable $delegate) { - if (!Security::database_is_ready()) { - return; + if (Security::database_is_ready()) { + try { + $this + ->getAuthenticationHandler() + ->authenticateRequest($request); + } catch (ValidationException $e) { + return new HTTPResponse( + "Bad log-in details: " . $e->getMessage(), + 400 + ); + } } - try { - $this - ->getAuthenticationHandler() - ->authenticateRequest($request); - } catch (ValidationException $e) { - throw new HTTPResponse_Exception( - "Bad log-in details: " . $e->getMessage(), - 400 - ); - } + return $delegate($request); } - - /** - * No-op - * - * @param HTTPRequest $request - * @param HTTPResponse $response - * @return bool|void - */ - public function postRequest(HTTPRequest $request, HTTPResponse $response) - { } -} diff --git a/src/Security/Member.php b/src/Security/Member.php index ac778ecba..30cac32d6 100644 --- a/src/Security/Member.php +++ b/src/Security/Member.php @@ -415,7 +415,7 @@ class Member extends DataObject */ public function beforeMemberLoggedIn() { - // @todo Move to middleware on the AuthenticationRequestFilter IdentityStore + // @todo Move to middleware on the AuthenticationMiddleware IdentityStore $this->extend('beforeMemberLoggedIn'); } From db080c0603a14a4a134a83400d46cbf34d19bae5 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 23 Jun 2017 14:45:54 +1200 Subject: [PATCH 07/18] NEW: Move session activation to SessionMiddleware. --- _config/requestprocessors.yml | 1 + src/Control/Director.php | 8 +------- src/Control/HTTPApplication.php | 2 -- src/Control/SessionMiddleware.php | 28 ++++++++++++++++++++++++++++ 4 files changed, 30 insertions(+), 9 deletions(-) create mode 100644 src/Control/SessionMiddleware.php diff --git a/_config/requestprocessors.yml b/_config/requestprocessors.yml index 5c53964b1..8903a665d 100644 --- a/_config/requestprocessors.yml +++ b/_config/requestprocessors.yml @@ -3,6 +3,7 @@ Name: requestprocessors --- SilverStripe\Control\Director: middlewares: + SessionMiddleware: 'SilverStripe\Control\SessionMiddleware' RequestProcessor: 'SilverStripe\Control\RequestProcessor' FlushMiddleware: '%$SilverStripe\Control\FlushMiddleware' diff --git a/src/Control/Director.php b/src/Control/Director.php index 59fb66efd..1042f7e7e 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -132,13 +132,7 @@ class Director implements TemplateGlobalProvider } // Generate output - $result = static::handleRequest($request); - - // Save session data. Note that save() will start/resume the session if required. - $request->getSession()->save(); - - // Return - return $result; + return static::handleRequest($request); } /** diff --git a/src/Control/HTTPApplication.php b/src/Control/HTTPApplication.php index f3610f113..ed545a34c 100644 --- a/src/Control/HTTPApplication.php +++ b/src/Control/HTTPApplication.php @@ -96,8 +96,6 @@ class HTTPApplication implements Application // Ensure boot is invoked return $this->execute($request, function (HTTPRequest $request) { - // Start session and execute - $request->getSession()->init(); return Director::direct($request); }, $flush); } diff --git a/src/Control/SessionMiddleware.php b/src/Control/SessionMiddleware.php new file mode 100644 index 000000000..bae7f9644 --- /dev/null +++ b/src/Control/SessionMiddleware.php @@ -0,0 +1,28 @@ +getSession()->init(); + + // Generate output + $response = $delegate($request); + + // Save session data, even if there was an exception. + // Note that save() will start/resume the session if required. + } finally { + $request->getSession()->save(); + } + + return $response; + } +} From 72a7655e95f89aa63fe9ed7b3bce3d7fd32cd989 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 23 Jun 2017 15:08:08 +1200 Subject: [PATCH 08/18] NEW: Moved allowed-hosts checking to a middleware. --- _config/requestprocessors.yml | 6 ++++ src/Control/AllowedHostsMiddleware.php | 45 ++++++++++++++++++++++++++ src/Control/Director.php | 8 ----- 3 files changed, 51 insertions(+), 8 deletions(-) create mode 100644 src/Control/AllowedHostsMiddleware.php diff --git a/_config/requestprocessors.yml b/_config/requestprocessors.yml index 8903a665d..c0bcbb9f2 100644 --- a/_config/requestprocessors.yml +++ b/_config/requestprocessors.yml @@ -3,7 +3,13 @@ Name: requestprocessors --- SilverStripe\Control\Director: middlewares: + AllowedHostsMiddleware: '%$SilverStripe\Control\AllowedHostsMiddleware' SessionMiddleware: 'SilverStripe\Control\SessionMiddleware' RequestProcessor: 'SilverStripe\Control\RequestProcessor' FlushMiddleware: '%$SilverStripe\Control\FlushMiddleware' + +SilverStripe\Core\Injector\Injector: + SilverStripe\Control\AllowedHostsMiddleware: + properties: + AllowedHosts: "`SS_ALLOWED_HOSTS`" diff --git a/src/Control/AllowedHostsMiddleware.php b/src/Control/AllowedHostsMiddleware.php new file mode 100644 index 000000000..533481762 --- /dev/null +++ b/src/Control/AllowedHostsMiddleware.php @@ -0,0 +1,45 @@ +allowedHosts; + } + + /** + * @param $allowedHosts string A comma-separted list of allowed Host header values + */ + public function setAllowedHosts($allowedHosts) + { + $this->allowedHosts = $allowedHosts; + } + + /** + * @inheritdoc + */ + public function process(HTTPRequest $request, callable $delegate) + { + if ($this->allowedHosts && !Director::is_cli()) { + $allowedHosts = preg_split('/ *, */', $this->allowedHosts); + + // check allowed hosts + if (!in_array($request->getHeader('Host'), $allowedHosts)) { + return new HTTPResponse('Invalid Host', 400); + } + } + + return $delegate($request); + } +} diff --git a/src/Control/Director.php b/src/Control/Director.php index 1042f7e7e..081d1ef42 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -123,14 +123,6 @@ class Director implements TemplateGlobalProvider */ public static function direct(HTTPRequest $request) { - // check allowed hosts - if (getenv('SS_ALLOWED_HOSTS') && !static::is_cli()) { - $allowedHosts = explode(',', getenv('SS_ALLOWED_HOSTS')); - if (!in_array(static::host(), $allowedHosts)) { - return new HTTPResponse('Invalid Host', 400); - } - } - // Generate output return static::handleRequest($request); } From 10866c0809623426e875c335ee9175e11935f98c Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 23 Jun 2017 15:12:15 +1200 Subject: [PATCH 09/18] API: Replace Director::direct() with Director::handleRequest(). There was no longer any code in direct() and so I opted to expose the handleRequest() method instead. --- src/Control/Director.php | 57 +++++++++++---------------------- src/Control/HTTPApplication.php | 2 +- src/Control/HTTPRequest.php | 4 +-- src/Forms/Form.php | 2 +- 4 files changed, 23 insertions(+), 42 deletions(-) diff --git a/src/Control/Director.php b/src/Control/Director.php index 081d1ef42..a44f6198d 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -16,13 +16,13 @@ use SilverStripe\View\TemplateGlobalProvider; /** * Director is responsible for processing URLs, and providing environment information. * - * The most important part of director is {@link Director::direct()}, which is passed a URL and will + * The most important part of director is {@link Director::handleRequest()}, which is passed an HTTPRequest and will * execute the appropriate controller. * * Director also has a number of static methods that provide information about the environment, such as * {@link Director::$environment_type}. * - * @see Director::direct() + * @see Director::handleRequest() * @see Director::$rules * @see Director::$environment_type */ @@ -100,40 +100,10 @@ class Director implements TemplateGlobalProvider protected static $environment_type; /** - * Process the given URL, creating the appropriate controller and executing it. - * - * Request processing is handled as follows: - * - Director::direct() creates a new HTTPResponse object and passes this to - * Director::handleRequest(). - * - Director::handleRequest($request) checks each of the Director rules and identifies a controller - * to handle this request. - * - Controller::handleRequest($request) is then called. This will find a rule to handle the URL, - * and call the rule handling method. - * - RequestHandler::handleRequest($request) is recursively called whenever a rule handling method - * returns a RequestHandler object. - * - * In addition to request processing, Director will manage the session, and perform the output of - * the actual response to the browser. - * - * @uses handleRequest() rule-lookup logic is handled by this. - * @uses TestController::handleRequest() This handles the page logic for a Director::direct() call. - * @param HTTPRequest $request - * @return HTTPResponse - * @throws HTTPResponse_Exception - */ - public static function direct(HTTPRequest $request) - { - // Generate output - return static::handleRequest($request); - } - - /** - * Test a URL request, returning a response object. This method is the counterpart of - * Director::direct() that is used in functional testing. It will execute the URL given, and + * Test a URL request, returning a response object. This method is a wrapper around + * Director::handleRequest() to assist with functional testing. It will execute the URL given, and * return the result as an HTTPResponse object. * - * @uses TestController::handleRequest() Handles the page logic for a Director::direct() call. - * * @param string $url The URL to visit. * @param array $postVars The $_POST & $_FILES variables. * @param array|Session $session The {@link Session} object representing the current session. @@ -162,7 +132,7 @@ class Director implements TemplateGlobalProvider ) { return static::mockRequest( function (HTTPRequest $request) { - return static::direct($request); + return static::handleRequest($request); }, $url, $postVars, @@ -315,13 +285,24 @@ class Director implements TemplateGlobalProvider } /** - * Handle an HTTP request, defined with a HTTPRequest object. + * Process the given URL, creating the appropriate controller and executing it. + * + * Request processing is handled as follows: + * - Director::handleRequest($request) checks each of the Director rules and identifies a controller + * to handle this request. + * - Controller::handleRequest($request) is then called. This will find a rule to handle the URL, + * and call the rule handling method. + * - RequestHandler::handleRequest($request) is recursively called whenever a rule handling method + * returns a RequestHandler object. + * + * In addition to request processing, Director will manage the session, and perform the output of + * the actual response to the browser. * - * @skipUpgrade * @param HTTPRequest $request * @return HTTPResponse + * @throws HTTPResponse_Exception */ - protected static function handleRequest(HTTPRequest $request) + public static function handleRequest(HTTPRequest $request) { $rules = Director::config()->uninherited('rules'); diff --git a/src/Control/HTTPApplication.php b/src/Control/HTTPApplication.php index ed545a34c..51b386ca1 100644 --- a/src/Control/HTTPApplication.php +++ b/src/Control/HTTPApplication.php @@ -96,7 +96,7 @@ class HTTPApplication implements Application // Ensure boot is invoked return $this->execute($request, function (HTTPRequest $request) { - return Director::direct($request); + return Director::handleRequest($request); }, $flush); } diff --git a/src/Control/HTTPRequest.php b/src/Control/HTTPRequest.php index 72dc820a9..95fd02384 100644 --- a/src/Control/HTTPRequest.php +++ b/src/Control/HTTPRequest.php @@ -846,7 +846,7 @@ class HTTPRequest implements ArrayAccess * Using GET for the "_method" override is not supported, * as GET should never carry out state changes. * Alternatively you can use a custom HTTP header 'X-HTTP-Method-Override' - * to override the original method in {@link Director::direct()}. + * to override the original method. * The '_method' POST parameter overrules the custom HTTP header. * * @param string $origMethod Original HTTP method from the browser request @@ -857,7 +857,7 @@ class HTTPRequest implements ArrayAccess { if (isset($postVars['_method'])) { if (!in_array(strtoupper($postVars['_method']), array('GET','POST','PUT','DELETE','HEAD'))) { - user_error('Director::direct(): Invalid "_method" parameter', E_USER_ERROR); + user_error('HTTPRequest::detect_method(): Invalid "_method" parameter', E_USER_ERROR); } return strtoupper($postVars['_method']); } else { diff --git a/src/Forms/Form.php b/src/Forms/Form.php index c1bb1dc8b..a7254fd98 100644 --- a/src/Forms/Form.php +++ b/src/Forms/Form.php @@ -1032,7 +1032,7 @@ class Form extends ViewableData implements HasRequestHandler * As most browsers only support GET and POST in * form submissions, all other HTTP methods are * added as a hidden field "_method" that - * gets evaluated in {@link Director::direct()}. + * gets evaluated in {@link HTTPRequest::detect_method()}. * See {@link FormMethod()} to get a HTTP method * for safe insertion into a
tag. * From 4d89daac786ac501a4c651d4bbe62d8eac701388 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 23 Jun 2017 15:34:51 +1200 Subject: [PATCH 10/18] NEW: Register Injector::inst()->get(HTTPRequest) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HTTPRequest is provided as a service so that global references for session, hostname, etc can be facilitated. It’s a bit of a hack and should be avoided but we’re unlikely to scrub it completely from the Silverstripe 4 code. --- src/Control/Director.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Control/Director.php b/src/Control/Director.php index a44f6198d..0dc6a1d82 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -304,6 +304,8 @@ class Director implements TemplateGlobalProvider */ public static function handleRequest(HTTPRequest $request) { + Injector::inst()->registerService($request, HTTPRequest::class); + $rules = Director::config()->uninherited('rules'); // Get global middlewares @@ -376,11 +378,15 @@ class Director implements TemplateGlobalProvider } // Call the handler with the given middlewares - return self::callWithMiddlewares( + $response = self::callWithMiddlewares( $request, $middlewares, $handler ); + + Injector::inst()->unregisterNamedObject(HTTPRequest::class); + + return $response; } /** From c4d038f20dfcba582c7ab35b596b106ae0938075 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 23 Jun 2017 15:47:40 +1200 Subject: [PATCH 11/18] NEW: Add HTTPRequest::getScheme()/setScheme() NEW: Add HTTPRequest::setIP() API: Rely on HTTPRequestBuilder to set scheme and IP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These changes tidy up HTTPRequest making it a container for information and removing special logic from it. This makes it less feature-rich: it doesn’t contain trusted-proxy logic. This will be able to provided by a middleware. The new getScheme() method is designed to be closish to PSR-7’s getUri()->getScheme() equivalent. There are no more direct $_SERVER references in HTTPRequest. --- src/Control/HTTPRequest.php | 68 ++++++++++++++++++++---------- src/Control/HTTPRequestBuilder.php | 11 +++++ 2 files changed, 56 insertions(+), 23 deletions(-) diff --git a/src/Control/HTTPRequest.php b/src/Control/HTTPRequest.php index 95fd02384..1a23308ef 100644 --- a/src/Control/HTTPRequest.php +++ b/src/Control/HTTPRequest.php @@ -52,6 +52,20 @@ class HTTPRequest implements ArrayAccess */ protected $httpMethod; + /** + * The URL scheme in lowercase: http or https + * + * @var string + */ + protected $scheme; + + /** + * The client IP address + * + * @var string + */ + protected $ip; + /** * Contains alls HTTP GET parameters passed into this request. * @@ -146,6 +160,7 @@ class HTTPRequest implements ArrayAccess $this->getVars = (array) $getVars; $this->postVars = (array) $postVars; $this->body = $body; + $this->scheme = "http"; } /** @@ -757,35 +772,23 @@ class HTTPRequest implements ArrayAccess } /** - * Returns the client IP address which - * originated this request. + * Returns the client IP address which originated this request. * * @return string */ public function getIP() { - $headerOverrideIP = null; - if (TRUSTED_PROXY) { - $headers = (getenv('SS_TRUSTED_PROXY_IP_HEADER')) ? array(getenv('SS_TRUSTED_PROXY_IP_HEADER')) : null; - if (!$headers) { - // Backwards compatible defaults - $headers = array('HTTP_CLIENT_IP', 'HTTP_X_FORWARDED_FOR'); - } - foreach ($headers as $header) { - if (!empty($_SERVER[$header])) { - $headerOverrideIP = $_SERVER[$header]; - break; - } - } - } + return $this->ip; + } - if ($headerOverrideIP && filter_var($headerOverrideIP, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) { - return $this->getIPFromHeaderValue($headerOverrideIP); - } elseif (isset($_SERVER['REMOTE_ADDR'])) { - return $_SERVER['REMOTE_ADDR']; - } else { - return null; - } + /** + * Sets the client IP address which originated this request. + * + * @param $ip string + */ + public function setIP($ip) + { + $this->ip = $ip; } /** @@ -837,6 +840,25 @@ class HTTPRequest implements ArrayAccess return $this->httpMethod; } + /** + * Return the URL scheme (e.g. "http" or "https"). + * Equivalent to PSR-7 getUri()->getScheme() + * @return string + */ + public function getScheme() { + return $this->scheme; + } + + /** + * Set the URL scheme (e.g. "http" or "https"). + * Equivalent to PSR-7 getUri()->getScheme(), + * + * @param string $scheme + */ + public function setScheme($scheme) { + $this->scheme = $scheme; + } + /** * Gets the "real" HTTP method for a request. * diff --git a/src/Control/HTTPRequestBuilder.php b/src/Control/HTTPRequestBuilder.php index a30657c72..c1d039cdc 100644 --- a/src/Control/HTTPRequestBuilder.php +++ b/src/Control/HTTPRequestBuilder.php @@ -45,6 +45,17 @@ class HTTPRequestBuilder $input ); + // Set the scheme to HTTPS if needed + if ((!empty($variables['_SERVER']['HTTPS']) && $variables['_SERVER']['HTTPS'] != 'off') + || isset($variables['_SERVER']['SSL'])) { + $request->setScheme('https'); + } + + // Set the client IP + if (!empty($variables['_SERVER']['REMOTE_ADDR'])) { + $request->setIP($variables['_SERVER']['REMOTE_ADDR']); + } + // Add headers $headers = static::extractRequestHeaders($variables['_SERVER']); foreach ($headers as $header => $value) { From ccc86306b6a0d6eb9bbb8b41b209b8595393d591 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 23 Jun 2017 17:28:04 +1200 Subject: [PATCH 12/18] =?UTF-8?q?NEW:=20Add=20TrustedProxyMiddleware=20API?= =?UTF-8?q?:=20SS=5FTRUSTED=5FPROXY=5FHOST=5FHEADER=20replace=20with=20mid?= =?UTF-8?q?dleware=20config=20API:=20SS=5FTRUSTED=5FPROXY=5FPROTOCOL=5FHEA?= =?UTF-8?q?DER=20replace=20with=20middleware=20config=20API:=20SS=5FTRUSTE?= =?UTF-8?q?D=5FPROXY=5FIP=5FHEADER=20replace=20with=20middleware=20config?= =?UTF-8?q?=20API:=20Front-End-Https=20=3D=20=E2=80=9Con=E2=80=9D=20header?= =?UTF-8?q?=20no=20longer=20supported?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This middleware replaces the TRUSTED_PROXY setting and shifts its configuration out of the env vars and bootstrap and into the Director flow. --- _config/requestprocessors.yml | 4 + .../03_Environment_Management.md | 3 - .../09_Security/04_Secure_Coding.md | 12 ++ docs/en/04_Changelogs/4.0.0.md | 3 + src/Control/Director.php | 41 +--- src/Control/HTTPRequest.php | 23 --- src/Control/TrustedProxyMiddleware.php | 191 ++++++++++++++++++ src/includes/constants.php | 21 -- 8 files changed, 219 insertions(+), 79 deletions(-) create mode 100644 src/Control/TrustedProxyMiddleware.php diff --git a/_config/requestprocessors.yml b/_config/requestprocessors.yml index c0bcbb9f2..280eb1e6f 100644 --- a/_config/requestprocessors.yml +++ b/_config/requestprocessors.yml @@ -3,6 +3,7 @@ Name: requestprocessors --- SilverStripe\Control\Director: middlewares: + TrustedProxyMiddleware: '%$SilverStripe\Control\TrustedProxyMiddleware' AllowedHostsMiddleware: '%$SilverStripe\Control\AllowedHostsMiddleware' SessionMiddleware: 'SilverStripe\Control\SessionMiddleware' RequestProcessor: 'SilverStripe\Control\RequestProcessor' @@ -13,3 +14,6 @@ SilverStripe\Core\Injector\Injector: SilverStripe\Control\AllowedHostsMiddleware: properties: AllowedHosts: "`SS_ALLOWED_HOSTS`" + SilverStripe\Control\TrustedProxyMiddleware: + properties: + TrustedProxyIPs: "`SS_TRUSTED_PROXY_IPS`" diff --git a/docs/en/00_Getting_Started/03_Environment_Management.md b/docs/en/00_Getting_Started/03_Environment_Management.md index 4af8b9d7e..b56a8bcd6 100644 --- a/docs/en/00_Getting_Started/03_Environment_Management.md +++ b/docs/en/00_Getting_Started/03_Environment_Management.md @@ -80,9 +80,6 @@ SilverStripe core environment variables are listed here, though you're free to d | `SS_ERROR_LOG` | Relative path to the log file. | | `SS_PROTECTED_ASSETS_PATH` | Path to secured assets - defaults to ASSET_PATH/.protected | | `SS_DATABASE_MEMORY` | Used for SQLite3 DBs | -| `SS_TRUSTED_PROXY_PROTOCOL_HEADER` | Used to define the proxy header to be used to determine HTTPS status | -| `SS_TRUSTED_PROXY_IP_HEADER` | Used to define the proxy header to be used to determine request IPs | -| `SS_TRUSTED_PROXY_HOST_HEADER` | Used to define the proxy header to be used to determine the requested host name | | `SS_TRUSTED_PROXY_IPS` | IP address or CIDR range to trust proxy headers from. If left blank no proxy headers are trusted. Can be set to 'none' (trust none) or '*' (trust all) | | `SS_ALLOWED_HOSTS` | A comma deliminated list of hostnames the site is allowed to respond to | | `SS_MANIFESTCACHE` | The manifest cache to use (defaults to file based caching). Must be a CacheInterface or CacheFactory class name | diff --git a/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md b/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md index 0fad9f2b4..bf583f18f 100644 --- a/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md +++ b/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md @@ -556,6 +556,18 @@ In order to prevent this kind of attack, it's necessary to whitelist trusted pro server IPs using the SS_TRUSTED_PROXY_IPS define in your `.env`. SS_TRUSTED_PROXY_IPS="127.0.0.1,192.168.0.1" + +If you wish to change the headers that are used to find the proxy information, you should reconfigure the +TrustedProxyMiddleware service: + + :::yml + SilverStripe\Control\TrustedProxyMiddleware: + properties: + ProxyHostHeaders: X-Forwarded-Host + ProxySchemeHeaders: X-Forwarded-Protocol + ProxyIPHeaders: X-Forwarded-Ip + + SS_TRUSTED_PROXY_HOST_HEADER="HTTP_X_FORWARDED_HOST" SS_TRUSTED_PROXY_IP_HEADER="HTTP_X_FORWARDED_FOR" SS_TRUSTED_PROXY_PROTOCOL_HEADER="HTTP_X_FORWARDED_PROTOCOL" diff --git a/docs/en/04_Changelogs/4.0.0.md b/docs/en/04_Changelogs/4.0.0.md index 014ef24f7..b24948bb2 100644 --- a/docs/en/04_Changelogs/4.0.0.md +++ b/docs/en/04_Changelogs/4.0.0.md @@ -1364,6 +1364,9 @@ After (`mysite/_config/config.yml`): * 'BlockUntrustedIPS' env setting has been removed. All IPs are untrusted unless `SS_TRUSTED_PROXY_IPS` is set to '*' See [Environment Management docs](/getting-started/environment_management/) for full details. + * `SS_TRUSTED_PROXY_HOST_HEADER`, `SS_TRUSTED_PROXY_PROTOCOL_HEADER`, and `SS_TRUSTED_PROXY_IP_HEADER` + are no longer supported. These settings should go into the Injector service configuration for + TrustedProxyMiddleware instead. * `MODULES_PATH` removed * `MODULES_DIR` removed * `SS_HOST` removed. Use `SS_BASE_URL` instead. diff --git a/src/Control/Director.php b/src/Control/Director.php index 0dc6a1d82..a0bd5b930 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -384,6 +384,8 @@ class Director implements TemplateGlobalProvider $handler ); + // 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); return $response; @@ -518,18 +520,9 @@ class Director implements TemplateGlobalProvider } } - // Validate proxy-specific headers - if (TRUSTED_PROXY) { - // Check headers to validate - $headers = getenv('SS_TRUSTED_PROXY_HOST_HEADER') - ? explode(',', getenv('SS_TRUSTED_PROXY_HOST_HEADER')) - : ['HTTP_X_FORWARDED_HOST']; // Backwards compatible defaults - foreach ($headers as $header) { - if (!empty($_SERVER[$header])) { - // Get the first host, in case there's multiple separated through commas - return strtok($_SERVER[$header], ','); - } - } + $request = Injector::inst()->get(HTTPRequest::class); + if ($request && $host = $request->getHeader('Host')) { + return $host; } // Check given header @@ -587,26 +580,10 @@ class Director implements TemplateGlobalProvider } } - // See https://en.wikipedia.org/wiki/List_of_HTTP_header_fields - // See https://support.microsoft.com/en-us/kb/307347 - if (TRUSTED_PROXY) { - $headers = getenv('SS_TRUSTED_PROXY_PROTOCOL_HEADER') - ? explode(',', getenv('SS_TRUSTED_PROXY_PROTOCOL_HEADER')) - : ['HTTP_X_FORWARDED_PROTO', 'HTTP_X_FORWARDED_PROTOCOL', 'HTTP_FRONT_END_HTTPS']; - foreach ($headers as $header) { - $headerCompareVal = ($header === 'HTTP_FRONT_END_HTTPS' ? 'on' : 'https'); - if (!empty($_SERVER[$header]) && strtolower($_SERVER[$header]) == $headerCompareVal) { - return true; - } - } - } - - // Check common $_SERVER - if ((!empty($_SERVER['HTTPS']) && $_SERVER['HTTPS'] != 'off')) { - return true; - } - if (isset($_SERVER['SSL'])) { - return true; + // Check the current request + $request = Injector::inst()->get(HTTPRequest::class); + if ($request && $host = $request->getHeader('Host')) { + return $request->getScheme() === 'https'; } // Check default_base_url diff --git a/src/Control/HTTPRequest.php b/src/Control/HTTPRequest.php index 1a23308ef..b03369df1 100644 --- a/src/Control/HTTPRequest.php +++ b/src/Control/HTTPRequest.php @@ -791,29 +791,6 @@ class HTTPRequest implements ArrayAccess $this->ip = $ip; } - /** - * Extract an IP address from a header value that has been obtained. Accepts single IP or comma separated string of - * IPs - * - * @param string $headerValue The value from a trusted header - * @return string The IP address - */ - 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); - foreach ($ips as $ip) { - $ip = trim($ip); - if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE)) { - return $ip; - } - } - } - return $headerValue; - } - /** * Returns all mimetypes from the HTTP "Accept" header * as an array. diff --git a/src/Control/TrustedProxyMiddleware.php b/src/Control/TrustedProxyMiddleware.php new file mode 100644 index 000000000..fc74b0c12 --- /dev/null +++ b/src/Control/TrustedProxyMiddleware.php @@ -0,0 +1,191 @@ +trustedProxyIPs; + } + + /** + * Set the comma-separated list of IP ranges that are trusted to provide proxy headers + * + * @param $trustedProxyIPs string + */ + public function setTrustedProxyIPs($trustedProxyIPs) + { + $this->trustedProxyIPs = $trustedProxyIPs; + } + + /** + * Return the comma-separated list of headers from which to lookup the hostname + * + * @return string + */ + public function getProxyHostHeaders() + { + return $this->proxyHostHeaders; + } + + /** + * Set the comma-separated list of headers from which to lookup the hostname + * + * @param $proxyHostHeaders string + */ + public function setProxyHostHeaders($proxyHostHeaders) + { + $this->proxyHostHeaders = $proxyHostHeaders; + } + + /** + * Return the comma-separated list of headers from which to lookup the client IP + * + * @return string + */ + public function getProxyIPHeaders() + { + return $this->proxyIPHeaders; + } + + /** + * Set the comma-separated list of headers from which to lookup the client IP + * + * @param $proxyIPHeaders string + */ + public function setProxyIPHeaders($proxyIPHeaders) + { + $this->proxyIPHeaders = $proxyIPHeaders; + } + + /** + * Return the comma-separated list of headers from which to lookup the client scheme (http/https) + * + * @return string + */ + public function getProxySchemeHeaders() + { + return $this->proxySchemeHeaders; + } + + /** + * Set the comma-separated list of headers from which to lookup the client scheme (http/https) + * + * @param $proxySchemeHeaders string + */ + public function setProxySchemeHeaders($proxySchemeHeaders) + { + $this->proxySchemeHeaders = $proxySchemeHeaders; + } + + public function process(HTTPRequest $request, callable $delegate) + { + // If this is a trust proxy + if ($this->isTrustedProxy($request)) { + // Replace host + foreach ($this->proxyHostHeaders as $header) { + $hostList = $request->getHeader($header); + if ($hostList) { + $request->setHeader('Host', strtok($hostList, ',')); + break; + } + } + + // Replace scheme + foreach ($this->proxySchemeHeaders as $header) { + $scheme = $request->getHeader($header); + if ($scheme) { + $request->setScheme(strtolower($scheme)); + break; + } + } + + // Replace IP + foreach ($this->proxyIPHeaders as $header) { + $ipHeader = $this->getIPFromHeaderValue($request->getHeader($header)); + if ($ipHeader) { + $request->setIP($ipHeader); + break; + } + } + } + + return $delegate($request); + } + + /** + * Determine if the current request is coming from a trusted proxy + * + * @return boolean True if the request's source IP is a trusted proxy + */ + protected function isTrustedProxy($request) + { + // Disabled + if (empty($this->trustedProxyIPs) || $trustedIPs === 'none') { + return false; + } + + // Allow all + if ($trustedIPs === '*') { + return true; + } + + // Validate IP address + if ($ip = $request->getIP()) { + return IPUtils::checkIP($ip, explode(',', $trustedIPs)); + } + + return false; + } + + /** + * Extract an IP address from a header value that has been obtained. + * Accepts single IP or comma separated string of IPs + * + * @param string $headerValue The value from a trusted header + * @return string The IP address + */ + 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); + foreach ($ips as $ip) { + $ip = trim($ip); + if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)) { + return $ip; + } else { + return null; + } + } + } + return $headerValue; + } +} diff --git a/src/includes/constants.php b/src/includes/constants.php index 2eee0dacc..7adad4dd7 100644 --- a/src/includes/constants.php +++ b/src/includes/constants.php @@ -73,27 +73,6 @@ if (!getenv('SS_IGNORE_DOT_ENV')) { }); } -/** - * Validate whether the request comes directly from a trusted server or not - * This is necessary to validate whether or not the values of X-Forwarded- - * or Client-IP HTTP headers can be trusted - */ -if (!defined('TRUSTED_PROXY')) { - define('TRUSTED_PROXY', call_user_func(function () { - $trustedIPs = getenv('SS_TRUSTED_PROXY_IPS'); - if (empty($trustedIPs) || $trustedIPs === 'none') { - return false; - } - if ($trustedIPs === '*') { - return true; - } - // Validate IP address - if (isset($_SERVER['REMOTE_ADDR'])) { - return IPUtils::checkIP($_SERVER['REMOTE_ADDR'], explode(',', $trustedIPs)); - } - return false; - })); -} if (!defined('BASE_URL')) { define('BASE_URL', call_user_func(function () { From e92c63c5457bf56fe320d83977e3475c1fc60908 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Sun, 25 Jun 2017 14:13:36 +1200 Subject: [PATCH 13/18] API: Remove $sid argument of Session::start() NEW: Pass HTTPRequest to session NEW: Pass HTTPReuqest optionally to Director statics The session handler now expects to operate on a specific HTTPRequest object. --- src/Control/Director.php | 20 +++++++++------- src/Control/Session.php | 23 ++++++++----------- src/Control/SessionMiddleware.php | 4 ++-- .../Startup/ErrorControlChainMiddleware.php | 2 +- 4 files changed, 25 insertions(+), 24 deletions(-) diff --git a/src/Control/Director.php b/src/Control/Director.php index a0bd5b930..52a9dd8ff 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -509,7 +509,7 @@ class Director implements TemplateGlobalProvider * * @return string */ - public static function host() + public static function host(HTTPRequest $request = null) { // Check if overridden by alternate_base_url if ($baseURL = self::config()->get('alternate_base_url')) { @@ -520,7 +520,9 @@ class Director implements TemplateGlobalProvider } } - $request = Injector::inst()->get(HTTPRequest::class); + if (!$request) { + $request = Injector::inst()->get(HTTPRequest::class, true, ['GET', '/']); + } if ($request && $host = $request->getHeader('Host')) { return $host; } @@ -549,9 +551,9 @@ class Director implements TemplateGlobalProvider * * @return bool|string */ - public static function protocolAndHost() + public static function protocolAndHost(HTTPRequest $request = null) { - return static::protocol() . static::host(); + return static::protocol($request) . static::host($request); } /** @@ -559,9 +561,9 @@ class Director implements TemplateGlobalProvider * * @return string */ - public static function protocol() + public static function protocol(HTTPRequest $request = null) { - return (self::is_https()) ? 'https://' : 'http://'; + return (self::is_https($request)) ? 'https://' : 'http://'; } /** @@ -569,7 +571,7 @@ class Director implements TemplateGlobalProvider * * @return bool */ - public static function is_https() + public static function is_https(HTTPRequest $request = null) { // Check override from alternate_base_url if ($baseURL = self::config()->uninherited('alternate_base_url')) { @@ -581,7 +583,9 @@ class Director implements TemplateGlobalProvider } // Check the current request - $request = Injector::inst()->get(HTTPRequest::class); + if (!$request) { + $request = Injector::inst()->get(HTTPRequest::class, true, ['GET', '/']); + } if ($request && $host = $request->getHeader('Host')) { return $request->getScheme() === 'https'; } diff --git a/src/Control/Session.php b/src/Control/Session.php index bf5f3f230..d8f4473b8 100644 --- a/src/Control/Session.php +++ b/src/Control/Session.php @@ -172,10 +172,10 @@ class Session /** * Init this session instance before usage */ - public function init() + public function init(HTTPRequest $request) { if (!$this->isStarted()) { - $this->start(); + $this->start($request); } // Funny business detected! @@ -183,7 +183,7 @@ class Session if ($this->data['HTTP_USER_AGENT'] !== $this->userAgent()) { $this->clearAll(); $this->destroy(); - $this->start(); + $this->start($request); } } } @@ -191,10 +191,10 @@ class Session /** * Destroy existing session and restart */ - public function restart() + public function restart(HTTPRequest $request) { $this->destroy(); - $this->init(); + $this->init($request); } /** @@ -210,9 +210,9 @@ class Session /** * Begin session * - * @param string $sid + * @param $request The request for which to start a session */ - public function start($sid = null) + public function start(HTTPRequest $request) { if ($this->isStarted()) { throw new BadMethodCallException("Session has already started"); @@ -223,7 +223,7 @@ class Session $path = Director::baseURL(); } $domain = $this->config()->get('cookie_domain'); - $secure = Director::is_https() && $this->config()->get('cookie_secure'); + $secure = Director::is_https($request) && $this->config()->get('cookie_secure'); $session_path = $this->config()->get('session_store_path'); $timeout = $this->config()->get('timeout'); @@ -255,9 +255,6 @@ class Session session_name('SECSESSID'); } - if ($sid) { - session_id($sid); - } session_start(); $this->data = isset($_SESSION) ? $_SESSION : array(); @@ -480,13 +477,13 @@ class Session * Save data to session * Only save the changes, so that anyone manipulating $_SESSION directly doesn't get burned. */ - public function save() + public function save(HTTPRequest $request) { if ($this->changedData) { $this->finalize(); if (!$this->isStarted()) { - $this->start(); + $this->start($request); } $this->recursivelyApply($this->changedData, $_SESSION); diff --git a/src/Control/SessionMiddleware.php b/src/Control/SessionMiddleware.php index bae7f9644..cd8c393c4 100644 --- a/src/Control/SessionMiddleware.php +++ b/src/Control/SessionMiddleware.php @@ -12,7 +12,7 @@ class SessionMiddleware implements HTTPMiddleware { try { // Start session and execute - $request->getSession()->init(); + $request->getSession()->init($request); // Generate output $response = $delegate($request); @@ -20,7 +20,7 @@ class SessionMiddleware implements HTTPMiddleware // Save session data, even if there was an exception. // Note that save() will start/resume the session if required. } finally { - $request->getSession()->save(); + $request->getSession()->save($request); } return $response; diff --git a/src/Core/Startup/ErrorControlChainMiddleware.php b/src/Core/Startup/ErrorControlChainMiddleware.php index d5a10ef2a..e853ebd08 100644 --- a/src/Core/Startup/ErrorControlChainMiddleware.php +++ b/src/Core/Startup/ErrorControlChainMiddleware.php @@ -84,7 +84,7 @@ class ErrorControlChainMiddleware implements HTTPMiddleware $this->getApplication()->getKernel()->boot(false); // Ensure session is started - $request->getSession()->init(); + $request->getSession()->init($request); // Next, check if we're in dev mode, or the database doesn't have any security data, or we are admin if (Director::isDev() || !Security::database_is_ready() || Permission::check('ADMIN')) { From 69fe166897686b50ef2536808fb3ba5712408174 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Sun, 25 Jun 2017 18:03:03 +1200 Subject: [PATCH 14/18] API: Director::handleRequest() is no longer static - use a Director service NEW: Add HTMLMiddlewareAware trait to HTTPApplication, Director, and RequestHandler NEW: Allow service specs to be passed to Director rules. This refactor of the controller middlewares takes a service definition approach rather than a static-method-and-config approach that Director historically had. The use of a trait for middleware means that the Middlewares array property can be defined on RequestHandler, Director, and HTTPApplication objects in the same way. --- _config/requestprocessors.yml | 18 ++++---- _config/security.yml | 7 ++-- src/Control/Director.php | 45 +++++++++----------- src/Control/HTTPApplication.php | 57 ++----------------------- src/Control/HTTPMiddlewareAware.php | 64 +++++++++++++++++++++++++++++ src/Control/RequestHandler.php | 3 ++ 6 files changed, 104 insertions(+), 90 deletions(-) create mode 100644 src/Control/HTTPMiddlewareAware.php diff --git a/_config/requestprocessors.yml b/_config/requestprocessors.yml index 280eb1e6f..41704d179 100644 --- a/_config/requestprocessors.yml +++ b/_config/requestprocessors.yml @@ -1,16 +1,16 @@ --- Name: requestprocessors --- -SilverStripe\Control\Director: - middlewares: - TrustedProxyMiddleware: '%$SilverStripe\Control\TrustedProxyMiddleware' - AllowedHostsMiddleware: '%$SilverStripe\Control\AllowedHostsMiddleware' - SessionMiddleware: 'SilverStripe\Control\SessionMiddleware' - RequestProcessor: 'SilverStripe\Control\RequestProcessor' - FlushMiddleware: '%$SilverStripe\Control\FlushMiddleware' - - 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' + SilverStripe\Control\AllowedHostsMiddleware: properties: AllowedHosts: "`SS_ALLOWED_HOSTS`" diff --git a/_config/security.yml b/_config/security.yml index 03dbf19ea..be0cf758f 100644 --- a/_config/security.yml +++ b/_config/security.yml @@ -19,10 +19,11 @@ SilverStripe\Core\Injector\Injector: --- Name: coresecurity --- -SilverStripe\Control\Director: - middlewares: - - %$SilverStripe\Security\AuthenticationMiddleware SilverStripe\Core\Injector\Injector: + SilverStripe\Control\Director: + properties: + Middlewares: + - %$SilverStripe\Security\AuthenticationMiddleware SilverStripe\Security\AuthenticationMiddleware: properties: AuthenticationHandler: %$SilverStripe\Security\AuthenticationHandler diff --git a/src/Control/Director.php b/src/Control/Director.php index 52a9dd8ff..ed1a565d9 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -29,6 +29,7 @@ use SilverStripe\View\TemplateGlobalProvider; class Director implements TemplateGlobalProvider { use Configurable; + use HTTPMiddlewareAware; /** * Specifies this url is relative to the base. @@ -132,7 +133,7 @@ class Director implements TemplateGlobalProvider ) { return static::mockRequest( function (HTTPRequest $request) { - return static::handleRequest($request); + return Injector::inst()->get(Director::class)->handleRequest($request); }, $url, $postVars, @@ -302,15 +303,12 @@ class Director implements TemplateGlobalProvider * @return HTTPResponse * @throws HTTPResponse_Exception */ - public static function handleRequest(HTTPRequest $request) + public function handleRequest(HTTPRequest $request) { Injector::inst()->registerService($request, HTTPRequest::class); $rules = Director::config()->uninherited('rules'); - // Get global middlewares - $middlewares = Director::config()->uninherited('middlewares') ?: []; - // Default handler - mo URL rules matched, so return a 404 error. $handler = function () { return new HTTPResponse('No URL rule was matched', 404); @@ -326,18 +324,6 @@ class Director implements TemplateGlobalProvider } } - // Add controller-specific middlewares - if (isset($controllerOptions['Middlewares'])) { - // Force to array - if (!is_array($controllerOptions['Middlewares'])) { - $controllerOptions['Middlewares'] = [$controllerOptions['Middlewares']]; - } - $middlewares = array_merge($middlewares, $controllerOptions['Middlewares']); - } - - // Remove null middlewares (may be included due to limitatons of config yml) - $middlewares = array_filter($middlewares); - // Match pattern $arguments = $request->match($pattern, true); if ($arguments !== false) { @@ -363,12 +349,25 @@ class Director implements TemplateGlobalProvider // Find the controller name $controller = $arguments['Controller']; - $controllerObj = Injector::inst()->create($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 { - return $controllerObj->handleRequest($request); + // 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(); } @@ -377,12 +376,8 @@ class Director implements TemplateGlobalProvider } } - // Call the handler with the given middlewares - $response = self::callWithMiddlewares( - $request, - $middlewares, - $handler - ); + // Call the handler with the configured middlewares + $response = $this->callMiddleware($request, $handler); // 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 diff --git a/src/Control/HTTPApplication.php b/src/Control/HTTPApplication.php index 51b386ca1..552ffba69 100644 --- a/src/Control/HTTPApplication.php +++ b/src/Control/HTTPApplication.php @@ -3,6 +3,7 @@ namespace SilverStripe\Control; use SilverStripe\Core\Application; +use SilverStripe\Core\Injector\Injector; use SilverStripe\Control\HTTPMiddleware; use SilverStripe\Core\Kernel; @@ -11,10 +12,8 @@ use SilverStripe\Core\Kernel; */ class HTTPApplication implements Application { - /** - * @var HTTPMiddleware[] - */ - protected $middlewares = []; + + use HTTPMiddlewareAware; /** * @var Kernel @@ -26,54 +25,6 @@ class HTTPApplication implements Application $this->kernel = $kernel; } - /** - * @return HTTPMiddleware[] - */ - public function getMiddlewares() - { - return $this->middlewares; - } - - /** - * @param HTTPMiddleware[] $middlewares - * @return $this - */ - public function setMiddlewares($middlewares) - { - $this->middlewares = $middlewares; - return $this; - } - - /** - * @param HTTPMiddleware $middleware - * @return $this - */ - public function addMiddleware(HTTPMiddleware $middleware) - { - $this->middlewares[] = $middleware; - return $this; - } - - /** - * Call middleware - * - * @param HTTPRequest $request - * @param callable $last Last config to call - * @return HTTPResponse - */ - protected function callMiddleware(HTTPRequest $request, $last) - { - // Reverse middlewares - $next = $last; - /** @var HTTPMiddleware $middleware */ - foreach (array_reverse($this->getMiddlewares()) as $middleware) { - $next = function ($request) use ($middleware, $next) { - return $middleware->process($request, $next); - }; - } - return call_user_func($next, $request); - } - /** * Get the kernel for this application * @@ -96,7 +47,7 @@ class HTTPApplication implements Application // Ensure boot is invoked return $this->execute($request, function (HTTPRequest $request) { - return Director::handleRequest($request); + return Injector::inst()->get(Director::class)->handleRequest($request); }, $flush); } diff --git a/src/Control/HTTPMiddlewareAware.php b/src/Control/HTTPMiddlewareAware.php new file mode 100644 index 000000000..d3aff5574 --- /dev/null +++ b/src/Control/HTTPMiddlewareAware.php @@ -0,0 +1,64 @@ +middlewares; + } + + /** + * @param HTTPMiddleware[] $middlewares + * @return $this + */ + public function setMiddlewares($middlewares) + { + // Allow nulls in the middlewares array to deal with limitations of yml config + $this->middlewares = array_filter((array)$middlewares); + return $this; + } + + /** + * @param HTTPMiddleware $middleware + * @return $this + */ + public function addMiddleware(HTTPMiddleware $middleware) + { + $this->middlewares[] = $middleware; + return $this; + } + + /** + * Call middleware + * + * @param $request The request to pass to the middlewares and callback + * @param $last The callback to call after all middlewares + * @return HTTPResponse + */ + public function callMiddleware(HTTPRequest $request, callable $last) + { + // Reverse middlewares + $next = $last; + /** @var HTTPMiddleware $middleware */ + foreach (array_reverse($this->getMiddlewares()) as $middleware) { + $next = function ($request) use ($middleware, $next) { + return $middleware->process($request, $next); + }; + } + return $next($request); + } +} diff --git a/src/Control/RequestHandler.php b/src/Control/RequestHandler.php index 86f4460aa..ce76900e0 100644 --- a/src/Control/RequestHandler.php +++ b/src/Control/RequestHandler.php @@ -46,6 +46,9 @@ use BadMethodCallException; */ class RequestHandler extends ViewableData { + + use HTTPMiddlewareAware; + /** * Optional url_segment for this request handler * From 67887febc55a05e4c3613b3428d295b3e2eff5af Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Mon, 26 Jun 2017 11:24:50 +1200 Subject: [PATCH 15/18] fix - session now uses request --- src/Control/Session.php | 16 ++++++---------- src/Dev/SapphireTest.php | 2 +- tests/php/Control/SessionTest.php | 13 ++++++++----- 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/Control/Session.php b/src/Control/Session.php index d8f4473b8..c734cbdb6 100644 --- a/src/Control/Session.php +++ b/src/Control/Session.php @@ -145,13 +145,9 @@ class Session * * @return string */ - protected function userAgent() + protected function userAgent($request) { - if (isset($_SERVER['HTTP_USER_AGENT'])) { - return $_SERVER['HTTP_USER_AGENT']; - } else { - return ''; - } + return $request->getHeader('User-Agent'); } /** @@ -180,7 +176,7 @@ class Session // Funny business detected! if (isset($this->data['HTTP_USER_AGENT'])) { - if ($this->data['HTTP_USER_AGENT'] !== $this->userAgent()) { + if ($this->data['HTTP_USER_AGENT'] !== $this->userAgent($request)) { $this->clearAll(); $this->destroy(); $this->start($request); @@ -468,9 +464,9 @@ class Session /** * Set user agent key */ - public function finalize() + public function finalize(HTTPRequest $request) { - $this->set('HTTP_USER_AGENT', $this->userAgent()); + $this->set('HTTP_USER_AGENT', $this->userAgent($request)); } /** @@ -480,7 +476,7 @@ class Session public function save(HTTPRequest $request) { if ($this->changedData) { - $this->finalize(); + $this->finalize($request); if (!$this->isStarted()) { $this->start($request); diff --git a/src/Dev/SapphireTest.php b/src/Dev/SapphireTest.php index e5604faf8..a8455bf3e 100644 --- a/src/Dev/SapphireTest.php +++ b/src/Dev/SapphireTest.php @@ -911,7 +911,7 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly // Custom application $app->execute($request, function (HTTPRequest $request) { // Start session and execute - $request->getSession()->init(); + $request->getSession()->init($request); // Invalidate classname spec since the test manifest will now pull out new subclasses for each internal class // (e.g. Member will now have various subclasses of DataObjects that implement TestOnly) diff --git a/tests/php/Control/SessionTest.php b/tests/php/Control/SessionTest.php index afdda1013..acc50c84a 100644 --- a/tests/php/Control/SessionTest.php +++ b/tests/php/Control/SessionTest.php @@ -4,6 +4,7 @@ namespace SilverStripe\Control\Tests; use SilverStripe\Control\Session; use SilverStripe\Dev\SapphireTest; +use SilverStripe\Control\HTTPRequest; /** * Tests to cover the {@link Session} class @@ -107,20 +108,22 @@ class SessionTest extends SapphireTest public function testUserAgentLockout() { // Set a user agent - $_SERVER['HTTP_USER_AGENT'] = 'Test Agent'; + $req1 = new HTTPRequest('GET', '/'); + $req1->setHeader('User-Agent', 'Test Agent'); // Generate our session $s = new Session(array()); - $s->init(); + $s->init($req1); $s->set('val', 123); - $s->finalize(); + $s->finalize($req1); // Change our UA - $_SERVER['HTTP_USER_AGENT'] = 'Fake Agent'; + $req2 = new HTTPRequest('GET', '/'); + $req2->setHeader('User-Agent', 'Test Agent'); // Verify the new session reset our values $s2 = new Session($s); - $s2->init(); + $s2->init($req2); $this->assertNotEquals($s2->get('val'), 123); } } From 7aa67f856ba4f76a0036845f0681d108a4c5c1ec Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 26 Jun 2017 12:32:12 +1200 Subject: [PATCH 16/18] Move files to middleware folder --- src/Control/{ => Middleware}/AllowedHostsMiddleware.php | 0 src/Control/{ => Middleware}/FlushMiddleware.php | 0 src/Control/{ => Middleware}/HTTPMiddleware.php | 0 src/Control/{ => Middleware}/HTTPMiddlewareAware.php | 0 src/Control/{ => Middleware}/SessionMiddleware.php | 0 src/Control/{ => Middleware}/TrustedProxyMiddleware.php | 0 6 files changed, 0 insertions(+), 0 deletions(-) rename src/Control/{ => Middleware}/AllowedHostsMiddleware.php (100%) rename src/Control/{ => Middleware}/FlushMiddleware.php (100%) rename src/Control/{ => Middleware}/HTTPMiddleware.php (100%) rename src/Control/{ => Middleware}/HTTPMiddlewareAware.php (100%) rename src/Control/{ => Middleware}/SessionMiddleware.php (100%) rename src/Control/{ => Middleware}/TrustedProxyMiddleware.php (100%) diff --git a/src/Control/AllowedHostsMiddleware.php b/src/Control/Middleware/AllowedHostsMiddleware.php similarity index 100% rename from src/Control/AllowedHostsMiddleware.php rename to src/Control/Middleware/AllowedHostsMiddleware.php diff --git a/src/Control/FlushMiddleware.php b/src/Control/Middleware/FlushMiddleware.php similarity index 100% rename from src/Control/FlushMiddleware.php rename to src/Control/Middleware/FlushMiddleware.php diff --git a/src/Control/HTTPMiddleware.php b/src/Control/Middleware/HTTPMiddleware.php similarity index 100% rename from src/Control/HTTPMiddleware.php rename to src/Control/Middleware/HTTPMiddleware.php diff --git a/src/Control/HTTPMiddlewareAware.php b/src/Control/Middleware/HTTPMiddlewareAware.php similarity index 100% rename from src/Control/HTTPMiddlewareAware.php rename to src/Control/Middleware/HTTPMiddlewareAware.php diff --git a/src/Control/SessionMiddleware.php b/src/Control/Middleware/SessionMiddleware.php similarity index 100% rename from src/Control/SessionMiddleware.php rename to src/Control/Middleware/SessionMiddleware.php diff --git a/src/Control/TrustedProxyMiddleware.php b/src/Control/Middleware/TrustedProxyMiddleware.php similarity index 100% rename from src/Control/TrustedProxyMiddleware.php rename to src/Control/Middleware/TrustedProxyMiddleware.php From d20ab50f9d480372a2b6489278f9876d1a85134c Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Sun, 25 Jun 2017 15:12:29 +1200 Subject: [PATCH 17/18] 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() From f699650b5fdb0c53b14484b5c40e76d812c0be64 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Tue, 27 Jun 2017 10:19:51 +1200 Subject: [PATCH 18/18] Update based on feedback --- .../02_Controllers/05_Middlewares.md | 3 +-- src/Control/Director.php | 2 +- .../Middleware/TrustedProxyMiddleware.php | 23 +++++-------------- src/Core/Injector/Injector.php | 12 ++++------ tests/php/Core/Injector/InjectorTest.php | 18 ++------------- 5 files changed, 14 insertions(+), 44 deletions(-) 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 50c5015a0..1069cc84c 100644 --- a/docs/en/02_Developer_Guides/02_Controllers/05_Middlewares.md +++ b/docs/en/02_Developer_Guides/02_Controllers/05_Middlewares.md @@ -111,9 +111,8 @@ property. The controller which does the work should be registered under the SilverStripe\Control\Director: rules: special\section: - Controller: SpecialRouteMiddleware + Controller: %$SpecialRouteMiddleware ## API Documentation * [api:SilverStripe\Control\HTTPMiddleware] - diff --git a/src/Control/Director.php b/src/Control/Director.php index 3921b8164..1977cffa6 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -372,7 +372,7 @@ 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, true); + Injector::inst()->unregisterNamedObject(HTTPRequest::class); return $response; } diff --git a/src/Control/Middleware/TrustedProxyMiddleware.php b/src/Control/Middleware/TrustedProxyMiddleware.php index 39ab45141..0d865e6c3 100644 --- a/src/Control/Middleware/TrustedProxyMiddleware.php +++ b/src/Control/Middleware/TrustedProxyMiddleware.php @@ -82,17 +82,13 @@ class TrustedProxyMiddleware implements HTTPMiddleware } /** - * Set the array of headers from which to lookup the hostname - * Can also specify comma-separated list as a single string. + * Set the array of headers from which to lookup the hostname. * - * @param array|string $proxyHostHeaders + * @param array $proxyHostHeaders * @return $this */ public function setProxyHostHeaders($proxyHostHeaders) { - if (is_string($proxyHostHeaders)) { - $proxyHostHeaders = preg_split('/ *, */', $proxyHostHeaders); - } $this->proxyHostHeaders = $proxyHostHeaders ?: []; return $this; } @@ -108,17 +104,13 @@ class TrustedProxyMiddleware implements HTTPMiddleware } /** - * Set the array of headers from which to lookup the client IP - * Can also specify comma-separated list as a single string. + * Set the array of headers from which to lookup the client IP. * - * @param array|string $proxyIPHeaders + * @param array $proxyIPHeaders * @return $this */ public function setProxyIPHeaders($proxyIPHeaders) { - if (is_string($proxyIPHeaders)) { - $proxyIPHeaders = preg_split('/ *, */', $proxyIPHeaders); - } $this->proxyIPHeaders = $proxyIPHeaders ?: []; return $this; } @@ -137,14 +129,11 @@ class TrustedProxyMiddleware implements HTTPMiddleware * Set array of headers from which to lookup the client scheme (http/https) * Can also specify comma-separated list as a single string. * - * @param array|string $proxySchemeHeaders + * @param array $proxySchemeHeaders * @return $this */ public function setProxySchemeHeaders($proxySchemeHeaders) { - if (is_string($proxySchemeHeaders)) { - $proxySchemeHeaders = preg_split('/ *, */', $proxySchemeHeaders); - } $this->proxySchemeHeaders = $proxySchemeHeaders ?: []; return $this; } @@ -210,7 +199,7 @@ class TrustedProxyMiddleware implements HTTPMiddleware // Validate IP address $ip = $request->getIP(); if ($ip) { - return IPUtils::checkIP($ip, preg_split('/ *, */', $trustedIPs)); + return IPUtils::checkIP($ip, preg_split('/\s*,\s*/', $trustedIPs)); } return false; diff --git a/src/Core/Injector/Injector.php b/src/Core/Injector/Injector.php index 02754bf46..2156c1969 100644 --- a/src/Core/Injector/Injector.php +++ b/src/Core/Injector/Injector.php @@ -851,15 +851,12 @@ 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, $flushSpecs = false) + public function unregisterNamedObject($name) { unset($this->serviceCache[$name]); - if ($flushSpecs) { - unset($this->specs[$name]); - } + unset($this->specs[$name]); return $this; } @@ -867,10 +864,9 @@ 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, $flushSpecs = false) + public function unregisterObjects($types) { if (!is_array($types)) { $types = [ $types ]; @@ -884,7 +880,7 @@ class Injector implements ContainerInterface throw new InvalidArgumentException("Global unregistration is not allowed"); } if ($object instanceof $filterClass) { - $this->unregisterNamedObject($key, $flushSpecs); + $this->unregisterNamedObject($key); break; } } diff --git a/tests/php/Core/Injector/InjectorTest.php b/tests/php/Core/Injector/InjectorTest.php index bca527c10..4501bbaa1 100644 --- a/tests/php/Core/Injector/InjectorTest.php +++ b/tests/php/Core/Injector/InjectorTest.php @@ -811,15 +811,8 @@ class InjectorTest extends SapphireTest $this->assertTrue($injector->has('NamedService')); $this->assertEquals($service, $injector->get('NamedService')); - // Unregister by name only: New instance of the - // old class will be constructed + // Unregister service by name $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 @@ -827,15 +820,8 @@ class InjectorTest extends SapphireTest $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 + // Unregister service by class $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)); }