Tread carefully... less you introduce a security issue...

@PhilETaylor Could you please explain more details about the security issue causes by this PR? It is almost the same with the code you wrote for Joomla 3.4.6 except that I try to validate the relative url (which is always considered as none internal urls with your change)

To validate the relative url, I use most of the code in Joomla router. If we can find a menu item which match the given url, then the url will be an treated as internal url and I think it is correct.

Right now, even on the site has a menu item with alias blog for example, JUri::isInternal('/blog') still return false and it is not correct. I am just trying to fix that bug.

I never said it causes security issues, I said to be careful that you dont introduce one... 3 full days went into the last changes to secure this method against mis-use.

IMHO /blog is not an internal url. IMHO Internal urls should be non-sef and start with index.php, this is the way the internal Joomla code consumes this function every time. However there are those that disagree with me and want to open this method up to all kinds of urls being thrown at it.

This cannot be merged at the moment as you have broken all the unit tests as well. Those need fixing as well as more examples added to prove your changes provide good results

I dont have time to see if your changes create security issues today, however the unit tests cover some of the major security issues and so once you fix that you will see

Thanks for your reply.

1. For unit tests, I don't have experience with it (I mentioned before), so I could not write new or correct the unit tests.

2. For internal URL, the /blog should be considered as valid internal URL as there is a menu item on the site has blog alias.

If we force to internal URL as be started with index.php as you said, then the code below (which is valid) won't work properly anymore:

$app = JFactory::getApplication();
$user = JFactory::getUser();
$Itemid = $app->input->getInt('Itemid', 0);
if (!$user->id)
{
    $return = JRoute::_('index.php?Itemid=' . $Itemid);
    $app->redirect('index.php?option=com_users&view=login&return=' . base64_encode($return), 'Please Login to continue accessing to this page');
}

It would be nice if we can work together to solve the issue. Otherwise, third party extensions and templates will have to change to work with the new code in 3.4.6.

In summary, this changes will only affect the SEF relative urls compare with what we are having now

1. Before this change, even we have a menu item with alias menu-item-alias, the url /menu-item-alias is still considered as not valid internal URL

2. After this change, the URL /menu-item-alias, or /menu-item-alias/sub-alias will be considered as valid internal url

Please replace JURITest.php with the code from https://gist.github.com/be84f0615aa64e2cbed6 and then commit to your branch - this should resolve the unit tests

Thanks @PhilETaylor. It solved the unit tests error, simpler than I thought

Now, I have some questions and would like to receive feedback

1. With the new code, if we have a menu item menu-item-alias/sub-menu-item-alias, JUri::isInternal() will return true for both menu-item-alias/sub-menu-item-alias and /menu-item-alias/sub-menu-item-alias. Will we have any problem with it? Or we should only accept /menu-item-alias/sub-menu-item-alias as a valid internal url?

2. The JUri::isInternal() also return true for url like this /menu-item-alias/sub-menu-item-alias/it-does-not-exists although it does not actually exists and will cause 404 error. As long as there is menu item with alias match a segment in the given URL JUri::isInternal() will return true. Do you see any problem with it?

3. The new code also treated the url such as component/com_content as a valid internal URL, too

If this change doesn't cause any security issue, I hope other developers can help review the code to make sure we have a correct implement of this method.

I'm sorry, but I fear that there is no sane way to achieve what you are trying to do in our system.