When we tell the FileTransfer classes to operate on files or directories, we pass in the absolute path via DRUPAL_ROOT. Of course, some FTP servers are setup chroot. This causes it to fail.

We should have a function which can be called in each FileTransfer class (and an abstract one which does nothing) that will find out if there is a chroot, and then automatically accommodate it in future calls to that object. This belongs in a factory implementation because the class should be usable without this check, but is desired in 90% of cases.

CommentFileSizeAuthor
#44 filetransfer_chroot_other_fixes-528326-44.patch13.96 KBcwgordon7
#42 filetransfer_chroot_other_fixes-528326-42.patch14.39 KBJacobSingh
#40 filetransfer_chroot_other_fixes-528326-40.patch13.95 KBJacobSingh
#37 filetransfer.patch16.97 KBadrian
#34 filetransfer_chroot_other_fixes-528326-34.patch16.58 KBJacobSingh
#28 filetransfer_chroot_other_fixes-528326-28.patch16.65 KBJacobSingh
#27 filetransfer_chroot_other_fixes-528326-27.patch16.64 KBJacobSingh
#25 filetransfer_chroot_other_fixes-528326-25.patch12.92 KByched
#22 filetransfer_chroot_other_fixes-528326-16.patch13.26 KBJacobSingh
#20 filetransfer_chroot_other_fixes-528326-16.patch13.26 KBJacobSingh
#16 filetransfer_chroot_other_fixes-528326-16_.patch12.66 KBJacobSingh
#9 filetransfer_chroot_other_fixes-528326-8_.patch12.66 KBJacobSingh
#2 filetransfer_chroot_other_fixes-528326-1_.patch12.5 KBJacobSingh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented
Status: Active » Needs review

This patch handles it. It was a royal PITA.

Ftp w/ wrappers was also totally broken, looks like someone copy-pasted from the extension one and it needed a bit of cleanup. So I had to do some work there.

Now if you are using a chrooted FTP host, it should "just work".

I think this also solves #524404: FileTransfer classes need to support windows paths being thrown at them
(but untested)

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented
FileSize
filetransfer_chroot_other_fixes-528326-1_.patch12.5 KB

It's even better when the patch is attached.

Status: Needs review » Needs work

The last submitted patch failed testing.

cwgordon7’s picture

cwgordon7 CreditAttribution: cwgordon7 commented

Looks good, however:

- Fails to run tests(?)
- Comments need to begin with a space and a capital letter and end with a period.
- "+ " - no trailing white space.
- I sometimes have Drupal installations as subfolders of another Drupal installation. (E.g. cwgordon.com vs. cwgordon.com/foo/ or foo.cwgordon.com). I believe that under the current code the Drupal in the parent folder will always be selected? Perhaps the way to approach this is to create a file with a random name in the files directory and check for that, which should be unique to each Drupal installation?

int’s picture

int CreditAttribution: int commented

For me this path work's fine.

But I think that the user should confirm that the ftp location of drupal is fine, or at least and for security, one random file with random content in the tmp modules folder.

See here:
#395474-197: Plugin Manager in Core: Part 2 (integration with update status)
#395474-198: Plugin Manager in Core: Part 2 (integration with update status)
#395474-199: Plugin Manager in Core: Part 2 (integration with update status)

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented

I agree about the random file.

But. I think this is good enough for now, and fixes lots of other bugs, and should go in ASAP.

If someone wants to keep Drupal installs many levels deep, AND has a chrooted FTP server, we could provide a setting perhaps.

The problem with the random file is that it requires the FileTransfer classes to use Drupal APIs AFAICT to find a writable folder. Or it has to be passed the file path, which is really ugly - but still a possibility - and would require refactoring.

I'm very firm about this. If we ever want this thing to be able to update Drupal core, we should not be using any core APIs. It also is bad for encapsulation generally. Maybe there is some other way to determine a chroot? Maybe the chroot determining should be done earlier in the process from outside the class, using the class to determine it, but then storing the chroot info somewhere else - like in the variable table - and passing it in.

Here's another idea I had, but didn't implement because I didn't want to delay this patch getting out: We could check the utime of the file. This is very likely to be unique across multiple installs. All of our current transfer mechanisms support this.

Thoughts? (but not too many, because this at least is better than nothing, let's do the comment cleanup and get a commit please).

Best,
Jacob

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented

Weird, the testbot doesn't give a reason. Anyway, Charlie, can you just solve the comment / style issues and throw a new one up? If it passes, I bet we can get it committed today. Let's make a followup for a more sophisticated chroot, because I think what we've got here is better than nada, and the current code for the FTP Wrapper class is already broken.

yched’s picture

yched CreditAttribution: yched commented

FWIW, this doesn't seem to fix #395474: Plugin Manager in Core: Part 2 (integration with update status) on my windows setup.
The FTP server still receives CWD D:\... which he cannot handle.

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented
FileSize
filetransfer_chroot_other_fixes-528326-8_.patch12.66 KB

Actually, come to think of it, I think the code as it stands will be able to handle Drupal installs inside other installs. Take this example (even uses the same folder name):

Drupal install in /home/jacob/drupal another in /home/jacob/drupal/drupal
chrooted to /home/jacob.

function findChroot() {
    // If the file exists as is, there is no chroot.
    if ($this->isFile(__FILE__)) {
      return FALSE;
    }
    
    $parts = explode('/', dirname(__FILE__));
    $chroot = '';
    while (count($parts)) {
      $check = implode($parts, '/');
      if ($this->isFile($check . '/' . basename(__FILE__))) {
        // Remove the trailing slash.
        return substr($chroot,0,-1);
      }
      $chroot .= array_shift($parts) . '/';
    }
    return FALSE;
  }

if called from /home/jacob/drupal/includes/filetransfer/filetransfer.inc:

$check = implode($parts, '/');
if ($this->isFile($check . '/' . basename(__FILE__))) {

/home/jacob/drupal/includes/filetransfer/filetransfer.inc
/jacob/drupal/includes/filetransfer/filetransfer.inc
/drupal/includes/filetransfer/filetransfer.inc <- Found it - chroot set to /home/jacob

if called from /home/jacob/drupal/drupal/includes/filetransfer/filetransfer.inc:

/home/jacob/drupal/drupal/includes/filetransfer/filetransfer.inc
/jacob/drupal/drupal/includes/filetransfer/filetransfer.inc
/drupal/drupal/includes/filetransfer/filetransfer.inc <- Found it - chroot set to /home/jacob.

The fear I think, is that it would keep going and then find /drupal/includes/filetransfer/filetransfer.inc. But it doesn't because it already found the correct one.

Since all of the commands to actually move files are using absolute paths, they will reference the correct location. I actually don't see why you would need a temp file to figure it out. Maybe I'm missing something here, but I think it works.

Attached is hopefully a final patch which fixes the comments and whitespace.

int’s picture

int CreditAttribution: int commented

Ok.
But in case of symlink's ? We can have ftp symlink's and with other's names. Ex.

Folders:
/home/jacob/drupal
/home/jacob/drupal/drupal
/home/jacob/drupal2
/home/jacob/www.example.org
/home/jacob/example.org
/home/jacob/drupal7

Ftp Folders: (with ln -s [symlinks])
/ftp/site1 -> /home/jacob/drupal
/ftp/site2 -> /home/jacob/drupal/drupal
/ftp/site3 -> /home/jacob/drupal2
/ftp/site4 -> /home/jacob/www.example.org
/ftp/site5 -> /home/jacob/example.org
/ftp/drupal -> /home/jacob/drupal7

Maybe none use this way..(I don't use this way), But maybe some shared host, some bad configuration or other's config, we can have this, or other things that we don't know yet. So, that said, it would be nice to at least we can see and verified if it found the correct path.

Maybe I'm thinking to must in security, but this functionality is to deleted and create files, and we need some fallback (for situation that we don't know yet).

int’s picture

int CreditAttribution: int commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented

@int: I don't see how a problem would arise in the case you provided. Please provide a concrete example, and if possible test where it might occur.

Thanks!
Jacob

int’s picture

int CreditAttribution: int commented

if I have this: (2 sites)
/home/int/drupal/
/home/int/drupal2/drupal/

And I have ftp root here:
/ftp/
With two symlink's pointing to the two drupal folder
Like this:
/sites1/ -> /home/int/drupal/
/sites2/ -> /home/int/drupal2/drupal/

So this will work? Will detected the correct path? I think that is impossible in this case..

Maybe if the we also have this symlink will update the wrong folder??
/drupal2/ -> /home/int/drupal2/drupal/ (we are upgrading the drupal2 instalation, but the symlink drupal2 (virual folder) is pointing to /home/int/drupal2/drupal/

Maybe has no sense..
We can commit this and next think it better..

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented

Can someone find out what is up with the test bot here?

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented
FileSize
filetransfer_chroot_other_fixes-528326-16_.patch12.66 KB

Ah, I see, it's because of the test which I thought has been disabled because it ran differently on different platforms.

I added isFile to the Test class, but I can't get simpletest to run anymore... get:

Column not found: 1054 Unknown column 'last_prefix' in 'field list'

Whatever, have to deal with it later I guess. Here is a new one, maybe the test bot will like it.

int’s picture

int CreditAttribution: int commented
Status: Needs work » Needs review

for test bot.

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented

The first problem will happen. If someone has a different FTP root which is not on the same path as their install, it won't find it. There is nothing we can do in this case except provide a setting for the path in settings.php. Honestly though, this is a serious edge case.

The 2nd problem wouldn't be a problem AFAICT. It would check like this:

/home/int/drupal2/includes/..
/int/drupal2/includes..
/drupal2/includes/...

So it would catch it. I don't see how you could have a symlink which is the same name as a folder in the same directory. Maybe I'm missing the point, but as long as I can fix this problem with the test, I'll try to get this through.

Best,
J

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented
Status: Needs work » Needs review
FileSize
filetransfer_chroot_other_fixes-528326-16.patch13.26 KB

Ah, forgot to include my change... let's see if this one is better.

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented
Status: Needs work » Needs review
FileSize
filetransfer_chroot_other_fixes-528326-16.patch13.26 KB

I think the testbot is lying... please test again!

int’s picture

int CreditAttribution: int commented
Status: Needs review » Reviewed & tested by the community

It's working fine.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Needs review

- The difference between fixPath() and sanetizePath() was confusing at first. It feels like bad API design. Fortunately, fixPath is not part of the public API. Still a protected fucntion though. Given that it is Windows-specific, can't we name it something like convertWindowsToUnix() or something?

- Initially I thought that isFile() was redundant because you could do !isDirectory() but that is not valid.

- I find it strange that functions like fixPath() and sanetizePath() don't return a value. Why do this by reference?

- PHPdoc is sparse, especially in a function like isFile in filetransfer.inc.

yched’s picture

yched CreditAttribution: yched commented
FileSize
filetransfer_chroot_other_fixes-528326-25.patch12.92 KB

Here's a patch with the edits I had to make to have #395474: Plugin Manager in Core: Part 2 (integration with update status) work on my windows setup.
Changes in :
fixPath()
findChroot()
checkPath()

Those are raw fixes, and quite possibly not the best code organization, but that code is largely above my head. At least it outlines the changes needed for windows.
This patch also does not address any of Dries's remarks in #24.

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented
FileSize
filetransfer_chroot_other_fixes-528326-27.patch16.64 KB

Okay, here's another go.

This addresses some of Dries's concerns and is based on of yched's latest patch (so hopefully works for Windows).

Honestly though, what we've got now is broken - only the ftp extension class even works, so we should get this in soon or it will be a dependency for other issues.

I also slipped in new feature which we will likely use in the new plugin manager: chmod support.

I've test it in chrooted and non-chrooted environments. One silly thing here though is that the stream wrapper doesn't support chmod, so you end up having to use the extension anyway, making it kinda moot. But for now, I think this is okay, when the PM implementation firms up, we can decide to ditch this class.

Best,
Jacob

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented
FileSize
filetransfer_chroot_other_fixes-528326-28.patch16.65 KB

With a small fix for an E_NOTICE

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented
Status: Needs work » Needs review
int’s picture

int CreditAttribution: int commented
Status: Needs review » Reviewed & tested by the community
Dries’s picture

Dries CreditAttribution: Dries commented

I'd love to have yched do one last test/review of the patch.

yched’s picture

yched CreditAttribution: yched commented
Status: Reviewed & tested by the community » Needs work

Tested #28 with the last patch in (now abandoned ?) #395474-218: Plugin Manager in Core: Part 2 (integration with update status).
Actually it didn't work on my setup.
For it to work, I had to add this line at the beginning of FileTransferFTPWrapper::removeDirectoryJailed() :

$directory = str_replace($this->chroot . '/', '', $directory);

I'm obviously not saying this is the right fix, just pointing that the function received $directory as [a_path_that_happens_to_be_$this->chroot]/sites/all/modules/foo,
which failed, while $directory = 'sites/all/modules/foo' worked.
After that change, the rest of the FTP operations worked (remove directory, copy new files).

yched’s picture

yched CreditAttribution: yched commented

It seems like the explanation could be that the chroot should be stripped in fixRemotePath() (called in FileTransfer::removeDirectory()), but the branch

      if (!empty($this->chroot) && strpos($path, $this->chroot) === 0) {
        $path = ($path == $this->chroot) ? '' : substr($path, strlen($this->chroot));
      }

fails because $this->chroot is not yet set at this point.

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented
FileSize
filetransfer_chroot_other_fixes-528326-34.patch16.58 KB

Okay, let's try again!

This time rolled from git, hopefully no issues.

int’s picture

int CreditAttribution: int commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

adrian’s picture

adrian CreditAttribution: adrian commented
Status: Needs work » Needs review
FileSize
filetransfer.patch16.97 KB

re-rolled and tested the patch here

passes all the files tests and applies properly.

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented

The testbot doesn't like either of us. I'll diff our patches (although obviously, something is commonly confusing for the bot and try again. Probably also should ditch git until this is working properly.

Best,
J

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented
Status: Needs work » Needs review
FileSize
filetransfer_chroot_other_fixes-528326-40.patch13.95 KB

Okay, test bot, this is between you and me.

Status: Needs review » Needs work

The last submitted patch failed testing.

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented
FileSize
filetransfer_chroot_other_fixes-528326-42.patch14.39 KB

You will submit to my will testbot. There will be no E_NOTICE, you are on E_NOTICE.

cwgordon7’s picture

cwgordon7 CreditAttribution: cwgordon7 commented
Status: Needs work » Needs review
cwgordon7’s picture

cwgordon7 CreditAttribution: cwgordon7 commented
FileSize
filetransfer_chroot_other_fixes-528326-44.patch13.96 KB

Patch included an unrelated change in file.inc, I removed it, no functional changes.

JacobSingh’s picture

JacobSingh CreditAttribution: JacobSingh commented

Thanks charlie, my bad.

int’s picture

int CreditAttribution: int commented
Status: Needs review » Reviewed & tested by the community

works for me, and is by far, better that before.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

peda_servus’s picture

peda_servus CreditAttribution: peda_servus commented
Version: 7.x-dev » 7.0
Priority: Critical » Major
Status: Closed (fixed) » Active

I just tested the drupal7 update manager on two of my machines and it still does not work if the ftp server is cofigured to chroot its users.

Test environment 1:
- debian-6.0.1 squeeze
- apache 2.2.16
- vsftpd 2.3.2
- drupal version: 7.0
If the option chroot_local_user=YES in vsftpd.conf is set upgrading or installing modules via the update manager doesn't work. The errormessage says: File Transfer failed, reason: Cannot create directory /home/peda/drupal-7.0/sites/all/modules
Uncommenting the option leads to update manager working. (but I really want users on a shared webhost to be chrooted to their home dirs)
Test environment 2:
- exchanged vsftpd with proftpd
Same here.

Can you please fix this or, if already fixed and this is just because I'm unable to configure the ftp server properly give me a hint on how to get this working.

greets peda

bfroehle’s picture

bfroehle CreditAttribution: bfroehle commented

@peda_servus: Just to be clear, can you provide the exact steps that triggered the error? For example, I navigated to XYZT, clicked on FOO, entered my FTP credentials, etc.

peda_servus’s picture

peda_servus CreditAttribution: peda_servus commented

No prob:

1) Navigate to http://localhost/?q=admin/modules (localhost is the debian squeeze box)
2) Click on Install Modules
3) Enter URL to fetch module src from (e.g. http://ftp.drupal.org/files/projects/video-7.x-1.0-alpha1.tar.gz)
4) Click install
5) Enter ftp credentials > Continue
6) Read Error Message:
* Warning: ftp_mkdir(): /home/peda/drupal-7.0/sites/all/modules: No such file or directory in FileTransferFTPExtension->createDirectoryJailed() (line 72 of /home/peda/drupal-7.0/includes/filetransfer/ftp.inc).
* Installation failed! See the log below for more information.

video

* Error installing / updating
* File Transfer failed, reason: Cannot create directory /home/peda/drupal-7.0/sites/all/modules

Next steps

The directory /home/peda/drupal-7.0/sites/all/modules exists, is owned by the ftp user and has rwxr-xr-x permissions.

When I uncomment the "DefaultRoot ~" line in /etc/proftpd/proftpd.conf restart the ftp server and do the exact same steps the installation works.

Scott M. Sanders’s picture

Scott M. Sanders CreditAttribution: Scott M. Sanders commented

Following

peda_servus’s picture

peda_servus CreditAttribution: peda_servus commented

hmm seems to work now (updated to r5564 from acquia svn), but only if the fto user owns everything in the drupal directory and its "homedir" is the drupal root. (thats not exactly what i always want but i can live with that)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Active » Closed (fixed)

Please create follow-up issues for any remaining bugs. This one has been fixed long ago.

Scott M. Sanders’s picture

Scott M. Sanders CreditAttribution: Scott M. Sanders commented

Here's a couple open issues virtually identical to this one:

http://drupal.org/node/1144690
http://drupal.org/node/842620