pkg / sftp

SFTP support for the go.crypto/ssh package
BSD 2-Clause "Simplified" License
1.5k stars 380 forks source link

Filezilla lists directories as files with this package #446

Closed onetwopunch closed 3 years ago

onetwopunch commented 3 years ago

This may not be an issue with this package, but I'm hoping maybe someone here has solved this. I'm creating an FTP server backed by GCS and everything works great on CyberDuck, but on Filezilla, the directories are being shown as files, this means when I try to double-click into the directory to put a file into it, I'm getting Read method instead of a List as I should. My IsDir method on the virtual file implementation is being called and showing the correct response.

Just wondering if anyone has experience testing this library with FileZilla and how to get it to show directories. Current relevant code looks like this:

type listerat []os.FileInfo

// Modeled after strings.Reader's ReadAt() implementation
func (f listerat) ListAt(ls []os.FileInfo, offset int64) (int, error) {
    var n int
    if offset >= int64(len(f)) {
        return 0, io.EOF
    }
    n = copy(ls, f[offset:])
    if n < len(ls) {
        return n, io.EOF
    }
    return n, nil
}

func (fs *gcsHandler) Filelist(r *sftp.Request) (sftp.ListerAt, error) {
    fs.log.Printf("Method: %s", r.Method)
    switch r.Method {
    case "List":
        fs.log.Printf("Listing directory for path %s", r.Filepath)

        prefix := r.Filepath[1:]
        if prefix != "" {
            prefix += "/"
        }

        objects := fs.bucket.Objects(r.Context(), &storage.Query{
            Delimiter: "/",
            Prefix:    prefix,
        })

        var list []os.FileInfo
        for {
            objAttrs, err := objects.Next()
            if err == iterator.Done {
                break
            }

            if err != nil {
                return nil, fmt.Errorf("unable to iterate directory %s: %s", r.Filepath, err)
            }

            // Don't include self.
            if ((prefix != "") && (objAttrs.Prefix == prefix)) || ((objAttrs.Prefix == "") && (objAttrs.Name == prefix)) {
                continue
            }
            list = append(list, &SyntheticFileInfo{
                prefix:  prefix,
                objAttr: objAttrs,
            })
        }
        return listerat(list), nil
...
}

I've looked all over for anything close to this on FileZilla forums, etc. but nothing, so I'm wondering if it may be how they interact with this package.

puellanivis commented 3 years ago

I’m kind of taking stabs here, but:

The IsDir() attribute is not used in the process of constructing file attributes. It is necessary that SyntheticFileInfo.Mode() return a mode that has mode & os.ModeDir != 0 otherwise the appropriate S_IFDIR will not be set in the response marshalled.

Are any other os.ModeType flags other than os.ModeDir set on the Mode() response? This can also cause chaos with the current implementation. In the use filexfer PR I’m reworking the way the marshaller works to try and avoid this “multiple os.ModeType values are set” possible bug.

If neither of these apply, then I might need more implementation details from the SyntheticFileInfo type, and the type of objAttrs to be sure of any other weird going on.

onetwopunch commented 3 years ago

@puellanivis Thanks for the response, and sorry for the lag. The Mode() function does | with os.ModeDir iff it is a directory, so that value cannot be 0. However your response got me looking into other methods of the FileInfo interface and while the spec says that Sys() can be nil, and that's how I had it, it turns out something with FileZilla is using that method to determine mode and not the Mode or IsDir.

Once I made this change, everything works as expected on Filezilla:

 func (f *SyntheticFileInfo) Sys() interface{} { // underlying data source (can return nil)
-       return nil
+       var mode uint16 = 0100777
+       if f.IsDir() {
+               mode = 040777
+       }
+       return &syscall.Stat_t{
+               Mode: mode,
+       }
 }

Without you I never would have thought about Mode being the determining factor here, so thank you! My theory is that CyberDuck does a walk of the entire tree first to determine mode or something, which is why it was working there. Any other insight you can provide would be awesome, but I'll go ahead and close the issue for now since it's resolved on my end. Thanks again!

puellanivis commented 3 years ago

Wait, I thought these were interacting with FileZilla through an SFTP connection, not through code?

If your returning this SyntheticFileInfo to our code, returning nil in the Sys() and it’s not working, then that should still be a bug in our code.

puellanivis commented 3 years ago

Would you be able to share with us the SyntheticFileInfo.Mode() function? Also, what specific version are you using of pkg/sftp in your go.mod?

drakkan commented 3 years ago

Would you be able to share with us the SyntheticFileInfo.Mode() function? Also, what specific version are you using of pkg/sftp in your go.mod?

@puellanivis this is something that I want to investigate since a lot of time, but I'm always busy with other things ...

Please take a look here:

https://github.com/drakkan/sftpgo/blob/main/vfs/fileinfo.go https://github.com/drakkan/sftpgo/blob/main/vfs/sys_unix.go#L28 https://github.com/drakkan/sftpgo/blob/main/vfs/sys_windows.go#L10

I have to apply hacks like the following ones also in sftpfs (an SFTP server served over SFTP) to avoid this issue

https://github.com/drakkan/sftpgo/blob/main/vfs/sftpfs.go#L228 https://github.com/drakkan/sftpgo/blob/main/vfs/sftpfs.go#L431

so it affects both server and client

puellanivis commented 3 years ago

https://github.com/pkg/sftp/pull/452 This should fix the problem.

drakkan commented 3 years ago

452 is now merged, please confirm that it fixes the issue for you too, thank you

onetwopunch commented 3 years ago

I am interacting with FileZilla via FTP...as in I'm using pkg/sftp as my SFTP server but using GCS as the backend where the files are actually stored, so I needed an implementation of FileInfo, sorry if that wasn't clear. Happy to share the whole implementation of SyntheticFileInfo as it was when things were failing:

type SyntheticFileInfo struct {
    objAttr *storage.ObjectAttrs
    prefix  string
}

func (f *SyntheticFileInfo) Name() string { // base name of the file
    if f.objAttr.Prefix != "" {
        return f.objAttr.Prefix[len(f.prefix) : len(f.objAttr.Prefix)-1]
    }

    return f.objAttr.Name[len(f.prefix):]
}

func (f *SyntheticFileInfo) Size() int64 { // length in bytes for regular files; system-dependent for others
    return f.objAttr.Size
}

func (f *SyntheticFileInfo) Mode() os.FileMode { // file mode bits
    if f.IsDir() {
        return os.ModeDir | 0777
    }

    return 0777
}

func (f *SyntheticFileInfo) ModTime() time.Time { // modification time
    if f.IsDir() {
        return time.Now()
    }

    return f.objAttr.Updated
}

func (f *SyntheticFileInfo) IsDir() bool { // abbreviation for Mode().IsDir()
    if f.objAttr.Prefix != "" {
        return true
    } else if len(f.objAttr.Name) > 0 {
        if f.objAttr.Name[len(f.objAttr.Name)-1:] == "/" && f.objAttr.Size == 0 {
            return true
        }
    }
    return false
}

func (f *SyntheticFileInfo) Sys() interface{} { // underlying data source (can return nil)
    return nil
}

The version in my go.mod is github.com/pkg/sftp v1.12.1-0.20201118115123-7230c61342c8

I'll try the new version today and see if that works! Thank you!

onetwopunch commented 3 years ago

Just verified that this fixed the issue! Thank you so much for the quick turn around.

puellanivis commented 3 years ago

🎉 great news! Thanks for helping put me on the right path to find the issue.