From fd1d68cbaac5c8a44aaab335700eba8f3723c754 Mon Sep 17 00:00:00 2001 From: Lukas Winkler Date: Mon, 2 Nov 2020 20:12:09 +0100 Subject: simplified cookies (#14444) --- CHANGELOG.md | 1 + core/Cookie.php | 62 ++++++++-------- core/Tracker/IgnoreCookie.php | 2 +- plugins/ExampleSettingsPlugin/SystemSettings.php | 2 +- tests/PHPUnit/Unit/CookieTest.php | 91 ++++++++++++++++++++++++ 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 */ -- cgit v1.2.3