Contrib.social

PrivateFiles check may fail or give misleading results for alternate stream wrappers

Open on Drupal.org →
Created on 13 September 2023, over 1 year ago
Updated 12 July 2024, 5 months ago

Problem/Motivation

Problem 1: \Drupal\security_review\Checks\PrivateFiles::run() gets the private files directory path by statically calling a method on core's private stream wrapper class:

$file_directory_path = PrivateStream::basePath();

This may yield a false or misleading check result on sites where the private stream wrapper service has been swapped out for another implementation (for example, when using S3).

Problem 2: The check then attempts to get the realpath of the directory to see if it is within the Drupal root:

if (empty($file_directory_path)) {
  // Private files feature is not enabled.
  $result = CheckResult::SUCCESS;
  $visible = FALSE;
}
elseif (
  // Make a relative path from the Drupal root to the private files path; if
  // the relative path doesn't start with '../', it's most likely contained
  // in the Drupal root.
  \strpos($filesystem->makePathRelative(
    \realpath($file_directory_path),
    \DRUPAL_ROOT
  ), '../') !== 0 &&
   ...

If $settings['file_private_path'] was set in settings.php, but the directory itself does not exist, this throws an exception because the \realpath() call returns FALSE:
Symfony\Component\Filesystem\Exception\InvalidArgumentException: The end path "" is not absolute. in Symfony\Component\Filesystem\Filesystem->makePathRelative() (line 452 of /var/www/html/vendor/symfony/filesystem/Filesystem.php). Drupal\security_review\Checks\PrivateFiles->run() (Line: 168)

This is also an issue when it comes to resolving Problem 1 because some stream wrappers do not support realpath (again, such as S3).

Steps to reproduce

To reproduce problem 1, install the S3fs module, configure as normal and enable the option to "use S3 for private:// files". Then run the security report. The report results will be based on the local file system, not the overridden stream wrapper.

To reproduce problem 2, in settings.php set $settings['file_private_path'] to a path that does not exist, then run the security report. This throws the exception mentioned above.

Proposed resolution

Use the stream_wrapper.private service to get the realpath of the private files directory (if set), like so:

$file_directory_path = $this->container->has('stream_wrapper.private')
      ? $this->container->get('stream_wrapper.private')->realpath()
      : NULL;

and replace the later \realpath($file_directory_path) calls with $file_directory_path, since it now contains the realpath.

This uses the correct stream wrapper, whether it's the built-in one or overridden. The service's realpath() method also returns FALSE if the path does not exist—or, in the case of S3, if the stream wrapper does not support realpath—so the if (empty($file_directory_path)) statement will abort the check, thus avoiding the exception.

🐛 Bug report
Status

Fixed

Version

3.0

Component

Code

Created by

🇺🇸United States muriqui

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024