search

pkg / sftp

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

request server: add WithStartDirectory option #498

Closed drakkan closed 2 years ago

drakkan commented 2 years ago

Alternative approach to support a start directory.

I tested #497 and it works fine but I don't like that pkg/sftp clean the path and I have to clean the raw path again in my code.

This approach seems cleaner and the RealPathFileLister is useless if we go this way, we can remove this interface.

The only drawback is that NewRequest now requires an already cleaned path. Prior to this patch the path passed to NewRequest was converted to an absolute UNIX path using / as base. I'll try to fix this compatibility issue later

drakkan commented 2 years ago

Hi @puellanivis

thanks for your review, I'll fix the PR based on your comments.

I know that removing the RealPathLister is a breaking change, but it does not work for real use cases. It only works if you have a client that initially requests the start directory and then sends absolute paths based on the response. I think it is very unlikely that anyone actually uses this interface. If someone is really using it, they should fix their code by migrating to the new server option.

puellanivis commented 2 years ago

We can just label the interface as deprecated, and say that implementing it won’t actually do anything. The same as we have ErrSshFxOk. Saying “they should fix their code” is all fine and good, but semver is pretty strict on this, and we need to bump the major version in order to remove it.

drakkan commented 2 years ago

Thank you!

shanehooker commented 1 year ago

I am not finding an example of how to use this with a real file system. I am a bit confused by the InMemHandler(). I would have prefered the WithStartDirectory() to have been a method applied to NewServer() which would have done the same thing as starting 'sftp-server -d [startDirectory]'.

Is there an example of how this can be called and work with a real file system?

puellanivis commented 1 year ago

The reason for NewRequestServer over NewServer is that the latter can only operate upon the local OS filesystem. Meaning, you cannot establish some other backing store, like s3, or even a caching temporary filesystem held in memory, lost during system restart (that is InMemoryHandler). So, we have elected to essentially “deprecate” NewServer as an insuffienciently extendable interface.

sftp-server -d [startDirectory] works by first doing an os.Chdir() before starting up the service, and letting the inherent OS filesystem management take care of the rest. You should be able to get the NewServer to work the same way.