🔒️ Fixed Best Practice, Bug, and Security Issues

sugeng-sulistiyawan · Oct 20, 2024 · 57d795d · 57d795d
1 parent e627b10
commit 57d795d
Show file tree

Hide file tree

Showing 12 changed files with 91 additions and 78 deletions.
diff --git a/src/AbstractComponent.php b/src/AbstractComponent.php
@@ -9,6 +9,7 @@
 use League\Flysystem\PathNormalizer;
 use League\Flysystem\WhitespacePathNormalizer;
 use yii\base\Component;
+use yii\base\InvalidConfigException;
 
 /**
  * Class AbstractComponent
@@ -132,4 +133,20 @@ public function convertToDateTime($dateValue)
      * @return FilesystemAdapter
      */
     abstract protected function initAdapter();
+
+    /**
+     * Validate property
+     * 
+     * @param array $properties
+     * @return void
+     * @throws InvalidConfigException
+     */
+    public function validateProperties(array $properties = [])
+    {
+        foreach ($properties as $property) {
+            if (empty($this->$property)) {
+                throw new InvalidConfigException("The \"$property\" property must be set.");
+            }
+        }
+    }
 }
diff --git a/src/AsyncAwsS3Component.php b/src/AsyncAwsS3Component.php
@@ -110,18 +110,15 @@ class AsyncAwsS3Component extends AbstractComponent
 
     /**
      * @inheritdoc
+     * @throws InvalidConfigException
      */
     public function init()
     {
-        if (empty($this->accessKeyId)) {
-            throw new InvalidConfigException('The "accessKeyId" property must be set.');
-        }
-        if (empty($this->accessKeySecret)) {
-            throw new InvalidConfigException('The "accessKeySecret" property must be set.');
-        }
-        if (empty($this->bucket)) {
-            throw new InvalidConfigException('The "bucket" property must be set.');
-        }
+        $this->validateProperties([
+            'bucket',
+            'accessKeyId',
+            'accessKeySecret',
+        ]);
 
         parent::init();
     }

diff --git a/src/AwsS3Component.php b/src/AwsS3Component.php
@@ -104,20 +104,16 @@ class AwsS3Component extends AbstractComponent
 
     /**
      * @inheritdoc
+     * @throws InvalidConfigException
      */
     public function init()
     {
+        $properties = ['bucket'];
         if (empty($this->credentials)) {
-            if (empty($this->key)) {
-                throw new InvalidConfigException('The "key" property must be set.');
-            }
-            if (empty($this->secret)) {
-                throw new InvalidConfigException('The "secret" property must be set.');
-            }
-        }
-        if (empty($this->bucket)) {
-            throw new InvalidConfigException('The "bucket" property must be set.');
+            $properties[] = 'key';
+            $properties[] = 'secret';
         }
+        $this->validateProperties($properties);
 
         parent::init();
     }

diff --git a/src/GoogleCloudStorageComponent.php b/src/GoogleCloudStorageComponent.php
@@ -140,12 +140,13 @@ class GoogleCloudStorageComponent extends AbstractComponent
 
     /**
      * @inheritdoc
+     * @throws InvalidConfigException
      */
     public function init()
     {
-        if (empty($this->bucket)) {
-            throw new InvalidConfigException('The "bucket" property must be set.');
-        }
+        $this->validateProperties([
+            'bucket',
+        ]);
 
         parent::init();
     }

diff --git a/src/GoogleDriveComponent.php b/src/GoogleDriveComponent.php
@@ -83,34 +83,26 @@ class GoogleDriveComponent extends AbstractComponent
      */
     protected $client;
 
+    /**
+     * @var string[]
+     */
+    protected $_availableOptions = [
+        'teamDriveId',
+        'sharedFolderId',
+    ];
+
     /**
      * @inheritdoc
+     * @throws InvalidConfigException
      */
     public function init()
     {
-        if (empty($this->clientId)) {
-            throw new InvalidConfigException('The "clientId" property must be set.');
-        }
-
-        if (empty($this->clientSecret)) {
-            throw new InvalidConfigException('The "clientSecret" property must be set.');
-        }
-
-        if (empty($this->refreshToken)) {
-            throw new InvalidConfigException('The "refreshToken" property must be set.');
-        }
-
-        if (!empty($this->teamDriveId)) {
-            $this->options['teamDriveId'] = $this->teamDriveId;
-        }
-
-        if (!empty($this->sharedFolderId)) {
-            $this->options['sharedFolderId'] = $this->sharedFolderId;
-        }
-
-        if (empty($this->secret)) {
-            throw new InvalidConfigException('The "secret" property must be set.');
-        }
+        $this->validateProperties([
+            'clientId',
+            'clientSecret',
+            'refreshToken',
+            'secret',
+        ]);
 
         $this->initEncrypter($this->secret);
 
@@ -131,9 +123,19 @@ protected function initAdapter()
             $client->setApplicationName($this->applicationName);
         }
 
+        foreach ($this->_availableOptions as $property) {
+            if (!empty($this->$property)) {
+                $this->options[$property] = $this->$property;
+            }
+        }
+
         $this->client = $client;
         $service = new Drive($this->client);
 
-        return new GoogleDriveAdapter($service, $this->prefix, $this->options);
+        $adapter = new GoogleDriveAdapter($service, $this->prefix, $this->options);
+        // for UrlGeneratorAdapterTrait
+        $adapter->component = $this;
+
+        return $adapter;
     }
 }
diff --git a/src/LocalComponent.php b/src/LocalComponent.php
@@ -44,15 +44,14 @@ class LocalComponent extends AbstractComponent
 
     /**
      * @inheritdoc
+     * @throws InvalidConfigException
      */
     public function init()
     {
-        if (empty($this->path)) {
-            throw new InvalidConfigException('The "path" property must be set.');
-        }
-        if (empty($this->secret)) {
-            throw new InvalidConfigException('The "secret" property must be set.');
-        }
+        $this->validateProperties([
+            'path',
+            'secret',
+        ]);
 
         $this->initEncrypter($this->secret);
 

diff --git a/src/SftpComponent.php b/src/SftpComponent.php
@@ -134,15 +134,14 @@ class SftpComponent extends AbstractComponent
 
     /**
      * @inheritdoc
+     * @throws InvalidConfigException
      */
     public function init()
     {
-        if (empty($this->host)) {
-            throw new InvalidConfigException('The "host" property must be set.');
-        }
-        if (empty($this->username)) {
-            throw new InvalidConfigException('The "username" property must be set.');
-        }
+        $this->validateProperties([
+            'host',
+            'username',
+        ]);
 
         $this->passphrase = $this->passphrase ?: ($this->password ?: ($this->username ?: Yii::$app->id));
         $this->initEncrypter($this->passphrase);

diff --git a/src/WebDavComponent.php b/src/WebDavComponent.php
@@ -91,12 +91,13 @@ class WebDavComponent extends AbstractComponent
 
     /**
      * @inheritdoc
+     * @throws InvalidConfigException
      */
     public function init()
     {
-        if (empty($this->baseUri)) {
-            throw new InvalidConfigException('The "baseUri" property must be set.');
-        }
+        $this->validateProperties([
+            'baseUri',
+        ]);
 
         parent::init();
     }

diff --git a/src/ZipArchiveComponent.php b/src/ZipArchiveComponent.php
@@ -44,15 +44,14 @@ class ZipArchiveComponent extends AbstractComponent
 
     /**
      * @inheritdoc
+     * @throws InvalidConfigException
      */
     public function init()
     {
-        if (empty($this->pathToZip)) {
-            throw new InvalidConfigException('The "pathToZip" property must be set.');
-        }
-        if (empty($this->secret)) {
-            throw new InvalidConfigException('The "secret" property must be set.');
-        }
+        $this->validateProperties([
+            'pathToZip',
+            'secret',
+        ]);
 
         $this->initEncrypter($this->secret);
 

diff --git a/src/actions/FileAction.php b/src/actions/FileAction.php
@@ -52,7 +52,7 @@ public function run($data = null)
         $filesystem = Yii::$app->get($this->component);
 
         try {
-            $params = Json::decode($filesystem->decrypt($data));
+            $params = Json::decode($filesystem?->decrypt($data));
 
             $now = (int) (new DateTimeImmutable())->getTimestamp();
             $expires = (int) $params['expires'];

diff --git a/src/adapter/ZipArchiveAdapter.php b/src/adapter/ZipArchiveAdapter.php
@@ -207,6 +207,8 @@ public function setVisibility(string $path, string $visibility): void
     {
         $archive = $this->zipArchiveProvider->createZipArchive();
         $location = $this->pathPrefixer->prefixPath($path);
+
+        /** @var array|false $stats */
         $stats = $archive->statName($location) ?: $archive->statName($location . '/');
 
         if ($stats === false) {
@@ -377,7 +379,7 @@ public function copy(string $source, string $destination, Config $config): void
             $this->writeStream($destination, $readStream, $config);
         } catch (Throwable $exception) {
             if (isset($readStream)) {
-                @fclose($readStream);
+                /** @scrutinizer ignore-unhandled */ @fclose($readStream);
             }
 
             throw UnableToCopyFile::fromLocationTo($source, $destination, $exception);
@@ -452,7 +454,7 @@ private function setVisibilityAttribute(string $statsName, string $visibility, Z
     // =================================================
 
     /**
-     * @var UrlGeneratorComponentTrait|AbstractComponent
+     * @var AbstractComponent|UrlGeneratorComponentTrait
      */
     public $component;
 
@@ -464,7 +466,7 @@ public function publicUrl(string $path, Config $config): string
             'expires' => 0,
         ];
 
-        return Url::toRoute([$this->component->action, 'data' => $this->component->encrypt(Json::encode($params))], true);
+        return Url::toRoute([$this->component?->action, 'data' => $this->component?->encrypt(Json::encode($params))], true);
     }
 
     public function temporaryUrl(string $path, DateTimeInterface $expiresAt, Config $config): string
@@ -475,6 +477,6 @@ public function temporaryUrl(string $path, DateTimeInterface $expiresAt, Config
             'expires' => (int) $expiresAt->getTimestamp(),
         ];
 
-        return Url::toRoute([$this->component->action, 'data' => $this->component->encrypt(Json::encode($params))], true);
+        return Url::toRoute([$this->component?->action, 'data' => $this->component?->encrypt(Json::encode($params))], true);
     }
 }
diff --git a/src/traits/UrlGeneratorAdapterTrait.php b/src/traits/UrlGeneratorAdapterTrait.php
@@ -19,37 +19,37 @@
 trait UrlGeneratorAdapterTrait
 {
     /**
-     * @var UrlGeneratorComponentTrait|AbstractComponent
+     * @var AbstractComponent|UrlGeneratorComponentTrait
      */
     public $component;
 
     public function publicUrl(string $path, /** @scrutinizer ignore-unused */Config $config): string
     {
         // TODO: Use absolute path and don't encrypt
-        if ($this->component->prefix) {
-            $prefixer = new PathPrefixer($this->component->prefix);
+        if ($this->component?->prefix) {
+            $prefixer = new PathPrefixer((string) $this->component?->prefix);
             $path = $prefixer->stripPrefix($path);
         }
         $params = [
             'path' => $path,
             'expires' => 0,
         ];
 
-        return Url::toRoute([$this->component->action, 'data' => $this->component->encrypt(Json::encode($params))], true);
+        return Url::toRoute([$this->component?->action, 'data' => $this->component?->encrypt(Json::encode($params))], true);
     }
 
     public function temporaryUrl(string $path, DateTimeInterface $expiresAt, /** @scrutinizer ignore-unused */Config $config): string
     {
         // TODO: Use absolute path and don't encrypt
-        if ($this->component->prefix) {
-            $prefixer = new PathPrefixer($this->component->prefix);
+        if ($this->component?->prefix) {
+            $prefixer = new PathPrefixer((string) $this->component?->prefix);
             $path = $prefixer->stripPrefix($path);
         }
         $params = [
             'path' => $path,
             'expires' => (int) $expiresAt->getTimestamp(),
         ];
 
-        return Url::toRoute([$this->component->action, 'data' => $this->component->encrypt(Json::encode($params))], true);
+        return Url::toRoute([$this->component?->action, 'data' => $this->component?->encrypt(Json::encode($params))], true);
     }
 }