From 51a5aa4e044d4e12b5b696d2302fcfa54f468708 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sat, 16 Jul 2022 09:55:51 +0200 Subject: [PATCH] RealPathFileLister: allow to return an error added legacyRealPathFileLister for backward compatibility Fixes #512 --- request-example.go | 13 +---- request-interfaces.go | 19 +++++-- request-server.go | 17 +++++-- request-server_test.go | 109 +++++++++++++++++++++++++++++++++++------ 4 files changed, 124 insertions(+), 34 deletions(-) diff --git a/request-example.go b/request-example.go index ba22bcd0..ab6c675e 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 e5dc49bb..2e5ee6be 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 b7dadd6c..7a99db64 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) diff --git a/request-server_test.go b/request-server_test.go index c8e64d63..8eb0916a 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) {