Skip to content

Fix removing query string from url after redirect #25603

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 3, 2020

Conversation

arendarenko
Copy link
Contributor

@arendarenko arendarenko commented Nov 14, 2019

Description (*)

See #18717 (comment)

Fixed Issues (if relevant)

  1. UrlRewrite removes query string from url, if url has trailing slash #18717: UrlRewrite removes query string from url, if url has trailing slash

Manual testing scenarios (*)

See #18717 (comment)

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

add ability to save query param from request during redirect to target path
update unit test
@m2-assistant
Copy link

m2-assistant bot commented Nov 14, 2019

Hi @arendarenko. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@sdzhepa
Copy link
Contributor

sdzhepa commented Nov 14, 2019

Hello there
FYI: @arendarenko @ihor-sviziev @kokoc @akaplya @VladimirZaets

Previously fix related to this issue has been reverted by internal Magento team.
More details #18717 (comment)
IMO this PR could require additional architect review/approve

@ghost ghost added the Progress: on hold label Nov 15, 2019
@ihor-sviziev
Copy link
Contributor

Moved to "on hold", waiting for discussion with Architects

update fixture
update integration test
@magento-engcom-team
Copy link
Contributor

Hi @lbajsarowicz, thank you for the review.
ENGCOM-6303 has been created to process this Pull Request
✳️ @lbajsarowicz, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@ihor-sviziev
Copy link
Contributor

Hi @lbajsarowicz,
From my perspective we need to approve it also, but we have some complains from Architects against it. Now I’m discussing this PR in Slack with @akaplya, hope we’ll decide to accept it.

@ihor-sviziev
Copy link
Contributor

I double checked this PR after discussion with @akaplya (details here #18717 (comment)) - changes looks good.

Also I covered cases when target URL is external URL (not magento) with integration test, in this case GET params that came to magento should not be added to redirect URL.

Now this solution should works fine. Let's wait once all tests will be passing and then I'll approve this PR.

Cover with integration tests cases with external URLs as target path
@ihor-sviziev
Copy link
Contributor

Hi @Zyles,
Could you add steps to reproduce, so we’ll be able to double check it?

@Zyles
Copy link

Zyles commented Dec 17, 2019

Hi @Zyles,
Could you add steps to reproduce, so we’ll be able to double check it?

Magento 2.3.3
Nginx
Letsencrypt SSL

image

image

image

Nginx config:

upstream fastcgi_backend {
    server   unix:/run/php/php7.3-fpm.sock;
}

server {
    listen 80;
    listen [::]:80;
    server_name store.example.com;
    return 301 https://store.example.com$request_uri;
}

server {

    listen [::]:443 ssl http2 ipv6only=on;
    listen 443 ssl http2;

    ssl on;
    ssl_ciphers EECDH+CHACHA20:EECDH+AES128:RSA+AES128:EECDH+AES256:RSA+AES256:EECDH+3DES:RSA+3DES:!MD5;
    ssl_certificate /etc/letsencrypt/live/example.com/fullchain.pem;
    ssl_certificate_key /etc/letsencrypt/live/example.com/privkey.pem;

    ssl_session_timeout 1d;
    ssl_session_cache shared:SSL:50m;
    ssl_session_tickets off;
    ssl_protocols TLSv1 TLSv1.1 TLSv1.2;
    ssl_prefer_server_ciphers on;
    ssl_stapling on;
    ssl_stapling_verify on;

    server_name store.example.com;
    set $MAGE_ROOT /home/http/example.com;
    set $MAGE_DEBUG_SHOW_ARGS 1;

    root $MAGE_ROOT/pub;

    index index.php;
    autoindex off;
    charset UTF-8;
    error_page 404 403 = /errors/404.php;

# PHP entry point for setup application
location ~* ^/setup($|/) {
    root $MAGE_ROOT;
    location ~ ^/setup/index.php {
        fastcgi_pass   fastcgi_backend;

        fastcgi_param  PHP_FLAG  "session.auto_start=off \n suhosin.session.cryptua=off";
	    fastcgi_param  PHP_VALUE "memory_limit=2G \n max_execution_time=600";
        fastcgi_read_timeout 600s;
        fastcgi_connect_timeout 600s;

        fastcgi_index  index.php;
        fastcgi_param  SCRIPT_FILENAME  $document_root$fastcgi_script_name;
        include        fastcgi_params;
    }

    location ~ ^/setup/(?!pub/). {
        deny all;
    }

    location ~ ^/setup/pub/ {
        add_header X-Frame-Options "SAMEORIGIN";
    }
}

# PHP entry point for update application
location ~* ^/update($|/) {
    root $MAGE_ROOT;

    location ~ ^/update/index.php {
        fastcgi_split_path_info ^(/update/index.php)(/.+)$;
        fastcgi_pass   fastcgi_backend;
        fastcgi_index  index.php;
        fastcgi_param  SCRIPT_FILENAME  $document_root$fastcgi_script_name;
        fastcgi_param  PATH_INFO        $fastcgi_path_info;
        include        fastcgi_params;
    }

    # Deny everything but index.php
    location ~ ^/update/(?!pub/). {
        deny all;
    }

    location ~ ^/update/pub/ {
        add_header X-Frame-Options "SAMEORIGIN";
    }
}

location / {
    try_files $uri $uri/ /index.php$is_args$args;
}

location /db/ {
    try_files $uri $uri/ /index.php$is_args$args;
}


location /pub/ {
    location ~ ^/pub/media/(downloadable|customer|import|theme_customization/.*\.xml) {
        deny all;
    }
    alias $MAGE_ROOT/pub/;
    add_header X-Frame-Options "SAMEORIGIN";
}

location /static/ {
    # Uncomment the following line in production mode
    # expires max;

    # Remove signature of the static files that is used to overcome the browser cache
    location ~ ^/static/version {
        rewrite ^/static/(version\d*/)?(.*)$ /static/$2 last;
    }

    location ~* \.(ico|jpg|jpeg|png|gif|svg|js|css|swf|eot|ttf|otf|woff|woff2)$ {
        add_header Cache-Control "public";
        add_header X-Frame-Options "SAMEORIGIN";
        expires +1y;

        if (!-f $request_filename) {
            rewrite ^/static/?(.*)$ /static.php?resource=$1 last;
        }
    }
    location ~* \.(zip|gz|gzip|bz2|csv|xml)$ {
        add_header Cache-Control "no-store";
        add_header X-Frame-Options "SAMEORIGIN";
        expires    off;

        if (!-f $request_filename) {
           rewrite ^/static/?(.*)$ /static.php?resource=$1 last;
        }
    }
    if (!-f $request_filename) {
        rewrite ^/static/?(.*)$ /static.php?resource=$1 last;
    }
    add_header X-Frame-Options "SAMEORIGIN";
}

location /media/ {
    try_files $uri $uri/ /get.php$is_args$args;

    location ~ ^/media/theme_customization/.*\.xml {
        deny all;
    }

    location ~* \.(ico|jpg|jpeg|png|gif|svg|js|css|swf|eot|ttf|otf|woff|woff2)$ {
        add_header Cache-Control "public";
        add_header X-Frame-Options "SAMEORIGIN";
        expires +1y;
        try_files $uri $uri/ /get.php$is_args$args;
    }
    location ~* \.(zip|gz|gzip|bz2|csv|xml)$ {
        add_header Cache-Control "no-store";
        add_header X-Frame-Options "SAMEORIGIN";
        expires    off;
        try_files $uri $uri/ /get.php$is_args$args;
    }
    add_header X-Frame-Options "SAMEORIGIN";
}

location /media/customer/ {
    deny all;
}

location /media/downloadable/ {
    deny all;
}

location /media/import/ {
    deny all;
}

# PHP entry point for main application
location ~ (index|get|static|errors/report|errors/404|errors/503|health_check|elastic)\.php$ {
    try_files $uri =404;
    fastcgi_pass   fastcgi_backend;
    fastcgi_buffers 1024 4k;

    fastcgi_param  PHP_FLAG  "session.auto_start=off \n suhosin.session.cryptua=off";
    #fastcgi_param  PHP_VALUE "memory_limit=768M \n max_execution_time=18000";
    fastcgi_param  PHP_VALUE "memory_limit=2G \n max_execution_time=600";
    fastcgi_read_timeout 600s;
    fastcgi_connect_timeout 600s;

    fastcgi_index  index.php;
    fastcgi_param  SCRIPT_FILENAME  $document_root$fastcgi_script_name;
    include        fastcgi_params;
}

gzip on;
gzip_disable "msie6";

gzip_comp_level 6;
gzip_min_length 1100;
gzip_buffers 16 8k;
gzip_proxied any;
gzip_types
    text/plain
    text/css
    text/js
    text/xml
    text/javascript
    application/javascript
    application/x-javascript
    application/json
    application/xml
    application/xml+rss
    image/svg+xml;
gzip_vary on;

# Banned locations (only reached if the earlier PHP entry point regexes don't match)
location ~* (\.php$|\.htaccess$|\.git) {
    deny all;
}

# Deny access to sensitive files
location /.user.ini {
    deny all;
}


}

@ihor-sviziev
Copy link
Contributor

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev. Thank you for your request. I'm working on Magento instance for you

@ihor-sviziev
Copy link
Contributor

@Zyles ok, you provieded your environment configuration, but not steps to reproduce.

Actually I tried it on my own environment - everything works fine.
Unfortunately spinning up test instance doens't work somehow right now, so I can't give you test instance for checking it.

Please add more details, which URL rewrite do you have in the magento, which request you're doing, what are expected and actual result?

Also would be great in order to prevent any caching issues to send request via curl -I command, for instance curl -I http://example.com/my-category/?p=2

@sdzhepa
Copy link
Contributor

sdzhepa commented Dec 17, 2019

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

Hi @sdzhepa, here is your new Magento instance.
Admin access: https://pr-25603.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@Zyles
Copy link

Zyles commented Dec 17, 2019

@Zyles ok, you provieded your environment configuration, but not steps to reproduce.

Actually I tried it on my own environment - everything works fine.
Unfortunately spinning up test instance doens't work somehow right now, so I can't give you test instance for checking it.

Please add more details, which URL rewrite do you have in the magento, which request you're doing, what are expected and actual result?

Also would be great in order to prevent any caching issues to send request via curl -I command, for instance curl -I http://example.com/my-category/?p=2

Go to a category.

Click paginate to page 2.

301 redirect to page 1.

/category/?p=2 -> /category/

Or type in the URL manually.

@Zyles
Copy link

Zyles commented Dec 17, 2019

$ curl -I https://store.magento/category/?p=2
HTTP/2 301
server: nginx
date: Tue, 17 Dec 2019 15:35:16 GMT
content-type: text/html; charset=UTF-8
location: https://store.magento/category/
set-cookie: PHPSESSID=uhtn3l6bb2t597g07do0424c09; expires=Fri, 17-Jan-2020 01:35:15 GMT; Max-Age=2628000; path=/; domain=store.magento; secure; HttpOnly
pragma: no-cache
cache-control: max-age=0, must-revalidate, no-cache, no-store
expires: Mon, 17 Dec 2018 15:35:15 GMT
strict-transport-security: max-age=31536000
content-security-policy: upgrade-insecure-requests;
x-content-type-options: nosniff
x-xss-protection: 1; mode=block
x-frame-options: SAMEORIGIN

@Zyles
Copy link

Zyles commented Dec 17, 2019

With SEO settings category suffix blank. No "/".

Creates duplicate content:

$ curl -I https://store.magento/category/?p=2
HTTP/2 200
server: nginx
date: Tue, 17 Dec 2019 15:38:11 GMT
content-type: text/html; charset=UTF-8
content-length: 363506
vary: Accept-Encoding
set-cookie: PHPSESSID=177nksbcr7e1jqf80ni3js7fkh; expires=Fri, 17-Jan-2020 01:38:11 GMT; Max-Age=2628000; path=/; domain=store.magento; secure; HttpOnly
pragma: no-cache
cache-control: max-age=0, must-revalidate, no-cache, no-store
expires: Mon, 17 Dec 2018 15:37:40 GMT
strict-transport-security: max-age=31536000
content-security-policy: upgrade-insecure-requests;
x-content-type-options: nosniff
x-xss-protection: 1; mode=block
x-frame-options: SAMEORIGIN
$ curl -I https://store.magento/category?p=2
HTTP/2 200
server: nginx
date: Tue, 17 Dec 2019 15:39:23 GMT
content-type: text/html; charset=UTF-8
content-length: 364548
vary: Accept-Encoding
pragma: no-cache
cache-control: max-age=0, must-revalidate, no-cache, no-store
expires: Mon, 17 Dec 2018 15:39:23 GMT
strict-transport-security: max-age=31536000
content-security-policy: upgrade-insecure-requests;
x-content-type-options: nosniff
x-xss-protection: 1; mode=block
x-frame-options: SAMEORIGIN

/category/?p=2 is not the same as /category?p=2

https://searchfacts.com/url-trailing-slash/

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Dec 17, 2019 via email

@ihor-sviziev
Copy link
Contributor

@Zyles i just checked on test instance and wasn’t able to reproduce it, so I think it’s web server configuration issue.

https://pr-25603.instances.magento-community.engineering/category-1/?p=2

Just checked your nginx configuration and found following line:

    try_files $uri $uri/ /index.php$is_args$args;

I believe the / near the $url/ causing this issue.

@Zyles
Copy link

Zyles commented Dec 17, 2019

try_files $uri $uri /index.php$is_args$args;

Same problem.

@Zyles
Copy link

Zyles commented Dec 17, 2019

Does this demo have the pull request patch installed?

Can you populate the demo with sample products?

@Zyles
Copy link

Zyles commented Dec 17, 2019

@magento-engcom-team give me 2.3.3 instance

@magento-engcom-team
Copy link
Contributor

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

@magento-engcom-team
Copy link
Contributor

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

@Zyles
Copy link

Zyles commented Dec 17, 2019

https://i-25603-2-3-3.instances.magento-community.engineering/category/?p=2

Is it possible to apply pull request on this instance?

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Dec 17, 2019 via email

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Dec 17, 2019

Ah, no. We can request changes for this PR + changes from 2.4-develop, or changes from 2.3.3

BTW I added small performance profile to following instance, so there enough products
So there is changes from this PR on top of 2.4-develop branch
https://pr-25603.instances.magento-community.engineering/category-2/

@Zyles
Copy link

Zyles commented Dec 17, 2019

Reproduced ERR_TOO_MANY_REDIRECTS on local fresh install 2.3.3 with patched file:

vendor/magento/module-url-rewrite/Controller/Router.php:
$target = $this->url->getUrl('', ['_direct' => $target, '_query' => $request->getParams()]);

Please try 2.3.3. We can't upgrade to 2.4-develop on live website.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Dec 18, 2019

@Zyles

Reproduced ERR_TOO_MANY_REDIRECTS on local fresh install 2.3.3 with patched file:

vendor/magento/module-url-rewrite/Controller/Router.php:
$target = $this->url->getUrl('', ['_direct' => $target, '_query' => $request->getParams()]);

Please try 2.3.3. We can't upgrade to 2.4-develop on live website.

Yeah, I finally found it.
Only in Magento 2.3.3 were added changes that suppose to fix described issue, but after this release came - it caused another issues, so in 2.4-develop that changes were reverted. More info: #18717 (comment)

So in order to have correct fix for 2.3.3 - before you need to apply following commit fa468e7 (reverts changes that were added to 2.3.3) and only then apply changes from this PR.

So changes from this PR should works fine. Could you confirm that?

@Zyles
Copy link

Zyles commented Dec 18, 2019

Thank you, seems to work now.

@m2-assistant
Copy link

m2-assistant bot commented Jan 3, 2020

Hi @arendarenko, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@sdzhepa sdzhepa mentioned this pull request May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants