Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for working directory in Server #528

Merged
merged 12 commits into from
Oct 18, 2022
Merged

Add support for working directory in Server #528

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6a7168c
Add support for working directory in Server
mafredri Oct 14, 2022
624034d
Add tests for toLocalPath
mafredri Oct 15, 2022
7de2651
Simplify path check in `toLocalPath`
mafredri Oct 17, 2022
ff98544
Simplify test
mafredri Oct 17, 2022
78e5961
Revert unintended change
mafredri Oct 17, 2022
32ac60b
Fix use of double-toLocalPath in `sshFxpOpendirPacket`
mafredri Oct 17, 2022
ae4859f
Update tests for `toLocalPath`
mafredri Oct 17, 2022
bf40a02
Cleanup intent of `toLocalPath` test
mafredri Oct 18, 2022
d0f0413
Add test for invalid input on Windows
mafredri Oct 18, 2022
b3f237d
Add linux counterpart test
mafredri Oct 18, 2022
c93b0a0
Refactor tests and make `toLocalPath` a method on `Server`
mafredri Oct 18, 2022
8514370
Fix PR comments (build tags, %q)
mafredri Oct 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}
6 changes: 5 additions & 1 deletion request-plan9.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ func testOsSys(sys interface{}) error {
return nil
}

func toLocalPath(p string) string {
func toLocalPath(workDir, p string) string {
if workDir != "" && !path.IsAbs(p) {
p = path.Join(workDir, p)
}

lp := filepath.FromSlash(p)

if path.IsAbs(p) {
Expand Down
7 changes: 6 additions & 1 deletion request-unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package sftp

import (
"errors"
"path"
"syscall"
)

Expand All @@ -22,6 +23,10 @@ func testOsSys(sys interface{}) error {
return nil
}

func toLocalPath(p string) string {
func toLocalPath(workDir, p string) string {
if workDir != "" && !path.IsAbs(p) {
p = path.Join(workDir, p)
}

return p
}
111 changes: 111 additions & 0 deletions request_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"io"
"os"
"runtime"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -247,3 +248,113 @@ 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",
mafredri marked this conversation as resolved.
Show resolved Hide resolved
},
}
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")
})
}
}
6 changes: 5 additions & 1 deletion request_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ func testOsSys(sys interface{}) error {
return nil
}

func toLocalPath(p string) string {
func toLocalPath(workDir, p string) string {
if workDir != "" && !path.IsAbs(p) {
p = path.Join(workDir, p)
}

lp := filepath.FromSlash(p)

if path.IsAbs(p) {
Expand Down
40 changes: 26 additions & 14 deletions server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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{
Expand All @@ -240,7 +251,7 @@ 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,
Expand All @@ -256,13 +267,14 @@ func handlePacket(s *Server, p orderedRequest) error {
rpkt = statusFromError(p.ID, err)
}
case *sshFxpOpendirPacket:
p.Path = toLocalPath(p.Path)
lp := toLocalPath(s.workDir, p.Path)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s another p.Path that wasn’t replaced by lp in the &sshFxpOpenPacket packet. I’d probably recommend backing out the s/lp/p.Path/ change instead.

Less change, less chance of new bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow. The only other assignment to p.Path that I can find is in (*sshFxpSetstatPacket).respond.

This change was to prevent the following scenario: toLocalPath(s.workDir, toLocalPath(s.workDir, p.Path)) which happens in (*sshFxpOpenPacket).respond because of this assignment to p.Path. This becomes a problem when workDir is being used and we're not checking filepath.IsAbs, resulting in double workDir prefix on Windows and Plan 9.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ach so! You’re absolutely right. The toLocalPath really should be deferred in that case until the .respond happens. Nice bug catch!


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{
ID: p.ID,
Expand Down Expand Up @@ -446,7 +458,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)
}
Expand Down Expand Up @@ -484,7 +496,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 {
Expand Down