diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c99ae96ec6..c24610cc368 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/core/API/Proxy.php b/core/API/Proxy.php index 12adbef6338..5463be29bda 100644 --- a/core/API/Proxy.php +++ b/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; } diff --git a/core/API/Request.php b/core/API/Request.php index 43bdc551358..059639fe423 100644 --- a/core/API/Request.php +++ b/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); diff --git a/core/AssetManager/UIAssetFetcher/PluginUmdAssetFetcher.php b/core/AssetManager/UIAssetFetcher/PluginUmdAssetFetcher.php index c31177af158..29b25300e32 100644 --- a/core/AssetManager/UIAssetFetcher/PluginUmdAssetFetcher.php +++ b/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) { diff --git a/core/Exception/NotSupportedBrowserException.php b/core/Exception/NotSupportedBrowserException.php index deee520d6db..d83125787ce 100644 --- a/core/Exception/NotSupportedBrowserException.php +++ b/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) { diff --git a/core/Exception/PluginDeactivatedException.php b/core/Exception/PluginDeactivatedException.php index 7e93e82db04..6bf30bff887 100644 --- a/core/Exception/PluginDeactivatedException.php +++ b/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); } } diff --git a/core/Exception/PluginNotFoundException.php b/core/Exception/PluginNotFoundException.php new file mode 100644 index 00000000000..9c6475de99f --- /dev/null +++ b/core/Exception/PluginNotFoundException.php @@ -0,0 +1,23 @@ +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; diff --git a/core/FrontController.php b/core/FrontController.php index 715dc91ee3d..40ac4262923 100644 --- a/core/FrontController.php +++ b/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); } diff --git a/core/Http/ControllerResolver.php b/core/Http/ControllerResolver.php index 91e1ece9660..81572e0399e 100644 --- a/core/Http/ControllerResolver.php +++ b/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) diff --git a/core/Plugin/Manager.php b/core/Plugin/Manager.php index f6bf7510cdd..cfab9af2d6b 100644 --- a/core/Plugin/Manager.php +++ b/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); diff --git a/tests/PHPUnit/Integration/AssetManager/UIAssetFetcher/PluginUmdAssetFetcherTest.php b/tests/PHPUnit/Integration/AssetManager/UIAssetFetcher/PluginUmdAssetFetcherTest.php index 9c54c228f12..ad78c7dd2c1 100644 --- a/tests/PHPUnit/Integration/AssetManager/UIAssetFetcher/PluginUmdAssetFetcherTest.php +++ b/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); diff --git a/tests/PHPUnit/Integration/Plugin/ManagerTest.php b/tests/PHPUnit/Integration/Plugin/ManagerTest.php index a53a247494f..8a4f7117f54 100644 --- a/tests/PHPUnit/Integration/Plugin/ManagerTest.php +++ b/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 */ diff --git a/tests/PHPUnit/System/ResponseCodeTest.php b/tests/PHPUnit/System/ResponseCodeTest.php index dd0ac187394..f5f6c6ed5c7 100644 --- a/tests/PHPUnit/System/ResponseCodeTest.php +++ b/tests/PHPUnit/System/ResponseCodeTest.php @@ -101,6 +101,46 @@ public function testProcessedApiShouldHaveCorrectHttpStatus() $this->assertEquals(200, $info['http_code']); } + public function testApiMissingRequiredParameterShouldReturn400() + { + [$response, $info] = $this->curl( + Fixture::getRootUrl() . 'tests/PHPUnit/proxy/index.php?module=API&method=SitesManager.getSiteFromId', + ['token_auth' => Fixture::ADMIN_USER_TOKEN] + ); + + $this->assertEquals(400, $info['http_code']); + $this->assertStringContainsString('Please specify a value for \'idSite\'', $response); + } + + public function testValidModuleInvalidActionShouldReturn404() + { + [$response, $info] = $this->curl( + Fixture::getRootUrl() . 'tests/PHPUnit/proxy/index.php?module=CoreHome&action=doesNotExist' + ); + + $this->assertEquals(404, $info['http_code']); + $this->assertStringContainsString("Action 'doesNotExist' not found in the module 'CoreHome'", $response); + } + + public function testMissingAssetChunkShouldReturn404() + { + [, $info] = $this->curl( + Fixture::getRootUrl() . 'tests/PHPUnit/proxy/index.php?module=Proxy&action=getPluginUmdJs&plugin=NotAPlugin' + ); + + $this->assertEquals(404, $info['http_code']); + } + + public function testMissingPluginShouldReturn404HttpStatus() + { + [$response, $info] = $this->curl( + Fixture::getRootUrl() . 'tests/PHPUnit/proxy/index.php?module=NotAPlugin&action=index' + ); + + $this->assertEquals(404, $info['http_code']); + $this->assertStringContainsString('The plugin NotAPlugin was not found.', $response); + } + private function curl($url, $postParams = []) { if (!function_exists('curl_init')) {