Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jul 9, 2021

Conversation

puellanivis
Copy link
Collaborator

Not even joking:

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

@drakkan
Copy link
Collaborator

drakkan commented Jul 9, 2021

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
Copy link
Collaborator Author

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
Copy link
Collaborator

drakkan commented Jul 9, 2021

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
Copy link
Collaborator Author

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
Copy link
Collaborator

drakkan commented Jul 9, 2021

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

@drakkan drakkan merged commit 7bfa5f2 into master Jul 9, 2021
@puellanivis
Copy link
Collaborator Author

🤷‍♀️ 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. 🤣

@puellanivis puellanivis deleted the patch/fix-filezilla branch July 9, 2021 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants