In admin, url params may get url encoded more than once, fix backend url model

Author: adrian-martinez-interactiv4

app/code/Magento/Backend/Model/Url.php

@@ -5,9 +5,9 @@
  */
 namespace Magento\Backend\Model;
 
+use Magento\Framework\App\ObjectManager;
 use Magento\Framework\Serialize\Serializer\Json;
 use Magento\Framework\Url\HostChecker;
-use Magento\Framework\App\ObjectManager;
 
 /**
  * Class \Magento\Backend\Model\UrlInterface
@@ -179,10 +179,10 @@ protected function _isSecure()
     protected function _setRouteParams(array $data, $unsetOldParams = true)
     {
         if (isset($data['_nosecret'])) {
-            $this->setNoSecret(true);
+            $this->turnOffSecretKey();
             unset($data['_nosecret']);
         } else {
-            $this->setNoSecret(false);
+            $this->turnOnSecretKey();
         }
         unset($data['_scope_to_url']);
         return parent::_setRouteParams($data, $unsetOldParams);
@@ -206,29 +206,33 @@ public function getUrl($routePath = null, $routeParams = null)
             unset($routeParams['_cache_secret_key']);
             $cacheSecretKey = true;
         }
-        $result = parent::getUrl($routePath, $routeParams);
-        if (!$this->useSecretKey()) {
-            return $result;
-        }
+
+        $this->_setRouteParams([]);
         $this->_setRoutePath($routePath);
         $routeName = $this->_getRouteName('*');
         $controllerName = $this->_getControllerName(self::DEFAULT_CONTROLLER_NAME);
         $actionName = $this->_getActionName(self::DEFAULT_ACTION_NAME);
-        if ($cacheSecretKey) {
-            $secret = [self::SECRET_KEY_PARAM_NAME => "\${$routeName}/{$controllerName}/{$actionName}\$"];
-        } else {
-            $secret = [
-                self::SECRET_KEY_PARAM_NAME => $this->getSecretKey($routeName, $controllerName, $actionName),
-            ];
-        }
-        if (is_array($routeParams)) {
-            $routeParams = array_merge($secret, $routeParams);
-        } else {
-            $routeParams = $secret;
+
+        if ($this->useSecretKey()) {
+            if ($cacheSecretKey) {
+                $secret = [self::SECRET_KEY_PARAM_NAME => "\${$routeName}/{$controllerName}/{$actionName}\$"];
+            } else {
+                $secret = [
+                    self::SECRET_KEY_PARAM_NAME => $this->getSecretKey($routeName, $controllerName, $actionName),
+                ];
+            }
+            if (is_array($routeParams)) {
+                $routeParams = array_merge($routeParams, $secret);
+            } else {
+                $routeParams = $secret;
+            }
         }
-        if (is_array($this->_getRouteParams())) {
-            $routeParams = array_merge($this->_getRouteParams(), $routeParams);
+
+        $routePathParams = $this->_getRouteParams();
+        if (is_array($routePathParams)) {
+            $routeParams = array_merge($routePathParams, $routeParams);
         }
+
         return parent::getUrl("{$routeName}/{$controllerName}/{$actionName}", $routeParams);
     }
 

dev/tests/integration/framework/Magento/TestFramework/TestCase/AbstractBackendController.php

@@ -12,6 +12,11 @@
  */
 abstract class AbstractBackendController extends \Magento\TestFramework\TestCase\AbstractController
 {
+    /**
+     * @var \Magento\Backend\Model\UrlInterface
+     */
+    protected $_urlBuilder;
+
     /**
      * @var \Magento\Backend\Model\Auth\Session
      */
@@ -40,8 +45,7 @@ protected function setUp()
     {
         parent::setUp();
 
-        $this->_objectManager->get(\Magento\Backend\Model\UrlInterface::class)->turnOffSecretKey();
-
+        $this->_urlBuilder = $this->_objectManager->get(\Magento\Backend\Model\UrlInterface::class)->turnOffSecretKey();
         $this->_auth = $this->_objectManager->get(\Magento\Backend\Model\Auth::class);
         $this->_session = $this->_auth->getAuthStorage();
         $credentials = $this->_getAdminCredentials();

dev/tests/integration/testsuite/Magento/Catalog/Controller/Adminhtml/Product/Action/AttributeTest.php

@@ -17,32 +17,24 @@ class AttributeTest extends \Magento\TestFramework\TestCase\AbstractBackendContr
      */
     public function testSaveActionRedirectsSuccessfully()
     {
-        $objectManager = \Magento\TestFramework\Helper\Bootstrap::getObjectManager();
-
         /** @var $session \Magento\Backend\Model\Session */
-        $session = $objectManager->get(\Magento\Backend\Model\Session::class);
+        $session = $this->_objectManager->get(\Magento\Backend\Model\Session::class);
         $session->setProductIds([1]);
 
         $this->dispatch('backend/catalog/product_action_attribute/save/store/0');
 
         $this->assertEquals(302, $this->getResponse()->getHttpResponseCode());
-        /** @var \Magento\Backend\Model\UrlInterface $urlBuilder */
-        $urlBuilder = $objectManager->get(\Magento\Framework\UrlInterface::class);
 
         /** @var \Magento\Catalog\Helper\Product\Edit\Action\Attribute $attributeHelper */
-        $attributeHelper = $objectManager->get(\Magento\Catalog\Helper\Product\Edit\Action\Attribute::class);
-        $expectedUrl = $urlBuilder->getUrl(
+        $attributeHelper = $this->_objectManager->get(\Magento\Catalog\Helper\Product\Edit\Action\Attribute::class);
+
+        $this->_urlBuilder->turnOffSecretKey();
+        $expectedUrl = $this->_urlBuilder->getUrl(
             'catalog/product/index',
             ['store' => $attributeHelper->getSelectedStoreId()]
         );
-        $isRedirectPresent = false;
-        foreach ($this->getResponse()->getHeaders() as $header) {
-            if ($header->getFieldName() === 'Location' && strpos($header->getFieldValue(), $expectedUrl) === 0) {
-                $isRedirectPresent = true;
-            }
-        }
-
-        $this->assertTrue($isRedirectPresent);
+
+        $this->assertRedirect($this->stringStartsWith($expectedUrl));
     }
 
     /**

dev/tests/integration/testsuite/Magento/Customer/Controller/Adminhtml/IndexTest.php

@@ -287,6 +287,7 @@ public function testSaveActionExistingCustomerAndExistingAddressData()
         ];
         $this->getRequest()->setPostValue($post);
         $this->getRequest()->setParam('id', 1);
+        $this->_urlBuilder->turnOffSecretKey();
         $this->dispatch('backend/customer/index/save');
 
         /** Check that success message is set */
@@ -324,7 +325,8 @@ public function testSaveActionExistingCustomerAndExistingAddressData()
         $subscriber->loadByCustomerId($customerId);
         $this->assertNotEmpty($subscriber->getId());
         $this->assertEquals(1, $subscriber->getStatus());
-        $this->assertRedirect($this->stringStartsWith($this->_baseControllerUrl . 'index/key/'));
+
+        $this->assertRedirect($this->stringStartsWith($this->_baseControllerUrl . 'index/'));
     }
 
     /**
@@ -353,6 +355,7 @@ public function testSaveActionExistingCustomerUnsubscribeNewsletter()
         ];
         $this->getRequest()->setPostValue($post);
         $this->getRequest()->setParam('id', 1);
+        $this->_urlBuilder->turnOffSecretKey();
         $this->dispatch('backend/customer/index/save');
 
         /** @var \Magento\Newsletter\Model\Subscriber $subscriber */
@@ -370,7 +373,7 @@ public function testSaveActionExistingCustomerUnsubscribeNewsletter()
             \Magento\Framework\Message\MessageInterface::TYPE_SUCCESS
         );
 
-        $this->assertRedirect($this->stringStartsWith($this->_baseControllerUrl . 'index/key/'));
+        $this->assertRedirect($this->stringStartsWith($this->_baseControllerUrl . 'index/'));
     }
 
     /**
@@ -410,13 +413,14 @@ public function testSaveActionExistingCustomerChangeEmail()
         ];
         $this->getRequest()->setPostValue($post);
         $this->getRequest()->setParam('id', 1);
+        $this->_urlBuilder->turnOffSecretKey();
         $this->dispatch('backend/customer/index/save');
 
         /**
          * Check that no errors were generated and set to session
          */
         $this->assertSessionMessages($this->isEmpty(), \Magento\Framework\Message\MessageInterface::TYPE_ERROR);
-        $this->assertRedirect($this->stringStartsWith($this->_baseControllerUrl . 'index/key/'));
+        $this->assertRedirect($this->stringStartsWith($this->_baseControllerUrl . 'index/'));
     }
 
     /**
@@ -479,6 +483,7 @@ public function testSaveActionCoreException()
             ],
         ];
         $this->getRequest()->setPostValue($post);
+        $this->_urlBuilder->turnOffSecretKey();
         $this->dispatch('backend/customer/index/save');
         /*
          * Check that error message is set
@@ -491,7 +496,7 @@ public function testSaveActionCoreException()
             $post,
             Bootstrap::getObjectManager()->get(\Magento\Backend\Model\Session::class)->getCustomerFormData()
         );
-        $this->assertRedirect($this->stringStartsWith($this->_baseControllerUrl . 'new/key/'));
+        $this->assertRedirect($this->stringStartsWith($this->_baseControllerUrl . 'new/'));
     }
 
     /**

dev/tests/integration/testsuite/Magento/Newsletter/Controller/Adminhtml/NewsletterTemplateTest.php

@@ -146,11 +146,8 @@ public function testSaveActionTemplateWithGetAndVerifyRedirect()
         /**
          * Check that correct redirect performed.
          */
-        $backendUrlModel = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()->get(
-            \Magento\Backend\Model\UrlInterface::class
-        );
-        $backendUrlModel->turnOffSecretKey();
-        $url = $backendUrlModel->getUrl('newsletter');
+        $this->_urlBuilder->turnOffSecretKey();
+        $url = $this->_urlBuilder->getUrl('*/template');
         $this->assertRedirect($this->stringStartsWith($url));
     }
 }