Merge pull request #65 from silverstripe-security/pulls/4.2/ss-2018-009

[SS-2018-009] Allow forced redirects to HTTPS for responses with basic authentication

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`

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
+        return $this->redirectToScheme($request, $scheme, $host);
-        $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;
     }
 
     /**
@@ -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;
+    }
 }

tests/php/Control/Middleware/CanonicalURLMiddlewareTest.php

@@ -0,0 +1,98 @@
+<?php
+
+namespace SilverStripe\Control\Tests\Middleware;
+
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Control\HTTPResponse;
+use SilverStripe\Control\Middleware\CanonicalURLMiddleware;
+use SilverStripe\Dev\SapphireTest;
+
+class CanonicalURLMiddlewareTest extends SapphireTest
+{
+    /**
+     * Stub middleware class, partially mocked
+     *
+     * @var CanonicalURLMiddleware
+     */
+    protected $middleware;
+
+    protected function setUp()
+    {
+        parent::setUp();
+
+        /** @var CanonicalURLMiddleware $middleware */
+        $this->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');
+    }
+}