pkg / sftp

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

[bugfix] FileZilla parses the directory listing in longname #452

Closed puellanivis closed 3 years ago

puellanivis commented 3 years ago

Not even joking:

        wchar_t chr = permissionToken[0];
        …
        if (chr == 'd' || chr == 'l') {
                entry.flags |= CDirentry::flag_dir;
        }
drakkan commented 3 years ago

Hi,

while this patch should work for the server implementation, it likely does not help for the request server or for the fileinfo returned from the client implementation. If you get directory contents from an sftp client and then serve the returned os.Fileinfo directly over sftp (using a request server in my case) the issue happen too. We can merge this patch in the meantime and fix these other issue in future

puellanivis commented 3 years ago

I tested this specifically with sftpgo with an sshfs backend to an openssh server, with Filezilla as the endpoint client, and it works.

Both server implementations use the runLs command internally for responding to SSH_FXP_READDIR requests: https://github.com/pkg/sftp/blob/master/request.go#L419

The only places where we setup an sshFxpNameAttr that doesn’t have a LongName assignment is in tests, and the only places where we do not use runLs is on responses to SSH_FXP_REALPATH and SSH_FXP_READLINK.

drakkan commented 3 years ago

I tested this specifically with sftpgo with an sshfs backend to an openssh server, with Filezilla as the endpoint client, and it works.

Both server implementations use the runLs command internally for responding to SSH_FXP_READDIR requests: https://github.com/pkg/sftp/blob/master/request.go#L419

The only places where we setup an sshFxpNameAttr that doesn’t have a LongName assignment is in tests, and the only places where we do not use runLs is on responses to SSH_FXP_REALPATH and SSH_FXP_READLINK.

ops, great, thank you! Let me remove my workarounds within sftpgo (it worked without this patch too) and I'll confirm in few minutes. Thanks!!!

puellanivis commented 3 years ago

After you confirm, we should be good to merge. I tested the compile with aix/ppc64 (the weirdest Nlink value) and it worked, so everything that was already working, should still be working fine.

drakkan commented 3 years ago

wow, it works. Thanks and sorry for my laziness, I could have solved this issue a long time ago instead of adding workarounds inside SFTPGo

puellanivis commented 3 years ago

🤷‍♀️ It was a really weird set of symptoms, so no need to be sorry. You had stuff to do, and tracking down weird bugs isn’t always a high priorty thing. 🤣