Repository: pkg/sftp
Pull request #498 (2022-03-01 17:11:39)

request server: add WithStartDirectory option
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](https://github.com/pkg/sftp/blob/aa9a37d6394f61dbcfadf3478acf028a4f39b866/request.go#L147) 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: 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: 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.

Patch:
diff --git a/request-interfaces.go b/request-interfaces.go
index c8c424c..e5dc49b 100644
--- a/request-interfaces.go
+++ b/request-interfaces.go
@@ -90,6 +90,8 @@ type LstatFileLister interface {
 // We use "/" as start directory for relative paths, implementing this
 // interface you can customize the start directory.
 // You have to return an absolute POSIX path.
+//
+// Deprecated: if you want to set a start directory use WithStartDirectory RequestServerOption instead.
 type RealPathFileLister interface {
 	FileLister
 	RealPath(string) string
diff --git a/request-server.go b/request-server.go
index 5fa828b..b7dadd6 100644
--- a/request-server.go
+++ b/request-server.go
@@ -27,6 +27,8 @@ type RequestServer struct {
 	*serverConn
 	pktMgr *packetManager
 
+	startDirectory string
+
 	mu           sync.RWMutex
 	handleCount  int
 	openRequests map[string]*Request
@@ -47,6 +49,14 @@ func WithRSAllocator() RequestServerOption {
 	}
 }
 
+// WithStartDirectory sets a start directory to use as base for relative paths.
+// If unset the default is "/"
+func WithStartDirectory(startDirectory string) RequestServerOption {
+	return func(rs *RequestServer) {
+		rs.startDirectory = cleanPath(startDirectory)
+	}
+}
+
 // NewRequestServer creates/allocates/returns new RequestServer.
 // Normally there will be one server per user-session.
 func NewRequestServer(rwc io.ReadWriteCloser, h Handlers, options ...RequestServerOption) *RequestServer {
@@ -62,6 +72,8 @@ func NewRequestServer(rwc io.ReadWriteCloser, h Handlers, options ...RequestServ
 		serverConn: svrConn,
 		pktMgr:     newPktMgr(svrConn),
 
+		startDirectory: "/",
+
 		openRequests: make(map[string]*Request),
 	}
 
@@ -210,11 +222,11 @@ func (rs *RequestServer) packetWorker(ctx context.Context, pktChan chan orderedR
 			if realPather, ok := rs.Handlers.FileList.(RealPathFileLister); ok {
 				realPath = realPather.RealPath(pkt.getPath())
 			} else {
-				realPath = cleanPath(pkt.getPath())
+				realPath = cleanPathWithBase(rs.startDirectory, pkt.getPath())
 			}
 			rpkt = cleanPacketPath(pkt, realPath)
 		case *sshFxpOpendirPacket:
-			request := requestFromPacket(ctx, pkt)
+			request := requestFromPacket(ctx, pkt, rs.startDirectory)
 			handle := rs.nextRequest(request)
 			rpkt = request.opendir(rs.Handlers, pkt)
 			if _, ok := rpkt.(*sshFxpHandlePacket); !ok {
@@ -222,7 +234,7 @@ func (rs *RequestServer) packetWorker(ctx context.Context, pktChan chan orderedR
 				rs.closeRequest(handle)
 			}
 		case *sshFxpOpenPacket:
-			request := requestFromPacket(ctx, pkt)
+			request := requestFromPacket(ctx, pkt, rs.startDirectory)
 			handle := rs.nextRequest(request)
 			rpkt = request.open(rs.Handlers, pkt)
 			if _, ok := rpkt.(*sshFxpHandlePacket); !ok {
@@ -235,7 +247,10 @@ func (rs *RequestServer) packetWorker(ctx context.Context, pktChan chan orderedR
 			if !ok {
 				rpkt = statusFromError(pkt.ID, EBADF)
 			} else {
-				request = NewRequest("Stat", request.Filepath)
+				request = &Request{
+					Method:   "Stat",
+					Filepath: cleanPathWithBase(rs.startDirectory, request.Filepath),
+				}
 				rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 			}
 		case *sshFxpFsetstatPacket:
@@ -244,15 +259,24 @@ func (rs *RequestServer) packetWorker(ctx context.Context, pktChan chan orderedR
 			if !ok {
 				rpkt = statusFromError(pkt.ID, EBADF)
 			} else {
-				request = NewRequest("Setstat", request.Filepath)
+				request = &Request{
+					Method:   "Setstat",
+					Filepath: cleanPathWithBase(rs.startDirectory, request.Filepath),
+				}
 				rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 			}
 		case *sshFxpExtendedPacketPosixRename:
-			request := NewRequest("PosixRename", pkt.Oldpath)
-			request.Target = pkt.Newpath
+			request := &Request{
+				Method:   "PosixRename",
+				Filepath: cleanPathWithBase(rs.startDirectory, pkt.Oldpath),
+				Target:   cleanPathWithBase(rs.startDirectory, pkt.Newpath),
+			}
 			rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 		case *sshFxpExtendedPacketStatVFS:
-			request := NewRequest("StatVFS", pkt.Path)
+			request := &Request{
+				Method:   "StatVFS",
+				Filepath: cleanPathWithBase(rs.startDirectory, pkt.Path),
+			}
 			rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 		case hasHandle:
 			handle := pkt.getHandle()
@@ -263,7 +287,7 @@ func (rs *RequestServer) packetWorker(ctx context.Context, pktChan chan orderedR
 				rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 			}
 		case hasPath:
-			request := requestFromPacket(ctx, pkt)
+			request := requestFromPacket(ctx, pkt, rs.startDirectory)
 			rpkt = request.call(rs.Handlers, pkt, rs.pktMgr.alloc, orderID)
 			request.close()
 		default:
diff --git a/request.go b/request.go
index c6da4b6..116c27a 100644
--- a/request.go
+++ b/request.go
@@ -168,9 +168,11 @@ func (r *Request) copy() *Request {
 }
 
 // New Request initialized based on packet data
-func requestFromPacket(ctx context.Context, pkt hasPath) *Request {
-	method := requestMethod(pkt)
-	request := NewRequest(method, pkt.getPath())
+func requestFromPacket(ctx context.Context, pkt hasPath, baseDir string) *Request {
+	request := &Request{
+		Method:   requestMethod(pkt),
+		Filepath: cleanPathWithBase(baseDir, pkt.getPath()),
+	}
 	request.ctx, request.cancelCtx = context.WithCancel(ctx)
 
 	switch p := pkt.(type) {
@@ -180,13 +182,13 @@ func requestFromPacket(ctx context.Context, pkt hasPath) *Request {
 		request.Flags = p.Flags
 		request.Attrs = p.Attrs.([]byte)
 	case *sshFxpRenamePacket:
-		request.Target = cleanPath(p.Newpath)
+		request.Target = cleanPathWithBase(baseDir, p.Newpath)
 	case *sshFxpSymlinkPacket:
 		// NOTE: given a POSIX compliant signature: symlink(target, linkpath string)
 		// this makes Request.Target the linkpath, and Request.Filepath the target.
-		request.Target = cleanPath(p.Linkpath)
+		request.Target = cleanPathWithBase(baseDir, p.Linkpath)
 	case *sshFxpExtendedPacketHardlink:
-		request.Target = cleanPath(p.Newpath)
+		request.Target = cleanPathWithBase(baseDir, p.Newpath)
 	}
 	return request
 }


Test patch:
diff --git a/request-server_test.go b/request-server_test.go
index 88862fb..c8e64d6 100644
--- a/request-server_test.go
+++ b/request-server_test.go
@@ -37,7 +37,7 @@ func (cs csPair) testHandler() *root {
 
 const sock = "/tmp/rstest.sock"
 
-func clientRequestServerPair(t *testing.T) *csPair {
+func clientRequestServerPair(t *testing.T, options ...RequestServerOption) *csPair {
 	skipIfWindows(t)
 	skipIfPlan9(t)
 
@@ -62,7 +62,6 @@ func clientRequestServerPair(t *testing.T) *csPair {
 		require.NoError(t, err)
 
 		handlers := InMemHandler()
-		var options []RequestServerOption
 		if *testAllocator {
 			options = append(options, WithRSAllocator())
 		}
@@ -781,6 +780,37 @@ func TestRequestStatVFSError(t *testing.T) {
 	checkRequestServerAllocator(t, p)
 }
 
+func TestRequestStartDirOption(t *testing.T) {
+	startDir := "/start/dir"
+	p := clientRequestServerPair(t, WithStartDirectory(startDir))
+	defer p.Close()
+
+	// create the start directory
+	err := p.cli.MkdirAll(startDir)
+	require.NoError(t, err)
+	// the working directory must be the defined start directory
+	wd, err := p.cli.Getwd()
+	require.NoError(t, err)
+	require.Equal(t, startDir, wd)
+	// upload a file using a relative path, it must be uploaded to the start directory
+	fileName := "file.txt"
+	_, err = putTestFile(p.cli, fileName, "")
+	require.NoError(t, err)
+	// we must be able to stat the file using both a relative and an absolute path
+	for _, filePath := range []string{fileName, path.Join(startDir, fileName)} {
+		fi, err := p.cli.Stat(filePath)
+		require.NoError(t, err)
+		assert.Equal(t, fileName, fi.Name())
+	}
+	// list dir contents using a relative path
+	entries, err := p.cli.ReadDir(".")
+	assert.NoError(t, err)
+	assert.Len(t, entries, 1)
+	// delete the file using a relative path
+	err = p.cli.Remove(fileName)
+	assert.NoError(t, err)
+}
+
 func TestCleanDisconnect(t *testing.T) {
 	p := clientRequestServerPair(t)
 	defer p.Close()
@@ -831,6 +861,7 @@ func TestRealPath(t *testing.T) {
 func TestCleanPath(t *testing.T) {
 	assert.Equal(t, "/", cleanPath("/"))
 	assert.Equal(t, "/", cleanPath("."))
+	assert.Equal(t, "/", cleanPath(""))
 	assert.Equal(t, "/", cleanPath("/."))
 	assert.Equal(t, "/", cleanPath("/a/.."))
 	assert.Equal(t, "/a/c", cleanPath("/a/b/../c"))