Issue #3296302 by cmlara, Omlidakt: resolvePath() does not peroperly handle...

Issue #3296302 by cmlara, Omlidakt: resolvePath() does not peroperly handle "s3://." + missing tests

src/S3fsFileService.php

@@ -12,6 +12,7 @@ use Drupal\Core\File\Exception\FileWriteException;
 use Drupal\Core\File\FileSystemInterface;
 use Drupal\Core\Site\Settings;
 use Drupal\Core\StreamWrapper\StreamWrapperManagerInterface;
+use Drupal\s3fs\Traits\S3fsPathsTrait;
 use Psr\Log\LoggerInterface;
 
 /**
@@ -29,6 +30,8 @@ use Psr\Log\LoggerInterface;
  */
 class S3fsFileService implements FileSystemInterface {
 
+  use S3fsPathsTrait;
+
   /**
    * The inner service.
    *
@@ -640,31 +643,4 @@ class S3fsFileService implements FileSystemInterface {
     return TRUE;
   }
 
-  /**
-   * Helper method to resolve a path to its non-relative location.
-   *
-   * Based on vfsStreamWrapper::resolvePath().
-   *
-   * @param string $path
-   *   The path to resolve.
-   *
-   * @return string
-   *   The resolved path.
-   */
-  protected function resolvePath(string $path): string {
-    $newPath = [];
-    foreach (explode('/', $path) as $pathPart) {
-      if ('.' !== $pathPart) {
-        if ('..' !== $pathPart) {
-          $newPath[] = $pathPart;
-        }
-        elseif (count($newPath) > 1) {
-          array_pop($newPath);
-        }
-      }
-    }
-
-    return implode('/', $newPath);
-  }
-
 }

src/StreamWrapper/S3fsStream.php

@@ -14,6 +14,7 @@ use Drupal\Core\StringTranslation\StringTranslationTrait;
 use Drupal\Core\Url;
 use Drupal\s3fs\S3fsException;
 use Drupal\s3fs\S3fsServiceInterface;
+use Drupal\s3fs\Traits\S3fsPathsTrait;
 use Psr\Http\Message\RequestInterface;
 
 /**
@@ -25,6 +26,7 @@ use Psr\Http\Message\RequestInterface;
 class S3fsStream extends StreamWrapper implements StreamWrapperInterface {
 
   use StringTranslationTrait;
+  use S3fsPathsTrait;
 
   const API_VERSION = '2006-03-01';
 
@@ -979,10 +981,15 @@ class S3fsStream extends StreamWrapper implements StreamWrapperInterface {
    * @see http://php.net/manual/en/streamwrapper.mkdir.php
    */
   public function mkdir($uri, $mode, $options) {
+    // Resolve relative path and strip any trailing slash that we mustn't
+    // store in the cache.
     $uri = $this->resolvePath($uri);
-    // Some Drupal plugins call mkdir with a trailing slash. We mustn't store
-    // that slash in the cache.
-    $uri = rtrim($uri, '/');
+
+    if (StreamWrapperManager::getTarget($uri) == '') {
+      // Don't store the root path in the database.
+      // Always consider this successful.
+      return TRUE;
+    }
 
     if (mb_strlen($uri) > S3fsServiceInterface::MAX_URI_LENGTH) {
       return FALSE;
@@ -1037,13 +1044,13 @@ class S3fsStream extends StreamWrapper implements StreamWrapperInterface {
       return FALSE;
     }
 
-    // We need a version of $uri with no / because folders are cached with no /.
+    // We need a version of $uri with no / because folders are cached with no /,
+    // this is provided by resolvePath() above.
     // We also need one with the /, because it might be a file in S3 that
     // ends with /. In addition, we must differentiate against files with this
     // folder's name as a substring.
-    // e.g. rmdir('s3://foo/bar') should ignore s3://foo/barbell.jpg.
-    $base_path = rtrim($uri, '/');
-    $slash_path = $base_path . '/';
+    // e.g. rmdir('s3://foo/bar') should ignore s3://foo/barbell.jpg.;
+    $slash_path = $uri . '/';
 
     // Check if the folder is empty.
     $query = \Drupal::database()->select('s3fs_file', 's');
@@ -1115,8 +1122,7 @@ class S3fsStream extends StreamWrapper implements StreamWrapperInterface {
     }
 
     $scheme = StreamWrapperManager::getScheme($uri);
-    $base_path = rtrim($uri, '/');
-    $slash_path = $base_path . '/';
+    $slash_path = $uri . '/';
 
     // If this path was originally a root folder (e.g. s3://), the above code
     // removed *both* slashes but only added one back. So we need to add
@@ -1606,31 +1612,4 @@ class S3fsStream extends StreamWrapper implements StreamWrapperInterface {
     return $params;
   }
 
-  /**
-   * Helper method to resolve a path to its non-relative location.
-   *
-   * Based on vfsStreamWrapper::resolvePath().
-   *
-   * @param string $path
-   *   The path to resolve.
-   *
-   * @return string
-   *   The resolved path.
-   */
-  protected function resolvePath(string $path): string {
-    $newPath = [];
-    foreach (explode('/', $path) as $pathPart) {
-      if ('.' !== $pathPart) {
-        if ('..' !== $pathPart) {
-          $newPath[] = $pathPart;
-        }
-        elseif (count($newPath) > 1) {
-          array_pop($newPath);
-        }
-      }
-    }
-
-    return implode('/', $newPath);
-  }
-
 }

src/Traits/S3fsPathsTrait.php

+<?php
+
+namespace Drupal\s3fs\Traits;
+
+use Drupal\Core\StreamWrapper\StreamWrapperManager;
+
+/**
+ * S3fs path helper functions.
+ *
+ * @ingroup s3fs
+ */
+trait S3fsPathsTrait {
+
+  /**
+   * Resolve a (possibly) relative path to its non-relative form.
+   *
+   * Based on vfsStreamWrapper::resolvePath().
+   *
+   * @param string $path
+   *   The path to resolve.
+   *
+   * @return string
+   *   The resolved path.
+   */
+  protected function resolvePath(string $path): string {
+    $scheme = StreamWrapperManager::getScheme($path);
+    $target = StreamWrapperManager::getTarget($path);
+    $new_path = [];
+    foreach (explode('/', $target) as $target_part) {
+      if ('.' !== $target_part) {
+        if ('..' !== $target_part) {
+          $new_path[] = $target_part;
+        }
+        elseif (count($new_path) > 0) {
+          array_pop($new_path);
+        }
+      }
+    }
+
+    $imploded_path = implode('/', $new_path);
+    return $scheme . '://' . $imploded_path;
+  }
+
+}

tests/src/Unit/S3fsPathsTraitTest.php

+<?php
+
+namespace Drupal\Tests\s3fs\Unit;
+
+use Drupal\s3fs\Traits\S3fsPathsTrait;
+use Drupal\Tests\UnitTestCase;
+
+require_once __DIR__ . '/../../fixtures/S3fsCssOptimizerMock.php';
+
+/**
+ * Tests S3fsPathResolutionTrait.
+ *
+ * @group s3fs
+ */
+class S3fsPathsTraitTest extends UnitTestCase {
+
+  use S3fsPathsTrait;
+
+  /**
+   * Test relative path resolution.
+   *
+   * @dataProvider resolvePathDataProvider
+   */
+  public function testPathResolution(string $uri, string $expected): void {
+    $this->assertEquals($expected, $this->resolvePath($uri));
+  }
+
+  /**
+   * Data for testing relative path resolution.
+   *
+   * @return array
+   *   An array of test data.
+   */
+  public function resolvePathDataProvider(): array {
+    return [
+      'Fully Resolved file' => [
+        's3://folder1/folder2/test.txt',
+        's3://folder1/folder2/test.txt',
+      ],
+      'Fully Resolved directory' => [
+        's3://folder1/folder2/',
+        's3://folder1/folder2',
+      ],
+      'Scheme root' => [
+        's3://',
+        's3://',
+      ],
+      'Root with single dot ' => [
+        's3://.',
+        's3://',
+      ],
+      'Root with double dot' => [
+        's3://..',
+        's3://',
+      ],
+      'Path contains a double dot folder ' => [
+        's3://folder1/../folder1/folder2/test1.txt',
+        's3://folder1/folder2/test1.txt',
+      ],
+      'Path starts with a double dot folder ' => [
+        's3://../folder1/folder2/test1.txt',
+        's3://folder1/folder2/test1.txt',
+      ],
+      'Path contains a single dot folder ' => [
+        's3://folder1/./folder2/test1.txt',
+        's3://folder1/folder2/test1.txt',
+      ],
+      'Path starts with a single dot folder ' => [
+        's3://./folder1/folder2/test1.txt',
+        's3://folder1/folder2/test1.txt',
+      ],
+
+    ];
+  }
+
+}