No Code Attached Yet J3 Issue
avatar durian808
durian808
10 Mar 2022

Steps to reproduce the issue

Any code that calls getOptions() in libraries/joomla/form/fields/filelist.php, where $this->directory is NOT a relative path, in my example "/images/virtuemart/payment".

Expected result

No is_dir() warning

Actual result

Warning: is_dir(): open_basedir restriction in effect. File(/images/virtuemart/payment) is not within the allowed path(s): (/var/www/vhosts/XXXXX.com/:/tmp/) in /var/www/vhosts/XXXX.com/httpdocs/libraries/joomla/form/fields/filelist.php on line 193

System information (as much as possible)

Joomla 3.10.6
PHP 7.4.27

Additional comments

Related to #35895 #36788

Workaround:

I replaced

       $path = $this->directory;

        if (!is_dir($path))
        {
            $path = JPATH_ROOT . '/' . $path;
        }

with

$path = JPATH_ROOT . '/' . $this->directory;

avatar durian808 durian808 - open - 10 Mar 2022
avatar joomla-cms-bot joomla-cms-bot - change - 10 Mar 2022
Labels Added: No Code Attached Yet
avatar joomla-cms-bot joomla-cms-bot - labeled - 10 Mar 2022
avatar chmst chmst - change - 10 Mar 2022
Labels Added: J3 Issue
avatar chmst chmst - labeled - 10 Mar 2022
avatar durian808 durian808 - change - 11 Mar 2022
The description was changed
avatar durian808 durian808 - edited - 11 Mar 2022
avatar dgrammatiko
dgrammatiko - comment - 11 Mar 2022

in my example "/images/virtuemart/payment".

Your path IS NOT relative, it is absolute. If you want to define a relative directory then it should be images/virtuemart/payment (missing the first /)

Also you SHOULD NEVER, EVER put executable files (eg .php) inside the images or the media folders from a security point of view...

avatar durian808 durian808 - change - 11 Mar 2022
The description was changed
avatar durian808 durian808 - edited - 11 Mar 2022
avatar durian808
durian808 - comment - 11 Mar 2022

@dgrammatiko Thanks for the correction – I meant to say, "is NOT a relative path", now fixed. The leading "/" is what causes is_dir to throw the warning. I think the function is meant to handle both cases, leading "/" and no leading "/".

RE:

Also you SHOULD NEVER, EVER put executable files (eg .php) inside the images or the media folders from a security point of view...

I don't know why you are mentioning this, because it has no bearing on this bug report, correct?

avatar dgrammatiko
dgrammatiko - comment - 11 Mar 2022

it has no bearing on this bug report, correct?

Because it's a potential security issue. The folders images and media are supposed to only contain static assets, nothing .php...

avatar durian808
durian808 - comment - 11 Mar 2022

Dunno why the heck you are saying that. Are you talking about this .php file?

libraries/joomla/form/fields/filelist.php

This is the code that has the bug on line 193.

Again, what you are saying has no bearing on this bug report.

avatar dgrammatiko
dgrammatiko - comment - 11 Mar 2022

There is no bug. Your code is wrong, remove the first slash

avatar durian808
durian808 - comment - 11 Mar 2022

No, you are wrong. The function is designed to handle leading "/" or no leading "/". My workaround is the correct solution**. You need to look more closely at this, or get out of the way so another developer can do that.

And why don't you admit that you are also wrong for mentioning this, because it has nothing to do with the bug report?

Because it's a potential security issue. The folders images and media are supposed to only contain static assets, nothing .php

Your attitude is not helpful here.

** UPDATE: Not necessarily completely correct because $this->directory may possibly be a full path from the server's root.

avatar dgrammatiko
dgrammatiko - comment - 11 Mar 2022

a relative path, in my example "/images/virtuemart/payment".

Again you shouldn't roll PHP files in the images or media folder. Which part of what I wrote you didn't understand?

avatar durian808
durian808 - comment - 11 Mar 2022

Dude, I'm gonna have to report you!

Do these look like .php files to you?

bank-transfer-logo.png
Paypal-Logo.png

avatar brianteeman
brianteeman - comment - 11 Mar 2022

am i being thick - why not just change your code to use the correct relative path?

avatar durian808
durian808 - comment - 11 Mar 2022

** CORRECTED **

Clearly, the function should handle, and I think is intended to handle, the case of leading "/" and not leading "/".

The function is actually using is_dir() here to determine if JPATH_ROOT needs to be added to the front of the path.

If is_dir() returns FALSE, this is the case where $this->directory has a leading "/" even though it's a directory in the Joomla tree, as in my example, "/images/virtuemart/payment"; and it would happen as well with a fully-qualified path, from the server's root, where that path does not resolve to a valid directory.

$path = $this->directory;

if (!is_dir($path))
{
$path = JPATH_ROOT . '/' . $path;
}

$path = JPath::clean($path);

JPath::clean() converts any "//" to "/".

It's a separate issue to check if the resultant $path is to a valid directory, and that's when is_dir() should be used. If is_dir() then fails, I think the function should return a failure because it was passed an invalid directory path.

This is not my code by the way, it's VirtueMart. Wasn't broken before, I believe until a newer PHP version.

avatar durian808
durian808 - comment - 11 Mar 2022

I am noting here that @dgrammatiko has not yet offered an apology.

avatar dgrammatiko
dgrammatiko - comment - 11 Mar 2022

Apology for what? I already said in my first comment that this is not a Joomla's bug. Also I added a comment about not using php files in directories for assets because you never actually provided enough code to see what this code was serving. BTW for images Joomla has specific API's that also offer overrides for the images, so that code is not only wrong because it's using absolute path instead of relative it also goes against the platform as it neglects the overriding aspect of static assets.

avatar durian808
durian808 - comment - 11 Mar 2022

Ah geez, please @brianteeman.

avatar dgrammatiko
dgrammatiko - comment - 11 Mar 2022

Here's how a correct use of this field looks like:

<field
name="image"
type="filelist"
label="COM_LANGUAGES_FIELD_IMAGE_LABEL"
stripext="1"
directory="media/mod_languages/images/"
hide_none="1"
hide_default="1"
fileFilter="\.gif$"
validate="options"
>
<option value="">JNONE</option>
</field>

Which renders correctly:
Screenshot 2022-03-11 at 23 50 48

Screenshot 2022-03-11 at 23 50 55

So, your assumption that this is a Joomla's bug was wrong (what I pointed out in my first comment). You should create an issue to the component that has the bug.

This should be closed as it's not a bug, Joomla has this code with this specific functionality for over 10 years and I don't see any benefits changing the method to accept absolute paths as relative.

My 2c, and I'm unsubscribing

avatar durian808
durian808 - comment - 11 Mar 2022

My position is that the programming of the function should be intelligent to anticipate either leading "/" or not leading "/", and I believe that was the intention of the authors of that function – I mean, it's quite clear...

$path = JPATH_ROOT . '/' . $path;

With a new PHP version, is_dir() started throwing these warnings, as has been addressed in #35895 #36788. This means many extensions that previously didn't see this warning are now seeing this warning.

Joomla functions should not rely on is_dir() throwing a warning to notify users that they've entered a path beginning with "/" when they should have used a full path, or a path relative to the Joomla root. Joomla functions should return errors to the caller when an invalid directory has been passed.

The purpose of is_dir() is to check if a directory path is valid... i.e. that the path isn't to a file, and that the directory exists and is accessible in the Joomla tree.

There are a great many instances of is_dir() being used in the Joomla core.

avatar richard67
richard67 - comment - 13 Mar 2022

@dgrammatiko I think you misunderstood the error message:

Warning: is_dir(): open_basedir restriction in effect. File(/images/virtuemart/payment) is not within the allowed path(s): (/var/www/vhosts/XXXXX.com/:/tmp/) in /var/www/vhosts/XXXX.com/httpdocs/libraries/joomla/form/fields/filelist.php on line 193

The File(/images/virtuemart/payment) is the path in question, and the filelist.php is the source which want to use that path, i.e. it is the file where the error happens, not the one for which the error happens. The author did not try to use a PHP file in his images folder.

@durian808 We all can make mistakes sometimes, and if someone makes a mistake, we do not insist on something like an apology. You should not do that either.

avatar durian808
durian808 - comment - 13 Mar 2022

@richard67 Thanks for providing backup, because I couldn't get through to Mr. Grammatiko.

We all can make mistakes sometimes, and if someone makes a mistake, we do not insist on something like an apology. You should not do that either.

Oh I fully understand that; however, Mr. Grammatiko was insistent and oblivious. Sometimes people who behave like that need a wake-up call. So, I'll throw this right back at you... YOU shouldn't be telling ME, "You should not do that either." Because you're not my daddy.

And, I was not "insisting" on an apology, I was simply stating that Mr. Grammatiko didn't offer one, where one would have been typically expected, and so I was noting that for the record. Something like, "Oh I see, sorry about that." Nothing of the sort from Mr. Grammatiko. No simple, courteous admission of a mistake.

The golden rule applies here, and had it been me with someone trying to tell me to "look again," I would sincerely look again, and then admit that I missed the point. Mr. Grammatiko came in locked and loaded to shoot down anything from the pesky outsider making the bug report. This doesn't bode well for Joomla. I've actually run into this behavior before when posting to this system. Some of the developers are high-and-mighty, seem overworked or burnt out, and... that is reflected in their responses here.

I have a background of over 20 years as a senior level software engineer in data communications and networking system software development and embedded software, and over 15 years of experience as a web developer, PHP programmer, and web hosting provider.

I encourage someone to please look at what I've posted here and to give it some good consideration. Thank you!

avatar richard67
richard67 - comment - 13 Mar 2022

@durian808 When checking the documentation to the filelist field here https://docs.joomla.org/Filelist_form_field_type , the example shows directory="administrator" and not directory="/administrator". So it should be clear that a path below the Joomla root should not start with a slash. A path starting with a slash is an absolute path from the webserver's point of view, and so maybe relative to the webserver root but not necessarily be relative to the Joomla root.

In my opinion you should be thankful that you got advice how to fix your code to use the right kind of path instead of making up arguments just for the sake of having been right at the end.

avatar dgrammatiko
dgrammatiko - comment - 13 Mar 2022

I think you misunderstood the error message:

@richard67 I think I didn't but anyways the comment about not having php files in images or media was a side note, as there wasn't any clues what the filelist had been used for. Also quite weird that this guy triggered by that...

Anyways, about the proposed solution: it's totally wrong and here's an actual example of my own code that using the filelist field to access a folder not in the Joomla root:

  • /var/www/html/site is the Joomla root
  • /var/www/html/protected is a folder that holds zips for a distributed software
  • directory="/var/www/html/protected/" is the XML declaration for the directory

This works with the current code. The idea that Joomla should not allowed to access anything other than what's inside the Joomla root is an artificial limit that only applies to installations on a cheap shared server but Joomla could be used in it's own VPN server, so in that case the limit will be rather annoying.

avatar durian808
durian808 - comment - 13 Mar 2022

Mr. Grammatiko still not backing down. Unbelievable. @brianteeman are you seeing this? Does anyone care?

It's not my code, it's Virtuemart, and there are many Joomla extensions that are affected by the change in the behavior of is_dir() and is_file() with the newer version of PHP. You people don't seem to care about how this is all functioning in the real world. Did you even look at the fact that the code is adding JPATH_ROOT to the beginning of the path? Why do you think the original authors did that?

avatar dgrammatiko
dgrammatiko - comment - 13 Mar 2022

it's Virtuemart

Then OPEN A ISSUE THERE...

avatar durian808
durian808 - comment - 13 Mar 2022

A "side note". Unbelievable. It was completely irrelevant to the bug report and shouldn't have even been mentioned.

avatar dgrammatiko
dgrammatiko - comment - 13 Mar 2022

THERE IS NO BUG

avatar durian808
durian808 - comment - 13 Mar 2022

No, no, no. Look... If you guys can't understand this, then get a developer in here who does.

avatar durian808
durian808 - comment - 13 Mar 2022

Grammatiko said he was unsubscribing... do it, brother.

avatar dgrammatiko
dgrammatiko - comment - 13 Mar 2022

First of all I'm not your brother and if you tagging me you inviting me back. Have you ever used GitHub?

avatar durian808
durian808 - comment - 13 Mar 2022

Wrong again dude. You never unsubscribed. What a joke.

avatar chmst chmst - change - 13 Mar 2022
Status New Closed
Closed_Date 0000-00-00 00:00:00 2022-03-13 12:03:54
Closed_By chmst
avatar chmst chmst - close - 13 Mar 2022
avatar chmst
chmst - comment - 13 Mar 2022

Please be polite, @durian808.
Your issue is not a bug, so I close this.

avatar durian808
durian808 - comment - 13 Mar 2022

Oh, then why are these bugs? #35895 #36788

avatar durian808
durian808 - comment - 13 Mar 2022

Does Grammatiko get a "be polite" too? Or is he above that because he's got some kind of seniority here?

avatar HLeithner HLeithner - change - 13 Mar 2022
Status Closed New
Closed_Date 2022-03-13 12:03:54
Closed_By chmst
avatar HLeithner HLeithner - reopen - 13 Mar 2022
avatar HLeithner
HLeithner - comment - 13 Mar 2022

I reopen this ticket, beside the rudeness of the ticket opener the concerns are failed the proposed solution seems to be wrong.

assuming that the path is relative because it's is_dir returns false seems wrong to me. Always prefixing JPATH_ROOT seems also wrong because it's possible that the developer like to haven an absolute path or a directory outside the joomla root directory.

My suggestion would be to check if $path starts with a / and depending on this add the JPATH_ROOT if it's not starting with a /. In this case I would expect the developer want's an absolute path and Joomla shouldn't add magic here.

in the end the Joomla! behavior is some kind of magic which hasn't been noticed since 12 years but should be changed to do the right thing anyway.

Which would not solve the problem of the issue starter but clarify the way it should work.

avatar brianteeman
brianteeman - comment - 13 Mar 2022

in the end the Joomla! behavior is some kind of magic which hasn't been noticed since 12 years but should be changed to do the right thing anyway.

Which would not solve the problem of the issue starter but clarify the way it should work.

While that may be valid (I havent checked but it probably is) it doesnt change the fact that the only way to fix the original problem is to change the code in the plugin

avatar dgrammatiko
dgrammatiko - comment - 13 Mar 2022

What @HLeithner proposed is changing

$path = $this->directory;
if (!is_dir($path))
{
$path = JPATH_ROOT . '/' . $path;
}
$path = Path::clean($path);

to

$path = Path::clean(strpos($this->directory, '/') === 0 ? $this->directory : JPATH_ROOT . '/' . $this->directory);

if (!$path) {
  throw new \Exception('nope');
}

But as @brianteeman and @HLeithner that is not solving the problem reported here, which is wrong use of the API

avatar HLeithner
HLeithner - comment - 13 Mar 2022

of course because I see no way that we can fix the problem in the plugin without changing the behavior for all other usages

avatar brianteeman
brianteeman - comment - 13 Mar 2022

in the end the Joomla! behavior is some kind of magic

Nope its the way that is_dir is intended to work

avatar HLeithner
HLeithner - comment - 13 Mar 2022

is_dir works as expected but why uses joomla such a strange way to detect if it is a relative path? that makes no sense to me.

because if I have a images directory in my root folder that function/magic to add the JPATH_ROOT would fail

avatar dgrammatiko
dgrammatiko - comment - 13 Mar 2022

@HLeithner there you go: #37261

btw I'm not sure if this would work on fat/ntfs machines, if it doesn't just close it

avatar HLeithner HLeithner - change - 13 Mar 2022
Status New Closed
Closed_Date 0000-00-00 00:00:00 2022-03-13 17:33:06
Closed_By HLeithner
avatar HLeithner HLeithner - close - 13 Mar 2022
avatar HLeithner
HLeithner - comment - 13 Mar 2022

Closing since we have a PR, thanks dimitris

Add a Comment

Login with GitHub to post a comment