magento2 icon indicating copy to clipboard operation
magento2 copied to clipboard

Redirect to secure_url removes url parameters (like "?product_list_limit=20")

Open fwolfst opened this issue 8 years ago • 11 comments

Preconditions

  1. Using magento 2.2.1
  2. php, mysql and other libraries current from vanilla ubuntu 16.04
  3. (not really necessary I GUESS) ssl-setup via nginx443->varnish80->nginx8080(magento)
  4. secure and unsecure URL set to the https adress (we dont want unencrypted traffic except between varnish80 and nginx8080). That means magento2 will always ask the browser to visit us on https if nginx and varnish didnt tell magento that it was already spoken to via https.

Steps to reproduce

  1. see preconditions, then e.g. curl --verbose --head --location http://yourshop/yourstuff/thestuff.html\?product_list_limit\=30 - note the http (not https) in the request -> this is a trivial example, loosing the product_list_limit is not so dramatic, but we actually deal with affiliate ids that are given as parameters ("?affiliate=me")

Expected result

  1. Magento answers with Location: https://yourshop/yourstuff/thestuff.html/?product_list_limit\=30

Actual result

  1. Magento answers with Location: https://yourshop/yourstuff/thestuff.html/ (missing product_list_limit or any other parameter at the end).

I guess one location where things go wrong is around here ./project-community-edition/vendor/magento/framework/App/Router/Base.php:372 and the getRedirectUrl implementation.

The unit test case should include cases where a parameter is given.

Probably this has side-effects or security consequences that I cannot overlook. In that case the parameters might have to be whitelisteable or the general behaviour could be made on/off-switcheable.

Probably this issue is related: https://github.com/magento/magento2/issues/12242 .

fwolfst avatar Dec 10 '17 22:12 fwolfst

If you want to have redirects working in a similar setting, the "trick" would be to to but nginx even more in front and let it do the redirection.

So, nginx 80 -> nginx 443 (just tls stuff) -> varnish (local, 8011 or whatever) -> nginx (local 8080, serving magento).

Still the original issue is not solved (and I am somewhat curious if there any security implications).

fwolfst avatar Dec 15 '17 08:12 fwolfst

@fwolfst, thank you for your report. We've acknowledged the issue and added to our backlog.

magento-engcom-team avatar Mar 07 '18 12:03 magento-engcom-team

the problem in http -> https redirect in nginx ??

magenx avatar Mar 07 '19 08:03 magenx

@fwolfst, thank you for your report.

Unfortunately, we are archiving this ticket now as it did not get much attention from both Magento Community and Core developers for an extended period. This is done in an effort to create a quality, community-driven backlog which will allow us to allocate the required attention more easily.

Please feel free to comment, reopen or create new ticket according to the Issue reporting guidelines if you are still facing this issue on the latest 2.4-develop branch. Thank you for collaboration.

magento-engcom-team avatar Dec 10 '19 14:12 magento-engcom-team

We are facing that issue as well in Magento 2.4.0 when a redirect to the configured base_url happens. Seems like the issue is in \Magento\Store\App\FrontController\Plugin\RequestPreprocessor::aroundDispatch.

There the redirect URL is built without any possibly present query parameters. Changing $this->_url->getDirectUrl(ltrim($request->getPathInfo(), '/'), ['_nosid' => true])

to

$this->_url->getDirectUrl(
   ltrim($request->getPathInfo(), '/'),
   ['_nosid' => true, '_query' => $request->getQuery()->toArray()]
)

seems to preserve the original query parameters.

aschrammel avatar Dec 09 '20 13:12 aschrammel

Is there any chance of getting it fixed in the near future?

pkocybik avatar Feb 25 '22 07:02 pkocybik

still not fixed.

tobiasolge avatar Dec 22 '23 11:12 tobiasolge

Hi @torhoehn. 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: :point_down:

  • [x] 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
  • [x] 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue.
  • [x] 3. Add Area: XXXXX label to the ticket, indicating the functional areas it may be related to.
  • [x] 4. Verify that the issue is reproducible on 2.4-develop branch
    Details- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
    - 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!
  • [x] 5. Add label Issue: Confirmed once verification is complete.
  • [ ] 6. Make sure that automatic system confirms that report has been added to the backlog.

m2-assistant[bot] avatar Apr 25 '24 11:04 m2-assistant[bot]

Unfortunately, not enough information was provided to create a Jira ticket. Please make sure you added the following label(s): Reproduced on 2.4.x, ^Area:.*

Once all required labels are present, please add Issue: Confirmed label again.

github-jira-sync-bot avatar Apr 25 '24 11:04 github-jira-sync-bot

Hi @engcom-November. 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: :point_down:

  • [ ] 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- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
    - 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.

m2-assistant[bot] avatar May 20 '24 12:05 m2-assistant[bot]

Hello @fwolfst,

Thank you for the report and collaboration!

We were not able to reproduce this on 2.4-develop. Configured ssl, secure and unsecure URL is set to the https. But ?product_list_limit\=30 parameter was intact in the request. Please take a look at the screenshot below: image

Below is the httpd-vhosts.conf

<VirtualHost *:443>
    DocumentRoot /opt/homebrew/var/www/magento7
    ServerName  local.magento7ce.com
    SSLEngine on
    SSLCertificateFile ...
    SSLCertificateKeyFile ...
    SSLCipherSuite ...
    <Directory "/opt/homebrew/var/www">
        AllowOverride all
        Require all granted
    </Directory>
</VirtualHost>

<VirtualHost *:80>
   ServerName local.magento7ce.com
   Redirect / https://local.magento7ce.com/
</VirtualHost>

Please reverify the issue on latest 2.4-develop branch.

Thank you.

engcom-November avatar May 20 '24 12:05 engcom-November

Hello @fwolfst,

As there is no activity on this issue for a long time, we believe the issue has been resolved, hence closing this issue. Feel free to raise a new issue or reopen this if you need more assistance.

Thank you.

engcom-November avatar Jun 04 '24 11:06 engcom-November