? Success

User tests: Successful: Unsuccessful:

avatar nextend
nextend
23 Nov 2018

Pull Request for Issue #23148

Summary of Changes

If __DIR__ holds /, simply use empty string in /index.php and /administrator/index.php

avatar nextend nextend - open - 23 Nov 2018
avatar nextend nextend - change - 23 Nov 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 23 Nov 2018
Category Administration
avatar nextend nextend - change - 23 Nov 2018
Labels Added: ?
avatar PhilETaylor
PhilETaylor - comment - 23 Nov 2018

I cannot test this but what about Windows webhosts? would that not then become:

c:\/defines.php

avatar nextend
nextend - comment - 23 Nov 2018

On Windows I do not think there is a way to use the DIRECTORY_SEPARATOR cd \ to access anything, so __DIR__ will never hold that value. But if that happen somehow, this fix will work there too.

avatar richard67
richard67 - comment - 30 Sep 2020

@SniperSister @zero-24 It seems this fixes an issue with Joomla running on a webserver which is isolated in a chroot enviroment. Is this something for the SST? Is is not easy to set up such an environment, so it might not be easy to find testers for this PR. Do you think the SST could help with that?

avatar PhilETaylor
PhilETaylor - comment - 30 Sep 2020

I have already wasted almost a whole week on this kind of thing (installing Joomla at /) in testing for mySites.guru - the solution for testing was a docker container with a VERY BADLY insecure configured web server to serve from the / filesystem, and then installed Joomla in the root of the linux container (ignoring all the other folders that were also in the /) ... gulp... it was not pretty. All this without chroot.

I might still have the docker image lying around... I'll need to check from the office tomorrow

avatar PhilETaylor
PhilETaylor - comment - 30 Sep 2020

I may also add that the ONLY webhost in the world where I have ever seen or heard of a Joomla Site with a / installation path is home.pl - and literally no where else. Looking at all the historic notes I have, and GH Issues in this repo, its always home.pl the webhost.

It might be easier to just say no :-)

avatar brianteeman
brianteeman - comment - 30 Sep 2020

that host has always been a problem. I remember discussing things with them at the first polish joomladay

avatar nextend
nextend - comment - 30 Sep 2020

@PhilETaylor You are right, I just traced back this in our support system and this was related to home.pl

avatar richard67
richard67 - comment - 30 Sep 2020

From code review the change in this PR here looks ok to me, and I also don't see a risk in it for "normal" hosts.

But it might be wrong, and that might be the risk then ;-)

And without a real test I can't say if it will fix everything for such a host.

So I have no idea what to do with this PR.

avatar PhilETaylor
PhilETaylor - comment - 30 Sep 2020

The only gotcha would be code that uses an if assuming that JPATH_BASE always had a value, like:

if(JPATH_BASE){ // would return false

but on (quickly) searching GitHub all of Joomla's use seems to be checking if its defined, not its valie

defined('JPATH_BASE') or die;
// or 
if (defined('JPATH_BASE')){ //

so as long as 3rd party developers are not relying on JPATH_BASE containing a value, then it "looks" ok to me...

Because what this PR would do on a site on home.pl is set JPATH_BASE === "" - so it would be defined, but would be a blank string, making things like the following still work:

require JPATH_BASE . "/this/file.php";

There needs to be a @joomla/security review though (Im not part of that team) because its a base path, and could/might/probably used in building file paths to assets and ensuring that people are not going outside of the JPATH_BASE, which, if its set to "" then depending on its use, could cause security issues. Ive not reviewed the code to find any instances like this, Im just thinking out loud. Knowing the recent security issues I have sent to the JSST I would say they have their hands full fighting those fires, so might not have time to review this @SniperSister ?

avatar PhilETaylor
PhilETaylor - comment - 30 Sep 2020

Another gotcha if JPATH_BASE === ""

define('JPATH_ROOT', realpath(JPATH_BASE));

define('JPATH_ROOT', realpath(JPATH_BASE));

if JPATH_BASE === "" then JPATH_ROOT would be defined as "/" - you can replicate this on your Mac,

Screenshot 2020-09-30 at 19 31 20

avatar richard67
richard67 - comment - 30 Sep 2020

Gulp.

avatar PhilETaylor
PhilETaylor - comment - 30 Sep 2020

Also... there are FIFTEEN places where JPATH_BASE can be defined... therefore this PR is not acceptable, as it only replaces a few of the 15....

see: https://github.com/joomla/joomla-cms/search?p=1&q=%22define%28%27JPATH_BASE%22

avatar zero-24
zero-24 - comment - 30 Sep 2020

I'm not sure where this is comming from when it is an hosting config error i would say fix the hosting right?

avatar PhilETaylor
PhilETaylor - comment - 30 Sep 2020

Also this PR in /administrator/index.php has:

$dir = __DIR__ === DIRECTORY_SEPARATOR ? '' : __DIR__;    // __DIR__ would be /administrator on home.pl
// $dir now === "/administrator"
define('JPATH_BASE', $dir);
// JPATH_BASE now === "/administrator"
require_once JPATH_BASE . '/includes/defines.php';		 

Therefore $dir = /administrator

So then it correctly includes /administrator/includes/defines.php

Which then ignores var $dir and recalculates

$parts = explode(DIRECTORY_SEPARATOR, JPATH_BASE);
array_pop($parts);

// Defines
define('JPATH_ROOT',          implode(DIRECTORY_SEPARATOR, $parts));

This leaves JPATH_ROOT === "" !!!!! When it should be /administrator/ (proof https://3v4l.org/AYRj3)

then defines.php goes on to do these when JPATH_ROOT now === "" leaving invalid paths like

//administrator
//libraries
//plugins
//etc...
define('JPATH_ADMINISTRATOR', JPATH_ROOT . DIRECTORY_SEPARATOR . 'administrator');
define('JPATH_LIBRARIES',     JPATH_ROOT . DIRECTORY_SEPARATOR . 'libraries');
define('JPATH_PLUGINS',       JPATH_ROOT . DIRECTORY_SEPARATOR . 'plugins');
define('JPATH_INSTALLATION',  JPATH_ROOT . DIRECTORY_SEPARATOR . 'installation');
define('JPATH_THEMES',        JPATH_BASE . DIRECTORY_SEPARATOR . 'templates');
define('JPATH_CACHE',         JPATH_BASE . DIRECTORY_SEPARATOR . 'cache');

So on code review alone I would say this PR is heading in the right direction - but is woefully inadequate in making a full solution :-)

But like I said, is this really a won't fix?

avatar richard67
richard67 - comment - 30 Sep 2020

Seems I did not look into it deep enough.

avatar PhilETaylor
PhilETaylor - comment - 30 Sep 2020

Also... there are FIFTEEN places where JPATH_BASE can be defined... therefore this PR is not acceptable, as it only replaces a few of the 15....

To be fair, some of those 15 are in testing etc... so would never run on home.pl, but still :)

avatar laoneo laoneo - change - 1 Apr 2022
Labels Added: ?
Removed: ?
avatar laoneo
laoneo - comment - 1 Apr 2022

@PhilETaylor did you test this pr?

avatar PhilETaylor
PhilETaylor - comment - 1 Apr 2022

Did you bother reading my exhaustive comments and statements where I clearly indicated that I had tested and provided extensive feedback on this PR ? Or did I just waste my time - again?

avatar PhilETaylor
PhilETaylor - comment - 1 Apr 2022

There needs to be a @joomla/security review though

another PR totally ignored by the JSST for years #fail

avatar laoneo
laoneo - comment - 1 Apr 2022

I was reading it and came across "So on code review alone I would say this PR is heading in the right direction", what made me unsure about the testing state from you. I would like to move forward with this one, but I can't test it by myself so I need others to look into it.

avatar PhilETaylor
PhilETaylor - comment - 1 Apr 2022

I suggest you read the entirety of my comments again and not just quote one.

this effects a single webhost in the world, or when Joomla is on a virtual file system where it appears it’s in the root folder of the server.

It’s a 80% a won’t fix - but that’s Not my decision.

I have provided my commentary already based on years of experience with this specific issue and on hours of testing the proposed changes in this PR.

It’s now down to the maintainers to make a decision

avatar HLeithner HLeithner - change - 21 Apr 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-04-21 16:39:44
Closed_By HLeithner
avatar HLeithner
HLeithner - comment - 21 Apr 2022

First sorry @nextend that this took so long, it's not trivial to test this.

I tested this on my infrastructure and it's a pain to get this running (I use a really restrictive setup) (you need to have /dev/random in the website else joomla can't have random values or even a session id). Surprisingly Joomla 3 runs pretty good on "/" as document root. Also the PR seems to solves an issue. Now the but(s). this PR (or better the fact that's joomla in "/") breaks our path obscurity filter for error messages (
image
)

Even if this could be fixed (add a check into the message filter if JPATH_ROOT is empty). We are still on Joomla 3.

If you try out Joomla 4 on "/" as base directory it seems similar to J3, the patch needs to be adapted, we also have some more locations where JPATH_BASE is set.

I think it's not worth to fix this mainly because having joomla root folder is extremely uncommon but I would except a proper pull request with all fields analysed where JPATH_BASE/ROOT is used (CMS and framework). For example we have about 37 occurrence where JPATH_ROOT is used in a str_replace which could lead to an unexpected behavior when JPATH_ROOT is empty (see image). I would expect same is true if JPATH_ROOT is "/".

I'm closing this PR as unsupported environment for Joomla. If you @nextend or anyone else want's to reopen it please fill free and check the complete cms with proper test introductions.

If someone would like to have this fix in a local installation a define.php file can be manually added with the following content:

define('JPATH_BASE', dirname(__DIR__) === DIRECTORY_SEPARATOR ? '' : dirname(__DIR__));
define('_JDEFINES', true)
require_once JPATH_BASE . '/includes/defines.php';

this should work at least the same as this PR.

avatar HLeithner HLeithner - close - 21 Apr 2022

Add a Comment

Login with GitHub to post a comment