Dynamic software update system using PHP

Question

I have this code I have made to check after new release, when there is a new one it ask you for install it, and then installs it if you accept.

The file is a zip file with the new folders and files inside there be unzip and pull into the system.

Can it be improved?

Update.php

<h1>DYNAMIC UPDATE SYSTEM</h1>
<?php

$localVersion = file_get_contents('current-release-versions.php');


ini_set('max_execution_time',60);
//Check For An Update
$getVersions = file_get_contents('http://yousite.com/UPDATE-PACKAGES/current-release-versions.php') or die ('ERROR');
$found = false;

if ($getVersions != '')
{
    echo "<p>CURRENT VERSION: {$localVersion}</p>";
    echo '<p>Reading Current Releases List</p>';
    $versionList = explode("\n", $getVersions);

  foreach ($versionList as $aV) {
        if ($aV > $localVersion) {
            echo '<p>New Update Found: v'.$aV.'</p>';
            $found = true;



      if(isset($_GET['doUpdate']) && !empty($_GET['doUpdate'])){
            if($_GET['doUpdate'] == true) {
          // capture the start time
          $start_time = microtime(true);

          // Download The File If We Do Not Have It
                if (!is_file('UPDATES/MMD-CMS-'.$aV.'.zip')) {

            echo '<p>Downloading New Update</p>';
                    $newUpdate = file_get_contents("http://yousite.com/UPDATE-PACKAGES/MMD-CMS-{$aV}.zip");
            if (!is_dir('UPDATES/' )) {
              mkdir ('UPDATES/');
            }

            $dlHandler = fopen('UPDATES/MMD-CMS-'.$aV.'.zip', 'w');

            if ( !fwrite($dlHandler, $newUpdate)) {
              echo '<p>Could not save new update. Operation aborted.</p>'; exit();
            }

            fclose($dlHandler);
                    echo '<p>Update Downloaded And Saved</p>';

                }
          else {
            echo '<p>Update already downloaded.</p>';
          }

          function deleteDirectory($dir) {
              if (!file_exists($dir)) return true;
              if (!is_dir($dir)) return unlink($dir);
              foreach (scandir($dir) as $item) {
                  if ($item == '.' || $item == '..') continue;
                  if (!deleteDirectory($dir.DIRECTORY_SEPARATOR.$item)) return false;
              }
              return rmdir($dir);
          }



                // Open The File And Do Stuff
                $zipHandle = zip_open('UPDATES/MMD-CMS-'.$aV.'.zip');


          echo '<ul>';
                while ($aF = zip_read($zipHandle) ) {

                    $thisFileName = zip_entry_name($aF);
                    $thisFileDir = dirname($thisFileName);
                    //Continue if its not a file
                    if (substr($thisFileName, -1, 1) == '/') {
              continue;
            }


                    //Make the directory if we need to...
            if (!is_dir($thisFileDir))
                    {
                         mkdir ($thisFileDir, 0777, true);
                         echo '<li>Created Directory '.$thisFileDir.'</li>';
                    }




              deleteDirectory(".DS_Store");


                    //Overwrite the file
                    if ( !is_dir($thisFileName) ) {
              $fileName = str_replace($thisFileDir."/", "", $thisFileName);

                            echo '<li>'.$fileName.'...........';
                            $contents = zip_entry_read($aF, zip_entry_filesize($aF));
                            $contents = str_replace("\r\n", "\n", $contents);
                            $updateThis = '';

                            //If we need to run commands, then do it.
                            if ( $thisFileName == 'upgrade.php' )
                            {
                                $upgradeExec = fopen ('upgrade.php','w');
                                fwrite($upgradeExec, $contents);
                                fclose($upgradeExec);
                                include ('upgrade.php');
                                unlink('upgrade.php');
                                echo' EXECUTED</li>';
                            }
                            else
                            {
                                $updateThis = fopen($thisFileName, 'w');
                                fwrite($updateThis, $contents);
                                fclose($updateThis);
                                unset($contents);
                                echo' UPDATED</li>';
                            }
              }

                }
          unlink('UPDATES/MMD-CMS-'.$aV.'.zip');
          deleteDirectory("__MACOSX");
          deleteDirectory(".DS_Store");
          echo "<li>
            Deleted MMD-CMS-{$aV}.zip
          </li>";
                echo '</ul>';

          // display how long the script took
          echo "execution took: ".
                  (microtime(true) - $start_time).
                  " seconds.";


                $updated = TRUE;
            }
            else {
          echo '<p>Update ready. <a href="?doUpdate=true">&raquo; Install Now?</a></p>';
        }
      }else{
        echo '<p>Update ready. <a href="?doUpdate=true">&raquo; Install Now?</a></p>';
      }
        }
    }


    if (isset($updated) && $updated == true)
    {
        echo '<p class="success">&raquo; CMS Updated to v'.$aV.'</p>';
    }
    else if ($found != true) echo '<p>&raquo; No update is available.</p>';


}
else echo '<p>Could not find latest realeases.</p>';

current-release-versions.php

1.00
1.02

Answer 1

The biggest problem I see with this code is the extreme level at which you intermingle your browser output with your PHP logic. Developing in this way is going to lead to a codebase that is nearly impossible to maintain. The basic structure of even teh simplest PHP page should be:

<PHP logic>
<View Output>

Where <View Output> might have some branching or variable injection via PHP, but that is it (ideally you eventualyl get comfortable with working with templating systems that allow you to fully separate the logic from the view).

---

You need to think about inverting your happy path vs. error path. Take for example your main conditional.

Instead of:

if ($getVersions != '') {
    // nested code
} else {
    // error output
}

Strive for:

if(empty($getVerions)) {
    // error. exit script.
}

// rest of code, now without nesting

This will help you form getting into a rat's nested of nested code that is extremely difficult to read. This also aligns with my first comment about doing PHP logic up front and the rendering to browser. To exapnd on that:

<PHP Logic>
    <Error handling => perhaps redirect to error page, set 500 series header, etc. Exit script)
<Happy Path output of view>

Live by the mantra - "Fail early. Fail loudly".

---

Stylistically, this code is a bit of a mess.

• The indentation is inconsistent, making it hard to read.
• You are mixing snake_case and camelCase in your user-defined variables. Be consistent.

---

$localVersion = file_get_contents('current-release-versions.php');

What is the intent here? Do you intend to get results from an HTTP request against this endpoint? If this is a file that you have on your system that this script needs, you should require or include it.

If the intent of this portion of code is to determine the current running version of the application, why is there a need to retrieve this here? This should likely be part of your app configuration.

---

ini_set('max_execution_time',60);

INI settings changing and similar configurations should typically happen at the very beginning of any script, not after you are part way through the logic, unless there is some reason you need to change this value dynamically based on some decision in code.

---

if(isset($_GET['doUpdate']) && !empty($_GET['doUpdate']))

This code is redundant just !empty($_GET['doUpdate']) would suffice. If the value is not set, it will show as empty.

---

if($_GET['doUpdate'] == true) {

Get out of the habit of using loose comparisons, especially on a decision as critical as whether to perform an update or not. One could pass any of the following values to trigger this code path:

true (boolean) not applicable to a value derived from $_GET
1 (integer) not applicable to $_GET
-1 (integer) not applicable to $_GET
"1" (string)
"-1" (string)
"php" (string) any non-zero length string

I am guessing this is not your intent.

Loose comparisons make your code fragile and should only be used in very limited cases (ideally with comment in code as to why loose comparison makes sense in that case).

---

$versionList = explode("\n", $getVersions);

If you are going to work with an exploded version of the file contents, when not just use file() from the get go, so 'explosion' is already done for you? Even as is, you would be likely be better off using PHP_EOL constant here than \n.

---

if ($aV > $localVersion) {

Here, $localVersion would hold a string like 1.00\n1.02. This comparison seems broken.

---

Do you control yousite.com? I am very concerned that you just trust this input and perform file system operations based on it without validating the contents at all. This is a significant security vulnerability.

---

I would be VERY concerned with performing any operation that can change data in your application via GET. Typically POST is used for these sort of operations, such that people don't accidentally bookmark the URL and keep returning to it, potentially performing unwanted changes in your application. Get in the habit of thinking as GET as a read-only protocol.

---

You don't seem very consistent in working with file paths. Most of this stuff is using relative file paths, which is likely to lead to a fragile application and/or lead to you putting files in a web directory that don't really belong there.

---

Your code is very happy path. You just assume many of your file retrieval, directory creation, file writing, etc. operations work. This makes this code extremely fragile.

---

mkdir ($thisFileDir, 0777, true);

You REALLY need to think twice before programmatically setting directory permissions to 0777. Are you doing this because you don't understand how directory permissions work?

---

                        if ( $thisFileName == 'upgrade.php' )
                        {
                            $upgradeExec = fopen ('upgrade.php','w');
                            fwrite($upgradeExec, $contents);
                            fclose($upgradeExec);
                            include ('upgrade.php');
                            unlink('upgrade.php');
                            echo' EXECUTED</li>';
                        }

This section of code looks REALLY problematic. You are, in essence, running eval() against some arbitrary input file. Read documentation on eval() and why it should be avoided at all costs.

---

Don't have functions declared arbitrarily in the middle of your code unless their might be some conditional declaration of the function (that doesn't seem to be the case here).

---

It is not really clear to me what this code is doing. It looks like this might be some mechanism to update the running application on your site. If this is the case, I would suggest you really rethink this whole thing, as there are any number of better ways to deploy code changes to an application. None of them involve trying to swap out files on the fly like this. It is clear that even the zip file artifact in this case has been poorly constructed based on the fact that you have to do clean up on unneeded, OS-specific directories and such. Don't operate using 10-15 year old ways of working with websites. This ground has been covered innumerable times before.