From 95b9d8efc15d62c1a048a13f4e94e34e26de4cb5 Mon Sep 17 00:00:00 2001
From: Sam <8619576+samjf@users.noreply.github.com>
Date: Wed, 13 Apr 2022 00:58:10 +1200
Subject: Remove invalid hosts tracker settings cache (#19021)

* remove invalid host tracker settings cache

* only check ini cache file if it exists

* Revert "remove invalid host tracker settings cache"

This reverts commit 88ec3b9d37517bfa290ea31022ecc423c8273ab5.

* check host structure before cache init

* fix tests - removing original file isn't necessary for test

* apply PSR12 code formatting

Co-authored-by: sgiehl <stefan@matomo.org>
---
 core/Config/Cache.php                              | 20 ++++---
 core/Config/IniFileChain.php                       | 67 ++++++++++++----------
 core/Url.php                                       |  2 +-
 .../PHPUnit/Unit/Config/IniFileChainCacheTest.php  | 39 +++++++------
 4 files changed, 72 insertions(+), 56 deletions(-)

diff --git a/core/Config/Cache.php b/core/Config/Cache.php
index 5548c14868..d0b781dea3 100644
--- a/core/Config/Cache.php
+++ b/core/Config/Cache.php
@@ -1,4 +1,5 @@
 <?php
+
 /**
  * Matomo - free/libre analytics platform
  *
@@ -36,9 +37,14 @@ class Cache extends File
         return PIWIK_INCLUDE_PATH . '/tmp/' . $host . '/cache/tracker';
     }
 
+    public static function hasHostConfig($mergedConfigSettings)
+    {
+        return isset($mergedConfigSettings['General']['trusted_hosts']) && is_array($mergedConfigSettings['General']['trusted_hosts']);
+    }
+
     public function isValidHost($mergedConfigSettings)
     {
-        if (!isset($mergedConfigSettings['General']['trusted_hosts']) || !is_array($mergedConfigSettings['General']['trusted_hosts'])) {
+        if (!self::hasHostConfig($mergedConfigSettings)) {
             return false;
         }
         // note: we do not support "enable_trusted_host_check" to keep things secure
@@ -51,10 +57,12 @@ class Cache extends File
         $host = Url::getHostSanitized($host); // Remove any port number to get actual hostname
         $host = Common::sanitizeInputValue($host);
 
-        if (empty($host)
+        if (
+            empty($host)
             || strpos($host, '..') !== false
             || strpos($host, '\\') !== false
-            || strpos($host, '/') !== false) {
+            || strpos($host, '/') !== false
+        ) {
             throw new \Exception('Unsupported host');
         }
 
@@ -70,19 +78,17 @@ class Cache extends File
         $hosts = Url::getTrustedHosts();
         $initialDir = $this->directory;
 
-        foreach ($hosts as $host)
-        {
+        foreach ($hosts as $host) {
             $dir = $this->makeCacheDir($host);
             if (@is_dir($dir)) {
                 $this->directory = $dir;
                 $success = parent::doDelete($id);
                 if ($success) {
-                    Piwik::postEvent('Core.configFileDeleted', array($this->getFilename($id)));
+                    Piwik::postEvent('Core.configFileDeleted', [$this->getFilename($id)]);
                 }
             }
         }
 
         $this->directory = $initialDir;
     }
-
 }
diff --git a/core/Config/IniFileChain.php b/core/Config/IniFileChain.php
index 2801cbeb26..83d0f73440 100644
--- a/core/Config/IniFileChain.php
+++ b/core/Config/IniFileChain.php
@@ -1,10 +1,12 @@
 <?php
+
 /**
  * Matomo - free/libre analytics platform
  *
  * @link https://matomo.org
  * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
  */
+
 namespace Piwik\Config;
 
 use Piwik\Common;
@@ -42,14 +44,14 @@ class IniFileChain
      *
      * @var array
      */
-    protected $settingsChain = array();
+    protected $settingsChain = [];
 
     /**
      * The merged INI settings.
      *
      * @var array
      */
-    protected $mergedSettings = array();
+    protected $mergedSettings = [];
 
     /**
      * Constructor.
@@ -57,7 +59,7 @@ class IniFileChain
      * @param string[] $defaultSettingsFiles The list of paths to INI files w/ the default setting values.
      * @param string|null $userSettingsFile The path to the user settings file.
      */
-    public function __construct(array $defaultSettingsFiles = array(), $userSettingsFile = null)
+    public function __construct(array $defaultSettingsFiles = [], $userSettingsFile = null)
     {
         $this->reload($defaultSettingsFiles, $userSettingsFile);
     }
@@ -71,7 +73,7 @@ class IniFileChain
     public function &get($name)
     {
         if (!isset($this->mergedSettings[$name])) {
-            $this->mergedSettings[$name] = array();
+            $this->mergedSettings[$name] = [];
         }
 
         $result =& $this->mergedSettings[$name];
@@ -146,12 +148,12 @@ class IniFileChain
 
         $dirty = false;
 
-        $configToWrite = array();
+        $configToWrite = [];
         foreach ($this->mergedSettings as $sectionName => $changedSection) {
-            if(isset($existingMutableSettings[$sectionName])){
+            if (isset($existingMutableSettings[$sectionName])) {
                 $existingMutableSection = $existingMutableSettings[$sectionName];
-            } else{
-                $existingMutableSection = array();
+            } else {
+                $existingMutableSection = [];
             }
 
             // remove default values from both (they should not get written to local)
@@ -162,7 +164,8 @@ class IniFileChain
 
             // if either local/config have non-default values and the other doesn't,
             // OR both have values, but different values, we must write to config.ini.php
-            if (empty($changedSection) xor empty($existingMutableSection)
+            if (
+                empty($changedSection) xor empty($existingMutableSection)
                 || (!empty($changedSection)
                     && !empty($existingMutableSection)
                     && self::compareElements($changedSection, $existingMutableSection))
@@ -207,9 +210,10 @@ class IniFileChain
     /**
      * Reloads settings from disk.
      */
-    public function reload($defaultSettingsFiles = array(), $userSettingsFile = null)
+    public function reload($defaultSettingsFiles = [], $userSettingsFile = null)
     {
-        if (!empty($defaultSettingsFiles)
+        if (
+            !empty($defaultSettingsFiles)
             || !empty($userSettingsFile)
         ) {
             $this->resetSettingsChain($defaultSettingsFiles, $userSettingsFile);
@@ -218,13 +222,15 @@ class IniFileChain
         $hasAbsoluteConfigFile = !empty($userSettingsFile) && strpos($userSettingsFile, DIRECTORY_SEPARATOR) === 0;
         $useConfigCache = !empty($GLOBALS['ENABLE_CONFIG_PHP_CACHE']) && $hasAbsoluteConfigFile;
 
-        if ($useConfigCache) {
+        if ($useConfigCache && is_file($userSettingsFile)) {
             $cache = new Cache();
             $values = $cache->doFetch(self::CONFIG_CACHE_KEY);
-            
-            if (!empty($values)
+
+            if (
+                !empty($values)
                 && isset($values['mergedSettings'])
-                && isset($values['settingsChain'][$userSettingsFile])) {
+                && isset($values['settingsChain'][$userSettingsFile])
+            ) {
                 $this->mergedSettings = $values['mergedSettings'];
                 $this->settingsChain = $values['settingsChain'];
                 return;
@@ -253,16 +259,18 @@ class IniFileChain
         if (!empty($GLOBALS['MATOMO_MODIFY_CONFIG_SETTINGS']) && !empty($this->mergedSettings)) {
             $this->mergedSettings = call_user_func($GLOBALS['MATOMO_MODIFY_CONFIG_SETTINGS'], $this->mergedSettings);
         }
-        
-        if ($useConfigCache
-            && !empty($this->mergedSettings)
-            && !empty($this->settingsChain)) {
 
+        if (
+            $useConfigCache
+            && !empty($this->mergedSettings)
+            && !empty($this->settingsChain)
+            && Cache::hasHostConfig($this->mergedSettings)
+        ) {
             $ttlOneHour = 3600;
             $cache = new Cache();
             if ($cache->isValidHost($this->mergedSettings)) {
                 // we make sure to save the config only if the host is valid...
-                $data = array('mergedSettings' => $this->mergedSettings, 'settingsChain' => $this->settingsChain);
+                $data = ['mergedSettings' => $this->mergedSettings, 'settingsChain' => $this->settingsChain];
                 $cache->doSave(self::CONFIG_CACHE_KEY, $data, $ttlOneHour);
             }
         }
@@ -278,7 +286,7 @@ class IniFileChain
 
     private function copy($merged)
     {
-        $copy = array();
+        $copy = [];
         foreach ($merged as $index => $value) {
             if (is_array($value)) {
                 $copy[$index] = $this->copy($value);
@@ -291,7 +299,7 @@ class IniFileChain
 
     private function resetSettingsChain($defaultSettingsFiles, $userSettingsFile)
     {
-        $this->settingsChain = array();
+        $this->settingsChain = [];
 
         if (!empty($defaultSettingsFiles)) {
             foreach ($defaultSettingsFiles as $file) {
@@ -308,7 +316,7 @@ class IniFileChain
     {
         $mergedSettings = $this->getMergedDefaultSettings();
 
-        $userSettings = end($this->settingsChain) ?: array();
+        $userSettings = end($this->settingsChain) ?: [];
         foreach ($userSettings as $sectionName => $section) {
             if (!isset($mergedSettings[$sectionName])) {
                 $mergedSettings[$sectionName] = $section;
@@ -326,9 +334,10 @@ class IniFileChain
     {
         $userSettingsFile = $this->getUserSettingsFile();
 
-        $mergedSettings = array();
+        $mergedSettings = [];
         foreach ($this->settingsChain as $file => $settings) {
-            if ($file == $userSettingsFile
+            if (
+                $file == $userSettingsFile
                 || empty($settings)
             ) {
                 continue;
@@ -396,14 +405,14 @@ class IniFileChain
         // ignore keys that are in $original but not in $modified
 
         if (empty($original) || !is_array($original)) {
-            $original = array();
+            $original = [];
         }
 
         if (empty($modified) || !is_array($modified)) {
-            $modified = array();
+            $modified = [];
         }
 
-        return array_udiff_assoc($modified, $original, array(__CLASS__, 'compareElements'));
+        return array_udiff_assoc($modified, $original, [__CLASS__, 'compareElements']);
     }
 
     /**
@@ -537,7 +546,7 @@ class IniFileChain
          *
          * @param array &$values Config values that will be saved
          */
-        Piwik::postEvent('Config.beforeSave', array(&$values));
+        Piwik::postEvent('Config.beforeSave', [&$values]);
         $values = $this->encodeValues($values);
 
         $writer = new IniWriter();
diff --git a/core/Url.php b/core/Url.php
index abe1c82006..cc572aed0b 100644
--- a/core/Url.php
+++ b/core/Url.php
@@ -657,7 +657,7 @@ class Url
      * Returns hostname, without port numbers
      *
      * @param $host
-     * @return array
+     * @return string
      */
     public static function getHostSanitized($host)
     {
diff --git a/tests/PHPUnit/Unit/Config/IniFileChainCacheTest.php b/tests/PHPUnit/Unit/Config/IniFileChainCacheTest.php
index c3d43a0e1b..9aecf438a5 100644
--- a/tests/PHPUnit/Unit/Config/IniFileChainCacheTest.php
+++ b/tests/PHPUnit/Unit/Config/IniFileChainCacheTest.php
@@ -1,10 +1,12 @@
 <?php
+
 /**
  * Matomo - free/libre analytics platform
  *
  * @link https://matomo.org
  * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
  */
+
 namespace Piwik\Tests\Unit\Config;
 
 use Piwik\Config;
@@ -15,7 +17,7 @@ class TestIniFileChain extends IniFileChain
 {
     public $addHostInfo = '';
 
-    public function __construct(array $defaultSettingsFiles = array(), $userSettingsFile = null, $addhostInfo = '')
+    public function __construct(array $defaultSettingsFiles = [], $userSettingsFile = null, $addhostInfo = '')
     {
         $this->addHostInfo = $addhostInfo;
         parent::__construct($defaultSettingsFiles, $userSettingsFile);
@@ -26,7 +28,7 @@ class TestIniFileChain extends IniFileChain
         $settings = parent::mergeFileSettings();
 
         if (!empty($this->addHostInfo)) {
-            $settings['General'] = ['trusted_hosts'=> [$this->addHostInfo]];
+            $settings['General'] = ['trusted_hosts' => [$this->addHostInfo]];
         }
 
         return $settings;
@@ -56,7 +58,7 @@ class IniFileChainCacheTest extends IniFileChainTest
 
     private function setTrustedHosts()
     {
-        Config::setSetting('General', 'trusted_hosts', array($this->testHost, 'foonot.exists'));
+        Config::setSetting('General', 'trusted_hosts', [$this->testHost, 'foonot.exists']);
     }
 
     public function tearDown(): void
@@ -70,7 +72,7 @@ class IniFileChainCacheTest extends IniFileChainTest
     /**
      * @dataProvider getMergingTestData
      */
-    public function test_reload_shouldNotPopulateCacheWhenNoTrustedHostIsConfigured($testDescription, $defaultSettingFiles, $userSettingsFile, $expected)
+    public function testReloadShouldNotPopulateCacheWhenNoTrustedHostIsConfigured($testDescription, $defaultSettingFiles, $userSettingsFile, $expected)
     {
         $value = $this->cache->doFetch(IniFileChain::CONFIG_CACHE_KEY);
         $this->assertEquals(false, $value);
@@ -86,14 +88,14 @@ class IniFileChainCacheTest extends IniFileChainTest
     /**
      * @dataProvider getMergingTestData
      */
-    public function test_reload_shouldNotPopulateCacheWhenTrustedHostIsNotValid($testDescription, $defaultSettingFiles, $userSettingsFile, $expected)
+    public function testReloadShouldNotPopulateCacheWhenTrustedHostIsNotValid($testDescription, $defaultSettingFiles, $userSettingsFile, $expected)
     {
         $value = $this->cache->doFetch(IniFileChain::CONFIG_CACHE_KEY);
         $this->assertEquals(false, $value);
 
         // reading the chain should populate the cache
         $fileChain = new TestIniFileChain($defaultSettingFiles, $userSettingsFile, 'foo.bar.com');
-        $expected['General'] = array('trusted_hosts' => array('foo.bar.com'));
+        $expected['General'] = ['trusted_hosts' => ['foo.bar.com']];
         $this->assertEquals($expected, $fileChain->getAll(), "'$testDescription' failed");
 
         $value = $this->cache->doFetch(IniFileChain::CONFIG_CACHE_KEY);
@@ -103,20 +105,20 @@ class IniFileChainCacheTest extends IniFileChainTest
     /**
      * @dataProvider getMergingTestData
      */
-    public function test_reload_shoulPopulateCacheWhenTrustedHostIsValid($testDescription, $defaultSettingFiles, $userSettingsFile, $expected)
+    public function testReloadShoulPopulateCacheWhenTrustedHostIsValid($testDescription, $defaultSettingFiles, $userSettingsFile, $expected)
     {
         $value = $this->cache->doFetch(IniFileChain::CONFIG_CACHE_KEY);
         $this->assertEquals(false, $value);
 
         // reading the chain should populate the cache
         $fileChain = new TestIniFileChain($defaultSettingFiles, $userSettingsFile, $this->testHost);
-        $expected['General'] = array('trusted_hosts' => array($this->testHost));
+        $expected['General'] = ['trusted_hosts' => [$this->testHost]];
         $this->assertEquals($expected, $fileChain->getAll(), "'$testDescription' failed");
 
         $value = $this->cache->doFetch(IniFileChain::CONFIG_CACHE_KEY);
         $settingsChain = $value['settingsChain'];
         unset($value['settingsChain']);
-        $this->assertEquals(array('mergedSettings' => $expected), $value);
+        $this->assertEquals(['mergedSettings' => $expected], $value);
 
         foreach ($defaultSettingFiles as $defaultSettingFile) {
             self::assertTrue(array_key_exists($defaultSettingFile, $settingsChain));
@@ -124,11 +126,11 @@ class IniFileChainCacheTest extends IniFileChainTest
 
         $this->assertNotEmpty(array_keys($settingsChain));
     }
-    
+
     /**
      * @dataProvider getMergingTestData
      */
-    public function test_reload_canReadFromCache($testDescription, $defaultSettingFiles, $userSettingsFile, $expected)
+    public function testReloadCanReadFromCache($testDescription, $defaultSettingFiles, $userSettingsFile, $expected)
     {
         $value = $this->cache->doFetch(IniFileChain::CONFIG_CACHE_KEY);
         $this->assertEquals(false, $value);
@@ -138,22 +140,21 @@ class IniFileChainCacheTest extends IniFileChainTest
 
         // reading the chain should populate the cache
         $fileChain = new TestIniFileChain($defaultSettingFiles, $userSettingsFileCopy, $this->testHost);
-        $expected['General'] = array('trusted_hosts' => array($this->testHost));
+        $expected['General'] = ['trusted_hosts' => [$this->testHost]];
         $this->assertEquals($expected, $fileChain->getAll(), "'$testDescription' failed");
 
-        // even though the passed config files don't exist it still returns the same result as it is fetched from
-        // cache
-        unlink($userSettingsFileCopy);
-        $testChain = new TestIniFileChain(array('foo'), $userSettingsFileCopy);
+        // ensure it can be read only from cache
+        $testChain = new TestIniFileChain(['foo'], $userSettingsFileCopy);
         $this->assertEquals($expected, $testChain->getAll(), "'$testDescription' failed");
+        unlink($userSettingsFileCopy);
     }
 
     /**
      * @dataProvider getMergingTestData
      */
-    public function test_populateCache_DeleteCache($testDescription, $defaultSettingFiles, $userSettingsFile, $expected)
+    public function testPopulateCacheDeleteCache($testDescription, $defaultSettingFiles, $userSettingsFile, $expected)
     {
-        $this->test_reload_shoulPopulateCacheWhenTrustedHostIsValid($testDescription, $defaultSettingFiles, $userSettingsFile, $expected);
+        $this->testReloadShoulPopulateCacheWhenTrustedHostIsValid($testDescription, $defaultSettingFiles, $userSettingsFile, $expected);
 
         $value = $this->cache->doFetch(IniFileChain::CONFIG_CACHE_KEY);
         $this->assertNotEmpty($value);
@@ -166,4 +167,4 @@ class IniFileChainCacheTest extends IniFileChainTest
         $value = $this->cache->doFetch(IniFileChain::CONFIG_CACHE_KEY);
         $this->assertEquals(false, $value);
     }
-}
\ No newline at end of file
+}
-- 
cgit v1.2.3