From 5fa5a0c46aea36c11383a3a7a4121ca9e235b3da Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Mon, 17 Feb 2025 10:21:20 +1300 Subject: [PATCH] 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. --- .../Middleware/AllowedHostsMiddleware.php | 17 ++- src/Core/BaseKernel.php | 28 +++++ .../Middleware/AllowedHostsMiddlewareTest.php | 119 ++++++++++++++++++ tests/php/Core/KernelTest.php | 61 +++++++++ 4 files changed, 222 insertions(+), 3 deletions(-) create mode 100644 tests/php/Control/Middleware/AllowedHostsMiddlewareTest.php diff --git a/src/Control/Middleware/AllowedHostsMiddleware.php b/src/Control/Middleware/AllowedHostsMiddleware.php index 8a7df32f4c7..7915dc8b139 100644 --- a/src/Control/Middleware/AllowedHostsMiddleware.php +++ b/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 ?? []) ) { diff --git a/src/Core/BaseKernel.php b/src/Core/BaseKernel.php index a1dbc503ba5..d3f96a0cb4e 100644 --- a/src/Core/BaseKernel.php +++ b/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 + ); + } + } } diff --git a/tests/php/Control/Middleware/AllowedHostsMiddlewareTest.php b/tests/php/Control/Middleware/AllowedHostsMiddlewareTest.php new file mode 100644 index 00000000000..8da267c64af --- /dev/null +++ b/tests/php/Control/Middleware/AllowedHostsMiddlewareTest.php @@ -0,0 +1,119 @@ + [ + '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']); + } +} diff --git a/tests/php/Core/KernelTest.php b/tests/php/Core/KernelTest.php index 42f05c2714e..35ed9b06892 100644 --- a/tests/php/Core/KernelTest.php +++ b/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); + } + } }