UrlRewrite removes query string from url, if url has trailing slash

[sergeynezbritskiy]

Preconditions (*)

When customer opens url with trailing slash and query string like http://magento-host.com/some-url-key/?foo=bar, Magento redirects to url without trailing slash and truncates query string, so customer is redirected to http://magento-host.com/some-url-key.

1. Magento version 2.2.5 or 2.2.6
2. No customizations for the store, Luma + Sample Data

Steps to reproduce (*)

1. Open any product or category page.
2. Manually add /?foo=bar&bar=baz to the url and open this link

Expected result (*)

1. After the page has been loaded, query string remain in url

Actual result (*)

1. Query string is removed from url

Additional information

1. The issue is not reproduced when we avoid using url rewrites, e.g. http://magento-host.com/catalog/product/view/id/{product-id}/?foo=bar&bar=baz, and actually there is no redirect in this case.
2. The issue seems to be happen in \Magento\UrlRewrite\Model\Storage\DbStorage::doFindOneByData

$data[UrlRewrite::REQUEST_PATH] = [
    rtrim($requestPath, '/'),
    rtrim($requestPath, '/') . '/',
];

We are looking for both urls (with trailing slash and without it), but when verifying the result from database we do not check it.

// If request path matches the DB value or it's redirect - we can return result from DB
$canReturnResultFromDb = ($resultFromDb[UrlRewrite::REQUEST_PATH] === $requestPath || in_array((int)$resultFromDb[UrlRewrite::REDIRECT_TYPE], $redirectTypes, true));

[magento-engcom-team]

Hi @sergeynezbritskiy. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

• Summary of the issue
• Information on your environment
• Steps to reproduce
• Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento-engcom-team give me $VERSION instance

where $VERSION is version tags (starting from 2.2.0+) or develop branches (for example: 2.3-develop).
For more details, please, review the Magento Contributor Assistant documentation.

@sergeynezbritskiy do you confirm that you was able to reproduce the issue on vanilla Magento instance following steps to reproduce?

• yes
• no

[sergeynezbritskiy]

https://github.com/sergeynezbritskiy/magento2/commit/7e26ec6a7cf1de8343ab05555de5a687da1518b8
Actually, I think this would be a fix for this issue. If this would be confirmed as a bug, then I will cover this fix with tests and will make a pull-request.
Thanks!

[orlangur]

@sergeynezbritskiy, URLs like http://magento-host.com/catalog/product/view/id/{product-id}/?foo=bar&bar=baz is not the right way to go. It should look like http://magento-host.com/catalog/product/view/id/{product-id}/foo/bar/bar/baz/ instead.

[sergeynezbritskiy]

@orlangur actually we can't change url format due to our SEO team restrictions. Moreover default magento layered navigation uses urls with query string, e.g. here is url from my local demo installation with sample data: http://demo.magento2.local/women/tops-women/jackets-women?climate=202; When I add trailing slash (http://demo.magento2.local/women/tops-women/jackets-women/?climate=202, which is actually a valid url in terms of http protocol),Magento redirects me to http://demo.magento2.local/women/tops-women/jackets-women.
I our case, we have a custom layered navigation module, which prettifies a little query string, making it like http://my-host.com/category-url-key?color=black&size=xl
But
We have a lot of promotion compains in social networks and Google, and generate these urls for these compains within some external ERP system, which actually creates urls with trailing slash, like http://my-host.com/category-url-key/?color=black&size=xl

[sergeynezbritskiy]

@orlangur due to my investigation, the issue probably comes from \Magento\UrlRewrite\Model\Storage\DbStorage:doFindOneByData, which returns 301 redirect for request_path with trailing slash, when it founds url rewrite with exactly the same request_path but without trailing slash, but not clear why the should be any redirect.
So comparing without slashes fixes the issue. Please check my commit https://github.com/sergeynezbritskiy/magento2/commit/7e26ec6a7cf1de8343ab05555de5a687da1518b8

[magento-engcom-team]

Hi @engcom-backlog-andrii. 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).

  Details

  If the issue has a valid description, the label Issue: Format is valid will be added to the issue automatically. Please, edit issue description if needed, until label Issue: Format is valid appears.

• 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue. If the report is valid, add Issue: Clear Description label to the issue by yourself.

• 3. Add Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

• 4. Verify that the issue is reproducible on 2.3-develop branch

  Details

  - Add the comment @magento-engcom-team give me 2.3-develop instance to deploy test instance on Magento infrastructure.
  - If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.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. Verify that the issue is reproducible on 2.2-develop branch.

  Details

  - Add the comment @magento-engcom-team give me 2.2-develop instance to deploy test instance on Magento infrastructure.
  - If the issue is reproducible on 2.2-develop branch, please add the label Reproduced on 2.2.x

• 6. Add label Issue: Confirmed once verification is complete.

• 7. Make sure that automatic system confirms that report has been added to the backlog.

[magento-engcom-team]

@engcom-backlog-andrii Thank you for verifying the issue. Based on the provided information internal tickets MAGETWO-96662, MAGETWO-96663 were created

[magento-engcom-team]

Hi @shikhamis11. Thank you for working on this issue.
Looks like this issue is already verified and confirmed. But if your want to validate it one more time, please, go though the following instruction:

• 1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

• 2. Verify that the issue is reproducible on 2.3-develop branch

  Details

  - Add the comment @magento-engcom-team give me 2.3-develop instance to deploy test instance on Magento infrastructure.
  - If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
  - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!

• 3. Verify that the issue is reproducible on 2.2-develop branch.

  Details

  - Add the comment @magento-engcom-team give me 2.2-develop instance to deploy test instance on Magento infrastructure.
  - If the issue is reproducible on 2.2-develop branch, please add the label Reproduced on 2.2.x

• 4. If the issue is not relevant or is not reproducible any more, feel free to close it.

[shikhamis11]

#MM19IN

[shikhamis11]

I am also able to reproduce this issue at magento 2.2-develop and 2.3-develop instance

[shikhamis11]

@sergeynezbritskiy your solution is seems fine I am creating Pull request with this

[magento-engcom-team]

Hi @sergeynezbritskiy.

Thank you for your report and collaboration!

The issue was fixed by Magento team. The fix was delivered into magento/magento2:2.3-develop branch(es).
Related commit(s):

• 16b6a0cb81577d82f8dc6e729c48381fc934530f
• 2153a1d2308c9d10a749a9f3e40f7d7a16938cdd
• 37e620ef52469c496a44b4cde069d3a22c90d0d1
• 08729c1c60bcd023536d8511183a87daa274de46
• 5538f649c27010fc80f30919e15a838513970bf0
• e8301afe523f16c3a4a7b394713a3f71a146106f
• 03f7ded23c112bd7d2590f66ffab496e1c43e8a2

The fix will be available with the upcoming 2.3.3 release.

[arendarenko]

@magento-engcom-team give me 2.3.3 instance

[magento-engcom-team]

Hi @arendarenko. Thank you for your request. I'm working on Magento 2.3.3 instance for you

[magento-engcom-team]

Hi @arendarenko, here is your Magento instance.
Admin access: https://i-18717-2-3-3.instances.magento-community.engineering/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

[arendarenko]

@magento-engcom-team give me 2.3-develop instance

[magento-engcom-team]

Hi @arendarenko. Thank you for your request. I'm working on Magento 2.3-develop instance for you

[magento-engcom-team]

Hi @arendarenko, here is your Magento instance.
Admin access: https://i-18717-2-3-develop.instances.magento-community.engineering/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

[arendarenko]

Hi @magento-engcom-team
I don't think it's a proper fixes, because with this changes Magento did not redirect user to a proper url (in our case - without trailling slash), and this causes that we have two urls which are returns the same result:
/some-url/?a=b
/some-url?a=b

It may cause SEO-indexation issues.

I think that proper way is redirect to url without trailling slash AND keep query params in it, e.g

/some-url/?a=b => /some-url?a=b

[ihor-sviziev]

This definitely related to #10043 that was fixed by me.
However at that time I missed cases with GET params, so

Expected behavior should be following:

#	Correct?	request_path	target_path	User Request	Actual Response	Expected Response
1	✔	page-one/	page-A/	http://magento.com/page-one	1. Redirect to http://magento.com/page-one/ (301); 2. Redirect to http://magento.com/page-A/	1. Redirect to http://magento.com/page-one/ (301); 2. Redirect to http://magento.com/page-A/
	✔			http://magento.com/page-one/	Redirect to http://magento.com/page-A/	Redirect to http://magento.com/page-A/
2	✔	page-two	page-B	http://magento.com/page-two	Redirect to http://magento.com/page-B	Redirect to http://magento.com/page-B
	✔			http://magento.com/page-two/	1. Redirect to http://magento.com/page-two (301); 2. Redirect to http://magento.com/page-B	1. Redirect to http://magento.com/page-two (301); 2. Redirect to http://magento.com/page-B
3	❌	page-one/	page-A/	http://magento.com/page-one?get_pagram1=value1	1. Redirect to http://magento.com/page-one/ (301); 2. Redirect to http://magento.com/page-A/	1. Redirect to http://magento.com/page-one/?get_pagram1=value1 (301); 2. Redirect to http://magento.com/page-A/?get_pagram1=value1
	❌			http://magento.com/page-one/?get_pagram1=value1	Redirect to http://magento.com/page-A/	Redirect to http://magento.com/page-A/?get_pagram1=value1
4	❌	page-two	page-B	http://magento.com/page-two?get_pagram1=value1	Redirect to http://magento.com/page-B	Redirect to http://magento.com/page-B?get_pagram1=value1
	❌			http://magento.com/page-two/?get_pagram1=value1	1. Redirect to http://magento.com/page-two (301); 2. Redirect to http://magento.com/page-B	1. Redirect to http://magento.com/page-two?get_pagram1=value1 (301); 2. Redirect to http://magento.com/page-B?get_pagram1=value1
5	❌	page-three	page-C?get_param1=value1	http://magento.com/page-two?get_pagram2=value2	Redirect to http://magento.com/page-C	Redirect to http://magento.com/page-C?get_param1=value1&get_pagram2=value2
	❌			http://magento.com/page-two/?get_pagram2=value2	1. Redirect to http://magento.com/page-three (301); 2. Redirect to http://magento.com/page-C	1. Redirect to http://magento.com/page-three?get_param1=value1&get_pagram2=value2 (301); 2. Redirect to http://magento.com/page-C?get_param1=value1&get_pagram2=value2

[m2-assistant]

Hi @sdzhepa. Thank you for working on this issue.
Looks like this issue is already verified and confirmed. But if you want to validate it one more time, please, go though the following instruction:

• 1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.

• 2. Verify that the issue is reproducible on 2.3-develop branch

  Details

  - Add the comment @magento give me 2.3-develop instance to deploy test instance on Magento infrastructure.
  - If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
  - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!

• 3. If the issue is not relevant or is not reproducible any more, feel free to close it.

---

• Join Magento Community Engineering Slack and ask your questions in #github channel.

[sdzhepa]

Hello @sergeynezbritskiy
FYI: @shikhamis11 @arendarenko @ihor-sviziev @orlangur

I just received an update about this issue from the internal Jira ticket.

Previously it was fixed by internal Magento team and delivered with internal Pull Request in the scope of MAGETWO-96663, commits: #18717 (comment)

Sometime later were found additional bugs after this fix and it was a reason for several internal discussions and investigation. Finally, after architect approval, it was decided to revert these changes.
Changes were reverted in MC-22635

Here is a quote with an explanation :

The fix introduced in this ticket(MAGETWO-96663) changes the expected system behavior and introduces new issues to other use case scenarios of the URL Rewrite module and therefore it has to be reverted.

Currently, Magento UrlRewrite module does not support operations with mixed URLs trailings (combination of suffixed and not suffixed with forward slashes “/“ URLs). URLs that cannot be fully matched to the record in the url_rewrite database table will be redirected to the nearest matching URL, during redirection all existing GET parameters will be removed from the URL due to security reasons.

As an alternative solution, Category and Product suffixes (Stores > Configuration > Catalog > Search Engine Optimization > Category URL Suffix and/ or Product URL Suffix) can be set to “/“. If necessary, in addition to that Web Server URL rewrites can be configured to conditionally add missing forward slashes “/“ at the end of the URLs.

Also, additional information about "how existing GET parameters being treated during redirection" was added/updated to the DevDocs (magento/merchdocs#128)
Affected documentation pages:

• https://docs.magento.com/m2/ce/user_guide/marketing/url-rewrite.html
• https://docs.magento.com/m2/ce/user_guide/marketing/url-rewrite-product.html
• https://docs.magento.com/m2/ce/user_guide/marketing/url-rewrite-category.html
• https://docs.magento.com/m2/ce/user_guide/marketing/url-rewrite-cms-page.html
• https://docs.magento.com/m2/ce/user_guide/marketing/url-rewrite-custom.html

[jamie-selesti]

I have this exact issue on Magento 2.2.10.

Steps to reproduce
For the config path of "catalog/seo/category_url_suffix" I have the value of "/"

Expected result
When I go to /category-a/?p=2 (for the 2nd page in the pagination list) Magento to load the next set of products in the category

Actual result
When I go to /category-a/?p=2 (for the 2nd page in the pagination list) Magento redirects me to /category/

Additional information
I know if I remove the value of "/" in the config path of "catalog/seo/category_url_suffix" the pagination then works as expected however our SEO team don't like the fact the URL structure will change to without the trailing slash so this is not a real fix for me

Can anyone advise on the best code fix?

[ihor-sviziev]

Hi,

We discussed this issue with @akaplya and did following decision:

Adding support for getting GET params to the redirect might cause security issue.

Potential security issues

Example 1:
Till Magento 2.3.3 we had SID (session ID) in GET parameter, and it was enabled by default. Now it is disabled, but still could be enabled.

In case if targetPath will be set to external URL - this session ID will be sent to external website and could be used by potential hackers.

Example 2
Adding support for GET params could also create Open redirect vulnerability, where customer could be redirected to external URL.

Here is options that we found that could be secure

Potential secure options

1. Make this feature optional and disabled by default, if customer understand the risks - he could enable it.
2. Add GET parameters only for redirects inside Magento, remove all GET parameters for external redirects.

We could detect if target URL is external in following ways:

How to detect if target URL is external

a. During saving URL rewrite - add validation if target URL is external - add to extra params some flag that shows that it's internal/external URL, and if it's internal URL - we could add GET params, otherwise - do not add them. We could start there from auto-generated URL rewrites, looks like it will be enough.

b. In case if target path NOT contains http:// or https:// - treat such URL as internal and add GET parameters, otherwise treat such URL as external and don't add GET parameters.

---

I believe implementing option 2b is the easiest way.
Additionally cases with external URLs should be covered, if they're not.

[Zyles]

This is a major issue.

Our pagination stopped working. Applying /category/?p=2 to the URL to paginate, it does a 301 redirect to /category/ so we can't paginate to the next page.

[ihor-sviziev]

This is a major issue.

Our pagination stopped working. Applying /category/?p=2 to the URL to paginate, it does a 301 redirect to /category/ so we can't paginate to the next page.

Very strange as we're having slash as url suffix for categories and we don’t have this issue on Magento 2.3.2-p2. Could you add steps to reproduce on clear magento installation?

[maaarghk]

@ihor-sviziev obviously not since the "fix" referred to here was introduced in 2.3.3

for anyone else fucked over by this, here's a module that applies the fix from #25603 on top of the revert fa468e7 - composer require maaarghk/fuck-magento - so you can happily go back to / as a category suffix.

or if your client has a stick up their arse maybe you can just put this code in app/code/FTS/FixTrailingSlash

xoxo

[alex-james]

Thank you @maaarghk for providing the link to the code which helped me solve this issue. The Magento 2.3.3 release note under URL Rewrites states:

Magento no longer removes the query string from URLs when the query string is preceded by a slash. Previously, when a customer opened a URL that contained a trailing slash and query string (for example, http://magento.host.com/sample-url-key/?cupcakes), Magento redirected the user to a URL that omitted the slash (http://magento.host.com/sample-url-key).

But from what I have seen in 2.3.3 it looks like it now strips out the all query string parameters regardless of whether there is a trailing slash or not in the url. This has now broken pagination and # items per page functionality on category page urls such as /square-gutter-brown-c-774/?product_list_limit=all

As you pointed out, the culprit here are the changes in Magento\UrlRewrite\Controller\Router which finds a match for the category in the "url_rewrite table" (because it is a category and under the covers it is redirecting to: /catalog/category/view/id/774) once it actions the redirect it looses all the query string parameters.

Feels like a slightly different issue to the one raised here and I am tempted to raise a new ticket but as the last 2 have been closed without even basic checking I'm not going to bother, seems like it is every man for themselves with Magento support at the moment.

[VladimirZaets]

Hi @sergeynezbritskiy. Thank you for your report.
The issue has been fixed in #25603 by @arendarenko in 2.4-develop branch
Related commit(s):

• 0693cc2a75a1646080501176e24d3fd41a72a6b5
• 19b7bdac3a33eb12395b9f362a7c03e013228cc8
• f63475c37e37dde8eef9f404f518eb415c7557b0
• b1a2a29671de32d90dbc10a3b0ced6a3d71b2b72

The fix will be available with the upcoming 2.4.0 release.

[vtransy]

Thanks @maaarghk confirms that your note helps me resolve the issue in 2.3.3. Magento 2.3.3 stops working on infinity loading cause by param issue. Added your extension fixed the issue.