Improve required private directories check (#17606)

4 files changed, 84 insertions, 19 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d82339ef1e..7bc5b5cd44 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,12 @@ This is the Developer Changelog for Matomo platform developers. All changes in o
 
 The Product Changelog at **[matomo.org/changelog](https://matomo.org/changelog)** lets you see more details about any Matomo release, such as the list of new guides and FAQs, security fixes, and links to all closed issues. 
 
+## Matomo 4.3.1
+
+### New commands
+
+* Added new command `core:create-security-files` which creates some web server security files if they haven't existed previously (useful when using for example Apache or IIS web server).
+
 ## Matomo 4.3.0
 
 ### Breaking Changes
diff --git a/plugins/CoreUpdater/Commands/SecurityFiles.php b/plugins/CoreUpdater/Commands/SecurityFiles.php
new file mode 100644
index 0000000000..4127f6d867
--- /dev/null
+++ b/plugins/CoreUpdater/Commands/SecurityFiles.php
@@ -0,0 +1,33 @@
+<?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\Plugins\CoreUpdater\Commands;
+
+use Piwik\Plugins\Installation\ServerFilesGenerator;
+use Piwik\Plugin\ConsoleCommand;
+use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Output\OutputInterface;
+
+/**
+ * @package CoreUpdater
+ */
+class SecurityFiles extends ConsoleCommand
+{
+    protected function configure()
+    {
+        $this->setName('core:create-security-files');
+
+        $this->setDescription('Creates some web server security files if they haven\'t existed previously. Useful when using for example Apache or IIS web server and Matomo cannot create these files automatically because of missing write permissions.');
+    }
+
+    protected function execute(InputInterface $input, OutputInterface $output)
+    {
+        ServerFilesGenerator::createFilesForSecurity();
+        $output->writeln('Done. To check if this worked please open the system report or run `./console diagnostics:run` and look out for the private directories check. If it doesn\'t work you may need to execute this command using a user that has write permissions or maybe you are not using Apache or IIS web server. Please note you may need to execut this command every time you update Matomo to a newer version.');
+    }
+}
diff --git a/plugins/Diagnostics/Diagnostic/RequiredPrivateDirectories.php b/plugins/Diagnostics/Diagnostic/RequiredPrivateDirectories.php
index a1673602ef..74d4175681 100644
--- a/plugins/Diagnostics/Diagnostic/RequiredPrivateDirectories.php
+++ b/plugins/Diagnostics/Diagnostic/RequiredPrivateDirectories.php
@@ -11,6 +11,7 @@ namespace Piwik\Plugins\Diagnostics\Diagnostic;
 use Piwik\Common;
 use Piwik\Filesystem;
 use Piwik\Http;
+use Piwik\Plugins\Installation\ServerFilesGenerator;
 use Piwik\SettingsPiwik;
 use Piwik\Translation\Translator;
 
@@ -43,7 +44,7 @@ class RequiredPrivateDirectories implements Diagnostic
             ['lang/en.json'],
         ];
 
-        // create test file to check if tmp/empty exists
+        // create test file to check if tmp/empty exists Note: This won't work in a load balanced environment!
         Filesystem::mkdir(PIWIK_INCLUDE_PATH . '/tmp');
         file_put_contents(PIWIK_INCLUDE_PATH . '/tmp/empty', 'test');
 
@@ -77,23 +78,12 @@ class RequiredPrivateDirectories implements Diagnostic
 
         $result = new DiagnosticResult($label);
 
-        $isConfigIniAccessible = $this->checkConfigIni($result, $baseUrl);
+        $isConfigIniAccessible = $this->isAccessible($result, $baseUrl . 'config/config.ini.php', ';', 'trusted_hosts[]');
 
         $atLeastOneIsAccessible = $isConfigIniAccessible;
         foreach ($testUrls as $testUrl) {
-            try {
-                $response = Http::sendHttpRequest($testUrl, $timeout = 2, null, null, null, false, false, true);
-                $status = $response['status'];
-
-                $isAccessible = !($status >= 400 && $status < 500);
-                if ($isAccessible) {
-                    $result->addItem(new DiagnosticResultItem(DiagnosticResult::STATUS_ERROR, $testUrl));
-                    $atLeastOneIsAccessible = true;
-                }
-            } catch (\Exception $e) {
-                $error = $e->getMessage();
-                $result->addItem(new DiagnosticResultItem(DiagnosticResult::STATUS_WARNING, 'Unable to execute check for ' .
-                    Common::sanitizeInputValue($testUrl) . ': ' . Common::sanitizeInputValue($error)));
+            if ($this->isAccessible($result, $testUrl, '', '')) {
+                $atLeastOneIsAccessible = true;
             }
         }
 
@@ -102,6 +92,7 @@ class RequiredPrivateDirectories implements Diagnostic
             if ($isConfigIniAccessible) {
                 $pathIsAccessible .= '<br/><br/>' . $this->translator->translate('Diagnostics_ConfigIniAccessible');
             }
+            $pathIsAccessible .= '<br/><br/><a href="https://matomo.org/faq/troubleshooting/how-do-i-fix-the-error-private-directories-are-accessible/" target="_blank" rel="noopener noreferrer">' . $this->translator->translate('General_ReadThisToLearnMore', ['', '']) . '</a>';
             $result->setLongErrorMessage($pathIsAccessible);
         } else {
             $result->addItem(new DiagnosticResultItem(DiagnosticResult::STATUS_OK, $this->translator->translate('Diagnostics_AllPrivateDirectoriesAreInaccessible')));
@@ -119,15 +110,45 @@ class RequiredPrivateDirectories implements Diagnostic
         return $testUrlsList;
     }
 
-    private function checkConfigIni(DiagnosticResult $result, $baseUrl)
+    private function isAccessible(DiagnosticResult $result, $testUrl, $publicIfResponseEquals, $publicIfResponseContains)
     {
-        $testUrl = $baseUrl . 'config/config.ini.php';
         try {
             $response = Http::sendHttpRequest($testUrl, $timeout = 2, null, null, null, false, false, true);
             $status = $response['status'];
 
-            $isAccessible = !($status >= 400 && $status < 500);
-            if ($isAccessible) {
+            if ($status >= 400 && $status < 500) {
+                return false;
+            } elseif ($status >= 300 && $status < 400) {
+                // follow the redirect
+                $response = Http::sendHttpRequest($testUrl, $timeout = 5, null, null, 5, false, false, true);
+                $isResolvedRedirectProtected = $response['status'] >= 400 &&  $response['status'] < 500;
+                if ($isResolvedRedirectProtected) {
+                    // eg someone redirect from http to https or the other way around
+                    return false;
+                }
+
+                // we check for content if possible as they may redirect these files eg to /home or something else
+                if (!$publicIfResponseContains || !$publicIfResponseEquals) {
+                    // it may or may not be an issue depending where they redirect to
+                    // TODO ideally we make this more clear maybe?
+                    $result->addItem(new DiagnosticResultItem(DiagnosticResult::STATUS_WARNING, $testUrl));
+                    return true;
+                }
+
+                if (trim($response) === $publicIfResponseEquals) {
+                    // we assume it is publicly accessible because either the exact expected content is returned or because we don't check for content match
+                    $result->addItem(new DiagnosticResultItem(DiagnosticResult::STATUS_ERROR, $testUrl));
+                    return true;
+                } elseif (strpos($response, $publicIfResponseContains) !== false) {
+                    // we assume it is publicly accessible because a unique content is included in the response or because we don't check for content contains
+                    $result->addItem(new DiagnosticResultItem(DiagnosticResult::STATUS_ERROR, $testUrl));
+                    return true;
+                }
+                // in other cases we assume it's not publicly accessible because we didn't get any expected output in the response
+                // so it seems like they redirect eg to the homepage or another page
+
+            } else {
+                // we assume the file is accessible publicly
                 $result->addItem(new DiagnosticResultItem(DiagnosticResult::STATUS_ERROR, $testUrl));
                 return true;
             }
diff --git a/plugins/Installation/ServerFilesGenerator.php b/plugins/Installation/ServerFilesGenerator.php
index 84ab0bbd05..45a072080f 100644
--- a/plugins/Installation/ServerFilesGenerator.php
+++ b/plugins/Installation/ServerFilesGenerator.php
@@ -81,6 +81,11 @@ Header set Cache-Control \"Cache-Control: private, no-cache, no-store\"
             $directoriesToProtect[dirname($file)] = $denyAll;
         }
 
+        $gitDir = PIWIK_INCLUDE_PATH . '/.git';
+        if (is_dir($gitDir) && is_writable($gitDir)) {
+            $directoriesToProtect[$gitDir] = $denyAll;
+        }
+
         foreach ($directoriesToProtect as $directoryToProtect => $content) {
             self::createHtAccess($directoryToProtect, $overwrite = true, $content);
         }