Repository: pkg/sftp (go)

Issue:
SSH_FXP_REALPATH and symlinks
Hi,

OpenSSH resolves symlinks for `SSH_FXP_REALPATH` calls (without failing if the symlink is broken), see [here](https://github.com/openssh/openssh-portable/blob/master/sftp-realpath.c#L51).

We simply return a cleaned path without resolving any symbolic links. This is what the v3 specs literally say (v6 has more details):

> The SSH_FXP_REALPATH request can be used to have the server
canonicalize any given path name to an absolute path. This is useful
for converting path names containing ".." components or relative
pathnames without a leading slash into absolute paths

If we want to behave like OpenSSH I think we can use `os.Lstat` and `os.Readlink` for our server implementation. 

For request-server it is more tricky, we would have to generate `Lstat` and `Readlink` calls ourself. Maybe we can just document our realpath behaviour and remove the depracation notice from [RealPathFileLister](https://github.com/pkg/sftp/blob/master/request-interfaces.go#L95). Request server users can implement RealPathFileLister if they want to resolve symlinks. What do you think about? Thanks


Pull request description:
RealPathFileLister: allow to return an error
added legacyRealPathFileLister for backward compatibility

Fixes #512

Patch:
diff --git a/request-example.go b/request-example.go
index ba22bcd..ab6c675 100644
--- a/request-example.go
+++ b/request-example.go
@@ -464,19 +464,10 @@ func (fs *root) Lstat(r *Request) (ListerAt, error) {
 	return listerat{file}, nil
 }
 
-// implements RealpathFileLister interface
-func (fs *root) Realpath(p string) string {
-	if fs.startDirectory == "" || fs.startDirectory == "/" {
-		return cleanPath(p)
-	}
-	return cleanPathWithBase(fs.startDirectory, p)
-}
-
 // In memory file-system-y thing that the Hanlders live on
 type root struct {
-	rootFile       *memFile
-	mockErr        error
-	startDirectory string
+	rootFile *memFile
+	mockErr  error
 
 	mu    sync.Mutex
 	files map[string]*memFile
diff --git a/request-interfaces.go b/request-interfaces.go
index e5dc49b..2e5ee6b 100644
--- a/request-interfaces.go
+++ b/request-interfaces.go
@@ -87,12 +87,25 @@ type LstatFileLister interface {
 }
 
 // RealPathFileLister is a FileLister that implements the Realpath method.
-// We use "/" as start directory for relative paths, implementing this
-// interface you can customize the start directory.
+// The built-in RealPath implementation does not resolve symbolic links.
+// By implementing this interface you can customize the returned path
+// and, for example, resolve symbolinc links if needed for your use case.
 // You have to return an absolute POSIX path.
 //
-// Deprecated: if you want to set a start directory use WithStartDirectory RequestServerOption instead.
+// Up to v1.13.5 the signature for the RealPath method was:
+//
+// RealPath(string) string
+//
+// we have added a legacyRealPathFileLister that implements the old method
+// to ensure that your code does not break.
+// You should use the new method signature to avoid future issues
 type RealPathFileLister interface {
+	FileLister
+	RealPath(string) (string, error)
+}
+
+// This interface is here for backward compatibility only
+type legacyRealPathFileLister interface {
 	FileLister
 	RealPath(string) string
 }
diff --git a/request-server.go b/request-server.go
index b7dadd6..7a99db6 100644
--- a/request-server.go
+++ b/request-server.go
@@ -219,12 +219,21 @@ func (rs *RequestServer) packetWorker(ctx context.Context, pktChan chan orderedR
 			rpkt = statusFromError(pkt.ID, rs.closeRequest(handle))
 		case *sshFxpRealpathPacket:
 			var realPath string
-			if realPather, ok := rs.Handlers.FileList.(RealPathFileLister); ok {
-				realPath = realPather.RealPath(pkt.getPath())
-			} else {
+			var err error
+
+			switch pather := rs.Handlers.FileList.(type) {
+			case RealPathFileLister:
+				realPath, err = pather.RealPath(pkt.getPath())
+			case legacyRealPathFileLister:
+				realPath = pather.RealPath(pkt.getPath())
+			default:
 				realPath = cleanPathWithBase(rs.startDirectory, pkt.getPath())
 			}
-			rpkt = cleanPacketPath(pkt, realPath)
+			if err != nil {
+				rpkt = statusFromError(pkt.ID, err)
+			} else {
+				rpkt = cleanPacketPath(pkt, realPath)
+			}
 		case *sshFxpOpendirPacket:
 			request := requestFromPacket(ctx, pkt, rs.startDirectory)
 			handle := rs.nextRequest(request)


Test patch:
diff --git a/request-server_test.go b/request-server_test.go
index c8e64d6..8eb0916 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, options ...RequestServerOption) *csPair {
+func clientRequestServerPairWithHandlers(t *testing.T, handlers Handlers, options ...RequestServerOption) *csPair {
 	skipIfWindows(t)
 	skipIfPlan9(t)
 
@@ -61,7 +61,6 @@ func clientRequestServerPair(t *testing.T, options ...RequestServerOption) *csPa
 		fd, err := l.Accept()
 		require.NoError(t, err)
 
-		handlers := InMemHandler()
 		if *testAllocator {
 			options = append(options, WithRSAllocator())
 		}
@@ -90,6 +89,10 @@ func clientRequestServerPair(t *testing.T, options ...RequestServerOption) *csPa
 	return pair
 }
 
+func clientRequestServerPair(t *testing.T, options ...RequestServerOption) *csPair {
+	return clientRequestServerPairWithHandlers(t, InMemHandler(), options...)
+}
+
 func checkRequestServerAllocator(t *testing.T, p *csPair) {
 	if p.svr.pktMgr.alloc == nil {
 		return
@@ -840,22 +843,96 @@ func TestUncleanDisconnect(t *testing.T) {
 }
 
 func TestRealPath(t *testing.T) {
-	root := &root{
-		rootFile:       &memFile{name: "/", modtime: time.Now(), isdir: true},
-		files:          make(map[string]*memFile),
-		startDirectory: "/apath",
+	startDir := "/startdir"
+	// the default InMemHandler does not implement the RealPathFileLister interface
+	// so we are using the builtin implementation here
+	p := clientRequestServerPair(t, WithStartDirectory(startDir))
+	defer p.Close()
+
+	realPath, err := p.cli.RealPath(".")
+	require.NoError(t, err)
+	assert.Equal(t, startDir, realPath)
+	realPath, err = p.cli.RealPath("/")
+	require.NoError(t, err)
+	assert.Equal(t, "/", realPath)
+	realPath, err = p.cli.RealPath("..")
+	require.NoError(t, err)
+	assert.Equal(t, "/", realPath)
+	realPath, err = p.cli.RealPath("../../..")
+	require.NoError(t, err)
+	assert.Equal(t, "/", realPath)
+	// test a relative path
+	realPath, err = p.cli.RealPath("relpath")
+	require.NoError(t, err)
+	assert.Equal(t, path.Join(startDir, "relpath"), realPath)
+}
+
+// In memory file-system which implements RealPathFileLister
+type rootWithRealPather struct {
+	root
+}
+
+// implements RealpathFileLister interface
+func (fs *rootWithRealPather) RealPath(p string) (string, error) {
+	if fs.mockErr != nil {
+		return "", fs.mockErr
+	}
+	return cleanPath(p), nil
+}
+
+func TestRealPathFileLister(t *testing.T) {
+	root := &rootWithRealPather{
+		root: root{
+			rootFile: &memFile{name: "/", modtime: time.Now(), isdir: true},
+			files:    make(map[string]*memFile),
+		},
 	}
+	handlers := Handlers{root, root, root, root}
+	p := clientRequestServerPairWithHandlers(t, handlers)
+	defer p.Close()
+
+	realPath, err := p.cli.RealPath(".")
+	require.NoError(t, err)
+	assert.Equal(t, "/", realPath)
+	realPath, err = p.cli.RealPath("relpath")
+	require.NoError(t, err)
+	assert.Equal(t, "/relpath", realPath)
+	// test an error
+	root.returnErr(ErrSSHFxPermissionDenied)
+	_, err = p.cli.RealPath("/")
+	require.ErrorIs(t, err, os.ErrPermission)
+}
+
+// In memory file-system which implements legacyRealPathFileLister
+type rootWithLegacyRealPather struct {
+	root
+}
 
-	p := root.Realpath(".")
-	assert.Equal(t, root.startDirectory, p)
-	p = root.Realpath("/")
-	assert.Equal(t, "/", p)
-	p = root.Realpath("..")
-	assert.Equal(t, "/", p)
-	p = root.Realpath("../../..")
-	assert.Equal(t, "/", p)
-	p = root.Realpath("relpath")
-	assert.Equal(t, path.Join(root.startDirectory, "relpath"), p)
+// implements RealpathFileLister interface
+func (fs *rootWithLegacyRealPather) RealPath(p string) string {
+	return cleanPath(p)
+}
+
+func TestLegacyRealPathFileLister(t *testing.T) {
+	root := &rootWithLegacyRealPather{
+		root: root{
+			rootFile: &memFile{name: "/", modtime: time.Now(), isdir: true},
+			files:    make(map[string]*memFile),
+		},
+	}
+	handlers := Handlers{root, root, root, root}
+	p := clientRequestServerPairWithHandlers(t, handlers)
+	defer p.Close()
+
+	realPath, err := p.cli.RealPath(".")
+	require.NoError(t, err)
+	assert.Equal(t, "/", realPath)
+	realPath, err = p.cli.RealPath("..")
+	require.NoError(t, err)
+	assert.Equal(t, "/", realPath)
+	realPath, err = p.cli.RealPath("relpath")
+	require.NoError(t, err)
+	assert.Equal(t, "/relpath", realPath)
 }
 
 func TestCleanPath(t *testing.T) {