Permission denied caused by race condition during ensureDirectory should be silenced

Thanks for fixing this bug. Race conditions are hard to test and recreate. At the very least we need a comment explaining why something is silenced.

The old patch doesn't work for Drupal 8.8.0 and also the warning is coming from a new file.
core/lib/Drupal/Component/FileSecurity/FileSecurity.php

This is the warning I get. I've created a new patch for it.

Warning: file_put_contents(.../twig/5df8f5904ff7b_field--node--title.html.t_U47XKAxSeaTanWRelNR6hRFQD//.htaccess): failed to open stream: Permission denied in Drupal\Component\FileSecurity\FileSecurity::writeFile() (line 158 of .../core/lib/Drupal/Component/FileSecurity/FileSecurity.php)

I actually encountered this randomly on a project. What turned out to fix it was the patch attached. We add the directory separator in \Drupal\Component\FileSecurity\FileSecurity::writeFile(). I think having two slashes breaks the file_exists($file_path) check.

nice finding. would it be possible to have a test?

@vijaycs85 I don't really know how to... it seems something based on environment and timing but once I made the change in #10 the random error went away.

I think we have comprehensive coverage of the class in \Drupal\Tests\Component\FileSecurity\FileSecurityTest and we know that the change in #10 is correct and that the double directory slash in

Warning: file_put_contents(.../twig/5df8f5904ff7b_field--node--title.html.t_U47XKAxSeaTanWRelNR6hRFQD//.htaccess): failed to open stream: Permission denied in Drupal\Component\FileSecurity\FileSecurity::writeFile() (line 158 of .../core/lib/Drupal/Component/FileSecurity/FileSecurity.php)

is wrong.

I've applied the patch of #10, but it doesn't solve the problem for me. The warnings occur after clearing the drupal caches, also when writing the twig php template files:

Warning: file_put_contents(sites/website.nl/files/php/twig/5e9ea69050773_region--messages.html.twi_1zZlP40KCScZ9hppQnJELmqDQ/.htaccess): failed to open stream: Permission denied in Drupal\Component\FileSecurity\FileSecurity::writeFile() (line 158 of /home/website/domains/website.nl/web/core/lib/Drupal/Component/FileSecurity/FileSecurity.php)

Warning: file_put_contents(sites/website.nl/files/php/twig/5e9ea69050773_region--messages.html.twi_1zZlP40KCScZ9hppQnJELmqDQ/.htaccess): failed to open stream: Permission denied in Drupal\Component\FileSecurity\FileSecurity::writeFile() (line 158 of /home/website/domains/website.nl/web/core/lib/Drupal/Component/FileSecurity/FileSecurity.php)

I think silencing using @ would be an appropriate solution; Silencing file_put_contents is also done in other places (See MTimeProtectedFastFileStorage and FileStorage for example. Also, file_put_contents returns false when it fails, so we don't really need an extra php-warning I think.

Patch #8 solved the problem for me, altrough patch #10 also applies for correctness of the path to the .htaccess file.

@ruuds thanks for testing #10. Interesting that we've got different types of fails leading to the same error. I wonder if taking an exclusive lock works here...

@alexpott your patch of #15 also seems to solve the issue for me. I will monitor this the coming days.

@Ruuds it looks like the patch in #15 is a non-starter :( - LOCK_EX is not supported by vfs - which we use in kernel tests etc... and I'm not sure it is supported by the public / private stream wrappers either. Ho hum.

Here's an alternative approach that also uses an exclusive lock to create the file. It also needs silencing but I feel this reduces calls and is the most optimal solution in performance and multi-process sensitive code.

Note we also have another version of this to keep up-to-date.

Well that's not right. I'm not coping at all with $force :(

Hmmm okay - let's go back to silencing the @file_put_contents().... but once we do that we can remove the directory checks. Since having them introduces a bigger window where concurrent requests can end up with this problem.

Changes look good to me. RTBC +1

Thanks for your effort @alexpott. The patch works great for me.

1. +++ b/composer/Plugin/VendorHardening/FileSecurity.php
   @@ -28,7 +28,7 @@ class FileSecurity {
       */
      public static function writeHtaccess($directory, $deny_public_access = TRUE, $force = FALSE) {
   -    return self::writeFile($directory, '/.htaccess', self::htaccessLines($deny_public_access), $force);
   +    return self::writeFile($directory, '.htaccess', self::htaccessLines($deny_public_access), $force);
      }
    
   

   Why are these changes necessary?

2. +++ b/composer/Plugin/VendorHardening/FileSecurity.php
   @@ -154,7 +154,9 @@ protected static function writeFile($directory, $filename, $contents, $force) {
   -    if (file_exists($directory) && is_writable($directory) && file_put_contents($file_path, $contents)) {
   +    // Try to write the file. This can fail if concurrent requests are both
   +    // trying to write a the same time.
   +    if (@file_put_contents($file_path, $contents)) {
          return @chmod($file_path, 0444);
   

   Is it worth adding some reasoning for why we don't bother checking if the file exists etc. here - just in case someone in a couple of years tries to add those back?

1. This is made because FileSecurity.php is duplicated from core. See the comment at the top of https://github.com/drupal/core-vendor-hardening/blob/8.8.x/FileSecurity.php
2. Maybe we can also reference this issue in the comment?

Re #24.1 and .2

Here's the whole method with the patch applied

  protected static function writeFile($directory, $filename, $contents, $force) {
    $file_path = $directory . DIRECTORY_SEPARATOR . $filename;
    // Don't overwrite if the file exists unless forced.
    if (file_exists($file_path) && !$force) {
      return TRUE;
    }
    // Try to write the file. This can fail if concurrent requests are both
    // trying to write a the same time.
    if (@file_put_contents($file_path, $contents)) {
      return @chmod($file_path, 0444);
    }
    return FALSE;
  }

Re #24.1 So we don't need to add the the / in the writeHtAccesss and writeWebConfig because we're adding it in writeFile already. In fact I found when the error was happening for me this was the cause. I suspect because the system has to do more work to resolve path/to//.htaccess than path/to/.htaccess

Re #24.2 I think the new comment covers this. We're already checking for file existence. The checks we're removing are about directory existing but now we're silencing errors from @file_put_contents checking the directory existence is pointless and unnecessary. Not sure that adding a comment to that purpose really adds to this. We added the file_exists($directory) && is_writable($directory) checks in #2176141: Add a return value to file_save_htaccess() with no real explanation too :( (my fault).

I think it'd be good to get @catch's signoff here specifically, based on the previous discussion.

The patch also seems to not work on 9.1.x; should we provide a D9 version?

The patch also seems to not work on 9.1.x; should we provide a D9 version?

@xjm Do you mean the patch doesn't apply?

It seemed to apply ok for me. I re-rolled #20 just to make sure.

Updating the issue version. This should be 9.1.x

OK so for #24.1 it's not so much that the change is necessary but that we're removing something unnecessary (which may be the cause of the bug in some cases).

#24.2 yeah I'm mostly concerned if someone tries to 'optimize' it at some point and re-introduces the bug, but that's probably too much history for a comment.

I think @catch's reply to 24.2 makes sense. As this is really hard ( problem is not consistently reproducable) to write a test for it should be marked in some way.

What else needs to be done before it can be committed?

I'm running the patch for two months now without any strange new issues.

Maybe change the comment to something like this: "Try to write the file. This can fail if concurrent requests are both trying to write a the same time. It wil also fail when $directory doesn’t exist or isn’t writable. We won’t explicitly test for these conditions because it introduces two extra windows wherein concurrency issues can occur."

I think my written English is not good enough to determine if this 'marks' the whole issue...

Improved the comment along the lines of #36. I guess the extra docs can't hurt. It's a bit in the weeds but if it prevents us making the same mistake in the future that's great.

Leaving at rtbc since this is only a comment change.

Committed/pushed to 9.1.x and cherry-picked back to 8.9.x, thanks!