Cannot extract themes and modules

Or is this simply just an issue with Windows/XAMPP and TAR.GZ files?

Doesn't seem to be a local/ Win32 problem, i have it also on an ubuntu webserver.
Todays Drupal 7 Version .

I, too, get the same behavior running on latest stable Ubuntu with Drupal 7.0-alpha4.

I first tried to get them from the http://ftp.drupal.org/files/projects/name.tar.gz as indicated. But drupal gave me a form where I was only allowed to select "FTP" as the protocol and no login I could think of would work. I would have assumed the "http://..." would have triggered a security-free http session. Perhaps the "...ftp.drupal.org.." part triggered some other piece of logic.

I then downloaded them via Firefox on Windows7 to a ubuntu smb share and used the browse tool. I got the message indicated in this original post, "Cannot extract temporary://devel-7.x-1.0-beta2.tar_.gz, not a valid archive." But it wasn't just the devel module; it was anything else I tried, too.

I wish I could be more help. This is all I've got.

I'm having exactly the same problem. Do you find out how to resolve this?

Same here... Have it installed locally on my mac and can't install modules or themes. I'm a total newbie though, I have about 20 minutes of experience with drupal so I'm no help, but would like to know if anyone has a solution.

No this isn't broken by #693084: Regression: file_munge_filename() extension handling broken by move to File Field. In fact that patch had to go in first before this could be fixed. I didn't include a fix in that patch because I felt it was a different issue (even tho originally the issue I referenced had to do with exactly this problem, it had changed over time into a separate security issue). I was going to open a critical and include a fix but had forgotten about it, so thanks for upgrading this.

Anyway, the fix is trivial, it simply needs a validator for the supported archive extensions. There is even a @todo in the code that says it needs one.

sorry cross posted.

We need to include all available archives supported by the module installer though =P There is already code in Drupal that fetches them, you can use that code to build the list for the validator.

Can someone explain to me (or give me a link explaining) how I install this patch? I tried modifying the code in /update/update.manager.inc, but that didn't work. I'm guessing I need to download the patch from somewhere? I'm still learning all this, so just point a finger in the right direction...

Thanks again!

Here is a quite well to read handbook page: http://drupal.org/patch/apply

Just fixing the component so this gets in front of the right people.

+++ modules/update/update.manager.inc	6 Jul 2010 12:11:42 -0000
@@ -539,8 +539,10 @@ function update_manager_install_form_sub
+      'file_validate_extensions' => array('zip tar tgz gz bz2'),    ¶

Trailing white-space here.

Looks RTBC to me afterwards.

43 critical left. Go review some!

If bot is green we can commit this nasty CRITICAL (yeah! one less) bug!

Two questions:

1. Would it make sense to move these extensions into a variable, so they could be configured on a per-site basis? (It might not make sense; just asking.)
2. Is there any possible way to write tests for this?

Re #19: it doesn't look like that list occurs a lot.

@ #19

1. yes.
archiver_get_info() returns a list of extensions supported by the user's Drupal site. It's just that the format returned requires some additional code to extract (the extensions). However Update Manager's installer will let you know when it attempts to extract an unsupported archive.

With this patch we're basically doing double archive validation. We're using an extension validator to ensure the file is an archive, and Update Manager has its own archive validation which does more than just check its extension. Are these two checks necessary considering they're doing the same thing?

The most simple fix for this is to let Update Manager decide whether to accept the file, just as it does for modules installed via URL. To do this you'd simply set 'file_validate_extensions' to an empty array which effectively turns off the extension validator for the file upload. The file then gets passed off to the archive handling functions where it's checked for a valid and supported archive.

2. yes.
justinrandell said on IRC he reallly wanted to write a test for this.

Ahh, so #693084: Regression: file_munge_filename() extension handling broken by move to File Field finally landed, yay. We certainly need a followup patch to actually fix update manager now that that's done, so this might as well be it. ;)

The basic idea is right. We want our own file extension validators here.

However, hard-coding the list is wrong.

archiver_get_info() already provides a mechanism for any Archiver classes to declare their supported extensions. And we already use this in the UI properly. See update_manager_install_form() for example.

So, if we want to keep allowing contrib to extend core here and provide other ArchiverInterface classes to handle other kinds of archives (i.e. bz2 isn't actually supported by core, yet), then we need to use that same mechanism to get the list of allowed extensions.

Given that there are now at least two places that need to construct this array of supported archive extensions, we should probably make a shared API function for it.

Otherwise, this is good to go.

Sorry, I totally missed comment #21 when I was asked to review this patch. I think it's fine to keep both validation checks in there. Better safe than sorry. Granted, once you have permission to upload code to your site via update manager, you can presumably be trusted. ;) But, I'd hate to see some crazy exploit that becomes possible because you can upload any file you want, even if update manager doesn't end up installing it for you.

#27 is a step closer, thanks. But now update_manager_install_form() should be modified to use update_manager_get_valid_archive_extensions() or we have duplicated code and a "shared" helper function that's only called in one place. ;)

Also, to me it seems like this helper isn't specific to update manager. It should probably live in includes/common.inc next to archiver_get_info(), something like archiver_get_valid_extensions(). Maybe not, maybe it can just live in update.manager.inc. But, if anyone is using the Archiver class, I don't see why they need to enable update.module to get access to this helper, which has nothing directly to do with update manager itself.

Thanks!
-Derek

Only a small difference is that update_manager_install_form uses the list separated by commas instead of spaces, so the implode function should be taken out of the helper function.

Also, there are still problems with file_munge_filename for multipart extensions (like tar.gz), but these don't occur as long as each part except the last one is allowed separately (so in this case it works because tar is allowed by itself). Working on this in #854498: file_munge_filename broken for multipart extensions.

Also, we need to check that return value of file_save_upload is in fact a file object, and not FALSE because of an error. I think we can just return in this case, because file_save_upload has already called drupal_set_message when we get FALSE back.

#854498: file_munge_filename broken for multipart extensions is a duplicate of this issue ( and its new title is misleading ).

Also, there are still problems with file_munge_filename for multipart extensions (like tar.gz), but these don't occur as long as each part except the last one is allowed separately (so in this case it works because tar is allowed by itself).

That's by design since Drupal 6 (not a bug).

As #25 pointed out:

You'll need to put the two archives in 'modules/update/tests' as aaa_update_test.tar.gz and ddd_update_test.tar.gz.

I re-upped the files and renamed new_test.tar.gz to ddd_update_test.tar.gz. There are a bunch of test files for the update module so adding these shouldn't be an issue.

Per dww: added archiver_get_extensions() to common.inc right next to archiver_get_info().

Fixed a test and changed some wording. The test also ensures all of the correct archive extensions are being allowed.

Per Stevel: now handles the case where file_save_upload() returns FALSE.

As for

Only a small difference is that update_manager_install_form uses the list separated by commas instead of spaces, so the implode function should be taken out of the helper function.

Space delimited strings are used elsewhere in Drupal (to show allowed extensions, etc), so it shouldn't be using commas anyway if we're going to be consistent.

+++ includes/common.inc
@@ -6773,6 +6773,27 @@ function archiver_get_info() {
+ * ¶

Trailing whitespace

+++ modules/update/update.manager.inc
@@ -538,10 +530,14 @@ function update_manager_install_form_submit($form, &$form_state) {
+    // @todo: find out if the module is already installed, if so, throw an error.

I think this todo is already done at line 580:

if ($updater->isInstalled()) {
  form_set_error($field, t('%project is already installed.', array('%project' => $project_title)));
     return;
   }

so the todo can be removed.

+++ modules/update/update.test
@@ -493,3 +493,49 @@ class UpdateTestContribCase extends UpdateTestHelper {
+      'description' => 'Tests the update module's ability to install new modules.',

Single quote inside a string enclosed by single quotes. Almost certain this is a bad idea :)

+++ modules/update/update.test
@@ -493,3 +493,49 @@ class UpdateTestContribCase extends UpdateTestHelper {
+  public function setup() {

Function name should be setUp() (capitalized U)

+++ modules/update/update.test
@@ -493,3 +493,49 @@ class UpdateTestContribCase extends UpdateTestHelper {
+    $this->assertText(t('To continue, provide your server connection details'), 'Authorize.php prompts for security credentials');

authorize.php does not ask for credentials when the user running php is the same as the owner of the drupal files (quite a common configuration I think), so It's probably not a good idea to check for this message being present.

Powered by Dreditor.

Thanks for reviewing. You're right on all counts.

New patch to deal with #33. Tests now only check if a module can be uploaded and extracted, which is what this issue is about.

In #32, only aaa_update_test.tar.gz needs to be added to Drupal. See that comment for where to place it.

Looks good from a human eye. Local testing is still running the test-suite, but I don't expect any problems to occur. Still, I'll wait for the tests to pass before marking this as RTBC.

but the patch depends on a binary archive file that hasn't been included into Drupal yet =P

The failed test works fine when modules/update/tests/aaa_update_test.tar.gz from #32 exists.

So RTBC for
- aaa_update_test.tar.gz from #32 going in modules/update/tests
- Patch from #35

Thanks Stevel, I was going to ask to see if you'd RTBC this.

Patch still applies.

Looks good. Committed to CVS HEAD.