Error

Error This pull request is targeted at the master branch. Pull requests should no longer be merged to master.

User tests: Successful: Unsuccessful:

nikosdion
23 Nov 2013

Overview

Note: See Joomla! Issue Tracker item 32785 Please enter your test results there, not in this Pull Request on GitHub, prefixing your message with @test. Thank you.

Technical analysis

BUG 1

Updating Joomla! is impossible on servers which have a reserved, unwriteable, system folder called logs. All Plesk-based servers and certain hosts put a directory called logs inside the public web root. They use it to store Apache log files. Since this directory is owned by a system user and has strict permissions (0700, 0750 or 0755) it is not writeable by PHP.

Joomla!'s update packages have a folder called logs with a sole index.html file in it. When the update archive extraction reaches that file the update hangs and the user is left with a partially updated, bricked site. As the users cannot make this directory writeable by any means, they end up with a site that is impossible to update.

This Pull Request modifies the archive extractor (administrator/components/com_joomlaupdate/restore.php) to ignore unwriteable files in the logs directory, allowing the update process to continue.

BUG 2

Moreover, logging was added recently to com_joomlaupdate. However, if the log directory is not set in Global Configuration or if it's set to an unwriteable directory the failure to write a log file results in an error message and inability to update Joomla!. This is just plain wrong, as logging is an optional feature and its failure MUST NOT block the update of the CMS. This PR wraps the JLog::add lines in com_joomlaupdate with a try/catch statement to effectively ignore any logging errors and allow the site to be updated.

Reproducing the bug and testing the PR

Reproducing the bug

Install a new Joomla! site based on the latest master branch
Go to Components, Joomla! Update and click on Options
Set the Update Server to Custom URL
Set the Custom URL to http://akeebatest.s3.amazonaws.com/fakejupdate/list.xml
Click on Save & Close. A new version 3.2.2 is detected. DO NOT TRY TO UPDATE JUST YET.
Make your site's logs directory unwriteable. IMPORTANT: Changing the permissions IS NOT enough! com_joomlaupdate will try to fix the permissions if it can. You are advised to change the directory's ownership to root (Linux, Mac OS X) or Administrator (Windows). Then give it 0700 permissions (Linux, Mac OS X) or remove everyone's access except Administrator (Windows)
Go back to your site and try to update to version 3.2.2.
You get an error message that the log could not be opened. BUG 2 CONFIRMED.
Create a new top-level directory on your site called log-ok and make sure it's writeable by PHP (e.g. give it 0777 permissions – yes, 0777 is the number of the beast but this is just a test site which we'll delete afterwards)
Go to Global Configuration and set the log directory from ...../logs to ...../log-ok (the dots represent the existing pathname already displayed)
Go back to Components, Joomla! Update and start the update
The package downloads and starts extracting.
The update stops somewhere between 3% and 4% with an error saying that ...../logs/index.html could not be opened for writing. BUG 1 CONFIRMED.

Testing the PR (true positive testing)

Let's make sure the PR fixes this issue.

On the site you have already created delete tmp/joomla.zip
Go to Global Configuration and set the log directory from ...../logok to ...../logs (the dots represent the existing pathname already displayed)
Go back to Components, Joomla! Update and start the update
The package downloads and starts extracting. FIX FOR BUG 2 CONFIRMED.
The update completes. FIX FOR BUG 1 CONFIRMED.

Note: you will get a warning similar to the one in this picture. This is expected, as my fake update package lacks a few files.

Also note that the Joomla! version is still 3.2.1. Did I mention enough times already that my update package is fake? :)

Cross-testing the PR (false positive testing)

Let's make sure that making a different directory unwriteable still leads to an error. This is desirable, as it indicates ownership/permissions issues on the site.

On the site you have already created delete tmp/joomla.zip if it still exists
Make your site's cli directory unwriteable. IMPORTANT: Changing the permissions IS NOT enough! com_joomlaupdate will try to fix the permissions if it can. You are advised to change the directory's ownership to root (Linux, Mac OS X) or Administrator (Windows). Then give it 0700 permissions (Linux, Mac OS X) or remove everyone's access except Administrator (Windows)
Go back to Components, Joomla! Update and start the update
The package downloads and starts extracting. It throws an error halfway through about not being able to write a file in the cli directory. FALSE POSITIVE AVOIDANCE TEST SUCCESSFUL.

Project management information

Backwards compatibility

Backwards compatibility is not affected.

Information to developers

None is required. This PR only modifies code used internally by Joomla!. Furthermore, the public API of the code is immutable after the application of this PR

Severity of this bug

Medium-High as it makes it impossible to update Joomla! on several popular hosts.

07a6f55 22 Nov 2013

Updating restore.php of Joomla! Update to not throw errors when the logs directory is unwriteable

49fa350 22 Nov 2013

Logging in com_joomlaupdate could cause a crash if site debugging is enabled and the logs directory is unwriteable.

442a925 22 Nov 2013

Actually fixes the restore.php file

nikosdion - open - 23 Nov 2013

brunobatista - comment - 23 Nov 2013

tranduyhung - comment - 6 Dec 2013

Hi @nikosdion

I got a warning after changing update URL:

Update: :Collection: Could not open http://akeebatest.s3.amazonaws.com/fakejupdate/list.xml

What in list.xml was:

    <Error>
        <Code>AccessDenied</Code>
        <Message>Access Denied</Message>
        <RequestId>2365783E16B5F52C</RequestId>
        <HostId>9imETGth645W0MBBKsqe6fl0qHG4gC2ILTHC1OvdCibJrvBl2zsBuXw6MWsQIxYe</HostId>
    </Error>

Hope it is just a temporary issue. I will try again later.

nikosdion - comment - 6 Dec 2013

@tranduyhung Thanks! I fixed the permissions issue, you can now retest.

tranduyhung - comment - 6 Dec 2013

Hi @nikosdion

Thank you!

We have a new issue, the fake package doesn't have joomla.xml file. I got this after the package was downloaded and extracted:

Warning
JInstaller: :Install: Cannot find XML setup file
Unable to detect manifest file
Failed deleting joomla.xml
Joomla! Version Update Status
Your site has been successfully updated. Your Joomla version is now 3.2.1.

Well, it should not say "Your site has been successfully updated" after a failed update :)

nikosdion - comment - 8 Dec 2013

@tranduyhung This is outside the scope of this PR. Let's solve one problem at a time

@oc666 No! If you add them inside one big try/catch block and the logs directory is unwriteable you will end up not running everything after the first failed log statement. In other words, the update would fail. That's why you need to wrap each and every JLog line with a try/catch.

garyamort - comment - 29 Jan 2014

I ran across this in some other components as well as the base JControllerLegacy[and the fof library).
https://github.com/joomla/joomla-cms/pull/2948/files

If it is acceptable to ignore log failures, then @oc666 has the correct approach - just the wrong file.

I made some hacky fallback logic in JLog::addEntry for the creation of the logger, however if instead you wrap line 153 it solves the entire issue:
https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/log/log.php#L153
try
{
self::$instance->addLogEntry($entry);
}
catch (\RuntimeException $e)
{
// Do nothing; the logs directory is probably not writeable (usually Plesk-based hosts)
}

This solution is a lot cleaner since:
1) It only catches RuntimeExceptions, which is what a JLogLogger is supposed to throw if there are problems. So any underlying PHP problems will not be suppressed.

2) It doesn't require editing ever single call to JLog::add since it is embedded into the method.

3) For any weird edge cases where some component cannot be executed if it does not have a log [for example, a component that uses the logfile to keep track of what items it has already processed], those components can create their own custom log class
MyComponentLog extends JLog
// Old functionality which will fail if unable to create a log entry
public static function add($entry, $priority = self::INFO, $category = '', $date = null)
{
// Automatically instantiate the singleton object if not already done.
if (empty(self::$instance))
{
self::setInstance(new JLog);
}

            // If the entry object isn't a JLogEntry object let's make one.
            if (!($entry instanceof JLogEntry))
            {
                    $entry = new JLogEntry((string) $entry, $priority, $category, $date);
            }

            self::$instance->addLogEntry($entry);
    }

And inside the component call MyComponentLog::add() instead of JLog::add()