ENH Add a warning if allowed hosts is not set. (#11612)

Adds ability to "wildcard" allow all hosts, which disables the logging.
Adds much needed test coverage.

src/Control/Middleware/AllowedHostsMiddleware.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\Control\Middleware;
 
+use InvalidArgumentException;
 use SilverStripe\Control\Director;
 use SilverStripe\Control\HTTPRequest;
 use SilverStripe\Control\HTTPResponse;
@@ -12,14 +13,16 @@
 class AllowedHostsMiddleware implements HTTPMiddleware
 {
     /**
-     * List of allowed hosts
+     * List of allowed hosts.
+     * Can be ['*'] to allow all hosts and disable the logged warning.
      *
      * @var array
      */
     private $allowedHosts = [];
 
     /**
-     * @return array List of allowed Host header values
+     * @return array List of allowed Host header values.
+     * Note that both an empty array and ['*'] can be used to allow all hosts.
      */
     public function getAllowedHosts()
     {
@@ -30,14 +33,21 @@ public function getAllowedHosts()
      * Sets the list of allowed Host header values
      * Can also specify a comma separated list
      *
+     * Note that both an empty array and ['*'] can be used to allow all hosts.
+     *
      * @param array|string $allowedHosts
      * @return $this
      */
     public function setAllowedHosts($allowedHosts)
     {
-        if (is_string($allowedHosts)) {
+        if ($allowedHosts === null) {
+            $allowedHosts = [];
+        } elseif (is_string($allowedHosts)) {
             $allowedHosts = preg_split('/ *, */', $allowedHosts ?? '');
         }
+        if (count($allowedHosts) > 1 && in_array('*', $allowedHosts)) {
+            throw new InvalidArgumentException('The wildcard "*" cannot be used in conjunction with actual hosts.');
+        }
         $this->allowedHosts = $allowedHosts;
         return $this;
     }
@@ -51,6 +61,7 @@ public function process(HTTPRequest $request, callable $delegate)
 
         // check allowed hosts
         if ($allowedHosts
+            && $allowedHosts !== ['*']
             && !Director::is_cli()
             && !in_array($request->getHeader('Host'), $allowedHosts ?? [])
         ) {

src/Core/BaseKernel.php

@@ -27,6 +27,7 @@
 use SilverStripe\View\ThemeManifest;
 use SilverStripe\View\ThemeResourceLoader;
 use Exception;
+use SilverStripe\Control\Middleware\AllowedHostsMiddleware;
 use SilverStripe\Dev\Deprecation;
 
 /**
@@ -222,6 +223,9 @@ protected function bootConfigs()
     {
         // After loading all other app manifests, include _config.php files
         $this->getModuleLoader()->getManifest()->activateConfig();
+
+        // Ensure everything is set up correctly
+        $this->validateConfiguration();
     }
 
     /**
@@ -388,6 +392,7 @@ public function activate()
         $this->getInjectorLoader()
             ->getManifest()
             ->registerService($this, Kernel::class);
+
         return $this;
     }
 
@@ -469,4 +474,27 @@ public function setThemeResourceLoader($themeResourceLoader)
         $this->themeResourceLoader = $themeResourceLoader;
         return $this;
     }
+
+    /**
+     * Validate configuration of the application is in a good state, ready for use.
+     *
+     * This method can be used to warn developers of any misconfiguration, or configuration
+     * which is missing but should be set according to best practice.
+     *
+     * In some cases, this could be used to halt execution if configuration critical to operation
+     * has not been set.
+     */
+    protected function validateConfiguration(): void
+    {
+        // Log a warning if allowed hosts hasn't been configured.
+        // This can include wildcard, but it must be explicitly set to ensure the developer is aware
+        // of the level of protection their application has against host header injection attacks.
+        $allowedHostsMiddleware = Injector::inst()->get(AllowedHostsMiddleware::class, true);
+        if (empty($allowedHostsMiddleware->getAllowedHosts())) {
+            Injector::inst()->get(LoggerInterface::class)->warning(
+                'Allowed hosts has not been set. Your application could be vulnerable to host header injection attacks.'
+                . ' Either set the SS_ALLOWED_HOSTS environment variable or the AllowedHosts property on ' . AllowedHostsMiddleware::class
+            );
+        }
+    }
 }

tests/php/Control/Middleware/AllowedHostsMiddlewareTest.php

@@ -0,0 +1,119 @@
+<?php
+
+namespace SilverStripe\Control\Tests\Middleware;
+
+use InvalidArgumentException;
+use ReflectionClass;
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Control\HTTPResponse;
+use SilverStripe\Core\Environment;
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Control\Middleware\AllowedHostsMiddleware;
+
+class AllowedHostsMiddlewareTest extends SapphireTest
+{
+    protected $usesDatabase = false;
+
+    public function provideProcess(): array
+    {
+        return [
+            'cli allow all' => [
+                'allowedHosts' => [],
+                'isCli' => true,
+                'allowed' => true,
+            ],
+            'cli ignores config' => [
+                'allowedHosts' => ['example.org'],
+                'isCli' => true,
+                'allowed' => true,
+            ],
+            'HTTP allow all' => [
+                'allowedHosts' => [],
+                'isCli' => false,
+                'allowed' => true,
+            ],
+            'HTTP allow all explicit' => [
+                'allowedHosts' => ['*'],
+                'isCli' => false,
+                'allowed' => true,
+            ],
+            'HTTP allow explicit host' => [
+                'allowedHosts' => ['www.example.com'],
+                'isCli' => false,
+                'allowed' => true,
+            ],
+            'HTTP allow explicit host multiple values' => [
+                'allowedHosts' => ['example.com', 'example.org', 'ww.example.org', 'www.example.com'],
+                'isCli' => false,
+                'allowed' => true,
+            ],
+            'HTTP allow explicit host string' => [
+                'allowedHosts' => 'example.com,example.org,ww.example.org,www.example.com',
+                'isCli' => false,
+                'allowed' => true,
+            ],
+            'HTTP host mismatch (missing subdomain)' => [
+                'allowedHosts' => ['example.com'],
+                'isCli' => false,
+                'allowed' => false,
+            ],
+            'HTTP host mismatch (different tld)' => [
+                'allowedHosts' => ['example.org'],
+                'isCli' => false,
+                'allowed' => false,
+            ],
+            'HTTP host mismatch multiple' => [
+                'allowedHosts' => ['example.org', 'www.example.org', 'example.com'],
+                'isCli' => false,
+                'allowed' => false,
+            ],
+            'HTTP host mismatch string' => [
+                'allowedHosts' => 'example.org,www.example.org,example.com',
+                'isCli' => false,
+                'allowed' => false,
+            ],
+        ];
+    }
+
+    /**
+     * @dataProvider provideProcess
+     */
+    public function testProcess(string|array $allowedHosts, bool $isCli, bool $allowed): void
+    {
+        $reflectionEnvironment = new ReflectionClass(Environment::class);
+        $origIsCli = $reflectionEnvironment->getStaticPropertyValue('isCliOverride');
+        $reflectionEnvironment->setStaticPropertyValue('isCliOverride', $isCli);
+
+        try {
+            $middleware = new AllowedHostsMiddleware();
+            $middleware->setAllowedHosts($allowedHosts);
+            $request = new HTTPRequest('GET', '/');
+            $request->addHeader('host', 'www.example.com');
+            $defaultResponse = new HTTPResponse();
+
+            $result = $middleware->process($request, function () use ($defaultResponse) {
+                return $defaultResponse;
+            });
+
+            if ($allowed) {
+                $this->assertSame(200, $result->getStatusCode());
+                $this->assertSame($defaultResponse, $result);
+            } else {
+                $this->assertSame(400, $result->getStatusCode());
+                $this->assertNotSame($defaultResponse, $result);
+            }
+        } finally {
+            $reflectionEnvironment->setStaticPropertyValue('isCliOverride', $origIsCli);
+        }
+    }
+
+    public function testProcessInvalidConfig(): void
+    {
+        $middleware = new AllowedHostsMiddleware();
+
+        $this->expectException(InvalidArgumentException::class);
+        $this->expectExceptionMessage('The wildcard "*" cannot be used in conjunction with actual hosts.');
+
+        $middleware->setAllowedHosts(['*', 'www.example.com']);
+    }
+}

tests/php/Core/KernelTest.php

@@ -3,10 +3,16 @@
 namespace SilverStripe\Core\Tests;
 
 use BadMethodCallException;
+use Exception;
+use Monolog\Logger;
+use Psr\Log\LoggerInterface;
+use ReflectionClass;
 use SilverStripe\Control\Director;
+use SilverStripe\Control\Middleware\AllowedHostsMiddleware;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Core\Config\ConfigLoader;
 use SilverStripe\Core\CoreKernel;
+use SilverStripe\Core\Environment;
 use SilverStripe\Core\Injector\Injector;
 use SilverStripe\Core\Injector\InjectorLoader;
 use SilverStripe\Core\Kernel;
@@ -81,4 +87,59 @@ public function testInvalidConfigDetection()
 
         $kernel->getConfigLoader()->getManifest();
     }
+
+    public function provideAllowedHostsWarning(): array
+    {
+        $scenarios = [
+            [
+                'config' => [],
+                'isCli' => true,
+                'shouldLog' => true,
+            ],
+            [
+                'config' => ['*'],
+                'isCli' => true,
+                'shouldLog' => false,
+            ],
+            [
+                'config' => ['www.example.com', 'example.org'],
+                'isCli' => true,
+                'shouldLog' => false,
+            ],
+        ];
+        // Test both in CLI and non-CLI context
+        foreach ($scenarios as $name => $scenario) {
+            $scenario['isCli'] = false;
+            $scenarios[$name . ' (non-CLI)'] = $scenario;
+        }
+        return $scenarios;
+    }
+
+    /**
+     * @dataProvider provideAllowedHostsWarning
+     */
+    public function testAllowedHostsWarning(array $config, bool $isCli, bool $shouldLog): void
+    {
+        // Prepare mock to check if a warning is logged or not
+        $mockLogger = $this->getMockBuilder(Logger::class)->setConstructorArgs(['testLogger'])->getMock();
+        $expectLog = $shouldLog ? $this->once() : $this->never();
+        $mockLogger->expects($expectLog)->method('warning');
+        Injector::inst()->registerService($mockLogger, LoggerInterface::class);
+
+        // Set the config in our middleware
+        $middleware = Injector::inst()->get(AllowedHostsMiddleware::class, true);
+        $middleware->setAllowedHosts($config);
+
+        $reflectionEnvironment = new ReflectionClass(Environment::class);
+        $origIsCli = $reflectionEnvironment->getStaticPropertyValue('isCliOverride');
+        $reflectionEnvironment->setStaticPropertyValue('isCliOverride', $isCli);
+
+        try {
+            $kernel = Injector::inst()->get(Kernel::class);
+            $kernel->nest(); // $kernel is no longer current kernel
+            $kernel->boot();
+        } finally {
+            $reflectionEnvironment->setStaticPropertyValue('isCliOverride', $origIsCli);
+        }
+    }
 }