Normalize 4xx responses for invalid plugins/actions/assets and quiet their logging

CHANGELOG.md

@@ -6,6 +6,15 @@ The Product Changelog at **[matomo.org/changelog](https://matomo.org/changelog)*
 
 ## Matomo 5.7.0
 
+### Breaking Changes
+
+* Client-side faults now map to consistent 4xx responses:
+  - Missing/invalid API params return 400
+  - Invalid actions return 404
+  - Missing chunks return 404
+  - Missing plugins return 404
+  - Deactivated plugins return 403
+
 ### New Features
 
 * New event `PrivacyManager.deleteDataSubjectsForDeletedSites` to enable plugins to be GDPR compliant, when tracking visit unrelated data.

core/API/Proxy.php

@@ -485,7 +485,7 @@ private function getSanitizedRequestParametersArray($requiredParameters, $parame
                     }
                 }
             } catch (Exception $e) {
-                throw new Exception(Piwik::translate('General_PleaseSpecifyValue', array($name)));
+                throw new BadRequestException(Piwik::translate('General_PleaseSpecifyValue', [$name]));
             }
             $finalParameters[$name] = $requestValue;
         }
@@ -541,7 +541,7 @@ private function getRequestParametersArray($requiredParameters, \Piwik\Request $
                     $requestValue = $request->$method($name, $defaultValue);
                 }
             } catch (Exception $e) {
-                throw new Exception(Piwik::translate('General_PleaseSpecifyValue', [$name]));
+                throw new BadRequestException(Piwik::translate('General_PleaseSpecifyValue', [$name]));
             }
             $finalParameters[$name] = $requestValue;
         }

core/API/Request.php

@@ -11,6 +11,7 @@
 
 use Exception;
 use Piwik\Access;
+use Piwik\Http\HttpCodeException;
 use Piwik\Request\AuthenticationToken;
 use Piwik\Cache;
 use Piwik\Common;
@@ -283,10 +284,17 @@ public function process()
                 return $response->getResponse($returnedValue, $module, $method);
             });
         } catch (Exception $e) {
-            StaticContainer::get(LoggerInterface::class)->error('Uncaught exception in API: {exception}', [
-                'exception' => $e,
-                'ignoreInScreenWriter' => true,
-            ]);
+            if ($e instanceof HttpCodeException && $e->getCode() >= 400 && $e->getCode() < 500) {
+                StaticContainer::get(LoggerInterface::class)->debug('Uncaught client error in API: {exception}', [
+                    'exception'            => $e,
+                    'ignoreInScreenWriter' => true,
+                ]);
+            } else {
+                StaticContainer::get(LoggerInterface::class)->error('Uncaught exception in API: {exception}', [
+                    'exception'            => $e,
+                    'ignoreInScreenWriter' => true,
+                ]);
+            }
 
             if (empty($response)) {
                 $response = new ResponseBuilder('console', $this->request);

core/AssetManager/UIAssetFetcher/PluginUmdAssetFetcher.php

@@ -14,6 +14,7 @@
 use Piwik\Config;
 use Piwik\Container\StaticContainer;
 use Piwik\Development;
+use Piwik\Exception\ThingNotFoundException;
 use Piwik\Plugin\Manager;
 
 class PluginUmdAssetFetcher extends UIAssetFetcher
@@ -202,7 +203,7 @@ protected function retrieveFileLocations()
             }
 
             if (!$foundChunk) {
-                throw new \Exception("Could not find chunk {$this->requestedChunk}");
+                throw new ThingNotFoundException('Could not find chunk {$this->requestedChunk}');
             }
 
             foreach ($foundChunk->getFiles() as $file) {

core/Exception/NotSupportedBrowserException.php

@@ -9,7 +9,9 @@
 
 namespace Piwik\Exception;
 
-class NotSupportedBrowserException extends \Piwik\Exception\Exception
+use Piwik\Http\HttpCodeException;
+
+class NotSupportedBrowserException extends \Piwik\Exception\Exception implements HttpCodeException
 {
     public function __construct($message)
     {

core/Exception/PluginDeactivatedException.php

@@ -9,13 +9,15 @@
 
 namespace Piwik\Exception;
 
+use Piwik\Http\HttpCodeException;
+
 /**
  * Exception thrown when the requested plugin is not activated in the config file
  */
-class PluginDeactivatedException extends \Exception
+class PluginDeactivatedException extends \Exception implements HttpCodeException
 {
     public function __construct($module)
     {
-        parent::__construct("The plugin $module is not enabled. You can activate the plugin on Settings > Plugins page in Matomo.");
+        parent::__construct("The plugin $module is not enabled. You can activate the plugin on Settings > Plugins page in Matomo.", 403);
     }
 }

core/Exception/PluginNotFoundException.php

@@ -0,0 +1,23 @@
+<?php
+
+/**
+ * Matomo - free/libre analytics platform
+ *
+ * @link    https://matomo.org
+ * @license https://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ */
+
+namespace Piwik\Exception;
+
+use Piwik\Http\HttpCodeException;
+
+/**
+ * Exception thrown when the requested plugin is not activated in the config file
+ */
+class PluginNotFoundException extends \Exception implements HttpCodeException
+{
+    public function __construct($module)
+    {
+        parent::__construct("The plugin $module was not found.", 404);
+    }
+}

core/Exception/ThingNotFoundException.php

@@ -0,0 +1,20 @@
+<?php
+
+/**
+ * Matomo - free/libre analytics platform
+ *
+ * @link    https://matomo.org
+ * @license https://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ */
+
+namespace Piwik\Exception;
+
+use Piwik\Http\HttpCodeException;
+
+class ThingNotFoundException extends \Piwik\Exception\Exception implements HttpCodeException
+{
+    public function __construct($message, $previous = null)
+    {
+        parent::__construct($message, 404, $previous);
+    }
+}

core/ExceptionHandler.php

@@ -16,6 +16,7 @@
 use Piwik\Container\ContainerDoesNotExistException;
 use Piwik\Container\StaticContainer;
 use Piwik\Exception\IRedirectException;
+use Piwik\Http\HttpCodeException;
 use Piwik\Plugins\CoreAdminHome\CustomLogo;
 use Piwik\Plugins\Monolog\Processor\ExceptionToTextProcessor;
 use Piwik\Log\LoggerInterface;
@@ -74,7 +75,7 @@ public static function dieWithHtmlErrorPage($exception)
     {
         // Set an appropriate HTTP response code.
         switch (true) {
-            case ( ($exception instanceof \Piwik\Http\HttpCodeException || $exception instanceof \Piwik\Exception\NotSupportedBrowserException) && $exception->getCode() > 0):
+            case ($exception instanceof HttpCodeException && $exception->getCode() > 0):
                 // For these exception types, use the exception-provided error code.
                 http_response_code($exception->getCode());
                 break;
@@ -87,8 +88,8 @@ public static function dieWithHtmlErrorPage($exception)
 
         // Log the error with an appropriate loglevel.
         switch (true) {
-            case ($exception instanceof \Piwik\Exception\NotSupportedBrowserException):
-                // These unsupported browsers are really a client-side problem, so log only at DEBUG level.
+            case ($exception instanceof HttpCodeException && $exception->getCode() >= 400 && $exception->getCode() < 500):
+                // Log exceptions, resulting in 4xx HTTP status code, only at debug level
                 self::logException($exception, Log::DEBUG);
                 break;
             default:
@@ -177,9 +178,8 @@ private static function getErrorResponse($ex)
             }
         }
 
-        // Unsupported browser errors shouldn't be written to the web server log. At DEBUG logging level this error will
-        // be written to the application log instead
-        $writeErrorLog = !($ex instanceof \Piwik\Exception\NotSupportedBrowserException);
+        // Exceptions that should result in 4xx status code should not be logged
+        $writeErrorLog = !($ex instanceof HttpCodeException && $ex->getCode() >= 400 && $ex->getCode() < 500);
 
         $redirectUrl = null;
         $countdownToRedirect = null;

core/FrontController.php

@@ -11,6 +11,8 @@
 
 use Exception;
 use Piwik\API\Request;
+use Piwik\Exception\PluginNotFoundException;
+use Piwik\Http\HttpCodeException;
 use Piwik\Request\AuthenticationToken;
 use Piwik\Config\GeneralConfig;
 use Piwik\Container\StaticContainer;
@@ -25,7 +27,6 @@
 use Piwik\Plugins\CoreAdminHome\CustomLogo;
 use Piwik\Session\SessionAuth;
 use Piwik\Session\SessionInitializer;
-use Piwik\SupportedBrowser;
 use Piwik\Log\LoggerInterface;
 
 /**
@@ -113,10 +114,17 @@ private static function generateSafeModeOutputFromError($lastError)
      */
     public static function generateSafeModeOutputFromException($e)
     {
-        StaticContainer::get(LoggerInterface::class)->error('Uncaught exception: {exception}', [
-            'exception' => $e,
-            'ignoreInScreenWriter' => true,
-        ]);
+        if ($e instanceof HttpCodeException && $e->getCode() >= 400 && $e->getCode() < 500) {
+            StaticContainer::get(LoggerInterface::class)->debug('Uncaught client error: {exception}', [
+                'exception'            => $e,
+                'ignoreInScreenWriter' => true,
+            ]);
+        } else {
+            StaticContainer::get(LoggerInterface::class)->error('Uncaught exception: {exception}', [
+                'exception'            => $e,
+                'ignoreInScreenWriter' => true,
+            ]);
+        }
 
         $error = array(
             'message' => $e->getMessage(),
@@ -498,6 +506,10 @@ protected function prepareDispatch($module, $action, $parameters)
             throw new PluginRequiresInternetException($module);
         }
 
+        if (!\Piwik\Plugin\Manager::getInstance()->isPluginInFilesystem($module)) {
+            throw new PluginNotFoundException($module);
+        }
+
         if (!\Piwik\Plugin\Manager::getInstance()->isPluginActivated($module)) {
             throw new PluginDeactivatedException($module);
         }

core/Http/ControllerResolver.php

@@ -10,7 +10,7 @@
 namespace Piwik\Http;
 
 use DI\FactoryInterface;
-use Exception;
+use Piwik\Exception\ThingNotFoundException;
 use Piwik\Plugin\ReportsProvider;
 use Piwik\Plugin\WidgetsProvider;
 
@@ -41,7 +41,7 @@ public function __construct(FactoryInterface $abstractFactory, WidgetsProvider $
      * @param string $module
      * @param string|null $action
      * @param array $parameters
-     * @throws Exception Controller not found.
+     * @throws ThingNotFoundException Controller not found.
      * @return callable The controller is a PHP callable.
      */
     public function getController($module, $action, array &$parameters)
@@ -61,7 +61,7 @@ public function getController($module, $action, array &$parameters)
             return $controller;
         }
 
-        throw new Exception(sprintf("Action '%s' not found in the module '%s'", $action, $module));
+        throw new ThingNotFoundException(sprintf("Action '%s' not found in the module '%s'", $action, $module));
     }
 
     private function createPluginController($module, $action)

core/Plugin/Manager.php

@@ -19,6 +19,7 @@
 use Piwik\Development;
 use Piwik\EventDispatcher;
 use Piwik\Exception\PluginDeactivatedException;
+use Piwik\Exception\PluginNotFoundException;
 use Piwik\Filesystem;
 use Piwik\Log;
 use Piwik\Notification;
@@ -296,10 +297,13 @@ public function doesPluginRequireInternetConnection($name)
      * Checks whether the given plugin is activated, if not triggers an exception.
      *
      * @param  string $pluginName
-     * @throws PluginDeactivatedException
+     * @throws PluginDeactivatedException|PluginNotFoundException
      */
-    public function checkIsPluginActivated($pluginName)
+    public function checkIsPluginActivated($pluginName): void
     {
+        if (!$this->isPluginInFilesystem($pluginName)) {
+            throw new PluginNotFoundException($pluginName);
+        }
         if (!$this->isPluginActivated($pluginName)) {
             throw new PluginDeactivatedException($pluginName);
         }
@@ -737,7 +741,7 @@ public function activatePlugin($pluginName)
         }
 
         if (!$this->isPluginInFilesystem($pluginName)) {
-            throw new \Exception("Plugin '$pluginName' cannot be found in the filesystem in plugins/ directory.");
+            throw new PluginNotFoundException($pluginName);
         }
         $this->deactivateThemeIfTheme($pluginName);
 

tests/PHPUnit/Integration/AssetManager/UIAssetFetcher/PluginUmdAssetFetcherTest.php

@@ -12,6 +12,7 @@
 use Piwik\AssetManager\UIAsset\OnDiskUIAsset;
 use Piwik\AssetManager\UIAssetFetcher\Chunk;
 use Piwik\AssetManager\UIAssetFetcher\PluginUmdAssetFetcher;
+use Piwik\Exception\ThingNotFoundException;
 use Piwik\Filesystem;
 use Piwik\Plugin\Manager;
 use Piwik\Tests\Framework\TestCase\UnitTestCase;
@@ -172,6 +173,17 @@ public function testGetChunkFilesWhenLoadingUmdsIndividually()
         $this->assertEquals($expectedChunkFiles, $actualChunkFiles);
     }
 
+    public function testGetChunkFilesThrows404WhenChunkIsMissing()
+    {
+        $plugins = array_keys(self::TEST_PLUGIN_DEPENDENCIES);
+        $instance = new PluginUmdAssetFetcher($plugins, null, 'does-not-exist', false);
+
+        $this->expectException(ThingNotFoundException::class);
+        $this->expectExceptionCode(404);
+
+        $instance->getCatalog();
+    }
+
     public function testGetChunkFilesWhenLoadingUmdsIndividuallyAndNotAllPluginsActivated()
     {
         $plugins = array_keys(self::TEST_PLUGIN_DEPENDENCIES);

tests/PHPUnit/Integration/Plugin/ManagerTest.php

@@ -12,6 +12,8 @@
 use Piwik\Common;
 use Piwik\Config;
 use Piwik\Container\StaticContainer;
+use Piwik\Exception\PluginDeactivatedException;
+use Piwik\Exception\PluginNotFoundException;
 use Piwik\Http\ControllerResolver;
 use Piwik\Plugin;
 use Piwik\Cache as PiwikCache;
@@ -160,6 +162,37 @@ public function testIsPluginInstalledPluginInstalledConfigButNotExists()
         $this->assertTrue($this->manager->isPluginInstalled('FooBarBaz', false));
     }
 
+    public function testCheckIsPluginActivatedThrows404WhenPluginMissing()
+    {
+        $this->expectException(PluginNotFoundException::class);
+        $this->expectExceptionCode(404);
+
+        $this->manager->checkIsPluginActivated('NotARealPlugin');
+    }
+
+    public function testCheckIsPluginActivatedThrows403WhenPluginDeactivated()
+    {
+        $pluginName = 'ExampleTheme';
+        $wasActivated = $this->manager->isPluginActivated($pluginName);
+
+        if ($wasActivated) {
+            $this->manager->deactivatePlugin($pluginName);
+        }
+
+        try {
+            $this->assertTrue($this->manager->isPluginInFilesystem($pluginName));
+
+            $this->expectException(PluginDeactivatedException::class);
+            $this->expectExceptionCode(403);
+
+            $this->manager->checkIsPluginActivated($pluginName);
+        } finally {
+            if ($wasActivated) {
+                $this->manager->activatePlugin($pluginName);
+            }
+        }
+    }
+
     /**
      * @dataProvider getPluginNameProvider
      */