From 30e2d9c4df0e8d431bdf41446c7dc4d5e74ad507 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Tue, 24 Apr 2018 10:22:50 +1200 Subject: [PATCH 1/2] [SS-2018-009] Allow forced redirects to HTTPS for responses with basic authentication --- .../Middleware/CanonicalURLMiddleware.php | 91 +++++++++++++++-- .../Middleware/CanonicalURLMiddlewareTest.php | 98 +++++++++++++++++++ 2 files changed, 179 insertions(+), 10 deletions(-) create mode 100644 tests/php/Control/Middleware/CanonicalURLMiddlewareTest.php diff --git a/src/Control/Middleware/CanonicalURLMiddleware.php b/src/Control/Middleware/CanonicalURLMiddleware.php index a43dab80a..b108a479e 100644 --- a/src/Control/Middleware/CanonicalURLMiddleware.php +++ b/src/Control/Middleware/CanonicalURLMiddleware.php @@ -34,6 +34,14 @@ class CanonicalURLMiddleware implements HTTPMiddleware */ protected $forceSSL = false; + /** + * Set if we should automatically redirect basic auth requests to HTTPS. A null value (default) will + * cause this property to return the value of the forceSSL property. + * + * @var bool|null + */ + protected $forceBasicAuthToSSL = null; + /** * Redirect type * @@ -137,6 +145,29 @@ class CanonicalURLMiddleware implements HTTPMiddleware return $this; } + /** + * @param bool|null $forceBasicAuth + * @return $this + */ + public function setForceBasicAuthToSSL($forceBasicAuth) + { + $this->forceBasicAuthToSSL = $forceBasicAuth; + return $this; + } + + /** + * @return bool + */ + public function getForceBasicAuthToSSL() + { + // Check if explicitly set + if (isset($this->forceBasicAuthToSSL)) { + return $this->forceBasicAuthToSSL; + } + // If not explicitly set, default to on if ForceSSL is on + return $this->getForceSSL(); + } + /** * Generate response for the given request * @@ -152,7 +183,16 @@ class CanonicalURLMiddleware implements HTTPMiddleware return $redirect; } - return $delegate($request); + /** @var HTTPResponse $response */ + $response = $delegate($request); + if ($this->hasBasicAuthPrompt($response) + && $request->getScheme() !== 'https' + && $this->getForceBasicAuthToSSL() + ) { + return $this->redirectToScheme($request, 'https'); + } + + return $response; } /** @@ -190,14 +230,7 @@ class CanonicalURLMiddleware implements HTTPMiddleware return null; } - // Rebuild url for request - $url = Controller::join_links("{$scheme}://{$host}", Director::baseURL(), $request->getURL(true)); - - // Force redirect - $response = new HTTPResponse(); - $response->redirect($url, $this->getRedirectType()); - HTTP::add_cache_headers($response); - return $response; + return $this->redirectToScheme($request, $scheme, $host); } /** @@ -224,7 +257,7 @@ class CanonicalURLMiddleware implements HTTPMiddleware * Return a valid request, if one is available, or null if none is available * * @param HTTPRequest $request - * @return mixed|null + * @return HTTPRequest|null */ protected function getOrValidateRequest(HTTPRequest $request = null) { @@ -328,4 +361,42 @@ class CanonicalURLMiddleware implements HTTPMiddleware } return empty($enabledEnvs) || in_array(Director::get_environment_type(), $enabledEnvs); } + + /** + * Determine whether the executed middlewares have added a basic authentication prompt + * + * @param HTTPResponse $response + * @return bool + */ + protected function hasBasicAuthPrompt(HTTPResponse $response = null) + { + if (!$response) { + return false; + } + return ($response->getStatusCode() === 401 && $response->getHeader('WWW-Authenticate')); + } + + /** + * Redirect the current URL to the specified HTTP scheme + * + * @param HTTPRequest $request + * @param string $scheme + * @param string $host + * @return HTTPResponse + */ + protected function redirectToScheme(HTTPRequest $request, $scheme, $host = null) + { + if (!$host) { + $host = $request->getHost(); + } + + $url = Controller::join_links("{$scheme}://{$host}", Director::baseURL(), $request->getURL(true)); + + // Force redirect + $response = HTTPResponse::create(); + $response->redirect($url, $this->getRedirectType()); + HTTP::add_cache_headers($response); + + return $response; + } } diff --git a/tests/php/Control/Middleware/CanonicalURLMiddlewareTest.php b/tests/php/Control/Middleware/CanonicalURLMiddlewareTest.php new file mode 100644 index 000000000..e4e3caa56 --- /dev/null +++ b/tests/php/Control/Middleware/CanonicalURLMiddlewareTest.php @@ -0,0 +1,98 @@ +middleware = $this->getMockBuilder(CanonicalURLMiddleware::class) + ->setMethods(['getRedirect']) + ->getMock(); + + $this->middleware->expects($this->any())->method('getRedirect')->willReturn(false); + + $this->middleware->setForceBasicAuthToSSL(true); + } + + public function testHttpsIsForcedForBasicAuth() + { + $this->middleware->expects($this->once())->method('getRedirect'); + + $request = new HTTPRequest('GET', '/'); + $mockResponse = (new HTTPResponse) + ->addHeader('WWW-Authenticate', 'basic') + ->setStatusCode(401); + + $result = $this->middleware->process($request, function () use ($mockResponse) { + return $mockResponse; + }); + + $this->assertNotSame($mockResponse, $result, 'New response is created and returned'); + $this->assertEquals(301, $result->getStatusCode(), 'Basic auth responses are redirected'); + $this->assertContains('https://', $result->getHeader('Location'), 'HTTPS is in the redirect location'); + } + + public function testMiddlewareDelegateIsReturnedWhenBasicAuthRedirectIsDisabled() + { + $this->middleware->expects($this->once())->method('getRedirect'); + $this->middleware->setForceBasicAuthToSSL(false); + + $request = new HTTPRequest('GET', '/'); + $mockResponse = (new HTTPResponse) + ->addHeader('WWW-Authenticate', 'basic') + ->setStatusCode(401); + + $result = $this->middleware->process($request, function () use ($mockResponse) { + return $mockResponse; + }); + $this->assertSame($mockResponse, $result, 'Response returned verbatim with auto redirect disabled'); + } + + public function testMiddlewareDelegateIsReturnedWhenNoBasicAuthIsPresent() + { + $this->middleware->expects($this->once())->method('getRedirect'); + + $request = new HTTPRequest('GET', '/'); + $mockResponse = (new HTTPResponse)->addHeader('Foo', 'bar'); + + $result = $this->middleware->process($request, function () use ($mockResponse) { + return $mockResponse; + }); + + $this->assertSame($mockResponse, $result, 'Non basic-auth responses are returned verbatim'); + } + + public function testGetForceBasicAuthToSSL() + { + $this->middleware->setForceBasicAuthToSSL(null); + + $this->middleware->setForceSSL(true); + $this->assertTrue($this->middleware->getForceBasicAuthToSSL(), 'Default falls over to forceSSL'); + + $this->middleware->setForceSSL(false); + $this->assertFalse($this->middleware->getForceBasicAuthToSSL(), 'Default falls over to forceSSL'); + + $this->middleware->setForceBasicAuthToSSL(true); + $this->assertTrue($this->middleware->getForceBasicAuthToSSL(), 'Explicitly set is returned'); + + $this->middleware->setForceBasicAuthToSSL(false); + $this->middleware->setForceSSL(true); + $this->assertFalse($this->middleware->getForceBasicAuthToSSL(), 'Explicitly set is returned'); + } +} From 1505a89a63782c477f6acafb1c57f2c78cd132f3 Mon Sep 17 00:00:00 2001 From: Robbie Averill Date: Tue, 24 Apr 2018 16:42:52 +1200 Subject: [PATCH 2/2] Update to include note about auto redirect to HTTPS for basic auth --- docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md | 5 +++++ 1 file changed, 5 insertions(+) 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 e11507cf8..d2a892464 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 @@ -691,6 +691,11 @@ if (!Director::isDev()) { Forcing HTTPS so requires a certificate to be purchased or obtained through a vendor such as [lets encrypt](https://letsencrypt.org/) and configured on your web server. +Note that by default enabling SSL will also enable `CanonicalURLMiddleware::forceBasicAuthToSSL` which will detect +and automatically redirect any requests with basic authentication headers to first be served over HTTPS. You can +disable this behaviour using `CanonicalURLMiddleware::singleton()->setForceBasicAuthToSSL(false)`, or via Injector +configuration in YAML. + We also want to ensure cookies are not shared between secure and non-secure sessions, so we must tell SilverStripe to use a [secure session](https://docs.silverstripe.org/en/3/developer_guides/cookies_and_sessions/sessions/#secure-session-cookie). To do this, you may set the `cookie_secure` parameter to `true` in your `config.yml` for `Session`