Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing PATH defines if index.php placed into the root folder of the filesystem #23149

Closed
wants to merge 8 commits into from

Conversation

nextend
Copy link

@nextend nextend commented Nov 23, 2018

Pull Request for Issue #23148

Summary of Changes

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

`__DIR__` constant can hold `/` if the PHP file in the root directory. 
`__DIR__`  The directory of the file. If used inside an include, the directory of the included file is returned. This is equivalent to dirname(__FILE__). This directory name does not have a trailing slash unless it is the root directory.
`__DIR__` constant can hold `/` if the PHP file in the root directory. 
`__DIR__`  The directory of the file. If used inside an include, the directory of the included file is returned. This is equivalent to dirname(__FILE__). This directory name does not have a trailing slash unless it is the root directory.
administrator/index.php Outdated Show resolved Hide resolved
Missing whitespaces fixed

Co-Authored-By: nextend <roland@nextendweb.com>
@PhilETaylor

This comment was marked as abuse.

@nextend
Copy link
Author

nextend commented Nov 23, 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.

@richard67
Copy link
Member

@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?

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@brianteeman
Copy link
Contributor

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

@nextend
Copy link
Author

nextend commented Sep 30, 2020

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

@richard67
Copy link
Member

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.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@richard67
Copy link
Member

Gulp.

@PhilETaylor

This comment was marked as abuse.

@zero-24
Copy link
Contributor

zero-24 commented Sep 30, 2020

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

@PhilETaylor

This comment was marked as abuse.

@richard67
Copy link
Member

Seems I did not look into it deep enough.

@PhilETaylor

This comment was marked as abuse.

@zero-24 zero-24 removed the PR-staging label Aug 9, 2021
@laoneo
Copy link
Member

laoneo commented Apr 1, 2022

@PhilETaylor did you test this pr?

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@laoneo
Copy link
Member

laoneo commented Apr 1, 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.

@PhilETaylor

This comment was marked as abuse.

@HLeithner
Copy link
Member

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.

@HLeithner HLeithner closed this Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants