Issue #2620304 by kim.pepper, andypost, phenaproxima, alexpott: htaccess...

Issue #2620304 by kim.pepper, andypost, phenaproxima, alexpott: htaccess functions should be a service

core/core.services.yml

@@ -424,6 +424,9 @@ services:
   logger.channel.form:
     parent: logger.channel_base
     arguments: ['form']
+  logger.channel.security:
+    parent: logger.channel_base
+    arguments: ['security']
   logger.log_message_parser:
     class: Drupal\Core\Logger\LogMessageParser
 
@@ -1650,6 +1653,9 @@ services:
       - { name: twig.loader, priority: -100 }
   element_info:
     alias: plugin.manager.element_info
+  file.htaccess_writer:
+    class: Drupal\Core\File\HtaccessWriter
+    arguments: ['@file_system', '@logger.channel.security']
   file.mime_type.guesser:
     class: Drupal\Core\File\MimeType\MimeTypeGuesser
     arguments: ['@stream_wrapper_manager']

core/includes/file.inc

@@ -6,13 +6,12 @@
  */
 
 use Drupal\Component\FileSystem\FileSystem as ComponentFileSystem;
-use Drupal\Component\Utility\UrlHelper;
 use Drupal\Component\PhpStorage\FileStorage;
 use Drupal\Component\Utility\Bytes;
+use Drupal\Component\Utility\UrlHelper;
 use Drupal\Core\File\FileSystem;
 use Drupal\Core\Site\Settings;
 use Drupal\Core\StreamWrapper\PublicStream;
-use Drupal\Core\StreamWrapper\PrivateStream;
 
 /**
  * Default mode for new directories. See drupal_chmod().
@@ -315,30 +314,15 @@ function file_prepare_directory(&$directory, $options = FILE_MODIFY_PERMISSIONS)
 
 /**
  * Creates a .htaccess file in each Drupal files directory if it is missing.
+ *
+ * @deprecated in Drupal 8.7.x, will be removed before Drupal 9.0.0. Use
+ * \Drupal\Core\File\HtaccessWriterInterface::ensure() instead.
+ *
+ * @see https://www.drupal.org/node/2940126
  */
 function file_ensure_htaccess() {
-  file_save_htaccess('public://', FALSE);
-  $private_path = PrivateStream::basePath();
-  if (!empty($private_path)) {
-    file_save_htaccess('private://', TRUE);
-  }
-  file_save_htaccess('temporary://', TRUE);
-
-  // If a staging directory exists then it should contain a .htaccess file.
-  // @todo https://www.drupal.org/node/2696103 catch a more specific exception
-  //   and simplify this code.
-  try {
-    $staging = config_get_config_directory(CONFIG_SYNC_DIRECTORY);
-  }
-  catch (\Exception $e) {
-    $staging = FALSE;
-  }
-  if ($staging) {
-    // Note that we log an error here if we can't write the .htaccess file. This
-    // can occur if the staging directory is read-only. If it is then it is the
-    // user's responsibility to create the .htaccess file.
-    file_save_htaccess($staging, TRUE);
-  }
+  @trigger_error("file_ensure_htaccess() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\File\HtaccessWriter::ensure() instead. See https://www.drupal.org/node/2940126", E_USER_DEPRECATED);
+  \Drupal::service('file.htaccess_writer')->ensure();
 }
 
 /**
@@ -352,31 +336,18 @@ function file_ensure_htaccess() {
  * @param bool $force_overwrite
  *   (Optional) Set to TRUE to attempt to overwrite the existing .htaccess file
  *   if one is already present. Defaults to FALSE.
+ *
+ * @return bool
+ *   TRUE when file exists or created successfully, FALSE otherwise.
+ *
+ * @deprecated in Drupal 8.7.x, will be removed before Drupal 9.0.0. Use
+ *   \Drupal\Core\File\HtaccessWriterInterface::save() instead.
+ *
+ * @see https://www.drupal.org/node/2940126
  */
 function file_save_htaccess($directory, $private = TRUE, $force_overwrite = FALSE) {
-  if (\Drupal::service('file_system')->uriScheme($directory)) {
-    $htaccess_path = file_stream_wrapper_uri_normalize($directory . '/.htaccess');
-  }
-  else {
-    $directory = rtrim($directory, '/\\');
-    $htaccess_path = $directory . '/.htaccess';
-  }
-
-  if (file_exists($htaccess_path) && !$force_overwrite) {
-    // Short circuit if the .htaccess file already exists.
-    return TRUE;
-  }
-  $htaccess_lines = FileStorage::htaccessLines($private);
-
-  // Write the .htaccess file.
-  if (file_exists($directory) && is_writable($directory) && file_put_contents($htaccess_path, $htaccess_lines)) {
-    return drupal_chmod($htaccess_path, 0444);
-  }
-  else {
-    $variables = ['%directory' => $directory, '@htaccess' => $htaccess_lines];
-    \Drupal::logger('security')->error("Security warning: Couldn't write .htaccess file. Please create a .htaccess file in your %directory directory which contains the following lines: <pre><code>@htaccess</code></pre>", $variables);
-    return FALSE;
-  }
+  @trigger_error('file_save_htaccess() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\File\HtaccessWriter::save() instead. See https://www.drupal.org/node/2940126', E_USER_DEPRECATED);
+  return \Drupal::service('file.htaccess_writer')->save($directory, $private, $force_overwrite);
 }
 
 /**
@@ -560,7 +531,7 @@ function file_unmanaged_prepare($source, &$destination = NULL, $replace = FILE_E
     return FALSE;
   }
   // Make sure the .htaccess files are present.
-  file_ensure_htaccess();
+  \Drupal::service('file.htaccess_writer')->ensure();
   return TRUE;
 }
 

core/includes/install.core.inc

@@ -1120,14 +1120,11 @@ function install_base_system(&$install_state) {
       ->save();
   }
 
-  // Call file_ensure_htaccess() to ensure that all of Drupal's standard
-  // directories (e.g., the public files directory and config directory) have
-  // appropriate .htaccess files. These directories will have already been
-  // created by this point in the installer, since Drupal creates them during
-  // the install_verify_requirements() task. Note that we cannot call
-  // file_ensure_access() any earlier than this, since it relies on
-  // system.module in order to work.
-  file_ensure_htaccess();
+  // Ensure that all of Drupal's standard directories (e.g., the public files
+  // directory and config directory) have appropriate .htaccess files. These
+  // directories will have already been created by this point in the installer,
+  // since Drupal creates them during the install_verify_requirements() task.
+  \Drupal::service("file.htaccess_writer")->ensure();
 
   // Prime the drupal_get_filename() static cache with the user module's
   // exact location.

core/lib/Drupal/Core/Config/FileStorage.php

@@ -79,7 +79,7 @@ protected function ensureStorage() {
     $success = file_prepare_directory($dir, FILE_CREATE_DIRECTORY | FILE_MODIFY_PERMISSIONS);
     // Only create .htaccess file in root directory.
     if ($dir == $this->directory) {
-      $success = $success && file_save_htaccess($this->directory, TRUE, TRUE);
+      $success = $success && $this->createHtaccess($this->directory);
     }
     if (!$success) {
       throw new StorageException('Failed to create config directory ' . $dir);
@@ -87,6 +87,20 @@ protected function ensureStorage() {
     return $this;
   }
 
+  /**
+   * Creates a .htaccess file in the given directory.
+   *
+   * @param string $directory
+   *   The directory.
+   *
+   * @return bool
+   *   TRUE if the .htaccess file was saved or already exists, FALSE otherwise.
+   */
+  private function createHtaccess($directory) {
+    // @todo Properly inject services https://www.drupal.org/node/2940135
+    return \Drupal::service('file.htaccess_writer')->save($directory, TRUE, TRUE);
+  }
+
   /**
    * {@inheritdoc}
    */

core/lib/Drupal/Core/File/HtaccessWriter.php

+<?php
+
+namespace Drupal\Core\File;
+
+use Drupal\Component\PhpStorage\FileStorage;
+use Drupal\Core\StreamWrapper\PrivateStream;
+use Drupal\Core\StringTranslation\TranslatableMarkup;
+use Psr\Log\LoggerInterface;
+
+/**
+ * Provides functions to manage Apache .htaccess files.
+ */
+class HtaccessWriter implements HtaccessWriterInterface {
+
+  /**
+   * The file system service.
+   *
+   * @var \Drupal\Core\File\FileSystemInterface
+   */
+  protected $fileSystem;
+
+  /**
+   * The logger.
+   *
+   * @var \Psr\Log\LoggerInterface
+   */
+  protected $logger;
+
+  /**
+   * Htaccess constructor.
+   *
+   * @param \Drupal\Core\File\FileSystemInterface $file_system
+   *   The file system service.
+   * @param \Psr\Log\LoggerInterface $logger
+   *   The logger.
+   */
+  public function __construct(FileSystemInterface $file_system, LoggerInterface $logger) {
+    $this->fileSystem = $file_system;
+    $this->logger = $logger;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function ensure() {
+    try {
+      foreach ($this->getHtaccessFiles() as $htaccessFile => $info) {
+        $this->save($info['directory'], $info['private']);
+      }
+
+      // If a staging directory exists then it should contain a .htaccess file.
+      // @todo https://www.drupal.org/node/2696103 catch a more specific
+      //   exception and simplify this code.
+      try {
+        $staging = config_get_config_directory(CONFIG_SYNC_DIRECTORY);
+      }
+      catch (\Exception $e) {
+        $staging = FALSE;
+      }
+      if ($staging) {
+        // Note that we log an error here if we can't write the .htaccess file.
+        // This can occur if the staging directory is read-only. If it is then
+        // it is the user's responsibility to create the .htaccess file.
+        $this->save($staging, TRUE);
+      }
+    }
+    catch (\Exception $e) {
+      $this->logger->error($e->getMessage());
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function save($directory, $private = TRUE, $forceOverwrite = FALSE) {
+    if ($this->fileSystem->uriScheme($directory)) {
+      $htaccessPath = $this->normalizeUri($directory . '/.htaccess');
+    }
+    else {
+      $directory = rtrim($directory, '/\\');
+      $htaccessPath = $directory . '/.htaccess';
+    }
+
+    if (file_exists($htaccessPath) && !$forceOverwrite) {
+      // Short circuit if the .htaccess file already exists.
+      return TRUE;
+    }
+    $htaccessLines = FileStorage::htaccessLines($private);
+
+    // Write the .htaccess file.
+    if (file_exists($directory) && is_writable($directory) && file_put_contents($htaccessPath, $htaccessLines)) {
+      return $this->fileSystem->chmod($htaccessPath, 0444);
+    }
+    else {
+      $this->logger->error("Security warning: Couldn't write .htaccess file. Please create a .htaccess file in your %directory directory which contains the following lines: <pre><code>@htaccess</code></pre>", [
+        '%directory' => $directory,
+        '@htaccess' => $htaccessLines,
+      ]);
+      return FALSE;
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getHtaccessFiles() {
+    $htaccessFiles = [];
+    $htaccessFiles['public://.htaccess'] = [
+      'title' => new TranslatableMarkup('Public files directory'),
+      'directory' => $this->fileSystem->realpath('public://'),
+      'private' => FALSE,
+    ];
+    if (PrivateStream::basePath()) {
+      $htaccessFiles['private://.htaccess'] = [
+        'title' => new TranslatableMarkup('Private files directory'),
+        'directory' => $this->fileSystem->realpath('private://'),
+        'private' => TRUE,
+      ];
+    }
+    $htaccessFiles['temporary://.htaccess'] = [
+      'title' => new TranslatableMarkup('Temporary files directory'),
+      'directory' => $this->fileSystem->realpath('temporary://'),
+      'private' => TRUE,
+    ];
+    return $htaccessFiles;
+  }
+
+  /**
+   * Wraps file_stream_wrapper_uri_normalize().
+   *
+   * @param string $uri
+   *   A URI to normalize.
+   *
+   * @return string
+   *   The normalized URI.
+   *
+   * @see file_stream_wrapper_uri_normalize()
+   */
+  private function normalizeUri($uri) {
+    return file_stream_wrapper_uri_normalize($uri);
+  }
+
+}

core/lib/Drupal/Core/File/HtaccessWriterInterface.php

+<?php
+
+namespace Drupal\Core\File;
+
+/**
+ * Interface for managing Apache .htaccess files.
+ */
+interface HtaccessWriterInterface {
+
+  /**
+   * Creates a .htaccess file in each Drupal files directory if it is missing.
+   */
+  public function ensure();
+
+  /**
+   * Creates a .htaccess file in the given directory.
+   *
+   * @param string $directory
+   *   The directory.
+   * @param bool $private
+   *   (Optional) FALSE indicates that $directory should be a web-accessible
+   *   directory. Defaults to TRUE which indicates a private directory.
+   * @param bool $forceOverwrite
+   *   (Optional) Set to TRUE to attempt to overwrite the existing .htaccess
+   *   file if one is already present. Defaults to FALSE.
+   *
+   * @return bool
+   *   TRUE if the .htaccess file was saved or already exists, FALSE otherwise.
+   */
+  public function save($directory, $private = TRUE, $forceOverwrite = FALSE);
+
+  /**
+   * Returns the list of system .htaccess files.
+   *
+   * @return array
+   *   An associative array of htaccess files keyed by path with following keys:
+   *   - title: The title of the location
+   *   - directory: The directory of the location
+   *   - private: Whether the .htaccess is placed in a directory considered to
+   *     be private.
+   */
+  public function getHtaccessFiles();
+
+}

core/modules/simpletest/simpletest.install

@@ -72,7 +72,7 @@ function simpletest_requirements($phase) {
       ]),
     ];
   }
-  elseif (!file_save_htaccess(\Drupal::root() . '/' . $site_directory, FALSE)) {
+  elseif (!\Drupal::service('file.htaccess_writer')->save(\Drupal::root() . '/' . $site_directory, FALSE)) {
     $requirements['simpletest_site_directory'] = [
       'title' => t('Simpletest site directory'),
       'value' => t('Not protected'),

core/modules/system/system.install

@@ -490,23 +490,10 @@ function system_requirements($phase) {
   if ($phase == 'runtime') {
     // Try to write the .htaccess files first, to prevent false alarms in case
     // (for example) the /tmp directory was wiped.
-    file_ensure_htaccess();
-    $file_system = \Drupal::service('file_system');
-    $htaccess_files['public://.htaccess'] = [
-      'title' => t('Public files directory'),
-      'directory' => $file_system->realpath('public://'),
-    ];
-    if (PrivateStream::basePath()) {
-      $htaccess_files['private://.htaccess'] = [
-        'title' => t('Private files directory'),
-        'directory' => $file_system->realpath('private://'),
-      ];
-    }
-    $htaccess_files['temporary://.htaccess'] = [
-      'title' => t('Temporary files directory'),
-      'directory' => $file_system->realpath('temporary://'),
-    ];
-    foreach ($htaccess_files as $htaccess_file => $info) {
+    /** @var \Drupal\Core\File\HtaccessWriterInterface $htaccessWriter */
+    $htaccessWriter = \Drupal::service("file.htaccess_writer");
+    $htaccessWriter->ensure();
+    foreach ($htaccessWriter->getHtaccessFiles() as $htaccess_file => $info) {
       // Check for the string which was added to the recommended .htaccess file
       // in the latest security update.
       if (!file_exists($htaccess_file) || !($contents = @file_get_contents($htaccess_file)) || strpos($contents, 'Drupal_Security_Do_Not_Remove_See_SA_2013_003') === FALSE) {

core/modules/system/system.module

@@ -929,13 +929,15 @@ function system_check_directory($form_element, FormStateInterface $form_state) {
     $logger->error('The directory %directory exists but is not writable and could not be made writable.', ['%directory' => $directory]);
   }
   elseif (is_dir($directory)) {
+    /** @var \Drupal\Core\File\HtaccessWriterInterface $htaccess */
+    $htaccess = \Drupal::service('file.htaccess_writer');
     if ($form_element['#name'] == 'file_public_path') {
       // Create public .htaccess file.
-      file_save_htaccess($directory, FALSE);
+      $htaccess->save($directory, FALSE);
     }
     else {
       // Create private .htaccess file.
-      file_save_htaccess($directory);
+      $htaccess->save($directory);
     }
   }
 

core/modules/system/tests/src/Functional/File/FileSaveHtaccessLoggingTest.php

@@ -24,14 +24,16 @@ public function testHtaccessSave() {
     // Verify that file_save_htaccess() returns FALSE if .htaccess cannot be
     // written and writes a correctly formatted message to the error log. Set
     // $private to TRUE so all possible .htaccess lines are written.
-    $this->assertFalse(file_save_htaccess($private, TRUE));
+    /** @var \Drupal\Core\File\HtaccessWriterInterface $htaccess */
+    $htaccess = \Drupal::service('file.htaccess_writer');
+    $this->assertFalse($htaccess->save($private, TRUE));
     $this->drupalLogin($this->rootUser);
     $this->drupalGet('admin/reports/dblog');
     $this->clickLink("Security warning: Couldn't write .htaccess file. Please…");
 
     $lines = FileStorage::htaccessLines(TRUE);
     foreach (array_filter(explode("\n", $lines)) as $line) {
-      $this->assertEscaped($line);
+      $this->assertSession()->assertEscaped($line);
     }
   }
 

core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php

@@ -88,7 +88,7 @@ public function testFileCheckDirectoryHandling() {
     // Remove .htaccess file to then test that it gets re-created.
     @drupal_unlink(file_default_scheme() . '://.htaccess');
     $this->assertFalse(is_file(file_default_scheme() . '://.htaccess'), 'Successfully removed the .htaccess file in the files directory.', 'File');
-    file_ensure_htaccess();
+    $this->container->get("file.htaccess_writer")->ensure();
     $this->assertTrue(is_file(file_default_scheme() . '://.htaccess'), 'Successfully re-created the .htaccess file in the files directory.', 'File');
     // Verify contents of .htaccess file.
     $file = file_get_contents(file_default_scheme() . '://.htaccess');

core/tests/Drupal/KernelTests/Core/File/HtaccessDeprecationTest.php

+<?php
+
+namespace Drupal\KernelTests\Core\File;
+
+use Drupal\Core\Site\Settings;
+use Drupal\KernelTests\KernelTestBase;
+
+/**
+ * Test for htaccess deprecations.
+ *
+ * @group File
+ * @group legacy
+ */
+class HtaccessDeprecationTest extends KernelTestBase {
+
+  /**
+   * Tests messages for deprecated functions.
+   *
+   * @expectedDeprecation file_save_htaccess() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\File\HtaccessWriter::save() instead. See https://www.drupal.org/node/2940126
+   * @expectedDeprecation file_ensure_htaccess() is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\File\HtaccessWriter::ensure() instead. See https://www.drupal.org/node/2940126
+   */
+  public function testDeprecatedFunctions() {
+    $public = Settings::get('file_public_path') . '/test/public';
+    file_save_htaccess($public);
+    file_ensure_htaccess();
+  }
+
+}

core/tests/Drupal/KernelTests/Core/File/HtaccessTest.php

@@ -9,42 +9,63 @@
 /**
  * Tests .htaccess file saving.
  *
+ * @coversDefaultClass \Drupal\Core\File\HtaccessWriter
  * @group File
  */
 class HtaccessTest extends KernelTestBase {
 
   /**
-   * Tests file_save_htaccess().
+   * The public directory.
+   *
+   * @var string
+   */
+  protected $public;
+
+  /**
+   * The Htaccess class under test.
+   *
+   * @var \Drupal\Core\File\HtaccessWriterInterface
+   */
+  protected $htaccess;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected static $modules = ['system'];
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+    $this->public = Settings::get('file_public_path') . '/test/public';
+    $this->htaccess = $this->container->get('file.htaccess_writer');
+  }
+
+  /**
+   * @covers ::save
    */
   public function testHtaccessSave() {
     // Prepare test directories.
-    $public = Settings::get('file_public_path') . '/test/public';
     $private = Settings::get('file_public_path') . '/test/private';
     $stream = 'public://test/stream';
 
-    // Verify that file_save_htaccess() returns FALSE if .htaccess cannot be
-    // written.
-    // Note: We cannot test the condition of a directory lacking write
-    // permissions, since at least on Windows file_save_htaccess() succeeds
-    // even when changing directory permissions to 0000.
-    $this->assertFalse(file_save_htaccess($public, FALSE));
-
     // Create public .htaccess file.
-    mkdir($public, 0777, TRUE);
-    $this->assertTrue(file_save_htaccess($public, FALSE));
-    $content = file_get_contents($public . '/.htaccess');
+    mkdir($this->public, 0777, TRUE);
+    $this->assertTrue($this->htaccess->save($this->public, FALSE));
+    $content = file_get_contents($this->public . '/.htaccess');
     $this->assertTrue(strpos($content, "SetHandler Drupal_Security_Do_Not_Remove_See_SA_2006_006") !== FALSE);
     $this->assertFalse(strpos($content, "Require all denied") !== FALSE);
     $this->assertFalse(strpos($content, "Deny from all") !== FALSE);
     $this->assertTrue(strpos($content, "Options -Indexes -ExecCGI -Includes -MultiViews") !== FALSE);
     $this->assertTrue(strpos($content, "SetHandler Drupal_Security_Do_Not_Remove_See_SA_2013_003") !== FALSE);
-    $this->assertFilePermissions($public . '/.htaccess', 0444);
+    $this->assertFilePermissions($this->public . '/.htaccess', 0444);
 
-    $this->assertTrue(file_save_htaccess($public, FALSE));
+    $this->assertTrue($this->htaccess->save($this->public, FALSE));
 
     // Create private .htaccess file.
     mkdir($private, 0777, TRUE);
-    $this->assertTrue(file_save_htaccess($private));
+    $this->assertTrue($this->htaccess->save($private));
     $content = file_get_contents($private . '/.htaccess');
     $this->assertTrue(strpos($content, "SetHandler Drupal_Security_Do_Not_Remove_See_SA_2006_006") !== FALSE);
     $this->assertTrue(strpos($content, "Require all denied") !== FALSE);
@@ -53,11 +74,11 @@ public function testHtaccessSave() {
     $this->assertTrue(strpos($content, "SetHandler Drupal_Security_Do_Not_Remove_See_SA_2013_003") !== FALSE);
     $this->assertFilePermissions($private . '/.htaccess', 0444);
 
-    $this->assertTrue(file_save_htaccess($private));
+    $this->assertTrue($this->htaccess->save($private));
 
     // Create an .htaccess file using a stream URI.
     mkdir($stream, 0777, TRUE);
-    $this->assertTrue(file_save_htaccess($stream));
+    $this->assertTrue($this->htaccess->save($stream));
     $content = file_get_contents($stream . '/.htaccess');
     $this->assertTrue(strpos($content, "SetHandler Drupal_Security_Do_Not_Remove_See_SA_2006_006") !== FALSE);
     $this->assertTrue(strpos($content, "Require all denied") !== FALSE);
@@ -66,7 +87,7 @@ public function testHtaccessSave() {
     $this->assertTrue(strpos($content, "SetHandler Drupal_Security_Do_Not_Remove_See_SA_2013_003") !== FALSE);
     $this->assertFilePermissions($stream . '/.htaccess', 0444);
 
-    $this->assertTrue(file_save_htaccess($stream));
+    $this->assertTrue($this->htaccess->save($stream));
   }
 
   /**
@@ -76,13 +97,10 @@ public function testHtaccessSave() {
    *   The URI of the file to check.
    * @param int $expected
    *   The expected file permissions; e.g., 0444.
-   *
-   * @return bool
-   *   Whether the actual file permissions match the expected.
    */
   protected function assertFilePermissions($uri, $expected) {
     $actual = fileperms($uri) & 0777;
-    return $this->assertIdentical($actual, $expected, new FormattableMarkup('@uri file permissions @actual are identical to @expected.', [
+    $this->assertSame($actual, $expected, new FormattableMarkup('@uri file permissions @actual are identical to @expected.', [
       '@uri' => $uri,
       '@actual' => 0 . decoct($actual),
       '@expected' => 0 . decoct($expected),