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

[5.2] Fix wrong core files and core folders permissions 777 on update from 5.2.0 or 5.2.1 #44379

Merged

Conversation

richard67
Copy link
Member

@richard67 richard67 commented Oct 30, 2024

Pull Request for Issue #44331 .

Summary of Changes

This pull request (PR) adds a procedure to the script.php file to fix permissions of files and folders which have wrong 777 permissions in a way which avoids to touch 3rd party files and folders.

The fix procedure runs only when updating from a 5.2.0 or a 5.2.1, and only files and folder which have permission 777 are fixed.

A new entry in the administrator/logs/joomla_update.php log shows if the fix procedure has run.

It uses a hard-coded text because a new language string would be removed again in 6.0, and it would not be known by the CLI when updating, and when updating in the backend it would only be available in English if not using a fully localized update package.

Testing Instructions

Test 1: File and folder permissions after update from a 5.2.0 new installation on Linux using the backend (administrator)

  1. Make a new installation of 5.2.0 stable on a Linux server.

  2. In a command shell in the Joomla root folder, check for wrong 777 file permissions:

find ./ -type f -perm 777
  1. Check for wrong 777 folder permissions:
find ./ -type d -perm 777
  1. Update to the patched package built by Drone for this PR, either with the custom update URL or with Upload&Update and a previously downloaded update package.
    You can find the custom update URL and the update package here: https://artifacts.joomla.org/drone/joomla/joomla-cms/5.2-dev/44379/downloads/80196/

  2. Check again the file and folder permissions.

  3. Check if there is an entry "Fixing permissions for files and folders." in the administrator/logs/joomla_update.php log just after the "Deleting removed files and folders.".

Test 2: File and folder permissions after update from a 5.2.0 new installation on Linux using the CLI

This test requires PHP CLI to be available.

  1. Make a new installation of 5.2.0 stable on a Linux server.

  2. In a command shell in the Joomla root folder, check for wrong file permissions:

find ./ -type f ! -perm 644
  1. Check for wrong folder permissions:
find ./ -type d ! -perm 755
  1. In backend (administrator), change the update source to "Custom URL" and enter the custom update URL created by Drone for this PR, which is: https://artifacts.joomla.org/drone/joomla/joomla-cms/5.2-dev/44379/downloads/80196/pr_list.xml

  2. In a command shell, change to the Joomla root.

  3. Use the CLI to check for updates:

php cli/joomla.php core:update:check
  1. Use the CLI to update:
php cli/joomla.php core:update
  1. Check again the file and folder permissions.

  2. Check if there is an entry "Fixing permissions for files and folders." in the administrator/logs/joomla_update.php log just after the "Deleting removed files and folders.".

Test 3: The fix does not run when updating from a 5.2.0 new installation on Windows

  1. Make a new installation of 5.2.0 stable on a Windows server.

  2. Update to the patched package built by Drone for this PR, either with the custom update URL or with Upload&Update and a previously downloaded update package.
    You can find the custom update URL and the update package here: https://artifacts.joomla.org/drone/joomla/joomla-cms/5.2-dev/44379/downloads/80196/

  3. Check if there is an entry "Fixing permissions for files and folders." in the administrator/logs/joomla_update.php log just after the "Deleting removed files and folders.".

Actual result BEFORE applying this Pull Request

Test 1: File and folder permissions after update from a 5.2.0 new installation on Linux using the backend (administrator)

All core files and folders have 777 permissions.

Test 2: File and folder permissions after update from a 5.2.0 new installation on Linux using the CLI

All core files and folders have 777 permissions.

Test 3 The fix does not run when updating from a 5.2.0 new installation on Windows

N/a.

Expected result AFTER applying this Pull Request

Test 1: File and folder permissions after update from a 5.2.0 new installation on Linux using the backend (administrator)

There are no core files or core folders with 777 permissions.

There is an entry "Fixing permissions for files and folders." in the administrator/logs/joomla_update.php log just after the "Deleting removed files and folders.".

Test 2: File and folder permissions after update from a 5.2.0 new installation on Linux using the CLI

There are no core files or core folders with 777 permissions.

There is an entry "Fixing permissions for files and folders." in the administrator/logs/joomla_update.php log just after the "Deleting removed files and folders.".

Test 3: The fix does not run when updating from a 5.2.0 new installation on Windows

There is no entry "Fixing permissions for files and folders." in the administrator/logs/joomla_update.php log just after the "Deleting removed files and folders.".

Link to documentations

Please select:

  • No documentation changes for docs.joomla.org needed

  • No documentation changes for manual.joomla.org needed

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-5.2-dev labels Oct 30, 2024
@brianteeman
Copy link
Contributor

I thought @SniperSister said they JSST were doing a new release to fix this?

@dgrammatiko
Copy link
Contributor

@richard67 worth creating a GitHub action that sets the correct permission to files/folders after each commit/merge or add the 2 bash commands in the build.php...

@richard67
Copy link
Member Author

richard67 commented Oct 30, 2024

I thought @SniperSister said they JSST were doing a new release to fix this?

@brianteeman That was for fixing the wrong permissions on all files in the packages. This one here is for fixing files in the repo. The updater would fix permissions on update or on reinstall core files for these 49 files, too, but on new installations they still would have the 755 permission. So that's 2 different things which are both needed, permissions fixed in the repo, and packages being build on a Unix file system and not NTFS.

worth creating a GitHub action that sets the correct permission to files/folders after each commit/merge or add the 2 bash commands in the build.php...

@dgrammatiko Yes. I would prefer a cyclic GitHub action resulting in a PR when mode changes needed, similar to the translation PRs, because currently our build.php script does not make any modifications which later would need a check-in and commit with git, and I would prefer not to change that behaviour. Of course we could add a big message to the build-script telling on the command line if some changes were made, but I still worry release managers might miss that message.

@richard67
Copy link
Member Author

richard67 commented Oct 30, 2024

P.S.: Of course we also could just do a chmod on the files in the build.php script before we pack the packages, but that would not change the permissions in the git index on GitHub. In this case the permissions in git would not matter at all for the packages. But of course it still needs to build the release on a Unix file system so that chmod has an effect.

@brianteeman
Copy link
Contributor

@richard67 thanks for the explanation

@richard67
Copy link
Member Author

@brianteeman Indeed it might turn out that this PR is not needed if the same change is done together with the release. I've made this PR here just in case if it helps with that and as some kind of documentation how the modes can be checked in git.

@richard67
Copy link
Member Author

P.S.: Even if this PR might be obsolete when the mode changes are done with the release (version bump commit), this PR can be used to check that. When the release has been made and the branch of this PR is updated to the latest 5.2.0-dev, it should show no changes.

@richard67 richard67 changed the title [5.2] Change file permissions from 755 to 644 for 49 files [5.2] Change file permissions for 49 files in git index and fix permissions on update Nov 1, 2024
@richard67
Copy link
Member Author

I have updated this PR to fix the permissions when updating from 5.2.0 which was a new installation.

@richard67 richard67 changed the title [5.2] Change file permissions for 49 files in git index and fix permissions on update [5.2] Change file permissions for 49 files in git index and fix folder permissions on update Nov 1, 2024
@richard67
Copy link
Member Author

@brianteeman I've updated this PR so it fixes the issue on update, too. I would be very happy if you could test tomorrow.

@brianteeman
Copy link
Contributor

Not at my pc to check but I am not sure if this will be enough. Scenario to check.
Install the faulty 5.2
Install some extensions. Check the permissions of those extensions - I suspect that they will inherit the faulty parent folder permissions.
Upload some media. Check the permissions of those files - I suspect that they will inherit the faulty parent folder permissions.
If my suspicion is correct then I'm not sure this PR will fix it as it is only correcting the specified files.

I could be completely wrong as I'm not at a PC to check

very surprised that the JSST have not released a fix for this yet

@richard67
Copy link
Member Author

Install some extensions. Check the permissions of those extensions - I suspect that they will inherit the faulty parent folder permissions.
Upload some media. Check the permissions of those files - I suspect that they will inherit the faulty parent folder permissions.
If my suspicion is correct then I'm not sure this PR will fix it as it is only correcting the specified files.

@brianteeman I assume these suspicions are not correct since on Unix/Linux new files and folders are created based on the user's umask. Inheritance of parent folders only plays a role when it comes to navigating to folders. You can't navigate to a folder if you don't have the permission (x) to navigate to its parent folder.

So I'd really be happy if you could test whenever you are back at a PC and can find the time. Thanks in advance.

@richard67
Copy link
Member Author

very surprised that the JSST have not released a fix for this yet

@brianteeman Me too, so I took the action to provide a fix. If it has enough tests it will hopefully come in time.

@brianteeman
Copy link
Contributor

brianteeman commented Nov 3, 2024

  1. Installed Joomla 5.2
  2. Created some new images and image folders
  3. Installed a large component
  4. Verified that the permissions were wrong on j5.2 and were correct on new images and component
  5. Updated joomla using the prebuilt update in this PR
  6. verified that ALL permissions were now correct

Questions

  1. Why "and the update log not being older than 1 hour."
  2. Can we add an entry in the joomla_update log to indicate that the permissions have been changed

@brianteeman
Copy link
Contributor

even an untranslated hard coded string would be useful imho

@richard67
Copy link
Member Author

Can we add an entry in the joomla_update log to indicate that the permissions have been changed

@brianteeman I‘m not sure if this would be easily possible inside the method or the one where it is called from, we currently don’t use logs there and just collect errors.

@richard67
Copy link
Member Author

P.S.: A log entry would also need a new language string as the logs should be translated, and at the time when people update that string would not be available yet in their language, that’s why I was not sure if I shall add a log.

@richard67
Copy link
Member Author

Testing instructions updated. Obsolete part removed (was just fixed with the 5.2.1 release).

@richard67 richard67 changed the title [5.2] Change file permissions for 49 files in git index and fix folder permissions on update [5.2] Fix wrong core files and core folders permissions 777 on update from 5.2.0 or 5.2.1 Nov 7, 2024
@brianteeman
Copy link
Contributor

so 5.2.1 is not a security update as it doesnt fix existing potentialy vulnerable sites at all?

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 38d7f06


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44379.

@richard67
Copy link
Member Author

so 5.2.1 is not a security update as it doesnt fix existing potentialy vulnerable sites at all?

@brianteeman Yes and no. It is a security update to avoid that the number of vulnerable new installations is increasing. For existing vulnerable sites 5.2.1 fixes at least the permissions of the files when the update is made with the administrator. When made with the CLI it is a bit different.

Maybe @SniperSister can explain better than me why we made that decision and why we classified 5.2.1 as security release.

@brianteeman
Copy link
Contributor

Installed the faulty 5.2 package
Updated to the 5.2.1 release
Verified that file and folder permissions were incorrect

Updated to the release from this PR
Verified that the file and folder permissions were now correct
Verified that the update log records the file and permissions were checked

image

image

@brianteeman
Copy link
Contributor

For existing vulnerable sites 5.2.1 fixes at least the permissions of the files when the update is made with the administrator

fixing the files is far less of an issue than fixing the folders which remain as 777

@richard67
Copy link
Member Author

fixing the files is far less of an issue than fixing the folders which remain as 777

@brianteeman I know. But fixing the folders requires a fix routine like the one provided with this PR, which always might include some risk (e.g. a new bug), while fixing the files happens already since ages with the updater (if not CLI) and so is no new risk.

So we wanted to be sure that this fix is properly tested.

Copy link

@ramalama ramalama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test 1

  1. Unzipped a 5.2.0 pkg and installed Joomla on a Linux Shared Hoster.
  2. Verified that wrong 777 permissions exist for files and folders.
  3. Updated with the supplied pkg Joomla_5.2.2-dev+pr.44379-Development-Update_Package.zip
  4. File and folder permission checked - no files/folders with 777 any more
  5. Checked successfully that there is an entry "Fixing permissions for files and folders." in the administrator/logs/joomla_update.php log just after the "Deleting removed files and folders.".

Test 2

  1. Unzipped a 5.2.0 pkg and installed Joomla on a Linux Shared Hoster.
  2. Verified wrong file permissions exist
  3. Verified wrong dir permission exist
  4. updated Joomla Installation Root to PR
  5. Check for update via CLI successful
  6. Update successfuly applied via CLI
  7. Successfully verified permissions (find ./ -type f ! -perm 644 and find ./ -type d ! -perm 755 yield now 0 results)
  8. Log Entry verified

Test 3

  1. Installation of J5.2.0 on a Win 11 Client with Xampp 8.2.0
  2. Successfully updated via update package Joomla_5.2.2-dev+pr.44379-Development-Update_Package.zip
  3. Verfied that there is no "Fixing permissions for files and folders." in the update log.

@richard67
Copy link
Member Author

@ramalama Please do not use GitHub approval to mark a successful real test. Please use the issue tracker. Go to https://issues.joomla.org/tracker/joomla-cms/44379 , use the blue "Test this" button at the top left corner, select your test result and submit. Otherwise your test is not counted. Please do the same with other pull requests which you have recently tested and approved on GitHub only. GitHub approval only means code review.

@ramalama
Copy link

ramalama commented Nov 8, 2024

I have tested this item ✅ successfully on 38d7f06


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44379.

@SniperSister
Copy link
Contributor

I have tested this item ✅ successfully on 38d7f06

Works as described! I also tested the performance impact of the chmod operation and on my test machine it caused an additional 750ms of script runtime, which is very reasonable.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44379.

@richard67
Copy link
Member Author

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/44379.

@richard67 richard67 added the Language Change This is for Translators label Nov 9, 2024
@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 9, 2024
@richard67 richard67 added this to the Joomla! 5.2.2 milestone Nov 9, 2024
@richard67 richard67 added Release Blocker and removed Language Change This is for Translators labels Nov 9, 2024
@Hackwar Hackwar enabled auto-merge (squash) November 10, 2024 19:39
@Hackwar
Copy link
Member

Hackwar commented Nov 10, 2024

Thank you for this fix.

@Hackwar Hackwar merged commit 63305a4 into joomla:5.2-dev Nov 10, 2024
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 10, 2024
@richard67 richard67 deleted the 5.2-dev-fix-file-permissions-2024-10-30 branch November 10, 2024 20:05
@richard67
Copy link
Member Author

@brianteeman @ramalama @SniperSister Thanks for testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants