Skip to content

Commit

Permalink
Merge pull request #44067 from nextcloud/fix/migrate-header-check-to-…
Browse files Browse the repository at this point in the history
…setupcheck

Migrate header check to setupcheck API
  • Loading branch information
come-nc authored Mar 14, 2024
2 parents d4ac4b8 + 6278cf1 commit d435f0c
Show file tree
Hide file tree
Showing 12 changed files with 401 additions and 755 deletions.
1 change: 1 addition & 0 deletions apps/settings/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@
'OCA\\Settings\\SetupChecks\\PushService' => $baseDir . '/../lib/SetupChecks/PushService.php',
'OCA\\Settings\\SetupChecks\\RandomnessSecure' => $baseDir . '/../lib/SetupChecks/RandomnessSecure.php',
'OCA\\Settings\\SetupChecks\\ReadOnlyConfig' => $baseDir . '/../lib/SetupChecks/ReadOnlyConfig.php',
'OCA\\Settings\\SetupChecks\\SecurityHeaders' => $baseDir . '/../lib/SetupChecks/SecurityHeaders.php',
'OCA\\Settings\\SetupChecks\\SupportedDatabase' => $baseDir . '/../lib/SetupChecks/SupportedDatabase.php',
'OCA\\Settings\\SetupChecks\\SystemIs64bit' => $baseDir . '/../lib/SetupChecks/SystemIs64bit.php',
'OCA\\Settings\\SetupChecks\\TempSpaceAvailable' => $baseDir . '/../lib/SetupChecks/TempSpaceAvailable.php',
Expand Down
1 change: 1 addition & 0 deletions apps/settings/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ class ComposerStaticInitSettings
'OCA\\Settings\\SetupChecks\\PushService' => __DIR__ . '/..' . '/../lib/SetupChecks/PushService.php',
'OCA\\Settings\\SetupChecks\\RandomnessSecure' => __DIR__ . '/..' . '/../lib/SetupChecks/RandomnessSecure.php',
'OCA\\Settings\\SetupChecks\\ReadOnlyConfig' => __DIR__ . '/..' . '/../lib/SetupChecks/ReadOnlyConfig.php',
'OCA\\Settings\\SetupChecks\\SecurityHeaders' => __DIR__ . '/..' . '/../lib/SetupChecks/SecurityHeaders.php',
'OCA\\Settings\\SetupChecks\\SupportedDatabase' => __DIR__ . '/..' . '/../lib/SetupChecks/SupportedDatabase.php',
'OCA\\Settings\\SetupChecks\\SystemIs64bit' => __DIR__ . '/..' . '/../lib/SetupChecks/SystemIs64bit.php',
'OCA\\Settings\\SetupChecks\\TempSpaceAvailable' => __DIR__ . '/..' . '/../lib/SetupChecks/TempSpaceAvailable.php',
Expand Down
2 changes: 2 additions & 0 deletions apps/settings/lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
use OCA\Settings\SetupChecks\PushService;
use OCA\Settings\SetupChecks\RandomnessSecure;
use OCA\Settings\SetupChecks\ReadOnlyConfig;
use OCA\Settings\SetupChecks\SecurityHeaders;
use OCA\Settings\SetupChecks\SupportedDatabase;
use OCA\Settings\SetupChecks\SystemIs64bit;
use OCA\Settings\SetupChecks\TempSpaceAvailable;
Expand Down Expand Up @@ -214,6 +215,7 @@ public function register(IRegistrationContext $context): void {
$context->registerSetupCheck(PhpOutputBuffering::class);
$context->registerSetupCheck(RandomnessSecure::class);
$context->registerSetupCheck(ReadOnlyConfig::class);
$context->registerSetupCheck(SecurityHeaders::class);
$context->registerSetupCheck(SupportedDatabase::class);
$context->registerSetupCheck(SystemIs64bit::class);
$context->registerSetupCheck(TempSpaceAvailable::class);
Expand Down
2 changes: 1 addition & 1 deletion apps/settings/lib/SetupChecks/OcxProviders.php
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public function run(): SetupResult {
];

foreach ($providers as $provider) {
foreach ($this->runHEAD($this->urlGenerator->getWebroot() . $provider) as $response) {
foreach ($this->runRequest('HEAD', $this->urlGenerator->getWebroot() . $provider, ['httpErrors' => false]) as $response) {
$testedProviders[$provider] = true;
if ($response->getStatusCode() === 200) {
$workingProviders[] = $provider;
Expand Down
160 changes: 160 additions & 0 deletions apps/settings/lib/SetupChecks/SecurityHeaders.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2023 Côme Chilliet <[email protected]>
*
* @author Côme Chilliet <[email protected]>
*
* @license GNU AGPL version 3 or any later version
*
* 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\SetupChecks;

use OCP\Http\Client\IClientService;
use OCP\IConfig;
use OCP\IL10N;
use OCP\IURLGenerator;
use OCP\SetupCheck\ISetupCheck;
use OCP\SetupCheck\SetupResult;
use Psr\Log\LoggerInterface;

class SecurityHeaders implements ISetupCheck {

use CheckServerResponseTrait;

public function __construct(
protected IL10N $l10n,
protected IConfig $config,
protected IURLGenerator $urlGenerator,
protected IClientService $clientService,
protected LoggerInterface $logger,
) {
}

public function getCategory(): string {
return 'security';
}

public function getName(): string {
return $this->l10n->t('HTTP headers');
}

public function run(): SetupResult {
$urls = [
['get', $this->urlGenerator->linkToRoute('heartbeat'), [200]],
];
$securityHeaders = [
'X-Content-Type-Options' => ['nosniff', 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($verb, $url, ['httpErrors' => false]) as $response) {
// Check that the response status matches
if (!in_array($response->getStatusCode(), $validStatuses)) {
$works = false;
continue;
}
$msg = '';
$msgParameters = [];
foreach ($securityHeaders as $header => [$expected, $accepted]) {
/* 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$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$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$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$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',
'no-referrer-when-downgrade',
'strict-origin',
'strict-origin-when-cross-origin',
'same-origin',
]
)."\n";
$msgParameters['w3c-recommendation'] = [
'type' => 'highlight',
'id' => 'w3c-recommendation',
'name' => 'W3C Recommendation',
'link' => 'https://www.w3.org/TR/referrer-policy/',
];
}

$transportSecurityValidity = $response->getHeader('Strict-Transport-Security');
$minimumSeconds = 15552000;
if (preg_match('/^max-age=(\d+)(;.*)?$/', $transportSecurityValidity, $m)) {
$transportSecurityValidity = (int)$m[1];
if ($transportSecurityValidity < $minimumSeconds) {
$msg .= $this->l10n->t('- The `Strict-Transport-Security` HTTP header is not set to at least `%d` seconds (current value: `%d`). For enhanced security, it is recommended to use a long HSTS policy.', [$minimumSeconds, $transportSecurityValidity])."\n";
}
} elseif (!empty($transportSecurityValidity)) {
$msg .= $this->l10n->t('- The `Strict-Transport-Security` HTTP header is malformed: `%s`. For enhanced security, it is recommended to enable HSTS.', [$transportSecurityValidity])."\n";
} else {
$msg .= $this->l10n->t('- The `Strict-Transport-Security` HTTP header is not set (should be at least `%d` seconds). For enhanced security, it is recommended to enable HSTS.', [$minimumSeconds])."\n";
}

if (!empty($msg)) {
return SetupResult::warning(
$this->l10n->t('Some headers are not set correctly on your instance')."\n".$msg,
$this->urlGenerator->linkToDocs('admin-security'),
$msgParameters,
);
}
// Skip the other requests if one works
$works = true;
break;
}
// If 'works' is null then we could not connect to the server
if ($works === null) {
return SetupResult::info(
$this->l10n->t('Could not check that your web server serves security headers correctly. Please check manually.'),
$this->urlGenerator->linkToDocs('admin-security'),
);
}
// 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]),
$this->urlGenerator->linkToDocs('admin-security'),
);
}
}
return SetupResult::success(
$this->l10n->t('Your server is correctly configured to send security headers.')
);
}
}
5 changes: 2 additions & 3 deletions apps/settings/src/admin.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,8 @@ window.addEventListener('DOMContentLoaded', () => {
$.when(
OC.SetupChecks.checkWebDAV(),
OC.SetupChecks.checkSetup(),
OC.SetupChecks.checkGeneric(),
).then((check1, check2, check3) => {
const messages = [].concat(check1, check2, check3)
).then((check1, check2) => {
const messages = [].concat(check1, check2)
const $el = $('#postsetupchecks')
$('#security-warning-state-loading').addClass('hidden')

Expand Down
14 changes: 7 additions & 7 deletions apps/settings/tests/SetupChecks/OcxProvicersTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ protected function setUp(): void {
$this->logger = $this->createMock(LoggerInterface::class);

$this->setupcheck = $this->getMockBuilder(OcxProviders::class)
->onlyMethods(['runHEAD'])
->onlyMethods(['runRequest'])
->setConstructorArgs([
$this->l10n,
$this->config,
Expand All @@ -79,7 +79,7 @@ public function testSuccess(): void {

$this->setupcheck
->expects($this->exactly(2))
->method('runHEAD')
->method('runRequest')
->willReturnOnConsecutiveCalls($this->generate([$response]), $this->generate([$response]));

$result = $this->setupcheck->run();
Expand All @@ -94,7 +94,7 @@ public function testLateSuccess(): void {

$this->setupcheck
->expects($this->exactly(2))
->method('runHEAD')
->method('runRequest')
->willReturnOnConsecutiveCalls($this->generate([$response1, $response1, $response1]), $this->generate([$response2])); // only one response out of two

$result = $this->setupcheck->run();
Expand All @@ -107,7 +107,7 @@ public function testNoResponse(): void {

$this->setupcheck
->expects($this->exactly(2))
->method('runHEAD')
->method('runRequest')
->willReturnOnConsecutiveCalls($this->generate([]), $this->generate([])); // No responses

$result = $this->setupcheck->run();
Expand All @@ -121,7 +121,7 @@ public function testPartialResponse(): void {

$this->setupcheck
->expects($this->exactly(2))
->method('runHEAD')
->method('runRequest')
->willReturnOnConsecutiveCalls($this->generate([$response]), $this->generate([])); // only one response out of two

$result = $this->setupcheck->run();
Expand All @@ -135,7 +135,7 @@ public function testInvalidResponse(): void {

$this->setupcheck
->expects($this->exactly(2))
->method('runHEAD')
->method('runRequest')
->willReturnOnConsecutiveCalls($this->generate([$response]), $this->generate([$response])); // only one response out of two

$result = $this->setupcheck->run();
Expand All @@ -151,7 +151,7 @@ public function testPartialInvalidResponse(): void {

$this->setupcheck
->expects($this->exactly(2))
->method('runHEAD')
->method('runRequest')
->willReturnOnConsecutiveCalls($this->generate([$response1]), $this->generate([$response2]));

$result = $this->setupcheck->run();
Expand Down
Loading

0 comments on commit d435f0c

Please sign in to comment.