thanks for this patch, as far as I've seen it is working.
but what is annoying me (a bit) is that this task (replacing "http" with "https") is something that should be handled by the JRoute::_() method if called with the right parameters, and I think this isn't the case here (in the original code).

the original code is:

<form action="<?php echo JRoute::_(htmlspecialchars(JUri::getInstance()->toString()), true, $params->get('usesecure')); ?>" method="post" id="login-form" class="form-inline">

within JRoute::_() the case that the first parameter is a string leads to returning this string unmodified except for some special cases that aren't given here. but JRoute::_() accepts as first parameter as well a JUri instance, and in that case it will also honor the third parameter $ssl.

so the following code should work for the joomla#7928 issue as well:

<form action="<?php echo JRoute::_(JUri::getInstance(), true, $params->get('usesecure')); ?>" method="post" id="login-form" class="form-inline">

can you please give it a try?

@jo-sf It might work. However, I am not sure we should pass that full URL to JRoute::_ method.

All we need to do is get the current URL, change the schema to https and pass it to action attribute of the form. I don't see any reason we need to pass it to JRoute::_ method just to change the url to https

@joomdonation thanks for giving it a try.

the JRoute::_() has been called here before, due to that I tried to find a solution that changes as few code as possible. moreover this method is doing the htmlspecialchars() encoding by default, at least if it is called with a JUri instance and not a string (this is controlled by the second parameter which is true by default).

as far as I've seen the htmlspecialchars() call has been added not before the 3.4.4 release.

Please also update "modules/mod_login/tmpl/default_logout.php" accordingly. We are still left with JRoute::_() there.

Thanks.

@bvitnik
I assume you mean switching back to HTTP when logging out if "usesecure" is set. Unfortunately this cannot be handled in the default_logout.php template - I tried but doing so I got a warning message from the browser saying that secure information will be transferred using an unsecure transport method (http). Since messages like that will annoy the users this is in my opinion not a good way. As far as I've seen this needs to be handled in modules/mod_login/helper.php.

I'm currently testing a more thorough solution entirely covering the secure transmission of registration and login information. In doing so I discovered that you currently need to enable "usesecure" for the login process for each login module at your site separately (with the demo site delivered with Joomla there are 3 places where you can log in), and there is currently no solution for the "login form" menu item. Moreover the data transferred during the registration process is always transferred via HTTP.

Part of my solution is to move the "usesecure" setting from the login modules to the users component where it will control both the secure transfer of registration data and the secure login. There is still one open issue in my solution: using non-standard ports for HTTP and HTTPS is currently not covered, it is silenty assumed that the default ports 80 and 443 are used.

I plan to find a solution for that issue and test it accordingly, and when I'm done I will create a pull request containing all patches necessary. From my current implementation for this topic I can say that it will be a larger patch, it already contains about 30 files to be changed. Due to that my own testing will still need some time - sorry!

@jo-sf
Sorry for answering you this late.

I wasn't really thinking in direction of switching back to HTTP on logout just that JRoute::_() is still used in "default_logout.php" which is pretty much useless. Beez3 "mod_login" override is also using JRoute::_().

I started with some template override for "mod_login" to make it more "beautiful" for my template and then ended up digging into the Joomla code spending hours trying to understand why form data was sent unencrypted. I guess these are those "benefits" of open-source software everyone is talking about :). At that time I wasn't aware that bug was reported and already fixed. So after some digging and GIT history checking I can only concur that user logging in Joomla need some serious rethinking.

Here is a brief history of the "mod_login":

1) Originally, "default.php" had a line like this:

<form action="index.php" method="post" name="login" id="form-login" style="clear: both;">

then people realized that it doesn't work for Joomla that is installed in a subdirectory. So they changed it to:

<form action="<?php echo JRoute::_( 'index.php', true, $params->get('usesecure')); ?>" method="post" name="login" id="form-login" style="clear: both;">

and also introduced an option to encrypt form data. That was done in commit aa1295e.

2) Then came commits 135a3df and 1b7c32c where the line was subsequently changed to:

<form action="<?php echo JRoute::_(JUri::current(), true, $params->get('usesecure')); ?>" method="post" id="login-form" class="form-inline">

and then to:

<form action="<?php echo JRoute::_(JUri::getInstance()->toString(), true, $params->get('usesecure')); ?>" method="post" id="login-form" class="form-inline">

This is where I'm getting confused. Am really a noob so correct me if I'm wrong but for the action attribute of the form element we only really need a path to "index.php", not a current URL with a query string, SEF path, or whatnot. We send a POST request to "index.php" and then get redirected to the URL that is base64 encoded in a hidden input element named "return". I don't see what those two commits tried to achieve except introducing a bug we are discussing about here :). So my understanding is that JRoute::_() was originally used to: change HTTP to HTTPS if needed, get a proper host, get a proper port and finally get a proper subdir if any, but JRoute::_() only worked if it was called with plain "index.php" instead of full URL. A case of "fixing something that was not broken" I guess.

Anyway, thanks a lot for investing your time fixing the user loggin problems the proper way :).

@bvitnik
No problem that you didn't answer immediately - and as you see I'm also answering only some hours later.

As I see you made a lot of historical investigations (or shall I say "archeological"?)...
You're right that it's not really necessary to use the current path in both the login and the logout form, since in both cases the path of the next page to display is encoded in the hidden "return" field. When I say "it's not necessary" this is true in what the visitor sees - if you look at tools like logfile analyzers or more generally visitor tracking tools this might be of some relevance since it looks as if the visitor jumps to the site's start page and then back to the page visited before.

The problem with JRoute::_() is that it returns the given path unmodified if it is not an array and if it neither starts with "&" nor with "index.php". Due to that the change in the login and logout templates from "index.php" to a string holding the current path result in ignoring the parameters $xhtml and $ssl. Someone later noticed that and changed the code such that htmlspecialchars() was called for the current path prior to passing it to JRoute::_() because even with $xhtml being set (it is set by default) the path wasn't encoded. Moreover the $ssl parameter is ignored currently.

Part of the changes I'm currently testing is to pass the current path to JRoute::_() as an JUri instance and not as string, and with a change within JRoute::_() (instead of testing whether $url is no array I now test whether $url is a string) both the $xhtml and the $ssl parameters were now honored.

Maybe you've seen that there are two different PHP files in mod_login, one for the login form and another one for the logout form. This has been changed some years ago (in August 2012) but it wasn't reflected e.g. in the beez3 template which still only have a template override for the login form. So when using the beez3 template you get the login form from that template but the default logout form from mod_login. I assume that this is the case with many other templates as well (as long as they override the login form). On the other hand the beez3 template still uses "index.php" as the login form's target which means that the "usesecure" setting will be honored if activated.

When implementing your own versions of the login and logout forms for your template you might very well call JRoute::_() with "index.php" as path since the path to be redirected to is given in the hidden "return" field - at least as long as you're not analyzing your visitor's behaviour.

The changes I'm about to complete and pass to github will cause all login and logout form overrides to be changed too since the "usesecure" setting will be no longer specific for each "login" module but it will come from the com_users component. But when I'm looking at the beez3 template where changes made to the mod_login module more than 3 years ago weren't reflected until today I don't think this will be a blocking point for my changes. It's maybe even possible to copy the "usesecure" setting from the com_users configuration to the module specific configuration in $params prior to executing the code for the login or logout form - this way many templates would continue to work as expected if they pass "index.php" as path and "$params->get('usesecure')" as $ssl.

@jo-sf
After some late night digging it was revealed to me how JRoute::_() works, even before I checked if issue was reported on GitHub, so I understand you perfectly. Initially I blamed JRoute::_() for not being coded properly but then I realized that JRoute::_() was not meant to be used the way it was used in mod_login. Rest of the Joomla was relying on those "index.php", "&" and not array tests.

Making the "usesecure" setting global instead of per module is the way to go but I already see people complaining that they can't do per module override of the setting. So it's probably the best to have per module setting with values "Use global", "YES" and "NO".

@bvitnik
I think there is some inconsistency between the inline documentation for JRoute::_() and its current implementation. The documentation at the beginning of the method says that the parameter $url is a string, but the check

if (!is_array($url) && ...

shows that $url might also be something else, apparently an array. My further investigations in this method showed that except for this check the method would work with an JUri object passed as $url very well, so I changed this check to

if (is_string($url) && ...

Doing so the switching from HTTP to HTTPS in the login form of mod_login worked as expected when I pass the current path as a JUri object to that function. There is another small change necessary in that method in order to switch back from HTTPS or HTTP protocol which should be done if you pass the value 2 for the parameter $ssl.

About the "usesecure" setting. In general I agree with you that it's best practize to have global settings that I can override e.g. in a module, an article or a menu item. But in this special case (login form) this is not true. With the activation of "usesecure" i want to achieve that usernames and passwords are always transferred vir HTTPS to the server. If I'm able to generally activate this and to deactivate this for a certain login form it would be possible to sniff the username and the password being transferred using this login form. And since Joomla uses one single user administration the evil one might login with the sniffed information using any of the login forms, and if this was the login of someone allowed to login at the backend too also the backend of the site is compromised.

Moreover the setting whether the registration URL shall be shown together with the login form exists only in the com_users settings, there is no way to deactivate it for any single login form. And so I plan to move the "usesecure" setting from mod_login to com_users.

@bvitnik
I did some more investigations on JRoute::_() and I found out that it returns the current URI when called with "index.php" as its first argument, but only if the current URI is a menu item. But when the current page is an article from e.g. a category blog the method returns the URI of the menu item for the category blog.

So passing "index.php" to JRoute::_() is not as bad as it looks for the first sight, and in the templates I checked (among them was beez3 and some other templates) the mod_login template overrides still use "index.php" as the first parameter.

Nevertheless I had to change the mod_login templates for login and logout such that they no longer call JRoute::_() for the form action URL since I found some cases where the result was sort of garbage. In my test environment I construct the form action URL without using JRoute::_() and it seems to work very well.

Moreover I moved the "usesecure" setting from mod_login to com_users and I import this setting in the mod_login parameter list prior to calling the login or logout template. This way those templates can still use $params->get('usesecure') as is the case in (I assume) most templates overriding the login and logout templates.

Another change I implemented is about using non-standard ports (means any other port than 80 for HTTP and any other port than 443 for HTTPS). This is relevant if you switch between HTTP and HTTPS as is e.g. the case when you activate "force_ssl" in the server configuration. So I added two new fields for HTTP and HTTPS ports to the server configuration which become visible as soon as you activate "force_ssl". In addition I had to modify all places where switching between HTTP and HTTPS is performed.

In testing I found a small security issue. If you try to login and make a mistake either in the user name or the password you get the login mask from com_users and in this login mask the user name and the password typed in before will be prefilled in this mask. For the user name this might be OK, but not for the password. So I changed this too.

I hope I'll find enough time the next days to create a pull request with all my changes. Probably I will create a separate pull request from the security issue mentioned above...

@jo-sf
Keep up the good work :).

@bvitnik
The two PRs are ready:

• PR #8604 for the small security issue (one file changed)
• PR #8619 for the rest (37 files changed)

Maybe you have the time (and the interest) to test my changes, they are based upon joomla:staging, so you probably need to copy the changes manually into your installation.

Would be nice to hear from you whether these changes work for you (see also the testing instructions in PR #8619 for details).

@jo-sf
Maybe on weekends. I hardly have any free time on workdays.

@bvitnik
No problem! I needed that long to build and test these changes, so it doesn't matter whether you're able to test my changes next day or later. I even assume that it will take some time until the Joomla team will be able to inspect and test my PR, leave alone to (hopefully) apply these changes to the Joomla code.