simplified cookies (#14444)

5 files changed, 127 insertions, 31 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2052b3ab11..7dc92b9456 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -121,6 +121,7 @@ These are only recommendations (because we will keep backward compatibility for
 * The signature of the event `Segment.addSegments` has been changed. It now has one parameter `SegmentsList $list`, which allows adding new segments to the list
 * The core plugin `CustomPiwikJs` has been renamed to `CustomJsTracker`
 * The class `Piwik\Plugins\CustomPiwikJs\TrackerUpdater` has been renamed to `Piwik\Plugins\CustomJsTracker\TrackerUpdater`
+* The method `Piwik\Cookie::set` no longer accepts an array as value
 * `Zend_Validate` and all subclasses have been completely removed. 
 * Matomo's mail component (`Piwik\Mail`) has been rewritten:
   * Zend_Mail has been removed. `Piwik\Mail` is now an independet class.
diff --git a/core/Cookie.php b/core/Cookie.php
index 1d7a4b5442..a535b7c3c9 100644
--- a/core/Cookie.php
+++ b/core/Cookie.php
@@ -81,9 +81,8 @@ class Cookie
      *                                  use 0 (int zero) to expire cookie at end of browser session
      * @param string $path The path on the server in which the cookie will be available on.
      * @param bool|string $keyStore Will be used to store several bits of data (eg. one array per website)
-     * @param bool $validateSignature If true, the cookie signature will be validated (default).
      */
-    public function __construct($cookieName, $expire = null, $path = null, $keyStore = false, $validateSignature = true)
+    public function __construct($cookieName, $expire = null, $path = null, $keyStore = false)
     {
         $this->name = $cookieName;
         $this->path = $path;
@@ -97,7 +96,7 @@ class Cookie
 
         $this->keyStore = $keyStore;
         if ($this->isCookieFound()) {
-            $this->loadContentFromCookie($validateSignature);
+            $this->loadContentFromCookie();
         }
     }
 
@@ -204,17 +203,17 @@ class Cookie
 
     /**
      * Extract signed content from string: content VALUE_SEPARATOR '_=' signature
+     * Only needed for BC.
      *
      * @param string $content
-     * @param bool $validate
      * @return string|bool  Content or false if unsigned
      */
-    private function extractSignedContent($content, $validate)
+    private function extractSignedContent($content)
     {
         $signature = substr($content, -40);
 
         if (substr($content, -43, 3) === self::VALUE_SEPARATOR . '_=' &&
-            (!$validate || $signature === sha1(substr($content, 0, -40) . SettingsPiwik::getSalt()))
+            ($signature === sha1(substr($content, 0, -40) . SettingsPiwik::getSalt()))
         ) {
             // strip trailing: VALUE_SEPARATOR '_=' signature"
             return substr($content, 0, -43);
@@ -229,9 +228,19 @@ class Cookie
      * Unserialize the array when necessary.
      * Decode the non numeric values that were base64 encoded.
      */
-    protected function loadContentFromCookie($validateSignature = true)
+    protected function loadContentFromCookie()
     {
-        $cookieStr = $this->extractSignedContent($_COOKIE[$this->name], $validateSignature);
+        // we keep trying to read signed content for BC ... if it detects a correctly signed cookie then we read
+        // this value
+        $cookieStr = $this->extractSignedContent($_COOKIE[$this->name]);
+        $isSigned = !empty($cookieStr);
+
+        if ($cookieStr === false
+            && !empty($_COOKIE[$this->name])
+            && strpos($_COOKIE[$this->name], '=') !== false) {
+            // cookie was set since Matomo 4
+            $cookieStr = $_COOKIE[$this->name];
+        }
 
         if ($cookieStr === false) {
             return;
@@ -243,13 +252,18 @@ class Cookie
             $varName = substr($nameValue, 0, $equalPos);
             $varValue = substr($nameValue, $equalPos + 1);
 
-            // no numeric value are base64 encoded so we need to decode them
             if (!is_numeric($varValue)) {
                 $tmpValue = base64_decode($varValue);
-                $varValue = safe_unserialize($tmpValue);
+                if ($isSigned) {
+                    // only unserialise content if it was signed meaning the cookie was generated pre Matomo 4
+                    $varValue = safe_unserialize($tmpValue);
+                } else {
+                    $varValue = $tmpValue;
+                }
 
                 // discard entire cookie
                 // note: this assumes we never serialize a boolean
+                // can only happen when it was signed pre Matomo 4
                 if ($varValue === false && $tmpValue !== 'b:0;') {
                     $this->value = array();
                     unset($_COOKIE[$this->name]);
@@ -269,25 +283,18 @@ class Cookie
      */
     public function generateContentString()
     {
-        $cookieStr = '';
+        $cookieStrArr = [];
 
         foreach ($this->value as $name => $value) {
-            if (!is_numeric($value)) {
-                $value = base64_encode(safe_serialize($value));
+            if (!is_numeric($value) && !is_string($value))  {
+                throw new \Exception('Only strings and numbers can be used in cookies. Value is of type ' . gettype($value));
+            } elseif (!is_numeric($value)) {
+                $value = base64_encode($value);
             }
-
-            $cookieStr .= "$name=$value" . self::VALUE_SEPARATOR;
-        }
-
-        if (!empty($cookieStr)) {
-            $cookieStr .= '_=';
-
-            // sign cookie
-            $signature = sha1($cookieStr . SettingsPiwik::getSalt());
-            return $cookieStr . $signature;
+            $cookieStrArr[] = "$name=$value"; 
         }
 
-        return '';
+        return implode(self::VALUE_SEPARATOR, $cookieStrArr);
     }
 
     /**
@@ -324,14 +331,11 @@ class Cookie
      * Registers a new name => value association in the cookie.
      *
      * Registering new values is optimal if the value is a numeric value.
-     * If the value is a string, it will be saved as a base64 encoded string.
-     * If the value is an array, it will be saved as a serialized and base64 encoded
-     * string which is not very good in terms of bytes usage.
-     * You should save arrays only when you are sure about their maximum data size.
+     * Only numbers and strings can be saved in the cookie.
      * A cookie has to stay small and its size shouldn't increase over time!
      *
      * @param string $name Name of the value to save; the name will be used to retrieve this value
-     * @param string|array|number $value Value to save. If null, entry will be deleted from cookie.
+     * @param string|number $value Value to save. If null, entry will be deleted from cookie.
      */
     public function set($name, $value)
     {
diff --git a/core/Tracker/IgnoreCookie.php b/core/Tracker/IgnoreCookie.php
index 2cd0ae8446..1520451817 100644
--- a/core/Tracker/IgnoreCookie.php
+++ b/core/Tracker/IgnoreCookie.php
@@ -58,7 +58,7 @@ class IgnoreCookie
         
         $thiryYears = time() + (86400 * 365 * 30);
 
-        $cookie = new Cookie($cookie_name, $thiryYears, $cookie_path, false, false);
+        $cookie = new Cookie($cookie_name, $thiryYears, $cookie_path, false);
         
         $domain = @Config::getInstance()->Tracker['cookie_domain'];
         if (!empty($domain)) {
diff --git a/plugins/ExampleSettingsPlugin/SystemSettings.php b/plugins/ExampleSettingsPlugin/SystemSettings.php
index 67b9fcc468..0901ace048 100644
--- a/plugins/ExampleSettingsPlugin/SystemSettings.php
+++ b/plugins/ExampleSettingsPlugin/SystemSettings.php
@@ -91,7 +91,7 @@ class SystemSettings extends \Piwik\Settings\Plugin\SystemSettings
             $field->uiControl = FieldConfig::UI_CONTROL_PASSWORD;
             $field->description = 'Password for the 3rd API where we fetch the value';
             $field->transform = function ($value) {
-                return sha1($value . 'salt');
+                return password_hash($value, PASSWORD_DEFAULT);
             };
         });
     }
diff --git a/tests/PHPUnit/Unit/CookieTest.php b/tests/PHPUnit/Unit/CookieTest.php
index aa04caf47f..bd7425836e 100644
--- a/tests/PHPUnit/Unit/CookieTest.php
+++ b/tests/PHPUnit/Unit/CookieTest.php
@@ -8,10 +8,101 @@
 
 namespace Piwik\Tests\Unit;
 
+use Piwik\Common;
 use Piwik\Cookie;
+use Piwik\SettingsPiwik;
 
 class CookieTest extends \PHPUnit\Framework\TestCase
 {
+    const TEST_COOKIE_NAME = 'fooBarTest';
+
+    /**
+     * @var Cookie
+     */
+    private $cookie;
+
+    public function setUp(): void
+    {
+        parent::setUp();
+        $this->cookie = $this->makeCookie();
+    }
+
+    public function tearDown(): void
+    {
+        unset($_COOKIE[self::TEST_COOKIE_NAME]);
+        parent::tearDown();
+    }
+
+    private function makeCookie()
+    {
+        return new Cookie(self::TEST_COOKIE_NAME);
+    }
+
+    public function test_loadContentFromCookie()
+    {
+        $_COOKIE[self::TEST_COOKIE_NAME] = 'hello=1.2:ignore=Kg==:foo=:bar=dGVzdDp2YWx1ZQ==';
+        $this->cookie = $this->makeCookie();
+        $this->assertEquals('1.2', $this->cookie->get('hello'));
+        $this->assertEquals('*', $this->cookie->get('ignore'));
+        $this->assertEquals('', $this->cookie->get('foo'));
+        $this->assertEquals('test:value', $this->cookie->get('bar'));
+    }
+
+    public function test_loadContentFromCookie_wontUnserialiseContentIfNotSigned()
+    {
+        $val = safe_serialize(['foobar']);
+        $_COOKIE[self::TEST_COOKIE_NAME] = 'hello=' . base64_encode($val) . ':_=foobar';
+        $this->cookie = $this->makeCookie();
+        $this->assertEquals(Common::sanitizeInputValues($val), $this->cookie->get('hello'));
+    }
+
+    public function test_loadContentFromCookie_willUnserialiseContentIfSigned()
+    {
+        $val = safe_serialize(['foobar']);
+        $cookieStr = 'hello=' . base64_encode($val) . ':_=';
+        $_COOKIE[self::TEST_COOKIE_NAME] = $cookieStr . sha1($cookieStr . SettingsPiwik::getSalt());
+        $this->cookie = $this->makeCookie();
+        $this->assertEquals(['foobar'], $this->cookie->get('hello'));
+    }
+
+    public function test_get_set()
+    {
+        $this->cookie->set('ignore', '*f1');
+        $this->assertEquals('*f1', $this->cookie->get('ignore'));
+    }
+
+    public function test_generateContentString_usesBase64encode_string()
+    {
+        $this->cookie->set('ignore', '*');
+        $this->assertEquals('ignore=Kg==', $this->cookie->generateContentString());
+    }
+
+    public function test_generateContentString_usesPlainTextNumber()
+    {
+        $this->cookie->set('hello', '1.2');
+        $this->assertEquals('hello=1.2', $this->cookie->generateContentString());
+
+        $this->cookie->set('hello', 1.2);
+        $this->assertEquals('hello=1.2', $this->cookie->generateContentString());
+    }
+
+    public function test_generateContentString_multipleFields()
+    {
+        $this->cookie->set('hello', '1.2');
+        $this->cookie->set('ignore', '*');
+        $this->cookie->set('foo', '');
+        $this->cookie->set('bar', 'test:value');
+        $this->assertEquals('hello=1.2:ignore=Kg==:foo=:bar=dGVzdDp2YWx1ZQ==', $this->cookie->generateContentString());
+    }
+
+    public function test_generateContentString_throwsExceptionWhenNotStringOrNumber()
+    {
+        $this->expectException(\Exception::class);
+        $this->expectExceptionMessage('Only strings and numbers can be used in cookies. Value is of type array');
+        $this->cookie->set('ignore', array('foo'));
+        $this->cookie->generateContentString();
+    }
+
     /**
      * Dataprovider for testJsonSerialize
      */