Contrib.social

Check for existing subdirectory returns false positive under specific conditions

Open on Drupal.org →
Created on 13 April 2023, over 1 year ago
Updated 29 March 2024, 8 months ago

Problem/Motivation

When adding a file, the Drupal file system first checks to see if a file exists, and if not then it will check if the specified directory exists. If not, it will be recursively created.

There is a bug in the check for subdirectories. If creating a file that is nested more than two directories deep, the variable for the first depth of directory will not exist and will end up checking to see if the final subdirectory exists. If that final subdirectory matches an already existing folder, the function will incorrectly return a true result for that folder existing.

Reproduced on local using Docksal as well as on a new Pantheon site.

Docksal:
Apache 2.4.53 (Unix)
MariaDB 10.6.5
PHP 8.1.14
Drupal version 9.5.7

Pantheon:
NGINX 1.21.6
PHP 8.1
Drupal version 9.5.7
MariaDB 10.4

Steps to reproduce

  1. Create new Drupal 9 project (tested with Docksal default)
  2. Install basic site `drush site:install -y`
  3. Set the write location in /web/sites/default/settings.php
    $settings['file_public_path'] = '/custom-dir';
    Where "custom-dir" is any root directory
  4. Create the new file write location
    `sudo mkdir /custom-dir`

The bug can now be reproduced with the following:

$ drush php:eval 'var_dump(is_dir("public://foo"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://foo/bar"));'
bool(false)
$ drush php:eval 'var_dump(is_dir("public://foo/custom-dir"));'
bool(true)

Proposed resolution

If the directory path does not exist, return FALSE instead of continuing checks.

diff --git a/core/lib/Drupal/Core/StreamWrapper/LocalStream.php b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
index b443096684..993a223021 100644
--- a/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
+++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
@@ -132,6 +132,10 @@ protected function getLocalPath($uri = NULL) {
     $realpath = realpath($path);
     if (!$realpath) {
       // This file does not yet exist.
+      if (realpath(dirname($path)) === FALSE) {
+        // Invalid or missing directory
+        return FALSE;
+      }
       $realpath = realpath(dirname($path)) . '/' . \Drupal::service('file_system')->basename($path);
     }
     $directory = realpath($this->getDirectoryPath());

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Fix an edge case bug in verifying LocalStream file directories

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
File system  →

Last updated about 12 hours ago

Created by

🇺🇸United States Jandor64

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Status changed to Needs review over 1 year ago
  • Comment over 1 year ago →
    cilefen
  • Status changed to Needs work over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States smustgrave

    Thank you for reporting and the detailed issue summary

    Can you provide a test case showing the issue too

    Thanks.

  • Comment over 1 year ago →
    🇺🇸United States Jandor64
  • Comment over 1 year ago →
    🇺🇸United States Jandor64

    Hello @smustgrave I have attempted to create a test for this, however it depends on the write folder being at the top level of the webserver and the standard docker image for creating tests doesn't allow modifying that location.

  • Comment 10 months ago →
    🇳🇿New Zealand claw

    I've independently just discovered this, and can confirm it also occurs in Drupal 10 (10.2.1)

    I also came across this on Pantheon - is_dir('public://x/x/files') is always TRUE.

    For me, this caused problems with generating image style derivative images - FileSystem::prepareDirectory() would not create the directory (because it appears to exist), and image generation would fail with the following message:

    TypeError: imagejpeg(): Argument #2 ($file) must be a file name or a stream resource, bool given in imagejpeg() (line 258 of core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php).

    The patch works for me for Drupal 9.5.

  • Comment 8 months ago →
    🇺🇸United States dandaman
  • Comment 8 months ago →
    🇺🇸United States dandaman

    Last week during a Drupal Camp’s contribution day, I tried to learn how to write tests to try to get this to move forward. But I definitely had some issues getting the testing suite running locally. Maybe I’ll try again at some point when I have more time.

  • Comment 8 months ago →
    🇺🇸United States smustgrave

    Based on your setup there’s some good documentation examples out there. Example I use ddev and phpstorm so this helped me

    https://www.previousnext.com.au/blog/running-and-debugging-phpunit-tests...

    Also if on Drupal community slack there is a channel #testing where someone may be of some help.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024