From 6a7168cf46fd0131e9f0cc3b739b60e2ce0cac08 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 14 Oct 2022 13:33:40 +0300 Subject: [PATCH 01/12] Add support for working directory in Server This commit allows the working directory for the (old) Server implementation to be changed without doing a `os.Chdir` first. The feature can be enabled with `sftp.WithServerWorkingDirectory(dir)` passed as an option to `sftp.NewServer`. It is useful when the `sftp` is used as part of a larger service that does more than just serve `sftp` and using `os.Chdir` is not an option. The fallback behavior (when the option is not specified) is that the path remains unmodified (as before). --- packet.go | 4 ++-- request-plan9.go | 11 ++++++++++- request-unix.go | 12 +++++++++++- request_windows.go | 11 ++++++++++- server.go | 49 ++++++++++++++++++++++++---------------------- 5 files changed, 59 insertions(+), 28 deletions(-) diff --git a/packet.go b/packet.go index 1a1a87d7..7fd605c6 100644 --- a/packet.go +++ b/packet.go @@ -1247,7 +1247,7 @@ func (p *sshFxpExtendedPacketPosixRename) UnmarshalBinary(b []byte) error { } func (p *sshFxpExtendedPacketPosixRename) respond(s *Server) responsePacket { - err := os.Rename(toLocalPath(p.Oldpath), toLocalPath(p.Newpath)) + err := os.Rename(toLocalPath(s.workDir, p.Oldpath), toLocalPath(s.workDir, p.Newpath)) return statusFromError(p.ID, err) } @@ -1276,6 +1276,6 @@ func (p *sshFxpExtendedPacketHardlink) UnmarshalBinary(b []byte) error { } func (p *sshFxpExtendedPacketHardlink) respond(s *Server) responsePacket { - err := os.Link(toLocalPath(p.Oldpath), toLocalPath(p.Newpath)) + err := os.Link(toLocalPath(s.workDir, p.Oldpath), toLocalPath(s.workDir, p.Newpath)) return statusFromError(p.ID, err) } diff --git a/request-plan9.go b/request-plan9.go index 2444da59..daffd3e1 100644 --- a/request-plan9.go +++ b/request-plan9.go @@ -16,7 +16,15 @@ func testOsSys(sys interface{}) error { return nil } -func toLocalPath(p string) string { +func toLocalPath(workDir, p string) string { + if workDir != "" { + if !filepath.IsAbs(p) && !path.IsAbs(p) { + // Ensure input is always in the same format. + p = filepath.ToSlash(p) + p = path.Join(workDir, p) + } + } + lp := filepath.FromSlash(p) if path.IsAbs(p) { @@ -28,6 +36,7 @@ func toLocalPath(p string) string { // e.g. "/#s/boot" to "#s/boot" return tmp } + } return lp diff --git a/request-unix.go b/request-unix.go index 50b08a38..2273d567 100644 --- a/request-unix.go +++ b/request-unix.go @@ -4,6 +4,8 @@ package sftp import ( "errors" + "path" + "path/filepath" "syscall" ) @@ -22,6 +24,14 @@ func testOsSys(sys interface{}) error { return nil } -func toLocalPath(p string) string { +func toLocalPath(workDir, p string) string { + if workDir != "" { + if !filepath.IsAbs(p) && !path.IsAbs(p) { + // Ensure input is always in the same format. + p = filepath.ToSlash(p) + p = path.Join(workDir, p) + } + } + return p } diff --git a/request_windows.go b/request_windows.go index 1f6d3df1..d53cecf2 100644 --- a/request_windows.go +++ b/request_windows.go @@ -14,7 +14,15 @@ func testOsSys(sys interface{}) error { return nil } -func toLocalPath(p string) string { +func toLocalPath(workDir, p string) string { + if workDir != "" { + if !filepath.IsAbs(p) && !path.IsAbs(p) { + // Ensure input is always in the same format. + p = filepath.ToSlash(p) + p = path.Join(workDir, p) + } + } + lp := filepath.FromSlash(p) if path.IsAbs(p) { @@ -38,6 +46,7 @@ func toLocalPath(p string) string { // e.g. "/C:" to "C:\\" return tmp } + } return lp diff --git a/server.go b/server.go index 529052b4..677a10c7 100644 --- a/server.go +++ b/server.go @@ -33,6 +33,7 @@ type Server struct { openFiles map[string]*os.File openFilesLock sync.RWMutex handleCount int + workDir string } func (svr *Server) nextHandle(f *os.File) string { @@ -128,6 +129,16 @@ func WithAllocator() ServerOption { } } +// WithServerWorkingDirectory sets a working directory to use as base +// for relative paths. +// If unset the default is current working directory (os.Getwd). +func WithServerWorkingDirectory(workDir string) ServerOption { + return func(s *Server) error { + s.workDir = cleanPath(workDir) + return nil + } +} + type rxPacket struct { pktType fxp pktBytes []byte @@ -174,7 +185,7 @@ func handlePacket(s *Server, p orderedRequest) error { } case *sshFxpStatPacket: // stat the requested file - info, err := os.Stat(toLocalPath(p.Path)) + info, err := os.Stat(toLocalPath(s.workDir, p.Path)) rpkt = &sshFxpStatResponse{ ID: p.ID, info: info, @@ -184,7 +195,7 @@ func handlePacket(s *Server, p orderedRequest) error { } case *sshFxpLstatPacket: // stat the requested file - info, err := os.Lstat(toLocalPath(p.Path)) + info, err := os.Lstat(toLocalPath(s.workDir, p.Path)) rpkt = &sshFxpStatResponse{ ID: p.ID, info: info, @@ -208,24 +219,24 @@ func handlePacket(s *Server, p orderedRequest) error { } case *sshFxpMkdirPacket: // TODO FIXME: ignore flags field - err := os.Mkdir(toLocalPath(p.Path), 0755) + err := os.Mkdir(toLocalPath(s.workDir, p.Path), 0o755) rpkt = statusFromError(p.ID, err) case *sshFxpRmdirPacket: - err := os.Remove(toLocalPath(p.Path)) + err := os.Remove(toLocalPath(s.workDir, p.Path)) rpkt = statusFromError(p.ID, err) case *sshFxpRemovePacket: - err := os.Remove(toLocalPath(p.Filename)) + err := os.Remove(toLocalPath(s.workDir, p.Filename)) rpkt = statusFromError(p.ID, err) case *sshFxpRenamePacket: - err := os.Rename(toLocalPath(p.Oldpath), toLocalPath(p.Newpath)) + err := os.Rename(toLocalPath(s.workDir, p.Oldpath), toLocalPath(s.workDir, p.Newpath)) rpkt = statusFromError(p.ID, err) case *sshFxpSymlinkPacket: - err := os.Symlink(toLocalPath(p.Targetpath), toLocalPath(p.Linkpath)) + err := os.Symlink(toLocalPath(s.workDir, p.Targetpath), toLocalPath(s.workDir, p.Linkpath)) rpkt = statusFromError(p.ID, err) case *sshFxpClosePacket: rpkt = statusFromError(p.ID, s.closeHandle(p.Handle)) case *sshFxpReadlinkPacket: - f, err := os.Readlink(toLocalPath(p.Path)) + f, err := os.Readlink(toLocalPath(s.workDir, p.Path)) rpkt = &sshFxpNamePacket{ ID: p.ID, NameAttrs: []*sshFxpNameAttr{ @@ -240,29 +251,21 @@ func handlePacket(s *Server, p orderedRequest) error { rpkt = statusFromError(p.ID, err) } case *sshFxpRealpathPacket: - f, err := filepath.Abs(toLocalPath(p.Path)) + f, err := filepath.Abs(toLocalPath(s.workDir, p.Path)) f = cleanPath(f) - rpkt = &sshFxpNamePacket{ - ID: p.ID, - NameAttrs: []*sshFxpNameAttr{ - { - Name: f, - LongName: f, - Attrs: emptyFileStat, - }, - }, - } + rpkt = cleanPacketPath(p, f) if err != nil { rpkt = statusFromError(p.ID, err) } case *sshFxpOpendirPacket: - p.Path = toLocalPath(p.Path) + p.Path = toLocalPath(s.workDir, p.Path) if stat, err := os.Stat(p.Path); err != nil { rpkt = statusFromError(p.ID, err) } else if !stat.IsDir() { rpkt = statusFromError(p.ID, &os.PathError{ - Path: p.Path, Err: syscall.ENOTDIR}) + Path: p.Path, Err: syscall.ENOTDIR, + }) } else { rpkt = (&sshFxpOpenPacket{ ID: p.ID, @@ -446,7 +449,7 @@ func (p *sshFxpOpenPacket) respond(svr *Server) responsePacket { osFlags |= os.O_EXCL } - f, err := os.OpenFile(toLocalPath(p.Path), osFlags, 0644) + f, err := os.OpenFile(toLocalPath(svr.workDir, p.Path), osFlags, 0o644) if err != nil { return statusFromError(p.ID, err) } @@ -484,7 +487,7 @@ func (p *sshFxpSetstatPacket) respond(svr *Server) responsePacket { b := p.Attrs.([]byte) var err error - p.Path = toLocalPath(p.Path) + p.Path = toLocalPath(svr.workDir, p.Path) debug("setstat name \"%s\"", p.Path) if (p.Flags & sshFileXferAttrSize) != 0 { From 624034d7c443d8236caf14b943722c6c1d8a9fad Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Sat, 15 Oct 2022 18:10:56 +0300 Subject: [PATCH 02/12] Add tests for toLocalPath --- request-plan9.go | 1 - request_test.go | 107 +++++++++++++++++++++++++++++++++++++++++++++ request_windows.go | 1 - 3 files changed, 107 insertions(+), 2 deletions(-) diff --git a/request-plan9.go b/request-plan9.go index daffd3e1..e27ea156 100644 --- a/request-plan9.go +++ b/request-plan9.go @@ -36,7 +36,6 @@ func toLocalPath(workDir, p string) string { // e.g. "/#s/boot" to "#s/boot" return tmp } - } return lp diff --git a/request_test.go b/request_test.go index 92f7c2bf..bcd42b11 100644 --- a/request_test.go +++ b/request_test.go @@ -3,8 +3,10 @@ package sftp import ( "bytes" "errors" + "fmt" "io" "os" + "runtime" "testing" "github.com/stretchr/testify/assert" @@ -247,3 +249,108 @@ func TestOpendirHandleReuse(t *testing.T) { rpkt = request.call(handlers, pkt, nil, 0) assert.IsType(t, &sshFxpNamePacket{}, rpkt) } + +func Test_toLocalPath(t *testing.T) { + type args struct { + workDir string + p string + } + tests := []struct { + name string + goos string + args args + want string + }{ + { + name: "empty path", + goos: "linux", + args: args{p: ""}, + want: "", + }, + { + name: "relative path", + goos: "linux", + args: args{p: "file"}, + want: "file", + }, + { + name: "absolute path", + goos: "linux", + args: args{p: "/file"}, + want: "/file", + }, + { + name: "empty path", + goos: "linux", + args: args{workDir: "/home/user", p: ""}, + want: "/home/user", + }, + { + name: "relative path", + goos: "linux", + args: args{workDir: "/home/user", p: "file"}, + want: "/home/user/file", + }, + { + name: "relative path with .", + goos: "linux", + args: args{workDir: "/home/user", p: "."}, + want: "/home/user", + }, + { + name: "absolute path", + goos: "linux", + args: args{workDir: "/home/user", p: "/file"}, + want: "/file", + }, + { + name: "empty path", + goos: "windows", + args: args{workDir: "C:\\Users\\User", p: ""}, + want: "C:\\Users\\User", + }, + { + name: "relative path", + goos: "windows", + args: args{workDir: "C:\\Users\\User", p: "file"}, + want: "C:\\Users\\User\\file", + }, + { + name: "relative path with .", + goos: "windows", + args: args{workDir: "C:\\Users\\User", p: "."}, + want: "C:\\Users\\User", + }, + { + name: "absolute path", + goos: "windows", + args: args{workDir: "C:\\Users\\User", p: "C:\\file"}, + want: "C:\\file", + }, + { + name: "relative unix-like path", + goos: "windows", + args: args{workDir: "C:\\Users\\User", p: "./file"}, + want: "C:\\Users\\User\\file", + }, + { + name: "absolute unix-like path", + goos: "windows", + args: args{workDir: "C:\\Users\\User", p: "/C:/file"}, + want: "C:\\file", + }, + } + for _, tt := range tests { + t.Run(fmt.Sprintf("%s %s %s", tt.goos, tt.args.workDir, tt.name), func(t *testing.T) { + if runtime.GOOS != tt.goos { + t.Skipf("Skipping test for %s on %s", tt.goos, runtime.GOOS) + } + + var s Server + if tt.args.workDir != "" { + _ = WithServerWorkingDirectory(tt.args.workDir)(&s) + } + assert.Equal(t, tt.want, toLocalPath(s.workDir, tt.args.p), "wrong local path") + }) + } +} diff --git a/request_windows.go b/request_windows.go index d53cecf2..b3a747d6 100644 --- a/request_windows.go +++ b/request_windows.go @@ -46,7 +46,6 @@ func toLocalPath(workDir, p string) string { // e.g. "/C:" to "C:\\" return tmp } - } return lp From 7de26511357125619c9cfd0c7bb4fce32e11bf90 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 17 Oct 2022 17:08:45 +0300 Subject: [PATCH 03/12] Simplify path check in `toLocalPath` --- request-plan9.go | 8 ++------ request-unix.go | 9 ++------- request_windows.go | 8 ++------ 3 files changed, 6 insertions(+), 19 deletions(-) diff --git a/request-plan9.go b/request-plan9.go index e27ea156..08969d40 100644 --- a/request-plan9.go +++ b/request-plan9.go @@ -17,12 +17,8 @@ func testOsSys(sys interface{}) error { } func toLocalPath(workDir, p string) string { - if workDir != "" { - if !filepath.IsAbs(p) && !path.IsAbs(p) { - // Ensure input is always in the same format. - p = filepath.ToSlash(p) - p = path.Join(workDir, p) - } + if workDir != "" && !path.IsAbs(p) { + p = path.Join(workDir, p) } lp := filepath.FromSlash(p) diff --git a/request-unix.go b/request-unix.go index 2273d567..c762fd06 100644 --- a/request-unix.go +++ b/request-unix.go @@ -5,7 +5,6 @@ package sftp import ( "errors" "path" - "path/filepath" "syscall" ) @@ -25,12 +24,8 @@ func testOsSys(sys interface{}) error { } func toLocalPath(workDir, p string) string { - if workDir != "" { - if !filepath.IsAbs(p) && !path.IsAbs(p) { - // Ensure input is always in the same format. - p = filepath.ToSlash(p) - p = path.Join(workDir, p) - } + if workDir != "" && !path.IsAbs(p) { + p = path.Join(workDir, p) } return p diff --git a/request_windows.go b/request_windows.go index b3a747d6..ac011a96 100644 --- a/request_windows.go +++ b/request_windows.go @@ -15,12 +15,8 @@ func testOsSys(sys interface{}) error { } func toLocalPath(workDir, p string) string { - if workDir != "" { - if !filepath.IsAbs(p) && !path.IsAbs(p) { - // Ensure input is always in the same format. - p = filepath.ToSlash(p) - p = path.Join(workDir, p) - } + if workDir != "" && !path.IsAbs(p) { + p = path.Join(workDir, p) } lp := filepath.FromSlash(p) From ff98544388df8c7f2e0d0b530795796400cfd7d0 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 17 Oct 2022 17:08:58 +0300 Subject: [PATCH 04/12] Simplify test --- request_test.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/request_test.go b/request_test.go index bcd42b11..a74a9367 100644 --- a/request_test.go +++ b/request_test.go @@ -346,11 +346,8 @@ func Test_toLocalPath(t *testing.T) { t.Skipf("Skipping test for %s on %s", tt.goos, runtime.GOOS) } - var s Server - if tt.args.workDir != "" { - _ = WithServerWorkingDirectory(tt.args.workDir)(&s) - } - assert.Equal(t, tt.want, toLocalPath(s.workDir, tt.args.p), "wrong local path") + workDir := cleanPath(tt.args.workDir) + assert.Equal(t, tt.want, toLocalPath(workDir, tt.args.p), "wrong local path") }) } } From 78e59618f6f9ea0d7b2543fcb8335fea495be6bd Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 17 Oct 2022 17:09:17 +0300 Subject: [PATCH 05/12] Revert unintended change --- server.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/server.go b/server.go index 677a10c7..92b26d63 100644 --- a/server.go +++ b/server.go @@ -253,7 +253,16 @@ func handlePacket(s *Server, p orderedRequest) error { case *sshFxpRealpathPacket: f, err := filepath.Abs(toLocalPath(s.workDir, p.Path)) f = cleanPath(f) - rpkt = cleanPacketPath(p, f) + rpkt = &sshFxpNamePacket{ + ID: p.ID, + NameAttrs: []*sshFxpNameAttr{ + { + Name: f, + LongName: f, + Attrs: emptyFileStat, + }, + }, + } if err != nil { rpkt = statusFromError(p.ID, err) } From 32ac60b179edd330fa2c0a2ad7e1db4868e3da1d Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 17 Oct 2022 17:11:18 +0300 Subject: [PATCH 06/12] Fix use of double-toLocalPath in `sshFxpOpendirPacket` --- server.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server.go b/server.go index 92b26d63..ccabf0c2 100644 --- a/server.go +++ b/server.go @@ -267,13 +267,13 @@ func handlePacket(s *Server, p orderedRequest) error { rpkt = statusFromError(p.ID, err) } case *sshFxpOpendirPacket: - p.Path = toLocalPath(s.workDir, p.Path) + lp := toLocalPath(s.workDir, p.Path) - if stat, err := os.Stat(p.Path); err != nil { + if stat, err := os.Stat(lp); err != nil { rpkt = statusFromError(p.ID, err) } else if !stat.IsDir() { rpkt = statusFromError(p.ID, &os.PathError{ - Path: p.Path, Err: syscall.ENOTDIR, + Path: lp, Err: syscall.ENOTDIR, }) } else { rpkt = (&sshFxpOpenPacket{ From ae4859f1cd8b002c4182f122b0f9dfdd64a8057b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 17 Oct 2022 17:20:39 +0300 Subject: [PATCH 07/12] Update tests for `toLocalPath` --- request_test.go | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/request_test.go b/request_test.go index a74a9367..e97bf5ac 100644 --- a/request_test.go +++ b/request_test.go @@ -263,19 +263,16 @@ func Test_toLocalPath(t *testing.T) { }{ { name: "empty path", - goos: "linux", args: args{p: ""}, want: "", }, { name: "relative path", - goos: "linux", args: args{p: "file"}, want: "file", }, { name: "absolute path", - goos: "linux", args: args{p: "/file"}, want: "/file", }, @@ -297,6 +294,12 @@ func Test_toLocalPath(t *testing.T) { args: args{workDir: "/home/user", p: "."}, want: "/home/user", }, + { + name: "relative path with . and file", + goos: "linux", + args: args{workDir: "/home/user", p: "./file"}, + want: "/home/user/file", + }, { name: "absolute path", goos: "linux", @@ -322,27 +325,27 @@ func Test_toLocalPath(t *testing.T) { want: "C:\\Users\\User", }, { - name: "absolute path", - goos: "windows", - args: args{workDir: "C:\\Users\\User", p: "C:\\file"}, - want: "C:\\file", - }, - { - name: "relative unix-like path", + name: "relative path with . and file", goos: "windows", args: args{workDir: "C:\\Users\\User", p: "./file"}, want: "C:\\Users\\User\\file", }, { - name: "absolute unix-like path", + name: "absolute path", goos: "windows", args: args{workDir: "C:\\Users\\User", p: "/C:/file"}, want: "C:\\file", }, } for _, tt := range tests { - t.Run(fmt.Sprintf("%s %s %s", tt.goos, tt.args.workDir, tt.name), func(t *testing.T) { - if runtime.GOOS != tt.goos { + var name string + if tt.goos == "" { + name = fmt.Sprintf("%s %s", tt.args.workDir, tt.name) + } else { + name = fmt.Sprintf("%s %s %s", tt.goos, tt.args.workDir, tt.name) + } + t.Run(name, func(t *testing.T) { + if tt.goos != "" && runtime.GOOS != tt.goos { t.Skipf("Skipping test for %s on %s", tt.goos, runtime.GOOS) } From bf40a02d096441e7dbb9eb4a668ee23fc7c5401c Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 18 Oct 2022 11:15:09 +0300 Subject: [PATCH 08/12] Cleanup intent of `toLocalPath` test --- request_test.go | 58 +++++++++++++++++++++---------------------------- 1 file changed, 25 insertions(+), 33 deletions(-) diff --git a/request_test.go b/request_test.go index e97bf5ac..bc09e9f6 100644 --- a/request_test.go +++ b/request_test.go @@ -3,7 +3,6 @@ package sftp import ( "bytes" "errors" - "fmt" "io" "os" "runtime" @@ -262,95 +261,88 @@ func Test_toLocalPath(t *testing.T) { want string }{ { - name: "empty path", + name: "empty path with no workdir", args: args{p: ""}, want: "", }, { - name: "relative path", + name: "relative path with no workdir", args: args{p: "file"}, want: "file", }, { - name: "absolute path", + name: "absolute path with no workdir", args: args{p: "/file"}, want: "/file", }, { - name: "empty path", + name: "workdir and empty path on Unix", goos: "linux", - args: args{workDir: "/home/user", p: ""}, + args: args{workDir: cleanPath("/home/user"), p: ""}, want: "/home/user", }, { - name: "relative path", + name: "workdir and relative path on Unix", goos: "linux", - args: args{workDir: "/home/user", p: "file"}, + args: args{workDir: cleanPath("/home/user"), p: "file"}, want: "/home/user/file", }, { - name: "relative path with .", + name: "workdir and relative path with . on Unix", goos: "linux", - args: args{workDir: "/home/user", p: "."}, + args: args{workDir: cleanPath("/home/user"), p: "."}, want: "/home/user", }, { - name: "relative path with . and file", + name: "workdir and relative path with . and file on Unix", goos: "linux", - args: args{workDir: "/home/user", p: "./file"}, + args: args{workDir: cleanPath("/home/user"), p: "./file"}, want: "/home/user/file", }, { - name: "absolute path", + name: "workdir and absolute path on Unix", goos: "linux", - args: args{workDir: "/home/user", p: "/file"}, + args: args{workDir: cleanPath("/home/user"), p: "/file"}, want: "/file", }, { - name: "empty path", + name: "workdir and empty path on Windows", goos: "windows", - args: args{workDir: "C:\\Users\\User", p: ""}, + args: args{workDir: cleanPath("C:\\Users\\User"), p: ""}, want: "C:\\Users\\User", }, { - name: "relative path", + name: "workdir and relative path on Windows", goos: "windows", - args: args{workDir: "C:\\Users\\User", p: "file"}, + args: args{workDir: cleanPath("C:\\Users\\User"), p: "file"}, want: "C:\\Users\\User\\file", }, { - name: "relative path with .", + name: "workdir and relative path with . on Windows", goos: "windows", - args: args{workDir: "C:\\Users\\User", p: "."}, + args: args{workDir: cleanPath("C:\\Users\\User"), p: "."}, want: "C:\\Users\\User", }, { - name: "relative path with . and file", + name: "workdir and relative path with . and file on Windows", goos: "windows", - args: args{workDir: "C:\\Users\\User", p: "./file"}, + args: args{workDir: cleanPath("C:\\Users\\User"), p: "./file"}, want: "C:\\Users\\User\\file", }, { - name: "absolute path", + name: "workdir and absolute path on Windows", goos: "windows", - args: args{workDir: "C:\\Users\\User", p: "/C:/file"}, + args: args{workDir: cleanPath("C:\\Users\\User"), p: "/C:/file"}, want: "C:\\file", }, } for _, tt := range tests { - var name string - if tt.goos == "" { - name = fmt.Sprintf("%s %s", tt.args.workDir, tt.name) - } else { - name = fmt.Sprintf("%s %s %s", tt.goos, tt.args.workDir, tt.name) - } - t.Run(name, func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { if tt.goos != "" && runtime.GOOS != tt.goos { t.Skipf("Skipping test for %s on %s", tt.goos, runtime.GOOS) } - workDir := cleanPath(tt.args.workDir) - assert.Equal(t, tt.want, toLocalPath(workDir, tt.args.p), "wrong local path") + assert.Equal(t, tt.want, toLocalPath(tt.args.workDir, tt.args.p), "wrong local path") }) } } From d0f04135b184380756d2d5c1ca7ad13771c3a85c Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 18 Oct 2022 11:17:10 +0300 Subject: [PATCH 09/12] Add test for invalid input on Windows --- request_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/request_test.go b/request_test.go index bc09e9f6..6947c601 100644 --- a/request_test.go +++ b/request_test.go @@ -335,6 +335,12 @@ func Test_toLocalPath(t *testing.T) { args: args{workDir: cleanPath("C:\\Users\\User"), p: "/C:/file"}, want: "C:\\file", }, + { + name: "workdir and non-unixy path on Windows prefixes workdir", + goos: "windows", + args: args{workDir: cleanPath("C:\\Users\\User"), p: "C:\file"}, + want: "C:\\Users\\User\\C:\file", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From b3f237d7adfb5ae813e661a98ce523529f76c508 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 18 Oct 2022 11:19:29 +0300 Subject: [PATCH 10/12] Add linux counterpart test --- request_test.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/request_test.go b/request_test.go index 6947c601..2ecc5230 100644 --- a/request_test.go +++ b/request_test.go @@ -305,6 +305,12 @@ func Test_toLocalPath(t *testing.T) { args: args{workDir: cleanPath("/home/user"), p: "/file"}, want: "/file", }, + { + name: "workdir and non-unixy path on Unix prefixes workdir", + goos: "linux", + args: args{workDir: cleanPath("/home/user"), p: "C:\\file"}, + want: "/home/user/C:\\file", + }, { name: "workdir and empty path on Windows", goos: "windows", @@ -338,8 +344,8 @@ func Test_toLocalPath(t *testing.T) { { name: "workdir and non-unixy path on Windows prefixes workdir", goos: "windows", - args: args{workDir: cleanPath("C:\\Users\\User"), p: "C:\file"}, - want: "C:\\Users\\User\\C:\file", + args: args{workDir: cleanPath("C:\\Users\\User"), p: "C:\\file"}, + want: "C:\\Users\\User\\C:\\file", }, } for _, tt := range tests { From c93b0a0af20494336b001146864ebb04e7d696d3 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 18 Oct 2022 18:15:21 +0300 Subject: [PATCH 11/12] Refactor tests and make `toLocalPath` a method on `Server` --- packet.go | 4 +- request-plan9.go | 23 --------- request-unix.go | 9 ---- request_test.go | 111 ----------------------------------------- request_windows.go | 35 ------------- server.go | 24 ++++----- server_plan9.go | 30 +++++++++++ server_unix.go | 16 ++++++ server_unix_test.go | 87 ++++++++++++++++++++++++++++++++ server_windows.go | 39 +++++++++++++++ server_windows_test.go | 87 ++++++++++++++++++++++++++++++++ 11 files changed, 273 insertions(+), 192 deletions(-) create mode 100644 server_plan9.go create mode 100644 server_unix.go create mode 100644 server_unix_test.go create mode 100644 server_windows.go create mode 100644 server_windows_test.go diff --git a/packet.go b/packet.go index 7fd605c6..d89ad997 100644 --- a/packet.go +++ b/packet.go @@ -1247,7 +1247,7 @@ func (p *sshFxpExtendedPacketPosixRename) UnmarshalBinary(b []byte) error { } func (p *sshFxpExtendedPacketPosixRename) respond(s *Server) responsePacket { - err := os.Rename(toLocalPath(s.workDir, p.Oldpath), toLocalPath(s.workDir, p.Newpath)) + err := os.Rename(s.toLocalPath(p.Oldpath), s.toLocalPath(p.Newpath)) return statusFromError(p.ID, err) } @@ -1276,6 +1276,6 @@ func (p *sshFxpExtendedPacketHardlink) UnmarshalBinary(b []byte) error { } func (p *sshFxpExtendedPacketHardlink) respond(s *Server) responsePacket { - err := os.Link(toLocalPath(s.workDir, p.Oldpath), toLocalPath(s.workDir, p.Newpath)) + err := os.Link(s.toLocalPath(p.Oldpath), s.toLocalPath(p.Newpath)) return statusFromError(p.ID, err) } diff --git a/request-plan9.go b/request-plan9.go index 08969d40..0e3b4836 100644 --- a/request-plan9.go +++ b/request-plan9.go @@ -3,8 +3,6 @@ package sftp import ( - "path" - "path/filepath" "syscall" ) @@ -15,24 +13,3 @@ func fakeFileInfoSys() interface{} { func testOsSys(sys interface{}) error { return nil } - -func toLocalPath(workDir, p string) string { - if workDir != "" && !path.IsAbs(p) { - p = path.Join(workDir, p) - } - - lp := filepath.FromSlash(p) - - if path.IsAbs(p) { - tmp := lp[1:] - - if filepath.IsAbs(tmp) { - // If the FromSlash without any starting slashes is absolute, - // then we have a filepath encoded with a prefix '/'. - // e.g. "/#s/boot" to "#s/boot" - return tmp - } - } - - return lp -} diff --git a/request-unix.go b/request-unix.go index c762fd06..d30b2569 100644 --- a/request-unix.go +++ b/request-unix.go @@ -4,7 +4,6 @@ package sftp import ( "errors" - "path" "syscall" ) @@ -22,11 +21,3 @@ func testOsSys(sys interface{}) error { } return nil } - -func toLocalPath(workDir, p string) string { - if workDir != "" && !path.IsAbs(p) { - p = path.Join(workDir, p) - } - - return p -} diff --git a/request_test.go b/request_test.go index 2ecc5230..92f7c2bf 100644 --- a/request_test.go +++ b/request_test.go @@ -5,7 +5,6 @@ import ( "errors" "io" "os" - "runtime" "testing" "github.com/stretchr/testify/assert" @@ -248,113 +247,3 @@ func TestOpendirHandleReuse(t *testing.T) { rpkt = request.call(handlers, pkt, nil, 0) assert.IsType(t, &sshFxpNamePacket{}, rpkt) } - -func Test_toLocalPath(t *testing.T) { - type args struct { - workDir string - p string - } - tests := []struct { - name string - goos string - args args - want string - }{ - { - name: "empty path with no workdir", - args: args{p: ""}, - want: "", - }, - { - name: "relative path with no workdir", - args: args{p: "file"}, - want: "file", - }, - { - name: "absolute path with no workdir", - args: args{p: "/file"}, - want: "/file", - }, - { - name: "workdir and empty path on Unix", - goos: "linux", - args: args{workDir: cleanPath("/home/user"), p: ""}, - want: "/home/user", - }, - { - name: "workdir and relative path on Unix", - goos: "linux", - args: args{workDir: cleanPath("/home/user"), p: "file"}, - want: "/home/user/file", - }, - { - name: "workdir and relative path with . on Unix", - goos: "linux", - args: args{workDir: cleanPath("/home/user"), p: "."}, - want: "/home/user", - }, - { - name: "workdir and relative path with . and file on Unix", - goos: "linux", - args: args{workDir: cleanPath("/home/user"), p: "./file"}, - want: "/home/user/file", - }, - { - name: "workdir and absolute path on Unix", - goos: "linux", - args: args{workDir: cleanPath("/home/user"), p: "/file"}, - want: "/file", - }, - { - name: "workdir and non-unixy path on Unix prefixes workdir", - goos: "linux", - args: args{workDir: cleanPath("/home/user"), p: "C:\\file"}, - want: "/home/user/C:\\file", - }, - { - name: "workdir and empty path on Windows", - goos: "windows", - args: args{workDir: cleanPath("C:\\Users\\User"), p: ""}, - want: "C:\\Users\\User", - }, - { - name: "workdir and relative path on Windows", - goos: "windows", - args: args{workDir: cleanPath("C:\\Users\\User"), p: "file"}, - want: "C:\\Users\\User\\file", - }, - { - name: "workdir and relative path with . on Windows", - goos: "windows", - args: args{workDir: cleanPath("C:\\Users\\User"), p: "."}, - want: "C:\\Users\\User", - }, - { - name: "workdir and relative path with . and file on Windows", - goos: "windows", - args: args{workDir: cleanPath("C:\\Users\\User"), p: "./file"}, - want: "C:\\Users\\User\\file", - }, - { - name: "workdir and absolute path on Windows", - goos: "windows", - args: args{workDir: cleanPath("C:\\Users\\User"), p: "/C:/file"}, - want: "C:\\file", - }, - { - name: "workdir and non-unixy path on Windows prefixes workdir", - goos: "windows", - args: args{workDir: cleanPath("C:\\Users\\User"), p: "C:\\file"}, - want: "C:\\Users\\User\\C:\\file", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if tt.goos != "" && runtime.GOOS != tt.goos { - t.Skipf("Skipping test for %s on %s", tt.goos, runtime.GOOS) - } - - assert.Equal(t, tt.want, toLocalPath(tt.args.workDir, tt.args.p), "wrong local path") - }) - } -} diff --git a/request_windows.go b/request_windows.go index ac011a96..bd1d6864 100644 --- a/request_windows.go +++ b/request_windows.go @@ -1,8 +1,6 @@ package sftp import ( - "path" - "path/filepath" "syscall" ) @@ -13,36 +11,3 @@ func fakeFileInfoSys() interface{} { func testOsSys(sys interface{}) error { return nil } - -func toLocalPath(workDir, p string) string { - if workDir != "" && !path.IsAbs(p) { - p = path.Join(workDir, p) - } - - lp := filepath.FromSlash(p) - - if path.IsAbs(p) { - tmp := lp - for len(tmp) > 0 && tmp[0] == '\\' { - tmp = tmp[1:] - } - - if filepath.IsAbs(tmp) { - // If the FromSlash without any starting slashes is absolute, - // then we have a filepath encoded with a prefix '/'. - // e.g. "/C:/Windows" to "C:\\Windows" - return tmp - } - - tmp += "\\" - - if filepath.IsAbs(tmp) { - // If the FromSlash without any starting slashes but with extra end slash is absolute, - // then we have a filepath encoded with a prefix '/' and a dropped '/' at the end. - // e.g. "/C:" to "C:\\" - return tmp - } - } - - return lp -} diff --git a/server.go b/server.go index ccabf0c2..c0665ed9 100644 --- a/server.go +++ b/server.go @@ -185,7 +185,7 @@ func handlePacket(s *Server, p orderedRequest) error { } case *sshFxpStatPacket: // stat the requested file - info, err := os.Stat(toLocalPath(s.workDir, p.Path)) + info, err := os.Stat(s.toLocalPath(p.Path)) rpkt = &sshFxpStatResponse{ ID: p.ID, info: info, @@ -195,7 +195,7 @@ func handlePacket(s *Server, p orderedRequest) error { } case *sshFxpLstatPacket: // stat the requested file - info, err := os.Lstat(toLocalPath(s.workDir, p.Path)) + info, err := os.Lstat(s.toLocalPath(p.Path)) rpkt = &sshFxpStatResponse{ ID: p.ID, info: info, @@ -219,24 +219,24 @@ func handlePacket(s *Server, p orderedRequest) error { } case *sshFxpMkdirPacket: // TODO FIXME: ignore flags field - err := os.Mkdir(toLocalPath(s.workDir, p.Path), 0o755) + err := os.Mkdir(s.toLocalPath(p.Path), 0o755) rpkt = statusFromError(p.ID, err) case *sshFxpRmdirPacket: - err := os.Remove(toLocalPath(s.workDir, p.Path)) + err := os.Remove(s.toLocalPath(p.Path)) rpkt = statusFromError(p.ID, err) case *sshFxpRemovePacket: - err := os.Remove(toLocalPath(s.workDir, p.Filename)) + err := os.Remove(s.toLocalPath(p.Filename)) rpkt = statusFromError(p.ID, err) case *sshFxpRenamePacket: - err := os.Rename(toLocalPath(s.workDir, p.Oldpath), toLocalPath(s.workDir, p.Newpath)) + err := os.Rename(s.toLocalPath(p.Oldpath), s.toLocalPath(p.Newpath)) rpkt = statusFromError(p.ID, err) case *sshFxpSymlinkPacket: - err := os.Symlink(toLocalPath(s.workDir, p.Targetpath), toLocalPath(s.workDir, p.Linkpath)) + err := os.Symlink(s.toLocalPath(p.Targetpath), s.toLocalPath(p.Linkpath)) rpkt = statusFromError(p.ID, err) case *sshFxpClosePacket: rpkt = statusFromError(p.ID, s.closeHandle(p.Handle)) case *sshFxpReadlinkPacket: - f, err := os.Readlink(toLocalPath(s.workDir, p.Path)) + f, err := os.Readlink(s.toLocalPath(p.Path)) rpkt = &sshFxpNamePacket{ ID: p.ID, NameAttrs: []*sshFxpNameAttr{ @@ -251,7 +251,7 @@ func handlePacket(s *Server, p orderedRequest) error { rpkt = statusFromError(p.ID, err) } case *sshFxpRealpathPacket: - f, err := filepath.Abs(toLocalPath(s.workDir, p.Path)) + f, err := filepath.Abs(s.toLocalPath(p.Path)) f = cleanPath(f) rpkt = &sshFxpNamePacket{ ID: p.ID, @@ -267,7 +267,7 @@ func handlePacket(s *Server, p orderedRequest) error { rpkt = statusFromError(p.ID, err) } case *sshFxpOpendirPacket: - lp := toLocalPath(s.workDir, p.Path) + lp := s.toLocalPath(p.Path) if stat, err := os.Stat(lp); err != nil { rpkt = statusFromError(p.ID, err) @@ -458,7 +458,7 @@ func (p *sshFxpOpenPacket) respond(svr *Server) responsePacket { osFlags |= os.O_EXCL } - f, err := os.OpenFile(toLocalPath(svr.workDir, p.Path), osFlags, 0o644) + f, err := os.OpenFile(svr.toLocalPath(p.Path), osFlags, 0o644) if err != nil { return statusFromError(p.ID, err) } @@ -496,7 +496,7 @@ func (p *sshFxpSetstatPacket) respond(svr *Server) responsePacket { b := p.Attrs.([]byte) var err error - p.Path = toLocalPath(svr.workDir, p.Path) + p.Path = svr.toLocalPath(p.Path) debug("setstat name \"%s\"", p.Path) if (p.Flags & sshFileXferAttrSize) != 0 { diff --git a/server_plan9.go b/server_plan9.go new file mode 100644 index 00000000..da4c92e0 --- /dev/null +++ b/server_plan9.go @@ -0,0 +1,30 @@ +//go:build plan9 +// +build plan9 + +package sftp + +import ( + "path" + "path/filepath" +) + +func (s *Server) toLocalPath(p string) string { + if s.workDir != "" && !path.IsAbs(p) { + p = path.Join(s.workDir, p) + } + + lp := filepath.FromSlash(p) + + if path.IsAbs(p) { + tmp := lp[1:] + + if filepath.IsAbs(tmp) { + // If the FromSlash without any starting slashes is absolute, + // then we have a filepath encoded with a prefix '/'. + // e.g. "/#s/boot" to "#s/boot" + return tmp + } + } + + return lp +} diff --git a/server_unix.go b/server_unix.go new file mode 100644 index 00000000..495b397c --- /dev/null +++ b/server_unix.go @@ -0,0 +1,16 @@ +//go:build !windows && !plan9 +// +build !windows,!plan9 + +package sftp + +import ( + "path" +) + +func (s *Server) toLocalPath(p string) string { + if s.workDir != "" && !path.IsAbs(p) { + p = path.Join(s.workDir, p) + } + + return p +} diff --git a/server_unix_test.go b/server_unix_test.go new file mode 100644 index 00000000..924dc479 --- /dev/null +++ b/server_unix_test.go @@ -0,0 +1,87 @@ +//go:build !windows && !plan9 +// +build !windows,!plan9 + +package sftp + +import ( + "testing" +) + +func TestServer_toLocalPath(t *testing.T) { + tests := []struct { + name string + withWorkDir string + p string + want string + }{ + { + name: "empty path with no workdir", + p: "", + want: "", + }, + { + name: "relative path with no workdir", + p: "file", + want: "file", + }, + { + name: "absolute path with no workdir", + p: "/file", + want: "/file", + }, + { + name: "workdir and empty path", + withWorkDir: "/home/user", + p: "", + want: "/home/user", + }, + { + name: "workdir and relative path", + withWorkDir: "/home/user", + p: "file", + want: "/home/user/file", + }, + { + name: "workdir and relative path with .", + withWorkDir: "/home/user", + p: ".", + want: "/home/user", + }, + { + name: "workdir and relative path with . and file", + withWorkDir: "/home/user", + p: "./file", + want: "/home/user/file", + }, + { + name: "workdir and absolute path", + withWorkDir: "/home/user", + p: "/file", + want: "/file", + }, + { + name: "workdir and non-unixy path prefixes workdir", + withWorkDir: "/home/user", + p: "C:\\file", + // This may look like a bug but it is the result of passing + // invalid input (a non-unixy path) to the server. + want: "/home/user/C:\\file", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // We don't need to initialize the Server further to test + // toLocalPath behavior. + s := &Server{} + if tt.withWorkDir != "" { + if err := WithServerWorkingDirectory(tt.withWorkDir)(s); err != nil { + t.Fatal(err) + } + } + + if got := s.toLocalPath(tt.p); got != tt.want { + t.Errorf("Server.toLocalPath() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/server_windows.go b/server_windows.go new file mode 100644 index 00000000..b35be730 --- /dev/null +++ b/server_windows.go @@ -0,0 +1,39 @@ +package sftp + +import ( + "path" + "path/filepath" +) + +func (s *Server) toLocalPath(p string) string { + if s.workDir != "" && !path.IsAbs(p) { + p = path.Join(s.workDir, p) + } + + lp := filepath.FromSlash(p) + + if path.IsAbs(p) { + tmp := lp + for len(tmp) > 0 && tmp[0] == '\\' { + tmp = tmp[1:] + } + + if filepath.IsAbs(tmp) { + // If the FromSlash without any starting slashes is absolute, + // then we have a filepath encoded with a prefix '/'. + // e.g. "/C:/Windows" to "C:\\Windows" + return tmp + } + + tmp += "\\" + + if filepath.IsAbs(tmp) { + // If the FromSlash without any starting slashes but with extra end slash is absolute, + // then we have a filepath encoded with a prefix '/' and a dropped '/' at the end. + // e.g. "/C:" to "C:\\" + return tmp + } + } + + return lp +} diff --git a/server_windows_test.go b/server_windows_test.go new file mode 100644 index 00000000..f6a68dc7 --- /dev/null +++ b/server_windows_test.go @@ -0,0 +1,87 @@ +//go:build windows +// +build windows + +package sftp + +import ( + "testing" +) + +func TestServer_toLocalPath(t *testing.T) { + tests := []struct { + name string + withWorkDir string + p string + want string + }{ + { + name: "empty path with no workdir", + p: "", + want: "", + }, + { + name: "relative path with no workdir", + p: "file", + want: "file", + }, + { + name: "absolute path with no workdir", + p: "/file", + want: "\\file", + }, + { + name: "workdir and empty path", + withWorkDir: "C:\\Users\\User", + p: "", + want: "C:\\Users\\User", + }, + { + name: "workdir and relative path", + withWorkDir: "C:\\Users\\User", + p: "file", + want: "C:\\Users\\User\\file", + }, + { + name: "workdir and relative path with .", + withWorkDir: "C:\\Users\\User", + p: ".", + want: "C:\\Users\\User", + }, + { + name: "workdir and relative path with . and file", + withWorkDir: "C:\\Users\\User", + p: "./file", + want: "C:\\Users\\User\\file", + }, + { + name: "workdir and absolute path", + withWorkDir: "C:\\Users\\User", + p: "/C:/file", + want: "C:\\file", + }, + { + name: "workdir and non-unixy path prefixes workdir", + withWorkDir: "C:\\Users\\User", + p: "C:\\file", + // This may look like a bug but it is the result of passing + // invalid input (a non-unixy path) to the server. + want: "C:\\Users\\User\\C:\\file", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // We don't need to initialize the Server further to test + // toLocalPath behavior. + s := &Server{} + if tt.withWorkDir != "" { + if err := WithServerWorkingDirectory(tt.withWorkDir)(s); err != nil { + t.Fatal(err) + } + } + + if got := s.toLocalPath(tt.p); got != tt.want { + t.Errorf("Server.toLocalPath() = %v, want %v", got, tt.want) + } + }) + } +} From 85143703909d41344a65ff88c9dc91468282be1e Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 18 Oct 2022 19:36:12 +0300 Subject: [PATCH 12/12] Fix PR comments (build tags, %q) --- server_unix_test.go => server_nowindows_test.go | 6 +++--- server_plan9.go | 3 --- server_windows_test.go | 5 +---- 3 files changed, 4 insertions(+), 10 deletions(-) rename server_unix_test.go => server_nowindows_test.go (93%) diff --git a/server_unix_test.go b/server_nowindows_test.go similarity index 93% rename from server_unix_test.go rename to server_nowindows_test.go index 924dc479..d38dbba3 100644 --- a/server_unix_test.go +++ b/server_nowindows_test.go @@ -1,5 +1,5 @@ -//go:build !windows && !plan9 -// +build !windows,!plan9 +//go:build !windows +// +build !windows package sftp @@ -80,7 +80,7 @@ func TestServer_toLocalPath(t *testing.T) { } if got := s.toLocalPath(tt.p); got != tt.want { - t.Errorf("Server.toLocalPath() = %v, want %v", got, tt.want) + t.Errorf("Server.toLocalPath() = %q, want %q", got, tt.want) } }) } diff --git a/server_plan9.go b/server_plan9.go index da4c92e0..4e8ed067 100644 --- a/server_plan9.go +++ b/server_plan9.go @@ -1,6 +1,3 @@ -//go:build plan9 -// +build plan9 - package sftp import ( diff --git a/server_windows_test.go b/server_windows_test.go index f6a68dc7..ca9ed027 100644 --- a/server_windows_test.go +++ b/server_windows_test.go @@ -1,6 +1,3 @@ -//go:build windows -// +build windows - package sftp import ( @@ -80,7 +77,7 @@ func TestServer_toLocalPath(t *testing.T) { } if got := s.toLocalPath(tt.p); got != tt.want { - t.Errorf("Server.toLocalPath() = %v, want %v", got, tt.want) + t.Errorf("Server.toLocalPath() = %q, want %q", got, tt.want) } }) }