diff --git a/app/code/core/Mage/Adminhtml/Model/System/Config/Source/Cookie/Samesite.php b/app/code/core/Mage/Adminhtml/Model/System/Config/Source/Cookie/Samesite.php index bfb4c575369..20dd2fe7659 100644 --- a/app/code/core/Mage/Adminhtml/Model/System/Config/Source/Cookie/Samesite.php +++ b/app/code/core/Mage/Adminhtml/Model/System/Config/Source/Cookie/Samesite.php @@ -12,15 +12,21 @@ */ class Mage_Adminhtml_Model_System_Config_Source_Cookie_Samesite { + public const NONE = 'None'; + + public const STRICT = 'Strict'; + + public const LAX = 'Lax'; + /** * @return array[] */ public function toOptionArray(): array { return [ - ['value' => 'None', 'label' => Mage::helper('adminhtml')->__('None')], - ['value' => 'Strict', 'label' => Mage::helper('adminhtml')->__('Strict')], - ['value' => 'Lax', 'label' => Mage::helper('adminhtml')->__('Lax')], + ['value' => self::NONE, 'label' => Mage::helper('adminhtml')->__('None')], + ['value' => self::STRICT, 'label' => Mage::helper('adminhtml')->__('Strict')], + ['value' => self::LAX, 'label' => Mage::helper('adminhtml')->__('Lax')], ]; } } diff --git a/app/code/core/Mage/Core/Model/Cookie.php b/app/code/core/Mage/Core/Model/Cookie.php index 0d8f4e417d3..11d2faf70b8 100644 --- a/app/code/core/Mage/Core/Model/Cookie.php +++ b/app/code/core/Mage/Core/Model/Cookie.php @@ -7,6 +7,8 @@ * @package Mage_Core */ +use Mage_Adminhtml_Model_System_Config_Source_Cookie_Samesite as CookieSameSite; + /** * Core cookie model * @@ -115,7 +117,7 @@ public function getPath() /** * Retrieve cookie lifetime * - * @return int + * @return int|string */ public function getLifetime() { @@ -163,7 +165,7 @@ public function getSameSite(): string { $sameSite = Mage::getStoreConfig(self::XML_PATH_COOKIE_SAMESITE, $this->getStore()); if (is_null($sameSite)) { - return 'None'; + return CookieSameSite::NONE; } return (string) $sameSite; } @@ -173,6 +175,7 @@ public function getSameSite(): string * Use secure on adminhtml only * * @return bool + * @throws Mage_Core_Exception */ public function isSecure() { @@ -180,7 +183,7 @@ public function isSecure() return $this->_getRequest()->isSecure(); } // Use secure cookie if unsecure base url is actually secure - if (preg_match('/^https:/', $this->getStore()->getBaseUrl(Mage_Core_Model_Store::URL_TYPE_LINK, false))) { + if (str_starts_with($this->getStore()->getBaseUrl(Mage_Core_Model_Store::URL_TYPE_LINK, false), 'https:')) { return true; } return false; @@ -198,6 +201,8 @@ public function isSecure() * @param bool $httponly * @param string $sameSite * @return $this + * @throws Zend_Controller_Response_Exception + * @throws Mage_Core_Exception */ public function set($name, $value, $period = null, $path = null, $domain = null, $secure = null, $httponly = null, $sameSite = null) { @@ -235,7 +240,7 @@ public function set($name, $value, $period = null, $path = null, $domain = null, $sameSite = $this->getSameSite(); } - if ($sameSite === 'None') { + if ($sameSite === CookieSameSite::NONE) { // Enforce specification SameSite None requires secure $secure = true; } @@ -267,6 +272,8 @@ public function set($name, $value, $period = null, $path = null, $domain = null, * @param bool $httponly * @param string $sameSite * @return $this + * @throws Zend_Controller_Response_Exception + * @throws Mage_Core_Exception */ public function renew($name, $period = null, $path = null, $domain = null, $secure = null, $httponly = null, $sameSite = null) { @@ -284,7 +291,7 @@ public function renew($name, $period = null, $path = null, $domain = null, $secu * Retrieve cookie or false if not exists * * @param string $name The cookie name - * @return mixed + * @return mixed|false */ public function get($name = null) { @@ -301,6 +308,8 @@ public function get($name = null) * @param int|bool $httponly * @param string $sameSite * @return $this + * @throws Zend_Controller_Response_Exception + * @throws Mage_Core_Exception */ public function delete($name, $path = null, $domain = null, $secure = null, $httponly = null, $sameSite = null) { diff --git a/app/code/core/Mage/Core/Model/Session/Abstract/Varien.php b/app/code/core/Mage/Core/Model/Session/Abstract/Varien.php index 12fe0a0ea7f..55582c323ea 100644 --- a/app/code/core/Mage/Core/Model/Session/Abstract/Varien.php +++ b/app/code/core/Mage/Core/Model/Session/Abstract/Varien.php @@ -7,6 +7,8 @@ * @package Mage_Core */ +use Mage_Adminhtml_Model_System_Config_Source_Cookie_Samesite as CookieSamesite; + /** * @package Mage_Core * @@ -109,6 +111,11 @@ public function start($sessionName = null) 'samesite' => $cookie->getSameSite(), ]; + if (!$cookie->isSecure() && $cookieParams['samesite'] === CookieSamesite::NONE) { + // PHP doesn't allow to set SameSite=None for non-secure cookies + unset($cookieParams['samesite']); + } + if (!$cookieParams['httponly']) { unset($cookieParams['httponly']); if (!$cookieParams['secure']) { @@ -129,12 +136,12 @@ public function start($sessionName = null) $this->setSessionName($sessionName); // Migrate old cookie from 'frontend' - if ($sessionName === \Mage_Core_Controller_Front_Action::SESSION_NAMESPACE + if ($sessionName === Mage_Core_Controller_Front_Action::SESSION_NAMESPACE && $cookie->get('frontend') - && ! $cookie->get(\Mage_Core_Controller_Front_Action::SESSION_NAMESPACE) + && ! $cookie->get(Mage_Core_Controller_Front_Action::SESSION_NAMESPACE) ) { $frontendValue = $cookie->get('frontend'); - $_COOKIE[\Mage_Core_Controller_Front_Action::SESSION_NAMESPACE] = $frontendValue; + $_COOKIE[Mage_Core_Controller_Front_Action::SESSION_NAMESPACE] = $frontendValue; $cookie->set(Mage_Core_Controller_Front_Action::SESSION_NAMESPACE, $frontendValue); $cookie->delete('frontend'); } @@ -172,7 +179,7 @@ public function start($sessionName = null) // Migrate old cookie from 'frontend' if (!$cookieValue - && $sessionName === \Mage_Core_Controller_Front_Action::SESSION_NAMESPACE + && $sessionName === Mage_Core_Controller_Front_Action::SESSION_NAMESPACE && $cookie->get('frontend_cid') ) { $cookieValue = $cookie->get('frontend_cid'); diff --git a/tests/unit/Mage/Core/Model/CookieTest.php b/tests/unit/Mage/Core/Model/CookieTest.php new file mode 100644 index 00000000000..90c013d8de9 --- /dev/null +++ b/tests/unit/Mage/Core/Model/CookieTest.php @@ -0,0 +1,142 @@ +setStore(null)); + } + + /** + * @group Model + */ + public function testGetStore(): void + { + self::assertInstanceOf(Mage_Core_Model_Store::class, self::$subject->getStore()); + } + + /** + * @group Model + */ + public function testGetDomain(): void + { + self::assertFalse(self::$subject->getDomain()); + } + + /** + * @group Model + */ + public function testGetConfigDomain(): void + { + self::assertIsString(self::$subject->getConfigDomain()); + } + + /** + * @group Model + */ + public function testGetPath(): void + { + self::assertIsString(self::$subject->getPath()); + } + + /** + * @group Model + */ + public function testGetLifetime(): void + { + self::assertIsNumeric(self::$subject->getLifetime()); + } + + /** + * @group Model + */ + public function testSetLifetime(): void + { + self::assertInstanceOf(Subject::class, self::$subject->setLifetime(0)); + } + + /** + * @group Model + */ + public function testGetHttponly(): void + { + self::assertIsBool(self::$subject->getHttponly()); + } + + /** + * @group Model + */ + public function testGetSameSite(): void + { + self::assertIsString(self::$subject->getSameSite()); + } + + /** + * @group Model + */ + public function testIsSecure(): void + { + self::assertIsBool(self::$subject->isSecure()); + } + + /** + * @group Model + * @runInSeparateProcess + */ + public function testSet(): void + { + self::assertInstanceOf(Subject::class, self::$subject->set('test', 'value')); + } + + /** + * @group Model + */ + public function testRenew(): void + { + self::assertInstanceOf(Subject::class, self::$subject->renew('test')); + } + + /** + * @group Model + */ + public function testGet(): void + { + self::assertIsArray(self::$subject->get()); + } + + /** + * @group Model + * @runInSeparateProcess + */ + public function testDelete(): void + { + self::assertInstanceOf(Subject::class, self::$subject->delete('test')); + } +}