Merge pull request #516 from drakkan/realpath

RealPathFileLister: allow to return an error

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

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
 }

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)

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) {