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

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

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.

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.

@richard67 thanks for the explanation

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

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.

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

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

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

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.

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.

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

even an untranslated hard coded string would be useful imho

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.

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.

@brianteeman I've added one log entry to see when the permission fix runs. So additional tests could be to check that this log is not there when updating something else than a 5.2.0 new installation, or when updating a 5.2.0 new installation on Windows.

I would prefer not to add more logs e.g. about reasons for early return or if the chmod commands were successful or not (which would require to check permissions again after each change), or to count files and folders which had 777 before or anything like that.

I've updated the testing instructions by a 3rd test on Windows to see it doesn't run. And I've deprecated the language string as the fix will be removed in 6.0. Possibly we will have a health check then to check and fix permissions in a more flexible way because 755 and 644 is not suitable for all.

Last remaining problem is that when updating with the CLI, the log is the plain language constant, not the (English) text.

The reason is that the CLI core update reads the com_joomlaupdate administrator language only when it's initialized, i.e. before the update is unpacked and the new language file has the new string.

I could add a hack to the script.php to check if the translated text is equal to the text constant and if so, use a hardcoded text, or to use a hardcoded text in general.

@brianteeman What would you suggest?

just use a hard coded text

I have tested this item ✅ successfully on a68239e

worked for me

@brianteeman I've just noticed that when updating with the CLI, the file permissions are not fixed (but the folder permissions are). When updating via backend file permissions are fixed by the extract.php. It seems the CLI uses its own extract routine. I could either add a fix for the files to my new method in script.php, but that would mean to go through all files in all folders, or the CLI update could be changed so it does the same as the extract.php does regarding file permissions. I will discuss that with SST. Of course opinions are welcome.

i didnt test with the cli

@brianteeman I've moved the test for Windows from 3 to 4 and added a new test 3 for the CLI update. Due to the code changes it needs to repeat also the test 2 for "normal" update via backend (not CLI). Could you test again, and if possible also the CLI test 3? Thanks in advance.

What is the status of this PR now that 5.2.1 has been released?

@brianteeman The plan is to intensively test it and ship it with 5.2.2 at the regular date end of this month. I have already adapted the PR to that regarding the version check, and I have removed the (due to log rotation) error prone check for new installation with the log file date. And I have updated the description and testing instructions. So it is ready for testing.

P.S.: See also the release announcement https://www.joomla.org/announcements/release-news/5917-joomla-5-2-1-security-release.html

For sites created with the affected 5.2.0 packages, an automated solution updating the permissions of affected files and folders will be shipped with the next regular 5.2.x release.

The mentioned automated solution is this PR here.

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

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

I have tested this item ✅ successfully on 38d7f06

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.

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

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

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.

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

I have tested this item ✅ successfully on 38d7f06

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.

RTC

Thank you for this fix.

@brianteeman @ramalama @SniperSister Thanks for testing.