Skip to content

feat(setupChecks): Enhance output for SecurityHeaders #55641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat(setupChecks): Enhance output for SecurityHeaders #55641

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae6aced
feat(setupChecks): Enhance output for SecurityHeaders
joshtrichards Oct 8, 2025
7ee98d2
chore: streamline class docblock a bit
joshtrichards Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 62 additions & 9 deletions apps/settings/lib/SetupChecks/SecurityHeaders.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,16 @@
use OCP\SetupCheck\SetupResult;
use Psr\Log\LoggerInterface;

/**
* Class SecurityHeaders
*
Comment on lines +22 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Class SecurityHeaders
*

I would strip these lines as they bring no information.

* Performs setup checks to verify that essential HTTP security headers are correctly configured
* on the Nextcloud instance. The check issues warnings or informational messages if recommended
* security headers are missing, malformed, or set to unsafe values.
*
* This class is used by the Nextcloud setup process to ensure that the web server delivers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* This class is used by the Nextcloud setup process to ensure that the web server delivers
* This class is used to ensure that the web server delivers

It’s not part of setup (as in installation), it’s in the admin overview.

* responses with proper security headers, helping to protect against common web vulnerabilities.
*/
class SecurityHeaders implements ISetupCheck {

use CheckServerResponseTrait;
Expand All @@ -39,6 +49,20 @@ public function getName(): string {
return $this->l10n->t('HTTP headers');
}

/**
* Executes the security header setup check.
*
Comment on lines +53 to +54
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Executes the security header setup check.
*

* This method sends HTTP requests to the server and analyzes the response headers
* to verify that essential security-related HTTP headers (such as X-Content-Type-Options,
* X-Robots-Tag, X-Frame-Options, X-Permitted-Cross-Domain-Policies, Referrer-Policy,
* and Strict-Transport-Security) are set correctly and meet recommended values.
*
* Returns a SetupResult indicating whether the server is properly configured,
* provides warnings for misconfiguration, or informational messages if the check
* cannot be performed.
*
* @return SetupResult Result of the security headers setup check.
Comment on lines +63 to +64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*
* @return SetupResult Result of the security headers setup check.

*/
public function run(): SetupResult {
$urls = [
['get', $this->urlGenerator->linkToRoute('heartbeat'), [200]],
Expand All @@ -50,7 +74,7 @@ public function run(): SetupResult {
'X-Permitted-Cross-Domain-Policies' => ['none', null],
];

foreach ($urls as [$verb,$url,$validStatuses]) {
foreach ($urls as [$verb, $url, $validStatuses]) {
$works = null;
foreach ($this->runRequest($verb, $url, ['httpErrors' => false]) as $response) {
// Check that the response status matches
Expand All @@ -61,21 +85,32 @@ public function run(): SetupResult {
$msg = '';
$msgParameters = [];
foreach ($securityHeaders as $header => [$expected, $accepted]) {
/* Convert to lowercase and remove spaces after comas */
/* Convert to lowercase and remove spaces after commas */
$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";
$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.',
Comment on lines +93 to +95
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not a big fan of the string split, it breaks searching for the error string in the code. What’s the added value?

[$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";
$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";
}
}
}

$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}.',
'- 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',
Expand All @@ -98,17 +133,35 @@ public function run(): SetupResult {
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";
$msg .= $this->l10n->t(
'- The `Strict-Transport-Security` HTTP header is set below the recommended minimum of `%d` seconds '
. '(current value: `%d`). '
. 'For better security, enable 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";
$msg .= $this->l10n->t(
'- The `Strict-Transport-Security` HTTP header is malformed: `%s`. '
. 'For better security, configure a valid HSTS policy. ',
[$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";
$msg .= $this->l10n->t(
'- The `Strict-Transport-Security` HTTP header is not set to the recommended minimum of `%d` seconds. '
. 'For better security, 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->l10n->t('Some headers are not set correctly on your instance.') . "\n"
. $msg . "\n"
. 'If you believe this is incorrect, review your `overwrite.cli.url` and `trusted_domains` settings. '
. 'These settings may include URLs that do not use HTTPS or bypass your reverse proxy, '
. 'which can affect header checks. '
. 'Additionally, ensure your DNS records and server configuration are consistent with your HTTPS setup.',
Comment on lines +161 to +164
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely not include unstranslated text in setupchecks results.
I’m also unsure about this added warning text, ideally we should improve the checks to make them less error-prone instead, and for special case document that in the linked documentation.
Maybe we should show a general setting on the overview page on which URL is used by the server for loopback checks, and make it configurable for special cases. Not sure.

$this->urlGenerator->linkToDocs('admin-security'),
$msgParameters,
);
Expand Down
Loading