Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions .phpstan.dist.baseline.neon
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 1 addition & 2 deletions app/code/core/Mage/Admin/Model/Redirectpolicy.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) ?
Expand Down
25 changes: 11 additions & 14 deletions app/code/core/Mage/Admin/Model/Session.php
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,9 @@ public function login(#[\SensitiveParameter] string $username, #[\SensitiveParam
if ($user->getId()) {
$this->renewSession();

// 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')->useSecretKey()) {
Mage::getSingleton('adminhtml/url')->renewSecretUrls();
}
Expand All @@ -170,8 +173,11 @@ public function login(#[\SensitiveParameter] string $username, #[\SensitiveParam
Mage::getSingleton('adminhtml/session')->setLocale($backendLocale);
}

$alternativeUrl = $this->_getRequestUri($request);
$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()
Expand Down Expand Up @@ -287,20 +293,11 @@ public function setIsFirstPageAfterLogin($value)
}

/**
* Custom REQUEST_URI logic
*
* @param Mage_Core_Controller_Request_Http $request
* @return string|null
* The requested url rebuilt with a fresh secret key, for the post-login redirect
*/
protected function _getRequestUri($request = null)
protected function _getRequestUri(): string
{
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]);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
]);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ public function getIsGrouped()
*/
public function getDeleteUrl()
{
return $this->getUrlSecure('*/*/delete', ['_current' => true]);
return $this->getUrl('*/*/delete', ['_current' => true]);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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')]);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion app/code/core/Mage/Adminhtml/Block/Review/Edit.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
]),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()]);
}

/**
Expand All @@ -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()]);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,23 +149,23 @@ public function getBackUrl()
*/
public function getCaptureUrl()
{
return $this->getUrlSecure('*/*/capture', ['invoice_id' => $this->getInvoice()->getId()]);
return $this->getUrl('*/*/capture', ['invoice_id' => $this->getInvoice()->getId()]);
}

/**
* @return string
*/
public function getVoidUrl()
{
return $this->getUrlSecure('*/*/void', ['invoice_id' => $this->getInvoice()->getId()]);
return $this->getUrl('*/*/void', ['invoice_id' => $this->getInvoice()->getId()]);
}

/**
* @return string
*/
public function getCancelUrl()
{
return $this->getUrlSecure('*/*/cancel', ['invoice_id' => $this->getInvoice()->getId()]);
return $this->getUrl('*/*/cancel', ['invoice_id' => $this->getInvoice()->getId()]);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()]);
}

/**
Expand Down
6 changes: 3 additions & 3 deletions app/code/core/Mage/Adminhtml/Block/Sales/Order/View.php
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ public function getEmailUrl()
*/
public function getCancelUrl()
{
return $this->getUrlSecure('*/*/cancel');
return $this->getUrl('*/*/cancel');
}

/**
Expand All @@ -284,15 +284,15 @@ public function getCreditmemoUrl()
*/
public function getHoldUrl()
{
return $this->getUrlSecure('*/*/hold');
return $this->getUrl('*/*/hold');
}

/**
* @return string
*/
public function getUnholdUrl()
{
return $this->getUrlSecure('*/*/unhold');
return $this->getUrl('*/*/unhold');
}

/**
Expand Down
2 changes: 1 addition & 1 deletion app/code/core/Mage/Adminhtml/Block/System/Design/Edit.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public function getDesignChangeId()
*/
public function getDeleteUrl()
{
return $this->getUrlSecure('*/*/delete', ['id' => $this->getDesignChangeId()]);
return $this->getUrl('*/*/delete', ['id' => $this->getDesignChangeId()]);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ public function isTextType()
*/
public function getDeleteUrl()
{
return $this->getUrlSecure('*/*/delete', ['_current' => true]);
return $this->getUrl('*/*/delete', ['_current' => true]);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion app/code/core/Mage/Adminhtml/Block/System/Store/Edit.php
Original file line number Diff line number Diff line change
Expand Up @@ -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()],
);
Expand Down
2 changes: 1 addition & 1 deletion app/code/core/Mage/Adminhtml/Block/Urlrewrite/Edit.php
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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),
]);
}
Expand Down
50 changes: 7 additions & 43 deletions app/code/core/Mage/Adminhtml/Controller/Action.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -165,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() || $this->_checkIsForcedFormKeyAction()) {
$isValidFormKey = $this->_validateFormKey();
if ($this->getRequest()->isPost()) {
$isValidKey = $this->_validateFormKey();
$keyErrorMsg = Mage::helper('adminhtml')->__('Invalid Form Key. Please refresh the page.');
} elseif (Mage::getSingleton('adminhtml/url')->useSecretKey()) {
$isValidSecretKey = $this->_validateSecretKey();
} else {
$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)) {
Expand All @@ -186,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());
Expand Down Expand Up @@ -362,34 +354,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
*
Expand Down
11 changes: 0 additions & 11 deletions app/code/core/Mage/Adminhtml/Helper/Data.php
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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
*/
Expand Down

This file was deleted.

Loading
Loading