From 000c05aac258f541cdbff9594d58f59f32c76667 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 16:29:54 +0000 Subject: [PATCH 1/6] Removed the admin secret key toggle so admin GET actions are always protected The admin/security/use_form_key toggle didn't trade one CSRF protection for another: turning it off dropped automatic GET protection and replaced it with a hand-maintained list of _setForcedFormKeyActions() / getUrlSecure() pairs that had already drifted out of sync (#1241). Admin urls now always carry the per-action secret key, so POST is always covered by the form key and GET by the secret key, and the whole opt-in list becomes dead code: - preDispatch() validates the form key on POST and the secret key otherwise, with no config branch - deleted _setForcedFormKeyActions(), _checkIsForcedFormKeyAction() and all ~56 controller call sites, most of which were the whole preDispatch override - deleted getUrlSecure()/getUrlSecureParams() and rewired their call sites to plain getUrl(), which now always mints the key - removed the system.xml field, its backend model, the config default and the isEnabledSecurityKeyUrl() helper; useSecretKey() now only reflects the per-url _nosecret opt-out (login flow, RSS) - upgrade script deletes stored admin/security/use_form_key rows - browser tests mint the secret key from the page's FORM_KEY via the new adminPathWithSecretKey() helper instead of disabling the protection Closes #1242 --- .phpstan.dist.baseline.neon | 6 - AGENTS.md | 4 +- .../core/Mage/Admin/Model/Redirectpolicy.php | 3 +- app/code/core/Mage/Admin/Model/Session.php | 21 +-- .../Block/Catalog/Category/Edit/Form.php | 2 +- .../Block/Catalog/Category/Tab/Attributes.php | 2 +- .../Adminhtml/Block/Catalog/Product/Edit.php | 2 +- .../Block/Newsletter/Template/Edit.php | 2 +- .../core/Mage/Adminhtml/Block/Review/Edit.php | 2 +- .../Block/Sales/Order/Creditmemo/View.php | 4 +- .../Block/Sales/Order/Invoice/View.php | 6 +- .../Block/Sales/Order/Shipment/View.php | 2 +- .../Mage/Adminhtml/Block/Sales/Order/View.php | 6 +- .../Adminhtml/Block/System/Design/Edit.php | 2 +- .../Block/System/Email/Template/Edit.php | 2 +- .../Adminhtml/Block/System/Store/Edit.php | 2 +- .../Mage/Adminhtml/Block/Urlrewrite/Edit.php | 2 +- .../Adminhtml/Block/Widget/Form/Container.php | 2 +- .../core/Mage/Adminhtml/Controller/Action.php | 39 +---- app/code/core/Mage/Adminhtml/Helper/Data.php | 11 -- .../Config/Backend/Admin/Usesecretkey.php | 19 --- app/code/core/Mage/Adminhtml/Model/Url.php | 27 +--- .../Api/OrphanedResourceController.php | 6 - .../controllers/Api/RoleController.php | 12 -- .../controllers/Api/UserController.php | 12 -- .../Catalog/CategoryController.php | 11 -- .../Catalog/Product/AttributeController.php | 1 - .../Catalog/Product/ReviewController.php | 12 -- .../Catalog/Product/SetController.php | 12 -- .../controllers/Catalog/ProductController.php | 12 -- .../controllers/Catalog/SearchController.php | 11 -- .../Checkout/AgreementController.php | 12 -- .../controllers/Cms/BlockController.php | 12 -- .../controllers/Cms/PageController.php | 12 -- .../controllers/Customer/GroupController.php | 12 -- .../controllers/CustomerController.php | 12 -- .../Newsletter/TemplateController.php | 11 -- .../Permissions/BlockController.php | 11 -- .../OrphanedResourceController.php | 6 - .../Permissions/RoleController.php | 12 -- .../Permissions/UserController.php | 12 -- .../controllers/Promo/CatalogController.php | 12 -- .../controllers/Promo/QuoteController.php | 12 -- .../Sales/Order/CreditmemoController.php | 7 - .../Sales/Order/InvoiceController.php | 12 -- .../Sales/Order/ShipmentController.php | 12 -- .../controllers/Sales/OrderController.php | 11 -- .../controllers/SitemapController.php | 12 -- .../System/Config/TestEmailController.php | 7 - .../controllers/System/DesignController.php | 12 -- .../System/Email/LogController.php | 7 - .../System/Email/TemplateController.php | 12 -- .../controllers/System/StoreController.php | 12 -- .../controllers/Tax/RuleController.php | 11 -- .../controllers/UrlrewriteController.php | 7 - .../Adminhtml/Api2/AttributeController.php | 12 -- .../Adminhtml/Api2/RoleController.php | 12 -- app/code/core/Mage/Core/Block/Abstract.php | 26 ---- app/code/core/Mage/Core/etc/config.xml | 3 +- app/code/core/Mage/Core/etc/system.xml | 9 -- .../sql/core_setup/upgrade-2.1.0-2.1.1.php | 23 +++ .../Block/Adminhtml/System/Tools/Cronjobs.php | 2 +- .../System/Tools/CronjobsController.php | 7 - .../Block/Adminhtml/Country/Grid.php | 2 +- .../Directory/Block/Adminhtml/Region/Grid.php | 2 +- .../Adminhtml/Directory/CountryController.php | 7 - .../Adminhtml/Directory/RegionController.php | 7 - .../Adminhtml/ProcessController.php | 7 - .../Adminhtml/Oauth/ConsumerController.php | 1 - .../Adminhtml/Widget/InstanceController.php | 12 -- .../Accessibilityscan/ScanController.php | 7 - .../Ai/controllers/Adminhtml/AiController.php | 10 -- .../Adminhtml/Apiplatform/RoleController.php | 7 - .../Adminhtml/Apiplatform/UserController.php | 7 - .../Adminhtml/Blog/CategoryController.php | 7 - .../Adminhtml/Blog/PostController.php | 7 - .../Cataloglinkrule/RuleController.php | 6 - .../Block/Adminhtml/Version/Grid.php | 2 +- .../Adminhtml/ContentversionController.php | 7 - .../Block/Adminhtml/Destination/Grid.php | 2 +- .../FeedManager/Block/Adminhtml/Feed/Edit.php | 2 +- .../Feedmanager/CategoryController.php | 7 - .../Feedmanager/DestinationController.php | 7 - .../Feedmanager/DynamicruleController.php | 7 - .../Adminhtml/Feedmanager/FeedController.php | 22 --- .../Adminhtml/GiftcardController.php | 10 -- .../Block/Adminhtml/Mediacleaner.php | 2 +- .../Mediacleaner/Grid/Renderer/Actions.php | 2 +- .../Adminhtml/MediacleanerController.php | 19 --- .../Queue/Block/Adminhtml/Message/View.php | 4 +- .../controllers/Adminhtml/QueueController.php | 7 - .../Block/Adminhtml/Request/View.php | 2 +- .../Adminhtml/Sales/RevocationController.php | 7 - .../template/contentversion/preview.phtml | 2 +- app/locale/en_US/Mage_Core.csv | 1 - .../Adminhtml/AdminUrlFormKeyTest.php | 142 +++++------------- tests/Browser/AdminQueryStringUrlsTest.php | 13 +- tests/Browser/AdminReindexDialogTest.php | 17 +-- .../Browser/AdminRelatedProductsGridTest.php | 14 +- tests/Browser/AdminStoreSwitcherTest.php | 13 +- tests/Pest.php | 32 ++++ 101 files changed, 145 insertions(+), 896 deletions(-) delete mode 100644 app/code/core/Mage/Adminhtml/Model/System/Config/Backend/Admin/Usesecretkey.php create mode 100644 app/code/core/Mage/Core/sql/core_setup/upgrade-2.1.0-2.1.1.php diff --git a/.phpstan.dist.baseline.neon b/.phpstan.dist.baseline.neon index 93a2ca8df1..4c173a43cc 100644 --- a/.phpstan.dist.baseline.neon +++ b/.phpstan.dist.baseline.neon @@ -3957,12 +3957,6 @@ parameters: count: 1 path: app/code/core/Mage/Adminhtml/Controller/Action.php - - - rawMessage: 'Method Mage_Adminhtml_Controller_Action::_setForcedFormKeyActions() has no return type specified.' - identifier: missingType.return - count: 1 - path: app/code/core/Mage/Adminhtml/Controller/Action.php - - rawMessage: Property Mage_Adminhtml_Controller_Action::$_usedModuleName has no type specified. identifier: missingType.property diff --git a/AGENTS.md b/AGENTS.md index df88a7091f..566346531d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -379,7 +379,9 @@ it('can process customer orders', function () { - **ALWAYS use `getParam()`** for request parameters in controllers; `getUserParam()` only checks route params and breaks query strings - Define `public const ADMIN_RESOURCE` in admin controllers for ACL -- Use `_setForcedFormKeyActions()` for state-changing actions (delete, save, etc.) +- Admin CSRF is automatic: POST validates the form key, GET validates the per-action secret key + every admin url carries. Use `$_publicActions` only for read-only endpoints that must be + reachable without a key; state-changing actions should be POST - Validate/sanitize user input at the model layer - Doctrine DBAL parameterized queries are automatic diff --git a/app/code/core/Mage/Admin/Model/Redirectpolicy.php b/app/code/core/Mage/Admin/Model/Redirectpolicy.php index 171c9e8788..143c8437c4 100644 --- a/app/code/core/Mage/Admin/Model/Redirectpolicy.php +++ b/app/code/core/Mage/Admin/Model/Redirectpolicy.php @@ -38,8 +38,7 @@ public function getRedirectUrl( if (empty($request)) { return null; } - $countRequiredParams = ($this->_urlModel->useSecretKey() - && $request->getParam(Mage_Adminhtml_Model_Url::SECRET_KEY_PARAM_NAME)) ? 1 : 0; + $countRequiredParams = $request->getParam(Mage_Adminhtml_Model_Url::SECRET_KEY_PARAM_NAME) ? 1 : 0; $countGetParams = count($request->getUserParams()) + count($request->getQuery()); return ($countGetParams > $countRequiredParams) ? diff --git a/app/code/core/Mage/Admin/Model/Session.php b/app/code/core/Mage/Admin/Model/Session.php index b44e4449e2..db3fd49408 100644 --- a/app/code/core/Mage/Admin/Model/Session.php +++ b/app/code/core/Mage/Admin/Model/Session.php @@ -160,9 +160,7 @@ public function login(#[\SensitiveParameter] string $username, #[\SensitiveParam if ($user->getId()) { $this->renewSession(); - if (Mage::getSingleton('adminhtml/url')->useSecretKey()) { - Mage::getSingleton('adminhtml/url')->renewSecretUrls(); - } + Mage::getSingleton('adminhtml/url')->renewSecretUrls(); $this->setIsFirstPageAfterLogin(true); $this->setUser($user); $this->setAcl(Mage::getResourceModel('admin/acl')->loadAcl()); @@ -170,7 +168,7 @@ public function login(#[\SensitiveParameter] string $username, #[\SensitiveParam Mage::getSingleton('adminhtml/session')->setLocale($backendLocale); } - $alternativeUrl = $this->_getRequestUri($request); + $alternativeUrl = $this->_getRequestUri(); $redirectUrl = $this->_urlPolicy->getRedirectUrl($user, $request, $alternativeUrl); if ($redirectUrl) { Mage::dispatchEvent('admin_session_user_login_success', ['user' => $user]); @@ -287,20 +285,13 @@ public function setIsFirstPageAfterLogin($value) } /** - * Custom REQUEST_URI logic + * The requested url rebuilt with a fresh secret key, for the post-login redirect * - * @param Mage_Core_Controller_Request_Http $request - * @return string|null + * @return string */ - protected function _getRequestUri($request = null) + protected function _getRequestUri() { - if (Mage::getSingleton('adminhtml/url')->useSecretKey()) { - return Mage::getSingleton('adminhtml/url')->getUrl('*/*/*', ['_current' => true]); - } - if ($request) { - return $request->getRequestUri(); - } - return null; + return Mage::getSingleton('adminhtml/url')->getUrl('*/*/*', ['_current' => true]); } /** diff --git a/app/code/core/Mage/Adminhtml/Block/Catalog/Category/Edit/Form.php b/app/code/core/Mage/Adminhtml/Block/Catalog/Category/Edit/Form.php index 62e1891c72..62c171760a 100644 --- a/app/code/core/Mage/Adminhtml/Block/Catalog/Category/Edit/Form.php +++ b/app/code/core/Mage/Adminhtml/Block/Catalog/Category/Edit/Form.php @@ -203,7 +203,7 @@ public function getHeader() */ public function getDeleteUrl(array $args = []) { - return $this->getUrlSecure('*/*/delete', [ + return $this->getUrl('*/*/delete', [ '_current' => true, '_query' => false, ...$args, ]); } diff --git a/app/code/core/Mage/Adminhtml/Block/Catalog/Category/Tab/Attributes.php b/app/code/core/Mage/Adminhtml/Block/Catalog/Category/Tab/Attributes.php index 2efc2df0f1..03e31bbc90 100644 --- a/app/code/core/Mage/Adminhtml/Block/Catalog/Category/Tab/Attributes.php +++ b/app/code/core/Mage/Adminhtml/Block/Catalog/Category/Tab/Attributes.php @@ -159,7 +159,7 @@ protected function _prepareForm() if ($this->getCategory() && $this->getCategory()->getId()) { // Runs the rules synchronously in this request, evaluating every product in the catalog, // so confirm before starting rather than letting a large catalog look like a hung page - $refreshUrl = $this->getUrlSecure('*/*/processDynamic', [ + $refreshUrl = $this->getUrl('*/*/processDynamic', [ 'id' => $this->getCategory()->getId(), 'store' => $this->getRequest()->getParam('store'), ]); diff --git a/app/code/core/Mage/Adminhtml/Block/Catalog/Product/Edit.php b/app/code/core/Mage/Adminhtml/Block/Catalog/Product/Edit.php index e2156f0f0c..3397088ec0 100644 --- a/app/code/core/Mage/Adminhtml/Block/Catalog/Product/Edit.php +++ b/app/code/core/Mage/Adminhtml/Block/Catalog/Product/Edit.php @@ -214,7 +214,7 @@ public function getIsGrouped() */ public function getDeleteUrl() { - return $this->getUrlSecure('*/*/delete', ['_current' => true]); + return $this->getUrl('*/*/delete', ['_current' => true]); } /** diff --git a/app/code/core/Mage/Adminhtml/Block/Newsletter/Template/Edit.php b/app/code/core/Mage/Adminhtml/Block/Newsletter/Template/Edit.php index 1a5aeab781..8c39c455f6 100644 --- a/app/code/core/Mage/Adminhtml/Block/Newsletter/Template/Edit.php +++ b/app/code/core/Mage/Adminhtml/Block/Newsletter/Template/Edit.php @@ -296,7 +296,7 @@ public function isTextType() */ public function getDeleteUrl() { - return $this->getUrlSecure('*/*/delete', ['id' => $this->getRequest()->getParam('id')]); + return $this->getUrl('*/*/delete', ['id' => $this->getRequest()->getParam('id')]); } /** diff --git a/app/code/core/Mage/Adminhtml/Block/Review/Edit.php b/app/code/core/Mage/Adminhtml/Block/Review/Edit.php index 0f340b912f..02bb791c2a 100644 --- a/app/code/core/Mage/Adminhtml/Block/Review/Edit.php +++ b/app/code/core/Mage/Adminhtml/Block/Review/Edit.php @@ -55,7 +55,7 @@ public function __construct() 'delete', 'onclick', Mage::helper('core/js')->getDeleteConfirmJs( - $this->getUrlSecure('*/*/delete', [ + $this->getUrl('*/*/delete', [ $this->_objectId => $this->getRequest()->getParam($this->_objectId), 'ret' => 'pending', ]), diff --git a/app/code/core/Mage/Adminhtml/Block/Sales/Order/Creditmemo/View.php b/app/code/core/Mage/Adminhtml/Block/Sales/Order/Creditmemo/View.php index fa3eafc311..08123864bb 100644 --- a/app/code/core/Mage/Adminhtml/Block/Sales/Order/Creditmemo/View.php +++ b/app/code/core/Mage/Adminhtml/Block/Sales/Order/Creditmemo/View.php @@ -139,7 +139,7 @@ public function getCaptureUrl() */ public function getVoidUrl() { - return $this->getUrlSecure('*/*/void', ['creditmemo_id' => $this->getCreditmemo()->getId()]); + return $this->getUrl('*/*/void', ['creditmemo_id' => $this->getCreditmemo()->getId()]); } /** @@ -149,7 +149,7 @@ public function getVoidUrl() */ public function getCancelUrl() { - return $this->getUrlSecure('*/*/cancel', ['creditmemo_id' => $this->getCreditmemo()->getId()]); + return $this->getUrl('*/*/cancel', ['creditmemo_id' => $this->getCreditmemo()->getId()]); } /** diff --git a/app/code/core/Mage/Adminhtml/Block/Sales/Order/Invoice/View.php b/app/code/core/Mage/Adminhtml/Block/Sales/Order/Invoice/View.php index 8b56358752..52485717a0 100644 --- a/app/code/core/Mage/Adminhtml/Block/Sales/Order/Invoice/View.php +++ b/app/code/core/Mage/Adminhtml/Block/Sales/Order/Invoice/View.php @@ -149,7 +149,7 @@ public function getBackUrl() */ public function getCaptureUrl() { - return $this->getUrlSecure('*/*/capture', ['invoice_id' => $this->getInvoice()->getId()]); + return $this->getUrl('*/*/capture', ['invoice_id' => $this->getInvoice()->getId()]); } /** @@ -157,7 +157,7 @@ public function getCaptureUrl() */ public function getVoidUrl() { - return $this->getUrlSecure('*/*/void', ['invoice_id' => $this->getInvoice()->getId()]); + return $this->getUrl('*/*/void', ['invoice_id' => $this->getInvoice()->getId()]); } /** @@ -165,7 +165,7 @@ public function getVoidUrl() */ public function getCancelUrl() { - return $this->getUrlSecure('*/*/cancel', ['invoice_id' => $this->getInvoice()->getId()]); + return $this->getUrl('*/*/cancel', ['invoice_id' => $this->getInvoice()->getId()]); } /** diff --git a/app/code/core/Mage/Adminhtml/Block/Sales/Order/Shipment/View.php b/app/code/core/Mage/Adminhtml/Block/Sales/Order/Shipment/View.php index 95f44950b0..89e591c41c 100644 --- a/app/code/core/Mage/Adminhtml/Block/Sales/Order/Shipment/View.php +++ b/app/code/core/Mage/Adminhtml/Block/Sales/Order/Shipment/View.php @@ -123,7 +123,7 @@ public function getPrintUrl() public function getCancelUrl(): string { - return $this->getUrlSecure('*/*/cancel', ['shipment_id' => $this->getShipment()->getId()]); + return $this->getUrl('*/*/cancel', ['shipment_id' => $this->getShipment()->getId()]); } /** diff --git a/app/code/core/Mage/Adminhtml/Block/Sales/Order/View.php b/app/code/core/Mage/Adminhtml/Block/Sales/Order/View.php index 00aae71a3d..a2a54a4379 100644 --- a/app/code/core/Mage/Adminhtml/Block/Sales/Order/View.php +++ b/app/code/core/Mage/Adminhtml/Block/Sales/Order/View.php @@ -260,7 +260,7 @@ public function getEmailUrl() */ public function getCancelUrl() { - return $this->getUrlSecure('*/*/cancel'); + return $this->getUrl('*/*/cancel'); } /** @@ -284,7 +284,7 @@ public function getCreditmemoUrl() */ public function getHoldUrl() { - return $this->getUrlSecure('*/*/hold'); + return $this->getUrl('*/*/hold'); } /** @@ -292,7 +292,7 @@ public function getHoldUrl() */ public function getUnholdUrl() { - return $this->getUrlSecure('*/*/unhold'); + return $this->getUrl('*/*/unhold'); } /** diff --git a/app/code/core/Mage/Adminhtml/Block/System/Design/Edit.php b/app/code/core/Mage/Adminhtml/Block/System/Design/Edit.php index 52f14c8ce7..48f1b950e0 100644 --- a/app/code/core/Mage/Adminhtml/Block/System/Design/Edit.php +++ b/app/code/core/Mage/Adminhtml/Block/System/Design/Edit.php @@ -63,7 +63,7 @@ public function getDesignChangeId() */ public function getDeleteUrl() { - return $this->getUrlSecure('*/*/delete', ['id' => $this->getDesignChangeId()]); + return $this->getUrl('*/*/delete', ['id' => $this->getDesignChangeId()]); } /** diff --git a/app/code/core/Mage/Adminhtml/Block/System/Email/Template/Edit.php b/app/code/core/Mage/Adminhtml/Block/System/Email/Template/Edit.php index beb523869f..2903fe6e02 100644 --- a/app/code/core/Mage/Adminhtml/Block/System/Email/Template/Edit.php +++ b/app/code/core/Mage/Adminhtml/Block/System/Email/Template/Edit.php @@ -262,7 +262,7 @@ public function isTextType() */ public function getDeleteUrl() { - return $this->getUrlSecure('*/*/delete', ['_current' => true]); + return $this->getUrl('*/*/delete', ['_current' => true]); } /** diff --git a/app/code/core/Mage/Adminhtml/Block/System/Store/Edit.php b/app/code/core/Mage/Adminhtml/Block/System/Store/Edit.php index afc6484285..4c3f2842b5 100644 --- a/app/code/core/Mage/Adminhtml/Block/System/Store/Edit.php +++ b/app/code/core/Mage/Adminhtml/Block/System/Store/Edit.php @@ -89,7 +89,7 @@ public function _getDeleteUrl($storeType, $backupAvailable = false) if ($backupAvailable) { $deleteUrl = $this->getUrl('*/*/delete' . $storeType, ['item_id' => Mage::registry('store_data')->getId()]); } else { - $deleteUrl = $this->getUrlSecure( + $deleteUrl = $this->getUrl( '*/*/delete' . $storeType . 'Post', ['item_id' => Mage::registry('store_data')->getId()], ); diff --git a/app/code/core/Mage/Adminhtml/Block/Urlrewrite/Edit.php b/app/code/core/Mage/Adminhtml/Block/Urlrewrite/Edit.php index 734ed4270b..b16e086637 100644 --- a/app/code/core/Mage/Adminhtml/Block/Urlrewrite/Edit.php +++ b/app/code/core/Mage/Adminhtml/Block/Urlrewrite/Edit.php @@ -133,7 +133,7 @@ protected function _setFormChild() Mage::helper('adminhtml')->__('Are you sure you want to do this?'), ) . '\', \'' - . $this->getUrlSecure('*/*/delete', ['id' => $this->getUrlrewriteId()]) + . $this->getUrl('*/*/delete', ['id' => $this->getUrlrewriteId()]) . '\')', 'class' => 'scalable delete', 'level' => -1, diff --git a/app/code/core/Mage/Adminhtml/Block/Widget/Form/Container.php b/app/code/core/Mage/Adminhtml/Block/Widget/Form/Container.php index c251af41d8..95a7a16086 100644 --- a/app/code/core/Mage/Adminhtml/Block/Widget/Form/Container.php +++ b/app/code/core/Mage/Adminhtml/Block/Widget/Form/Container.php @@ -135,7 +135,7 @@ public function getBackUrl() */ public function getDeleteUrl() { - return $this->getUrlSecure('*/*/delete', [ + return $this->getUrl('*/*/delete', [ $this->_objectId => $this->getRequest()->getParam($this->_objectId), ]); } diff --git a/app/code/core/Mage/Adminhtml/Controller/Action.php b/app/code/core/Mage/Adminhtml/Controller/Action.php index 9f6f2cfddd..3a0a53fcab 100644 --- a/app/code/core/Mage/Adminhtml/Controller/Action.php +++ b/app/code/core/Mage/Adminhtml/Controller/Action.php @@ -38,13 +38,6 @@ class Mage_Adminhtml_Controller_Action extends Mage_Core_Controller_Varien_Actio */ protected $_publicActions = []; - /** - *Array of actions which can't be processed without form key validation - * - * @var array - */ - protected $_forcedFormKeyActions = []; - /** * Used module name in current adminhtml controller */ @@ -169,10 +162,10 @@ public function preDispatch() $isValidSecretKey = true; $keyErrorMsg = ''; if (Mage::getSingleton('admin/session')->isLoggedIn()) { - if ($this->getRequest()->isPost() || $this->_checkIsForcedFormKeyAction()) { + if ($this->getRequest()->isPost()) { $isValidFormKey = $this->_validateFormKey(); $keyErrorMsg = Mage::helper('adminhtml')->__('Invalid Form Key. Please refresh the page.'); - } elseif (Mage::getSingleton('adminhtml/url')->useSecretKey()) { + } else { $isValidSecretKey = $this->_validateSecretKey(); $keyErrorMsg = Mage::helper('adminhtml')->__('Invalid Secret Key. Please refresh the page.'); } @@ -362,34 +355,6 @@ protected function _validateCurrentPassword(#[\SensitiveParameter] $password) return $user->validateCurrentPassword($password); } - /** - * Check forced use form key for action - * - * @return bool - */ - protected function _checkIsForcedFormKeyAction() - { - return in_array( - strtolower($this->getRequest()->getActionName()), - array_map('strtolower', $this->_forcedFormKeyActions), - ); - } - - /** - * Set actions name for forced use form key if "Secret Key to URLs" disabled - * - * @param array | string $actionNames - action names for forced use form key - */ - protected function _setForcedFormKeyActions($actionNames) - { - if (!Mage::helper('adminhtml')->isEnabledSecurityKeyUrl()) { - $actionNames = (is_array($actionNames)) ? $actionNames : (array) $actionNames; - $actionNames = array_merge($this->_forcedFormKeyActions, $actionNames); - $actionNames = array_unique($actionNames); - $this->_forcedFormKeyActions = $actionNames; - } - } - /** * Validate request parameter * diff --git a/app/code/core/Mage/Adminhtml/Helper/Data.php b/app/code/core/Mage/Adminhtml/Helper/Data.php index 0a350dca81..9460b8a110 100644 --- a/app/code/core/Mage/Adminhtml/Helper/Data.php +++ b/app/code/core/Mage/Adminhtml/Helper/Data.php @@ -14,7 +14,6 @@ class Mage_Adminhtml_Helper_Data extends Mage_Adminhtml_Helper_Help_Mapping public const XML_PATH_USE_CUSTOM_ADMIN_URL = 'default/admin/url/use_custom'; public const XML_PATH_USE_CUSTOM_ADMIN_PATH = 'default/admin/url/use_custom_path'; public const XML_PATH_CUSTOM_ADMIN_PATH = 'default/admin/url/custom_path'; - public const XML_PATH_ADMINHTML_SECURITY_USE_FORM_KEY = 'admin/security/use_form_key'; protected $_moduleName = 'Mage_Adminhtml'; @@ -71,16 +70,6 @@ public function decodeFilter(&$value) $value = trim(rawurldecode($value)); } - /** - * Check if enabled "Add Secret Key to URLs" functionality - * - * @return bool - */ - public function isEnabledSecurityKeyUrl() - { - return Mage::getStoreConfigFlag(self::XML_PATH_ADMINHTML_SECURITY_USE_FORM_KEY); - } - /** * Check if url starts with one of the admin front names */ diff --git a/app/code/core/Mage/Adminhtml/Model/System/Config/Backend/Admin/Usesecretkey.php b/app/code/core/Mage/Adminhtml/Model/System/Config/Backend/Admin/Usesecretkey.php deleted file mode 100644 index 0aefd155ca..0000000000 --- a/app/code/core/Mage/Adminhtml/Model/System/Config/Backend/Admin/Usesecretkey.php +++ /dev/null @@ -1,19 +0,0 @@ - - * SPDX-FileCopyrightText: 2022-2024 The OpenMage Contributors - * SPDX-FileCopyrightText: 2006-2020 Magento, Inc. - * SPDX-License-Identifier: OSL-3.0 - * @package Mage_Adminhtml - */ - -class Mage_Adminhtml_Model_System_Config_Backend_Admin_Usesecretkey extends Mage_Core_Model_Config_Data -{ - #[\Override] - protected function _afterSave() - { - Mage::getSingleton('adminhtml/url')->renewSecretUrls(); - return $this; - } -} diff --git a/app/code/core/Mage/Adminhtml/Model/Url.php b/app/code/core/Mage/Adminhtml/Model/Url.php index ebb9179264..dd7b707e3c 100644 --- a/app/code/core/Mage/Adminhtml/Model/Url.php +++ b/app/code/core/Mage/Adminhtml/Model/Url.php @@ -134,35 +134,14 @@ public function getSecretKey($controller = null, $action = null) } /** - * Return secret key settings flag + * Whether the secret key is added to the url being built. Always on except for urls that + * opt out via _nosecret (login-flow links, RSS feeds). * * @return bool */ public function useSecretKey() { - return Mage::getStoreConfigFlag('admin/security/use_form_key') && !$this->getNoSecret(); - } - - /** - * Enable secret key using - * - * @return $this - */ - public function turnOnSecretKey() - { - $this->setNoSecret(false); - return $this; - } - - /** - * Disable secret key using - * - * @return $this - */ - public function turnOffSecretKey() - { - $this->setNoSecret(true); - return $this; + return !$this->getNoSecret(); } /** diff --git a/app/code/core/Mage/Adminhtml/controllers/Api/OrphanedResourceController.php b/app/code/core/Mage/Adminhtml/controllers/Api/OrphanedResourceController.php index f1d94eb0ba..5aa13aa575 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Api/OrphanedResourceController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Api/OrphanedResourceController.php @@ -69,10 +69,4 @@ public function massDeleteAction(): void $this->_redirect('*/*/'); } - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('massDelete'); - return parent::preDispatch(); - } } diff --git a/app/code/core/Mage/Adminhtml/controllers/Api/RoleController.php b/app/code/core/Mage/Adminhtml/controllers/Api/RoleController.php index 29162311f4..7ab6330f4a 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Api/RoleController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Api/RoleController.php @@ -16,18 +16,6 @@ class Mage_Adminhtml_Api_RoleController extends Mage_Adminhtml_Controller_Action */ public const ADMIN_RESOURCE = 'system/api/roles'; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['delete', 'save']); - return parent::preDispatch(); - } - protected function _initAction() { $this->loadLayout(); diff --git a/app/code/core/Mage/Adminhtml/controllers/Api/UserController.php b/app/code/core/Mage/Adminhtml/controllers/Api/UserController.php index cfabc78042..9e4476675c 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Api/UserController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Api/UserController.php @@ -16,18 +16,6 @@ class Mage_Adminhtml_Api_UserController extends Mage_Adminhtml_Controller_Action */ public const ADMIN_RESOURCE = 'system/api/users'; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - protected function _initAction() { $this->loadLayout() diff --git a/app/code/core/Mage/Adminhtml/controllers/Catalog/CategoryController.php b/app/code/core/Mage/Adminhtml/controllers/Catalog/CategoryController.php index 843a53db12..aa29298343 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Catalog/CategoryController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Catalog/CategoryController.php @@ -566,15 +566,4 @@ public function processDynamicAction(): void $this->_redirect('*/*/edit', ['id' => $categoryId]); } - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['delete', 'processDynamic']); - return parent::preDispatch(); - } } diff --git a/app/code/core/Mage/Adminhtml/controllers/Catalog/Product/AttributeController.php b/app/code/core/Mage/Adminhtml/controllers/Catalog/Product/AttributeController.php index 7644949e78..012db188b3 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Catalog/Product/AttributeController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Catalog/Product/AttributeController.php @@ -36,7 +36,6 @@ protected function _getAllowedTags() #[\Override] public function preDispatch() { - $this->_setForcedFormKeyActions('delete'); parent::preDispatch(); $this->_entityTypeId = Mage::getModel('eav/entity')->setType(Mage_Catalog_Model_Product::ENTITY)->getTypeId(); return $this; diff --git a/app/code/core/Mage/Adminhtml/controllers/Catalog/Product/ReviewController.php b/app/code/core/Mage/Adminhtml/controllers/Catalog/Product/ReviewController.php index 2cb4014ad7..12a93b3c63 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Catalog/Product/ReviewController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Catalog/Product/ReviewController.php @@ -17,18 +17,6 @@ class Mage_Adminhtml_Catalog_Product_ReviewController extends Mage_Adminhtml_Con */ protected $_publicActions = ['edit']; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['delete', 'massDelete']); - return parent::preDispatch(); - } - #[Maho\Config\Route('/admin/catalog_product_review/index')] public function indexAction() { diff --git a/app/code/core/Mage/Adminhtml/controllers/Catalog/Product/SetController.php b/app/code/core/Mage/Adminhtml/controllers/Catalog/Product/SetController.php index e117ddaf92..01cfbf56a1 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Catalog/Product/SetController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Catalog/Product/SetController.php @@ -190,18 +190,6 @@ public function deleteAction(): void } } - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - /** * Define in register catalog_product entity type code as entityType */ diff --git a/app/code/core/Mage/Adminhtml/controllers/Catalog/ProductController.php b/app/code/core/Mage/Adminhtml/controllers/Catalog/ProductController.php index 12a38856d1..fa7bdcbb07 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Catalog/ProductController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Catalog/ProductController.php @@ -28,18 +28,6 @@ class Mage_Adminhtml_Catalog_ProductController extends Mage_Adminhtml_Controller */ protected $_publicActions = ['edit']; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['delete', 'massDelete']); - return parent::preDispatch(); - } - #[\Override] protected function _construct() { diff --git a/app/code/core/Mage/Adminhtml/controllers/Catalog/SearchController.php b/app/code/core/Mage/Adminhtml/controllers/Catalog/SearchController.php index 1004d861fe..bae071e0c3 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Catalog/SearchController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Catalog/SearchController.php @@ -184,15 +184,4 @@ public function massDeleteAction(): void $this->_redirect('*/*/index'); } - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['delete', 'massDelete']); - return parent::preDispatch(); - } } diff --git a/app/code/core/Mage/Adminhtml/controllers/Checkout/AgreementController.php b/app/code/core/Mage/Adminhtml/controllers/Checkout/AgreementController.php index 97311a9e03..141e08b555 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Checkout/AgreementController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Checkout/AgreementController.php @@ -16,18 +16,6 @@ class Mage_Adminhtml_Checkout_AgreementController extends Mage_Adminhtml_Control */ public const ADMIN_RESOURCE = 'sales/checkoutagreement'; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - #[Maho\Config\Route('/admin/checkout_agreement/index')] public function indexAction() { diff --git a/app/code/core/Mage/Adminhtml/controllers/Cms/BlockController.php b/app/code/core/Mage/Adminhtml/controllers/Cms/BlockController.php index c2a0cc3dad..184021fbe0 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Cms/BlockController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Cms/BlockController.php @@ -16,18 +16,6 @@ class Mage_Adminhtml_Cms_BlockController extends Mage_Adminhtml_Controller_Actio */ public const ADMIN_RESOURCE = 'cms/block'; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - /** * Init actions * diff --git a/app/code/core/Mage/Adminhtml/controllers/Cms/PageController.php b/app/code/core/Mage/Adminhtml/controllers/Cms/PageController.php index e946c73b4a..7ee7d78e29 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Cms/PageController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Cms/PageController.php @@ -210,18 +210,6 @@ public function deleteAction(): void $this->_redirect('*/*/'); } - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - #[\Override] protected function _isAllowed() { diff --git a/app/code/core/Mage/Adminhtml/controllers/Customer/GroupController.php b/app/code/core/Mage/Adminhtml/controllers/Customer/GroupController.php index 55eeb7b097..decf2b99ab 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Customer/GroupController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Customer/GroupController.php @@ -16,18 +16,6 @@ class Mage_Adminhtml_Customer_GroupController extends Mage_Adminhtml_Controller_ */ public const ADMIN_RESOURCE = 'customer/group'; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - protected function _initGroup() { $this->_title($this->__('Customers'))->_title($this->__('Customer Groups')); diff --git a/app/code/core/Mage/Adminhtml/controllers/CustomerController.php b/app/code/core/Mage/Adminhtml/controllers/CustomerController.php index 16f71b7a4d..9c179c1499 100644 --- a/app/code/core/Mage/Adminhtml/controllers/CustomerController.php +++ b/app/code/core/Mage/Adminhtml/controllers/CustomerController.php @@ -16,18 +16,6 @@ class Mage_Adminhtml_CustomerController extends Mage_Adminhtml_Controller_Action */ public const ADMIN_RESOURCE = 'customer/manage'; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['delete', 'massDelete']); - return parent::preDispatch(); - } - /** * @param string $idFieldName * @return $this diff --git a/app/code/core/Mage/Adminhtml/controllers/Newsletter/TemplateController.php b/app/code/core/Mage/Adminhtml/controllers/Newsletter/TemplateController.php index 93928f2338..657c50a893 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Newsletter/TemplateController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Newsletter/TemplateController.php @@ -260,15 +260,4 @@ public function wysiwygVariableAction(): void ); } - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } } diff --git a/app/code/core/Mage/Adminhtml/controllers/Permissions/BlockController.php b/app/code/core/Mage/Adminhtml/controllers/Permissions/BlockController.php index f6953cf0cc..c4003d11e7 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Permissions/BlockController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Permissions/BlockController.php @@ -190,15 +190,4 @@ public function blockGridAction(): void ->toHtml()); } - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } } diff --git a/app/code/core/Mage/Adminhtml/controllers/Permissions/OrphanedResourceController.php b/app/code/core/Mage/Adminhtml/controllers/Permissions/OrphanedResourceController.php index 9dab5b41b4..afa6cc361b 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Permissions/OrphanedResourceController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Permissions/OrphanedResourceController.php @@ -70,10 +70,4 @@ public function massDeleteAction(): void $this->_redirect('*/*/'); } - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('massDelete'); - return parent::preDispatch(); - } } diff --git a/app/code/core/Mage/Adminhtml/controllers/Permissions/RoleController.php b/app/code/core/Mage/Adminhtml/controllers/Permissions/RoleController.php index b0464a990e..cc6802a944 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Permissions/RoleController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Permissions/RoleController.php @@ -16,18 +16,6 @@ class Mage_Adminhtml_Permissions_RoleController extends Mage_Adminhtml_Controlle */ public const ADMIN_RESOURCE = 'system/acl/roles'; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - /** * Preparing layout for output * diff --git a/app/code/core/Mage/Adminhtml/controllers/Permissions/UserController.php b/app/code/core/Mage/Adminhtml/controllers/Permissions/UserController.php index 313fd695d6..532b604cd6 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Permissions/UserController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Permissions/UserController.php @@ -16,18 +16,6 @@ class Mage_Adminhtml_Permissions_UserController extends Mage_Adminhtml_Controlle */ public const ADMIN_RESOURCE = 'system/acl/users'; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - protected function _initAction() { $this->loadLayout() diff --git a/app/code/core/Mage/Adminhtml/controllers/Promo/CatalogController.php b/app/code/core/Mage/Adminhtml/controllers/Promo/CatalogController.php index ea9ca6ee95..91a1a4901b 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Promo/CatalogController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Promo/CatalogController.php @@ -23,18 +23,6 @@ class Mage_Adminhtml_Promo_CatalogController extends Mage_Adminhtml_Controller_A */ protected $_dirtyRulesNoticeMessage; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - protected function _initAction() { $this->loadLayout() diff --git a/app/code/core/Mage/Adminhtml/controllers/Promo/QuoteController.php b/app/code/core/Mage/Adminhtml/controllers/Promo/QuoteController.php index 2b99feb315..58b999b328 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Promo/QuoteController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Promo/QuoteController.php @@ -16,18 +16,6 @@ class Mage_Adminhtml_Promo_QuoteController extends Mage_Adminhtml_Controller_Act */ public const ADMIN_RESOURCE = 'promo/quote'; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - protected function _initRule() { $this->_title($this->__('Promotions'))->_title($this->__('Shopping Cart Price Rules')); diff --git a/app/code/core/Mage/Adminhtml/controllers/Sales/Order/CreditmemoController.php b/app/code/core/Mage/Adminhtml/controllers/Sales/Order/CreditmemoController.php index ec7376d42c..4535051439 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Sales/Order/CreditmemoController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Sales/Order/CreditmemoController.php @@ -10,13 +10,6 @@ class Mage_Adminhtml_Sales_Order_CreditmemoController extends Mage_Adminhtml_Controller_Sales_Creditmemo { - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['cancel', 'void']); - return parent::preDispatch(); - } - /** * Get requested items qtys and return to stock flags */ diff --git a/app/code/core/Mage/Adminhtml/controllers/Sales/Order/InvoiceController.php b/app/code/core/Mage/Adminhtml/controllers/Sales/Order/InvoiceController.php index 54bf99002d..a5cf112fcc 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Sales/Order/InvoiceController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Sales/Order/InvoiceController.php @@ -10,18 +10,6 @@ class Mage_Adminhtml_Sales_Order_InvoiceController extends Mage_Adminhtml_Controller_Sales_Invoice { - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['capture', 'cancel', 'void']); - return parent::preDispatch(); - } - /** * Get requested items qty's from request */ diff --git a/app/code/core/Mage/Adminhtml/controllers/Sales/Order/ShipmentController.php b/app/code/core/Mage/Adminhtml/controllers/Sales/Order/ShipmentController.php index c2560d52d4..595f3d63aa 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Sales/Order/ShipmentController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Sales/Order/ShipmentController.php @@ -10,18 +10,6 @@ class Mage_Adminhtml_Sales_Order_ShipmentController extends Mage_Adminhtml_Controller_Sales_Shipment { - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['cancel']); - return parent::preDispatch(); - } - /** * Initialize shipment items QTY */ diff --git a/app/code/core/Mage/Adminhtml/controllers/Sales/OrderController.php b/app/code/core/Mage/Adminhtml/controllers/Sales/OrderController.php index 3f8019be86..f95e78c3b0 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Sales/OrderController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Sales/OrderController.php @@ -805,15 +805,4 @@ public function guestOrderEmailChangeAction(): void $this->_redirect('*/*/view', ['order_id' => $orderId]); } - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['cancel', 'massCancel', 'hold', 'unhold', 'guestOrderEmailChange']); - return parent::preDispatch(); - } } diff --git a/app/code/core/Mage/Adminhtml/controllers/SitemapController.php b/app/code/core/Mage/Adminhtml/controllers/SitemapController.php index bb2ece8e2f..81b219f2ae 100644 --- a/app/code/core/Mage/Adminhtml/controllers/SitemapController.php +++ b/app/code/core/Mage/Adminhtml/controllers/SitemapController.php @@ -21,18 +21,6 @@ class Mage_Adminhtml_SitemapController extends Mage_Adminhtml_Controller_Action */ public const MAXIMUM_SITEMAP_NAME_LENGTH = 32; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - /** * Init actions * diff --git a/app/code/core/Mage/Adminhtml/controllers/System/Config/TestEmailController.php b/app/code/core/Mage/Adminhtml/controllers/System/Config/TestEmailController.php index 964118d66f..5688eec664 100644 --- a/app/code/core/Mage/Adminhtml/controllers/System/Config/TestEmailController.php +++ b/app/code/core/Mage/Adminhtml/controllers/System/Config/TestEmailController.php @@ -16,13 +16,6 @@ class Mage_Adminhtml_System_Config_TestEmailController extends Mage_Adminhtml_Co */ public const ADMIN_RESOURCE = 'system/config'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('send'); - return parent::preDispatch(); - } - /** * Send test email action */ diff --git a/app/code/core/Mage/Adminhtml/controllers/System/DesignController.php b/app/code/core/Mage/Adminhtml/controllers/System/DesignController.php index 036de0a4ae..1efb64e1d5 100644 --- a/app/code/core/Mage/Adminhtml/controllers/System/DesignController.php +++ b/app/code/core/Mage/Adminhtml/controllers/System/DesignController.php @@ -16,18 +16,6 @@ class Mage_Adminhtml_System_DesignController extends Mage_Adminhtml_Controller_A */ public const ADMIN_RESOURCE = 'system/design'; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - #[Maho\Config\Route('/admin/system_design/index')] public function indexAction(): void { diff --git a/app/code/core/Mage/Adminhtml/controllers/System/Email/LogController.php b/app/code/core/Mage/Adminhtml/controllers/System/Email/LogController.php index d8ee7e38d9..cd42ced94d 100644 --- a/app/code/core/Mage/Adminhtml/controllers/System/Email/LogController.php +++ b/app/code/core/Mage/Adminhtml/controllers/System/Email/LogController.php @@ -12,13 +12,6 @@ class Mage_Adminhtml_System_Email_LogController extends Mage_Adminhtml_Controlle { public const ADMIN_RESOURCE = 'system/tools/email_log'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('massDelete'); - return parent::preDispatch(); - } - #[Maho\Config\Route('/admin/system_email_log/index')] public function indexAction(): void { diff --git a/app/code/core/Mage/Adminhtml/controllers/System/Email/TemplateController.php b/app/code/core/Mage/Adminhtml/controllers/System/Email/TemplateController.php index bd49948793..d3b0266c19 100644 --- a/app/code/core/Mage/Adminhtml/controllers/System/Email/TemplateController.php +++ b/app/code/core/Mage/Adminhtml/controllers/System/Email/TemplateController.php @@ -217,18 +217,6 @@ public function defaultTemplateAction(): void $this->getResponse()->setBodyJson($template); } - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - /** * Load email template from request * diff --git a/app/code/core/Mage/Adminhtml/controllers/System/StoreController.php b/app/code/core/Mage/Adminhtml/controllers/System/StoreController.php index 7be8b1fa27..2124006dea 100644 --- a/app/code/core/Mage/Adminhtml/controllers/System/StoreController.php +++ b/app/code/core/Mage/Adminhtml/controllers/System/StoreController.php @@ -16,18 +16,6 @@ class Mage_Adminhtml_System_StoreController extends Mage_Adminhtml_Controller_Ac */ public const ADMIN_RESOURCE = 'system/store'; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['deleteWebsitePost', 'deleteGroupPost', 'deleteStorePost']); - return parent::preDispatch(); - } - /** * Init actions * diff --git a/app/code/core/Mage/Adminhtml/controllers/Tax/RuleController.php b/app/code/core/Mage/Adminhtml/controllers/Tax/RuleController.php index c8a02b1c54..db9f07634d 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Tax/RuleController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Tax/RuleController.php @@ -213,15 +213,4 @@ protected function _initAction() return $this; } - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } } diff --git a/app/code/core/Mage/Adminhtml/controllers/UrlrewriteController.php b/app/code/core/Mage/Adminhtml/controllers/UrlrewriteController.php index 219782ab03..8fd02e7e6c 100644 --- a/app/code/core/Mage/Adminhtml/controllers/UrlrewriteController.php +++ b/app/code/core/Mage/Adminhtml/controllers/UrlrewriteController.php @@ -16,13 +16,6 @@ class Mage_Adminhtml_UrlrewriteController extends Mage_Adminhtml_Controller_Acti */ public const ADMIN_RESOURCE = 'catalog/urlrewrite'; - #[\Override] - public function preDispatch(): Mage_Adminhtml_Controller_Action - { - $this->_setForcedFormKeyActions(['delete', 'massDelete']); - return parent::preDispatch(); - } - /** * Instantiate urlrewrite, product and category * diff --git a/app/code/core/Mage/Api2/controllers/Adminhtml/Api2/AttributeController.php b/app/code/core/Mage/Api2/controllers/Adminhtml/Api2/AttributeController.php index eade290224..f5bb5080ad 100644 --- a/app/code/core/Mage/Api2/controllers/Adminhtml/Api2/AttributeController.php +++ b/app/code/core/Mage/Api2/controllers/Adminhtml/Api2/AttributeController.php @@ -19,18 +19,6 @@ class Mage_Api2_Adminhtml_Api2_AttributeController extends Mage_Adminhtml_Contro */ public const ADMIN_RESOURCE = 'system/api'; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['save']); - return parent::preDispatch(); - } - /** * Show user types grid */ diff --git a/app/code/core/Mage/Api2/controllers/Adminhtml/Api2/RoleController.php b/app/code/core/Mage/Api2/controllers/Adminhtml/Api2/RoleController.php index 05669a19b0..384bdfd34d 100644 --- a/app/code/core/Mage/Api2/controllers/Adminhtml/Api2/RoleController.php +++ b/app/code/core/Mage/Api2/controllers/Adminhtml/Api2/RoleController.php @@ -15,18 +15,6 @@ class Mage_Api2_Adminhtml_Api2_RoleController extends Mage_Adminhtml_Controller_ { public const ADMIN_RESOURCE = 'system/api/rest_roles'; - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['delete', 'save']); - return parent::preDispatch(); - } - /** * Show grid */ diff --git a/app/code/core/Mage/Core/Block/Abstract.php b/app/code/core/Mage/Core/Block/Abstract.php index 9fc351a627..61e8e081ab 100644 --- a/app/code/core/Mage/Core/Block/Abstract.php +++ b/app/code/core/Mage/Core/Block/Abstract.php @@ -988,32 +988,6 @@ public function getUrl($route = '', $params = []) return $this->_getUrlModel()->getUrl($route, $params); } - /** - * Generate security url by route and parameters (add form key if "Add Secret Key to URLs" disabled) - * - * @param string $route - * @param array $params - * - * @return string - */ - public function getUrlSecure($route = '', $params = []) - { - return $this->getUrl($route, $this->getUrlSecureParams($params)); - } - - /** - * Same rule as getUrlSecure() for callers that build the url themselves, e.g. grid action - * columns. The form key is the root the per-action secret key is derived from, so it may - * only travel in a url when it is itself the token being checked. - */ - public function getUrlSecureParams(array $params = []): array - { - if (!Mage::helper('adminhtml')->isEnabledSecurityKeyUrl()) { - $params[Mage_Core_Model_Url::FORM_KEY] = $this->getFormKey(); - } - return $params; - } - /** * Generate base64-encoded url by route and parameters * diff --git a/app/code/core/Mage/Core/etc/config.xml b/app/code/core/Mage/Core/etc/config.xml index 2dcf85be27..e92fa8ef9e 100644 --- a/app/code/core/Mage/Core/etc/config.xml +++ b/app/code/core/Mage/Core/etc/config.xml @@ -8,7 +8,7 @@ SPDX-License-Identifier: AFL-3.0 - 2.1.0 + 2.1.1 @@ -329,7 +329,6 @@ SPDX-License-Identifier: AFL-3.0 - 1 2 2 0 diff --git a/app/code/core/Mage/Core/etc/system.xml b/app/code/core/Mage/Core/etc/system.xml index c7eef7a1ce..095d087086 100644 --- a/app/code/core/Mage/Core/etc/system.xml +++ b/app/code/core/Mage/Core/etc/system.xml @@ -1195,15 +1195,6 @@ SPDX-License-Identifier: AFL-3.0 0 0 - - - boolean - adminhtml/system_config_backend_admin_usesecretkey - 1 - 1 - 0 - 0 - boolean diff --git a/app/code/core/Mage/Core/sql/core_setup/upgrade-2.1.0-2.1.1.php b/app/code/core/Mage/Core/sql/core_setup/upgrade-2.1.0-2.1.1.php new file mode 100644 index 0000000000..a9ea64c390 --- /dev/null +++ b/app/code/core/Mage/Core/sql/core_setup/upgrade-2.1.0-2.1.1.php @@ -0,0 +1,23 @@ + + * SPDX-License-Identifier: OSL-3.0 + * @package Mage_Core + */ + +/** @var Mage_Core_Model_Resource_Setup $this */ +$installer = $this; +$installer->startSetup(); + +// The "Add Secret Key to URLs" toggle is gone: admin urls always carry the secret key now, +// so any stored override (including a 0 that used to disable it) is dead configuration. +$connection = $installer->getConnection(); +$connection->delete( + $this->getTable('core_config_data'), + $connection->prepareSqlCondition('path', [ + 'like' => 'admin/security/use_form_key', + ]), +); + +$installer->endSetup(); diff --git a/app/code/core/Mage/Cron/Block/Adminhtml/System/Tools/Cronjobs.php b/app/code/core/Mage/Cron/Block/Adminhtml/System/Tools/Cronjobs.php index 3c59fa4ea6..3ee3ded8dd 100644 --- a/app/code/core/Mage/Cron/Block/Adminhtml/System/Tools/Cronjobs.php +++ b/app/code/core/Mage/Cron/Block/Adminhtml/System/Tools/Cronjobs.php @@ -18,7 +18,7 @@ public function __construct() $this->_addButton('clear_history', [ 'label' => Mage::helper('cron')->__('Clear History'), - 'onclick' => "setLocation('{$this->getUrlSecure('*/*/clearHistory')}')", + 'onclick' => "setLocation('{$this->getUrl('*/*/clearHistory')}')", 'class' => 'delete', ]); } diff --git a/app/code/core/Mage/Cron/controllers/Adminhtml/System/Tools/CronjobsController.php b/app/code/core/Mage/Cron/controllers/Adminhtml/System/Tools/CronjobsController.php index 1187013fe1..6a1d4571e7 100644 --- a/app/code/core/Mage/Cron/controllers/Adminhtml/System/Tools/CronjobsController.php +++ b/app/code/core/Mage/Cron/controllers/Adminhtml/System/Tools/CronjobsController.php @@ -12,13 +12,6 @@ class Mage_Cron_Adminhtml_System_Tools_CronjobsController extends Mage_Adminhtml { public const ADMIN_RESOURCE = 'system/tools/cronjobs'; - #[\Override] - public function preDispatch(): self - { - $this->_setForcedFormKeyActions(['clearHistory', 'massDisable', 'massEnable', 'run', 'toggle']); - return parent::preDispatch(); - } - #[Maho\Config\Route('/admin/system_tools_cronjobs/index')] public function indexAction(): void { diff --git a/app/code/core/Mage/Directory/Block/Adminhtml/Country/Grid.php b/app/code/core/Mage/Directory/Block/Adminhtml/Country/Grid.php index ba21b1853f..e8b4896622 100644 --- a/app/code/core/Mage/Directory/Block/Adminhtml/Country/Grid.php +++ b/app/code/core/Mage/Directory/Block/Adminhtml/Country/Grid.php @@ -76,7 +76,7 @@ protected function _prepareColumns(): self ], [ 'caption' => Mage::helper('adminhtml')->__('Delete'), - 'url' => ['base' => '*/*/delete', 'params' => $this->getUrlSecureParams()], + 'url' => ['base' => '*/*/delete'], 'field' => 'id', 'confirm' => Mage::helper('directory')->__('Are you sure you want to delete this country?'), ], diff --git a/app/code/core/Mage/Directory/Block/Adminhtml/Region/Grid.php b/app/code/core/Mage/Directory/Block/Adminhtml/Region/Grid.php index 57e0ba33a5..9185b6aba8 100644 --- a/app/code/core/Mage/Directory/Block/Adminhtml/Region/Grid.php +++ b/app/code/core/Mage/Directory/Block/Adminhtml/Region/Grid.php @@ -75,7 +75,7 @@ protected function _prepareColumns(): self ], [ 'caption' => Mage::helper('adminhtml')->__('Delete'), - 'url' => ['base' => '*/*/delete', 'params' => $this->getUrlSecureParams()], + 'url' => ['base' => '*/*/delete'], 'field' => 'id', 'confirm' => Mage::helper('directory')->__('Are you sure you want to delete this region?'), ], diff --git a/app/code/core/Mage/Directory/controllers/Adminhtml/Directory/CountryController.php b/app/code/core/Mage/Directory/controllers/Adminhtml/Directory/CountryController.php index 05500dd8a8..340c9d09b7 100644 --- a/app/code/core/Mage/Directory/controllers/Adminhtml/Directory/CountryController.php +++ b/app/code/core/Mage/Directory/controllers/Adminhtml/Directory/CountryController.php @@ -12,13 +12,6 @@ class Mage_Directory_Adminhtml_Directory_CountryController extends Mage_Adminhtm { public const ADMIN_RESOURCE = 'system/directory/countries'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - protected function _initCountry(): Mage_Directory_Model_Country|false { $id = $this->getRequest()->getParam('id'); diff --git a/app/code/core/Mage/Directory/controllers/Adminhtml/Directory/RegionController.php b/app/code/core/Mage/Directory/controllers/Adminhtml/Directory/RegionController.php index 2bae657fca..d2c456ac33 100644 --- a/app/code/core/Mage/Directory/controllers/Adminhtml/Directory/RegionController.php +++ b/app/code/core/Mage/Directory/controllers/Adminhtml/Directory/RegionController.php @@ -12,13 +12,6 @@ class Mage_Directory_Adminhtml_Directory_RegionController extends Mage_Adminhtml { public const ADMIN_RESOURCE = 'system/directory/regions'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - protected function _initRegion(): Mage_Directory_Model_Region|false { $id = $this->getRequest()->getParam('id'); diff --git a/app/code/core/Mage/Index/controllers/Adminhtml/ProcessController.php b/app/code/core/Mage/Index/controllers/Adminhtml/ProcessController.php index db1ddc07e4..f0ea101f48 100644 --- a/app/code/core/Mage/Index/controllers/Adminhtml/ProcessController.php +++ b/app/code/core/Mage/Index/controllers/Adminhtml/ProcessController.php @@ -18,13 +18,6 @@ class Mage_Index_Adminhtml_ProcessController extends Mage_Adminhtml_Controller_A */ public const ADMIN_RESOURCE = 'system/index'; - #[\Override] - public function preDispatch(): self - { - $this->_setForcedFormKeyActions(['reindexProcess', 'reindexAll', 'massReindex', 'massChangeMode']); - return parent::preDispatch(); - } - /** * Initialize process object by request * diff --git a/app/code/core/Mage/Oauth/controllers/Adminhtml/Oauth/ConsumerController.php b/app/code/core/Mage/Oauth/controllers/Adminhtml/Oauth/ConsumerController.php index f6c5fcb3e8..2a1208dfdd 100644 --- a/app/code/core/Mage/Oauth/controllers/Adminhtml/Oauth/ConsumerController.php +++ b/app/code/core/Mage/Oauth/controllers/Adminhtml/Oauth/ConsumerController.php @@ -34,7 +34,6 @@ protected function _filter(array $data) #[\Override] public function preDispatch() { - $this->_setForcedFormKeyActions(['delete']); $this->_title($this->__('System')) ->_title($this->__('OAuth')) ->_title($this->__('Consumers')); diff --git a/app/code/core/Mage/Widget/controllers/Adminhtml/Widget/InstanceController.php b/app/code/core/Mage/Widget/controllers/Adminhtml/Widget/InstanceController.php index 734ded518a..b949f2a70c 100644 --- a/app/code/core/Mage/Widget/controllers/Adminhtml/Widget/InstanceController.php +++ b/app/code/core/Mage/Widget/controllers/Adminhtml/Widget/InstanceController.php @@ -271,18 +271,6 @@ public function templateAction(): void $this->getResponse()->setBody($templateChooser->toHtml()); } - /** - * Controller pre-dispatch method - * - * @return Mage_Adminhtml_Controller_Action - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions('delete'); - return parent::preDispatch(); - } - /** * Prepare widget parameters * diff --git a/app/code/core/Maho/AccessibilityScan/controllers/Adminhtml/Accessibilityscan/ScanController.php b/app/code/core/Maho/AccessibilityScan/controllers/Adminhtml/Accessibilityscan/ScanController.php index 9e038a4e5a..9409c7dbc7 100644 --- a/app/code/core/Maho/AccessibilityScan/controllers/Adminhtml/Accessibilityscan/ScanController.php +++ b/app/code/core/Maho/AccessibilityScan/controllers/Adminhtml/Accessibilityscan/ScanController.php @@ -12,13 +12,6 @@ class Maho_AccessibilityScan_Adminhtml_Accessibilityscan_ScanController extends { public const ADMIN_RESOURCE = 'system/tools/accessibilityscan'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['start', 'run', 'delete']); - return parent::preDispatch(); - } - #[Maho\Config\Route('/admin/accessibilityscan_scan/view')] public function viewAction(): void { diff --git a/app/code/core/Maho/Ai/controllers/Adminhtml/AiController.php b/app/code/core/Maho/Ai/controllers/Adminhtml/AiController.php index 3b65a05945..8dc013c841 100644 --- a/app/code/core/Maho/Ai/controllers/Adminhtml/AiController.php +++ b/app/code/core/Maho/Ai/controllers/Adminhtml/AiController.php @@ -31,16 +31,6 @@ class Maho_Ai_Adminhtml_AiController extends Mage_Adminhtml_Controller_Action /** Read-only JSON poll — skip URL secret key, session cookie + ACL is sufficient. */ protected $_publicActions = ['taskStatus']; - #[\Override] - public function preDispatch(): static - { - // fetchModels triggers outbound provider HTTP and writes the cached - // model list to core_config_data, so it's state-changing and needs - // a form key. - $this->_setForcedFormKeyActions(['reindexPost', 'fetchModels']); - return parent::preDispatch(); - } - #[\Override] protected function _isAllowed(): bool { diff --git a/app/code/core/Maho/ApiPlatform/controllers/Adminhtml/Apiplatform/RoleController.php b/app/code/core/Maho/ApiPlatform/controllers/Adminhtml/Apiplatform/RoleController.php index 6f0e7bdfcf..c00d5e63a6 100644 --- a/app/code/core/Maho/ApiPlatform/controllers/Adminhtml/Apiplatform/RoleController.php +++ b/app/code/core/Maho/ApiPlatform/controllers/Adminhtml/Apiplatform/RoleController.php @@ -12,13 +12,6 @@ class Maho_ApiPlatform_Adminhtml_Apiplatform_RoleController extends Mage_Adminht { public const ADMIN_RESOURCE = 'system/api/api2_roles'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['delete', 'save']); - return parent::preDispatch(); - } - protected function _initAction(): static { $this->loadLayout() diff --git a/app/code/core/Maho/ApiPlatform/controllers/Adminhtml/Apiplatform/UserController.php b/app/code/core/Maho/ApiPlatform/controllers/Adminhtml/Apiplatform/UserController.php index fdda3b942a..fc4003d24f 100644 --- a/app/code/core/Maho/ApiPlatform/controllers/Adminhtml/Apiplatform/UserController.php +++ b/app/code/core/Maho/ApiPlatform/controllers/Adminhtml/Apiplatform/UserController.php @@ -12,13 +12,6 @@ class Maho_ApiPlatform_Adminhtml_Apiplatform_UserController extends Mage_Adminht { public const ADMIN_RESOURCE = 'system/api/api2_users'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['delete', 'save']); - return parent::preDispatch(); - } - protected function _initAction(): static { $this->loadLayout() diff --git a/app/code/core/Maho/Blog/controllers/Adminhtml/Blog/CategoryController.php b/app/code/core/Maho/Blog/controllers/Adminhtml/Blog/CategoryController.php index 2437fcc95b..e6beec7a3f 100644 --- a/app/code/core/Maho/Blog/controllers/Adminhtml/Blog/CategoryController.php +++ b/app/code/core/Maho/Blog/controllers/Adminhtml/Blog/CategoryController.php @@ -12,13 +12,6 @@ class Maho_Blog_Adminhtml_Blog_CategoryController extends Mage_Adminhtml_Control { public const ADMIN_RESOURCE = 'cms/blog/categories'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['save', 'delete', 'massDelete']); - return parent::preDispatch(); - } - protected function _initAction(): self { $this->loadLayout() diff --git a/app/code/core/Maho/Blog/controllers/Adminhtml/Blog/PostController.php b/app/code/core/Maho/Blog/controllers/Adminhtml/Blog/PostController.php index fd9a218644..62d6b66052 100644 --- a/app/code/core/Maho/Blog/controllers/Adminhtml/Blog/PostController.php +++ b/app/code/core/Maho/Blog/controllers/Adminhtml/Blog/PostController.php @@ -10,13 +10,6 @@ class Maho_Blog_Adminhtml_Blog_PostController extends Mage_Adminhtml_Controller_ { public const ADMIN_RESOURCE = 'cms/blog/posts'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['save', 'delete', 'massDelete']); - return parent::preDispatch(); - } - protected function _initAction(): self { $this->loadLayout() diff --git a/app/code/core/Maho/CatalogLinkRule/controllers/Adminhtml/Cataloglinkrule/RuleController.php b/app/code/core/Maho/CatalogLinkRule/controllers/Adminhtml/Cataloglinkrule/RuleController.php index 160db9bb6e..afab7252a9 100644 --- a/app/code/core/Maho/CatalogLinkRule/controllers/Adminhtml/Cataloglinkrule/RuleController.php +++ b/app/code/core/Maho/CatalogLinkRule/controllers/Adminhtml/Cataloglinkrule/RuleController.php @@ -233,10 +233,4 @@ private function _renderRuleHtml(string $prefix): void $this->getResponse()->setBody($model->asHtmlRecursive()); } - #[\Override] - public function preDispatch(): bool|Mage_Core_Controller_Varien_Action - { - $this->_setForcedFormKeyActions(['delete', 'massDelete', 'massStatus']); - return parent::preDispatch(); - } } diff --git a/app/code/core/Maho/ContentVersion/Block/Adminhtml/Version/Grid.php b/app/code/core/Maho/ContentVersion/Block/Adminhtml/Version/Grid.php index e3834176d9..c96efb015c 100644 --- a/app/code/core/Maho/ContentVersion/Block/Adminhtml/Version/Grid.php +++ b/app/code/core/Maho/ContentVersion/Block/Adminhtml/Version/Grid.php @@ -74,7 +74,7 @@ protected function _prepareColumns(): self [ 'caption' => Mage::helper('contentversion')->__('Restore'), 'confirm' => Mage::helper('contentversion')->__('Are you sure you want to restore this version? The current content will be saved as a new version first.'), - 'url' => ['base' => 'adminhtml/contentversion/restore', 'params' => $this->getUrlSecureParams()], + 'url' => ['base' => 'adminhtml/contentversion/restore'], 'field' => 'version_id', ], ], diff --git a/app/code/core/Maho/ContentVersion/controllers/Adminhtml/ContentversionController.php b/app/code/core/Maho/ContentVersion/controllers/Adminhtml/ContentversionController.php index 48503867f9..7f3cbe9ce1 100644 --- a/app/code/core/Maho/ContentVersion/controllers/Adminhtml/ContentversionController.php +++ b/app/code/core/Maho/ContentVersion/controllers/Adminhtml/ContentversionController.php @@ -12,13 +12,6 @@ class Maho_ContentVersion_Adminhtml_ContentversionController extends Mage_Adminh { public const ADMIN_RESOURCE = 'cms/contentversion'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['restore']); - return parent::preDispatch(); - } - #[Maho\Config\Route('/admin/contentversion/restore')] public function restoreAction(): void { diff --git a/app/code/core/Maho/FeedManager/Block/Adminhtml/Destination/Grid.php b/app/code/core/Maho/FeedManager/Block/Adminhtml/Destination/Grid.php index c8bc9bb580..79be70303d 100644 --- a/app/code/core/Maho/FeedManager/Block/Adminhtml/Destination/Grid.php +++ b/app/code/core/Maho/FeedManager/Block/Adminhtml/Destination/Grid.php @@ -106,7 +106,7 @@ protected function _prepareColumns(): self ], [ 'caption' => $this->__('Test'), - 'url' => ['base' => '*/*/test', 'params' => $this->getUrlSecureParams()], + 'url' => ['base' => '*/*/test'], 'field' => 'id', ], ], diff --git a/app/code/core/Maho/FeedManager/Block/Adminhtml/Feed/Edit.php b/app/code/core/Maho/FeedManager/Block/Adminhtml/Feed/Edit.php index 62240317f8..50d675ac18 100644 --- a/app/code/core/Maho/FeedManager/Block/Adminhtml/Feed/Edit.php +++ b/app/code/core/Maho/FeedManager/Block/Adminhtml/Feed/Edit.php @@ -38,7 +38,7 @@ public function __construct() if ($this->_getFeed()->getId()) { $this->_addButton('duplicate', [ 'label' => $this->__('Duplicate'), - 'onclick' => "setLocation('" . $this->getUrlSecure('*/*/duplicate', ['id' => $this->_getFeed()->getId()]) . "')", + 'onclick' => "setLocation('" . $this->getUrl('*/*/duplicate', ['id' => $this->_getFeed()->getId()]) . "')", 'class' => 'add', ], -95); diff --git a/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/CategoryController.php b/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/CategoryController.php index eb9e13b226..1b11600efa 100644 --- a/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/CategoryController.php +++ b/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/CategoryController.php @@ -12,13 +12,6 @@ class Maho_FeedManager_Adminhtml_Feedmanager_CategoryController extends Mage_Adm { public const ADMIN_RESOURCE = 'catalog/feedmanager/category_mapping'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['save', 'autoMap']); - return parent::preDispatch(); - } - protected function _initAction(): self { $this->loadLayout() diff --git a/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/DestinationController.php b/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/DestinationController.php index 0b4ddd298e..3f89f13161 100644 --- a/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/DestinationController.php +++ b/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/DestinationController.php @@ -12,13 +12,6 @@ class Maho_FeedManager_Adminhtml_Feedmanager_DestinationController extends Mage_ { public const ADMIN_RESOURCE = 'catalog/feedmanager/destinations'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['delete', 'save', 'test', 'massStatus', 'massDelete']); - return parent::preDispatch(); - } - protected function _initAction(): self { $this->loadLayout() diff --git a/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/DynamicruleController.php b/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/DynamicruleController.php index a895562963..d42d71581e 100644 --- a/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/DynamicruleController.php +++ b/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/DynamicruleController.php @@ -12,13 +12,6 @@ class Maho_FeedManager_Adminhtml_Feedmanager_DynamicruleController extends Mage_ { public const ADMIN_RESOURCE = 'catalog/feedmanager/dynamicrules'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['delete', 'save', 'massDelete', 'massStatus']); - return parent::preDispatch(); - } - protected function _initAction(): self { $this->loadLayout() diff --git a/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php b/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php index 2dcbcaabe4..aa21045bf7 100644 --- a/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php +++ b/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php @@ -12,28 +12,6 @@ class Maho_FeedManager_Adminhtml_Feedmanager_FeedController extends Mage_Adminht { public const ADMIN_RESOURCE = 'catalog/feedmanager/feeds'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions([ - 'delete', - 'save', - 'duplicate', - 'generateInit', - 'generateBatch', - 'generateFinalize', - 'generateCancel', - 'reset', - 'forceReset', - 'massGenerate', - 'massBatchGenerate', - 'massStatus', - 'massDelete', - 'upload', - ]); - return parent::preDispatch(); - } - protected function _initAction(): self { $this->loadLayout() diff --git a/app/code/core/Maho/Giftcard/controllers/Adminhtml/GiftcardController.php b/app/code/core/Maho/Giftcard/controllers/Adminhtml/GiftcardController.php index 950050aacc..5963d46d7b 100644 --- a/app/code/core/Maho/Giftcard/controllers/Adminhtml/GiftcardController.php +++ b/app/code/core/Maho/Giftcard/controllers/Adminhtml/GiftcardController.php @@ -12,16 +12,6 @@ class Maho_Giftcard_Adminhtml_GiftcardController extends Mage_Adminhtml_Controll { public const ADMIN_RESOURCE = 'sales/giftcard/manage'; - /** - * Set forced form key actions for CSRF protection - */ - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['save', 'delete', 'massDelete', 'massStatus']); - return parent::preDispatch(); - } - /** * Init actions * diff --git a/app/code/core/Maho/MediaCleaner/Block/Adminhtml/Mediacleaner.php b/app/code/core/Maho/MediaCleaner/Block/Adminhtml/Mediacleaner.php index 6810e9718b..7741e66795 100644 --- a/app/code/core/Maho/MediaCleaner/Block/Adminhtml/Mediacleaner.php +++ b/app/code/core/Maho/MediaCleaner/Block/Adminhtml/Mediacleaner.php @@ -67,6 +67,6 @@ protected function buildItems(array $actionLabels): array protected function getActionUrl(string $action): string { - return $this->getUrlSecure('*/*/' . $action); + return $this->getUrl('*/*/' . $action); } } diff --git a/app/code/core/Maho/MediaCleaner/Block/Adminhtml/Mediacleaner/Grid/Renderer/Actions.php b/app/code/core/Maho/MediaCleaner/Block/Adminhtml/Mediacleaner/Grid/Renderer/Actions.php index 26c3592e0f..66fb7ac909 100644 --- a/app/code/core/Maho/MediaCleaner/Block/Adminhtml/Mediacleaner/Grid/Renderer/Actions.php +++ b/app/code/core/Maho/MediaCleaner/Block/Adminhtml/Mediacleaner/Grid/Renderer/Actions.php @@ -23,7 +23,7 @@ public function render(\Maho\DataObject $row) $links[] = sprintf( '%s', - $this->getUrlSecure('*/*/delete', ['image_id' => $row->getId()]), + $this->getUrl('*/*/delete', ['image_id' => $row->getId()]), $this->jsQuoteEscape($this->__('Are you sure?')), $this->__('Delete'), ); diff --git a/app/code/core/Maho/MediaCleaner/controllers/Adminhtml/MediacleanerController.php b/app/code/core/Maho/MediaCleaner/controllers/Adminhtml/MediacleanerController.php index 03a9a4a655..313c5fba18 100644 --- a/app/code/core/Maho/MediaCleaner/controllers/Adminhtml/MediacleanerController.php +++ b/app/code/core/Maho/MediaCleaner/controllers/Adminhtml/MediacleanerController.php @@ -12,25 +12,6 @@ class Maho_MediaCleaner_Adminhtml_MediacleanerController extends Mage_Adminhtml_ { public const ADMIN_RESOURCE = 'system/tools/mediacleaner'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions([ - 'synccategory', - 'syncproduct', - 'syncproductcache', - 'syncwysiwyg', - 'delete', - 'massDelete', - 'flushmediatmp', - 'flushmediaimport', - 'flushvarexport', - 'flushvarimportexport', - 'reset', - ]); - return parent::preDispatch(); - } - #[Maho\Config\Route('/admin/mediacleaner/index')] public function indexAction(): void { diff --git a/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php b/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php index b2f7ddd852..8421c55c4e 100644 --- a/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php +++ b/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php @@ -22,12 +22,12 @@ public function getBackUrl(): string public function getRetryUrl(): string { - return $this->getUrlSecure('*/*/retry', ['id' => $this->getMessage()?->getId()]); + return $this->getUrl('*/*/retry', ['id' => $this->getMessage()?->getId()]); } public function getDiscardUrl(): string { - return $this->getUrlSecure('*/*/discard', ['id' => $this->getMessage()?->getId()]); + return $this->getUrl('*/*/discard', ['id' => $this->getMessage()?->getId()]); } /** diff --git a/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php b/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php index 70d2f4c03f..1e6be6bbe9 100644 --- a/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php +++ b/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php @@ -24,13 +24,6 @@ class Maho_Queue_Adminhtml_QueueController extends Mage_Adminhtml_Controller_Act 'massdiscard' => 'system/tools/maho_queue/discard', ]; - #[\Override] - public function preDispatch(): static - { - $this->_setForcedFormKeyActions(['retry', 'discard', 'massRetry', 'massDiscard']); - return parent::preDispatch(); - } - #[\Override] protected function _isAllowed(): bool { diff --git a/app/code/core/Maho/Revocation/Block/Adminhtml/Request/View.php b/app/code/core/Maho/Revocation/Block/Adminhtml/Request/View.php index ff2fddccc2..9359605e1a 100644 --- a/app/code/core/Maho/Revocation/Block/Adminhtml/Request/View.php +++ b/app/code/core/Maho/Revocation/Block/Adminhtml/Request/View.php @@ -80,7 +80,7 @@ public function getLinkOrderUrl(): string public function getResendUrl(): string { - return $this->getUrlSecure('*/*/resend', ['id' => $this->getRevocationRequest()?->getId()]); + return $this->getUrl('*/*/resend', ['id' => $this->getRevocationRequest()?->getId()]); } public function getOrderViewUrl(Mage_Sales_Model_Order $order): string diff --git a/app/code/core/Maho/Revocation/controllers/Adminhtml/Sales/RevocationController.php b/app/code/core/Maho/Revocation/controllers/Adminhtml/Sales/RevocationController.php index 295664acea..48fc75b514 100644 --- a/app/code/core/Maho/Revocation/controllers/Adminhtml/Sales/RevocationController.php +++ b/app/code/core/Maho/Revocation/controllers/Adminhtml/Sales/RevocationController.php @@ -12,13 +12,6 @@ class Maho_Revocation_Adminhtml_Sales_RevocationController extends Mage_Adminhtm { public const ADMIN_RESOURCE = 'sales/revocation'; - #[\Override] - public function preDispatch() - { - $this->_setForcedFormKeyActions(['save', 'process', 'linkOrder', 'resend', 'massAccept', 'massReject']); - return parent::preDispatch(); - } - protected function _initAction(): self { $this->loadLayout() diff --git a/app/design/adminhtml/default/default/template/contentversion/preview.phtml b/app/design/adminhtml/default/default/template/contentversion/preview.phtml index a00e5edb99..12fe8d6fa2 100644 --- a/app/design/adminhtml/default/default/template/contentversion/preview.phtml +++ b/app/design/adminhtml/default/default/template/contentversion/preview.phtml @@ -17,7 +17,7 @@ $typeLabels = [ 'cms_block' => $this->__('CMS Block'), 'blog_post' => $this->__('Blog Post'), ]; -$restoreUrl = $this->getUrlSecure('adminhtml/contentversion/restore', ['version_id' => $versionId]); +$restoreUrl = $this->getUrl('adminhtml/contentversion/restore', ['version_id' => $versionId]); $confirmMsg = $this->jsQuoteEscape( $this->__('Are you sure you want to restore this version? The current content will be saved as a new version first.'), ); diff --git a/app/locale/en_US/Mage_Core.csv b/app/locale/en_US/Mage_Core.csv index b3f1c03f57..51e00778ca 100644 --- a/app/locale/en_US/Mage_Core.csv +++ b/app/locale/en_US/Mage_Core.csv @@ -7,7 +7,6 @@ "Warning! When using Store Code in URLs, in some cases system may not work properly if URLs without Store Codes are specified in the third party services (e.g. PayPal etc.).","Warning! When using Store Code in URLs, in some cases system may not work properly if URLs without Store Codes are specified in the third party services (e.g. PayPal etc.)." "A category object is required for determining the product request path.","A category object is required for determining the product request path." "Add Block Names to Hints","Add Block Names to Hints" -"Add Secret Key to URLs","Add Secret Key to URLs" "Add Store Code to Urls","Add Store Code to Urls" "Admin","Admin" "Admin Base URL","Admin Base URL" diff --git a/tests/Backend/Integration/Adminhtml/AdminUrlFormKeyTest.php b/tests/Backend/Integration/Adminhtml/AdminUrlFormKeyTest.php index 18f70b9715..c7cf45f80b 100644 --- a/tests/Backend/Integration/Adminhtml/AdminUrlFormKeyTest.php +++ b/tests/Backend/Integration/Adminhtml/AdminUrlFormKeyTest.php @@ -10,20 +10,12 @@ uses(Tests\MahoBackendTestCase::class); /** - * The form key is the session-wide CSRF root the per-action secret key is derived from, so it - * must never ride in an admin url that doesn't need it. Only getUrlSecure() may emit it, and - * only when secret keys are off, which is the sole config where _setForcedFormKeyActions() - * arms itself. With secret keys on nothing reads it, so no url should contain it. + * Admin GET protection is always on: every admin url carries the per-action secret key, and + * the session-wide form key it is derived from never rides in a url (POST carries it in a + * hidden input instead). The only exception is the explicit _nosecret opt-out used by the + * login flow and RSS feeds, where no session exists to validate against. */ -function adminUrlSetSecretKey(bool $enabled): void -{ - Mage::app()->getStore()->setConfig( - Mage_Adminhtml_Helper_Data::XML_PATH_ADMINHTML_SECURITY_USE_FORM_KEY, - $enabled ? 1 : 0, - ); -} - function adminUrlBlock(): Mage_Adminhtml_Block_Template { /** @var Mage_Adminhtml_Block_Template $block */ @@ -32,129 +24,65 @@ function adminUrlBlock(): Mage_Adminhtml_Block_Template } afterEach(function () { - Mage::app()->getStore()->resetConfig(); Mage::unregister('current_creditmemo'); }); -it('keeps the form key out of admin urls when secret keys are on', function () { - adminUrlSetSecretKey(true); - $block = adminUrlBlock(); - $plain = $block->getUrl('adminhtml/sales_order/view', ['order_id' => 1]); +it('always puts the secret key in admin urls and keeps the form key out', function () { + $url = adminUrlBlock()->getUrl('adminhtml/sales_order/view', ['order_id' => 1]); - expect($plain)->not->toContain('form_key') - ->and($plain)->toContain('/key/') - ->and($block->getUrlSecure('adminhtml/sales_order/hold', ['order_id' => 1]))->not->toContain('form_key'); + expect($url)->toContain('/key/') + ->and($url)->not->toContain('form_key'); }); -it('emits the form key only from getUrlSecure when secret keys are off', function () { - adminUrlSetSecretKey(false); - $block = adminUrlBlock(); - $plain = $block->getUrl('adminhtml/sales_order/view', ['order_id' => 1]); +it('matches the secret key in the url to the one the action validates', function () { + $url = adminUrlBlock()->getUrl('adminhtml/sales_order/view', ['order_id' => 1]); - expect($plain)->not->toContain('form_key') - ->and($plain)->not->toContain('/key/') - ->and($block->getUrlSecure('adminhtml/sales_order/hold', ['order_id' => 1])) - ->toContain('form_key/' . Mage::getSingleton('core/session')->getFormKey()); + preg_match('#/key/([^/]+)/#', $url, $m); + expect($m[1] ?? null)->toBe(Mage::getSingleton('adminhtml/url')->getSecretKey('sales_order', 'view')); }); -it('builds grid action params with the same form key rule as urls', function () { - adminUrlSetSecretKey(true); - expect(adminUrlBlock()->getUrlSecureParams())->toBe([]); - - adminUrlSetSecretKey(false); - expect(adminUrlBlock()->getUrlSecureParams(['id' => 7])) - ->toBe(['id' => 7, 'form_key' => Mage::getSingleton('core/session')->getFormKey()]); +it('drops the secret key only on urls that opt out via _nosecret', function () { + expect(adminUrlBlock()->getUrl('adminhtml/index/forgotpassword', ['_nosecret' => true])) + ->not->toContain('/key/'); }); /** - * Every delete button on a form container is a plain GET navigation, and the delete action is on - * the forced-form-key list of most admin controllers, so the url has to carry the key when secret - * keys are off and must not leak it when they're on. + * Buttons that build their own GET navigation used to need a hand-wired form key opt-in; + * now they all go through getUrl() and pick the secret key up automatically. */ -it('scopes the form key on the shared form container delete url', function () { - /** @var Mage_Adminhtml_Block_Widget_Form_Container $block */ - $block = Mage::app()->getLayout()->createBlock('adminhtml/widget_form_container'); +it('carries the secret key on buttons that build their own action url', function (string $blockAlias, string $method) { + $url = Mage::app()->getLayout()->createBlock($blockAlias)->{$method}(); - adminUrlSetSecretKey(true); - expect($block->getDeleteUrl())->not->toContain('form_key'); - - adminUrlSetSecretKey(false); - expect($block->getDeleteUrl()) - ->toContain('form_key/' . Mage::getSingleton('core/session')->getFormKey()); -}); - -/** - * These buttons build their own url instead of going through the container, so each one has to - * opt into getUrlSecure() by hand. setLocation() used to paper over a miss by appending the key - * to every navigation; without it a miss is a dead button. - */ -it('carries the form key on buttons that build their own forced-action url', function (string $blockAlias, string $method) { - adminUrlSetSecretKey(false); - $formKey = Mage::getSingleton('core/session')->getFormKey(); - - $block = Mage::app()->getLayout()->createBlock($blockAlias); - - expect($block->{$method}())->toContain('form_key/' . $formKey); - - adminUrlSetSecretKey(true); - expect(Mage::app()->getLayout()->createBlock($blockAlias)->{$method}())->not->toContain('form_key'); + expect($url)->toContain('/key/') + ->and($url)->not->toContain('form_key'); })->with([ 'queue retry' => ['queue/adminhtml_message_view', 'getRetryUrl'], 'queue discard' => ['queue/adminhtml_message_view', 'getDiscardUrl'], 'revocation resend' => ['revocation/adminhtml_request_view', 'getResendUrl'], ]); -/** - * Cancel and void on a credit memo are the same shape as their invoice counterparts: a plain GET - * navigation into a forced-form-key action. The state keeps every button branch in the constructor - * from touching an order that isn't there; only the urls are under test. - */ -it('carries the form key on the credit memo cancel and void urls', function (string $method) { +it('carries the secret key on the shared form container delete url', function () { + /** @var Mage_Adminhtml_Block_Widget_Form_Container $block */ + $block = Mage::app()->getLayout()->createBlock('adminhtml/widget_form_container'); + + expect($block->getDeleteUrl())->toContain('/key/') + ->and($block->getDeleteUrl())->not->toContain('form_key'); +}); + +it('carries the secret key on the credit memo cancel and void urls', function (string $method) { Mage::register('current_creditmemo', Mage::getModel('sales/order_creditmemo') ->setId(1) ->setState(Mage_Sales_Model_Order_Creditmemo::STATE_CANCELED)); $block = Mage::app()->getLayout()->createBlock('adminhtml/sales_order_creditmemo_view'); - adminUrlSetSecretKey(false); - expect($block->{$method}())->toContain('form_key/' . Mage::getSingleton('core/session')->getFormKey()); - - adminUrlSetSecretKey(true); - expect($block->{$method}())->not->toContain('form_key'); + expect($block->{$method}())->toContain('/key/') + ->and($block->{$method}())->not->toContain('form_key'); })->with(['getCancelUrl', 'getVoidUrl']); -/** - * A grid action column builds its own href and navigates through setLocation(), so an action on the - * forced-form-key list has to put the key in the column config itself. - */ -it('carries the form key on the feed destination grid test action', function () { - $testAction = function (): array { - $block = Mage::app()->getLayout()->createBlock('feedmanager/adminhtml_destination_grid'); - Closure::bind(fn() => $this->_prepareColumns(), $block, $block::class)(); - $actions = $block->getColumn('action')->getActions(); - return array_values(array_filter($actions, fn(array $a) => $a['url']['base'] === '*/*/test'))[0]; - }; - - adminUrlSetSecretKey(false); - expect($testAction()['url']['params']) - ->toBe([Mage_Core_Model_Url::FORM_KEY => Mage::getSingleton('core/session')->getFormKey()]); - - adminUrlSetSecretKey(true); - expect($testAction()['url']['params'])->toBe([]); -}); - -/** - * Instantiated directly because _prepareLayout() needs a registered category; only the url - * matters here. The tree's delete button is the one navigation category.js no longer patches - * the key onto, so the block has to supply it. - */ -it('carries the form key on the category tree delete url', function () { +it('carries the secret key on the category tree delete url', function () { $block = new Mage_Adminhtml_Block_Catalog_Category_Edit_Form(); - adminUrlSetSecretKey(false); - expect($block->getDeleteUrl()) - ->toContain('form_key/' . Mage::getSingleton('core/session')->getFormKey()); - - adminUrlSetSecretKey(true); - expect($block->getDeleteUrl())->not->toContain('form_key'); + expect($block->getDeleteUrl())->toContain('/key/') + ->and($block->getDeleteUrl())->not->toContain('form_key'); }); diff --git a/tests/Browser/AdminQueryStringUrlsTest.php b/tests/Browser/AdminQueryStringUrlsTest.php index c81d2b50ba..b1db938a29 100644 --- a/tests/Browser/AdminQueryStringUrlsTest.php +++ b/tests/Browser/AdminQueryStringUrlsTest.php @@ -20,8 +20,8 @@ * the path, and the param never arrives. Same defect as the store switcher; these are the two * other places that had it. * - * Secret keys are turned off for the run so the screens can be addressed directly; the query - * string the screens have to survive is therefore supplied by the test itself. + * The screens are addressed directly with a minted secret key; the query string the screens + * have to survive is supplied by the test itself. */ const QUERY_URLS_ADMIN_USER = 'query-urls-admin'; @@ -29,18 +29,11 @@ const QUERY_URLS_QUERY = 'pest_query=PestQueryStringUrls'; beforeEach(function () { - Mage::getModel('core/config')->saveConfig(Mage_Adminhtml_Helper_Data::XML_PATH_ADMINHTML_SECURITY_USE_FORM_KEY, 0); - Mage::app()->cleanCache(); - createQueryUrlsAdmin(); }); afterEach(function () { deleteQueryUrlsAdmin(); - - Mage::getModel('core/config')->deleteConfig(Mage_Adminhtml_Helper_Data::XML_PATH_ADMINHTML_SECURITY_USE_FORM_KEY); - Mage::app()->getStore()->resetConfig(); - Mage::app()->cleanCache(); }); function deleteQueryUrlsAdmin(): void @@ -83,7 +76,7 @@ function visitAdminPage(string $path, string $selector): object ->click('#step1 input[type="submit"]'); waitForPageLoad($page, '.nav-bar:visible'); - $page->navigate(MahoServer::baseUrl() . $path); + $page->navigate(MahoServer::baseUrl() . adminPathWithSecretKey($page, $path)); return waitForPageLoad($page, $selector); } diff --git a/tests/Browser/AdminReindexDialogTest.php b/tests/Browser/AdminReindexDialogTest.php index 3258b8b60e..26c3d94da7 100644 --- a/tests/Browser/AdminReindexDialogTest.php +++ b/tests/Browser/AdminReindexDialogTest.php @@ -12,20 +12,10 @@ uses(MahoBrowserTestCase::class)->group('browser'); -/** - * Secret keys in admin urls are turned off for the run so the test can address - * adminhtml/process/list directly; that also puts the reindex endpoints on the forced form key - * path, which is the protection this screen relies on in that configuration. - */ - const REINDEX_ADMIN_USER = 'reindex-dialog-admin'; const REINDEX_ADMIN_PASSWORD = 'Password123!'; -const REINDEX_SECRET_KEY_PATH = 'admin/security/use_form_key'; beforeEach(function () { - Mage::getModel('core/config')->saveConfig(REINDEX_SECRET_KEY_PATH, 0); - Mage::app()->cleanCache(); - createReindexAdmin(); }); @@ -33,11 +23,6 @@ // Mage::reset(), so there is no app left to clean up through afterEach(function () { deleteReindexAdmin(); - - // Drop the override rather than pin a value - Mage::getModel('core/config')->deleteConfig(REINDEX_SECRET_KEY_PATH); - Mage::app()->getStore()->resetConfig(); - Mage::app()->cleanCache(); }); function deleteReindexAdmin(): void @@ -79,7 +64,7 @@ function loginToIndexManagement(): object ->click('#step1 input[type="submit"]'); waitForPageLoad($page, '.nav-bar:visible'); - $page->navigate(MahoServer::baseUrl() . '/admin/process/list'); + $page->navigate(MahoServer::baseUrl() . adminPathWithSecretKey($page, '/admin/process/list')); return waitForPageLoad($page, '#indexer_processes_grid_table:visible'); } diff --git a/tests/Browser/AdminRelatedProductsGridTest.php b/tests/Browser/AdminRelatedProductsGridTest.php index 22c94adb1e..9304d7b62c 100644 --- a/tests/Browser/AdminRelatedProductsGridTest.php +++ b/tests/Browser/AdminRelatedProductsGridTest.php @@ -34,21 +34,9 @@ beforeEach(function () { deleteRelatedGridFixtures(); - - // Admin urls carry a secret key derived from the session's form key, which this process - // can't mint, so navigating straight to the product edit page needs the key turned off. - Mage::getModel('core/config')->saveConfig( - Mage_Adminhtml_Helper_Data::XML_PATH_ADMINHTML_SECURITY_USE_FORM_KEY, - '0', - ); - Mage::app()->cleanCache(); }); afterEach(function () { - Mage::getModel('core/config')->deleteConfig( - Mage_Adminhtml_Helper_Data::XML_PATH_ADMINHTML_SECURITY_USE_FORM_KEY, - ); - Mage::app()->cleanCache(); deleteRelatedGridFixtures(); }); @@ -160,7 +148,7 @@ function visitRelatedProductsTab(int $parentId): object ->click('#step1 input[type="submit"]'); waitForPageLoad($page, '.nav-bar:visible'); - $page->navigate(MahoServer::baseUrl() . '/admin/catalog_product/edit/id/' . $parentId); + $page->navigate(MahoServer::baseUrl() . adminPathWithSecretKey($page, '/admin/catalog_product/edit/id/' . $parentId)); waitForPageLoad($page, '#product_info_tabs_related'); // An inactive tab keeps its content attached, so `:visible` is what distinguishes the diff --git a/tests/Browser/AdminStoreSwitcherTest.php b/tests/Browser/AdminStoreSwitcherTest.php index 2124de8c58..fbce45c2ce 100644 --- a/tests/Browser/AdminStoreSwitcherTest.php +++ b/tests/Browser/AdminStoreSwitcherTest.php @@ -20,8 +20,8 @@ * the last query param's *value*: the store view never arrived, the form key came back * corrupted, and repeated switches accumulated. Only a browser exercises that js. * - * Secret keys are turned off for the run so the grid can be addressed directly; the query - * string the switcher has to preserve is therefore supplied by the test itself. + * The grid is addressed directly with a minted secret key; the query string the switcher + * has to preserve is supplied by the test itself. */ const STORE_SWITCHER_ADMIN_USER = 'store-switcher-admin'; @@ -29,18 +29,11 @@ const STORE_SWITCHER_QUERY = 'form_key=PestStoreSwitcherFormKey'; beforeEach(function () { - Mage::getModel('core/config')->saveConfig(Mage_Adminhtml_Helper_Data::XML_PATH_ADMINHTML_SECURITY_USE_FORM_KEY, 0); - Mage::app()->cleanCache(); - createStoreSwitcherAdmin(); }); afterEach(function () { deleteStoreSwitcherAdmin(); - - Mage::getModel('core/config')->deleteConfig(Mage_Adminhtml_Helper_Data::XML_PATH_ADMINHTML_SECURITY_USE_FORM_KEY); - Mage::app()->getStore()->resetConfig(); - Mage::app()->cleanCache(); }); function deleteStoreSwitcherAdmin(): void @@ -89,7 +82,7 @@ function visitProductGridWithQuery(): object ->click('#step1 input[type="submit"]'); waitForPageLoad($page, '.nav-bar:visible'); - $page->navigate(MahoServer::baseUrl() . '/admin/catalog_product/index/?' . STORE_SWITCHER_QUERY); + $page->navigate(MahoServer::baseUrl() . adminPathWithSecretKey($page, '/admin/catalog_product/index/?' . STORE_SWITCHER_QUERY)); return waitForPageLoad($page, '#store_switcher'); } diff --git a/tests/Pest.php b/tests/Pest.php index df3f8df054..cf00f910d6 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -434,6 +434,38 @@ function waitForPageLoad(object $page, string $selector): object return $page; } +/** + * Turn an admin path into one carrying the secret key its GET validation expects. + * + * Admin urls always validate a per-action secret key derived from the session's form key, + * which the logged-in admin page exposes as window.FORM_KEY. Minting the key from the page + * lets a test navigate straight to any admin action, which no plain deep link can do. + * + * $page must already be on a logged-in admin page. $path is "/frontName/controller/action" + * with optional extra "/param/value" segments and query string; the controller and action + * default to "index" exactly like the router, and the key segment is appended after the + * params, matching the shape the url generator emits. + */ +function adminPathWithSecretKey(object $page, string $path): string +{ + $formKey = (string) $page->script('window.FORM_KEY'); + if ($formKey === '') { + throw new RuntimeException('window.FORM_KEY is empty; is the page a logged-in admin page?'); + } + + [$pathPart, $query] = array_pad(explode('?', $path, 2), 2, null); + $segments = explode('/', trim($pathPart, '/')); + $front = $segments[0]; + $controller = $segments[1] ?? 'index'; + $action = $segments[2] ?? 'index'; + $extra = array_slice($segments, 3); + + $secretKey = Mage::helper('core')->getHash(strtolower($controller) . strtolower($action) . $formKey); + + $path = '/' . implode('/', array_merge([$front, $controller, $action], $extra, ['key', $secretKey])) . '/'; + return $path . ($query !== null ? '?' . $query : ''); +} + /* |-------------------------------------------------------------------------- | Message Queue Helper Functions From 5e8fd138cf1953477a43608d7cc278a4705f3fb4 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 16:47:04 +0000 Subject: [PATCH 2/6] Fixed the two findings PHPStan 2.2.8 introduced on main - LINK_GET_PARENTS shared LINK_GET_CHILDREN's value, making the parents branch of fetchLinkInformation() dead code; gave it its own value. The method's only live caller passes LINK_GET_CHILDREN, so behavior is unchanged. - $m[3] is always set in the cms/block directive callback (PHP only truncates trailing unmatched captures), so dropped the ?? on it. --- app/code/core/Mage/CatalogIndex/Model/Data/Abstract.php | 2 +- app/code/core/Maho/ApiPlatform/symfony/CrudResource.php | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/code/core/Mage/CatalogIndex/Model/Data/Abstract.php b/app/code/core/Mage/CatalogIndex/Model/Data/Abstract.php index 29cebcd484..2dd2026ea5 100644 --- a/app/code/core/Mage/CatalogIndex/Model/Data/Abstract.php +++ b/app/code/core/Mage/CatalogIndex/Model/Data/Abstract.php @@ -46,7 +46,7 @@ class Mage_CatalogIndex_Model_Data_Abstract extends Mage_Core_Model_Abstract protected $_haveParents = true; public const LINK_GET_CHILDREN = 1; - public const LINK_GET_PARENTS = 1; + public const LINK_GET_PARENTS = 2; /** * Initialize abstract resource model diff --git a/app/code/core/Maho/ApiPlatform/symfony/CrudResource.php b/app/code/core/Maho/ApiPlatform/symfony/CrudResource.php index 52af6aa527..3e1ddd4aa4 100644 --- a/app/code/core/Maho/ApiPlatform/symfony/CrudResource.php +++ b/app/code/core/Maho/ApiPlatform/symfony/CrudResource.php @@ -270,10 +270,10 @@ private static function resolveCmsBlockDirectives( return preg_replace_callback( $pattern, static function (array $m) use ($storeId, $seen, $depth): string { - // PHP fills unmatched alternation captures with '' (not unset), - // so $m[3] and $m[5] are both always present, one has the id, - // the other is empty. Pick whichever is non-empty. - $identifier = ($m[3] ?? '') !== '' ? $m[3] : ($m[5] ?? ''); + // PHP truncates trailing unmatched captures, so $m[3] always + // exists (empty when the second alternation matched) while + // $m[5] is only set for the second one. Pick the non-empty id. + $identifier = $m[3] !== '' ? $m[3] : ($m[5] ?? ''); if ($identifier === '' || isset($seen[$identifier])) { return ''; } From 3615598c347f86e174ffc8a2f649b10591b5fc16 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 16:54:13 +0000 Subject: [PATCH 3/6] Removed the dead empty-identifier guard in the cms/block directive callback PHPStan's match-shape analysis proves the block_id capture of whichever alternation matched is non-empty, so the === '' branch could never run. --- app/code/core/Maho/ApiPlatform/symfony/CrudResource.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/code/core/Maho/ApiPlatform/symfony/CrudResource.php b/app/code/core/Maho/ApiPlatform/symfony/CrudResource.php index 3e1ddd4aa4..c6fd713ac6 100644 --- a/app/code/core/Maho/ApiPlatform/symfony/CrudResource.php +++ b/app/code/core/Maho/ApiPlatform/symfony/CrudResource.php @@ -272,9 +272,10 @@ private static function resolveCmsBlockDirectives( static function (array $m) use ($storeId, $seen, $depth): string { // PHP truncates trailing unmatched captures, so $m[3] always // exists (empty when the second alternation matched) while - // $m[5] is only set for the second one. Pick the non-empty id. + // $m[5] is only set for the second one. Whichever alternation + // matched, its block_id capture is non-empty. $identifier = $m[3] !== '' ? $m[3] : ($m[5] ?? ''); - if ($identifier === '' || isset($seen[$identifier])) { + if (isset($seen[$identifier])) { return ''; } $seen[$identifier] = true; From 8459ffc686fb892b45c1bf10da618a03dbb05f0c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 16:59:04 +0000 Subject: [PATCH 4/6] Made the feed edit screen accept the keyless links its notifier sends Maho_FeedManager_Model_Notifier builds feedmanager_feed/edit urls with _nosecret for failure emails and admin inbox notifications, but the controller never declared the action public, so the links bounced to the dashboard whenever secret keys were validated. Declaring the render-only edit action public completes the same pairing the RSS admin feeds use. --- .../controllers/Adminhtml/Feedmanager/FeedController.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php b/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php index aa21045bf7..77e9a2fe5d 100644 --- a/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php +++ b/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php @@ -12,6 +12,9 @@ class Maho_FeedManager_Adminhtml_Feedmanager_FeedController extends Mage_Adminht { public const ADMIN_RESOURCE = 'catalog/feedmanager/feeds'; + /** Render-only form target of the keyless (_nosecret) links in failure emails and inbox notifications */ + protected $_publicActions = ['edit']; + protected function _initAction(): self { $this->loadLayout() From a9448255882e89907f811ffcefda8c9eea8629c8 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Tue, 11 Aug 2026 18:49:01 +0100 Subject: [PATCH 5/6] Closed the CSRF gaps and cache regression the always-on secret key exposed Review follow-ups to the secret-key work: - Admin OAuth authorize confirm/reject were public GET actions that grant or reject a token from an attacker-controllable oauth_token, a CSRF hole. The admin button block now builds those urls through adminhtml/url so they carry the per-action secret key, and only index/simple stay public. - FeedManager made feedmanager_feed/edit public, but the edit page auto-starts generation from a generate=1 url param, so a cross-site GET could trigger it. The trigger now rides a one-shot session flag armed only by the key/POST protected save and generate actions. - RSS basic-auth polls re-run admin login every request; renewSecretUrls() ran unconditionally and flushed the admin menu cache on every poll. It is skipped again for keyless (RSS) logins. - getSecretKey() takes an optional form key so the browser-test helper reuses the real derivation instead of a copy. - Upgrade script uses deleteConfigData() with an exact path and declares strict_types; _getRequestUri() gains its native return type; trailing blank lines left by the removed preDispatch overrides are cleaned up. --- app/code/core/Mage/Admin/Model/Session.php | 11 ++++--- app/code/core/Mage/Adminhtml/Model/Url.php | 7 ++-- .../Api/OrphanedResourceController.php | 1 - .../Catalog/CategoryController.php | 1 - .../controllers/Catalog/SearchController.php | 1 - .../Newsletter/TemplateController.php | 1 - .../Permissions/BlockController.php | 1 - .../OrphanedResourceController.php | 1 - .../controllers/Sales/OrderController.php | 1 - .../controllers/Tax/RuleController.php | 1 - .../sql/core_setup/upgrade-2.1.0-2.1.1.php | 10 ++---- .../Adminhtml/Oauth/Authorize/Button.php | 11 +++++++ .../Adminhtml/Oauth/AuthorizeController.php | 6 ++-- .../Cataloglinkrule/RuleController.php | 1 - .../FeedManager/Block/Adminhtml/Feed/Edit.php | 33 ++++++++++++++----- .../Adminhtml/Feedmanager/FeedController.php | 6 ++-- tests/Pest.php | 2 +- 17 files changed, 60 insertions(+), 35 deletions(-) diff --git a/app/code/core/Mage/Admin/Model/Session.php b/app/code/core/Mage/Admin/Model/Session.php index db3fd49408..a3dbb40d58 100644 --- a/app/code/core/Mage/Admin/Model/Session.php +++ b/app/code/core/Mage/Admin/Model/Session.php @@ -160,7 +160,12 @@ public function login(#[\SensitiveParameter] string $username, #[\SensitiveParam if ($user->getId()) { $this->renewSession(); - Mage::getSingleton('adminhtml/url')->renewSecretUrls(); + // Skip the admin-menu cache flush for keyless (RSS basic-auth) logins, which + // re-run login() on every poll: they never render the admin menu, so flushing + // it each poll would rebuild it for every real admin on their next page view. + if (!Mage::getSingleton('adminhtml/url')->getNoSecret()) { + Mage::getSingleton('adminhtml/url')->renewSecretUrls(); + } $this->setIsFirstPageAfterLogin(true); $this->setUser($user); $this->setAcl(Mage::getResourceModel('admin/acl')->loadAcl()); @@ -286,10 +291,8 @@ public function setIsFirstPageAfterLogin($value) /** * The requested url rebuilt with a fresh secret key, for the post-login redirect - * - * @return string */ - protected function _getRequestUri() + protected function _getRequestUri(): string { return Mage::getSingleton('adminhtml/url')->getUrl('*/*/*', ['_current' => true]); } diff --git a/app/code/core/Mage/Adminhtml/Model/Url.php b/app/code/core/Mage/Adminhtml/Model/Url.php index dd7b707e3c..baf72212c1 100644 --- a/app/code/core/Mage/Adminhtml/Model/Url.php +++ b/app/code/core/Mage/Adminhtml/Model/Url.php @@ -100,11 +100,14 @@ public function getUrl($routePath = null, $routeParams = null) * * @param string $controller Controller name * @param string $action Action name + * @param string $formKey Form key to salt with; defaults to the current session's form key. + * Pass it explicitly to mint a key for a different session (e.g. tests + * deep-linking with a browser session's form key). * @return string */ - public function getSecretKey($controller = null, $action = null) + public function getSecretKey($controller = null, $action = null, ?string $formKey = null) { - $salt = Mage::getSingleton('core/session')->getFormKey(); + $salt = $formKey ?? Mage::getSingleton('core/session')->getFormKey(); // Validate against what the user actually requested: after _forward() the dispatched // names change (e.g. catalog_category/index forwards to edit) but the URL's key was diff --git a/app/code/core/Mage/Adminhtml/controllers/Api/OrphanedResourceController.php b/app/code/core/Mage/Adminhtml/controllers/Api/OrphanedResourceController.php index 5aa13aa575..83340ac804 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Api/OrphanedResourceController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Api/OrphanedResourceController.php @@ -68,5 +68,4 @@ public function massDeleteAction(): void $this->_redirect('*/*/'); } - } diff --git a/app/code/core/Mage/Adminhtml/controllers/Catalog/CategoryController.php b/app/code/core/Mage/Adminhtml/controllers/Catalog/CategoryController.php index aa29298343..750905bbf3 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Catalog/CategoryController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Catalog/CategoryController.php @@ -565,5 +565,4 @@ public function processDynamicAction(): void $this->_redirect('*/*/edit', ['id' => $categoryId]); } - } diff --git a/app/code/core/Mage/Adminhtml/controllers/Catalog/SearchController.php b/app/code/core/Mage/Adminhtml/controllers/Catalog/SearchController.php index bae071e0c3..56ab308d61 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Catalog/SearchController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Catalog/SearchController.php @@ -183,5 +183,4 @@ public function massDeleteAction(): void $this->_redirect('*/*/index'); } - } diff --git a/app/code/core/Mage/Adminhtml/controllers/Newsletter/TemplateController.php b/app/code/core/Mage/Adminhtml/controllers/Newsletter/TemplateController.php index 657c50a893..0c78844505 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Newsletter/TemplateController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Newsletter/TemplateController.php @@ -259,5 +259,4 @@ public function wysiwygVariableAction(): void ), ); } - } diff --git a/app/code/core/Mage/Adminhtml/controllers/Permissions/BlockController.php b/app/code/core/Mage/Adminhtml/controllers/Permissions/BlockController.php index c4003d11e7..1a36583143 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Permissions/BlockController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Permissions/BlockController.php @@ -189,5 +189,4 @@ public function blockGridAction(): void ->createBlock('adminhtml/permissions_block_grid') ->toHtml()); } - } diff --git a/app/code/core/Mage/Adminhtml/controllers/Permissions/OrphanedResourceController.php b/app/code/core/Mage/Adminhtml/controllers/Permissions/OrphanedResourceController.php index afa6cc361b..13b8f415e5 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Permissions/OrphanedResourceController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Permissions/OrphanedResourceController.php @@ -69,5 +69,4 @@ public function massDeleteAction(): void $this->_redirect('*/*/'); } - } diff --git a/app/code/core/Mage/Adminhtml/controllers/Sales/OrderController.php b/app/code/core/Mage/Adminhtml/controllers/Sales/OrderController.php index f95e78c3b0..98a38c22ba 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Sales/OrderController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Sales/OrderController.php @@ -804,5 +804,4 @@ public function guestOrderEmailChangeAction(): void $this->_redirect('*/*/view', ['order_id' => $orderId]); } - } diff --git a/app/code/core/Mage/Adminhtml/controllers/Tax/RuleController.php b/app/code/core/Mage/Adminhtml/controllers/Tax/RuleController.php index db9f07634d..31029686fc 100644 --- a/app/code/core/Mage/Adminhtml/controllers/Tax/RuleController.php +++ b/app/code/core/Mage/Adminhtml/controllers/Tax/RuleController.php @@ -212,5 +212,4 @@ protected function _initAction() ; return $this; } - } diff --git a/app/code/core/Mage/Core/sql/core_setup/upgrade-2.1.0-2.1.1.php b/app/code/core/Mage/Core/sql/core_setup/upgrade-2.1.0-2.1.1.php index a9ea64c390..755b6fb637 100644 --- a/app/code/core/Mage/Core/sql/core_setup/upgrade-2.1.0-2.1.1.php +++ b/app/code/core/Mage/Core/sql/core_setup/upgrade-2.1.0-2.1.1.php @@ -6,18 +6,14 @@ * @package Mage_Core */ +declare(strict_types=1); + /** @var Mage_Core_Model_Resource_Setup $this */ $installer = $this; $installer->startSetup(); // The "Add Secret Key to URLs" toggle is gone: admin urls always carry the secret key now, // so any stored override (including a 0 that used to disable it) is dead configuration. -$connection = $installer->getConnection(); -$connection->delete( - $this->getTable('core_config_data'), - $connection->prepareSqlCondition('path', [ - 'like' => 'admin/security/use_form_key', - ]), -); +$installer->deleteConfigData('admin/security/use_form_key'); $installer->endSetup(); diff --git a/app/code/core/Mage/Oauth/Block/Adminhtml/Oauth/Authorize/Button.php b/app/code/core/Mage/Oauth/Block/Adminhtml/Oauth/Authorize/Button.php index 322db89382..34cca4d80d 100644 --- a/app/code/core/Mage/Oauth/Block/Adminhtml/Oauth/Authorize/Button.php +++ b/app/code/core/Mage/Oauth/Block/Adminhtml/Oauth/Authorize/Button.php @@ -12,6 +12,17 @@ class Mage_Oauth_Block_Adminhtml_Oauth_Authorize_Button extends Mage_Oauth_Block_Authorize_ButtonBaseAbstract { + /** + * Build confirm/reject urls through the admin url model so they carry the per-action secret + * key. This button renders inside the logged-in admin session, so the state-changing confirm + * and reject actions can validate that key instead of being exempted as public. + */ + #[\Override] + protected function _getUrlModelClass() + { + return 'adminhtml/url'; + } + /** * Retrieve confirm authorization url path * diff --git a/app/code/core/Mage/Oauth/controllers/Adminhtml/Oauth/AuthorizeController.php b/app/code/core/Mage/Oauth/controllers/Adminhtml/Oauth/AuthorizeController.php index 2945141a45..de22fe7a57 100644 --- a/app/code/core/Mage/Oauth/controllers/Adminhtml/Oauth/AuthorizeController.php +++ b/app/code/core/Mage/Oauth/controllers/Adminhtml/Oauth/AuthorizeController.php @@ -18,11 +18,13 @@ class Mage_Oauth_Adminhtml_Oauth_AuthorizeController extends Mage_Adminhtml_Cont protected $_sessionName = 'admin/session'; /** - * Array of actions which can be processed without secret key validation + * Only the entry pages are public: an external consumer redirects the admin's browser here + * with an oauth_token and no key can exist yet. The state-changing confirm/reject actions are + * reached from the in-session button form, which mints a valid secret key, so they validate it. * * @var array */ - public $_publicActions = ['index', 'simple', 'confirm', 'confirmSimple','reject', 'rejectSimple']; + public $_publicActions = ['index', 'simple']; /** * Disable showing of login form diff --git a/app/code/core/Maho/CatalogLinkRule/controllers/Adminhtml/Cataloglinkrule/RuleController.php b/app/code/core/Maho/CatalogLinkRule/controllers/Adminhtml/Cataloglinkrule/RuleController.php index afab7252a9..45924afcf7 100644 --- a/app/code/core/Maho/CatalogLinkRule/controllers/Adminhtml/Cataloglinkrule/RuleController.php +++ b/app/code/core/Maho/CatalogLinkRule/controllers/Adminhtml/Cataloglinkrule/RuleController.php @@ -232,5 +232,4 @@ private function _renderRuleHtml(string $prefix): void $this->getResponse()->setBody($model->asHtmlRecursive()); } - } diff --git a/app/code/core/Maho/FeedManager/Block/Adminhtml/Feed/Edit.php b/app/code/core/Maho/FeedManager/Block/Adminhtml/Feed/Edit.php index 50d675ac18..f52abd4aca 100644 --- a/app/code/core/Maho/FeedManager/Block/Adminhtml/Feed/Edit.php +++ b/app/code/core/Maho/FeedManager/Block/Adminhtml/Feed/Edit.php @@ -79,6 +79,24 @@ protected function _getFeedIdForJs(): string return $id ? (string) $id : 'null'; } + /** + * Read and clear the one-shot "generate this feed after save" flag for the current feed. + */ + protected function _consumeAutoGenerateFlag(): bool + { + $feedId = (int) $this->_getFeed()->getId(); + if (!$feedId) { + return false; + } + $session = Mage::getSingleton('adminhtml/session'); + $armedFor = (int) $session->getData('feed_generate_after_save'); + if ($armedFor === $feedId) { + $session->unsetData('feed_generate_after_save'); + return true; + } + return false; + } + /** * Get form scripts including batch generation JavaScript */ @@ -91,6 +109,11 @@ protected function _getFormScripts(): string $resetUrl = $this->getUrl('*/*/forceReset'); $uploadUrl = $this->getUrl('*/*/upload'); + // Auto-generation is armed only by the save/generate actions (form key or secret key + // protected), consumed once here. A keyless GET to the public edit action cannot set it, + // so it can never trigger generation cross-site. + $autoGenerate = $this->_consumeAutoGenerateFlag() ? 'true' : 'false'; + // Check if feed has a destination configured $hasDestination = (bool) $this->_getFeed()->getDestinationId(); $feedName = Mage::helper('core')->jsonEncode($this->_getFeed()->getName() ?: ''); @@ -331,18 +354,12 @@ function saveAndContinueEdit() { } }; - // Auto-start generation if URL has generate/1 parameter (after save) + // Auto-start generation after a save/generate that armed the one-shot flag document.addEventListener('DOMContentLoaded', function() { - // Check for both path-based (/generate/1/) and query string (?generate=1) formats - var shouldGenerate = window.location.pathname.indexOf('/generate/1') !== -1 || - window.location.search.indexOf('generate=1') !== -1; + var shouldGenerate = {$autoGenerate}; var feedId = {$this->_getFeedIdForJs()}; if (shouldGenerate && feedId) { - // Remove the generate param from URL to prevent re-triggering on refresh - var newUrl = window.location.pathname.replace(/\\/generate\\/1\\/?/, '/'); - window.history.replaceState({}, '', newUrl); - // Start generation after a short delay to let the page load // Use force=true to clean up any stuck jobs from the previous save attempt setTimeout(function() { diff --git a/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php b/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php index 77e9a2fe5d..ec96f1d497 100644 --- a/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php +++ b/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php @@ -148,7 +148,8 @@ public function saveAction(): void // Check if we should generate after save if ($this->getRequest()->getParam('generate_after_save')) { - $this->_redirect('*/*/edit', ['id' => $feed->getId(), 'generate' => '1']); + $this->_getSession()->setData('feed_generate_after_save', (int) $feed->getId()); + $this->_redirect('*/*/edit', ['id' => $feed->getId()]); return; } @@ -348,7 +349,8 @@ public function generateAction(): void return; } - $this->_redirect('*/*/edit', ['id' => $id, 'generate' => '1']); + $this->_getSession()->setData('feed_generate_after_save', $id); + $this->_redirect('*/*/edit', ['id' => $id]); } /** diff --git a/tests/Pest.php b/tests/Pest.php index cf00f910d6..7df5bd0527 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -460,7 +460,7 @@ function adminPathWithSecretKey(object $page, string $path): string $action = $segments[2] ?? 'index'; $extra = array_slice($segments, 3); - $secretKey = Mage::helper('core')->getHash(strtolower($controller) . strtolower($action) . $formKey); + $secretKey = Mage::getSingleton('adminhtml/url')->getSecretKey($controller, $action, $formKey); $path = '/' . implode('/', array_merge([$front, $controller, $action], $extra, ['key', $secretKey])) . '/'; return $path . ($query !== null ? '?' . $query : ''); From 0b986de465a4dc654bb2a0ed1e12f6851f12be2f Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Tue, 11 Aug 2026 20:23:54 +0100 Subject: [PATCH 6/6] Expired the feed auto-generate flag and removed redundant work found in review - The save-and-generate session flag now stores the feed id with a timestamp; the edit page consumes it clear-on-read and ignores flags older than 60 seconds, so a lost redirect can no longer start a stale generation on a later, unrelated visit. - login() no longer builds the keyed redirect url that request-less (RSS basic-auth) logins always discard, and its menu-flush guard uses useSecretKey() instead of respelling the same predicate. - preDispatch() tracks one key-validity flag instead of two mutually exclusive ones. - getSecretKey() parses the request path only when controller or action is missing. - The four browser tests share a single adminLoginAndVisit() helper, and the secret key url segment comes from SECRET_KEY_PARAM_NAME instead of a hard-coded 'key'. --- app/code/core/Mage/Admin/Model/Session.php | 9 +++++--- .../core/Mage/Adminhtml/Controller/Action.php | 11 +++++----- app/code/core/Mage/Adminhtml/Model/Url.php | 20 +++++++++--------- .../Adminhtml/Oauth/Authorize/Button.php | 2 +- .../FeedManager/Block/Adminhtml/Feed/Edit.php | 20 +++++++++--------- .../Adminhtml/Feedmanager/FeedController.php | 10 +++++++-- .../Adminhtml/AdminUrlFormKeyTest.php | 15 +++++++------ tests/Browser/AdminQueryStringUrlsTest.php | 11 +--------- tests/Browser/AdminReindexDialogTest.php | 16 ++++++-------- .../Browser/AdminRelatedProductsGridTest.php | 15 ++++++------- tests/Browser/AdminStoreSwitcherTest.php | 16 ++++++-------- tests/Pest.php | 21 ++++++++++++++++++- 12 files changed, 88 insertions(+), 78 deletions(-) diff --git a/app/code/core/Mage/Admin/Model/Session.php b/app/code/core/Mage/Admin/Model/Session.php index a3dbb40d58..9d12d0dc54 100644 --- a/app/code/core/Mage/Admin/Model/Session.php +++ b/app/code/core/Mage/Admin/Model/Session.php @@ -163,7 +163,7 @@ public function login(#[\SensitiveParameter] string $username, #[\SensitiveParam // Skip the admin-menu cache flush for keyless (RSS basic-auth) logins, which // re-run login() on every poll: they never render the admin menu, so flushing // it each poll would rebuild it for every real admin on their next page view. - if (!Mage::getSingleton('adminhtml/url')->getNoSecret()) { + if (Mage::getSingleton('adminhtml/url')->useSecretKey()) { Mage::getSingleton('adminhtml/url')->renewSecretUrls(); } $this->setIsFirstPageAfterLogin(true); @@ -173,8 +173,11 @@ public function login(#[\SensitiveParameter] string $username, #[\SensitiveParam Mage::getSingleton('adminhtml/session')->setLocale($backendLocale); } - $alternativeUrl = $this->_getRequestUri(); - $redirectUrl = $this->_urlPolicy->getRedirectUrl($user, $request, $alternativeUrl); + // The redirect policy bails out on an empty request (RSS basic-auth logins), + // so do not pay for building the keyed alternative url on that path. + $redirectUrl = $request + ? $this->_urlPolicy->getRedirectUrl($user, $request, $this->_getRequestUri()) + : null; if ($redirectUrl) { Mage::dispatchEvent('admin_session_user_login_success', ['user' => $user]); $this->_response->clearHeaders() diff --git a/app/code/core/Mage/Adminhtml/Controller/Action.php b/app/code/core/Mage/Adminhtml/Controller/Action.php index 3a0a53fcab..3f27dfa717 100644 --- a/app/code/core/Mage/Adminhtml/Controller/Action.php +++ b/app/code/core/Mage/Adminhtml/Controller/Action.php @@ -158,19 +158,18 @@ public function preDispatch() Mage::dispatchEvent('adminhtml_controller_action_predispatch_start', []); parent::preDispatch(); - $isValidFormKey = true; - $isValidSecretKey = true; + $isValidKey = true; $keyErrorMsg = ''; if (Mage::getSingleton('admin/session')->isLoggedIn()) { if ($this->getRequest()->isPost()) { - $isValidFormKey = $this->_validateFormKey(); + $isValidKey = $this->_validateFormKey(); $keyErrorMsg = Mage::helper('adminhtml')->__('Invalid Form Key. Please refresh the page.'); } else { - $isValidSecretKey = $this->_validateSecretKey(); + $isValidKey = $this->_validateSecretKey(); $keyErrorMsg = Mage::helper('adminhtml')->__('Invalid Secret Key. Please refresh the page.'); } } - if (!$isValidFormKey || !$isValidSecretKey) { + if (!$isValidKey) { $this->setFlag('', self::FLAG_NO_DISPATCH, true); $this->setFlag('', self::FLAG_NO_POST_DISPATCH, true); if ($this->getRequest()->getParam('isAjax', false) || $this->getRequest()->getParam('ajax', false)) { @@ -179,7 +178,7 @@ public function preDispatch() 'message' => $keyErrorMsg, ])); } else { - if (!$isValidFormKey) { + if ($this->getRequest()->isPost()) { Mage::getSingleton('adminhtml/session')->addError($keyErrorMsg); } $this->_redirect(Mage::getSingleton('admin/session')->getUser()->getStartupPageUrl()); diff --git a/app/code/core/Mage/Adminhtml/Model/Url.php b/app/code/core/Mage/Adminhtml/Model/Url.php index baf72212c1..ee736a0fa3 100644 --- a/app/code/core/Mage/Adminhtml/Model/Url.php +++ b/app/code/core/Mage/Adminhtml/Model/Url.php @@ -96,13 +96,13 @@ public function getUrl($routePath = null, $routeParams = null) } /** - * Generate secret key for controller and action based on form key + * Generate secret key for controller and action based on form key. + * + * $formKey overrides the current session's form key as the salt; pass it to mint a key + * for a different session (e.g. tests deep-linking with a browser session's form key). * * @param string $controller Controller name * @param string $action Action name - * @param string $formKey Form key to salt with; defaults to the current session's form key. - * Pass it explicitly to mint a key for a different session (e.g. tests - * deep-linking with a browser session's form key). * @return string */ public function getSecretKey($controller = null, $action = null, ?string $formKey = null) @@ -115,14 +115,14 @@ public function getSecretKey($controller = null, $action = null, ?string $formKe // Dispatched names come next; positional path parsing assumes the classic // admin// shape and mis-slices legacy:migrate-routes routes that // carry an extra frontName segment, so it stays only as a pre-dispatch fallback. - $p = explode('/', trim($this->getRequest()->getOriginalPathInfo(), '/')); - if (!$controller) { - $controller = $this->getRequest()->getBeforeForwardInfo('controller_name') + if (!$controller || !$action) { + $p = explode('/', trim($this->getRequest()->getOriginalPathInfo(), '/')); + $controller = $controller + ?: $this->getRequest()->getBeforeForwardInfo('controller_name') ?: $this->getRequest()->getControllerName() ?: (empty($p[1]) ? null : $p[1]); - } - if (!$action) { - $action = $this->getRequest()->getBeforeForwardInfo('action_name') + $action = $action + ?: $this->getRequest()->getBeforeForwardInfo('action_name') ?: $this->getRequest()->getActionName() ?: (empty($p[2]) ? null : $p[2]); } diff --git a/app/code/core/Mage/Oauth/Block/Adminhtml/Oauth/Authorize/Button.php b/app/code/core/Mage/Oauth/Block/Adminhtml/Oauth/Authorize/Button.php index 34cca4d80d..44dfd61e15 100644 --- a/app/code/core/Mage/Oauth/Block/Adminhtml/Oauth/Authorize/Button.php +++ b/app/code/core/Mage/Oauth/Block/Adminhtml/Oauth/Authorize/Button.php @@ -18,7 +18,7 @@ class Mage_Oauth_Block_Adminhtml_Oauth_Authorize_Button extends Mage_Oauth_Block * and reject actions can validate that key instead of being exempted as public. */ #[\Override] - protected function _getUrlModelClass() + protected function _getUrlModelClass(): string { return 'adminhtml/url'; } diff --git a/app/code/core/Maho/FeedManager/Block/Adminhtml/Feed/Edit.php b/app/code/core/Maho/FeedManager/Block/Adminhtml/Feed/Edit.php index f52abd4aca..124bfb184a 100644 --- a/app/code/core/Maho/FeedManager/Block/Adminhtml/Feed/Edit.php +++ b/app/code/core/Maho/FeedManager/Block/Adminhtml/Feed/Edit.php @@ -12,6 +12,9 @@ class Maho_FeedManager_Block_Adminhtml_Feed_Edit extends Mage_Adminhtml_Block_Wi { use Maho_FeedManager_Block_Adminhtml_Feed_Edit_FeedRegistryTrait; + /** One-shot session flag armed by the save/generate actions: ['id' => feed id, 'ts' => armed at] */ + public const AUTO_GENERATE_FLAG = 'feed_generate_after_save'; + public function __construct() { $this->_objectId = 'id'; @@ -81,20 +84,17 @@ protected function _getFeedIdForJs(): string /** * Read and clear the one-shot "generate this feed after save" flag for the current feed. + * The arming redirect lands immediately, so an old flag is stale (its response was lost) + * and must not start a generation on a later, unrelated visit to the edit page. */ protected function _consumeAutoGenerateFlag(): bool { $feedId = (int) $this->_getFeed()->getId(); - if (!$feedId) { - return false; - } - $session = Mage::getSingleton('adminhtml/session'); - $armedFor = (int) $session->getData('feed_generate_after_save'); - if ($armedFor === $feedId) { - $session->unsetData('feed_generate_after_save'); - return true; - } - return false; + $flag = Mage::getSingleton('adminhtml/session')->getData(self::AUTO_GENERATE_FLAG, true); + return $feedId + && is_array($flag) + && (int) ($flag['id'] ?? 0) === $feedId + && time() - (int) ($flag['ts'] ?? 0) <= 60; } /** diff --git a/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php b/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php index ec96f1d497..0e0e17505b 100644 --- a/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php +++ b/app/code/core/Maho/FeedManager/controllers/Adminhtml/Feedmanager/FeedController.php @@ -148,7 +148,10 @@ public function saveAction(): void // Check if we should generate after save if ($this->getRequest()->getParam('generate_after_save')) { - $this->_getSession()->setData('feed_generate_after_save', (int) $feed->getId()); + $this->_getSession()->setData( + Maho_FeedManager_Block_Adminhtml_Feed_Edit::AUTO_GENERATE_FLAG, + ['id' => (int) $feed->getId(), 'ts' => time()], + ); $this->_redirect('*/*/edit', ['id' => $feed->getId()]); return; } @@ -349,7 +352,10 @@ public function generateAction(): void return; } - $this->_getSession()->setData('feed_generate_after_save', $id); + $this->_getSession()->setData( + Maho_FeedManager_Block_Adminhtml_Feed_Edit::AUTO_GENERATE_FLAG, + ['id' => $id, 'ts' => time()], + ); $this->_redirect('*/*/edit', ['id' => $id]); } diff --git a/tests/Backend/Integration/Adminhtml/AdminUrlFormKeyTest.php b/tests/Backend/Integration/Adminhtml/AdminUrlFormKeyTest.php index c7cf45f80b..3bc5d1919e 100644 --- a/tests/Backend/Integration/Adminhtml/AdminUrlFormKeyTest.php +++ b/tests/Backend/Integration/Adminhtml/AdminUrlFormKeyTest.php @@ -64,9 +64,10 @@ function adminUrlBlock(): Mage_Adminhtml_Block_Template it('carries the secret key on the shared form container delete url', function () { /** @var Mage_Adminhtml_Block_Widget_Form_Container $block */ $block = Mage::app()->getLayout()->createBlock('adminhtml/widget_form_container'); + $url = $block->getDeleteUrl(); - expect($block->getDeleteUrl())->toContain('/key/') - ->and($block->getDeleteUrl())->not->toContain('form_key'); + expect($url)->toContain('/key/') + ->and($url)->not->toContain('form_key'); }); it('carries the secret key on the credit memo cancel and void urls', function (string $method) { @@ -75,14 +76,16 @@ function adminUrlBlock(): Mage_Adminhtml_Block_Template ->setState(Mage_Sales_Model_Order_Creditmemo::STATE_CANCELED)); $block = Mage::app()->getLayout()->createBlock('adminhtml/sales_order_creditmemo_view'); + $url = $block->{$method}(); - expect($block->{$method}())->toContain('/key/') - ->and($block->{$method}())->not->toContain('form_key'); + expect($url)->toContain('/key/') + ->and($url)->not->toContain('form_key'); })->with(['getCancelUrl', 'getVoidUrl']); it('carries the secret key on the category tree delete url', function () { $block = new Mage_Adminhtml_Block_Catalog_Category_Edit_Form(); + $url = $block->getDeleteUrl(); - expect($block->getDeleteUrl())->toContain('/key/') - ->and($block->getDeleteUrl())->not->toContain('form_key'); + expect($url)->toContain('/key/') + ->and($url)->not->toContain('form_key'); }); diff --git a/tests/Browser/AdminQueryStringUrlsTest.php b/tests/Browser/AdminQueryStringUrlsTest.php index b1db938a29..c00d706b46 100644 --- a/tests/Browser/AdminQueryStringUrlsTest.php +++ b/tests/Browser/AdminQueryStringUrlsTest.php @@ -7,7 +7,6 @@ declare(strict_types=1); -use Tests\Browser\MahoServer; use Tests\MahoBrowserTestCase; uses(MahoBrowserTestCase::class)->group('browser'); @@ -70,15 +69,7 @@ function createQueryUrlsAdmin(): void /** Log in and open an admin page, waiting for one of its elements. */ function visitAdminPage(string $path, string $selector): object { - $page = visit(MahoServer::baseUrl() . '/admin') - ->fill('#username', QUERY_URLS_ADMIN_USER) - ->fill('#login', QUERY_URLS_ADMIN_PASSWORD) - ->click('#step1 input[type="submit"]'); - - waitForPageLoad($page, '.nav-bar:visible'); - $page->navigate(MahoServer::baseUrl() . adminPathWithSecretKey($page, $path)); - - return waitForPageLoad($page, $selector); + return adminLoginAndVisit(QUERY_URLS_ADMIN_USER, QUERY_URLS_ADMIN_PASSWORD, $path, $selector); } /** diff --git a/tests/Browser/AdminReindexDialogTest.php b/tests/Browser/AdminReindexDialogTest.php index 26c3d94da7..dd58e8ef23 100644 --- a/tests/Browser/AdminReindexDialogTest.php +++ b/tests/Browser/AdminReindexDialogTest.php @@ -7,7 +7,6 @@ declare(strict_types=1); -use Tests\Browser\MahoServer; use Tests\MahoBrowserTestCase; uses(MahoBrowserTestCase::class)->group('browser'); @@ -58,15 +57,12 @@ function createReindexAdmin(): void function loginToIndexManagement(): object { - $page = visit(MahoServer::baseUrl() . '/admin') - ->fill('#username', REINDEX_ADMIN_USER) - ->fill('#login', REINDEX_ADMIN_PASSWORD) - ->click('#step1 input[type="submit"]'); - - waitForPageLoad($page, '.nav-bar:visible'); - $page->navigate(MahoServer::baseUrl() . adminPathWithSecretKey($page, '/admin/process/list')); - - return waitForPageLoad($page, '#indexer_processes_grid_table:visible'); + return adminLoginAndVisit( + REINDEX_ADMIN_USER, + REINDEX_ADMIN_PASSWORD, + '/admin/process/list', + '#indexer_processes_grid_table:visible', + ); } it('reports a single index through the progress dialog', function () { diff --git a/tests/Browser/AdminRelatedProductsGridTest.php b/tests/Browser/AdminRelatedProductsGridTest.php index 9304d7b62c..0db89ab468 100644 --- a/tests/Browser/AdminRelatedProductsGridTest.php +++ b/tests/Browser/AdminRelatedProductsGridTest.php @@ -7,7 +7,6 @@ declare(strict_types=1); -use Tests\Browser\MahoServer; use Tests\MahoBrowserTestCase; uses(MahoBrowserTestCase::class)->group('browser'); @@ -142,14 +141,12 @@ function visitRelatedProductsTab(int $parentId): object { createRelatedGridAdminUser(); - $page = visit(MahoServer::baseUrl() . '/admin') - ->fill('#username', RELATED_GRID_ADMIN_USERNAME) - ->fill('#login', RELATED_GRID_ADMIN_PASSWORD) - ->click('#step1 input[type="submit"]'); - - waitForPageLoad($page, '.nav-bar:visible'); - $page->navigate(MahoServer::baseUrl() . adminPathWithSecretKey($page, '/admin/catalog_product/edit/id/' . $parentId)); - waitForPageLoad($page, '#product_info_tabs_related'); + $page = adminLoginAndVisit( + RELATED_GRID_ADMIN_USERNAME, + RELATED_GRID_ADMIN_PASSWORD, + '/admin/catalog_product/edit/id/' . $parentId, + '#product_info_tabs_related', + ); // An inactive tab keeps its content attached, so `:visible` is what distinguishes the // tab having opened from the grid merely existing in the page. diff --git a/tests/Browser/AdminStoreSwitcherTest.php b/tests/Browser/AdminStoreSwitcherTest.php index fbce45c2ce..8fbf514a66 100644 --- a/tests/Browser/AdminStoreSwitcherTest.php +++ b/tests/Browser/AdminStoreSwitcherTest.php @@ -7,7 +7,6 @@ declare(strict_types=1); -use Tests\Browser\MahoServer; use Tests\MahoBrowserTestCase; uses(MahoBrowserTestCase::class)->group('browser'); @@ -76,15 +75,12 @@ function storeSwitcherStoreId(): int /** Log in and open the product grid with a query string on it. */ function visitProductGridWithQuery(): object { - $page = visit(MahoServer::baseUrl() . '/admin') - ->fill('#username', STORE_SWITCHER_ADMIN_USER) - ->fill('#login', STORE_SWITCHER_ADMIN_PASSWORD) - ->click('#step1 input[type="submit"]'); - - waitForPageLoad($page, '.nav-bar:visible'); - $page->navigate(MahoServer::baseUrl() . adminPathWithSecretKey($page, '/admin/catalog_product/index/?' . STORE_SWITCHER_QUERY)); - - return waitForPageLoad($page, '#store_switcher'); + return adminLoginAndVisit( + STORE_SWITCHER_ADMIN_USER, + STORE_SWITCHER_ADMIN_PASSWORD, + '/admin/catalog_product/index/?' . STORE_SWITCHER_QUERY, + '#store_switcher', + ); } /** diff --git a/tests/Pest.php b/tests/Pest.php index 7df5bd0527..bac50da28d 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -7,6 +7,7 @@ declare(strict_types=1); +use Tests\Browser\MahoServer; use Tests\Helpers\ApiV2Helper; // Autoload module API classes (Mage\Foo\Api\Bar, Maho\Foo\Api\Bar) in tests. @@ -462,10 +463,28 @@ function adminPathWithSecretKey(object $page, string $path): string $secretKey = Mage::getSingleton('adminhtml/url')->getSecretKey($controller, $action, $formKey); - $path = '/' . implode('/', array_merge([$front, $controller, $action], $extra, ['key', $secretKey])) . '/'; + $keySegments = [Mage_Adminhtml_Model_Url::SECRET_KEY_PARAM_NAME, $secretKey]; + $path = '/' . implode('/', array_merge([$front, $controller, $action], $extra, $keySegments)) . '/'; return $path . ($query !== null ? '?' . $query : ''); } +/** + * Log in to the admin and navigate straight to $path with a minted secret key, returning + * the page once $readySelector is present. + */ +function adminLoginAndVisit(string $username, string $password, string $path, string $readySelector): object +{ + $page = visit(MahoServer::baseUrl() . '/admin') + ->fill('#username', $username) + ->fill('#login', $password) + ->click('#step1 input[type="submit"]'); + + waitForPageLoad($page, '.nav-bar:visible'); + $page->navigate(MahoServer::baseUrl() . adminPathWithSecretKey($page, $path)); + + return waitForPageLoad($page, $readySelector); +} + /* |-------------------------------------------------------------------------- | Message Queue Helper Functions