OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.
https://www.openmage.org
Open Software License 3.0
870 stars 436 forks source link

Hardcoded use_admin_sid value #1887

Open addison74 opened 2 years ago

addison74 commented 2 years ago

In version 1.9.4.5 the Magento team brought a change that created issues for me these days with an extension. In /app/code/core/Mage/Admin/Model/Session.php the function logoutIndirect() (Logout user if was logged not from admin) was changed from this

    protected function logoutIndirect()
    {
        $user = $this->getUser();
        if ($user) {
            $extraData = $user->getExtra();
            if (isset($extraData['indirect_login']) && $this->getIndirectLogin()) {
                $this->unsetData('user');
                $this->setIndirectLogin(false);
            }
        }
    }

to this

    protected function logoutIndirect()
    {
        $user = $this->getUser();
        if ($user) {
            $extraData = $user->getExtra();
            if (
                !is_null(Mage::app()->getRequest()->getParam('SID'))
                && !$this->allowAdminSid()
                || isset($extraData['indirect_login'])
                && $this->getIndirectLogin()
            ) {
                $this->unsetData('user');
                $this->setIndirectLogin(false);
            }
        }
    }

I experienced issues when uploading/deleted files in Backend using the extension. The upload/delete buttons make a POST request with a SID parameter in the URL. The result is that my admin session is terminated suddenly, even when I am saving the product.

If we look at the new function above the first part of the if statement is always true because the allowAdminSid() method returns the value from /app/code/core/Mage/Core/etc/config.xml file that is set to 0 by default. It can't even be changed in the Backend interface.

As the upload/delete files POST requests always a SID parameters in the URL as the extension was designed, in order to fix this I am evaluating two approaches:

  1. Adding in extension /etc/config.xml file the following line in default section. I can add in extension config section a field to see the value and set it to Yes.
    <default>
    <web>
            <session>
                <use_admin_sid>0</use_admin_sid>
            </session>
    </web>
  1. Remove from extension files ->addSessionParam() method.

I don't like the value use_admin_sid is hardcoded in a config file. We should do something in OpenMage to help extensions that have not been updated in the past to work even after 1.9.4.5. A configurable value in Backend should be fine (Web/Session Validation Settings/Use SID on Backend (Yes/No).

Till we get this working my question is what shall I do in fixing this extension? To use the config value or remove the method? I am fine with a hardcoded config value. If I remove the method I have to evaluate a possible security breach.

j0ney3 commented 1 year ago

Did you ever make a decision/code change with regards to this issue? We're facing the same trouble and have (for now) reverted to the previous version of protected function logoutIndirect(), however, this is not a great option as it defeats the SID completely.