From 45c19a0bf804be4fc05b7df98283e94ad58c00de Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Wed, 5 Apr 2023 15:26:09 +0100 Subject: [PATCH 01/11] Unified CSRF configuration --- .../Mage/Adminhtml/Block/Checkout/Formkey.php | 2 +- .../controllers/MultishippingController.php | 6 +++--- .../controllers/OnepageController.php | 10 +++++----- app/code/core/Mage/Checkout/etc/system.xml | 18 ------------------ .../sql/checkout_setup/install-1.6.0.0.php | 8 -------- .../Mage/Core/Controller/Front/Action.php | 6 ++++-- .../controllers/SubscriberController.php | 15 --------------- app/code/core/Mage/Newsletter/etc/config.xml | 3 --- app/code/core/Mage/Newsletter/etc/system.xml | 19 ------------------- 9 files changed, 13 insertions(+), 74 deletions(-) diff --git a/app/code/core/Mage/Adminhtml/Block/Checkout/Formkey.php b/app/code/core/Mage/Adminhtml/Block/Checkout/Formkey.php index dab7433190b..346950421f8 100644 --- a/app/code/core/Mage/Adminhtml/Block/Checkout/Formkey.php +++ b/app/code/core/Mage/Adminhtml/Block/Checkout/Formkey.php @@ -30,7 +30,7 @@ class Mage_Adminhtml_Block_Checkout_Formkey extends Mage_Adminhtml_Block_Templat */ public function canShow() { - return !Mage::getStoreConfigFlag('admin/security/validate_formkey_checkout'); + return !Mage_Core_Controller_Front_Action::_isFormKeyEnabled(); } /** diff --git a/app/code/core/Mage/Checkout/controllers/MultishippingController.php b/app/code/core/Mage/Checkout/controllers/MultishippingController.php index 429efc71e1e..f56638ea99b 100644 --- a/app/code/core/Mage/Checkout/controllers/MultishippingController.php +++ b/app/code/core/Mage/Checkout/controllers/MultishippingController.php @@ -228,7 +228,7 @@ public function addressesPostAction() return; } - if ($this->isFormkeyValidationOnCheckoutEnabled() && !$this->_validateFormKey()) { + if (!$this->_validateFormKey()) { $this->_redirect('*/*/addresses'); return; } @@ -349,7 +349,7 @@ public function backToShippingAction() */ public function shippingPostAction() { - if ($this->isFormkeyValidationOnCheckoutEnabled() && !$this->_validateFormKey()) { + if (!$this->_validateFormKey()) { $this->_redirect('*/*/shipping'); return; } @@ -462,7 +462,7 @@ public function overviewAction() return $this; } - if ($this->isFormkeyValidationOnCheckoutEnabled() && !$this->_validateFormKey()) { + if (!$this->_validateFormKey()) { $this->_redirect('*/*/billing'); return; } diff --git a/app/code/core/Mage/Checkout/controllers/OnepageController.php b/app/code/core/Mage/Checkout/controllers/OnepageController.php index b832650819f..2cbc8cbb615 100644 --- a/app/code/core/Mage/Checkout/controllers/OnepageController.php +++ b/app/code/core/Mage/Checkout/controllers/OnepageController.php @@ -355,7 +355,7 @@ public function saveBillingAction() return; } - if ($this->isFormkeyValidationOnCheckoutEnabled() && !$this->_validateFormKey()) { + if (!$this->_validateFormKey()) { return; } @@ -402,7 +402,7 @@ public function saveShippingAction() return; } - if ($this->isFormkeyValidationOnCheckoutEnabled() && !$this->_validateFormKey()) { + if (!$this->_validateFormKey()) { return; } @@ -431,7 +431,7 @@ public function saveShippingMethodAction() return; } - if ($this->isFormkeyValidationOnCheckoutEnabled() && !$this->_validateFormKey()) { + if (!$this->_validateFormKey()) { return; } @@ -471,7 +471,7 @@ public function savePaymentAction() return; } - if ($this->isFormkeyValidationOnCheckoutEnabled() && !$this->_validateFormKey()) { + if (!$this->_validateFormKey()) { return; } @@ -554,7 +554,7 @@ protected function _initInvoice() */ public function saveOrderAction() { - if ($this->isFormkeyValidationOnCheckoutEnabled() && !$this->_validateFormKey()) { + if (!$this->_validateFormKey()) { $this->_redirect('*/*'); return; } diff --git a/app/code/core/Mage/Checkout/etc/system.xml b/app/code/core/Mage/Checkout/etc/system.xml index 2f3013325fc..051cac86745 100644 --- a/app/code/core/Mage/Checkout/etc/system.xml +++ b/app/code/core/Mage/Checkout/etc/system.xml @@ -215,23 +215,5 @@ - - - - - - - select - adminhtml/system_config_source_yesno - 4 - Important! Enabling this option means - that your custom templates used in checkout process contain form_key output. - Otherwise checkout may not work.]]> - 1 - - - - - diff --git a/app/code/core/Mage/Checkout/sql/checkout_setup/install-1.6.0.0.php b/app/code/core/Mage/Checkout/sql/checkout_setup/install-1.6.0.0.php index c145e105b31..c9facc53b91 100644 --- a/app/code/core/Mage/Checkout/sql/checkout_setup/install-1.6.0.0.php +++ b/app/code/core/Mage/Checkout/sql/checkout_setup/install-1.6.0.0.php @@ -779,12 +779,4 @@ } } -$setup->insert( - $this->getTable('core_config_data'), - [ - 'path' => 'admin/security/validate_formkey_checkout', - 'value' => '1' - ] -); - $installer->endSetup(); diff --git a/app/code/core/Mage/Core/Controller/Front/Action.php b/app/code/core/Mage/Core/Controller/Front/Action.php index 79e55bdc576..7ec964cf213 100644 --- a/app/code/core/Mage/Core/Controller/Front/Action.php +++ b/app/code/core/Mage/Core/Controller/Front/Action.php @@ -176,7 +176,7 @@ protected function _validateFormKey() * * @return bool */ - protected function _isFormKeyEnabled() + public static function _isFormKeyEnabled() { return Mage::getStoreConfigFlag(self::XML_CSRF_USE_FLAG_CONFIG_PATH); } @@ -184,10 +184,12 @@ protected function _isFormKeyEnabled() /** * Check if form_key validation enabled on checkout process * + * @deprecated + * @see _isFormKeyEnabled * @return bool */ protected function isFormkeyValidationOnCheckoutEnabled() { - return Mage::getStoreConfigFlag('admin/security/validate_formkey_checkout'); + return $this->_isFormKeyEnabled(); } } diff --git a/app/code/core/Mage/Newsletter/controllers/SubscriberController.php b/app/code/core/Mage/Newsletter/controllers/SubscriberController.php index 64c93a10af6..30b8ac1c535 100644 --- a/app/code/core/Mage/Newsletter/controllers/SubscriberController.php +++ b/app/code/core/Mage/Newsletter/controllers/SubscriberController.php @@ -22,11 +22,6 @@ */ class Mage_Newsletter_SubscriberController extends Mage_Core_Controller_Front_Action { - /** - * Use CSRF validation flag from newsletter config - */ - public const XML_CSRF_USE_FLAG_CONFIG_PATH = 'newsletter/security/enable_form_key'; - /** * New subscription action */ @@ -128,14 +123,4 @@ public function unsubscribeAction() } $this->_redirectReferer(); } - - /** - * Check if form key validation is enabled in newsletter config. - * - * @return bool - */ - protected function _isFormKeyEnabled() - { - return Mage::getStoreConfigFlag(self::XML_CSRF_USE_FLAG_CONFIG_PATH); - } } diff --git a/app/code/core/Mage/Newsletter/etc/config.xml b/app/code/core/Mage/Newsletter/etc/config.xml index f574dc6f9c9..27bcaddb1fd 100644 --- a/app/code/core/Mage/Newsletter/etc/config.xml +++ b/app/code/core/Mage/Newsletter/etc/config.xml @@ -185,9 +185,6 @@ 0 - - 0 - diff --git a/app/code/core/Mage/Newsletter/etc/system.xml b/app/code/core/Mage/Newsletter/etc/system.xml index 1e9b8a2d553..8e12185d399 100644 --- a/app/code/core/Mage/Newsletter/etc/system.xml +++ b/app/code/core/Mage/Newsletter/etc/system.xml @@ -105,25 +105,6 @@ - - - 1 - 1 - 1 - 1 - - - - select - adminhtml/system_config_source_yesno - 1 - 1 - 1 - 1 - Important! Enabling this option means that your custom templates used for newsletter subscription must contain form_key block output. Otherwise newsletter subscription will not work.]]> - - - From 4f61c2af9997bc73ca9a0e804ea1ed59a72fe0c4 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Thu, 6 Apr 2023 00:02:20 +0100 Subject: [PATCH 02/11] converted to helper method --- app/code/core/Mage/Core/Controller/Front/Action.php | 4 ++-- app/code/core/Mage/Core/Helper/Data.php | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/app/code/core/Mage/Core/Controller/Front/Action.php b/app/code/core/Mage/Core/Controller/Front/Action.php index 7ec964cf213..f16b88027fe 100644 --- a/app/code/core/Mage/Core/Controller/Front/Action.php +++ b/app/code/core/Mage/Core/Controller/Front/Action.php @@ -176,9 +176,9 @@ protected function _validateFormKey() * * @return bool */ - public static function _isFormKeyEnabled() + protected function _isFormKeyEnabled() { - return Mage::getStoreConfigFlag(self::XML_CSRF_USE_FLAG_CONFIG_PATH); + return Mage::helper('core')->isFormKeyEnabled(); } /** diff --git a/app/code/core/Mage/Core/Helper/Data.php b/app/code/core/Mage/Core/Helper/Data.php index 903fc7b1a56..8d544cc2bf6 100644 --- a/app/code/core/Mage/Core/Helper/Data.php +++ b/app/code/core/Mage/Core/Helper/Data.php @@ -1001,4 +1001,12 @@ public function unEscapeCSVData($data) } return $data; } + + /** + * @return bool + */ + public function isFormKeyEnabled() + { + return Mage::getStoreConfigFlag(Mage_Core_Controller_Front_Action::XML_CSRF_USE_FLAG_CONFIG_PATH); + } } From b1b7a62c1b8f0c98f0bca40cc2af9de9a422bc0c Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Thu, 6 Apr 2023 00:03:05 +0100 Subject: [PATCH 03/11] converted to helper method --- app/code/core/Mage/Adminhtml/Block/Checkout/Formkey.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/code/core/Mage/Adminhtml/Block/Checkout/Formkey.php b/app/code/core/Mage/Adminhtml/Block/Checkout/Formkey.php index 346950421f8..c001251b4b9 100644 --- a/app/code/core/Mage/Adminhtml/Block/Checkout/Formkey.php +++ b/app/code/core/Mage/Adminhtml/Block/Checkout/Formkey.php @@ -30,7 +30,7 @@ class Mage_Adminhtml_Block_Checkout_Formkey extends Mage_Adminhtml_Block_Templat */ public function canShow() { - return !Mage_Core_Controller_Front_Action::_isFormKeyEnabled(); + return !Mage::helper('core')->isFormKeyEnabled(); } /** From 387af189141d1e83c8fa19d6821a7471cb336577 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Thu, 6 Apr 2023 12:46:33 +0100 Subject: [PATCH 04/11] Removed configuration from readme --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index fff96bdb537..2956f721083 100644 --- a/README.md +++ b/README.md @@ -246,7 +246,6 @@ If you see SQL errors after upgrading please remember to check for this specific - `catalog/product_image/progressive_threshold` - `catalog/search/search_separator` - `dev/log/max_level` -- `newsletter/security/enable_form_key` - `sitemap/category/lastmod` - `sitemap/page/lastmod` - `sitemap/product/lastmod` From 6d7f369ed6a5fe34f00dc274cd31f6406ec52bfa Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Thu, 6 Apr 2023 12:47:59 +0100 Subject: [PATCH 05/11] removed translations --- app/locale/en_US/Mage_Checkout.csv | 1 - app/locale/en_US/Mage_Newsletter.csv | 1 - 2 files changed, 2 deletions(-) diff --git a/app/locale/en_US/Mage_Checkout.csv b/app/locale/en_US/Mage_Checkout.csv index aece1f29fac..b1f51ccb965 100644 --- a/app/locale/en_US/Mage_Checkout.csv +++ b/app/locale/en_US/Mage_Checkout.csv @@ -120,7 +120,6 @@ "Email Address","Email Address" "Empty Cart","Empty Cart" "Empty Shopping Cart Content Before","Empty Shopping Cart Content Before" -"Enable Form Key Validation On Checkout","Enable Form Key Validation On Checkout" "Enable Onepage Checkout","Enable Onepage Checkout" "Enable Terms and Conditions","Enable Terms and Conditions" "Enabled","Enabled" diff --git a/app/locale/en_US/Mage_Newsletter.csv b/app/locale/en_US/Mage_Newsletter.csv index d1e8841114e..fc258f2347a 100644 --- a/app/locale/en_US/Mage_Newsletter.csv +++ b/app/locale/en_US/Mage_Newsletter.csv @@ -89,7 +89,6 @@ "Save Newsletter","Save Newsletter" "Save Template","Save Template" "Save and Resume","Save and Resume" -"Security","Security" "Selected problem subscribers have been unsubscribed.","Selected problem subscribers have been unsubscribed." "Selected problems have been deleted.","Selected problems have been deleted." "Sender","Sender" From ca11e41b52e39e3ce46de98e99c41775d80d0e37 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Thu, 6 Apr 2023 13:05:53 +0100 Subject: [PATCH 06/11] removed translations --- app/locale/en_US/Mage_Newsletter.csv | 1 - 1 file changed, 1 deletion(-) diff --git a/app/locale/en_US/Mage_Newsletter.csv b/app/locale/en_US/Mage_Newsletter.csv index fc258f2347a..12e18bdcbe1 100644 --- a/app/locale/en_US/Mage_Newsletter.csv +++ b/app/locale/en_US/Mage_Newsletter.csv @@ -1,5 +1,4 @@ " Copy"," Copy" -"Important! Enabling this option means that your custom templates used for newsletter subscription must contain form_key block output. Otherwise newsletter subscription will not work.","Important! Enabling this option means that your custom templates used for newsletter subscription must contain form_key block output. Otherwise newsletter subscription will not work." "Action","Action" "Add New Template","Add New Template" "Add to Queue","Add to Queue" From 519f031d9bfc0ad8389e42ee4bab0e554f0729cb Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Fri, 7 Apr 2023 22:36:32 +0100 Subject: [PATCH 07/11] return data type --- app/code/core/Mage/Core/Helper/Data.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/code/core/Mage/Core/Helper/Data.php b/app/code/core/Mage/Core/Helper/Data.php index 8d544cc2bf6..c564fedc2e4 100644 --- a/app/code/core/Mage/Core/Helper/Data.php +++ b/app/code/core/Mage/Core/Helper/Data.php @@ -1002,10 +1002,7 @@ public function unEscapeCSVData($data) return $data; } - /** - * @return bool - */ - public function isFormKeyEnabled() + public function isFormKeyEnabled(): bool { return Mage::getStoreConfigFlag(Mage_Core_Controller_Front_Action::XML_CSRF_USE_FLAG_CONFIG_PATH); } From 11c67807f6b4cf1250be627954ae64388b447c35 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Fri, 7 Apr 2023 22:45:34 +0100 Subject: [PATCH 08/11] docbloc --- app/code/core/Mage/Core/Helper/Data.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/code/core/Mage/Core/Helper/Data.php b/app/code/core/Mage/Core/Helper/Data.php index c564fedc2e4..06e7025cd7a 100644 --- a/app/code/core/Mage/Core/Helper/Data.php +++ b/app/code/core/Mage/Core/Helper/Data.php @@ -1002,6 +1002,9 @@ public function unEscapeCSVData($data) return $data; } + /** + * @return bool + */ public function isFormKeyEnabled(): bool { return Mage::getStoreConfigFlag(Mage_Core_Controller_Front_Action::XML_CSRF_USE_FLAG_CONFIG_PATH); From 1dc24bf411b84c2046c961e19fc3f544eec55652 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Mon, 1 May 2023 19:22:48 +0200 Subject: [PATCH 09/11] changed label --- app/locale/en_US/Mage_Core.csv | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/locale/en_US/Mage_Core.csv b/app/locale/en_US/Mage_Core.csv index 4322d2f9f70..635445e55e5 100644 --- a/app/locale/en_US/Mage_Core.csv +++ b/app/locale/en_US/Mage_Core.csv @@ -50,7 +50,7 @@ "Before modifying the website code please make sure that it is not used in index.php.","Before modifying the website code please make sure that it is not used in index.php." "Block with name ""%s"" already exists","Block with name ""%s"" already exists" "Browser Capabilities Detection","Browser Capabilities Detection" -"CSRF protection","CSRF protection" +"CSRF protection","CSRF Protection" "CSS Settings","CSS Settings" "Cache Storage Management","Cache Storage Management" "Cache storage may contain additional data. Are you sure that you want flush it?","Cache storage may contain additional data. Are you sure that you want flush it?" From ff06f111b5d4f34f67c30e20c2522c14f38d4b58 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Mon, 1 May 2023 19:33:17 +0200 Subject: [PATCH 10/11] new warning message when setting is disabled --- app/code/core/Mage/Adminhtml/Block/Checkout/Formkey.php | 9 +++++++++ .../default/default/template/notification/formkey.phtml | 8 ++++---- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/app/code/core/Mage/Adminhtml/Block/Checkout/Formkey.php b/app/code/core/Mage/Adminhtml/Block/Checkout/Formkey.php index ae22f98e1a4..9148a4901fa 100644 --- a/app/code/core/Mage/Adminhtml/Block/Checkout/Formkey.php +++ b/app/code/core/Mage/Adminhtml/Block/Checkout/Formkey.php @@ -36,9 +36,18 @@ public function canShow() * Get url for edit Advanced -> Admin section * * @return string + * @deprecated */ public function getSecurityAdminUrl() { return Mage::helper("adminhtml")->getUrl('adminhtml/system_config/edit/section/admin'); } + + /** + * @return string + */ + public function getEnableCSRFUrl() + { + return Mage::helper("adminhtml")->getUrl('adminhtml/system_config/edit/section/system'); + } } diff --git a/app/design/adminhtml/default/default/template/notification/formkey.phtml b/app/design/adminhtml/default/default/template/notification/formkey.phtml index 0168a8e49be..94d8ea7df6f 100644 --- a/app/design/adminhtml/default/default/template/notification/formkey.phtml +++ b/app/design/adminhtml/default/default/template/notification/formkey.phtml @@ -20,9 +20,9 @@ canShow()): ?>
Important: - Formkey validation on checkout disabled. This may expose security risks. - We strongly recommend to Enable Form Key Validation On Checkout in - Admin / Security, - for protect your own checkout process. + Formkey validation is disabled. This may expose security risks. + We strongly recommend to enable in + Advanced / System, + to protect your checkout process, newsletter subscription, contact form and more.
From 204a6f4614fddded3b7566265783efb4a4344883 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Tue, 2 May 2023 10:48:49 +0200 Subject: [PATCH 11/11] suggestion by @addison74 --- .../default/default/template/notification/formkey.phtml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/design/adminhtml/default/default/template/notification/formkey.phtml b/app/design/adminhtml/default/default/template/notification/formkey.phtml index 94d8ea7df6f..33c42d1fd90 100644 --- a/app/design/adminhtml/default/default/template/notification/formkey.phtml +++ b/app/design/adminhtml/default/default/template/notification/formkey.phtml @@ -20,9 +20,9 @@ canShow()): ?>
Important: - Formkey validation is disabled. This may expose security risks. + Formkey validation is disabled. This may expose you to security risks. We strongly recommend to enable in - Advanced / System, - to protect your checkout process, newsletter subscription, contact form and more. + Advanced / System / CSRF Protection, + to protect your checkout, contact and newsletter forms.