File exists in FileTransferFTPExtension->createDirectoryJailed()

Uhhh, WTF? Why is the update manager trying to create a directory "sites/all/themes/corolla/images/.."?

That said, I can't reproduce this. I was able to install http://ftp.drupal.org/files/projects/corolla-7.x-1.17.tar.gz via the UI just fine.

Please provide more details on:
- The OS of your webserver
- Version of PHP you're using
- Exactly what file you're trying to install
...

I'm also having this problem using the 7.x-dev build when trying to install a module:

Failed: File Transfer failed, reason: Cannot create directory /var/www/sites/all/modules/pathauto/..

I'm also having this problem using the 7.x-beta2 when trying to install or upgrade any module.
I'm using CentOS 5.5 and PHP 5.3.3.

Error message (date.module upgrade):

Failed: Error installing / updating
Failed: File Transfer failed, reason: Cannot create directory /var/www/drupal-7.0-beta2/sites/all/modules/date/date/tests/.

Message in the log:
Warning: ftp_mkdir(): Create directory operation failed. in FileTransferFTPExtension->createDirectoryJailed() (line 61 of /var/www/drupal-7.0-beta2/includes/filetransfer/ftp.inc).

It seems that the update manager is trying to create a directory "xxx/xxx/xxx/." and it fails.

My first inclination was that this is a permissions problem, but in my tests simulating that I got an error message like
File Transfer failed, reason: Cannot create directory /Applications/MAMP/htdocs/drupal/sites/all/themes/pixture_reloaded
which you'll notice does not have the /. on the end which is characteristic of the errors posted here.

Tracing through function install(...) in includes/updater.inc:

• We must be successfully completing prepareInstallDirectory since in each bug report the error was on a subdirectory of the project.
• There error is then in $filetransfer->copyDirectory(...) call. Which then calls copyDirectoryJailed(...).

The difference stems from the behavior of this code:

foreach (new RecursiveIteratorIterator(new RecursiveDirectoryIterator($source), RecursiveIteratorIterator::SELF_FIRST) as $filename => $file) {
      $relative_path = substr($filename, strlen($source));
      ...
}

on the different platforms.

I've attached two text files which show the different behavior on MAMP and on Ubuntu 10.04.1 Server. In each case I added to the foreach loop above just an echo $relative_path statement. Notice that the MAMP installation returns only actual files and directories, whereas the Ubuntu installation returns '.' and '..' files as well.

Here's a patch which addresses the problem described in #6 by verifying that basename($relative_path) is not '.' or '..' before it tries to create the directory.

Can perhaps one of the earlier commenters test out this patch and let us know if it fixes the problem?

I'm not convinced that's the best place for this check. Seems safter to just check directly inside createDirectory() itself, so we're not nailed by any other code paths that might be trying to call createDirectory('.') or something.

I haven't tested this (and there's basically no simpletest coverage of the FileTransfer classes) so someone should really verify that this works. Don't just trust a green report from the bot.

Thanks,
-Derek

Derek: I debated about the best place for this too. I ended up settling on the location in #7 (i.e., in copyDirectoryJailed) because of the diversity of results that come from the RecursiveDirectoryIterator as described in #6.

Functionality wise, the difference between #7 and #9 comes down to the desired behavior of createDirectory. Should it ignore the input if attempting to make a '.' or '..' directory, or should it (possibly) throw an error depending on the whims of the implementation of createDirectoryJailed.

Regardless of the final decision --- #9 is fine with me --- I think we should include a brief comment outlining how '.' and '..' directories are treated.

Also, @int posted about the problem in #2. Perhaps the reply in #8 could be clarified to mention describe exactly how the patch was tested.

This patch checks to see that basename($directory) is not . or .. in both createDirectory() and removeDirectory() before proceeding with creating or removing a directory.

Additionally the documentation for the functions have been updated to reflect this:

--- includes/filetransfer/filetransfer.inc
+++ includes/filetransfer/filetransfer.inc
@@ -94,25 +94,37 @@ abstract class FileTransfer {
   /**
    * Creates a directory.
    *
+   * Creating a directory named '.' or '..' is not allowed and will be ignored.
+   *
    * @param $directory
    *   The directory to be created.
    */

and a similar statement for the function removeDirectory.

I just tried the latest patch (the one that applies the fix to CreateDirectory and RemoveDirectory) and got the same error as before (see attached). I checked that the changes had been made to the installed version of filetransfer.inc, but just in case this is a newbie mistake, do I need to do anything other than a drush cc to make sure it is using the new version?

I think we need to figure out why FilesystemIterator::SKIP_DOTS isn't being set as a flag for these directory iterators in some situations / platforms.

How about this solution? Let's use FilesystemIterator::SKIP_DOTS to avoid ever even trying to create or remove . or .. directories.

I was wondering if there was something like SKIP_DOTS. ;) I think #17 is the better way to go, although wow, what an ugly line of code to get a working directory iterator in PHP. ;)

  foreach (new RecursiveIteratorIterator(new RecursiveDirectoryIterator($directory, FilesystemIterator::SKIP_DOTS), RecursiveIteratorIterator::CHILD_FIRST) as $filename => $file) {

Maybe it's worth adding a helper function that puts most of this into a single function so that future changes can be done in one place, and the call sites can be more readable. Something like:

  $directory_iterator = $this->getDirectoryIterator($directory);
  foreach ($directory_iterator as $filename => $file) {

Just a thought...

@dww in #19: The thought is reasonable to create a helper function, however at best you could create a helper function for the duplicitous interior
new RecursiveDirectoryIterator($directory, FilesystemIterator::SKIP_DOTS), as the mode for the RecursiveIteratorIterator class are not shared between every instance. (Some use SELF_FIRST, others CHILD_FIRST).

So you could create something like

/**
 * Creates a local directory iterator.
 *
 * @param $directory
 *   The local path to a directory.
 * @param $mode
 *   The mode of operation for RecursiveIteratorIterator.  Options are
 *   LEAVES_ONLY (default), SELF_FIRST and CHILD_FIRST.
 *
 * @return
 *   A recursive iterator for all files and subdirectories of the given
 *   local $directory.  The special '.' and '..' directories are skipped.
 */
function getLocalDirectoryIterator($directory, $mode = RecursiveIteratorIterator::LEAVES_ONLY) {
  return new RecursiveIteratorIterator(new RecursiveDirectoryIterator($directory, FilesystemIterator::SKIP_DOTS), $mode);
}

And then you could call it as

 $directory_iterator = $this->getLocalDirectoryIterator($directory, RecursiveIteratorIterator::CHILD_FIRST);
  foreach ($directory_iterator as $filename => $file) {
...

but this is hardly an improvement.

I think we should just stick with #17.

Well, we don't need "Local" in that function name. Also, if the default mode is SELF_FIRST we only need to specify the final arg for one call site. So, I think a helper could still be an improvement.

That said, #17 certainly fixes a major bug (I haven't tested, and neither has the bot, but #18 by int says it works), so it should probably just get committed so we can focus on more serious problems like real bugs instead of the legibility of the code in this particular case.

I agree with @dww. Let's just commit #17 and move on to other bugs.

Happy to confirm that #17 fixed my problem.

Committed to CVS HEAD. Thanks.

This has apparently caused #979340: Class 'FilesystemIterator' not found when installing module from Update Manager. On second thought I'll close that one as a dup and just re-open this.

To summarize that issue: Installing a module from a url causes this error:

An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: http://d7.kswantech.com/authorize.php?batch=1&id=10&op=do StatusText: OK ResponseText: Fatal error: Class 'FilesystemIterator' not found in /srv/www/d7/includes/filetransfer/filetransfer.inc on line 211

Ugh. Yeah, needs work indeed.

According to http://www.php.net/manual/en/class.filesystemiterator.php SKIP_DOTS is included in the default flags whenever you instantiate a FilesystemIterator. RecursiveDirectoryIterator just extends FilesystemIterator, so I have no idea why #17 was needed in the first place. This appears to be totally platform dependent or something. Thanks, PHP! What a pain... :(

As noted in #979340: Class 'FilesystemIterator' not found when installing module from Update Manager, the error occurs on:
OS: Ubuntu 8.04
PHP 5.2.4-2ubuntu5.12

According to http://php.net/ChangeLog-5.php, it looks like FilesystemIterator was added in PHP 5.3.0. That would explain why it works for int on PHP 5.3.3-1ubuntu9.1 and doesn't work for me on PHP 5.2.4-2ubuntu5.12.

I think changing FilesystemIterator::SKIP_DOTS to RecursiveDirectoryIterator::SKIP_DOTS should work fine, as the change introduced in 5.3 was the name of the parent class of RecursiveDirectoryIterator. (And the parent class is what defines the constant SKIP_DOTS).

Sorry for the issue caused by the earlier patch.

Except #13 only handles 2 of the 3 possible directory traversals in update manager. We should probably prevent chmod() from working on . and .. too.

I can't believe how hard PHP makes this to get right. *sigh* Not sure which of the contradictory docs to trust. The changelog makes it look like FilesystemIterator was added in 5.3.0. However, http://www.php.net/manual/en/class.filesystemiterator.php has its own changelog that says: "5.2.11: Added FilesystemIterator::FOLLOW_SYMLINKS". I guess that's a doc bug.

I'll try to roll a patch in the next few minutes, stay tuned.

x-post, but now that I'm looking closely at this stuff, #31 still isn't going to handle chmod() properly in ftp and ssh in cases where the platform or PHP version conspire to mess with us. I can't believe I have to write this (PHP--) but I think manually implementing the equivalent of SKIP_DOTS ourselves in the parent class is the safest approach to defend us from platform-specific behavior. Argh. @bfroehle: if you want to roll a new patch, that'd be great -- I have 100 other things I need to be doing in the next few hours.

For now I've rolled back #17 (bold for scanning purposes, not because I'm yelling :D) while we get this figured out.

Downgrading from critical.

Here is #31 re-rolled against HEAD (due to webchick's rolling back of #17 in the previous post).

@dww: I'm not sure what you mean in #33 --- specifically with respect to chmod not being correct in the ftp and ssh connection classes. This patch gives proper behavior for the local connection class.

I think it should be up to each implementation to ensure proper behavior with respect to '.' and '..' directories, as not all implementations are going to work the same. For example, ssh's recursive chmod works by running chmod -R ... whereas ftp's recursive chmod actually recurses the directory tree manually.

Since this blew up a bunch of things last time, I'd like some reassurance on what "Works for me" means. What specific things did you test?

webchick's point (which I support) is that we (at least) need someone with PHP 5.2 and PHP 5.3 to test this patch before it's RTBC. @int: you're still at 5.3.3, right? Maybe kswan is willing to post results from 5.2?

Marked #981708: update.php: Class 'FilesystemIterator' not found as a duplicate of this one.

I can confirm that #36 does NOT work in PHP 5.2.13.

The error is the same: Undefined class constant 'SKIP_DOTS' in /Applications/MAMP/htdocs/drupal/includes/filetransfer/filetransfer.inc on line 211

#13 doesn't deal with '.' or '..' in the chmodJailed function. That would still need to be addressed.

Here I've created a class SkipDotsIterator which is an implementation of FilterIterator that skips '.' and '..' directories. The foreach statements are then modified like:

-    foreach (new RecursiveIteratorIterator(new RecursiveDirectoryIterator($directory), RecursiveIteratorIterator::CHILD_FIRST) as $filename => $file) {
+    foreach (new SkipDotsIterator(new RecursiveIteratorIterator(new RecursiveDirectoryIterator($directory), RecursiveIteratorIterator::CHILD_FIRST)) as $filename => $file) {

Tested to work on MAMP running PHP 5.2.13 and 5.3.2. I've also ran the relevant code sections outside of a Drupal install on on Ubuntu running PHP 5.3.2 to verify that it actually does skip dot files.

This needs review on actual Drupal installs on both php 5.2 and 5.3 on a variety of platforms.

#45: 935036-45-Skip-.-and-.-directories-in-Recursive-D.patch queued for re-testing.

I tested #45 on Ubuntu 8.04 (PHP 5.2.4-2ubuntu5.12). I was able to successfully install several modules using both "Install from a URL" and "Upload a module". Of course this was all with the local file transfer (not using FTP or SSH).

I also tested #36 on the same server and got the error message "An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: http://d7.kswantech.com/authorize.php?batch=1&id=24&op=do StatusText: OK ResponseText: Fatal error: Undefined class constant 'SKIP_DOTS' in /srv/www/d7/includes/filetransfer/filetransfer.inc on line 211"

So, #45 looks good and #36 is still bad on PHP 5.2.

#45 seems okay, but it sure makes me want to stab PHP in the eyes with something red hot and sharp.

Oh, wow. :( I second dww's stabbing. :(

Just a question. Could we simply do the "." vs. ".." check manually in abstract class FileTransfer (or subclass RecursiveIteratorIterator to do it)? Where we have $file->isDir() and such? That would make the code so incredibly easier to read...

Ok, dww explained in IRC that #13 is closer to what I mean, and that looks great. I understand there are a few other places we need to put that check, though. Hopefully dww can chime in about that.

I know this isn't the direction @webchick thinks we should go, but what if we combine #45 with @dww's suggestion in #19 of creating a helper function to return the iterator.

Since fundamentally the problem is PHP's handling of '.' and '..' directories in RecursiveDiretoryIterator, I think the band-aid should go on near there --- not off in random if($basename == '.' || $basename == '..') statements littered throughout the class.

My last comment got me thinking -- since the problem, fundamentally, is RecursiveDirectoryIterator we could just band-aid that directly:

/**
 * Provides an interface for iterating recursively over filesystem directories.
 *
 * Manually skips '.' and '..' directories, since no existing method is
 * available in PHP 5.2.
 *
 * @todo Depreciate in favor of RecursiveDirectoryIterator::SKIP_DOTS once PHP
 *   5.3 or later is required.
 */
class SkipDotsRecursiveDirectoryIterator extends RecursiveDirectoryIterator {
  /**
   * Constructs a SkipDotsRecursiveDirectoryIterator
   *
   * @param $path
   *   The path of the directory to be iterated over.
   */
  function __construct($path) {
    parent::__construct($path);
  }

  function next() {
    parent::next();
    while ($this->isDot()) {
      parent::next();
    }
  }
}

That actually seems much more palatable to me.

@webchick: Can you be more specific about which post you are referring to?

@int in #55: That won't work, since in PHP 5.2, the '.' and '..' directories will still appear but no mechanism would be in place to filter them out.

Sorry. Was referring to #54. It doesn't make the chain of things completely impossible to read, and does what we actually mean to do, which is extend RecursiveDirectoryIterator so that it doesn't suck monkey nuts.

Setting to needs review, since #54 has some support and needs to be tested.

I tested #54 on Ubuntu 8.04 (PHP 5.2.4-2ubuntu5.12). I was able to successfully install several modules using both "Install from a URL" and "Upload a module".

+1 RTBC. The update manager was unusable for me until this patch - now it works great with several modules.

PHP 5.2.11 for me.

Committed to HEAD. Thanks!

I checked the 2 files that the patch 54 was meant to fix and I already had the lines to be added in my files.

I had to manually set 3 directory permissions to 777: (775 did not work)

/modules
/includes/modules
/sites/all/modules

in order to install a module.

I am using the latest Drupal 7.8 download on a system with
Debian 6 squeeze, MySQL 5.1.49, PHP 5.3.2 and ISPConfig 3 - all up to date

1.) Is it a big security risk leaving these 3 directory permissions to 777?
2.) Does this problem indicate some other problem? IF so, what?

Thanks.

Eddie: You should really post your question in the forums.