Subdirectory in "update-extraction" is not deleted before extraction of other versions

+++ modules/update/update.manager.inc	20 Nov 2010 16:35:44 -0000
@@ -654,6 +654,15 @@
+  // Remove folder if exists or we may run into a mix of previous and new files.

folder -> directory.
Also I don't think we need to explain our recommended best practice here.
Maybe just:
"Remove the directory if it exists."

Otherwise looks good, although I haven't looked at the surrounding code.

Powered by Dreditor.

Well, I don't really care. This is a critical so let's get some more reviews on the actual code.

The code comment is not really clear. Is it proper English to begin with?

Try:

Remove the folder if it exists, otherwise it will contain a mixture of old outdated files mixed with the shiny new files.

#3: 977408_update-extraction_bug-D7.patch queued for re-testing.

Hmm... issue thread already has a status of needs review... Testbot just needs to wake up?

See #973432: D6 patches named -D6 are not tested for what the file naming convention was changed. See also the help text on the file attachment form element. ;)

Re-rolled the patch to fix the comment (wrap to 80 chars, more clear text).

I was a little worried about removing the directory like this since someone could do a lot of damage by calling update_manager_archive_extract() with the wrong arguments. In the FileTransfer classes, we have a bunch of code to enforce that operations are only happening inside a "jail", so you don't accidentally go off removing files from other parts of the filesystem that you shouldn't be touching. However, all of this code is happening in the Update manager before we reach authorize.php (so we don't have a FileTransfer class at all). Furthermore, this is just the local temp directory where we extract and verify the archive, not when we're touching the live code. Finally, we're about to clobber the directory by extracting the archive, this is just a cleaner way to do it without any surprises. I also audited the code to see all the call sites, and we're definitely invoking this function properly.

This is definitely a security hardening patch, since if someone can trick the web server to write malicious files into this extraction location (which the OS permissions will allow), those malicious files would be installed the next time Update manager was trying to update/install a module. This way, we ensure that the only files in the extraction directory are those that were in the archive. Of course, there's still a race condition and a window of time when the extracted files are still sitting there after they've been verified and before they've been installed, but at least with this patch, that window is a lot smaller.

I haven't re-tested with this patch, but I wanted to post it now to get the bot started on it. Not that we have any test coverage for these code paths. :( So, the bot isn't going to tell us much. This really needs thorough manual testing before it's truly RTBC.

Oh, and by the definitions in #974066: Friendly reminder: on critical issues and RCs and elsewhere, this isn't critical. It's still a major security hardening patch, but it doesn't impact the string freeze and could be fixed after an RC. I'd rather see it fixed now, but we shouldn't abuse the critical status.

Note: I manually tested this and it works as expected. I'd be willing to call this RTBC, but since technically the last patch was mine, I can't really do that. Hopefully once the bot confirms the patch applies cleanly (about all it's going to be able to tell us), someone should give this a final look and RTBC.

I finally have a break in the storm of seasonal work. I'll be online tonight to test everything I can and will make this my first target.

Pre-test:
updated to fresh cvs version and checked to see if I needed to do any db updates.

Test #1: Before patch
* checked to see if I had any updates to install for any of the modules I had previous installed : found 2
- Drupal Core (obviously)
- Zen Theme

* Checked the option to install the Zen Theme
* Update downloaded successfully
* Chose option to install the update in maintenance mode.
* Entered proper credentials to update via ftp. Submitted form
* Received HTTP Error 324 (ERR_EMPTY_RESPONSE) from chrome (as usual)
* checked the update-cache and update-extraction folders. Found that there was no zen folder in either.

Test #2; After patch
* Retried update for Zen theme
* followed the same steps as above
* Received HTTP error 324 (ERR_EMPTY_RESPONSE) from chrome (no change)
* check the update-cache and update-extraction folders (this time before and after the upgrade event) there was no zen folder created at either time.

OK, I'll test that today.

Tested #17 and it is working as hass described in #22.

Xubuntu 10.10
Apache 2.2.16
PHP 5.3.3
MySQL 5.1.49

Steps used to test:

1. Checked out HEAD from CVS.
2. Set up new database on MySQL 5.1.49.
3. Installed Drupal.
4. Applied patch in #17 successfully.
5. Installed Devel module beta-2 by using Install from a URL.
6. Put a file with the name "junkFile.txt" into the folder "devel" inside the "update-extraction" folder (see Screengrab 1.)
7. Enabled Devel module.
8. Ran cron.
9. Checked for updates.
10. Updated Devel from beta-2 to rc-1 through update manager.
    The file "junkFile.txt" is not in the "devel" folder inside the "update-extraction" folder (see Screengrab 2.)
    The file "junkFile.txt" is not in the "devel" folder inside the "sites/all/modules" folder (see Screengrab 2.)

I'm not setting the status to RTBC because of dww's comment in #17:

This really needs thorough manual testing before it's truly RTBC.

Maybe the code needs a review too? I can't do this.

WTF? This is alrdy fixed, I'm prety sure cause I saw that piece of code in my zip debug sprint...

EDIT
----

function update_manager_archive_extract($file, $directory) {
  $archiver = archiver_get_archiver($file);
  if (!$archiver) {
    throw new Exception(t('Cannot extract %file, not a valid archive.', array ('%file' => $file)));
  }

  // Remove the directory if it exists, otherwise it might contain a mixture of
  // old files mixed with the new files (e.g. in cases where files were removed
  // from a later release).
  $files = $archiver->listContents();
  // Unfortunately, we can only use the directory name for this. :(
  $project = drupal_substr($files[0], 0, -1);
  $extract_location = $directory . '/' . $project;
  if (file_exists($extract_location)) {
    file_unmanaged_delete_recursive($extract_location);
  }

  $archiver->extract($directory, $files);
  
  return $archiver;
} 

fixed?

fixed!

You are right. It is fixed. I checked out HEAD from CVS and checked the file manually. The code in update.manager.inc is the same as in the patch in #17. Thanks for catching this.

Tee heeeeeeeee.... ;) WHOOPS!

I accidentally included this fix in my final patch for #609772-50: Impossible to extend the FileTransfer class system in contrib, which no one noticed, and webchick happily committed it.

I'll check with her in IRC right now if she wants to do anything about this. I vote for leaving it fixed instead of re-breaking it and then fixing it again.