Issue #2950851 by mcdruid, alexpott, anavarre: invalid conf file warnings when skip_permissions_hardening is on

core/includes/install.inc

@@ -660,11 +660,14 @@ function drupal_install_system($install_state) {
  *   An optional bitmask created from various FILE_* constants.
  * @param $type
  *   The type of file. Can be file (default), dir, or link.
+ * @param bool $autofix
+ *   (optional) Determines whether to attempt fixing the permissions according
+ *   to the provided $mask. Defaults to TRUE.
  *
  * @return
  *   TRUE on success or FALSE on failure. A message is set for the latter.
  */
-function drupal_verify_install_file($file, $mask = NULL, $type = 'file') {
+function drupal_verify_install_file($file, $mask = NULL, $type = 'file', $autofix = TRUE) {
   $return = TRUE;
   // Check for files that shouldn't be there.
   if (isset($mask) && ($mask & FILE_NOT_EXIST) && file_exists($file)) {
@@ -686,7 +689,7 @@ function drupal_verify_install_file($file, $mask = NULL, $type = 'file') {
         switch ($current_mask) {
           case FILE_EXIST:
             if (!file_exists($file)) {
-              if ($type == 'dir') {
+              if ($type == 'dir' && $autofix) {
                 drupal_install_mkdir($file, $mask);
               }
               if (!file_exists($file)) {
@@ -695,32 +698,32 @@ function drupal_verify_install_file($file, $mask = NULL, $type = 'file') {
             }
             break;
           case FILE_READABLE:
-            if (!is_readable($file) && !drupal_install_fix_file($file, $mask)) {
+            if (!is_readable($file)) {
               $return = FALSE;
             }
             break;
           case FILE_WRITABLE:
-            if (!is_writable($file) && !drupal_install_fix_file($file, $mask)) {
+            if (!is_writable($file)) {
               $return = FALSE;
             }
             break;
           case FILE_EXECUTABLE:
-            if (!is_executable($file) && !drupal_install_fix_file($file, $mask)) {
+            if (!is_executable($file)) {
               $return = FALSE;
             }
             break;
           case FILE_NOT_READABLE:
-            if (is_readable($file) && !drupal_install_fix_file($file, $mask)) {
+            if (is_readable($file)) {
               $return = FALSE;
             }
             break;
           case FILE_NOT_WRITABLE:
-            if (is_writable($file) && !drupal_install_fix_file($file, $mask)) {
+            if (is_writable($file)) {
               $return = FALSE;
             }
             break;
           case FILE_NOT_EXECUTABLE:
-            if (is_executable($file) && !drupal_install_fix_file($file, $mask)) {
+            if (is_executable($file)) {
               $return = FALSE;
             }
             break;
@@ -728,6 +731,9 @@ function drupal_verify_install_file($file, $mask = NULL, $type = 'file') {
       }
     }
   }
+  if (!$return && $autofix) {
+    return drupal_install_fix_file($file, $mask);
+  }
   return $return;
 }
 

core/modules/system/system.install

@@ -436,12 +436,13 @@ function system_requirements($phase) {
     // directory. This allows additional files in the site directory to be
     // updated when they are managed in a version control system.
     if (Settings::get('skip_permissions_hardening')) {
-      $conf_errors[] = t('Protection disabled');
+      $error_value = t('Protection disabled');
       // If permissions hardening is disabled, then only show a warning for a
       // writable file, as a reminder, rather than an error.
       $file_protection_severity = REQUIREMENT_WARNING;
     }
     else {
+      $error_value = t('Not protected');
       // In normal operation, writable files or directories are an error.
       $file_protection_severity = REQUIREMENT_ERROR;
       if (!drupal_verify_install_file($site_path, FILE_NOT_WRITABLE, 'dir')) {
@@ -450,7 +451,7 @@ function system_requirements($phase) {
     }
     foreach (['settings.php', 'settings.local.php', 'services.yml'] as $conf_file) {
       $full_path = $site_path . '/' . $conf_file;
-      if (file_exists($full_path) && (Settings::get('skip_permissions_hardening') || !drupal_verify_install_file($full_path, FILE_EXIST | FILE_READABLE | FILE_NOT_WRITABLE))) {
+      if (file_exists($full_path) && !drupal_verify_install_file($full_path, FILE_EXIST | FILE_READABLE | FILE_NOT_WRITABLE, 'file', !Settings::get('skip_permissions_hardening'))) {
         $conf_errors[] = t("The file %file is not protected from modifications and poses a security risk. You must change the file's permissions to be non-writable.", ['%file' => $full_path]);
       }
     }
@@ -472,7 +473,7 @@ function system_requirements($phase) {
         ];
       }
       $requirements['configuration_files'] = [
-        'value' => t('Not protected'),
+        'value' => $error_value,
         'severity' => $file_protection_severity,
         'description' => $description,
       ];

core/modules/system/tests/src/Functional/System/SitesDirectoryHardeningTest.php

@@ -49,13 +49,16 @@ class SitesDirectoryHardeningTest extends BrowserTestBase {
     $settings = Settings::getAll();
     $settings['skip_permissions_hardening'] = TRUE;
     new Settings($settings);
-    $this->assertTrue(Settings::get('skip_permissions_hardening'), 'Able to set hardening to true');
+    $this->assertTrue(Settings::get('skip_permissions_hardening'), 'Able to set skip permissions hardening to true.');
     $this->makeWritable($site_path);
 
     // Manually trigger the requirements check.
     $requirements = $this->checkSystemRequirements();
     $this->assertEqual(REQUIREMENT_WARNING, $requirements['configuration_files']['severity'], 'Warning severity is properly set.');
-    $this->assertEqual($this->t('Protection disabled'), (string) $requirements['configuration_files']['description']['#context']['configuration_error_list']['#items'][0], 'Description is properly set.');
+    $this->assertEquals('Protection disabled', (string) $requirements['configuration_files']['value']);
+    $description = strip_tags(\Drupal::service('renderer')->renderPlain($requirements['configuration_files']['description']));
+    $this->assertContains('settings.php is not protected from modifications and poses a security risk.', $description);
+    $this->assertContains('services.yml is not protected from modifications and poses a security risk.', $description);
 
     $this->assertTrue(is_writable($site_path), 'Site directory remains writable when automatically fixing permissions is disabled.');
     $this->assertTrue(is_writable($settings_file), 'settings.php remains writable when automatically fixing permissions is disabled.');
@@ -66,7 +69,8 @@ class SitesDirectoryHardeningTest extends BrowserTestBase {
     new Settings($settings);
 
     // Manually trigger the requirements check.
-    $this->checkSystemRequirements();
+    $requirements = $this->checkSystemRequirements();
+    $this->assertEquals('Protected', (string) $requirements['configuration_files']['value']);
 
     $this->assertFalse(is_writable($site_path), 'Site directory is protected when automatically fixing permissions is enabled.');
     $this->assertFalse(is_writable($settings_file), 'settings.php is protected when automatically fixing permissions is enabled.');