User tests: Successful: Unsuccessful:
Pull Request resolves # .
This pull request (PR) updates the composer dependency "joomla/filesystem" from version 3.2.0 to version 3.3.0 and makes the necessary changes in the CMS core to adapt to the changes from this update.
It is a security hardening which makes sure that the File::upload method of that dependency does not allow to upload unsafe file by default, which includes executable file like e.g. PHP files.
The dependency update made with this PR with the 3.x version of the framework is equivalent to the update from version 4.1.0 to version 4.2.0 made in the 6.1-dev branch for 6.1.2-rc2 with this commit: a19f745
The code adjustments are equal to those made in the 6.1-dev branch for 6.1.2-rc3 with PR #48038 for allowing the upload of executable files, e.g. PHP files, where that shall be allowed by design because only superusers can do it, with one exception:
The changes in the template manager (file administrator/components/com_templates/src/Model/TemplateModel.php) are not backported as that would require additional changes. See this comment below for details: #48080 (comment)
It seems that with the change of the input filter from CMS to framework in 6.x with the framework filter missing the check for unsafe files, the possibility to upload PHP files in the template manages was enabled more or less by the way but not by intention.
It needs to be clarified if that shall be back-ported to 5.4.
If so, I will make the necessary PR which will also contain the change mentioned above.
Additional remark: The reason why the mentioned update has been made with the 6.1.2 release but not with 5.4.8 is because it's more urgent in 6.1 where the Joomla\CMS\Filesystem\File class which already included the security checks was moved to the b/c plugin, so it is much more likely that extension developers have already moved to the Joomla\Filesystem\File class where that feature was missing before the update.
Verify that the update made here is equivalent to the update in the 6.1-dev branch by checking the release notes and changes here:
Verify that the code adjustments made in this PR here are equal to those made with PR #48038 , except of the "joomla/filesystem" version in the code comments and except of the change in file administrator/components/com_templates/src/Model/TemplateModel.php, and that there are no other places in a current 5.4-dev branch where the File::upload method of the Joomla\Filesystem\File class is used.
Attention: This PR cannot be applied with the Patchtester component. It needs to have a local development environment with composer and NPM there you have checked out this PR and run a composer installand an npm ci, or you have to use the patched package created by Drone for this PR.
Test that the following still works with this PR applied:
PHP Warning: Attempt to read property "name" on null in administrator/components/com_joomlaupdate/src/Model/UpdateModel.php on line 2024administrator/components/com_templates/src/Model/TemplateModel.php.File::upload method of the Joomla\Filesystem\File class is used.Please select:
Documentation link for guide.joomla.org:
No documentation changes for guide.joomla.org needed
Pull Request link for manual.joomla.org:
No documentation changes for manual.joomla.org needed
| Status | New | ⇒ | Pending |
| Category | ⇒ | Administration com_installer com_joomlaupdate com_templates External Library Composer Change |
I have tested this item 🔴 unsuccessfully on 0e8e928
✅ 1. Checked 3.3.0 source code changes are identical to 4.2.0:
git clone https://github.com/joomla-framework/filesystem.git
cd filesystem
git diff 3.2.0 3.3.0 > /tmp/32-33.patch
git diff 4.1.0 4.2.0 > /tmp/41-42.patch
diff /tmp/32-33.patch /tmp/41-42.patch
✅ 2. Compared https://github.com/joomla/joomla-cms/pull/48038/changes with https://github.com/joomla/joomla-cms/pull/48080/changes for the tree PHP files
File::upload:
find . -name \*.php | xargs grep -n 'File::upload'gh pr checkout 48080 && composer icomposer info joomla/filesystem shows version 3.3.0libraries/vendor/joomla/filesystem/src/File.php contains the changesI have tested this item 🔴 unsuccessfully on 0e8e928
✅ 1. Checked 3.3.0 source code changes are identical to 4.2.0:
git clone https://github.com/joomla-framework/filesystem.git
cd filesystem
git diff 3.2.0 3.3.0 > /tmp/32-33.patch
git diff 4.1.0 4.2.0 > /tmp/41-42.patch
diff /tmp/32-33.patch /tmp/41-42.patch
✅ 2. Compared https://github.com/joomla/joomla-cms/pull/48038/changes with https://github.com/joomla/joomla-cms/pull/48080/changes for the tree PHP files
File::upload:
find . -name \*.php | xargs grep -n 'File::upload'gh pr checkout 48080 && composer icomposer info joomla/filesystem shows version 3.3.0libraries/vendor/joomla/filesystem/src/File.php contains the changes@muhme As the failed test fails without and with this PR, so this PR doesn't change anything on it, I might just have to change my testing instructions to something like "works like before".
Currently I haven't found the reason why uploading PHP files in the template manager works in 6.1 but doesn't work in 5.4.
@dgrammatiko Maybe you have an idea if there was a change for that in 6.0 or 6.1?
Currently I haven't found the reason why uploading PHP files in the template manager works in 6.1 but doesn't work in 5.4.
@richard67 You need to change this line https://github.com/joomla/joomla-cms/blob/5.4-dev/administrator/components/com_templates/src/Controller/TemplateController.php#L519 to
$upload = $this->input->files->get('files', null, 'raw');The reason it works for Joomla 6 but not Joomla 5 is because on Joomla 5, we use CMS Input and input->files->get will reject the unsafe files (PHP file in this case) unless we use raw filter in get method. See https://github.com/joomla/joomla-cms/blob/5.4-dev/libraries/src/Input/Files.php#L95
On Joomla 6, we use framework Input and it does not have this security feature, mean the unsafe files won't be rejected when calling input->files->get
@joomdonation Thanks a lot. I will update my PR here after work to fix that.
Maybe I should fix that with a separate PR?
Better have it included in this PR, otherwise the modification in TemplateModel to allow uploading un-safe files is useless.
| Labels |
Added:
Composer Dependency Changed
PR-5.4-dev
|
||
| Category | Administration com_installer com_joomlaupdate com_templates External Library Composer Change | ⇒ | Administration com_installer com_joomlaupdate External Library Composer Change |
| Title |
|
||||||
Better have it included in this PR, otherwise the modification in TemplateModel to allow uploading un-safe files is useless.
I have decided against this and have reverted the change in the administrator/components/com_templates/src/Model/TemplateModel.php.
To me it seems that the possibility to upload PHP files in the template manager just slipped through when the switch from the CMS to the framework filter was made.
It needs a separate decision if that shall be made possible in 5.4, too.
If that will be done, I can make a new PR.
I will put it on the table in the relevant teams.
@HLeithner @tecpromotion Could you review again with the latest change? See the updated PR description for details That would be great. Thanks in advance.
@muhme Could you test again? The change in the template manager was reverted, so it needs only the core update and extension update to be tested.
To me it seems that the possibility to upload PHP files in the template manager just slipped through when the switch from the CMS to the framework filter was made.
Just for information, It was decided to allow uploading PHP files through templates manager, see the description from PR for 6.1 #48038. @SniperSister agreed/advised to do so
To me it seems that the possibility to upload PHP files in the template manager just slipped through when the switch from the CMS to the framework filter was made.
Just for information, It was decided to allow uploading PHP files through templates manager, see the description from PR for 6.1 #48038. @SniperSister agreed/advised to do so
@joomdonation Yes, I know. But there it was not a change on functionality, it had worked there also before that PR.
Here on 5.4 it would be a change of functionality coming with a minor version. This is something which I don't want to decide alone.
To me it seems that the possibility to upload PHP files in the template manager just slipped through when the switch from the CMS to the framework filter was made.
Just for information, It was decided to allow uploading PHP files through templates manager, see the description from PR for 6.1 #48038. @SniperSister agreed/advised to do so
@joomdonation Yes, I know. But there it was not a change on functionality, it had worked there also before that PR.
Here on 5.4 it would be a change of functionality coming with a patch version. This is something which I don't want to decide alone.
I have tested this item ✅ successfully on 6b7def4
✅ 1. Checked 3.3.0 source code changes are identical to 4.2.0:
git clone https://github.com/joomla-framework/filesystem.git
cd filesystem
git diff 3.2.0 3.3.0 > /tmp/32-33.patch
git diff 4.1.0 4.2.0 > /tmp/41-42.patch
diff /tmp/32-33.patch /tmp/41-42.patch
✅ 2. Review the code adjustments
TemplateModel.php and w/o composer.json changes)File::upload:
find . -name \*.php | xargs grep -n 'File::upload'TemplateModel.phpgh pr checkout 48080 && composer icomposer info joomla/filesystem shows version 3.3.0libraries/vendor/joomla/filesystem/src/File.php contains the changesinstallation folder and 'Upload and Update' with Joomla_6.1.2-Stable-Update_Package.zipI have tested this item ✅ successfully on 6b7def4
✅ 1. Checked 3.3.0 source code changes are identical to 4.2.0:
git clone https://github.com/joomla-framework/filesystem.git
cd filesystem
git diff 3.2.0 3.3.0 > /tmp/32-33.patch
git diff 4.1.0 4.2.0 > /tmp/41-42.patch
diff /tmp/32-33.patch /tmp/41-42.patch
✅ 2. Review the code adjustments
TemplateModel.php and w/o composer.json changes)File::upload:
find . -name \*.php | xargs grep -n 'File::upload'TemplateModel.phpgh pr checkout 48080 && composer icomposer info joomla/filesystem shows version 3.3.0libraries/vendor/joomla/filesystem/src/File.php contains the changesinstallation folder and 'Upload and Update' with Joomla_6.1.2-Stable-Update_Package.zipI have tested this item ✅ successfully on 6b7def4
✅ 1. Checked 3.3.0 source code changes are identical to 4.2.0:
git clone https://github.com/joomla-framework/filesystem.git
cd filesystem
git diff 3.2.0 3.3.0 > /tmp/32-33.patch
git diff 4.1.0 4.2.0 > /tmp/41-42.patch
diff /tmp/32-33.patch /tmp/41-42.patch
TemplateModel.php and w/o composer.json changes)File::upload:
find . -name \*.php | xargs grep -n 'File::upload'TemplateModel.phpgh pr checkout 48080 && composer icomposer info joomla/filesystem shows version 3.3.0libraries/vendor/joomla/filesystem/src/File.php contains the changesinstallation folder and 'Upload and Update' with Joomla_6.1.2-Stable-Update_Package.zip
@HLeithner @tecpromotion Could you review and if ok approve this PR. That would be great. Thanks in advance.