Meaningfull error for copy method

[artur-stepien]

Currently if the the copy method fails, it gives no details about what paths where provided as the parameters. It makes it had to debug (especially installer errors). This adds a proper source/destination details like in the stream copy above.

Summary of Changes

Expands the error message.

Testing Instructions

Try to install an extension with files added in the extension manifest but not in the zip file.

Documentation Changes Required

No

[HLeithner]

iirc we have parts in the cms which directly outputs the error message, writing the information into the message would be a information disclosure security issue.

Also the Exception it self is wrong there should be an own exception for every issue, which might could hold the additonal information.

[artur-stepien]

iirc we have parts in the cms which directly outputs the error message, writing the information into the message would be a information disclosure security issue.

Also the Exception it self is wrong there should be an own exception for every issue, which might could hold the additonal information.

You do understand its the same info that is already used in that method right?

[richard67]

You do understand its the same info that is already used in that method right?

@artur-stepien The full paths $src and $dest should at least be treated with Path::removeRoot($src), see `

filesystem/src/File.php

Line 118 in fe6edb9

Path::removeRoot($src)

, in order not to produce a full path disclosure if the exception message is shown to anybody in the frontend (which can happen). If that is missing a few lines (line 127) above your change, it is a mistake.

[artur-stepien]

You do understand its the same info that is already used in that method right?

@artur-stepien The full paths $src and $dest should at least be treated with Path::removeRoot($src), see `

filesystem/src/File.php

Line 118 in fe6edb9

Path::removeRoot($src)

, in order not to produce a full path disclosure if the exception message is shown to anybody in the frontend (which can happen). If that is missing a few lines (line 127) above your change, it is a mistake.

These are not full paths. When they end up in a flash message they have [ROOT] and [TMP] placeholders.

[richard67]

These are not full paths. When they end up in a flash message they have [ROOT] and [TMP] placeholders.

@artur-stepien As @HLeithner has already mentioned, this might not be the case in every scenario. That's why Path::removeRoot($src) in line 118.
You are right, in line 127 Path::removeRootis also not used. But that's a mistake.

I suggest you change both lines, i.e. change line 127 to

throw new FilesystemException(sprintf('%1$s(%2$s, %3$s): %4$s', __METHOD__, Path::removeRoot($src), Path::removeRoot($dest), $stream->getError()));

and line 136 to

throw new FilesystemException(sprintf('%1$s(%2$s, %3$s): %4$s', __METHOD__, Path::removeRoot($src), Path::removeRoot($dest), 'Copy failed'));

@HLeithner You wrote:

Also the Exception it self is wrong there should be an own exception for every issue

I think that would be a bigger clean up to be done with a separate PR as we have that at several other places, too.

[richard67]

@artur-stepien @HLeithner I just see that the filesystem exception already calls Path::removeRoot here: https://github.com/joomla-framework/filesystem/blob/3.x-dev/src/Exception/FilesystemException.php#L33

And that method removes every occurrence of the Joomla root or the PHP temp folder from the exception message.

So I have to correct my previous comments, it is not necessary to wrap the $src and $dst into Path::removeRoot calls.