Use RequestSanitizer in API/Proxy.php instead of Common::getRequestVa…

…r() so we can specify through DI which API request parameters to not sanitize.
matomo-org · Jun 2, 2015 · 0597f7f · 0597f7f
1 parent 86558fe
commit 0597f7f
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 34 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
      *