Incorrect Base URLs Due To Improper Handling of Store Emulation

[zaximus84]

Preconditions and environment

Set up at least 2 store views with different base urls configured.

Steps to reproduce

Execute a process that emulates multiple store views, and within each emulation, uses Magento\Framework\Url::getUrl to produce fully-qualified urls that should be store-specific. The most common real world use case for this is a cron job that processes a queue of emails across store scopes, wrapping each email send in emulation and generating urls to render within the emails. However, the relevant logic can be exhibited with a simple sandbox script. Put this in a file in the var directory and run it from the command line:

<?php

include('../app/bootstrap.php');

use Magento\Framework\App\Filesystem\DirectoryList;

$params = $_SERVER;
$params[\Magento\Framework\App\Bootstrap::INIT_PARAM_FILESYSTEM_DIR_PATHS] = [
    DirectoryList::PUB => [DirectoryList::URL_PATH => ''],
    DirectoryList::MEDIA => [DirectoryList::URL_PATH => 'media'],
    DirectoryList::STATIC_VIEW => [DirectoryList::URL_PATH => 'static'],
    DirectoryList::UPLOAD => [DirectoryList::URL_PATH => 'media/upload'],

];
$bootstrap = \Magento\Framework\App\Bootstrap::create(BP, $params);
$objectManager = $bootstrap->getObjectManager();

$area = 'crontab';
$state = $objectManager->get('Magento\Framework\App\State');
$state->setAreaCode($area);
$configLoader = $objectManager->create('Magento\Framework\App\ObjectManager\ConfigLoader');
$objectManager->configure($configLoader->load($area));

/** @var Magento\Store\Model\App\Emulation $emulation */
$emulation = $objectManager->create(Magento\Store\Model\App\Emulation::class);

/** @var Magento\Store\Model\StoreManagerInterface $storeManager */
$storeManager = $objectManager->create(Magento\Store\Model\StoreManagerInterface::class);

/** @var Magento\Framework\UrlInterface $url */
$url = $objectManager->get(Magento\Framework\UrlInterface::class);

echo "\n";
foreach ($storeManager->getStores() as $store) {
    $emulation->startEnvironmentEmulation($store->getId());
    echo $url->getUrl('customer/account') . "\n";
    echo $url->getUrl('catalog/product/view', ['id' => $store->getId()]) . "\n";
    $emulation->stopEnvironmentEmulation();
}
echo "\n";

Expected result

For the script above, imagining you have store 1 with base url https://aaa.test, and store 2 with base url https://bbb.test, you'd expect the following output:

https://aaa.test/customer/account
https://aaa.test/catalog/product/view/id/1
https://bbb.test/customer/account
https://bbb.test/catalog/product/view/id/2

Actual result

Caching causes data from the first emulated scope execution to produce incorrect results in subsequent (different) scopes. You'd see this instead:

https://aaa.test/customer/account
https://aaa.test/catalog/product/view/id/1
https://aaa.test/customer/account
https://aaa.test/catalog/product/view/id/2

Additional information

This issue has been reported in a couple of other places without follow up:
#32687
#35274

Primary Issue - Cached Scope
Magento\Framework\Url has its own in-memory caching of scope. It has no idea when emulation has changed the store scope, so it doesn't know that the last scope it used (and cached) shouldn't be reused. _getScope() is used to retrieve the scope for operations such as getBaseUrl, and it will use the cached scope if there is one. If there isn't, it calls setScope with null, which ultimately calls the store manager to resolve the scope.

Initially, I thought this issue could be solved by modifying _getScope and removing the hasData check. This would do away with the scope caching altogether. Given that the scope resolution is ultimately handled by the store repository, which has its own in-memory caching, there's no real affect on performance. However, this causes issues whenever a _scope param is passed. getBaseUrl calls setScope($params['_scope']) to modify the scope, causing the custom scope to be cached in the data array. If the next _getScope call freshly calculates the scope instead of using $this->_getData('scope'), it ends up ignoring the custom scope.

I determined 2 viable solutions for this scope issue:

1. Inject Magento\Framework\Url into Magento\Store\Model\App\Emulation. At the end of startEnvironmentEmulation() and stopEnvironmentEmulation(), call $this->url->unsetData('scope'). This removes any cached scope and ensures that the next getUrl() call will freshly calculate the scope.
2. Modify Magento\Framework\Url to be more specific about when it uses a cached scope object. The only use case I found for needing the caching is with the _scope param. I found it effective to update _getScope() to _getScope(bool $useCache = false), and pass true to that in the final _getScope() call in getBaseUrl(). All other calls to _getScope() then default to not using the cache.

Secondary Issue - Cached Links
If the cached scope is dealt with, the example script still produces this:

https://aaa.test/customer/account
https://aaa.test/catalog/product/view/id/1
https://aaa.test/customer/account
https://bbb.test/catalog/product/view/id/2

The second account link is still wrong. This is due to link caching toward the end of getUrl(). The product urls have unique parameters, so they're not found in the cache. The account links don't, so the url cached for the first scope gets reused for the second scope. The scope needs to be added to $cachedParams to ensure cached values are not shared between scopes. Since this relies on some kind of mechanism to determine the current scope, it obviously requires that the primary issue above already be solved.

My Solution
I don't think the Emulation class should really be responsible for managing Url's scope, so I opted to fix both issues with changes to Magento\Framework\Url. I fixed the main caching issue by limiting cached scope use to cases where _scope was passed as a parameter, and I made sure scope is present in the link cache params. This is my patch:

diff --git a/Url.php b/Url.php
--- a/Url.php
+++ b/Url.php
@@ -438,9 +438,9 @@ class Url extends \Magento\Framework\DataObject implements \Magento\Framework\Ur
      *
      * @return \Magento\Framework\Url\ScopeInterface
      */
-    protected function _getScope()
+    protected function _getScope(bool $useCache = false)
     {
-        if (!$this->hasData('scope')) {
+        if (!$this->hasData('scope') || !$useCache) {
             $this->setScope(null);
         }
         return $this->_getData('scope');
@@ -481,7 +481,7 @@ class Url extends \Magento\Framework\DataObject implements \Magento\Framework\Ur
             $this->getRouteParamsResolver()->setType(UrlInterface::URL_TYPE_DIRECT_LINK);
         }

-        $result = $this->_getScope()->getBaseUrl($this->_getType(), $this->_isSecure());
+        $result = $this->_getScope(true)->getBaseUrl($this->_getType(), $this->_isSecure());

         // setting back the original scope
         $this->setScope($origScope);
@@ -866,9 +866,10 @@ class Url extends \Magento\Framework\DataObject implements \Magento\Framework\Ur
             );
         }

-        $cachedParams = $routeParams;
-        if ($isArray) {
-            ksort($cachedParams);
+        $cachedParams = $routeParams ?? [];
+        ksort($cachedParams);
+        if (!isset($cachedParams['_scope'])) {
+            $cachedParams['_scope'] = $this->_getScope()->getId();
         }

         $cacheKey = sha1($routePath . $this->serializer->serialize($cachedParams));

Magento\Backend\Model\Url, a subclass of Magento\Framework\Url, must also be patched with the new method signature:

diff --git a/Model/Url.php b/Model/Url.php
--- a/Model/Url.php
+++ b/Model/Url.php
@@ -438,7 +438,7 @@ class Url extends \Magento\Framework\Url implements \Magento\Backend\Model\UrlIn
      *
      * @return \Magento\Store\Model\Store
      */
-    protected function _getScope()
+    protected function _getScope(bool $useCache = false)
     {
         if (!$this->_scope) {
             $this->_scope = $this->_storeFactory->create(

This has yet to be fully tested on my current project, so I'm not comfortable submitting it as PR at the moment.

Release note

No response

Triage and priority

• Severity: S0 - Affects critical data or functionality and leaves users without workaround.
• Severity: S1 - Affects critical data or functionality and forces users to employ a workaround.
• Severity: S2 - Affects non-critical data or functionality and forces users to employ a workaround.
• Severity: S3 - Affects non-critical data or functionality and does not force users to employ a workaround.
• Severity: S4 - Affects aesthetics, professional look and feel, “quality” or “usability”.

[m2-assistant]

Hi @zaximus84. Thank you for your report.
To speed up processing of this issue, make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce.

• For more details, review the Magento Contributor Assistant documentation.
• Add a comment to assign the issue: @magento I am working on this
• To learn more about issue processing workflow, refer to the Code Contributions.

---

Join Magento Community Engineering Slack and ask your questions in #github channel.
⚠️ According to the Magento Contribution requirements, all issues must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.
🕙 You can find the schedule on the Magento Community Calendar page.
📞 The triage of issues happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

[zaximus84]

My suggested patch with the $useCache flag is incompatible with some fringe uses of the Url model across the application. As an example, this code in \Magento\LoginAsCustomerAdminUi\Controller\Adminhtml\Login\Login::getLoginProceedRedirectUrl produces the wrong url:

$redirectUrl = $this->url
            ->setScope($targetStore)
            ->getUrl('loginascustomer/login/index', ['_query' => $queryParameters, '_nosid' => true]);

The scope ends up getting recalculated during getUrl. I think in a perfect world, setScope would be a protected method, and forced scope could only be passed via _scope:

$redirectUrl = $this->url
            ->getUrl('loginascustomer/login/index', ['_query' => $queryParameters, '_nosid' => true, '_scope' => $targetStore->getCode()]);

But as long as there are still occurrences of code externally calling setScope on the Url model, my suggested patch is not viable.

[m2-assistant]

Hi @engcom-Hotel. Thank you for working on this issue.
In order to make sure that issue has enough information and ready for development, please read and check the following instruction: 👇

• 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
• 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue.
• 3. Add Area: XXXXX label to the ticket, indicating the functional areas it may be related to.
• 4. Verify that the issue is reproducible on 2.4-develop branch
  Details

  - If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
  - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
• 5. Add label Issue: Confirmed once verification is complete.
• 6. Make sure that automatic system confirms that report has been added to the backlog.

[engcom-Hotel]

Hello @zaximus84,

Thanks for the report and collaboration!

The issue has been reproducible for us:

Hence confirming the issue. I suggest you to create a community PR with the patch provided by you in the main description.

Thanks