Bugfix/ter 144 (#187)

* [BUGFIX] Add new extension configuration options in documentation [TER-144] [TER-154] * [BUGFIX] Do not ignore file delivery logic for secure downloads file storage [TER-144] [TER-156] * [TASK] Add tests for empty secured directories [TER-144] [TER-155] * [BUGFIX] Fix secured directories when configured empty [TER-144] [TER-156] * [BUGFIX] Use the member function [TER-144] [TER-156] * [TASK] Ensure compatibility with TYPO3 10 [TER-144] [TER-156]
Leuchtfeuer · Oct 27, 2023 · d3394f5 · d3394f5
1 parent 111a95f
commit d3394f5
Show file tree

Hide file tree

Showing 8 changed files with 150 additions and 1 deletion.
diff --git a/Classes/Domain/Transfer/ExtensionConfiguration.php b/Classes/Domain/Transfer/ExtensionConfiguration.php
@@ -248,6 +248,9 @@ public function getSecuredDirs(): string
 
     public function getSecuredDirectoriesPattern(): string
     {
+        if ($this->getSecuredDirs() === '') {
+            return '';
+        }
         return sprintf('#^(%s)#i', $this->getSecuredDirs());
     }
 

diff --git a/Classes/EventListener/SecureDownloadsEventListener.php b/Classes/EventListener/SecureDownloadsEventListener.php
@@ -18,6 +18,7 @@
 use Leuchtfeuer\SecureDownloads\Service\SecureDownloadService;
 use TYPO3\CMS\Core\Core\Environment;
 use TYPO3\CMS\Core\Imaging\Event\ModifyIconForResourcePropertiesEvent;
+use TYPO3\CMS\Core\Information\Typo3Version;
 use TYPO3\CMS\Core\Resource\Driver\AbstractHierarchicalFilesystemDriver;
 use TYPO3\CMS\Core\Resource\Event\GeneratePublicUrlForResourceEvent;
 use TYPO3\CMS\Core\Resource\Exception;
@@ -26,6 +27,7 @@
 use TYPO3\CMS\Core\Resource\Folder;
 use TYPO3\CMS\Core\Resource\ProcessedFile;
 use TYPO3\CMS\Core\SingletonInterface;
+use TYPO3\CMS\Core\Utility\GeneralUtility;
 use TYPO3\CMS\Core\Utility\PathUtility;
 
 /**
@@ -54,9 +56,12 @@ public function onResourceStorageEmitPreGeneratePublicUrlSignal(GeneratePublicUr
         $driver = $event->getDriver();
         $resource = $event->getResource();
 
-        if ($driver instanceof AbstractHierarchicalFilesystemDriver && ($resource instanceof File || $resource instanceof ProcessedFile) && $resource->getStorage()->isPublic()) {
+        if ($driver instanceof AbstractHierarchicalFilesystemDriver && ($resource instanceof File || $resource instanceof ProcessedFile) && ($resource->getStorage()->isPublic() || $driver instanceof SecureDownloadsDriver)) {
             try {
                 $originalPathShouldBeSecured = false;
+                if ($driver instanceof SecureDownloadsDriver) {
+                    $driver->determineSecureDownloadsDriverBaseUrl();
+                }
                 if ($resource instanceof ProcessedFile) {
                     $originalPublicUrl = $driver->getPublicUrl($resource->getOriginalFile()->getIdentifier());
                     $originalPathShouldBeSecured = $this->secureDownloadService->pathShouldBeSecured($originalPublicUrl);
@@ -66,6 +71,9 @@ public function onResourceStorageEmitPreGeneratePublicUrlSignal(GeneratePublicUr
                     $securedUrl = $this->getSecuredUrl($event->isRelativeToCurrentScript(), $publicUrl, $driver);
                     $event->setPublicUrl($securedUrl);
                 }
+                if ($driver instanceof SecureDownloadsDriver && GeneralUtility::makeInstance(Typo3Version::class)->getMajorVersion() === 11) {
+                    $event->setPublicUrl('/' . $event->getPublicUrl());
+                }
             } catch (Exception $exception) {
                 // Do nothing.
             }

diff --git a/Classes/Resource/Driver/SecureDownloadsDriver.php b/Classes/Resource/Driver/SecureDownloadsDriver.php
@@ -14,7 +14,9 @@
  *
  ***/
 
+use TYPO3\CMS\Core\Core\Environment;
 use TYPO3\CMS\Core\Resource\Driver\LocalDriver;
+use TYPO3\CMS\Core\Utility\PathUtility;
 
 /**
  * The Secure Downloads file storage.
@@ -26,4 +28,22 @@ class SecureDownloadsDriver extends LocalDriver
     const DRIVER_NAME = 'Secure Downloads';
 
     const BASE_PATH = 'sdl/';
+
+    public function determineSecureDownloadsDriverBaseUrl(): void
+    {
+        if ($this->baseUri === null) {
+            if (!empty($this->configuration['baseUri'])) {
+                $this->baseUri = rtrim($this->configuration['baseUri'], '/') . '/';
+            } elseif (str_starts_with($this->absoluteBasePath, Environment::getPublicPath())) {
+                // use site-relative URLs
+                $temporaryBaseUri = rtrim(PathUtility::stripPathSitePrefix($this->absoluteBasePath), '/');
+                if ($temporaryBaseUri !== '') {
+                    $uriParts = explode('/', $temporaryBaseUri);
+                    $uriParts = array_map('rawurlencode', $uriParts);
+                    $temporaryBaseUri = implode('/', $uriParts) . '/';
+                }
+                $this->baseUri = $temporaryBaseUri;
+            }
+        }
+    }
 }
diff --git a/Classes/Service/SecureDownloadService.php b/Classes/Service/SecureDownloadService.php
@@ -63,6 +63,10 @@ public function folderShouldBeSecured(string $publicUrl): bool
     {
         $pattern = $this->extensionConfiguration->getSecuredDirectoriesPattern();
 
+        if ($pattern === '') {
+            return false;
+        }
+
         $result = (bool)preg_match($pattern, rtrim($publicUrl, '/'));
 
         if (!$result && substr($publicUrl, 0, 1) === '/') {

diff --git a/Classes/UserFunctions/CheckConfiguration.php b/Classes/UserFunctions/CheckConfiguration.php
@@ -128,6 +128,10 @@ public function renderCheckDirs(): string
 
     protected function isDirectoryMatching(string $directoryPath): bool
     {
+        if ($this->directoryPattern === '') {
+            return false;
+        }
+
         $result = preg_match($this->directoryPattern, $directoryPath) === 1;
 
         if (!$result && substr($directoryPath, 0, 1) === '/') {

diff --git a/Documentation/Admin/ExtensionConfiguration/Index.rst b/Documentation/Admin/ExtensionConfiguration/Index.rst
@@ -22,6 +22,8 @@ Properties
    ==================================== ==================================== ==================
    Property                             Tab                                  Type
    ==================================== ==================================== ==================
+   checkAccess_                         Parsing                              user
+   checkDirs_                           Parsing                              user
    createFileStorage_                   Parsing                              boolean
    securedDirs_                         Parsing                              string
    securedFiletypes_                    Parsing                              string
@@ -39,10 +41,42 @@ Properties
    forcedownloadtype_                   File Delivery                        string
    allowPublicAccess_                   File Delivery                        boolean
    log_                                 Module                               boolean
+   skipCheckConfiguration_              Backend                              boolean
    ==================================== ==================================== ==================
 
 .. ### BEGIN~OF~TABLE ###
 
+.. _admin-extensionConfiguration-checkAccess:
+
+checkAccess
+-----------------
+.. container:: table-row
+
+   Property
+        checkAccess
+   Data type
+        user
+   Default
+        :code:`0`
+   Description
+        Used internally for rendering the "Check Direct File Access" section in the extension's configuration module. The configured value has no impact.
+
+.. _admin-extensionConfiguration-checkAccess:
+
+checkAccess
+-----------------
+.. container:: table-row
+
+   Property
+        checkDirs
+   Data type
+        user
+   Default
+        :code:`0`
+   Description
+        Used internally for rendering the "Checks directories matching the pattern" section in the extension's configuration module. The configured value has no impact.
+
+
 .. _admin-extensionConfiguration-createFileStorage:
 
 createFileStorage
@@ -340,4 +374,19 @@ log
          Each file access will be logged to database, this could be a performance issue, if you have a high traffic site. If you
          decide to turn it on, a backend module will be activated to see the traffic caused by user/ file
 
+.. _admin-extensionConfiguration-skipCheckConfiguration:
+
+skipCheckConfiguration
+---
+.. container:: table-row
+
+   Property
+         skipCheckConfiguration
+   Data type
+         boolean
+   Default
+         :code:`false`
+   Description
+         Skip checking the secured files and directories in the extension's configuration module. The sections "Check Direct File Access" and "Checks directories matching the pattern" will not be rendered. This option may be useful if you have many or large secured directories.
+
 .. ### END~OF~TABLE ###
diff --git a/Tests/Unit/Service/SecureDownloadServiceTest.php b/Tests/Unit/Service/SecureDownloadServiceTest.php
@@ -59,6 +59,38 @@ public function someDirectoriesPatternTests()
         self::assertFalse($secureDownloadService->folderShouldBeSecured('fileadmin-secure'));
     }
 
+    /**
+     * @test
+     */
+    public function emptyDirectoriesPatternTests()
+    {
+        $extensionConfiguration = $this->getMockBuilder(ExtensionConfiguration::class)
+            ->disableOriginalConstructor()
+            ->onlyMethods([])
+            ->getMock();
+
+        $configuration = [
+            'securedDirs' => '',
+            'securedFiletypes' => 'pdf|jpe?g|gif|png|odt|pptx?|docx?|xlsx?|zip|rar|tgz|tar|gz',
+        ];
+
+        $this->invokeMethod($extensionConfiguration, 'setPropertiesFromConfiguration', [$configuration]);
+
+        $secureDownloadService = new SecureDownloadService($extensionConfiguration);
+
+        //matching
+
+        self::assertFalse($secureDownloadService->folderShouldBeSecured('typo3temp'));
+        self::assertFalse($secureDownloadService->folderShouldBeSecured('/typo3temp'));
+        self::assertFalse($secureDownloadService->folderShouldBeSecured('fileadmin/secure'));
+        self::assertFalse($secureDownloadService->folderShouldBeSecured('fileadmin/secure/something_else'));
+        self::assertFalse($secureDownloadService->folderShouldBeSecured('/fileadmin/secure'));
+        self::assertFalse($secureDownloadService->folderShouldBeSecured('nomatch'));
+        self::assertFalse($secureDownloadService->folderShouldBeSecured('fileadmin'));
+        self::assertFalse($secureDownloadService->folderShouldBeSecured('/fileadmin-secure'));
+        self::assertFalse($secureDownloadService->folderShouldBeSecured('fileadmin-secure'));
+    }
+
     /**
      * @test
      */

diff --git a/Tests/Unit/UserFunctions/CheckConfigurationTest.php b/Tests/Unit/UserFunctions/CheckConfigurationTest.php
@@ -59,4 +59,33 @@ public function someDirectoriesPatternTests()
         self::assertFalse($this->invokeMethod($checkConfiguration, 'isDirectoryMatching', ['/fileadmin-secure']));
         self::assertFalse($this->invokeMethod($checkConfiguration, 'isDirectoryMatching', ['fileadmin-secure']));
     }
+
+    /**
+     * @test
+     */
+    public function emptyDirectoriesPatternTests()
+    {
+        $extensionConfiguration = $this->getMockBuilder(ExtensionConfiguration::class)
+            ->disableOriginalConstructor()
+            ->onlyMethods([])
+            ->getMock();
+
+        $configuration = [
+            'securedDirs' => '',
+        ];
+
+        $this->invokeMethod($extensionConfiguration, 'setPropertiesFromConfiguration', [$configuration]);
+
+        $checkConfiguration = new CheckConfiguration($extensionConfiguration);
+
+        self::assertFalse($this->invokeMethod($checkConfiguration, 'isDirectoryMatching', ['typo3temp']));
+        self::assertFalse($this->invokeMethod($checkConfiguration, 'isDirectoryMatching', ['/typo3temp']));
+        self::assertFalse($this->invokeMethod($checkConfiguration, 'isDirectoryMatching', ['fileadmin/secure']));
+        self::assertFalse($this->invokeMethod($checkConfiguration, 'isDirectoryMatching', ['fileadmin/secure/something_else']));
+        self::assertFalse($this->invokeMethod($checkConfiguration, 'isDirectoryMatching', ['/fileadmin/secure']));
+        self::assertFalse($this->invokeMethod($checkConfiguration, 'isDirectoryMatching', ['nomatch']));
+        self::assertFalse($this->invokeMethod($checkConfiguration, 'isDirectoryMatching', ['fileadmin']));
+        self::assertFalse($this->invokeMethod($checkConfiguration, 'isDirectoryMatching', ['/fileadmin-secure']));
+        self::assertFalse($this->invokeMethod($checkConfiguration, 'isDirectoryMatching', ['fileadmin-secure']));
+    }
 }