Closed drakkan closed 3 years ago
I’m kind of hesitant to add yet more behaviors exposed through these interfaces… I’m not sure what a better solution would look like, but I feel like this path is unsustainable and overly complicated.
Thank you for the review, I tryed to fix the code based on your comments. Other libraries that I use within SFTPGo have a similar structure, for example:
In this case we could also add a server option that allows to set the start directory and use this value instead of /
in cleanPath
.
An optional interface allows full control to the caller but I'm not sure that this is required.
Do you have better ideas?
Yeah, following up on my concern about the ReadPathLister
this is direction that we’re wanting to go towards: extensible interfaces. I just had a knee-jerk reaction to it. 😆
Thank you
Hi @puellanivis,
I'm trying to add a start directory feature to SFTPGo, see https://github.com/drakkan/sftpgo/issues/705.
I implemented the RealPathLister
interface and with smart clients like sftp
CLI or FileZilla this is enough. These clients request the current directory when they connect and then send absolute paths.
I started to write the test cases, take a look here:
currentDir, err := client.Getwd()
assert.NoError(t, err)
assert.Equal(t, startDir, currentDir)
entries, err := client.ReadDir(".")
client.Getwd
and client.RealPath
are ok, but if I do client.ReadDir(".")
I get the /
contents: our client sends a relative path and it is converted server side as absolute in our cleanPath
method. I think this happen only for the request server.
I think it would be better to avoid path normalization server side and delegate them to the request server implementation but it would be a backward incompatible change at this point.
Add a check for relative path and call the RealPathLister
could do the trick but it seems quite ugly and I don't want to add something like this.
Do you have better suggestions? Thanks
🤔 Hm. It seems the standard never really required all paths to be absolute. Although, it specifies only that relative paths be from a rooted point (the user’s home directory), and provides no way to change that directory once set. The fact that the relative directory support does not have any way to traverse directories is precisely the reason why requiring all paths be absolute has worked for so long, client have to do that relative path tracking almost entirely themselves already, so why not also do it for the user directory as well.
I think is a case where we’re slightly out of spec, and should be doing different behavior, but also… the assumptions made lead to a design that is going to be difficult to “fix” now.
This allow to customize the responses for SSH_FXP_REALPATH requests and so implementing features like a start directory.
I'm not sure if this is the best way to add this feature, maybe we could consider a server option too