Confirmed

I used a full url (internal as it must not be external) and it works here, whether as a menu item or from the login module.

Forgot to say I tested on staging.

Can you explain what you mean by a full url.
In my test I redirected to "blog"
It worked on one site and didn't work on the other
On 15 Dec 2015 8:26 am, "infograf768" notifications@github.com wrote:

Forgot to say I tested on staging.

—
Reply to this email directly or view it on GitHub
#8689 (comment).

I used something like (Sample Sites in sample sql)
index.php?option=com_content&view=article&id=38

tested on an updated 3.4.5 and I had no issue, whether before killing the session or logging with another browser where no session was set.

Thanks I will retest with a non-sef url. Can you also test with a sef url

On 15 December 2015 at 08:51, infograf768 notifications@github.com wrote:

I used something like (Sample Sites in sample sql)
index.php?option=com_content&view=article&id=38

tested on an updated 3.4.5 and I had no issue, whether before killing the
session or logging with another browser where no session was set.

—
Reply to this email directly or view it on GitHub
#8689 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

No issue either when using
http://localhost:8888/Joomla_3.4.5/index.php/content-modules here

I personally worked on this for the Joomla 3.4.6 release.

There is no regression - there is a security fix.

Let me explain, prior to Joomla 3.4.6 there was a security bug that allowed a hacker to redirect a user after login through incorrect use of the redirect url, as it can be overwritten by user supplied data.

In Joomla 3.4.6 additional hardening of JURI::isInternal() took place - with full unit testing (a rare thing in Joomla!) the isInternal() function was truely hardened.

To be clear, as the docs are not, the redirect url MUST be an internal url, it MUST start with index.php? and be a non-sef url.

Examples:
index.php?option=com_content&view=article&id=38

Incorrect examples of a redirect url:
http://bbc.co.uk/
http://mysite.com/blog
/blog

Yes these might have worked in the past - but that was due to a bug in the way Joomla validated the urls. Now that security has been applied and the urls tested correctly the above examples will fail.

My tests with Joomla 3.4.6 (NOT staging) using the test sample data

1. index.php?option=com_content&view=article&id=38
   Works correctly

2. index.php/park-home
   Redirects to index.php/parks-home/login with a 404

3. /park-home (with full sef enabled)
   Redirects to /user-profile/profile

This undocumented change has now broken existing web sites which were setup
correctly

On 15 December 2015 at 09:03, Brian Teeman brian@teeman.net wrote:

Thanks I will retest with a non-sef url. Can you also test with a sef url

On 15 December 2015 at 08:51, infograf768 notifications@github.com
wrote:

I used something like (Sample Sites in sample sql)
index.php?option=com_content&view=article&id=38

tested on an updated 3.4.5 and I had no issue, whether before killing the
session or logging with another browser where no session was set.

—
Reply to this email directly or view it on GitHub
#8689 (comment)
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@brianteeman what you were relying on in the past was Joomla allowing incorrect handling of the passed url to the redirect param. What has now happened is that its been "fixed" to work correctly and the security bug you were relying on in the past has been fixed.

The redirect url should be an internal url starting with index.php. This has always been the case, but due to a security bug, other input has worked.

Please see the examples and unit tests at the bottom of this file:
https://github.com/joomla/joomla-cms/blob/staging/tests/unit/suites/libraries/joomla/uri/JURITest.php

As stated in my example a url with index.php/parks-home did not work - that
type of url is not in the unit tests

You may say it has always been the case that a non-sef url should be used
and it was a bug that you could. I dont really care about that

What I care about is that existing web sites that were working perfectly
have now been broken. There is nothing in the release notes to say that.
The tooltip for the "link" does not state that you must use a non-sef url
etc etc

On 15 December 2015 at 09:17, Phil Taylor notifications@github.com wrote:

Please see the examples and unit tests at the bottom of this file:

https://github.com/joomla/joomla-cms/blob/staging/tests/unit/suites/libraries/joomla/uri/JURITest.php

—
Reply to this email directly or view it on GitHub
#8689 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

It would have been easier if the redirect link in the menu item was the same as in the login module which does NOT ask for a url but presents a dropdown select of available menu items to redirect to.

Thats much easier to use but his will still mean that anyone using a redirect will have to update their link

I agree further work on communicating should be done - however the tool tip states that it should be an internal url and not an external one.

In Joomla 3.5.6 you select the login/logout redirection URL from a dropdown in the module - without any typing - in the module, however I see now that when you create a menu item to the login page, the options allow you to type in anything you like - although the tool tip states it must be internal and not external there is little other assistance.

The fix was to fix a security issue

I have added another test for partial SEF and it tests correctly as an internal URL so your bug is not related to the changes made to the JUri::isInternal function.

     * Test hardening of JUri::isInternal against non internal links
     *
     * @return void
     *
     * @covers JUri::isInternal
     */
    public function testPartialSEF()
    {
        $this->assertTrue(
            $this->object->isInternal('index.php/parks-home'),
            'index.php/parks-home should be internal'
        );
    }

The fix was to fix a security issue

doesnt change the fact that it has broken existing web sites

No, it has fixed a security issue that you were relying on - there is a huge difference.

@Klipper can you please provide the examples of the URLS that you have set manually as your redirect url, as well as telling me if they are the login or logout redirection urls please.

Thinking aloud
Isnt is possible for Joomla! to try to unwrap the SEF URL to non-SEF before redirection and THEN perform the isInternal() check.

yes - although this has never been a feature, and if the plan is to move to a "select menu item to redirect to from the dropdown" then it would not be a needed feature.

It cant be a plan as it would break backwards compatibility but then you
just did that with your untested code

On 15 December 2015 at 09:45, Phil Taylor notifications@github.com wrote:

yes - although this has never been a feature, and if the plan is to move
to a "select menu item to redirect to from the dropdown" then it would not
be a needed feature.

—
Reply to this email directly or view it on GitHub
#8689 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

The code was tested Brian, by several humans, and came with full unit tests both before and after refactoring of the 3 lines of code.

The security issue reported was resolved and the documented feature still works correctly after application of the changes. I'm sorry that you chose to rely on buggy insecure code for your redirection, however I make no excuse for coding in a secure manner - even if that breaks your choice to use the bug to your advantage.

The changes are minor - and only effect the isInternal function, and only check if the provided url is an internal one - as it has always done - but now it correctly identifies user supplied data of urls which are not internal as such - whereas before a hacker could redirect you to anywhere on the internet if they so chose.

Also I believe it is the case that the redirection page will become a dropdown, as this is already the case when adding a link to the Logout page as a menu item, and when editing the Login module - the only place this is not yet the case is on a Login Menu Item options.

I have just tested again, manually, and then with unit tests, the partial sef url "index.php/search" as the login redirection page in a menu item it works as advertised, redirecting me to index.php/search on login.

I still see no bug here, when the correct url is provided, starting with an index.php as it always should have been the case, (as thats the way the code was written).

Internal link - a link on my site
External link - a link on another site
On 15 Dec 2015 9:57 am, "Phil Taylor" notifications@github.com wrote:

The code was tested Brian, by several humans, and came with full unit
tests both before and after refactoring of the 3 lines of code.

The security issue reported was resolved and the documented feature still
works correctly after application of the changes. I'm sorry that you chose
to rely on buggy insecure code for your redirection, however I make no
excuse for coding in a secure manner - even if that breaks your choice to
use the bug to your advantage.

The changes are minor - and only effect the isInternal function, and only
check if the provided url is an internal one - as it has always done - but
now it correctly identifies user supplied data of urls which are not
internal as such - whereas before a hacker could redirect you to anywhere
on the internet if they so chose.

—
Reply to this email directly or view it on GitHub
#8689 (comment).

More like relative and absolute maybe.

What Im looking for here, from people, is concrete examples of urls they believe should be allowed to be used, and fail. At the moment I have not got that from the opening poster or Brian.

Not one person who tested the fix before release expressed any issues - and with full and complete unit test coverage before and after I have faith that the changes apply the security required, while at the same time allowing valid internal urls to be used as they have always been.

However if you have been relying on a security bug, and relying on the lack of filtering of user supplied information, then I make no excuse for breaking that. Joomla needs to be secure.

I have provided valid internal urls that do not work any more

On 15 December 2015 at 10:32, Phil Taylor notifications@github.com wrote:

What Im looking for here, from people, is concrete examples of urls they
believe should be allowed to be used, and fail. At the moment I have not
got that from the opening poster or Brian.

Not one person who tested the fix before release expressed any issues -
and with full and complete unit test coverage before and after I have faith
that the changes apply the security required, while at the same time
allowing valid internal urls to be used as they have always been.

However if you have been relying on a security bug, and relying on the
lack of filtering of user supplied information, then I make no excuse for
breaking that. Joomla needs to be secure.

—
Reply to this email directly or view it on GitHub
#8689 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@brianteeman The redirection of index.php/parks-home is not related to this code change I made. The url still validates as being an internal url correctly.

The only code changed in Joomla 3.4.6 around the urls was the validation of isInternal which simply returns true or false.

I have installed Joomla 3.4.5 and set the login menu item option to redirect to index.php/parks-home and on login this gives a 404 after a redirection to index.php/parks-home/login thus proving that the redirection issue @brianteeman is reporting is nothing to do with the security applied in Joomla 3.4.6 at all and was infact a bug in earlier versions of Joomla.

Im still waiting to her from the opening poster with the urls they are trying to redirect to - but I have conclusively ruled out @brianteeman report as invalid against Joomla 3.4.6 as the redirection bug he is reporting was there in Joomla 3.4.5 and is unrelated to validation of user input, but rather is a redirection issue.

Hi @PhilETaylor

How about the case #3 which Brian reported

/park-home (with full sef enabled)

isn't it a valid internal URL?

As a login redirect with SEF enabled this works for me:
index.php/en/the-page-i-want-to-display

If I want a logout redirect to the homepage I need a slash in front - is that 'working as designed?'
/index.php

index.php only redirects to the login form with the url http://mydomain/en/2015-10-05-06-59-09
index.php/en gives a 404 error.

@Gitjk /index.php is not a valid input as it should start with index.php always. /index.php would fail in Joomla 3.4.6 that is correct.

Again, the reason this might have worked in older versions of Joomla is that there was no validation or filtering for malicious redirects at all. So valid, malicious, and invalid input was allowed in the backend and on the frontend an attacker could overrule your request with a crafted returnUrl param.

@joomdonation Why not test it yourself instead of asking. As it happens, in Joomla 3.5.6 a login redirection to /parks-home does not resolve as an internal url as it doesnt start with index.php and as already stated, the correct data entry should be the non-sef version of the url

Again, the reason this might have worked in older versions of Joomla is that there was no validation or filtering for malicious redirects at all. So valid, malicious, and invalid input was allowed in the backend and on the frontend an attacker could overrule your request with a crafted returnUrl param.

In a menu login setup, the full url also works afaik. It works different from the module construction. The module setup has a dropdown, the menu item type login has an open textfield. There you can solve the redirect problem with the complete url.

The module type dropdown to an Itemid is the future. This is how it should work in the menu item options - but no one has changed that yet. When the dropdown selection is used, the JURI::isInternal() method changes in Joomla 3.5.6 is not even invoked as we know the menu item selected is internal.

The hardened method changed is here:

You can clearly see what it will and will not allow - however I agree its not easy to see :-) thats why there are loads of unit tests on it!!

The redirect url I used as value for 'Login Redirect' was '/InlogHome' a menu-item in a 'dummy' menu (no menu-module attached). This 'InlogHome' menu-item pointed to the final page I want to redirect to.

In my humble opinion this is a internal (SEF) url an not an external url as Brian said.

Reading all your comments I managed to fix the redirect by changing the URL in 'Login Redirect' to : index.php?option=com_content&view=article&id=54&Itemid=336
I had to add the &Itemid=336 to get all the params, moduleassignments etc. of the 'InlogHome' menu-item.

I understand that this is a securityfix as Phil explains, but there has been no documentation about.
I agree that the way a redirect gets chosen using a dropdown is much better.

I agree that we could add SEF Support to this, but that would mean reverse engineering and rerouting of the url back to non-sef before checking it - as without that there is no way to tell if /parks-home is internal as /customDevScript folder might have a index.php in it and be malicious.

Reading the unit tests you will see that a hell of a lot of thought has gone into getting this right. about 3 days full time for a change of only about 3 lines big (!!) and as a result a lot of malicious redirects are now being blocked where they were not before.

BTW using index.php/LoginHome returns 404 error

@Klipper that depends on your site - as index.php/login works with the default data

"/index.php would fail in Joomla 3.4.6 that is correct."

In my J3.4.6 /index.php in the logout redirect field doesn't fail. It redirects to the homepage, which is what I intend to do in this case.

Not sure if this is supposed to work either, but it does work without any error...

Login menu item's login redirect is set to / only and it does send folks to the homepage after login.

After reading through @PhilETaylor's comments I did update it to point to the proper Itemid number instead using index.php?Itemid=

I agree that we could add SEF Support to this

IMO, it's a must. If Joomla can't validate SEF URL's correctly as internal to the site, then the whole point of using them is quickly defeated.

actually the only reason for the isInternal check is security, its not used as a "is this url sef or not, an internal url" Its not about validating SEF urls at all - its about making sure that user supplied strings are not offsite urls

The real fix is not to allow hackers/user to provide base64 encoded returnUrls in the requests - whoever implemented that should be shot! Without this fix a hacker could email you a link, and when you login, he could direct you to any url on the internet - one that could then CSRF and do whatever it liked - coupled with the security issues in com_templates that I fixed this could be to upload/delete anything any where he likes!

Wouldn't adding SEF Support just be a moot point if the ultimate end goal is to have the menu items dropdown list that people select from?

like I said - the correct way to "fix" this is to remove the crappy passing around of urls in base64 encode as params to forms and links. That will take time, if it ever happens.

Doesn't the Logout Menu Item Type use a dropdown style to give the users a list of menu items to redirect people to? If so, couldn't Joomla just re-use that code and incorporate it into the Login Menu Item Type?

No, the login module has a dropdown and the logout menu item type (redirect after login / logout) is free text.

Oh, right. So the Login Module could be a starting point then for someone to tackle this feature.

It should be - but in doing so it would break the backward compatibility of people being able to paste in whatever junk they link and redirecting to that. So it will probably never get fixed in the 3.5.x series...

Please lets refrain from personal comments. Its not my B/C policy it is Joomla's

Of course lets not break a policy on security grounds.... shudder the thought...

When you guys are finished, perhaps you could issue some sort of bulletin on this? Regardless of the reasons behind the change, this has "broken" many sites currently in production, and the cause is less than obvious.

I think breaking B/C compatibility because of security is not the problem of course.
Breaking B/C compatibility because of security without any documentation is a the problem.

As I wrote, I managed to fix the redirect, but I probably I never could, when I didn't added this issue at the issuetracker to find out what was going on. Please add clear documentation when a fix gives B/C troubles!

Actually it has fixed millions of sites from a security issue so the theoretical "many" sites are better off.

There is nothing to fix - the current released code is acceptable, and works as intended - if the fixing of the broken security on the redirects effects you, then replace your redirects with valid non-sef urls starting with index.php

I don't see "many broken sites" in production.

Furthermore, there are no plans for Joomla 3.4.x releases at all - so there are no opportunities for improvements to this.

I will of course add a note in the correct place - which is here https://docs.joomla.org/Category:Version_3.4.6_FAQ

https://docs.joomla.org/Category:Version_3.4.6_FAQ updated with

===== Login/Logout Redirections broken after upgrade to Joomla 3.4.6 =====

Please see the [https://github.com//issues/8689 tracker item] for more details. Basically the security of redirection after login/logout was hardened in Joomla 3.4.6 - and this fixed a bug that some relied upon for years. If you are using a manually set url in a login/logout menu item options, then you must make sure that is an internal url, of non-sef, starting with index.php so that Joomla redirects you correctly. It has always been the case however it was not until now that validation took place. Also note that in other places and in Joomla 3.5, you are no longer be allowed to manually set a url - you only get a dropdown box to select a menu item to redirect to - this is the future and is much more secure.

@joomla/security Please clarify if it is the intention going forward that JUri::isInternal() will only correctly validate non-SEF URL's starting with index.php (in essence, limiting the method to only validating a URL is internal to the Joomla application). If this is indeed the case, then please provide documentation for developers to securely validate that a SEF URL or other URL which may be internal to the domain but not the Joomla application (for example, an article on https://developer.joomla.org which links to https://developer.joomla.org/cms-packages/ which is not part of the Joomla application but internal to the domain) can be validated. As this is a backward compatibility break for security reasons, IMO it is fair to require the JSST to document in full the break in the Joomla API (not just this specific end user feature) and provide documentation for implementations which may be valid use cases beyond the new scope of this method.

It has always been the case that this function was used to "limiting the method to only validating a URL is internal to the Joomla application" - however it was broken and not fit for purpose.

The only thing that is "now broken" is that the function, that was not fit for purpose, now is secure and validates input correctly - as per the unit tests and use cases presented to me to fix.

Within Joomla Core Code - NEVER is JUri::isInternal() called with anything other than a non-sef url starting with index.php - therefore it is safe to assume that it its intended use - and thus all tests and code were written on that basis. JUri::isInternal() is normally (and exclusivly in the core) used just before passing the string to the redirection like the code below, which in itself is wrapped in JRoute::_() to SEF-enable it. Thus again - if this is how ALL the core code is written then there is no way developers should be doing anything different or expecting the code to do anything other than the way I coded the security fix.

$this->setRedirect(JRoute::_($redirect, false));

If you were relying on the "broken-ness" of the feature then I'm sorry - its no longer broken. That to me is progress and not a b/c break.

Specifically - it was hardened against being able to call non-joomla urls on the same domain (i.e. /customDevScript.php?www.example.com)

As there are no plans for Joomla 3.4.7 there is no opportunity to change this to provide additional features that it never had, namely to validate if a url is within the same domain, albeit some other app, or to use SEF Urls.

Joomla 3.5 is soon to be released and I do not believe any security changes have been made here https://github.com/joomla/joomla-cms/blob/3.5-dev/libraries/joomla/uri/uri.php#L265 so you will be free to revert to your insecure non-validations in Joomla 3.5.0 unless someone changes that.

The 3.5-dev branch is dead as it has been merged to staging already. It only remains open because of a few outstanding pull requests.

Please provide documentation backing your claim that the function was limited to only the purpose you define. The only reference I've located is an obscure page in the docs wiki based on the 1.5 API. The inline documentation only states "Checks if the supplied URL is internal", which is extremely vague. I could argue therefore that you've rewritten the API to suit a purpose you seem to think is the One Right Way™ and are telling Joomla users and developers to live with it or hack the API.

I am not demanding the patch be reverted. I am asking that the JSST provide documentation to validate what could be assumed to be valid use cases that are no longer valid based on the security fix and have provided one use case that demonstrates it is indeed valid to consider a redirect outside the Joomla application internal to the overall website or domain.

Within Joomla Core Code - NEVER is JUri::isInternal() called with anything other than a non-sef url starting with index.php

Negative. The redirect system plugin attempts to validate URLs as internal (see https://github.com/joomla/joomla-cms/blob/3.5.0-beta/plugins/system/redirect/redirect.php#L110-L116) to determine if the URL should be routed via JRoute or to just use the user input URL as given.

The current router might makes it not easy but I think isInternal should return true if it is an internal URL, I don't care if it is a SEF or non SEF url, internal means I am staying on the same server or "not leaving the CMS". That's what I would expect when I am new to the framework and develop an extension. So as long as I am not leaving that what Joomla is controlling then isInternal() === true. If that is not the case then it is a B/C break

ah well - Im not sure then exactly what you are demanding - there is nothing I can do other than explain the reasoning behind the changes, the reasons the change was implemented in the way it was, and provide the passing unit tests.

When this was coded, and tested, no one considered its wider use outside of the Joomla Site - that to me, is not internal. Assumptions had to be made based on the core Joomla code, and the vulnerability that was presented. If those assumptions were wrong, and the subsequent peer review and code testing were wrong then I'm sorry - but again, there is nothing I can do now.

Saying that a jump from Joomla to Magento, because its on the same server in the same webspace - is, I had assumed rightly or wrongly, not the purpose of isInternal()

As for 3.5 dev - who knows where that is hiding now - could be anywhere - as I have repeatedly proved.

The unit tests show what is and what is not acceptable based on these changes, and all unit tests pass. They show clear examples of what would and would not be acceptable as urls passed to isInternal in order to get a true/false return.

The security changes took a considerable amount of time to ensure they covered all the tested variants and I have no more answers to give. Or time.

@PhilETaylor I am not blaming you for the changes, I am saying that we have a use case that we need to cover, not more not less. Mention the unit tests is in this case not really valid, because you changed the code and the tests at the same time. So it is not a surprise that tests pass.

I'm not arguing against hardening the method by any means. However, I feel the interpretation you chose to make of the method's intent leaned rather heavily toward a very specific purpose and did not lean toward a wider variety of potentially valid use cases that could be considered acceptable given the extremely vague method documentation and the pre-existing behavior. What's done is done, if the stance being taken is that no further changes are going to be made then so be it.

I don't think it's necessarily right to limit isInternal() to only within the Joomla application. There are some rather edge cases that I think warrant allowing some additional flexibility. Something I just thought of reading your "jump from Joomla to Magento" comment would be a SSO bridge. Supposing someone had a Joomla installation which was their primary user management system and operated the same domain with a phpBB and Magento install running in parallel to the Joomla install (for this to work it'd most likely need to be a subdirectory) and had written extensions to route those application's login/out processes into Joomla, the user could not be redirected back to those applications without bypassing the redirect in com_users now.

Maybe this means adding an optional flag to isInternal() to allow it to validate something is internal to the domain without being internal to Joomla. I don't know the right answer, but I do feel that the change in isInternal() made a change that hurt user experience more than necessary.

@rdeutz All existing tests (albeit not many!) prior to refactoring still passed after refactoring - that is the purpose of having tests and therefore its a valid point to state that they still pass. The unit tests also clearly document the type of input that will and will not return a true or a false from the method.

@mbabker All the examples I saw in the Joomla code, were just before a redirect, and if the isInternal was false then the url provided by joomla core was always index.php?option=com_something and therefore I thought it was right to assume that it was correct to assume that the input to isInternal should be at least the same. Redirecting to anywhere other than a Joomla Url to even SSO or magento within the same webspace was and should be considered a security risk based on the vulnerability presented.

ESPECIALLY as a hacker can override and provide ANY URL base64 encoded in the "return" param in the GET/POST!!!! This is where the root security issue is - why for petes sake is Joomla passing around base64encoded urls as form/link params! This is the design issue that really needs fixing.

No where within Joomla core does such a redirect happen and maybe wrongly I assumed that was correct.

So if I have made wrong assumptions based on reading code, and as you admit - scarce inline documentation - and interpretation of "internal" & "external" then I'm sorry - but hey I don't see anyone else stepping up to fix these things. The reporter was even too busy to fully test his own report!

If base64 encoded is the wrong way to go, then would this official documentation page be in need of an update? (I'm looking at the "Overriding" section in particular)

that is a clear example of everything that is wrong /facepalm/

@PhilETaylor I will no go into much detail but prior test testing other behauviour, the tests you added testing also the isInternal function. Just to make sure someone/you doesn't get it wrong, I like that you wrote tests, thats that way we should do it.

What we are doing here is to find out what is the right way and common sense of understanding the function. If you came across the function, how would you think what isInternal should do?

Lovely. Now I'm wondering about those 3rd party extensions that offer to "redirect after login" and what methods they are using to accomplish redirecting after folks log in.

@justinherrin they probably use the insecure passing around of base64 urls as that is all Joomla allows - the correct way would be to have that in the session and for Joomla to redirect based on the session value instead of some manipulatable url param.

@rdeutz If I came across the function I would do exactly what I did, look for example of how its used, and use it just like others do - and thats why I did. If I were to approach the security issue again I would probably come to the same assumptions - although I would have begged for extra time and ripped out the whole insecure mess of passing around base64encoded urls and actually do this is a more secure manner than manipulatable params... however we have to safeguard b/c at all costs apparently - something that seriously holds back the Joomla project ...

Redirecting to anywhere other than a Joomla Url to even SSO or magento within the same webspace was and should be considered a security risk based on the vulnerability presented.

It is a risk, absolutely. If you're installing and using SSO type solutions, you need to be aware of the inherent risks that are present, but Joomla should not be making the bold statement that you can't redirect outside Joomla under any condition. Anyone maintaining such a solution now has to either override the frontend com_users MVC to bypass the internal check or create their own component to handle the bridge functions (authentication and redirection).

This issue affects the default template of the article view of com_content (/components/com_content/views/article/tmpl/default.php). Line 139:

<?php $link->setVar('return', base64_encode(JRoute::_(ContentHelperRoute::getArticleRoute($this->item->slug, $this->item->catid, $this->item->language), false))); ?>

Running through JRoute returns a relative URL that fails isInternal(). This causes the CMS to "forget" the return URL when login is required to access restricted content.

The corrected line should be:

<?php $link->setVar('return', base64_encode(ContentHelperRoute::getArticleRoute($this->item->slug, $this->item->catid, $this->item->language), false)); ?>

I've changed this in an override, but seems like this should be something that is addressed in core code.

ok so that is 7 places that I can see. The correct way to fix this is to save the URL into the session and then retrieve it later - like I said - but that is not going to happen today.

I dont have all the answers - this is not going to get fixed in the Joomla 3.4.x series unless another 0-day vulnerability needs a release.

Its not the end of the world. But I agree its far from perfect.

Ironically I see that its put into the session during login:

// Set the return URL in the user state to allow modification by plugins
        $app->setUserState('users.login.form.return', $data['return']);

So someone was thinking along the right lines - but at the wrong point in the process.

And I see that before redirecting after login the URL goes through another JRoute::_() and so removing these like you say is not a problem as they still get SEFed before the redirect (line 86 com_users/controllers/user.php)

What are the 7 places? All in com_content? It would be helpful to know the extent of this so we can apply fixes ourselves in lieu of a released fix.

this is not related to logon redirect, but is related to a js redirect to an internal url no longer working after upgrading to 3.4.6. reading the thread, i think this may be related...

my site www.skyrun.com has js on the homepage that routes users to non-SEF versions of urls (even though i run with SEF on) an example of a url would be:
index.php?option=com_jomholiday&view=item&id=1&Itemid=185

this no longer works as a js redirect, but works if i type in www.skyrun.com/index.php?option=com_jomholiday&view=item&id=1&Itemid=185 on joomla 3.4.5

tried putting a / on front and http://www.skyrun.com/ on front but something is rewriting the url to www.skyrun.com/index.php (without all the parameters). this did not happen in joomla 3.4.5.

use-case:
1) visit www.skyrun.com
2) type '8401' and wait for the dropdown box and press search (can see the url inspecting source)

this redirects to www.skyrun.com/index.php, used to direct to www.skyrun.com/index.php?option=com_jomholiday&view=item&id=1&Itemid=185

could this hardening have also impacted the way non-sef urls work from any js on a joomla site?? is there a way i could specify the url so that it would still work? need a way to redirect internally of course...

@rossfeldman see the attached PR #8698

I would not exclude the possibility of a 3.4.6, especially if we need to fix a regression.

@chrisdavenport 3.4.6 is already released - I think you mean 3.4.7 - and this is not a reason for a release.

@barrycox - you have bigger fish to fry - like your code is accessible to SQL injection, plainly gives the sql in plain text so hackers know exactly what to hack!!! You should remove/harden this asap! If you conduct a real search http://www.skyrun.com/autocomplete.php?search=8401 then you see that the json is

[
{
value: "index.php?option=com_jomholiday&view=item&id=1&Itemid=185",
title: "8401 Buffalo Lodge ",
text: "Keystone 1br Condo",
image: "undefined"
}
]

This has absolutly nothing to do with the isInternal hardening - if you can provide the source of autocomplete.php then maybe I can advice you better, however nothing I see here leads me to believe that this is connected.

Duh, yeah I meant 3.4.7. Why is fixing a regression not a reason for a maintenance release?

There are no plans for Joomla 3.4.7 as far as I am aware. Joomla 3.5.0 will be released early in the new year as far as I am aware. However, there were no plans for Joomla 3.4.6 as far as I am aware or the patches to other end of life series as far as I am aware - so who knows - I'm not in control, I'm just another unpaid contributor who gives up their time... I hold no badges...

Maybe @wilsonge will make the decision if he wants to revert and re-release - but I think the publicity would be very bad for Joomla for such a minor issue - and then by reverting we reintroduce a security issue again... not my call - I owned up and took responsibility to talk about it here - but thats as much as Im paid to do... oh wait, I'm not even paid for that :) (or maybe @rdeutz maybe?)

i removed the debug code that showed the sql error when you didn't pass a search parameter. thanks.

i think the issue is in the uikit autocomplete js that redirects to the url above in json. when it redircts from their code, it doesn't work, but when i type in the url above from my json, it does work. if i pass unrelated parameters, it's ok, but as soon as a pass option=com_something, something somewhere seems to take the parameters off the redirected url.

i am intrigued by the offer to look over the code though! how can i send it to you securely?

My offer was to check it was not related to isInternal() changes in Joomla 3.4.6 (you see, many people think I don't, but I do actually care if my code (ahem, Open Source Contribution) has broken something!) You can email me direct at phil@phil-taylor.com (and if you are a GPG or SMIME user than my encryption keys are widely distributed).

A comment that is not about the fix
but it is about the need of supporting return variable in URLs

they probably use the insecure passing around of base64 urls as that is all Joomla allows - the correct way would be to have that in the session and for Joomla to redirect based on the session value instead of some manipulatable url param

yes the above is better, but session is simply not enough!
and it is not enough, in 2 cases:

• if you have user browsing multiple pages in the website that use return URLs, you will still need at least some parameter for -indexing- the session array that will hold multiple return URLs

• also sending to users links with return URLs is needed, again in this case you will need either the return in URL, or will need to implement a URL variable for indexing but since user does not yet have a session you will need to add entries in a DB table and use the index, which sounds more work than supporting return in the URL

I agree at minimum we need to ensure that SEF URL's work properly here. I don't think this requires us to immediately release 3.4.7 with 3.5 so close on the horizon. Let's get the full extent of this issue fully documented and start working on a fix :) By the time that fix + unit tests are ready we will probably be around the RC's for 3.5 anyhow with christmas coming up!

also sending to users links with return URLs is needed,

This is the whole problem - these urls MUST be internal URLs else a phish can send you a link - you login to your site and then get redirected to a hackers page, who knows where you came from, and that you have an active session, and then can exploit you... This is the whole point of hardening the function

SEF Urls have NEVER been working properly here, they were not "validated" as internal urls - they just failed to be checked correctly - there was no checking...

@PhilETaylor

Yes, i agree that they must be "internal", i did not say otherwise

• i commented about strong need to support return URLs

• about if isInternal() should only support Joomla urls and not possible to redirect e.g. to Magento, i cannot comment about it now,

• i understand your arguments about need to do this fix

If you want to see how bad it used to be take a look at the before code - shocking...

At this point, honestly the only way forward is to come to a consensus on what the valid behaviors SHOULD be for each case where a return URL is allowed and what the expected behavior SHOULD be for determining what an internal URL is. Not what happens today, or on Sunday before the release went out, or what the intent was when most of this code was (re-)written eight or nine years ago. It's a blank slate.

• What scenarios dictate that a return URL must ONLY be within the Joomla application? How should that return URL be defined?
• What scenarios are acceptable (with full awareness that there is a security weakness in it) for a return URL to leave the Joomla application? How should that return URL be defined and validated?

To me, the questions regarding the use cases should be answered first as that helps to define the API definition and behavior. With that settled upon, then implementation details can be debated.

Anything else at this point is really getting close to an ego measuring contest to prove someone's "superiority" over others. It's established now there was a flaw in the pre-existing behavior and it was patched. It's established now that patching said behavior has broken what was previously acceptable use cases (legitimate or otherwise is not for debate).

Well I'll leave that to someone who can devote time to it and get it right... I'll bow out here...

@PhilETaylor wrote

I agree that we could add SEF Support to this, but that would mean reverse engineering and rerouting of the url back to non-sef before checking it - as without that there is no way to tell if /parks-home is internal as /customDevScript folder might have a index.php in it and be malicious.

If may website homepage URL is http://example.com/joomla
then http://example.com/joomla/index.php?option=com_content
will be an internal URL and JUri::isInternal() would return TRUE.

But JUri::isInternal() would return TRUE
also for http://example.com/joomla/customDevScript/index.php
which might be malicious as you mentioned
and it is not an internal link.

JUri::isInternal() has to check if everything what is before index.php matches to JUri::base().
Otherwise it is not an internal URL.
Currently JUri::isInternal() allows any URL with index.php when host matches to website host.

I have a simple idea, if I did not miss something, how to allow SEF URLs.
For example.
Extract from URL http://example.com/joomla/customDevScript/index.php
part of JUri::base() which is http://example.com/joomla
and check if rest of it is a file or directory
file_exists(JPATH_BASE.trim('customDevScript/index.php', '/'))
If it does not exist then it might be a SEF URL and we could assumed that it is the internal URL.

I think we can borrow some code from parseSefRoute method inside JRouterSite class to validate if a relative sef URL is an internal url of the site.

With that in mind, I come up with this code.

public static function isInternal($url)
{
    $app  = JFactory::getApplication();
    $uri  = static::getInstance($url);
    $base = $uri->toString(array('scheme', 'host', 'port', 'path'));
    $host = $uri->toString(array('scheme', 'host', 'port'));


    if (empty($host) && JString::strpos($uri->path, 'index.php') === 0)
    {
        return true;
    }

    // Try to check if this url is from a a menu item
    if (empty($host) && $uri->path && $app->get('sef_rewrite'))
    {
        $items           = $app->getMenu()->getMenu();
        $route_lowercase = JString::strtolower($uri->path);

        // Remove base path from url, in case site is in sub-folder
        $base = JUri::base(true);
        if ($base && JString::strpos($route_lowercase, $base) !== false)
        {
            $route_lowercase = JString::substr($route_lowercase, strlen($base));
        }

        // If the url start with /, remove the '/' character from URL before matching URL
        if ($route_lowercase[0] == '/')
        {
            $route_lowercase = JString::substr($route_lowercase, 1);
        }

        $lang_tag = $app->getLanguage()->getTag();

        // Iterate through all items and check route matches.
        foreach ($items as $item)
        {
            if ($item->route && JString::strpos($route_lowercase . '/', $item->route . '/') === 0 && $item->type != 'menulink')
            {
                // Usual method for non-multilingual site.
                if (!$app->getLanguageFilter())
                {
                    return true;
                }
                // Multilingual site.
                elseif ($item->language == '*' || $item->language == $lang_tag)
                {
                    return true;
                }
            }
        }
    }

    // @see JURITest
    if (!empty($host) &&
        (preg_match('#' . preg_quote(static::base(), '#') . '#', $base)
            || $host === static::getInstance(static::base())->host && strpos($uri->path, 'index.php') !== false
            || $base === $host && preg_match('#' . preg_quote($base, '#') . '#', static::base()))
    )
    {
        return true;
    }

    return false;
}

The only difference with what we are having now is that it tries to match the given url with the route of one the menu items created in the site. If there is a menu item route matches this url, it will return true mean the passed url is an internal URL

My initial tests show that it works good. And if this works OK, seems the PR #8698 is not needed which is a good thing

If anyone wants to review the change, please see https://github.com/joomla/joomla-cms/compare/staging...joomdonation:patch-22?expand=1.

that would need completely refactoring so that unit tests could be run on it.

Unfortunately, I don't have any experience with writing unit tests, so I think I will have to stop here, sorry.

There is also one more thing which I found while attempt to solve this issue, not sure if it is a bug with redirect method of JApplicationWeb. It happens when we have a site in sub-folder. For example, if the site is http://localhost/joomla and we set the redirect URL to /menu-item-alias (the one /park-home which Brian mentioned), the modified code above will return true which is expected

However, after that, Joomla will redirect user to http://localhost/menu-item-alias (http://localhost//park-home).

I expect that when the site is http://localhost/joomla and call JFactory::getApplication()->redirect('/menu-item-alias') will redirect user to http://localhost/joomla/menu-item-alias. However, users is actually being redirected to http://localhost/menu-item-alias which is an invalid URL.

I can help with getting the curent unit tests to run :) But we need to add new test cases for the SEF URL's etc as well as fixing up the existing ones. Can people think of the right cases please so I can code them :)

This issue has broken third party login modules such as SourceCoast's mod_sclogin. When trying to redirect to the same page (not a user specified URL or menu item), redirection to the user profile occurs:

https://www.sourcecoast.com/forums/non-commercial-extensions/sclogin-enhanced-login/11310-joomla-login-redirection-url-always-to-profile-with-patch-to-j-346#p56117

Jordan

I would like to point out that if using the stock Joomla mod_login and setting the login redirection page to 'default' then this correctly functions and remains on the same page after login. But specifying a redirection login as with com_users menu item fails.

There is a new issue #8728. Please find an solution for this redirection problem. Secutity is important but this make us a lot of trouble. What to say ourer customers?

Joomla 3.4.5 isInternal work fine

JURI::isInternal('/index.php?option=com_test'); - return true
JURI::isInternal('/login'); - return true

Joomla 3.4.6
JURI::isInternal('/index.php?option=com_test'); - return false
JURI::isInternal('/login'); - return false

So what I hear is that you would prefer to revert back to an insecure method - that doesn't actually check if a url isInternal even though that is what the method is called... or you want a complete refactoring of the method to support all kinds of different crap you can throw at it - including the ability to redirect to urls that are "internal" to the Joomla install, and "internal" to the same webspace (or same domain, or tld, or domain.tld.com/subfolder/) and all other kinds of definitions of "internal" ...

So until someone can give a complete list of the crap you want to throw at the function - and the reasons behind it - there is no code that can be written.

From a website at example.com, - you are demanding isInternal to return true if the input is

index.php
/index.php
/parks-home/ ( A SEF Url to the base joomla install)
parks-home
parks-home/
/subfolder/index.php ( A url to another different joomla install in a sub folder)
subfolder/index.php
subfolder/index.php?option=com_test
subdomain.example.com/index.php ( A url to another different joomla install on a sub domain )
subdomain.example.com/subfolder/index.php ( A url to another different joomla install in a sub folder on a sub domain )
example.com/index.php?option=com_test (When and only when com_test is installed)
example.com/index.php/com_test (When and only when com_test is installed and SEF on one mode)
example.com/test/ (When and only when com_test is installed and SEF on a different mode)
and yet more examples from you guys...
example.com/magento/ (whether or not magento actually exists in this folder!)
example.com/wordpress/ (whether or not wordpress actually exists in this folder!)

I'll add this as a gist so you can edit it and add your own examples...

Edit away here: https://gist.github.com/PhilETaylor/322a8528ab79d904e704

Basically I think we also disagree fundamentally on what "internal" means. I believe the correct definition should be internal to the Joomla installation, within the Joomla app/mvc/session etc... But already @mbabker conflicted with that wanting to redirect to anywhere within the same webspace...

There are no easy answers.

Also for anyone reading that has a problem - the simple solution is to change YOUR redirect URLS to a non-sef url starting with index.php (for the time being) - problem solved.... for now...

Please keep your language a little cleaner

On 18 December 2015 at 13:00, Phil Taylor notifications@github.com wrote:

Also for anyone reading that has a problem - the simple solution is to
change YOUR redirect URLS to a non-sef url starting with index.php -
problem solved....

—
Reply to this email directly or view it on GitHub
#8689 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Whatever - at least Im trying to take ownership and fix something instead of MOANING

i have not found that to be the case when the index.php has
?option=com_something after it.

Barry Cox, CEO
SkyRun Vacation Rentals
970-660-4422 or 877-SKYRUN-1 x801
barry@skyrun.com | biz.SkyRun.com | www.SkyRun.com

On 12/18/2015 6:00 AM, Phil Taylor wrote:

Also for anyone reading that has a problem - the simple solution is to
change YOUR redirect URLS to a non-sef url starting with index.php -
problem solved....

—
Reply to this email directly or view it on GitHub
#8689 (comment).

The unit tests clearly cover cases like index.php?option=com_something and they pass as being an internal url by returning a true.

@PhilETaylor If I understand this discussion correctly, SEF URLS passing the isInternal() test in 3.4.5 is (part of) the vulnerability that 3.4.6 tries to fix, right?
So, in 3.4.6 http://myjoomla.mydomain.mytld/gosomewhere should NOT pass the isInternal() test, right?

Actually this is what I did to "fix" my website after the 3.4.6 upgrade: In "login redirect" replace "gosomewhere" by "http://myjoomla.mydomain.mytld/gosomewhere". It works?!? - "gosomewhere" is a menu item.

@ItsReallyMeDE if you already have a menu item setup for the page you wish to redirect to, it would be best to use the non-SEF link. For example... index.php?Itemid=22 (where 22 is the menu item ID number)

That is not a valid URL and will not load the component

On 18 December 2015 at 15:23, Justin Herrin notifications@github.com
wrote:

@ItsReallyMeDE https://github.com/ItsReallyMeDE if you already have a
menu item setup for the page you wish to redirect to, it would be best to
use the non-SEF link. For example... index.php?Itemid=22 (where 22 is
the menu item ID number)

—
Reply to this email directly or view it on GitHub
#8689 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Hmmm interesting. I've been using that format in some situations without an issue at all. In any event, from what I'm ready in this issue's comments, the best practices method to pull off the redirect is to begin with index.php and not necessarily use a SEF url. And seeing how @ItsReallyMeDE already has a menu item created for the redirect page, I would assume he should be using that non-SEF link.

Try it with the default testing data. You will see it doesn't work

Sorry to insist, but I have understood what I "should" be doing.

The fact of the matter is, that the URL that I quoted DOES work in 3.4.6

Is it supposed to work after the security fix in 3.4.6 ?

I would just like to have a simple yes or no on that question :-)

<That is not a valid URL and will not load the component

in fact, this is exactly how it works for the login module with the dropdown choice.
See in its helper.

The unit tests clearly cover cases like index.php?option=com_something and they pass as being an internal url by returning a true.

See https://github.com/joomla/joomla-cms/blob/3.4.5/tests/unit/suites/libraries/joomla/uri/JURITest.php

There are no unit tests for isInternal in 3.4.5. So the only unit tests that are written and pass are the ones you wrote while applying this fix. You dictated what you thought the method should do upon the CMS and wrote tests that back your interpretation of that function and are using that as your claim that it functions exactly as it should.

But already @mbabker conflicted with that wanting to redirect to anywhere within the same webspace

I already mentioned what I think the way forward is. I'm not pointing fingers and I'm not going to dig out my steel toe boots to start kicking people where the sun don't shine in defense of my opinions. Intentional or otherwise, I feel you have broken what should be considered acceptable use cases for considering a URL internal. Hell, throw out the same domain crap and just focus on the fact that you've broken validation of SEF URLs in this function. So at a minimum, routes similar to index.php?option=com_content&view=article&id=1, index.php/component/content/article/1, and component/content/article/1 should validate as true; all are valid internal routes and all can be generated by Joomla's router.

index.php?option=com_something

This has always returned true from even before the unit tests!

It's hard to break something that was never happening to start with! The non secure code allowed pretty much anything to validate - even hackers external urls redirecting you to exploits! So you relied on that - you relied on the fact that the code was insecure and not up to the job - I have already agreed that my assumptions on locking it down were narrow, based on observed examples in joomla core only and in several people's opinions incorrect - and I have already agreed that the function can and should be improved further - what I'm looking for now is an agreement from interested parties on exactly the exact URLS they want to consider "internal" as we ALL have different opinions on what internal means!

Internal to me means powered by joomla in the same joomla site - so I agree this should be refactored to allow all crazy manor of sef routeable urls and other joomla routes like /administrator - allowing also for 3rd party sef

I don't agree that non joomla routeable urls are internal - /custom.php and /magneto/ should not be classed as internal in my opinion - however as you have said - we could further improve the API to allow passing additional flags so that the code API can be used by others in this way although joomla core would not

Sent from my iPhone

On 18 Dec 2015, at 16:50, Michael Babker notifications@github.com wrote:

The unit tests clearly cover cases like index.php?option=com_something and they pass as being an internal url by returning a true.

See https://github.com/joomla/joomla-cms/blob/3.4.5/tests/unit/suites/libraries/joomla/uri/JURITest.php

There are no unit tests for isInternal in 3.4.5. So the only unit tests that are written and pass are the ones you wrote while applying this fix. You dictated what you thought the method should do upon the CMS and wrote tests that back your interpretation of that function and are using that as your claim that it functions exactly as it should.

But already @mbabker conflicted with that wanting to redirect to anywhere within the same webspace

I already mentioned what I think the way forward is. I'm not pointing fingers and I'm not going to dig out my steel toe boots to start kicking people where the sun don't shine in defense of my opinions. Intentional or otherwise, I feel you have broken what should be considered acceptable use cases for considering a URL internal. Hell, throw out the same domain crap and just focus on the fact that you've broken validation of SEF URLs in this function. So at a minimum, routes similar to index.php?option=com_content&view=article&id=1, index.php/component/content/article/1, and component/content/article/1 should validate as true; all are valid internal routes and all can be generated by Joomla's router.

—
Reply to this email directly or view it on GitHub.

You are the only one who has a different understanding of what is internal

On 18 December 2015 at 17:04, Phil Taylor notifications@github.com wrote:

index.php?option=com_something

This has always returned true from even before the unit tests!

---

It's hard to break something that was never happening to start with! The
non secure code allowed pretty much anything to validate - even hackers
external urls redirecting you to exploits! So you relied on that - you
relied on the fact that the code was insecure and not up to the job - I
have already agreed that my assumptions on locking it down were narrow,
based on observed examples in joomla core only and in several people's
opinions incorrect - and I have already agreed that the function can and
should be improved further - what I'm looking for now is an agreement from
interested parties on exactly the exact URLS they want to consider
"internal" as we ALL have different opinions on what internal means!

Internal to me means powered by joomla in the same joomla site - so I
agree this should be refactored to allow all crazy manor of sef routeable
urls and other joomla routes like /administrator - allowing also for 3rd
party sef

I don't agree that non joomla routeable urls are internal - /custom.php
and /magneto/ should not be classed as internal in my opinion - however as
you have said - we could further improve the API to allow passing
additional flags so that the code API can be used by others in this way
although joomla core would not

Sent from my iPhone

On 18 Dec 2015, at 16:50, Michael Babker notifications@github.com
wrote:

The unit tests clearly cover cases like index.php?option=com_something
and they pass as being an internal url by returning a true.

See
https://github.com/joomla/joomla-cms/blob/3.4.5/tests/unit/suites/libraries/joomla/uri/JURITest.php

There are no unit tests for isInternal in 3.4.5. So the only unit tests
that are written and pass are the ones you wrote while applying this fix.
You dictated what you thought the method should do upon the CMS and wrote
tests that back your interpretation of that function and are using that as
your claim that it functions exactly as it should.

But already @mbabker conflicted with that wanting to redirect to
anywhere within the same webspace

I already mentioned what I think the way forward is. I'm not pointing
fingers and I'm not going to dig out my steel toe boots to start kicking
people where the sun don't shine in defense of my opinions. Intentional or
otherwise, I feel you have broken what should be considered acceptable use
cases for considering a URL internal. Hell, throw out the same domain crap
and just focus on the fact that you've broken validation of SEF URLs in
this function. So at a minimum, routes similar to
index.php?option=com_content&view=article&id=1,
index.php/component/content/article/1, and component/content/article/1
should validate as true; all are valid internal routes and all can be
generated by Joomla's router.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub
#8689 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Then maybe I should resign and let someone else work on this then!!!

Pathetic

Sent from my iPhone

On 18 Dec 2015, at 17:22, Brian Teeman notifications@github.com wrote:

You are the only one who has a different understanding of what is internal

On 18 December 2015 at 17:04, Phil Taylor notifications@github.com wrote:

index.php?option=com_something

This has always returned true from even before the unit tests!

---

It's hard to break something that was never happening to start with! The
non secure code allowed pretty much anything to validate - even hackers
external urls redirecting you to exploits! So you relied on that - you
relied on the fact that the code was insecure and not up to the job - I
have already agreed that my assumptions on locking it down were narrow,
based on observed examples in joomla core only and in several people's
opinions incorrect - and I have already agreed that the function can and
should be improved further - what I'm looking for now is an agreement from
interested parties on exactly the exact URLS they want to consider
"internal" as we ALL have different opinions on what internal means!

Internal to me means powered by joomla in the same joomla site - so I
agree this should be refactored to allow all crazy manor of sef routeable
urls and other joomla routes like /administrator - allowing also for 3rd
party sef

I don't agree that non joomla routeable urls are internal - /custom.php
and /magneto/ should not be classed as internal in my opinion - however as
you have said - we could further improve the API to allow passing
additional flags so that the code API can be used by others in this way
although joomla core would not

Sent from my iPhone

On 18 Dec 2015, at 16:50, Michael Babker notifications@github.com
wrote:

The unit tests clearly cover cases like index.php?option=com_something
and they pass as being an internal url by returning a true.

See
https://github.com/joomla/joomla-cms/blob/3.4.5/tests/unit/suites/libraries/joomla/uri/JURITest.php

There are no unit tests for isInternal in 3.4.5. So the only unit tests
that are written and pass are the ones you wrote while applying this fix.
You dictated what you thought the method should do upon the CMS and wrote
tests that back your interpretation of that function and are using that as
your claim that it functions exactly as it should.

But already @mbabker conflicted with that wanting to redirect to
anywhere within the same webspace

I already mentioned what I think the way forward is. I'm not pointing
fingers and I'm not going to dig out my steel toe boots to start kicking
people where the sun don't shine in defense of my opinions. Intentional or
otherwise, I feel you have broken what should be considered acceptable use
cases for considering a URL internal. Hell, throw out the same domain crap
and just focus on the fact that you've broken validation of SEF URLs in
this function. So at a minimum, routes similar to
index.php?option=com_content&view=article&id=1,
index.php/component/content/article/1, and component/content/article/1
should validate as true; all are valid internal routes and all can be
generated by Joomla's router.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub
#8689 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
—
Reply to this email directly or view it on GitHub.

I'm unsubscribed from this thread - if you don't like the code I supplied, which was tested by four others, peer reviewed, unit tested and secure - and keep the personal attacks coming - then you can now fix it yourself...

Good luck!

There are very few that understand the true security issue here

Those that are bucking the system with crazy external redirections will get a shock when the drop down approach is applied consistently so you can't provide your own string as a url! The drop down of menu ids in the module further supports my opinion that internal is a valid joomla internal menu item!!!

Fight amongst yourself ... I'm done with the personal attacks!

Sent from my iPhone

On 18 Dec 2015, at 17:22, Brian Teeman notifications@github.com wrote:

You are the only one who has a different understanding of what is internal

On 18 December 2015 at 17:04, Phil Taylor notifications@github.com wrote:

index.php?option=com_something

This has always returned true from even before the unit tests!

---

It's hard to break something that was never happening to start with! The
non secure code allowed pretty much anything to validate - even hackers
external urls redirecting you to exploits! So you relied on that - you
relied on the fact that the code was insecure and not up to the job - I
have already agreed that my assumptions on locking it down were narrow,
based on observed examples in joomla core only and in several people's
opinions incorrect - and I have already agreed that the function can and
should be improved further - what I'm looking for now is an agreement from
interested parties on exactly the exact URLS they want to consider
"internal" as we ALL have different opinions on what internal means!

Internal to me means powered by joomla in the same joomla site - so I
agree this should be refactored to allow all crazy manor of sef routeable
urls and other joomla routes like /administrator - allowing also for 3rd
party sef

I don't agree that non joomla routeable urls are internal - /custom.php
and /magneto/ should not be classed as internal in my opinion - however as
you have said - we could further improve the API to allow passing
additional flags so that the code API can be used by others in this way
although joomla core would not

Sent from my iPhone

On 18 Dec 2015, at 16:50, Michael Babker notifications@github.com
wrote:

The unit tests clearly cover cases like index.php?option=com_something
and they pass as being an internal url by returning a true.

See
https://github.com/joomla/joomla-cms/blob/3.4.5/tests/unit/suites/libraries/joomla/uri/JURITest.php

There are no unit tests for isInternal in 3.4.5. So the only unit tests
that are written and pass are the ones you wrote while applying this fix.
You dictated what you thought the method should do upon the CMS and wrote
tests that back your interpretation of that function and are using that as
your claim that it functions exactly as it should.

But already @mbabker conflicted with that wanting to redirect to
anywhere within the same webspace

I already mentioned what I think the way forward is. I'm not pointing
fingers and I'm not going to dig out my steel toe boots to start kicking
people where the sun don't shine in defense of my opinions. Intentional or
otherwise, I feel you have broken what should be considered acceptable use
cases for considering a URL internal. Hell, throw out the same domain crap
and just focus on the fact that you've broken validation of SEF URLs in
this function. So at a minimum, routes similar to
index.php?option=com_content&view=article&id=1,
index.php/component/content/article/1, and component/content/article/1
should validate as true; all are valid internal routes and all can be
generated by Joomla's router.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub
#8689 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/
—
Reply to this email directly or view it on GitHub.

Phil you're using one user level feature which implements JUri::isInternal() checks to dictate how the entire API is supposed to function. That's why I'm saying that at a high level the hardening has gone too far.

I have already agreed that and offered to refactor it.

But now someone else can do that - I'll use my free/contribution time I have available to test other security issues ... That someone else has written code for. I'm done here.

Sent from my iPhone

On 18 Dec 2015, at 17:50, Michael Babker notifications@github.com wrote:

Phil you're using one user level feature which implements JUri::isInternal() checks to dictate how the entire API is supposed to function. That's why I'm saying that at a high level the hardening has gone too far.

—
Reply to this email directly or view it on GitHub.

If our router was implemented properly, I would expect the code of JUri::isInternal() could just be simple like this:

public static function isInternal($url)
{
    $uri    = JUri::getInstance($url);
    $router = JFactory::getApplication()->getRouter();
    $vars   = $router->parse($uri);
    if (!empty($vars['option']) && JComponentHelper::isEnabled($vars['option']))
    {
        return true;
    }

    return false;
}

Sadly, our router always return the variables of home menu item in case it could not match any menu item with the given url (which is strange behavior, I think). For example, the code below:

$uri    = JUri::getInstance('http://google.com/abc');
$router = JFactory::getApplication()->getRouter();
$vars   = $router->parse($uri);
var_dump($vars);

return the following result:

array (size=3)
'Itemid' => string '101' (length=3)
'option' => string 'com_content' (length=11)
'view' => string 'featured' (length=8)

I found the code which causes this strange behavior but could not understand the reason behind it. Maybe someone understand deeply Joomla router could explain the reason of this strange behavior ?

The home menu item is the system default menu item. It's been discussed here in the past. It's a misleading label to call it only the home page.

I thought we had updated all the tooltip to be default not home
On 21 Dec 2015 1:44 pm, "Michael Babker" notifications@github.com wrote:

The home menu item is the system default menu item. It's been discussed
here in the past. It's a misleading label to call it only the home page.

—
Reply to this email directly or view it on GitHub
#8689 (comment).

In the edit view the tooltip for the Default Page field starts with "Sets this menu item as the default or home page of the site." and the grid view for the menu item list uses a home column with a tooltip that says "Default".

I know it is system default menu item. I just don't understand why the router return variables of system default menu item even when the given uri is even not a url of the site (like the sample URL http://google.com/abc in the sample code I wrote above)

There is also another issue with the router which I reported at #8742.

Seems our router still has some issues/bugs.

I would just like to mention that urlencode and base64_encode both add a forward slash to the string "/index.php...." Thus when testing an decoded variables using isInternal, it will fail.

OK. Today, I give it another try. Most of the code is borrowed from JRouterSite class. Below is the code:

public static function isInternal($url)
{
    $app  = JFactory::getApplication();
    $uri  = static::getInstance($url);
    $base = $uri->toString(array('scheme', 'host', 'port', 'path'));
    $host = $uri->toString(array('scheme', 'host', 'port'));

    // Validate base url
    if (stripos($base, static::base()) !== 0 && !empty($host))
    {
        return false;
    }

    // Decode URL to convert percent-encoding to unicode so that strings match when routing.
    $path = urldecode($uri->getPath());

    // Remove base path from url, in case site is in sub-folder
    $basePath = static::base(true);
    if ($basePath && JString::strpos($path, $basePath) === 0)
    {
        $path = str_replace($basePath, '', $path);
    }

    // Remove / character from the path
    $path = trim($path, '/');

    if (empty($path) || JString::strpos($path, 'index.php') === 0)
    {
        return true;
    }

    if ($app->get('sef_rewrite'))
    {
        // Remove language code if it is available in URL
        if ($app->getLanguageFilter())
        {
            $languages = JLanguageHelper::getLanguages('sef');
            $parts     = explode('/', $path);
            $sef       = $parts[0];

            if (isset($languages[$sef]))
            {
                $path     = str_replace($sef . '/', '', $path);
                $lang_tag = $languages[$sef]->lang_code;
            }
        }

        // Remove the suffix
        if ($app->get('sef_suffix'))
        {
            if ($suffix = pathinfo($path, PATHINFO_EXTENSION))
            {
                $path = str_replace('.' . $suffix, '', $path);
            }
        }

        $segments = explode('/', $path);

        if (count($segments) > 1 && $segments[0] == 'component')
        {
            $option = 'com_' . $segments[1];
            if (JComponentHelper::isEnabled($option))
            {
                return true;
            }
        }
        else
        {
            $items           = $app->getMenu()->getMenu();
            $route_lowercase = JString::strtolower($path);

            foreach ($items as $item)
            {
                if ($item->route && JString::strpos($route_lowercase . '/', $item->route . '/') === 0 && $item->type != 'menulink')
                {
                    // Usual method for non-multilingual site.
                    if (!$app->getLanguageFilter())
                    {
                        return true;
                    }
                    // Multilingual site.
                    elseif ($item->language == '*' || $item->language == $lang_tag)
                    {
                        return true;
                    }
                }
            }
        }
    }

    return false;
}

The base url must he validated. I use the code from Joomla 3.4.5

if (stripos($base, static::base()) !== 0 && !empty($host))
{
        return false;
}

Could @PhilETaylor helps review that part to see whether it is secure? If not, could you please help with the code to make sure that part (base url) validated properly ?

The path of given URL also must be validated:

• The / character is removed (using trim command) before the path is checked. So index.php, /index.php, or /index.php/ is the same

• If path starts with index.php, the url is internal url. This is the same with the original implementation of this method

• If the URL is a SEF URL:

• If the site is a multilingual website, the path starts with language-code/menu-item-alias/sub-menu-item-alias to be an internal url.

• If the site is a single language website, the url must has path starts with menu-item-alias/sub-menu-item-alias to be an internal url
• The special sef url component/content/article/1 is also considered as a valid internal URL

I think it covers most of the cases we discussed so far. Hope to receive feedback so that we can implement this method properly.

@joomdonation
I tested your proposed code here.
On a monolanguage site:
I used as redirect

article-category-list/73-nouveaupour-testmail.html

But my test site is in a folder, therefore I get a 404 as the link obtained on login redirection is

http://localhost:8888/article-category-list/73-nouveaupour-testmail.html

instead of

http://localhost:8888/trunkgitnew/article-category-list/73-nouveaupour-testmail.html

Now, concerning multilingual:
we just do not use the redirections set in the login module or login menu item.
see the method onUserLogin:
https://github.com/joomla/joomla-cms/blob/staging/plugins/system/languagefilter/languagefilter.php#L518-L595

As these depend on the user preferred site language and if login from an associated item or not.

@infograf768 : The code I proposed only used to check if the given URL is internal or not. It doesn't handle the redirection. The redirection is handled by different code https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/application/web.php#L479

From what I can see from the code, the redirect method doesn't handle redirection properly when your site is in a sub-folder. But that's a different issue.

Where are we on this? I am postponing my upgrade to 3.4.6 and beyond as I know this is an issue on my sites.

@skurvish No changes are planned to this for the Joomla 3.4.x series. Postponing your upgrade to Joomla 3.4.8 puts your Joomla site the situation where you will be hacked running Joomla 3.4.6 - if you dont like these changes you should still upgrade to Joomla 3.4.8 and then just change the core code you dont like to suit your own needs - afterall its open source and free (free as in freedom).

True, but then I am left with core changes I have to manage for every J! update.

Please can someone provide simple instructions of how to fix this issue for those of us who have upgraded and now are struggling. I think that I have tried with both internal and external URLS (not completely sure I understand the difference), neither work. My tests are documented here: http://forum.joomla.org/viewtopic.php?f=708&t=904115&p=3358221#p3358221

Thanks.

I am experiencing the same problem (Joomla 3.4.8), has a solution been determined? If so, please provide clear instructions on how to fix the issue.
Thanks so much,
Dawn

Hi,
I'm experiencing the same problem in Joomla 3.4.8 , and the trouble is in JUri::isInternal($data['return']) that detect

JURI::base(); -> http://localhost/torresbus/
$data['return'] -> /torresbus/index.php?option=com_buses&task=travel.procesdata

for JUri::isInternal($data['return']) is false, before update it works ok.

thanks a lot

I proposed a PR fixes this issue #9069. Could you (users had this issue) help testing the PR?

@infograf768, you inform us that

we just do not use the redirections set in the login module or login menu item.

Could something like this be considered inside the languagefilter.php, after line 552 in order to take into account the redirection parameter when the system have to automatically change the language after the user login?

if (is_null($this->app->getUserState('users.login.form.return'))) {
    // Try to get association from the current active menu item
    $active = $menu->getActive();
} else {
    $active = $menu->getItems( 'link', $this->app->getUserState('users.login.form.return'))[0];
}
$foundAssociation = false;

The problem is that without this implementation the redirect parameter in the login menu is ignored and if you use the "Login menu for guest and Logout menu for registered" setup method, without a correct redirect you will see the error "You are not authorised to view this resource".

@mannybiker

Not sure I understand,
anyhow this code $this->app->getUserState('users.login.form.return'))[0] throw a parse error

A question why URLs inside database configuration should be checked by isInternal()?

• if the database was hacked, then the site is hacked already
• maybe we can set a flag inside the session if the URL is coming from DB ???

otherwise the code of components and modules / etc could be updated to get the redirect URL from DB (if such configuration exists) and use it when the isInternal() check for return URL (that comes from session) is not passing the check

@infograf768 Sorry for my unclear explanation.
It is a well discussed problem, when you have to create a menu with login and logout link, where you want the login link to be only visible for guests and the logout only to registered users. To avoid raising "You are not authorised to view this resource" you need to redirect the users to a valid page after the login otherwise the CMS try to redirect him to the same login page that is no more available since the user is now identified as registered. Here the vital need to have the redirect parameter of com_users working also when languagefilter plugin is enabled on a MultiLanguage site, but as you said "we just do not use the redirections set in the login module" in case of MultiLanguage.

The return url is set in the users.login.form.return register key before executing the onUserLogin in the languagefilter, but the setUserState used in that method overwrite the redirect parameter set in the com_users. For this reasons I used the getUserState('users.login.form.return') to read if there is already a value and if it is, I use that one. I did not test all the cases, for sure it is just a hack I needed to apply, but the code I posted should work since I am using it. But pay attention, it is not
$this->app->getUserState('users.login.form.return'))[0] but
$menu->getItems( 'link', $this->app->getUserState('users.login.form.return'))[0];
the [0] is for the array getItems returns, moreover this way you would have a ")" in ")[0]" that is not opened anywhere :)

$menu->getItems( 'link', $this->app->getUserState('users.login.form.return'))[0];

returns a parse error on PHP 5.3.x. I just corrected a similar code ;)
see #9562
Not a big deal. Will test again your proposal.

@mannybiker
hmm, can't test here, I also have errors on php 5.4.4 when I try to dump. Please create a PR and explain how to test in details including the pages set for guest and registered and the settings in mod_login. Use screenshots to clarify.

Closed as this has been continued elsewhere

@PhilETaylor
Unwanted consequence of this: it breaks login to read more as the return uses JRoute(). We are redirected to the user profile instead of the article.:
#9846

if you close this, why i still have this issue on joomla 3.5.1?

no matter if i use sef or non-sef url redirect after login dont work and it always open user profile page..

The bug has been solved (for core templates) in
#9959

and will be available in 3.6.0 release.

Look at the code there and modify your template to fit.

Should we not route the links anymore in our extensions or what?

As far as I know, it concerns only "return" when JUri::isInternal() is used.

Hopefully this will help...and for the record, when I was using the earlier versions, I recall that logging out did work and would end up on the home page as it should. But over the last while, I noticed this is no longer happening. Here are my results and testing (local testing environment). So it appears it's not quite fixed in 3.6.

LIVE SITE Logout with SEF on and I am using Joomla 3.6
https://www.shapedpixels.com/index.php?option=com_users&task=user.logout&735f809f209e3857bbbaf3d005e96f8a=1&return=L3N1cHBvcnQK

Here is what happens to me...my test local site on XAMPP using the core login menu item link as well as the login module. I am using the default Joomla template, not my own.

Menu Item - Login Page with SEF off
I click on the menu item, takes me to the login page, I then login. I get redirected to my page that I set (this works).
I click on the menu item "Login" again. Takes me to the logout page. I click the logout button. I am redirected to the home page (this works).

Menu Item - Login Page with SEF on
I click on the menu item, takes me to the login page, I then login. I get redirected to my page that I set (this works).
I click on the menu item "Login" again. Takes me to the logout page. I click the logout button. I am redirected to the home page (this works).

Login Module - Login Page with SEF off
I click on login on the module. I get redirected to the Members Home page (this works)
I click on the Logout button on the module. I stay on the page I logged on (although now it's blank without content.) (breadcrumbs say "Home":

Login Module - Login Page with SEF on
I click on login on the module. I get redirected to the Members Home page (this works)
I click on the Logout button on the module. I stay on the page I logged on (although now it's blank without content.) (breadcrumbs say "Home":
I also noticed that menu items set for "Registered" are still shown in my menu. But if I click on one, I have to log in.

Using the Extension Quick Logout with a Menu Item - Login to the Login page and I have SEF off
I click on the menu item, takes me to the login page, I then login. I get redirected to my page that I set (this works).
I click on the menu item "Logoout". It logs me out to some crazy URL that gives a page without content (breadcrumbs say "Home":
http://localhost/joomla/index.php?option=com_users&task=user.logout&aa95e57f38e43844435ec7b260f4cce7=1&return=L2pvb21sYS9pbmRleC5waHA/b3B0aW9uPWNvbV9jb250ZW50JnZpZXc9ZmVhdHVyZWQmSXRlbWlkPTEwMQo=

Using the extension Quick Logoout with a menu Item - Logout and I have SEF on
I click on the menu item, takes me to the login page, I then login. I get redirected to my page that I set (this works).
I click on the menu item "Logoout". It logs me out to some crazy URL that gives a page without content (breadcrumbs say "Home":
http://localhost/joomla/index.php?option=com_users&task=user.logout&3c8e9a2830407b11319a7f0b8cbf2937=1&return=L2pvb21sYS8K

Possibly related, but if I set the login menu item as Internal URL instead of Menu Item, I set the urls using dynamic structure:

Login:
index.php?option=com_content&view=article&Itemid=9

Logout:
/
But I also tried this:
index.php?option=com_content&view=featured

After saving, I get this Warning:
Only one of the login redirect fields should have a value.
Only one of the logout redirect fields should have a value.

Then everything gets set back to default settings prior to my changing to use Internal.

@Mocha365 You need to clear the textfield of the custom URL or to select Default in the listbox to use the relative opposite option and save without errors.

For the Logout plugin you are right, the new modifications break the plugin, but since this is not part of the core of Joomla I do not think this should be the right place to talking about. Instead the author of the plugin could be asked about it, if he is still intentioned in updating his script.

In any case something like this should fix the problem if you are using the new redirect feature via the menu item selection, in the login module:

change this in the quicklogout.php file:
This line
$url = JUri::getInstance(JURI::base())->toString(array('scheme','host')).JRoute::_($item->link . '&Itemid=' . $item->id,false);
with this
$url = $params->get("quick_logout_redirect");

and

$loloc .= base64_encode( htmlspecialchars_decode( $url )."\n" );
with this
$loloc .= base64_encode( htmlspecialchars_decode( $url ));

I experienced this issue on an Intranet site we have setup with Single Sign On. My frontend editors have a link to 'logout' so they can see pages the same as other users (without unpublished articles). After an update from 3.4.1 to 3.6.2 they were no longer able to logout. Clicking on the logout link, then logout button went as usual and did not produce any errors on the site or in logs, but the user remained logged in.

After a few days of banging my head against the wall I found this issue tracker and decided to experiment with my logout URL in the menu item 'options' tab. The original URL i was using was "/index.php?nosso=2". In the end I simply removed the leading slash so it was "index.php?nosso=2" and my problem is resolved.

I'm glad that work was done to improve security, but it took days of searching to find that this was the root cause. Communication is key.

Thanks,
Nick