From e3680f5a938ecbca0e0093289c9a971e64c0ec1c Mon Sep 17 00:00:00 2001 From: Sven Reichel Date: Thu, 2 Oct 2025 06:13:57 +0200 Subject: [PATCH 1/2] Don't set cookie samesite to "none" for unsecured pages --- .../System/Config/Source/Cookie/Samesite.php | 12 +- app/code/core/Mage/Core/Model/Cookie.php | 19 ++- .../Core/Model/Session/Abstract/Varien.php | 15 +- tests/unit/Mage/Core/Model/CookieTest.php | 156 ++++++++++++++++++ 4 files changed, 190 insertions(+), 12 deletions(-) create mode 100644 tests/unit/Mage/Core/Model/CookieTest.php 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..48bd6c53f8d --- /dev/null +++ b/tests/unit/Mage/Core/Model/CookieTest.php @@ -0,0 +1,156 @@ +setStore(null)); + } + + /** + * @group Model + * @group test + */ + public function testGetStore(): void + { + self::assertInstanceOf(Mage_Core_Model_Store::class, self::$subject->getStore()); + } + + /** + * @group Model + * @group test + */ + public function testGetDomain(): void + { + self::assertFalse(self::$subject->getDomain()); + } + + /** + * @group Model + * @group test + */ + public function testGetConfigDomain(): void + { + self::assertIsString(self::$subject->getConfigDomain()); + } + + /** + * @group Model + * @group test + */ + public function testGetPath(): void + { + self::assertIsString(self::$subject->getPath()); + } + + /** + * @group Model + * @group test + */ + public function testGetLifetime(): void + { + self::assertIsNumeric(self::$subject->getLifetime()); + } + + /** + * @group Model + * @group test + */ + public function testSetLifetime(): void + { + self::assertInstanceOf(Subject::class, self::$subject->setLifetime(0)); + } + + /** + * @group Model + * @group test + */ + public function testGetHttponly(): void + { + self::assertIsBool(self::$subject->getHttponly()); + } + + /** + * @group Model + * @group test + */ + public function testGetSameSite(): void + { + self::assertIsString(self::$subject->getSameSite()); + } + + /** + * @group Model + * @group test + */ + public function testIsSecure(): void + { + self::assertIsBool(self::$subject->isSecure()); + } + + /** + * @group Model + * @group test + * @runInSeparateProcess + */ + public function testSet(): void + { + self::assertInstanceOf(Subject::class, self::$subject->set('test', 'value')); + } + + /** + * @group Model + * @group test + */ + public function testRenew(): void + { + self::assertInstanceOf(Subject::class, self::$subject->renew('test')); + } + + /** + * @group Model + * @group test + */ + public function testGet(): void + { + self::assertIsArray(self::$subject->get()); + } + + /** + * @group Model + * @group test + * @runInSeparateProcess + */ + public function testDelete(): void + { + self::assertInstanceOf(Subject::class, self::$subject->delete('test')); + } +} From 8fb5de81ba3b37c6404bacfab9efd4d51a03d726 Mon Sep 17 00:00:00 2001 From: Sven Reichel Date: Thu, 2 Oct 2025 09:25:11 +0200 Subject: [PATCH 2/2] updated tests --- tests/unit/Mage/Core/Model/CookieTest.php | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/tests/unit/Mage/Core/Model/CookieTest.php b/tests/unit/Mage/Core/Model/CookieTest.php index 48bd6c53f8d..90c013d8de9 100644 --- a/tests/unit/Mage/Core/Model/CookieTest.php +++ b/tests/unit/Mage/Core/Model/CookieTest.php @@ -28,7 +28,6 @@ public static function setUpBeforeClass(): void /** * @group Model - * @group test */ public function testSetStore(): void { @@ -37,7 +36,6 @@ public function testSetStore(): void /** * @group Model - * @group test */ public function testGetStore(): void { @@ -46,7 +44,6 @@ public function testGetStore(): void /** * @group Model - * @group test */ public function testGetDomain(): void { @@ -55,7 +52,6 @@ public function testGetDomain(): void /** * @group Model - * @group test */ public function testGetConfigDomain(): void { @@ -64,7 +60,6 @@ public function testGetConfigDomain(): void /** * @group Model - * @group test */ public function testGetPath(): void { @@ -73,7 +68,6 @@ public function testGetPath(): void /** * @group Model - * @group test */ public function testGetLifetime(): void { @@ -82,7 +76,6 @@ public function testGetLifetime(): void /** * @group Model - * @group test */ public function testSetLifetime(): void { @@ -91,7 +84,6 @@ public function testSetLifetime(): void /** * @group Model - * @group test */ public function testGetHttponly(): void { @@ -100,7 +92,6 @@ public function testGetHttponly(): void /** * @group Model - * @group test */ public function testGetSameSite(): void { @@ -109,7 +100,6 @@ public function testGetSameSite(): void /** * @group Model - * @group test */ public function testIsSecure(): void { @@ -118,7 +108,6 @@ public function testIsSecure(): void /** * @group Model - * @group test * @runInSeparateProcess */ public function testSet(): void @@ -128,7 +117,6 @@ public function testSet(): void /** * @group Model - * @group test */ public function testRenew(): void { @@ -137,7 +125,6 @@ public function testRenew(): void /** * @group Model - * @group test */ public function testGet(): void { @@ -146,7 +133,6 @@ public function testGet(): void /** * @group Model - * @group test * @runInSeparateProcess */ public function testDelete(): void