Skip to content
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

Website Redirect: Issues in Apache and Nginx #634

Closed
iliajie opened this issue Sep 5, 2023 · 19 comments
Closed

Website Redirect: Issues in Apache and Nginx #634

iliajie opened this issue Sep 5, 2023 · 19 comments

Comments

@iliajie
Copy link
Collaborator

iliajie commented Sep 5, 2023

Hello, Jamie. I suggest we fix these last issues before finally making Virtualmin 7.8 release.

The issue number 1 that manifests in both Nginx and Apache.

# 1 Setup Example:

image

Created redirect, even though Include sub-paths in redirect is enabled doesn't include it in redirect, i.e. on the configs (Apache and Nginx) the redirect looks like https://google.com/ rather than https://google.com/$1. We can manually set it up in the UI to redirect to https://google.com/$1 but this is not user friendly.


The issue number 2 is redirect loop that happens (again in both Apache and Nginx) when source URL path is set to / and the redirect happens inside of the same domain.

# 2 Setup Example:

image

So, if a user wants to redirect the root of the site to some dir it won't work.

Created redirect line in Apache is:

RedirectMatch ^/(?!.well-known)(.*)$ https://ubuntu22-pro.virtualmin.dev/newdir/

.. what would work in this example is:

RedirectMatch ^/(?!well-known|newdir)(.*)$ https://ubuntu22-pro.virtualmin.dev/newdir/

In case we use alternative setup, i.e. using URL at this website option instead, e.g.:

image

.. then the redirect loop still happens. Created code in Apache config is:

RewriteCond %{HTTPS} on
RewriteRule ^/(?!.well-known)(.*)$ https://%{HTTP_HOST}/newdir/ [R]

.. while working example is:

RewriteCond %{HTTPS} on
RewriteRule ^/(?!.well-known|newdir)(.*)$ https://%{HTTP_HOST}/newdir/ [R]

Issue number 3 - Explanations for Include sub-paths in redirect needs a fix, as the logic seems completely reversed, e.g.:

image

This first part When this is set to Yes, the sub-directory should be When this is set to No, the sub-directory. The second paragraph If No was selected in this example should be If Yes was selected in this example .. The very last paragraph However, if Yes is chosen you can use the special code $1 in the destination URL. shouldn't even exist as we should add $1 automatically and not complicate things for the user.

It has to be intuitively clear what's happening. If a user needs to refer to the documentation, then we do something wrong in the UI ..

@jcameron
Copy link
Collaborator

jcameron commented Sep 6, 2023

So the problem is that we don't have good support on that page for doing a redirect to another URL on the same domain uisng the same protocol, which is a little surprising! I'll need to do some work to fix this properly ...

@iliajie
Copy link
Collaborator Author

iliajie commented Sep 26, 2023

I'll need to do some work to fix this properly

Jamie, did you have chance to have a look at this?

@jcameron
Copy link
Collaborator

I'm still working on it - will update this bug when it's done..

@jcameron
Copy link
Collaborator

jcameron commented Oct 4, 2023

Ok, please check out the latest commits and let me know how it works for you now ...

@iliajie
Copy link
Collaborator Author

iliajie commented Oct 5, 2023

It doesn't solve the initial problem, i.e. redirect loop, when creating a redirect from / to /en for example (as shown in the screenshot above).

Also, I tried both options of Redirect sub-directories to. Anyway, this is how redirect is created with the first option:

RedirectMatch ^/(?!.well-known)(.*)$ /en

and this is how it looks with the second options:

RedirectMatch /(.*)$ /en

.. both create redirect loop.

@jcameron
Copy link
Collaborator

jcameron commented Oct 6, 2023

Oh, so what we really need is a new option to only match the specific path in the redirect.

@iliajie
Copy link
Collaborator Author

iliajie commented Oct 6, 2023

I would like it to work without any options. We should exclude the path on root, i.e. / redirects as I suggested earlier, at the start of this ticket.
And speaking of options, I would also like to drop that Redirect sub-directories to option. It's just too complicated. We need to make things work in a most straight forward and robust way, without those super confusing options. They are even confusing to me.

@jcameron
Copy link
Collaborator

jcameron commented Oct 9, 2023

Ok with the latest set of commits, the user can choose how sub-directory redirects work so that a redirect of / doesn't cause a loop.

@iliajie
Copy link
Collaborator Author

iliajie commented Oct 20, 2023

Alright!

I can see now that you added another option called Ignore sub-directories. It works now as intended with /. I have tested it with Apache and Nginx, it seems to work.

However, there is still one bug left in Nginx module.

If I use / as Source URL path, and URL on this website set to /php74 and option Same sub-directory under destination URL is selected, after Save, Nginx config gets this entry:

rewrite ^/(?!.well-known) /php74 redirect;

However, it is not detected by Virtualmin and you cannot set it or change it in the UI anyhow ..

@jcameron
Copy link
Collaborator

The latest commit to the Nginx module should fix this...

@iliajie
Copy link
Collaborator Author

iliajie commented Oct 23, 2023

Alright, great! Thanks!

Now as the final stroke, can we make Auto option for Redirect sub-directories to? For example / should always be set to a way which won't end up in redirect loop. Perhaps, there are more we can do for automatic detection? Defaults should work just fine, without asking too many questions.

Also, why we assume that if a user makes redirect for / it won't be used to renew LE? I think we should always add an exclusion for .well-known, right?

@jcameron
Copy link
Collaborator

Maybe we could display an error if the user creates a redirect that would cause an infinite loop?

@iliajie
Copy link
Collaborator Author

iliajie commented Oct 24, 2023

Maybe we could display an error if the user creates a redirect that would cause an infinite loop?

I think this would be a good addition as well. But still that Redirect sub-directories to? is way to complicated, even though useful and powerful.

@jcameron
Copy link
Collaborator

I'll add that check ..

@pixel-paul
Copy link

Hi both,

Has there been a release that includes these changes?

I'm still having problems redirecting a www.example.com URL to example.com with Nginx.

Thanks,

Paul

@jcameron
Copy link
Collaborator

jcameron commented Dec 5, 2023

@pixel-paul what problem are you seeing exactly?

@pixel-paul
Copy link

@iliajie suggested a method to make the www. to non-www redirect work, but this just results in a redirect loop:

virtualmin/virtualmin-nginx#6 (comment)

@pixel-paul
Copy link

I'm now running Virtualmin 7.9.0 and I still cannot get non-www to redirect to www. Note I am using Nginx

This is the redirect requirement:

https://example.com -> https://www.example.com

If I create a rule as follows:

image

I get a redirect loop. After it is added, I cannot see the entry in the Virtualmin Website Redirects UI, however I can see it is added in the nginx conf as:

rewrite ^/(?!.well-known) "https://www.example/$1" permanent;

I have multiple rewrite rules, so could this be an order issue? For example, Virtualmin creates the Nginx records as follows:

rewrite ^\Q/2019/xyz/beginner/\E(.*) "https://$host/xyz-beginner$1" permanent;
rewrite ^\Q/beginner/week/singles/\E(.*) "https://$host/xyz/single$1" permanent;
rewrite ^/(?!.well-known) "https://www.example.co.uk/$1" permanent;

@iliajie
Copy link
Collaborator Author

iliajie commented Dec 19, 2023

I don't think we have released Nginx plugin yet.

If you apply the latest code from virtualmin/virtualmin-nginx, does it work then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants