User tests: Successful: Unsuccessful:
Pull Request for Issue #23148
If __DIR__
holds /
, simply use empty string in /index.php
and /administrator/index.php
Status | New | ⇒ | Pending |
Category | ⇒ | Administration |
Labels |
Added:
?
|
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.
@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?
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
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 :-)
that host has always been a problem. I remember discussing things with them at the first polish joomladay
@PhilETaylor You are right, I just traced back this in our support system and this was related to home.pl
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.
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 ?
Another gotcha if JPATH_BASE === ""
define('JPATH_ROOT', realpath(JPATH_BASE));
joomla-cms/tests/unit/bootstrap.php
Line 69 in d3b544e
if JPATH_BASE === "" then JPATH_ROOT would be defined as "/"
- you can replicate this on your Mac,
Gulp.
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
I'm not sure where this is comming from when it is an hosting config error i would say fix the hosting right?
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
?
Seems I did not look into it deep enough.
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 :)
Labels |
Added:
?
Removed: ? |
@PhilETaylor did you test this pr?
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?
There needs to be a @joomla/security review though
another PR totally ignored by the JSST for years #fail
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.
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
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-04-21 16:39:44 |
Closed_By | ⇒ | HLeithner |
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 (
)
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.
I cannot test this but what about Windows webhosts? would that not then become:
c:\/defines.php