fix: Migrate security headers check tests and fix the SetupCheck implementation

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>

2 files changed, 233 insertions, 10 deletions

diff --git a/apps/settings/lib/SetupChecks/SecurityHeaders.php b/apps/settings/lib/SetupChecks/SecurityHeaders.php
index 9079df7e39b..4d89b198f89 100644
--- a/apps/settings/lib/SetupChecks/SecurityHeaders.php
+++ b/apps/settings/lib/SetupChecks/SecurityHeaders.php
@@ -29,7 +29,6 @@ namespace OCA\Settings\SetupChecks;
 use OCP\Http\Client\IClientService;
 use OCP\IConfig;
 use OCP\IL10N;
-use OCP\IRequest;
 use OCP\IURLGenerator;
 use OCP\SetupCheck\ISetupCheck;
 use OCP\SetupCheck\SetupResult;
@@ -43,7 +42,6 @@ class SecurityHeaders implements ISetupCheck {
 		protected IL10N $l10n,
 		protected IConfig $config,
 		protected IURLGenerator $urlGenerator,
-		protected IRequest $request,
 		protected IClientService $clientService,
 		protected LoggerInterface $logger,
 	) {
@@ -63,14 +61,14 @@ class SecurityHeaders implements ISetupCheck {
 		];
 		$securityHeaders = [
 			'X-Content-Type-Options' => ['nosniff', null],
-			'X-Robots-Tag' => ['noindex, nofollow', null],
+			'X-Robots-Tag' => ['noindex,nofollow', null],
 			'X-Frame-Options' => ['sameorigin', 'deny'],
 			'X-Permitted-Cross-Domain-Policies' => ['none', null],
 		];
 
 		foreach ($urls as [$verb,$url,$validStatuses]) {
 			$works = null;
-			foreach ($this->runRequest($url, $verb, ['httpErrors' => false]) as $response) {
+			foreach ($this->runRequest($verb, $url, ['httpErrors' => false]) as $response) {
 				// Check that the response status matches
 				if (!in_array($response->getStatusCode(), $validStatuses)) {
 					$works = false;
@@ -79,25 +77,26 @@ class SecurityHeaders implements ISetupCheck {
 				$msg = '';
 				$msgParameters = [];
 				foreach ($securityHeaders as $header => [$expected, $accepted]) {
-					$value = strtolower($response->getHeader($header));
+					/* Convert to lowercase and remove spaces after comas */
+					$value = preg_replace('/,\s+/', ',', strtolower($response->getHeader($header)));
 					if ($value !== $expected) {
 						if ($accepted !== null && $value === $accepted) {
-							$msg .= $this->l10n->t('- The `%1` HTTP header is not set to `%2`. Some features might not work correctly, as it is recommended to adjust this setting accordingly.', [$header, $expected]);
+							$msg .= $this->l10n->t('- The `%1$s` HTTP header is not set to `%2$s`. Some features might not work correctly, as it is recommended to adjust this setting accordingly.', [$header, $expected])."\n";
 						} else {
-							$msg .= $this->l10n->t('- The `%1` HTTP header is not set to `%2`. This is a potential security or privacy risk, as it is recommended to adjust this setting accordingly.', [$header, $expected]);
+							$msg .= $this->l10n->t('- The `%1$s` HTTP header is not set to `%2$s`. This is a potential security or privacy risk, as it is recommended to adjust this setting accordingly.', [$header, $expected])."\n";
 						}
 					}
 				}
 
 				$xssfields = array_map('trim', explode(';', $response->getHeader('X-XSS-Protection')));
 				if (!in_array('1', $xssfields) || !in_array('mode=block', $xssfields)) {
-					$msg .= $this->l10n->t('- The `%1` HTTP header does not contain `%2`. This is a potential security or privacy risk, as it is recommended to adjust this setting accordingly.', ['X-XSS-Protection', '1; mode=block']);
+					$msg .= $this->l10n->t('- The `%1$s` HTTP header does not contain `%2$s`. This is a potential security or privacy risk, as it is recommended to adjust this setting accordingly.', ['X-XSS-Protection', '1; mode=block'])."\n";
 				}
 
 				$referrerPolicy = $response->getHeader('Referrer-Policy');
 				if (!preg_match('/(no-referrer(-when-downgrade)?|strict-origin(-when-cross-origin)?|same-origin)(,|$)/', $referrerPolicy)) {
 					$msg .= $this->l10n->t(
-						'- The `%1` HTTP header is not set to `%2`, `%3`, `%4`, `%5` or `%6`. This can leak referer information. See the {w3c-recommendation}.',
+						'- The `%1$s` HTTP header is not set to `%2$s`, `%3$s`, `%4$s`, `%5$s` or `%6$s`. This can leak referer information. See the {w3c-recommendation}.',
 						[
 							'Referrer-Policy',
 							'no-referrer',
@@ -106,7 +105,7 @@ class SecurityHeaders implements ISetupCheck {
 							'strict-origin-when-cross-origin',
 							'same-origin',
 						]
-					);
+					)."\n";
 					$msgParameters['w3c-recommendation'] = [
 						'type' => 'highlight',
 						'id' => 'w3c-recommendation',
@@ -127,6 +126,12 @@ class SecurityHeaders implements ISetupCheck {
 					$this->l10n->t('Could not check that your web server serves security headers correctly. Please check manually.'),
 				);
 			}
+			// Otherwise if we fail we can abort here
+			if ($works === false) {
+				return SetupResult::warning(
+					$this->l10n->t("Could not check that your web server serves security headers correctly, unable to query `%s`", [$url]),
+				);
+			}
 		}
 		return SetupResult::success(
 			$this->l10n->t('Your server is correctly configured to send security headers.')
diff --git a/apps/settings/tests/SetupChecks/SecurityHeadersTest.php b/apps/settings/tests/SetupChecks/SecurityHeadersTest.php
new file mode 100644
index 00000000000..4f3304a081d
--- /dev/null
+++ b/apps/settings/tests/SetupChecks/SecurityHeadersTest.php
@@ -0,0 +1,218 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * @copyright Copyright (c) 2024 Côme Chilliet <come.chilliet@nextcloud.com>
+ *
+ * @author Côme Chilliet <come.chilliet@nextcloud.com>
+ *
+ * @license AGPL-3.0-or-later
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+namespace OCA\Settings\Tests;
+
+use OCA\Settings\SetupChecks\SecurityHeaders;
+use OCP\Http\Client\IClientService;
+use OCP\Http\Client\IResponse;
+use OCP\IConfig;
+use OCP\IL10N;
+use OCP\IURLGenerator;
+use OCP\SetupCheck\SetupResult;
+use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
+use Test\TestCase;
+
+class SecurityHeadersTest extends TestCase {
+	private IL10N|MockObject $l10n;
+	private IConfig|MockObject $config;
+	private IURLGenerator|MockObject $urlGenerator;
+	private IClientService|MockObject $clientService;
+	private LoggerInterface|MockObject $logger;
+	private SecurityHeaders|MockObject $setupcheck;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		/** @var IL10N|MockObject */
+		$this->l10n = $this->getMockBuilder(IL10N::class)
+			->disableOriginalConstructor()->getMock();
+		$this->l10n->expects($this->any())
+			->method('t')
+			->willReturnCallback(function ($message, array $replace) {
+				return vsprintf($message, $replace);
+			});
+
+		$this->config = $this->createMock(IConfig::class);
+		$this->urlGenerator = $this->createMock(IURLGenerator::class);
+		$this->clientService = $this->createMock(IClientService::class);
+		$this->logger = $this->createMock(LoggerInterface::class);
+
+		$this->setupcheck = $this->getMockBuilder(SecurityHeaders::class)
+			->onlyMethods(['runRequest'])
+			->setConstructorArgs([
+				$this->l10n,
+				$this->config,
+				$this->urlGenerator,
+				$this->clientService,
+				$this->logger,
+			])
+			->getMock();
+	}
+
+	public function testInvalidStatusCode(): void {
+		$this->setupResponse(500, []);
+
+		$result = $this->setupcheck->run();
+		$this->assertMatchesRegularExpression('/^Could not check that your web server serves security headers correctly/', $result->getDescription());
+		$this->assertEquals(SetupResult::WARNING, $result->getSeverity());
+	}
+
+	public function testAllHeadersMissing(): void {
+		$this->setupResponse(200, []);
+
+		$result = $this->setupcheck->run();
+		$this->assertMatchesRegularExpression('/^Some headers are not set correctly on your instance/', $result->getDescription());
+		$this->assertEquals(SetupResult::WARNING, $result->getSeverity());
+	}
+
+	public function testSomeHeadersMissing(): void {
+		$this->setupResponse(
+			200,
+			[
+				'X-Robots-Tag' => 'noindex, nofollow',
+				'X-Frame-Options' => 'SAMEORIGIN',
+				'Strict-Transport-Security' => 'max-age=15768000;preload',
+				'X-Permitted-Cross-Domain-Policies' => 'none',
+				'Referrer-Policy' => 'no-referrer',
+			]
+		);
+
+		$result = $this->setupcheck->run();
+		$this->assertEquals(
+			"Some headers are not set correctly on your instance\n- The `X-Content-Type-Options` HTTP header is not set to `nosniff`. This is a potential security or privacy risk, as it is recommended to adjust this setting accordingly.\n- The `X-XSS-Protection` HTTP header does not contain `1; mode=block`. This is a potential security or privacy risk, as it is recommended to adjust this setting accordingly.\n",
+			$result->getDescription()
+		);
+		$this->assertEquals(SetupResult::WARNING, $result->getSeverity());
+	}
+
+	public function dataSuccess(): array {
+		return [
+			// description => modifiedHeaders
+			'basic' => [[]],
+			'extra-xss-protection' => [['X-XSS-Protection' => '1; mode=block; report=https://example.com']],
+			'no-space-in-x-robots' => [['X-Robots-Tag' => 'noindex,nofollow']],
+			'strict-origin-when-cross-origin' => [['Referrer-Policy' => 'strict-origin-when-cross-origin']],
+			'referrer-no-referrer-when-downgrade' => [['Referrer-Policy' => 'no-referrer-when-downgrade']],
+			'referrer-strict-origin' => [['Referrer-Policy' => 'strict-origin']],
+			'referrer-strict-origin-when-cross-origin' => [['Referrer-Policy' => 'strict-origin-when-cross-origin']],
+			'referrer-same-origin' => [['Referrer-Policy' => 'same-origin']],
+		];
+	}
+
+	/**
+	 * @dataProvider dataSuccess
+	 */
+	public function testSuccess($headers): void {
+		$headers = array_merge(
+			[
+				'X-XSS-Protection' => '1; mode=block',
+				'X-Content-Type-Options' => 'nosniff',
+				'X-Robots-Tag' => 'noindex, nofollow',
+				'X-Frame-Options' => 'SAMEORIGIN',
+				'Strict-Transport-Security' => 'max-age=15768000',
+				'X-Permitted-Cross-Domain-Policies' => 'none',
+				'Referrer-Policy' => 'no-referrer',
+			],
+			$headers
+		);
+		$this->setupResponse(
+			200,
+			$headers
+		);
+
+		$result = $this->setupcheck->run();
+		$this->assertEquals(
+			'Your server is correctly configured to send security headers.',
+			$result->getDescription()
+		);
+		$this->assertEquals(SetupResult::SUCCESS, $result->getSeverity());
+	}
+
+	public function dataFailure(): array {
+		return [
+			// description => modifiedHeaders
+			'x-robots-none' => [['X-Robots-Tag' => 'none'], "- The `X-Robots-Tag` HTTP header is not set to `noindex,nofollow`. This is a potential security or privacy risk, as it is recommended to adjust this setting accordingly.\n"],
+			'xss-protection-1' => [['X-XSS-Protection' => '1'], "- The `X-XSS-Protection` HTTP header does not contain `1; mode=block`. This is a potential security or privacy risk, as it is recommended to adjust this setting accordingly.\n"],
+			'xss-protection-0' => [['X-XSS-Protection' => '0'], "- The `X-XSS-Protection` HTTP header does not contain `1; mode=block`. This is a potential security or privacy risk, as it is recommended to adjust this setting accordingly.\n"],
+			'referrer-origin' => [['Referrer-Policy' => 'origin'], "- The `Referrer-Policy` HTTP header is not set to `no-referrer`, `no-referrer-when-downgrade`, `strict-origin`, `strict-origin-when-cross-origin` or `same-origin`. This can leak referer information. See the {w3c-recommendation}.\n"],
+			'referrer-origin-when-cross-origin' => [['Referrer-Policy' => 'origin-when-cross-origin'], "- The `Referrer-Policy` HTTP header is not set to `no-referrer`, `no-referrer-when-downgrade`, `strict-origin`, `strict-origin-when-cross-origin` or `same-origin`. This can leak referer information. See the {w3c-recommendation}.\n"],
+			'referrer-unsafe-url' => [['Referrer-Policy' => 'unsafe-url'], "- The `Referrer-Policy` HTTP header is not set to `no-referrer`, `no-referrer-when-downgrade`, `strict-origin`, `strict-origin-when-cross-origin` or `same-origin`. This can leak referer information. See the {w3c-recommendation}.\n"],
+		];
+	}
+
+	/**
+	 * @dataProvider dataFailure
+	 */
+	public function testFailure(array $headers, string $msg): void {
+		$headers = array_merge(
+			[
+				'X-XSS-Protection' => '1; mode=block',
+				'X-Content-Type-Options' => 'nosniff',
+				'X-Robots-Tag' => 'noindex, nofollow',
+				'X-Frame-Options' => 'SAMEORIGIN',
+				'Strict-Transport-Security' => 'max-age=15768000',
+				'X-Permitted-Cross-Domain-Policies' => 'none',
+				'Referrer-Policy' => 'no-referrer',
+			],
+			$headers
+		);
+		$this->setupResponse(
+			200,
+			$headers
+		);
+
+		$result = $this->setupcheck->run();
+		$this->assertEquals(
+			'Some headers are not set correctly on your instance'."\n$msg",
+			$result->getDescription()
+		);
+		$this->assertEquals(SetupResult::WARNING, $result->getSeverity());
+	}
+
+	protected function setupResponse(int $statuscode, array $headers): void {
+		$response = $this->createMock(IResponse::class);
+		$response->expects($this->atLeastOnce())->method('getStatusCode')->willReturn($statuscode);
+		$response->expects($this->any())->method('getHeader')
+			->willReturnCallback(
+				fn (string $header): string => $headers[$header] ?? ''
+			);
+
+		$this->setupcheck
+			->expects($this->atLeastOnce())
+			->method('runRequest')
+			->willReturnOnConsecutiveCalls($this->generate([$response]));
+	}
+
+	/**
+	 * Helper function creates a nicer interface for mocking Generator behavior
+	 */
+	protected function generate(array $yield_values) {
+		return $this->returnCallback(function () use ($yield_values) {
+			yield from $yield_values;
+		});
+	}
+}