diff --git a/config/global.php b/config/global.php index d89549efda7..6bdf04a2ced 100644 --- a/config/global.php +++ b/config/global.php @@ -64,5 +64,10 @@ 'observers.global' => array(), - 'Piwik\EventDispatcher' => DI\object()->constructorParameter('observers', DI\get('observers.global')) + 'Piwik\EventDispatcher' => DI\object()->constructorParameter('observers', DI\get('observers.global')), + + 'RequestSanitizer.requestParameterSanitizeBlacklist' => array(), + + 'Piwik\Http\RequestSanitizer' => DI\object() + ->constructorParameter('requestParameterSanitizeBlacklist', DI\get('RequestSanitizer.requestParameterSanitizeBlacklist')), ); diff --git a/core/API/Proxy.php b/core/API/Proxy.php index 1576df9ba07..f16dd49afde 100644 --- a/core/API/Proxy.php +++ b/core/API/Proxy.php @@ -11,6 +11,8 @@ use Exception; use Piwik\Common; +use Piwik\Container\StaticContainer; +use Piwik\Http\RequestSanitizer; use Piwik\Piwik; use Piwik\Singleton; use ReflectionClass; @@ -37,12 +39,18 @@ class Proxy extends Singleton // when a parameter doesn't have a default value we use this private $noDefaultValue; + /** + * @var RequestSanitizer + */ + private $requestSanitizer; + /** * protected constructor */ - protected function __construct() + protected function __construct(RequestSanitizer $requestSanitizer = null) { $this->noDefaultValue = new NoDefaultValue(); + $this->requestSanitizer = $requestSanitizer ?: StaticContainer::get('Piwik\Http\RequestSanitizer'); } /** @@ -158,12 +166,12 @@ public function call($className, $methodName, $parametersRequest) // get the list of parameters required by the method $parameterNamesDefaultValues = $this->getParametersList($className, $methodName); - // load parameters in the right order, etc. - $finalParameters = $this->getRequestParametersArray($parameterNamesDefaultValues, $parametersRequest); - // allow plugins to manipulate the value $pluginName = $this->getModuleNameFromClassName($className); + // load parameters in the right order, etc. + $finalParameters = $this->getRequestParametersArray("$pluginName.$methodName", $parameterNamesDefaultValues, $parametersRequest); + /** * Triggered before an API request is dispatched. * @@ -374,41 +382,26 @@ public function setHideIgnoredFunctions($hideIgnoredFunctions) /** * Returns an array containing the values of the parameters to pass to the method to call * + * @param string $method * @param array $requiredParameters array of (parameter name, default value) * @param array $parametersRequest * @throws Exception * @return array values to pass to the function call */ - private function getRequestParametersArray($requiredParameters, $parametersRequest) + private function getRequestParametersArray($method, $requiredParameters, $parametersRequest) { $finalParameters = array(); foreach ($requiredParameters as $name => $defaultValue) { - try { - if ($defaultValue instanceof NoDefaultValue) { - $requestValue = Common::getRequestVar($name, null, null, $parametersRequest); - } else { - try { - if ($name == 'segment' && !empty($parametersRequest['segment'])) { - // segment parameter is an exception: we do not want to sanitize user input or it would break the segment encoding - $requestValue = ($parametersRequest['segment']); - } else { - $requestValue = Common::getRequestVar($name, $defaultValue, null, $parametersRequest); - } - } catch (Exception $e) { - // Special case: empty parameter in the URL, should return the empty string - if (isset($parametersRequest[$name]) - && $parametersRequest[$name] === '' - ) { - $requestValue = ''; - } else { - $requestValue = $defaultValue; - } - } - } - } catch (Exception $e) { + $isValuePresent = Common::isParameterPresentInRequest($name, $parametersRequest); + + $value = $isValuePresent ? $parametersRequest[$name] : $defaultValue; + if ($value instanceof NoDefaultValue) { throw new Exception(Piwik::translate('General_PleaseSpecifyValue', array($name))); } - $finalParameters[] = $requestValue; + + $value = $this->requestSanitizer->sanitizeApiParameter($method, $name, $value); + + $finalParameters[] = $value; } return $finalParameters; } diff --git a/core/Common.php b/core/Common.php index c0605c3af39..c4a62c8a1f7 100644 --- a/core/Common.php +++ b/core/Common.php @@ -350,10 +350,7 @@ public static function getRequestVar($varName, $varDefault = null, $varType = nu // there is no value $varName in the REQUEST so we try to use the default value if (empty($varName) - || !isset($requestArrayToUse[$varName]) - || (!is_array($requestArrayToUse[$varName]) - && strlen($requestArrayToUse[$varName]) === 0 - ) + || !self::isParameterPresentInRequest($varName, $requestArrayToUse) ) { if (is_null($varDefault)) { throw new Exception("The parameter '$varName' isn't set in the Request, and a default value wasn't provided."); @@ -1205,4 +1202,20 @@ protected static function checkValidLanguagesIsSet($validLanguages) } return $validLanguages; } + + /** + * Returns true if a query parameter exists in an array of query parameters and if the value is not convertible + * to an empty string. + * + * It is important for this to check that the value is not convertible to an empty string. Values like `false` + * and empty strings are not considered valid query parameter values. + * + * @param string $name + * @param array $parametersRequest + * @return bool + */ + public static function isParameterPresentInRequest($name, $parametersRequest) + { + return isset($parametersRequest[$name]) && (is_array($parametersRequest[$name]) || strlen($parametersRequest[$name]) !== 0); + } } diff --git a/core/Http/RequestSanitizer.php b/core/Http/RequestSanitizer.php index ae19a2b55e2..dddb64b4e13 100644 --- a/core/Http/RequestSanitizer.php +++ b/core/Http/RequestSanitizer.php @@ -20,6 +20,25 @@ class RequestSanitizer // Flag used with htmlspecialchar. See php.net/htmlspecialchars. const HTML_ENCODING_QUOTE_STYLE = ENT_QUOTES; + /** + * @var array[] + */ + private $requestParameterSanitizeBlacklist = array(); + + public function __construct(array $requestParameterSanitizeBlacklist) + { + foreach ($requestParameterSanitizeBlacklist as $entry) { + list($apiMethod, $params) = $entry; + + if (isset($this->requestParameterSanitizeBlacklist[$apiMethod])) { + $this->requestParameterSanitizeBlacklist[$apiMethod] = + array_merge($this->requestParameterSanitizeBlacklist[$apiMethod], $params); + } else { + $this->requestParameterSanitizeBlacklist[$apiMethod] = $params; + } + } + } + /** * Sanitizes a single value. * @@ -129,6 +148,31 @@ public function sanitizeJsonValues($value) return $this->sanitizeValues($value, $alreadyStripslashed = true); } + /** + * Sanitizes an API parameter unless it has been blacklisted from sanitization. + * + * @param string $apiMethod The API method being called, eg, SitesManager.getAllSites. + * @param string $name The name of the API parameter being sanitized. + * @param string $value The value of the API parameter. + * @return string + */ + public function sanitizeApiParameter($apiMethod, $name, $value) + { + // segment parameter is an exception: we do not want to sanitize user input or it would break the segment encoding + if ($name == 'segment') { + return $value; + } + + // check if the API parameter was blacklisted in DI + if (!empty($this->requestParameterSanitizeBlacklist[$apiMethod]) + && in_array($name, $this->requestParameterSanitizeBlacklist[$apiMethod]) + ) { + return $value; + } + + return $this->sanitizeValues($value); + } + /** * Sanitize a single input value *