htaccess functions should be a service

rework against current core, no interdiff cos lots of changes

Last patch have only part of previous

Proper patch, also added missing doc blocks, still needs tests for new service & exception?!

+++ b/core/includes/file.inc
@@ -354,29 +335,21 @@ function file_ensure_htaccess() {
+  @trigger_error('file_save_htaccess() is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\File\Htaccess::save(). See https://www.drupal.org/node/.', E_USER_DEPRECATED);

and needs CR

@phenaproxima and I worked on this. Rather than write new tests, we adapted existing test of file_save_htaccess and added test coverage of the deprecation notices.

Complete reroll for patch in #2 against latest core. Hope to get a green flag.

While working on CR https://www.drupal.org/node/2940126, I found that https://www.drupal.org/node/2418133 does not mentions about conversion of file_htaccess_lines() probably there was a reason for it... fixed

Here's updated patch adds ref to CR & fix most of failures

@chiranjeeb2410 please don't hijack issue patches

Fix test failures by replacing usage, added todo and issue #2940135: Use file_system service to \Drupal\Core\Config\FileStorage

a bit of clean-up

working on this for SprintWeekend2018

Re-roll of #22

Fixes a few tests.

Oops! Didn't mean to post my local composer changes!

Here's the correct interdiff between #31 and #28

+++ b/core/tests/Drupal/KernelTests/Core/File/HtaccessTest.php
@@ -3,48 +3,92 @@
+ * @coversDefaultClass \Drupal\Core\File\Htaccess
  * @group File
+ * @group legacy
...
+  public function testDeprecatedFunctions() {
+    file_save_htaccess($this->public);
+    file_ensure_htaccess();

It looks that test needs split or only testDeprecatedFunctions() should be marked as @legacy

Thanks. Fixes for #34

Let's get commiter's piv, imo good to go

1. +++ b/core/core.services.yml
   @@ -1644,6 +1647,9 @@ services:
   +  file.htaccess:
   

   I think a better service name would be file.htaccess_writer or even core.htaccess_writer as that's the main purpose. Maybe someone else has better suggestions but file.htaccess does not seem that great to me.

2. +++ b/core/core.services.yml
   @@ -1644,6 +1647,9 @@ services:
   +    class: Drupal\Core\File\Htaccess
   

   HtaccessWriter I think is a better name.

3. +++ b/core/lib/Drupal/Core/File/Htaccess.php
   @@ -0,0 +1,118 @@
   +    $htaccess_lines = FileStorage::htaccessLines($private);
   

   Let's move \Drupal\Component\PhpStorage\FileStorage::htaccessLines() to here and deprecate that. It was only put there to have it in an autoloadable class and this is a better home.

4. I'm not sure about the new exception and the behaviour change. Ie. file_save_htaccess() writes an error to the log but the new service::save() does not. It throws an exception. Combining functional change with refactoring to a service feels wrong to me.

Thanks for the review!

1. Fixed
2. Fixed
3. Fixed
4. I moved the logging call inside the HtAccessWriter::save() method so its BC. Only new usages of the service will need to handle the exception. Does this address the issue sufficiently?

Hmm. Seems that moving the \Drupal\Component\PhpStorage\FileStorage::htaccessLines to \Drupal\Core\File\HtaccessWriter::htaccessLines is causing a fail because we calling code Drupal\Core from within a Drupal\Component.

Any thoughts on how to resolve that apart from duplicating the code?

@kim.pepper I think we should continue with calling \Drupal\Component\PhpStorage\FileStorage::htaccessLines() then and file a follow-up to try to resolve this. The other answer is that we should try to make the HtAccessWriter a component but that is going to be nigh on impossible. For me this points to the fact that PhpStorage doesn't really feel like a component either.

Maybe we can find some other similar stuff and make a security component that has this ¯\_(ツ)_/¯

I've reverted the htaccess() move and created follow up #3013210: Add drupal/core-filesecurity component for writing htaccess files.

Remove unused import.

#37 is addressed so back to RTBC.

+++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
@@ -0,0 +1,122 @@
+    catch (\Exception $e) {
+      $this->logger->error($e->getMessage());
+      return FALSE;
+    }
...
+      $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

", [
+ '%directory' => $directory,
+ '@htaccess' => $htaccess_lines,
+ ]);
+ throw new HtaccessNotWriteableException($directory, $htaccess_lines);

This will result in duplicate errors logged. Why do we want this to throw an exception? If we feel that that is the better architecture then I guess that we could make that change here but it feels an unnecessary change. We need on some level to justify this change.

Re #47 I've removed the exception.

I also added an interface.

Interface additions is good. Back to RTBC as #47 is addressed.

One thing I'm asking myself is does this need to be its own service. One thought was to add this to FileSystem but OTOH this is really a HTTP server detail. The next question I asked myself is have we moved all the htaccess logic possible to the new service and I think there's something we're missing. I think it might be worth moving much of the code from system_requirements() here. So that checking and ensuring is done in the same place. Perhaps there is other htaccess related code dotted about the system that could be moved here.

Here's the current code:

  // Test the contents of the .htaccess files.
  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) {
      // 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) {
        $url = 'https://www.drupal.org/SA-CORE-2013-003';
        $requirements[$htaccess_file] = [
          'title' => $info['title'],
          'value' => t('Not fully protected'),
          'severity' => REQUIREMENT_ERROR,
          'description' => t('See <a href=":url">@url</a> for information about the recommended .htaccess file which should be added to the %directory directory to help protect against arbitrary code execution.', [':url' => $url, '@url' => $url, '%directory' => $info['directory']]),
        ];
      }
    }
  }

We could move this into the new service as something like:

  public function checkRequirements() {
    $requirements = [];
    $htaccess_files['public://.htaccess'] = [
      'title' => new TranlatableMarkup('Public files directory'),
      'directory' => $this->fileSystem->realpath('public://'),
    ];
    if (PrivateStream::basePath()) {
      $htaccess_files['private://.htaccess'] = [
        'title' => new TranlatableMarkup('Private files directory'),
        'directory' => $this->fileSystem->realpath('private://'),
      ];
    }
    $htaccess_files['temporary://.htaccess'] = [
      'title' => new TranlatableMarkup('Temporary files directory'),
      'directory' => $this->fileSystem->realpath('temporary://'),
    ];
    foreach ($htaccess_files 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) {
        $url = 'https://www.drupal.org/SA-CORE-2013-003';
        $requirements[$htaccess_file] = [
          'title' => $info['title'],
          'value' =>  new TranlatableMarkup('Not fully protected'),
          'severity' => REQUIREMENT_ERROR,
          'description' =>  new TranlatableMarkup('See <a href=":url">@url</a> for information about the recommended .htaccess file which should be added to the %directory directory to help protect against arbitrary code execution.', [':url' => $url, '@url' => $url, '%directory' => $info['directory']]),
        ];
      }
    }
    return $requirements;
  }

And maybe even refactor the list of directories into a protected method to be shared by the ensure method - so that list is always obtained from the same place.

@alexpott requirements sounds like follow-up and could be merged with #2909480: Move REQUIREMENT_* constants out of install.inc file

@andypost there thing is we're introducing a new interface so now is a good time to get it right.

#50 sounds like a nice cleanup.

+++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
@@ -25,6 +26,17 @@ class HtaccessWriter implements HtaccessWriterInterface {
+  protected $htaccessFiles;

@@ -102,4 +114,55 @@ public function save($directory, $private = TRUE, $force_overwrite = FALSE) {
+    foreach ($this->htaccessFiles() as $htaccessFile => $info) {
...
+  protected function htaccessFiles() {
+    $this->htaccessFiles['public://.htaccess'] = [

Any reason to use protected var if you always call a method?

Ah good point!

I've also re-used the htaccessFiles() method in HtaccessWriter::save() as @alexpott suggested in #50

Removal of unneeded changes

+++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
@@ -0,0 +1,158 @@
+  public function checkRequirements() {
...
+        $requirements[$htaccessFile] = [
+          'title' => $info['title'],
+          'value' => new TranslatableMarkup('Not fully protected'),
...
+    return $requirements;

Naming... maybe better to call it getter...

Fixes #57

Also changed variables to camelCase for consistency.

Looks good, couple of observations

1. +++ b/core/includes/install.core.inc
   @@ -1127,7 +1127,7 @@ function install_base_system(&$install_state) {
      // 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.
   

   this comment is now outdated

2. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -87,6 +87,20 @@ protected function ensureStorage() {
   +  protected function createHtaccess($directory) {
   

   Given we're going to remove this at some stage, should we make it private instead?

3. +++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
   @@ -0,0 +1,158 @@
   +      // @todo https://www.drupal.org/node/2696103 catch a more specific exception
   +      //   and simplify this code.
   

   nit - phps > 80 chars

4. +++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
   @@ -0,0 +1,158 @@
   +      $htaccessPath = file_stream_wrapper_uri_normalize($directory . '/.htaccess');
   

   Do we want this call to procedural code to be wrapped in a private method so we can extract easily later?

5. +++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
   @@ -0,0 +1,158 @@
   +      // in the latest security update.
   

   should we remove the 'latest security update' comment now, its not really correct anymore?

6. +++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
   @@ -0,0 +1,158 @@
   +   * Return the list of HtAccess files to scan.
   

   nit: this is the only place we capitalize Access. should it just be '.htacess files to scan'?

All feedback in #59 has been addressed so back to rtbc

+++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
@@ -0,0 +1,172 @@
+        $staging = config_get_config_directory(CONFIG_SYNC_DIRECTORY);

Is there already an issue to move this out of bootstrap.inc?

Rest of the patch looks OK, this looks slightly odd as a service to me, but it's probably because it's only being called by other legacy procedural code atm.

There's no issue to deal with the $config_directories global. Configuring this in settings.php and having it available as a global feels super unnecessary now. I think this is a legacy of having an active directory and the live config being stored there. I think we could move this to $settings now and use the Settings singleton. I've created #3018015: Move the $config_directories to $settings so it can be accessed via the Settings singleton

I was wrong there was an issue to deal with $config_directories - #2980712: Define config directory in settings

+++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
@@ -0,0 +1,172 @@
+    foreach ($this->getHtaccessFiles() as $htaccessFile => $info) {
...
+  protected function getHtaccessFiles() {

+++ b/core/lib/Drupal/Core/File/HtaccessWriterInterface.php
@@ -0,0 +1,42 @@
+  public function checkRequirements();

+++ b/core/modules/system/system.install
@@ -490,35 +490,10 @@ function system_requirements($phase) {
-    foreach ($htaccess_files 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) {
-        $url = 'https://www.drupal.org/SA-CORE-2013-003';
-        $requirements[$htaccess_file] = [
-          'title' => $info['title'],
-          'value' => t('Not fully protected'),
-          'severity' => REQUIREMENT_ERROR,
-          'description' => t('See <a href=":url">@url</a> for information about the recommended .htaccess file which should be added to the %directory directory to help protect against arbitrary code execution.', [':url' => $url, '@url' => $url, '%directory' => $info['directory']]),
-        ];
-      }
-    }

Looking at this now. I'm not sure that the requirements stuff should be here on the interface. In some future world where requirement gathering will be an event maybe this service will listen to that event can construct the appropriate Requirement objects to place text on the requirements page but we're a long way from that. We could consider making getHtaccessFiles() public and using that in system_requirements(). I realise that the existence of checkRequirements on the interface is my fault - cf #50. So here's a patch

I think you're right re the requirements stuff. LGTM.

Committed 56ccba0 and pushed to 8.7.x. Thanks!

+++ b/core/includes/file.inc
@@ -560,7 +531,7 @@ function file_unmanaged_prepare($source, &$destination = NULL, $replace = FILE_E
   }
   // Make sure the .htaccess files are present.
-  file_ensure_htaccess();
+  \Drupal::service('file.htaccess_writer')->ensure();
   return TRUE;

+++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
@@ -0,0 +1,143 @@
+   * {@inheritdoc}
+   */
+  public function save($directory, $private = TRUE, $forceOverwrite = FALSE) {
+    if ($this->fileSystem->uriScheme($directory)) {
+      $htaccessPath = $this->normalizeUri($directory . '/.htaccess');
+    }
+    else {

Sorry for being late to the party, but this introduces some tricky cross-dependency issues.

file_unmanaged_prepare() is being moved into the fileSystem service in #2244513: Move the unmanaged file APIs to the file_system service (file.inc). Which is injected into this new service. That means they will both need each other.

Similar with the config stuff, not sure what the plan is with config_get_config_directory(), but it's a bit strange that this low-level API is calling out to the config system, and then Config\FileStorage uses this API as well. Likely config_get_config_directory() will be even more low-level and possibly just a static function (maybe on Settings?), so possibly that's not a big issue.

config_get_config_directory() is being dealt with here #2980712: Define config directory in settings.

file_unmanaged_prepare() is a bit trickier to deal with - only thing I can really think of right now is incorporating this into the fileSystem service?

Reverted the patch so we can discuss #69 more.

One advantage of having this as a separate service is that nginx users (or other webserver users) can swap this out for different implementations. Which I think I think is a good thing. So maybe an event that'll allow us to break the circular dependency is a good idea (as per @Berdir in Slack).

If we agree that having an event in the file_system service is the best way forward, then I think we can commit this again and then introduce that over there? We can only do it after one of these issues is in anyway.

Agree that an event is a good idea. :-)

I don't think we should add events solely to break circular dependencies unless there's no other choice, there's still a circular dependency when we do that, just one that's obscured by a layer of indirection.

However alexpott reminded me about issues like #2619250: Make .htaccess usage work for the widest possible configurations without relaxing security and document pitfalls which provide a use-case for making things alterable here, so that seems a better reason to do it.

So are we agreed this approach is the preferred one? Or should we postpone on #2244513: Move the unmanaged file APIs to the file_system service (file.inc)

This doesn't seem to handle the htaccess writing that's done in \Drupal\Component\PhpStorage::ensureDirectory().

+++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
@@ -0,0 +1,143 @@
+<?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 {

Also, because Drupal\Component\PhpStorage::ensureDirectory is in Drupal\Component, that wouldn't be able to use this service, which is in Drupal\Core.

@joachim surely we can't use services in component namespace, that's why it using file_ functions and duplicates code a bit, but nice catch... not sure we can do anything about it

> surely we can't use services in component namespace

Could we put this new htaccess service in the Component namespace, and then Component\PhpStorage can depend on it?

If there are aspects of the htaccess service that need to be in the Core namespace, we can add a 2nd htaccess service there that subclasses / replaces / wraps the Component namespace service.

That might be an improvement, but I don't think it untangles the circular dependencies.

Here is a reroll of #65.

1. +++ b/core/includes/file.inc
   @@ -354,31 +338,18 @@ function file_ensure_htaccess() {
    /**
   @@ -563,7 +534,7 @@ function file_unmanaged_prepare($source, &$destination = NULL, $replace = FILE_E
   
   @@ -563,7 +534,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;
    }
    
   

   this function is now deprecated, so we might want to not even touch it. However, what we do need to touch now is the one inside file_system that replaces this. In #3034072: Move file uri/scheme functions from file.inc and FileSystem to StreamWrapperManager I realized that prepare stuff is even more fun than I thought, because it uses file_build_uri(), which in turn uses \file_default_scheme(), which uses \Drupal::config().

   With the final patch for the unmanaged functions, that is now a protected function that is not called from the outside anymore, it's just for copy() and move().

   Still thinking how we can make that a service or so, but it seems a bit weird that the crucial validation of source and destination would be in a separate service. One option might be to deprecate the empty-destination thing to get rid of that at least?

   And then it's just this htaccess stuff, we could do a setter-injection of this service in file_system?

2. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -87,6 +87,20 @@ protected function ensureStorage() {
   +  private function createHtaccess($directory) {
   +    // @todo Properly inject services https://www.drupal.org/node/2940135
   +    return \Drupal::service('file.htaccess_writer')->save($directory, TRUE, TRUE);
   +  }
   

   In that issue, I proposed to use a get/set pattern with optional injection for file_system, we could possibly do the same for this service here? Becaue right now, that other service is already RTBC, but we'll see if it gets in.

> That might be an improvement, but I don't think it untangles the circular dependencies.

Where do we get circularity?

I think that we should take a step back here, and pause work on the patch. We should research in the code all the places that need to write htaccess files, and then figure out the dependency hierarchy.

Re-roll of #88 adding the setter injection of htaccessWriter into FileSystem service as per @Berdir's suggestion in #84.

Re: #90 @joachim the circular dependency is: FileSystem -> HtaccessWriter -> FileSystem, hence the setter injection here.

Looks like symfony is detecting a circular reference even with setter injection. Trying a lazy-loading approach instead.

1. +++ b/core/includes/file.inc
   @@ -319,30 +318,15 @@ function file_prepare_directory(&$directory, $options = FileSystemInterface::MOD
   +  @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);
   

   8.8 now

2. +++ b/core/includes/install.core.inc
   @@ -1113,14 +1113,11 @@ function install_base_system(&$install_state) {
   +  // 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();
   

   single quotes?

3. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -87,6 +87,20 @@ protected function ensureStorage() {
   +  private function createHtaccess($directory) {
   +    // @todo Properly inject services https://www.drupal.org/node/2940135
   +    return \Drupal::service('file.htaccess_writer')->save($directory, TRUE, TRUE);
   +  }
   

   this issue was closed with the final approach being exactly this, so we can remove the todo, maybe comment why we do it like this.

4. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -516,8 +523,7 @@ protected function prepareDestination($source, &$destination, $replace) {
        // Make sure the .htaccess files are present.
   -    // @todo Replace with a service in https://www.drupal.org/project/drupal/issues/2620304.
   -    file_ensure_htaccess();
   +    $this->getHtaccessWriter()->ensure();
   

   Not sure what to do with this.

   I'm not sure why these two lines even exist. They have been like this since 2009, as part of the stream wrapper API introduction. (still inlined into copy back then)

   But it's just about ensuring the .htaccess files in the root-level public://, private:// and so on exist, why does a copy/move need to ensure that?

   Do we have any tests that fail if we remove it?

5. +++ b/core/tests/Drupal/KernelTests/Core/File/DirectoryTest.php
   @@ -94,7 +94,7 @@ public function testFileCheckDirectoryHandling() {
        $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');
   

   some more double quotes.

6. +++ b/core/tests/Drupal/KernelTests/Core/File/HtaccessTest.php
   @@ -9,42 +9,63 @@
   +
   +  /**
   +   * The Htaccess class under test.
   +   *
   +   * @var \Drupal\Core\File\HtaccessWriterInterface
   +   */
   +  protected $htaccess;
   

   HtaccessWriter maybe, for variable and inline reference?

1. Fixed
2. Fixed
3. Fixed
4. I'll do this in a separate patch
5. Fixed
6. Renamed the variable, but IMO it makes it easier to read to keep it as a property and initialise it in the setUp()

Oops forgot to add patches.

Here's a patch for #94.4 removing the call to $this->getHtaccessWriter()->ensure() inside prepareDestination()

+++ b/core/lib/Drupal/Core/File/FileSystem.php
@@ -515,9 +515,6 @@ protected function prepareDestination($source, &$destination, $replace) {
       throw new FileException("File '$source' could not be copied because it would overwrite itself.");
     }
-    // Make sure the .htaccess files are present.
-    // @todo Replace with a service in https://www.drupal.org/project/drupal/issues/2620304.
-    file_ensure_htaccess();
   }

The big question is whether this is OK. I asked @alexpott and @dww about this, but they weren't sure either. I *think* it is, because they only care about the top-level directory and prepareDestination() will at most once in a lifetime of a drupal site attempt to actually create sites/default/files (not sure it tries to do that at all), so why does it need to check that every. single. time we copy/move a file around? Makes no sense to me :)

So where do we go from here? I'm tagging Needs framework manager review to get some input from the framework managers themselves.

I guess that makes sense, and also back to RTBC, as I said, IMHO this removal makes sense :)

This is a pretty good example where OOP shines. We're now able to swap this implementation for another that nullifies the behaviour, i.e. if you use nginx as a web server, there is no need for .htaccess files to be created.

Sweet :)

Reroll after #3021434: Properly deprecate drupal_unlink()

Just a re-roll so back to RTBC

Re-roll of #103

Updated to use latest deprecation message format.

Re-roll of #106. Also updated deprecation message format to comply with coding standards.

Removes usages of deprecated FileSystem::uriScheme() and file_stream_wrapper_uri_normalize()

Re-roll after #2980712: Define config directory in settings

Fail due to #3059332: Mark kernel tests that perform no assertions as risky

Gah, not sure why file_save_htaccess($public) is false in the deprecation test?

1. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -79,7 +79,7 @@ protected function ensureStorage() {
        // 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) {
   
   +++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
   @@ -0,0 +1,133 @@
   +
   +      $staging = Settings::get('config_sync_directory', 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);
   +      }
   

   I'm wondering if the call in FileStorage is even still necessary. We are explicitly checking the staging idrectory in the ensure() method and support for other config directories is deprecated per https://www.drupal.org/node/3018145.

   But maybe we can leave it like this and think about that in a follow-up? Now that it's a private method, we can always remove it later.

2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -505,9 +505,6 @@ protected function prepareDestination($source, &$destination, $replace) {
          throw new FileException("File '$source' could not be copied because it would overwrite itself.");
        }
   -    // Make sure the .htaccess files are present.
   -    // @todo Replace with a service in https://www.drupal.org/project/drupal/issues/2620304.
   -    file_ensure_htaccess();
      }
   

   I pointed out before why I think it is OK to remove this call, see #98

3. Gah, not sure why file_save_htaccess($public) is false in the deprecation test?

   Because the parent directory doesn't exist apparently, adding this line above makes it work:

   \Drupal::service('file_system')->prepareDirectory($public, FileSystemInterface::CREATE_DIRECTORY);

   Lets add that to make it a bit more realistic, then I'll set it back to RTBC.

1. Created follow-up #3067440: Remove call to htaccess->ensure() in config FileStorage
2. Agree. We haven't had a framework manager agree to this yet though.
3. Fixed

2. Yup, this was meant as a pointer for said framework maintainer :)

Back to RTBC.

Re-roll of #118

Updated follow-up: #3013210-3: Add drupal/core-filesecurity component for writing htaccess files

1. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -87,6 +87,23 @@ protected function ensureStorage() {
   +  private function createHtaccess($directory) {
   +    return \Drupal::service('file.htaccess_writer')->save($directory, TRUE, TRUE);
   

   so we didn't opt for the event? I can't see any discussion of why the event idea was dropped after comment #77

2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -508,9 +508,6 @@ protected function prepareDestination($source, &$destination, $replace) {
   -    // Make sure the .htaccess files are present.
   -    // @todo Replace with a service in https://www.drupal.org/project/drupal/issues/2620304.
   -    file_ensure_htaccess();
   

   I'm also not sure we can do this.

   Yes, we setup this file on install, but in some cases we copy databases around or migrate sites from one host to another, I think in those cases, we should be still ensuring this file exists.

3. +++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
   @@ -0,0 +1,133 @@
   +    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

   ", [
   + '%directory' => $directory,
   + '@htaccess' => $htaccessLines,
   + ]);
   

   we don't need this else, we return above.

4. +++ b/core/modules/system/system.install
   @@ -498,23 +498,10 @@ function system_requirements($phase) {
   +    $htaccessWriter->ensure();
   

   hmm, although we do end up creating them again here... which will alert the admin if update.module is enabled - see update_page_top

   What if we put a call somewhere like drupal_flush_all_caches as well, then perhaps we could do away with it in prepareDirectory

5. +++ b/core/modules/system/system.module
   +++ b/core/modules/system/system.module
   @@ -933,13 +933,15 @@ function system_check_directory($form_element, FormStateInterface $form_state) {
   

   wow this function 🤔 only used by locale module but defined in system.module and in RAM on every drupal page request.

   Worth a follow up to deprecate it/move it to an include/move it to locale.module?

1. Yeah, I agree that got lost in the confusion. I read that comment from @catch and it sounded like he was against the idea of an event. But re-reading he says he thinks would be an ok situation for it.
   
   What would that look like? Would we just have an 'DirectoryCreated' event and have the HtAccessWriter a subscriber? If users are using nginx + php-fpm could just remove the subscriber?
2. @Berdir commented in #98
   

   The big question is whether this is OK. I asked @alexpott and @dww about this, but they weren't sure either. I *think* it is, because they only care about the top-level directory and prepareDestination() will at most once in a lifetime of a drupal site attempt to actually create sites/default/files (not sure it tries to do that at all), so why does it need to check that every. single. time we copy/move a file around? Makes no sense to me :)

3. Patching coming...
4. I don't quite follow what you're suggesting here. 🤔
5. Created #3073684: [pp-1] Deprecate system_check_directory()

Patch for #123.3

I don't quite follow what you're suggesting here. 🤔

I'm saying that the ::ensure call also happens when hook_requirements fires.
Which seems to occur from \Drupal\system\Controller\SystemController::overview which means any admin overview page would also trigger this, so maybe we don't need it for prepareDirectory.

Confirmed with @larowlan in slack that he meant prepareDestination() in #125, so I think that addresses #122.2 & .4

Just need some consensus on the event design decision?

Had a go at using an event in config/FileStorage.php and it introduced a lot of changes.

Putting it up here to see what blows up.

affecting this constructors always reminds me about related #2657548: [PP-1] Add a factory method to create FileStorage instances

Fixing some test fails.

Having trouble tracking down FileStorage::__construct() with a NULL $event_dispatcher. 🤔

So here's the test fail for DefaultConfigTest:

Remaining deprecation notices (2)

  2x: Calling FileStorage::__construct() without the $event_dispatcher argument is deprecated in drupal:8.8.0 and is required in drupal:9.0.0. See https://www.drupal.org/node/2940126
    2x in DefaultConfigTest::testDefaultConfig from Drupal\KernelTests\Core\Config

Here's DefaultConfigTest::testDefaultConfig():

  public function testDefaultConfig() {
    $typed_config = \Drupal::service('config.typed');
    // Create a configuration storage with access to default configuration in
    // every module, profile and theme.
    $default_config_storage = new TestInstallStorage();
[ .. ]

What's a TestInstallStorage? It's a subclass of InstallStorage which is a subclass of FileStorage.

So there's one. :-)

Same deal for TestViewsTest.

There's also ExtensionInstallStorage which suffers the same fate in a few different tests, like for instance ExtensionInstallStorageTest.

Ouch. Maybe we can explore the event subscriber idea in a follow-up? Adding 40kb to this patch to avoid a single \Drupal::service() is pretty wild. We've struggled with dependencies of FileStorage before in #2940135: Use file_system service to \Drupal\Core\Config\FileStorage And while that was even worse due to cross-dependency, part of the decision was that we'd need so many changes.

Yep sure. I guess that's a question for the framework managers?

I'd say it's best time to summon cmi2 initiative here - config storage is pita to extend but it often required, meantime they manage https://www.drupal.org/node/2943918
It looks more like "super class" which needs split, file system also becomes more "smart" so probably htaccess writer could live at composer level...

+++ b/core/lib/Drupal/Core/Config/BootstrapConfigStorageFactory.php
@@ -54,7 +54,7 @@ public static function getDatabaseStorage() {
+    return new FileStorage(config_get_config_directory(CONFIG_ACTIVE_DIRECTORY), StorageInterface::DEFAULT_COLLECTION, \Drupal::service('event_dispatcher'));

yeah, sorry about the side-track, but we're kind of playing whack-a-mole here, one singleton call swapped for another. I'll discuss with another framework manager, but I'm leaning towards this not being worth it, unless we have a solid use-case for someone else implementing an event listener.

OK cool. I think to untangle the mess we first need to fix:

1. #2838474: Remove dependency of "file_system" service on "logger"
2. then #2657548: [PP-1] Add a factory method to create FileStorage instances

This should help manage FileStorage changes going forward allowing us to introduce events etc.

Since we've agreed we're not going down the event route just now, here's a re-roll of #124 to get us back on track.

Per #127 and #138, lets try this again with the previous approach. We have issues in #139 to improve DI and avoid circular dependencies, until then, I think going with the full DI approach would be extremely challenging.

I have pinged the security team for a review of the removed call in every prepareDestination

In #98 Berdir asked if it's okay to remove the call to "ensure" from prepareDestination().

One argument for ensuring that the .htaccess files are in place in the main files directories (/ stream wrappers) regularly is that it's not uncommon for (e.g. object injection) exploits to be able to delete files.

If this happens and the .htaccess in the top of the public files directory is removed, an attacker may then be able to upload php files which the webserver would execute (in certain configurations).

Ensuring that the top-level .htaccess is in place before every upload mitigates this risk.

If that's removed, what else regularly ensures that the main files directories are protected? Per larowlan (in slack) it looks like with the patch from #140 the check is done on "a) status report b) install c) every admin/{system|config|help} etc page that uses the menu overview listing".

Perhaps that's sufficient in many cases, but it's not hard to imagine a site which would become more vulnerable as a result of this change.

If it's problematic (because of dependencies) to do the check from prepareDestination(), one option may be to add a check on cron runs? Not a perfect replacement, but it would offer a little extra protection.

Patch which adds the htaccess check to system_cron().

I did also consider adding it to file_cron(), but we run this check from system_requirements() at runtime, and not file_requirements().

You could possibly argue either way.

Let's see whether tests are okay with it added to system_cron().

If we want to keep this, we should add a test which removes a .htaccess file and then runs cron, to ensure it gets added back in again.

\Drupal\KernelTests\Core\File\DirectoryTest::testFileCheckDirectoryHandling already has this:

    // Remove .htaccess file to then test that it gets re-created.
    @$file_system->unlink($default_scheme . '://.htaccess');
    $this->assertFalse(is_file($default_scheme . '://.htaccess'), 'Successfully removed the .htaccess file in the files directory.', 'File');
    $this->container->get('file.htaccess_writer')->ensure();
    $this->assertTrue(is_file($default_scheme . '://.htaccess'), 'Successfully re-created the .htaccess file in the files directory.', 'File');

We could add another test which does the same thing but runs cron instead of calling ensure() directly.

I'm happy to work on that, but will wait for further (security) review before doing so.

Hey folks. Not to be a downer here or anything... But.. :-)

In #3057094-101: Add Composer vendor/ hardening plugin to core we need to be able to generate htaccess and web.config files to place within the vendor directory in a Composer plugin context (meaning: No Drupal core.) If we don't change anything about how drupal/core-php-storage works, then we're golden. However:

In #3013210-4: Add drupal/core-filesecurity component for writing htaccess files there's the thought of further teasing this htaccess behavior out into its own component.

The thing is, we need to do one or the other, sooner rather than later, so that we can progress on the Composer initiative. #3057094: Add Composer vendor/ hardening plugin to core is now blocked on #3013210: Add drupal/core-filesecurity component for writing htaccess files, which could also mean being blocked on this issue.

We're blocked because we want to build a Composer project template, and once that template out in the wild, it might well be tricky for users to update if our Composer plugin suddenly has different dependencies than the lock file they committed.

Personally I'm +1 on making another component that only knows how to make htaccess files in #3013210: Add drupal/core-filesecurity component for writing htaccess files, and decorating it as a service in this issue.

Personally I'm +1 on making another component that only knows how to make htaccess files in #3013210: Add drupal/core-filesecurity component for writing htaccess files, and decorating it as a service in this issue.

Yeah I think that makes the most sense. There is so many dependencies in here that there's no way a replacement for file_ensure_htaccess() and file_save_htaccess() can be done with a component alone.

Postponing on #3013210: Add drupal/core-filesecurity component for writing htaccess files

If/when #3013210: Add drupal/core-filesecurity component for writing htaccess files goes in I think we can replace lots of usages of file_save_htaccess() with just FileSecurity::writeHtaccess() component calls, rather than calling this new service. The component has zero dependencies, whereas this depends on StreamWrapperManager, FileSystem, and Logger adding a lot of complexity for such a low-level service.

The only differences now are:

1. This code allows you to provide a stream wrapper
2. rtrims trailing slashes
3. Logs an error if the htaccess file couldn't be written

Let's do a review as part of this issue once the FileSecurity component lands.

#3013210: Add drupal/core-filesecurity component for writing htaccess files just landed 🎉

Here's a re-roll of #144.

I like the cron idea!

Fixes test due to error message in dblog, trying to get the realpath() when calling \Drupal\Core\File\HtaccessWriter::ensure(). We don't need to use the realpath for file_put_contents().

This means we can remove file_system as a dependency of HtaccessWriter! 🎉

Oops. Forgot to use $this->logger().

So now HtaccessWriter::save() is just:

1. Normalizing a stream wrapper or trimming trailing slashes from a directory
2. Logging an error if it cannot be written.

I'm wondering how we provide guidance around when to use HtaccessWriter::save() over just FileSecurity::writeHtaccess() ?

@kim.pepper thanks! code looks much more clear now!

Btw looking through usage it looks there only 2 public methods needed for each directory ("ensure" + "getState") + internal "write" can catch real file system exceptions and use logger.
The static list (public, temp) ... [private] all could be declared in file_system as core defaults so writer can read it there.

+++ b/core/lib/Drupal/Core/File/HtaccessWriter.php
@@ -0,0 +1,111 @@
+    $htaccessFiles['temporary://.htaccess'] = [
+      'title' => new TranslatableMarkup('Temporary files directory'),
+      'directory' => 'temporary://',
+      'private' => TRUE,
+    ];

+++ b/core/lib/Drupal/Core/File/HtaccessWriterInterface.php
@@ -0,0 +1,44 @@
+   * @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();

Better to convert array to valueObject... kinda fileSytem::getFileSecurity($dir...) then use its methods ensure() and status() (which is used a lot in system module)

Thanks @andypost

I've moved HtaccessWriter::save() to a protected method (and re-named to write()). I've also updated the file_save_htaccess() deprecation message to refer people to FileSecurity. However, we still need to maintain BC so I've duplicated the streamwrapper normalize call and logging in the deprecated function.

I've also replaced usages in system_check_directory() to call FileSecurity::writeHtaccess() directly, as we a showing form errors if there is a problem.

I also liked the idea of a value object, so created \Drupal\Core\File\ProtectedDirectory and re-wrote getHtaccessFiles() to defaultProtectedDirs() that returns a ProtectedDirectory objects.

Let's see how this goes!

I also liked the idea of a value object, so created \Drupal\Core\File\ProtectedDirectory and re-wrote getHtaccessFiles() to defaultProtectedDirs() that returns a ProtectedDirectory objects.

Forgot to mention I didn't move this to FileSystem, as I wanted to avoid having HtaccessWriter depend on FileSystem and re-introduce the circular dependency!

Replace use of HtaccessWriter::save() with FileSecurity::writeHtaccess() in simpletest.install.

A couple of fails since htaccessWriter::write() was made protected. Changed it to public and @internal instead.