From 67c684714dc064cb8e27d8f06147ac313effe8e3 Mon Sep 17 00:00:00 2001 From: Kim Pepper <kim@pepper.id.au> Date: Fri, 28 Mar 2025 11:42:28 +1100 Subject: [PATCH 1/3] [#944582] Also check for files directory executable permission --- core/includes/install.core.inc | 4 +- core/includes/install.inc | 21 ++++---- .../Component/FileSystem/FileSystem.php | 2 +- core/lib/Drupal/Core/File/FileSystem.php | 48 ++++++++++++++++++- .../Drupal/Core/File/FileSystemInterface.php | 34 +++++++++++++ core/lib/Drupal/Core/Updater/Updater.php | 4 +- .../config/src/Form/ConfigImportForm.php | 2 +- core/modules/media/media.install | 2 +- .../src/Plugin/migrate/process/FileCopy.php | 2 +- .../system/src/Form/PerformanceForm.php | 2 +- core/modules/system/system.install | 2 +- core/modules/system/system.module | 2 +- 12 files changed, 104 insertions(+), 21 deletions(-) diff --git a/core/includes/install.core.inc b/core/includes/install.core.inc index 7d1b4da8a986..d532351b81ff 100644 --- a/core/includes/install.core.inc +++ b/core/includes/install.core.inc @@ -1972,7 +1972,7 @@ function install_check_translations($langcode, $server_pattern): array { // Get values so the requirements errors can be specific. if (drupal_verify_install_file($translations_directory, FILE_EXIST, 'dir')) { $readable = is_readable($translations_directory); - $writable = is_writable($translations_directory); + $writable = $file_system->isWritable($translations_directory); $translations_directory_exists = TRUE; } @@ -2162,7 +2162,7 @@ function install_check_requirements($install_state) { // Otherwise, if $file does not exist yet, we can try to copy // $default_file to create it. elseif (!$exists) { - $copied = drupal_verify_install_file($site_path, FILE_EXIST | FILE_WRITABLE, 'dir') && @copy($default_file, $file); + $copied = drupal_verify_install_file($site_path, FILE_EXIST | FILE_WRITABLE | FILE_EXECUTABLE, 'dir') && @copy($default_file, $file); if ($copied) { // If the new $file file has the same owner as $default_file this means // $default_file is owned by the webserver user. This is an inherent diff --git a/core/includes/install.inc b/core/includes/install.inc index d1e7e8656661..5050b82014ab 100644 --- a/core/includes/install.inc +++ b/core/includes/install.inc @@ -309,6 +309,8 @@ function drupal_verify_install_file($file, $mask = NULL, $type = 'file', $auto_f } } + /** @var \Drupal\Core\File\FileSystemInterface $file_system */ + $file_system = \Drupal::service('file_system'); // Verify file permissions. if (isset($mask)) { $masks = [FILE_EXIST, FILE_READABLE, FILE_WRITABLE, FILE_EXECUTABLE, FILE_NOT_READABLE, FILE_NOT_WRITABLE, FILE_NOT_EXECUTABLE]; @@ -333,13 +335,13 @@ function drupal_verify_install_file($file, $mask = NULL, $type = 'file', $auto_f break; case FILE_WRITABLE: - if (!is_writable($file)) { + if (!$file_system->isWritable($file)) { $return = FALSE; } break; case FILE_EXECUTABLE: - if (!is_executable($file)) { + if (!$file_system->isExecutable($file)) { $return = FALSE; } break; @@ -351,13 +353,13 @@ function drupal_verify_install_file($file, $mask = NULL, $type = 'file', $auto_f break; case FILE_NOT_WRITABLE: - if (is_writable($file)) { + if ($file_system->isWritable($file)) { $return = FALSE; } break; case FILE_NOT_EXECUTABLE: - if (is_executable($file)) { + if ($file_system->isExecutable($file)) { $return = FALSE; } break; @@ -440,7 +442,8 @@ function drupal_install_fix_file($file, $mask, $message = TRUE) { if (!file_exists($file)) { return FALSE; } - + /** @var \Drupal\Core\File\FileSystemInterface $file_system */ + $file_system = \Drupal::service('file_system'); $mod = fileperms($file) & 0777; $masks = [FILE_READABLE, FILE_WRITABLE, FILE_EXECUTABLE, FILE_NOT_READABLE, FILE_NOT_WRITABLE, FILE_NOT_EXECUTABLE]; @@ -458,13 +461,13 @@ function drupal_install_fix_file($file, $mask, $message = TRUE) { break; case FILE_WRITABLE: - if (!is_writable($file)) { + if (!$file_system->isWritable($file)) { $mod |= 0222; } break; case FILE_EXECUTABLE: - if (!is_executable($file)) { + if (!$file_system->isExecutable($file)) { $mod |= 0111; } break; @@ -476,13 +479,13 @@ function drupal_install_fix_file($file, $mask, $message = TRUE) { break; case FILE_NOT_WRITABLE: - if (is_writable($file)) { + if ($file_system->isWritable($file)) { $mod &= ~0222; } break; case FILE_NOT_EXECUTABLE: - if (is_executable($file)) { + if ($file_system->isExecutable($file)) { $mod &= ~0111; } break; diff --git a/core/lib/Drupal/Component/FileSystem/FileSystem.php b/core/lib/Drupal/Component/FileSystem/FileSystem.php index b4e8389d755a..85305132d877 100644 --- a/core/lib/Drupal/Component/FileSystem/FileSystem.php +++ b/core/lib/Drupal/Component/FileSystem/FileSystem.php @@ -36,7 +36,7 @@ public static function getOsTemporaryDirectory() { $directories[] = sys_get_temp_dir(); foreach ($directories as $directory) { - if (is_dir($directory) && is_writable($directory)) { + if (is_dir($directory) && is_writable($directory) && is_executable($directory)) { // Both sys_get_temp_dir() and ini_get('upload_tmp_dir') can return // paths with a trailing directory separator. return rtrim($directory, DIRECTORY_SEPARATOR); diff --git a/core/lib/Drupal/Core/File/FileSystem.php b/core/lib/Drupal/Core/File/FileSystem.php index 8920c3daf1e5..7d8f05a68e9e 100644 --- a/core/lib/Drupal/Core/File/FileSystem.php +++ b/core/lib/Drupal/Core/File/FileSystem.php @@ -531,7 +531,7 @@ public function prepareDirectory(&$directory, $options = self::MODIFY_PERMISSION } } - $writable = is_writable($directory); + $writable = $this->isWritable($directory); if (!$writable && ($options & static::MODIFY_PERMISSIONS)) { return $this->chmod($directory); } @@ -734,4 +734,50 @@ protected function doScanDirectory($dir, $mask, array $options = [], $depth = 0) return array_merge(array_merge(...$files_in_sub_dirs), $files_in_this_directory); } + /** + * Determines if a directory is writable by the web server. + * + * PHP's is_writable() does not fully support stream wrappers, so this + * function fills that gap. + * In order to be able to write files within the directory, the directory + * itself must be writable, and it must also have the executable bit set. This + * helper function checks both at the same time. + * + * @param string $uri + * A URI or pathname pointing to the directory that will be checked. + * + * @return bool + * TRUE if the directory is writable and executable; FALSE otherwise. + */ + public function isWritable($uri) { + // By converting the URI to a normal path using drupal_realpath(), we can + // correctly handle both stream wrappers and normal paths. + $realpath = $this->realpath($uri); + return is_writable($realpath ?: $uri) && $this->isExecutable($uri); + } + + /** + * Determines if a file or directory is executable. + * + * PHP's is_executable() does not fully support stream wrappers, so this + * function fills that gap. + * + * @param string $uri + * A URI or pathname pointing to the file or directory that will be checked. + * + * @return bool + * TRUE if the file or directory is executable; FALSE otherwise. + * + * @see is_executable() + * @ingroup php_wrappers + */ + public function isExecutable($uri) { + // By converting the URI to a normal path using drupal_realpath(), we can + // correctly handle both stream wrappers and normal paths. + $realpath = $this->realpath($uri); + $filename = $realpath ?: $uri; + // Determine whether the URI is an executable file or a directory. + return is_executable($filename) || is_dir($filename); + } + } diff --git a/core/lib/Drupal/Core/File/FileSystemInterface.php b/core/lib/Drupal/Core/File/FileSystemInterface.php index f3beabfe2209..8f3967d0a2b0 100644 --- a/core/lib/Drupal/Core/File/FileSystemInterface.php +++ b/core/lib/Drupal/Core/File/FileSystemInterface.php @@ -518,4 +518,38 @@ public function getTempDirectory(); */ public function scanDirectory($dir, $mask, array $options = []); + /** + * Determines if a directory is writable by the web server. + * + * PHP's is_writable() does not fully support stream wrappers, so this + * function fills that gap. + * In order to be able to write files within the directory, the directory + * itself must be writable, and it must also have the executable bit set. This + * helper function checks both at the same time. + * + * @param string $uri + * A URI or pathname pointing to the directory that will be checked. + * + * @return bool + * TRUE if the directory is writable and executable; FALSE otherwise. + */ + public function isWritable($uri); + + /** + * Determines if a file or directory is executable. + * + * PHP's is_executable() does not fully support stream wrappers, so this + * function fills that gap. + * + * @param string $uri + * A URI or pathname pointing to the file or directory that will be checked. + * + * @return bool + * TRUE if the file or directory is executable; FALSE otherwise. + * + * @see is_executable() + * @ingroup php_wrappers + */ + public function isExecutable($uri); + } diff --git a/core/lib/Drupal/Core/Updater/Updater.php b/core/lib/Drupal/Core/Updater/Updater.php index 908fdf7616a0..d0213ec7358b 100644 --- a/core/lib/Drupal/Core/Updater/Updater.php +++ b/core/lib/Drupal/Core/Updater/Updater.php @@ -339,7 +339,7 @@ public function prepareInstallDirectory(&$filetransfer, $directory) { // Make the parent dir writable if need be and create the dir. if (!is_dir($directory)) { $parent_dir = dirname($directory); - if (!is_writable($parent_dir)) { + if (!\Drupal::service('file_system')->isWritable($parent_dir)) { @chmod($parent_dir, 0755); // It is expected that this will fail if the directory is owned by the // FTP user. If the FTP user == web server, it will succeed. @@ -385,7 +385,7 @@ public function prepareInstallDirectory(&$filetransfer, $directory) { * If the chmod should be applied recursively. */ public function makeWorldReadable(&$filetransfer, $path, $recursive = TRUE) { - if (!is_executable($path)) { + if (!\Drupal::service('file_system')->isExecutable($path)) { // Set it to read + execute. $new_perms = fileperms($path) & 0777 | 0005; $filetransfer->chmod($path, $new_perms, $recursive); diff --git a/core/modules/config/src/Form/ConfigImportForm.php b/core/modules/config/src/Form/ConfigImportForm.php index 74c73b8d999a..bcd47d4e0ed8 100644 --- a/core/modules/config/src/Form/ConfigImportForm.php +++ b/core/modules/config/src/Form/ConfigImportForm.php @@ -77,7 +77,7 @@ public function getFormId() { */ public function buildForm(array $form, FormStateInterface $form_state) { $directory = $this->settings->get('config_sync_directory'); - $directory_is_writable = is_writable($directory); + $directory_is_writable = \Drupal::service('file_system')->isWritable($directory); if (!$directory_is_writable) { $this->messenger()->addError($this->t('The directory %directory is not writable.', ['%directory' => $directory])); } diff --git a/core/modules/media/media.install b/core/modules/media/media.install index f0acbf482da4..f3514d1128c5 100644 --- a/core/modules/media/media.install +++ b/core/modules/media/media.install @@ -23,7 +23,7 @@ function media_requirements($phase): array { if ($phase == 'install') { $destination = 'public://media-icons/generic'; \Drupal::service('file_system')->prepareDirectory($destination, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS); - $is_writable = is_writable($destination); + $is_writable = \Drupal::service('file_system')->isWritable($destination); $is_directory = is_dir($destination); if (!$is_writable || !$is_directory) { if (!$is_directory) { diff --git a/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php index 61d0ace73d0a..030fa184b62c 100644 --- a/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php +++ b/core/modules/migrate/src/Plugin/migrate/process/FileCopy.php @@ -145,7 +145,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable // If the directory exists and is writable, avoid // \Drupal\Core\File\FileSystemInterface::prepareDirectory() call and write // the file to destination. - if (!is_dir($dir) || !is_writable($dir)) { + if (!is_dir($dir) || !$this->fileSystem->isWritable($dir)) { if (!$this->fileSystem->prepareDirectory($dir, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS)) { throw new MigrateException("Could not create or write to directory '$dir'"); } diff --git a/core/modules/system/src/Form/PerformanceForm.php b/core/modules/system/src/Form/PerformanceForm.php index 3a1cefb471bf..1567bc5a0a12 100644 --- a/core/modules/system/src/Form/PerformanceForm.php +++ b/core/modules/system/src/Form/PerformanceForm.php @@ -129,7 +129,7 @@ public function buildForm(array $form, FormStateInterface $form_state) { ]; $directory = 'assets://'; - $is_writable = is_dir($directory) && is_writable($directory); + $is_writable = is_dir($directory) && \Drupal::service('file_system')->isWritable($directory); $disabled = !$is_writable; $disabled_message = ''; if (!$is_writable) { diff --git a/core/modules/system/system.install b/core/modules/system/system.install index 8da736101f1c..3f0e5fe23159 100644 --- a/core/modules/system/system.install +++ b/core/modules/system/system.install @@ -859,7 +859,7 @@ function system_requirements($phase): array { if ($phase == 'install') { \Drupal::service('file_system')->prepareDirectory($directory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS); } - $is_writable = is_writable($directory); + $is_writable = \Drupal::service('file_system')->isWritable($directory); $is_directory = is_dir($directory); if (!$is_writable || !$is_directory) { $description = ''; diff --git a/core/modules/system/system.module b/core/modules/system/system.module index c7ed1808206f..cfba75fbd106 100644 --- a/core/modules/system/system.module +++ b/core/modules/system/system.module @@ -356,7 +356,7 @@ function system_check_directory($form_element, FormStateInterface $form_state) { $logger->error('The directory %directory does not exist and could not be created.', ['%directory' => $directory]); } - if (is_dir($directory) && !is_writable($directory) && !$file_system->chmod($directory)) { + if (is_dir($directory) && !$file_system->isWritable($directory) && !$file_system->chmod($directory)) { // If the directory is not writable and cannot be made so. $form_state->setErrorByName($form_element['#parents'][0], t('The directory %directory exists but is not writable and could not be made writable.', ['%directory' => $directory])); $logger->error('The directory %directory exists but is not writable and could not be made writable.', ['%directory' => $directory]); -- GitLab From 140ac62a4f2ec6e7f2a45b00888bb1d63161a4df Mon Sep 17 00:00:00 2001 From: Mohit Aghera <mohit.beta@gmail.com> Date: Tue, 17 Jun 2025 18:21:28 +0530 Subject: [PATCH 2/3] MR feedback fixes. --- core/lib/Drupal/Core/File/FileSystem.php | 8 ++++++-- core/lib/Drupal/Core/File/FileSystemInterface.php | 4 ++-- core/modules/config/src/Form/ConfigImportForm.php | 2 +- core/modules/system/src/Form/PerformanceForm.php | 13 +++++++++++-- core/modules/system/system.install | 11 +++++++---- 5 files changed, 27 insertions(+), 11 deletions(-) diff --git a/core/lib/Drupal/Core/File/FileSystem.php b/core/lib/Drupal/Core/File/FileSystem.php index 7d8f05a68e9e..b169d89d922c 100644 --- a/core/lib/Drupal/Core/File/FileSystem.php +++ b/core/lib/Drupal/Core/File/FileSystem.php @@ -749,10 +749,14 @@ protected function doScanDirectory($dir, $mask, array $options = [], $depth = 0) * @return bool * TRUE if the directory is writable and executable; FALSE otherwise. */ - public function isWritable($uri) { + public function isWritable($uri): bool { // By converting the URI to a normal path using drupal_realpath(), we can // correctly handle both stream wrappers and normal paths. $realpath = $this->realpath($uri); + $filename = $realpath ?: $uri; + if (\is_dir($filename) !== TRUE) { + return FALSE; + } return is_writable($realpath ?: $uri) && $this->isExecutable($uri); } @@ -771,7 +775,7 @@ public function isWritable($uri) { * @see is_executable() * @ingroup php_wrappers */ - public function isExecutable($uri) { + public function isExecutable($uri): bool { // By converting the URI to a normal path using drupal_realpath(), we can // correctly handle both stream wrappers and normal paths. $realpath = $this->realpath($uri); diff --git a/core/lib/Drupal/Core/File/FileSystemInterface.php b/core/lib/Drupal/Core/File/FileSystemInterface.php index 8f3967d0a2b0..a669cf88704e 100644 --- a/core/lib/Drupal/Core/File/FileSystemInterface.php +++ b/core/lib/Drupal/Core/File/FileSystemInterface.php @@ -533,7 +533,7 @@ public function scanDirectory($dir, $mask, array $options = []); * @return bool * TRUE if the directory is writable and executable; FALSE otherwise. */ - public function isWritable($uri); + public function isWritable($uri): bool; /** * Determines if a file or directory is executable. @@ -550,6 +550,6 @@ public function isWritable($uri); * @see is_executable() * @ingroup php_wrappers */ - public function isExecutable($uri); + public function isExecutable($uri): bool; } diff --git a/core/modules/config/src/Form/ConfigImportForm.php b/core/modules/config/src/Form/ConfigImportForm.php index bcd47d4e0ed8..b1448471f309 100644 --- a/core/modules/config/src/Form/ConfigImportForm.php +++ b/core/modules/config/src/Form/ConfigImportForm.php @@ -77,7 +77,7 @@ public function getFormId() { */ public function buildForm(array $form, FormStateInterface $form_state) { $directory = $this->settings->get('config_sync_directory'); - $directory_is_writable = \Drupal::service('file_system')->isWritable($directory); + $directory_is_writable = $this->fileSystem->isWritable($directory); if (!$directory_is_writable) { $this->messenger()->addError($this->t('The directory %directory is not writable.', ['%directory' => $directory])); } diff --git a/core/modules/system/src/Form/PerformanceForm.php b/core/modules/system/src/Form/PerformanceForm.php index 1567bc5a0a12..94781948f426 100644 --- a/core/modules/system/src/Form/PerformanceForm.php +++ b/core/modules/system/src/Form/PerformanceForm.php @@ -5,6 +5,7 @@ use Drupal\Core\Asset\AssetCollectionOptimizerInterface; use Drupal\Core\Config\TypedConfigManagerInterface; use Drupal\Core\Extension\ModuleHandlerInterface; +use Drupal\Core\File\FileSystemInterface; use Drupal\Core\Form\ConfigFormBase; use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\Core\Datetime\DateFormatterInterface; @@ -62,14 +63,21 @@ class PerformanceForm extends ConfigFormBase { * The JavaScript asset collection optimizer service. * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler * The module handler. + * @param \Drupal\Core\File\FileSystemInterface|null $fileSystem + * The file system service. */ - public function __construct(ConfigFactoryInterface $config_factory, TypedConfigManagerInterface $typedConfigManager, DateFormatterInterface $date_formatter, AssetCollectionOptimizerInterface $css_collection_optimizer, AssetCollectionOptimizerInterface $js_collection_optimizer, ModuleHandlerInterface $module_handler) { + public function __construct(ConfigFactoryInterface $config_factory, TypedConfigManagerInterface $typedConfigManager, DateFormatterInterface $date_formatter, AssetCollectionOptimizerInterface $css_collection_optimizer, AssetCollectionOptimizerInterface $js_collection_optimizer, ModuleHandlerInterface $module_handler, protected ?FileSystemInterface $fileSystem = NULL) { parent::__construct($config_factory, $typedConfigManager); $this->dateFormatter = $date_formatter; $this->cssCollectionOptimizer = $css_collection_optimizer; $this->jsCollectionOptimizer = $js_collection_optimizer; $this->moduleHandler = $module_handler; + + if ($this->fileSystem === NULL) { + @trigger_error('Calling ' . __CLASS__ . ' constructor without the $fileSystem argument is deprecated in drupal:11.2.0 and it will be required in drupal:12.0.0. See https://www.drupal.org/project/drupal/issues/944582', E_USER_DEPRECATED); + $this->fileSystem = \Drupal::service('file_system'); + } } /** @@ -82,7 +90,8 @@ public static function create(ContainerInterface $container) { $container->get('date.formatter'), $container->get('asset.css.collection_optimizer'), $container->get('asset.js.collection_optimizer'), - $container->get('module_handler') + $container->get('module_handler'), + $container->get('file_system'), ); } diff --git a/core/modules/system/system.install b/core/modules/system/system.install index 692885bd4717..6fb685adedb8 100644 --- a/core/modules/system/system.install +++ b/core/modules/system/system.install @@ -78,6 +78,9 @@ function system_requirements($phase): array { $theme_extension_list->reset(); $requirements = []; + /** @var \Drupal\Core\File\FileSystemInterface $fileSystem */ + $fileSystem = \Drupal::service('file_system'); + // Report Drupal version if ($phase == 'runtime') { $requirements['drupal'] = [ @@ -783,7 +786,7 @@ function system_requirements($phase): array { // By default no private files directory is configured. For private files // to be secure the admin needs to provide a path outside the webroot. PrivateStream::basePath(), - \Drupal::service('file_system')->getTempDirectory(), + $fileSystem->getTempDirectory(), ]; } @@ -821,7 +824,7 @@ function system_requirements($phase): array { if (!empty($config_sync_directory)) { // If we're installing Drupal try and create the config sync directory. if (!is_dir($config_sync_directory) && $phase == 'install') { - \Drupal::service('file_system')->prepareDirectory($config_sync_directory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS); + $fileSystem->prepareDirectory($config_sync_directory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS); } if (!is_dir($config_sync_directory)) { if ($phase == 'install') { @@ -857,9 +860,9 @@ function system_requirements($phase): array { continue; } if ($phase == 'install') { - \Drupal::service('file_system')->prepareDirectory($directory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS); + $fileSystem->prepareDirectory($directory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS); } - $is_writable = \Drupal::service('file_system')->isWritable($directory); + $is_writable = $fileSystem->isWritable($directory); $is_directory = is_dir($directory); if (!$is_writable || !$is_directory) { $description = ''; -- GitLab From 62125fa8b536fbc092250b70d2eed97da9df5f7f Mon Sep 17 00:00:00 2001 From: Mohit Aghera <mohit.beta@gmail.com> Date: Wed, 18 Jun 2025 08:05:59 +0530 Subject: [PATCH 3/3] Self review fixes. --- core/modules/system/src/Form/PerformanceForm.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/modules/system/src/Form/PerformanceForm.php b/core/modules/system/src/Form/PerformanceForm.php index 94781948f426..3dd93eff710e 100644 --- a/core/modules/system/src/Form/PerformanceForm.php +++ b/core/modules/system/src/Form/PerformanceForm.php @@ -138,7 +138,7 @@ public function buildForm(array $form, FormStateInterface $form_state) { ]; $directory = 'assets://'; - $is_writable = is_dir($directory) && \Drupal::service('file_system')->isWritable($directory); + $is_writable = is_dir($directory) && $this->fileSystem->isWritable($directory); $disabled = !$is_writable; $disabled_message = ''; if (!$is_writable) { -- GitLab