Allow setting arbitrary keys in the cookie array

src/Model/User/Cookie.php

@@ -41,126 +41,118 @@ class Cookie
 	const HTTPONLY = true;
 
 	/** @var string The remote address of this node */
-	private $remoteAddr = '0.0.0.0';
+	private $remoteAddr;
 	/** @var bool True, if the connection is ssl enabled */
-	private $sslEnabled = false;
+	private $sslEnabled;
 	/** @var string The private key of this Friendica node */
 	private $sitePrivateKey;
 	/** @var int The default cookie lifetime */
-	private $lifetime = self::DEFAULT_EXPIRE * 24 * 60 * 60;
+	private $lifetime;
-	/** @var array The $_COOKIE array */
+	/** @var array The Friendica cookie data array */
-	private $cookie;
+	private $data;
 
-	public function __construct(IConfig $config, App\BaseURL $baseURL, array $server = [], array $cookie = [])
+	/**
+	 * @param IConfig     $config
+	 * @param App\BaseURL $baseURL
+	 * @param array       $SERVER The $_SERVER array
+	 * @param array       $COOKIE The $_COOKIE array
+	 */
+	public function __construct(IConfig $config, App\BaseURL $baseURL, array $SERVER = [], array $COOKIE = [])
 	{
-		if (!empty($server['REMOTE_ADDR'])) {
-			$this->remoteAddr = $server['REMOTE_ADDR'];
-		}
-
 		$this->sslEnabled     = $baseURL->getSSLPolicy() === App\BaseURL::SSL_POLICY_FULL;
 		$this->sitePrivateKey = $config->get('system', 'site_prvkey');
 
 		$authCookieDays = $config->get('system', 'auth_cookie_lifetime',
 			self::DEFAULT_EXPIRE);
 		$this->lifetime = $authCookieDays * 24 * 60 * 60;
-		$this->cookie   = $cookie;
+
+		$this->remoteAddr = ($SERVER['REMOTE_ADDR'] ?? null) ?: '0.0.0.0';
+
+		$this->data = json_decode($COOKIE[self::NAME] ?? '[]', true) ?: [];
 	}
 
 	/**
-	 * Checks if the Friendica cookie is set for a user
+	 * Returns the value for a key of the Friendica cookie
-	 *
-	 * @param string $hash       The cookie hash
-	 * @param string $password   The user password
-	 * @param string $privateKey The private Key of the user
-	 *
-	 * @return boolean True, if the cookie is set
 	 *
+	 * @param string $key
+	 * @param mixed  $default
+	 * @return mixed|null The value for the provided cookie key
 	 */
-	public function check(string $hash, string $password, string $privateKey)
+	public function get(string $key, $default = null)
 	{
-		return hash_equals(
+		return $this->data[$key] ?? $default;
-			$this->getHash($password, $privateKey),
-			$hash
-		);
 	}
 
 	/**
-	 * Set the Friendica cookie for a user
+	 * Set a single cookie key value.
+	 * Overwrites an existing value with the same key.
 	 *
-	 * @param int      $uid        The user id
+	 * @param $key
-	 * @param string   $password   The user password
+	 * @param $value
-	 * @param string   $privateKey The user private key
+	 * @return bool
-	 * @param int|null $seconds    optional the seconds
+	 */
+	public function set($key, $value): bool
+	{
+		return $this->setMultiple([$key => $value]);
+	}
+
+	/**
+	 * Sets multiple cookie key values.
+	 * Overwrites existing values with the same key.
+	 *
+	 * @param array $values
+	 * @return bool
+	 */
+	public function setMultiple(array $values): bool
+	{
+		$this->data = $values + $this->data;
+
+		return $this->send();
+	}
+
+	/**
+	 * Remove a cookie key
+	 *
+	 * @param string $key
+	 */
+	public function unset(string $key)
+	{
+		if (isset($this->data[$key])) {
+			unset($this->data[$key]);
+
+			$this->send();
+		}
+	}
+
+	/**
+	 * Clears the Friendica cookie
+	 */
+	public function clear(): bool
+	{
+		$this->data = [];
+		// make sure cookie is deleted on browser close, as a security measure
+		return $this->setCookie( '', -3600, $this->sslEnabled);
+	}
+
+	/**
+	 * Send the cookie, should be called every time $this->data is changed or to refresh the cookie.
 	 *
 	 * @return bool
 	 */
-	public function set(int $uid, string $password, string $privateKey, int $seconds = null)
+	public function send(): bool
 	{
-		if (!isset($seconds)) {
+		return $this->setCookie(
-			$seconds = $this->lifetime + time();
+			json_encode(['ip' => $this->remoteAddr] + $this->data),
-		} elseif (isset($seconds) && $seconds != 0) {
+			$this->lifetime + time(),
-			$seconds = $seconds + time();
+			$this->sslEnabled
-		}
-
-		$value = json_encode([
-			'uid'  => $uid,
-			'hash' => $this->getHash($password, $privateKey),
-			'ip'   => $this->remoteAddr,
-		]);
-
-		return $this->setCookie(self::NAME, $value, $seconds, $this->sslEnabled);
-	}
-
-	/**
-	 * Returns the data of the Friendicas user cookie
-	 *
-	 * @return mixed|null The JSON data, null if not set
-	 */
-	public function getData()
-	{
-		// When the "Friendica" cookie is set, take the value to authenticate and renew the cookie.
-		if (isset($this->cookie[self::NAME])) {
-			$data = json_decode($this->cookie[self::NAME]);
-			if (!empty($data)) {
-				return $data;
-			}
-		}
-
-		return null;
-	}
-
-	/**
-	 * Clears the Friendica cookie of this user after leaving the page
-	 */
-	public function clear()
-	{
-		// make sure cookie is deleted on browser close, as a security measure
-		return $this->setCookie(self::NAME, '', -3600, $this->sslEnabled);
-	}
-
-	/**
-	 * Calculate the hash that is needed for the Friendica cookie
-	 *
-	 * @param string $password   The user password
-	 * @param string $privateKey The private key of the user
-	 *
-	 * @return string Hashed data
-	 */
-	private function getHash(string $password, string $privateKey)
-	{
-		return hash_hmac(
-			'sha256',
-			hash_hmac('sha256', $password, $privateKey),
-			$this->sitePrivateKey
 		);
 	}
 
 	/**
-	 * Send a cookie - protected, internal function for test-mocking possibility
+	 * setcookie() wrapper: protected, internal function for test-mocking possibility
 	 *
 	 * @link  https://php.net/manual/en/function.setcookie.php
 	 *
-	 * @param string $name
 	 * @param string $value  [optional]
 	 * @param int    $expire [optional]
 	 * @param bool   $secure [optional]
@@ -168,9 +160,43 @@ class Cookie
 	 * @return bool If output exists prior to calling this function,
 	 *
 	 */
-	protected function setCookie(string $name, string $value = null, int $expire = null,
+	protected function setCookie(string $value = null, int $expire = null,
-	                             bool $secure = null)
+	                             bool $secure = null): bool
 	{
-		return setcookie($name, $value, $expire, self::PATH, self::DOMAIN, $secure, self::HTTPONLY);
+		return setcookie(self::NAME, $value, $expire, self::PATH, self::DOMAIN, $secure, self::HTTPONLY);
+	}
+
+	/**
+	 * Calculate a hash of a user's private data for storage in the cookie.
+	 * Hashed twice, with the user's own private key first, then the node's private key second.
+	 *
+	 * @param string $privateData User private data
+	 * @param string $privateKey  User private key
+	 *
+	 * @return string Hashed data
+	 */
+	public function hashPrivateData(string $privateData, string $privateKey): string
+	{
+		return hash_hmac(
+			'sha256',
+			hash_hmac('sha256', $privateData, $privateKey),
+			$this->sitePrivateKey
+		);
+	}
+
+	/**
+	 * @param string $hash        Hash from a cookie key value
+	 * @param string $privateData User private data
+	 * @param string $privateKey  User private key
+	 *
+	 * @return boolean
+	 *
+	 */
+	public function comparePrivateDataHash(string $hash, string $privateData, string $privateKey): bool
+	{
+		return hash_equals(
+			$this->hashPrivateData($privateData, $privateKey),
+			$hash
+		);
 	}
 }

src/Security/Authentication.php

@@ -33,6 +33,7 @@ use Friendica\Database\DBA;
 use Friendica\DI;
 use Friendica\Model\User;
 use Friendica\Network\HTTPException;
+use Friendica\Repository\TwoFactor\TrustedBrowser;
 use Friendica\Util\DateTimeFormat;
 use Friendica\Util\Network;
 use Friendica\Util\Strings;
@@ -100,16 +101,13 @@ class Authentication
 	 */
 	public function withSession(App $a)
 	{
-		$data = $this->cookie->getData();
-
 		// When the "Friendica" cookie is set, take the value to authenticate and renew the cookie.
-		if (isset($data->uid)) {
+		if ($this->cookie->get('uid')) {
-
 			$user = $this->dba->selectFirst(
 				'user',
 				[],
 				[
-					'uid'             => $data->uid,
+					'uid'             => $this->cookie->get('uid'),
 					'blocked'         => false,
 					'account_expired' => false,
 					'account_removed' => false,
@@ -117,24 +115,25 @@ class Authentication
 				]
 			);
 			if ($this->dba->isResult($user)) {
-				if (!$this->cookie->check($data->hash,
+				if (!$this->cookie->comparePrivateDataHash($this->cookie->get('hash'),
 					$user['password'] ?? '',
-					$user['prvkey'] ?? '')) {
+					$user['prvkey'] ?? '')
-					$this->logger->notice("Hash doesn't fit.", ['user' => $data->uid]);
+				) {
+					$this->logger->notice("Hash doesn't fit.", ['user' => $this->cookie->get('uid')]);
 					$this->session->clear();
 					$this->cookie->clear();
 					$this->baseUrl->redirect();
 				}
 
 				// Renew the cookie
-				$this->cookie->set($user['uid'], $user['password'], $user['prvkey']);
+				$this->cookie->send();
 
 				// Do the authentification if not done by now
 				if (!$this->session->get('authenticated')) {
 					$this->setForUser($a, $user);
 
 					if ($this->config->get('system', 'paranoia')) {
-						$this->session->set('addr', $data->ip);
+						$this->session->set('addr', $this->cookie->get('ip'));
 					}
 				}
 			}
@@ -377,12 +376,15 @@ class Authentication
 			 */
 			if ($this->session->get('remember')) {
 				$this->logger->info('Injecting cookie for remembered user ' . $user_record['nickname']);
-				$this->cookie->set($user_record['uid'], $user_record['password'], $user_record['prvkey']);
+				$this->cookie->setMultiple([
+					'uid'  => $user_record['uid'],
+					'hash' => $this->cookie->hashPrivateData($user_record['password'], $user_record['prvkey']),
+				]);
 				$this->session->remove('remember');
 			}
 		}
 
-		$this->twoFactorCheck($user_record['uid'], $a);
+		$this->redirectForTwoFactorAuthentication($user_record['uid'], $a);
 
 		if ($interactive) {
 			if ($user_record['login_date'] <= DBA::NULL_DATETIME) {
@@ -404,19 +406,23 @@ class Authentication
 	}
 
 	/**
+	 * Decides whether to redirect the user to two-factor authentication.
+	 * All return calls in this method skip two-factor authentication
+	 *
 	 * @param int $uid The User Identified
 	 * @param App $a   The Friendica Application context
 	 *
 	 * @throws HTTPException\ForbiddenException In case the two factor authentication is forbidden (e.g. for AJAX calls)
+	 * @throws HTTPException\InternalServerErrorException
 	 */
-	private function twoFactorCheck(int $uid, App $a)
+	private function redirectForTwoFactorAuthentication(int $uid, App $a)
 	{
 		// Check user setting, if 2FA disabled return
 		if (!$this->pConfig->get($uid, '2fa', 'verified')) {
 			return;
 		}
 
-		// Check current path, if 2fa authentication module return
+		// Check current path, if public or 2fa module return
 		if ($a->argc > 0 && in_array($a->argv[0], ['2fa', 'view', 'help', 'api', 'proxy', 'logout'])) {
 			return;
 		}

tests/Util/StaticCookie.php

@@ -35,22 +35,24 @@ class StaticCookie extends Cookie
 
 	/**
 	 * Send a cookie - protected, internal function for test-mocking possibility
-	 * @see Cookie::setCookie()
 	 *
-	 * @link  https://php.net/manual/en/function.setcookie.php
-	 *
-	 * @param string $name
 	 * @param string $value  [optional]
 	 * @param int    $expire [optional]
 	 * @param bool   $secure [optional]
+	 * @return bool
 	 *
 	 * @noinspection PhpMissingParentCallCommonInspection
 	 *
+	 * @link         https://php.net/manual/en/function.setcookie.php
+	 *
+	 * @see          Cookie::setCookie()
 	 */
-	protected function setCookie(string $name, string $value = null, int $expire = null, bool $secure = null)
+	protected function setCookie(string $value = null, int $expire = null, bool $secure = null): bool
 	{
-		self::$_COOKIE[$name] = $value;
+		self::$_COOKIE[self::NAME] = $value;
 		self::$_EXPIRE = $expire;
+
+		return true;
 	}
 
 	public static function clearStatic()

tests/src/Model/User/CookieTest.php

@@ -128,30 +128,20 @@ class CookieTest extends MockedTest
 		$cookie = new Cookie($this->config, $this->baseUrl, [], $cookieData);
 		self::assertInstanceOf(Cookie::class, $cookie);
 
-		$assertData = $cookie->getData();
+		if (isset($uid)) {
-
+			self::assertEquals($uid, $cookie->get('uid'));
-		if (!$hasValues) {
-			self::assertEmpty($assertData);
 		} else {
-			self::assertNotEmpty($assertData);
+			self::assertNull($cookie->get('uid'));
-			if (isset($uid)) {
+		}
-				self::assertObjectHasAttribute('uid', $assertData);
+		if (isset($hash)) {
-				self::assertEquals($uid, $assertData->uid);
+			self::assertEquals($hash, $cookie->get('hash'));
-			} else {
+		} else {
-				self::assertObjectNotHasAttribute('uid', $assertData);
+			self::assertNull($cookie->get('hash'));
-			}
+		}
-			if (isset($hash)) {
+		if (isset($ip)) {
-				self::assertObjectHasAttribute('hash', $assertData);
+			self::assertEquals($ip, $cookie->get('ip'));
-				self::assertEquals($hash, $assertData->hash);
+		} else {
-			} else {
+			self::assertNull($cookie->get('ip'));
-				self::assertObjectNotHasAttribute('hash', $assertData);
-			}
-			if (isset($ip)) {
-				self::assertObjectHasAttribute('ip', $assertData);
-				self::assertEquals($ip, $assertData->ip);
-			} else {
-				self::assertObjectNotHasAttribute('ip', $assertData);
-			}
 		}
 	}
 
@@ -196,7 +186,7 @@ class CookieTest extends MockedTest
 		$cookie = new Cookie($this->config, $this->baseUrl);
 		self::assertInstanceOf(Cookie::class, $cookie);
 
-		self::assertEquals($assertTrue, $cookie->check($assertHash, $password, $userPrivateKey));
+		self::assertEquals($assertTrue, $cookie->comparePrivateDataHash($assertHash, $password, $userPrivateKey));
 	}
 
 	public function dataSet()
@@ -210,7 +200,6 @@ class CookieTest extends MockedTest
 				'assertHash'  => 'b657a15cfe7ed1f7289c9aa51af14a9a26c966f4ddd74e495fba103d8e872a39',
 				'remoteIp'    => '0.0.0.0',
 				'serverArray' => [],
-				'lifetime'    => null,
 			],
 			'withServerArray' => [
 				'serverKey'   => 23,
@@ -220,32 +209,11 @@ class CookieTest extends MockedTest
 				'assertHash'  => 'b657a15cfe7ed1f7289c9aa51af14a9a26c966f4ddd74e495fba103d8e872a39',
 				'remoteIp'    => '1.2.3.4',
 				'serverArray' => ['REMOTE_ADDR' => '1.2.3.4',],
-				'lifetime'    => null,
-			],
-			'withLifetime0'   => [
-				'serverKey'   => 23,
-				'uid'         => 0,
-				'password'    => '234',
-				'privateKey'  => '124',
-				'assertHash'  => 'b657a15cfe7ed1f7289c9aa51af14a9a26c966f4ddd74e495fba103d8e872a39',
-				'remoteIp'    => '1.2.3.4',
-				'serverArray' => ['REMOTE_ADDR' => '1.2.3.4',],
-				'lifetime'    => 0,
-			],
-			'withLifetime'     => [
-				'serverKey'   => 23,
-				'uid'         => 0,
-				'password'    => '234',
-				'privateKey'  => '124',
-				'assertHash'  => 'b657a15cfe7ed1f7289c9aa51af14a9a26c966f4ddd74e495fba103d8e872a39',
-				'remoteIp'    => '1.2.3.4',
-				'serverArray' => ['REMOTE_ADDR' => '1.2.3.4',],
-				'lifetime'    => 2 * 24 * 60 * 60,
 			],
 		];
 	}
 
-	public function assertCookie($uid, $hash, $remoteIp, $lifetime)
+	public function assertCookie($uid, $hash, $remoteIp)
 	{
 		self::assertArrayHasKey(Cookie::NAME, StaticCookie::$_COOKIE);
 
@@ -258,11 +226,7 @@ class CookieTest extends MockedTest
 		self::assertObjectHasAttribute('ip', $data);
 		self::assertEquals($remoteIp, $data->ip);
 
-		if (isset($lifetime) && $lifetime !== 0) {
+		self::assertLessThanOrEqual(time() + Cookie::DEFAULT_EXPIRE * 24 * 60 * 60, StaticCookie::$_EXPIRE);
-			self::assertLessThanOrEqual(time() + $lifetime, StaticCookie::$_EXPIRE);
-		} else {
-			self::assertLessThanOrEqual(time() + Cookie::DEFAULT_EXPIRE * 24 * 60 * 60, StaticCookie::$_EXPIRE);
-		}
 	}
 
 	/**
@@ -270,7 +234,7 @@ class CookieTest extends MockedTest
 	 *
 	 * @dataProvider dataSet
 	 */
-	public function testSet($serverKey, $uid, $password, $privateKey, $assertHash, $remoteIp, $serverArray, $lifetime)
+	public function testSet($serverKey, $uid, $password, $privateKey, $assertHash, $remoteIp, $serverArray)
 	{
 		$this->baseUrl->shouldReceive('getSSLPolicy')->andReturn(true)->once();
 		$this->config->shouldReceive('get')->with('system', 'site_prvkey')->andReturn($serverKey)->once();
@@ -279,17 +243,20 @@ class CookieTest extends MockedTest
 		$cookie = new StaticCookie($this->config, $this->baseUrl, $serverArray);
 		self::assertInstanceOf(Cookie::class, $cookie);
 
-		$cookie->set($uid, $password, $privateKey, $lifetime);
+		$cookie->setMultiple([
+			'uid' => $uid,
+			'hash' => $assertHash,
+		]);
 
-		self::assertCookie($uid, $assertHash, $remoteIp, $lifetime);
+		self::assertCookie($uid, $assertHash, $remoteIp);
 	}
 
 	/**
-	 * Test two different set() of the cookie class (first set is invalid)
+	 * Test the set() method of the cookie class
 	 *
 	 * @dataProvider dataSet
 	 */
-	public function testDoubleSet($serverKey, $uid, $password, $privateKey, $assertHash, $remoteIp, $serverArray, $lifetime)
+	public function testDoubleSet($serverKey, $uid, $password, $privateKey, $assertHash, $remoteIp, $serverArray)
 	{
 		$this->baseUrl->shouldReceive('getSSLPolicy')->andReturn(true)->once();
 		$this->config->shouldReceive('get')->with('system', 'site_prvkey')->andReturn($serverKey)->once();
@@ -298,12 +265,10 @@ class CookieTest extends MockedTest
 		$cookie = new StaticCookie($this->config, $this->baseUrl, $serverArray);
 		self::assertInstanceOf(Cookie::class, $cookie);
 
-		// Invalid set, should get overwritten
+		$cookie->set('uid', $uid);
-		$cookie->set(-1, 'invalid', 'nothing', -234);
+		$cookie->set('hash', $assertHash);
 
-		$cookie->set($uid, $password, $privateKey, $lifetime);
+		self::assertCookie($uid, $assertHash, $remoteIp);
-
-		self::assertCookie($uid, $assertHash, $remoteIp, $lifetime);
 	}
 
 	/**