From 7902cfa58a09eff0165a9962f562d1bced2d8045 Mon Sep 17 00:00:00 2001
From: Samuel Georges <sam@daftspunk.com>
Date: Sat, 14 Dec 2019 12:37:44 +1100
Subject: [PATCH] Simplify security check

Logic in ComponentPartial was rolled back and moved to the Controller. Since there are issues with throwing exceptions inside the component partial lookup logic (exceptions are conditionally suppressed), it seems like it would be better to bubble up the security logic to the controller level as a simple base dir security check, which is no longer concerned about any suppression logic. This looks to have logic parity with the previous solution

Refs #4652
---
 modules/cms/classes/ComponentPartial.php | 33 +++++-------------------
 modules/cms/classes/Controller.php       | 18 +++++++------
 2 files changed, 16 insertions(+), 35 deletions(-)

diff --git a/modules/cms/classes/ComponentPartial.php b/modules/cms/classes/ComponentPartial.php
index 1b8f74a78..68a444b2c 100644
--- a/modules/cms/classes/ComponentPartial.php
+++ b/modules/cms/classes/ComponentPartial.php
@@ -107,10 +107,6 @@ class ComponentPartial extends Extendable implements CmsObjectContract
             $partial = Partial::loadCached($theme, $component->alias . '/' . $fileName);
         }
 
-        if ($partial !== null) {
-            static::ensureValidPartialPath($partial->getFileName(), $partial->getFilePath(), null);
-        }
-
         return $partial;
     }
 
@@ -164,36 +160,19 @@ class ComponentPartial extends Extendable implements CmsObjectContract
      */
     protected function validateFileName($fileName)
     {
+        if (!FileHelper::validatePath($fileName, $this->maxNesting)) {
+            throw new ApplicationException(Lang::get('cms::lang.cms_object.invalid_file', [
+                'name' => $fileName
+            ]));
+        }
+
         if (!strlen(File::extension($fileName))) {
             $fileName .= '.'.$this->defaultExtension;
         }
 
-        static::ensureValidPartialPath($fileName, $this->getFilePath($fileName), $this->maxNesting);
-
         return $fileName;
     }
 
-    /**
-     * Ensures that a partial path is valid and local to the application.
-     *
-     * @param string $fileName
-     * @param        $realpath
-     * @param int    $maxNesting
-     *
-     * @return bool
-     * @throws ApplicationException
-     */
-    protected static function ensureValidPartialPath($fileName, $realpath, $maxNesting = 2)
-    {
-        if (FileHelper::validatePath($fileName, $maxNesting) && FileHelper::validateIsLocalFile($realpath)) {
-            return true;
-        }
-
-        throw new ApplicationException(Lang::get('cms::lang.cms_object.invalid_file', [
-            'name' => $fileName,
-        ]));
-    }
-
     /**
      * Returns the file content.
      * @return string
diff --git a/modules/cms/classes/Controller.php b/modules/cms/classes/Controller.php
index d02ad33a5..74cb8e269 100644
--- a/modules/cms/classes/Controller.php
+++ b/modules/cms/classes/Controller.php
@@ -1003,14 +1003,7 @@ class Controller
              * Check the component partial
              */
             if ($partial === null) {
-                try {
-                    $partial = ComponentPartial::loadCached($componentObj, $partialName);
-                } catch (ApplicationException $e) {
-                    if ($throwException) {
-                        throw $e;
-                    }
-                    return false;
-                }
+                $partial = ComponentPartial::loadCached($componentObj, $partialName);
             }
 
             if ($partial === null) {
@@ -1037,6 +1030,15 @@ class Controller
             return false;
         }
 
+        /*
+         * Security check
+         */
+        if (!\Cms\Helpers\File::validateIsLocalFile($partial->getFilePath())) {
+            throw new CmsException(Lang::get('cms::lang.cms_object.invalid_file', [
+                'name' => $partial->getFileName()
+            ]));
+        }
+
         /*
          * Run functions for CMS partials only (Cms\Classes\Partial)
          */