From f235d628b6251ac435e09fc5fe32d3bd8adbc054 Mon Sep 17 00:00:00 2001 From: Florent Clairambault Date: Fri, 22 Sep 2017 01:17:27 +0200 Subject: [PATCH 1/3] Checking if we can write the file *before* opening the transfer connection. Fix for issue #30 --- server/client_handler.go | 2 +- server/handle_files.go | 32 +++++++++++++++++--------------- tests/misc_commands_test.go | 2 +- 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/server/client_handler.go b/server/client_handler.go index cc7616ba..f15eb142 100644 --- a/server/client_handler.go +++ b/server/client_handler.go @@ -172,7 +172,7 @@ func (c *clientHandler) writeMessage(code int, message string) { func (c *clientHandler) TransferOpen() (net.Conn, error) { if c.transfer == nil { c.writeMessage(550, "No passive connection declared") - return nil, errors.New("No passive connection declared") + return nil, errors.New("no passive connection declared") } c.writeMessage(150, "Using transfer connection") conn, err := c.transfer.Open() diff --git a/server/handle_files.go b/server/handle_files.go index 08f1a2b3..e64dbddf 100644 --- a/server/handle_files.go +++ b/server/handle_files.go @@ -19,17 +19,30 @@ func (c *clientHandler) handleAPPE() { // Handles both the "STOR" and "APPE" commands func (c *clientHandler) handleStoreAndAppend(append bool) { + file, err := c.openFile(c.absPath(c.param), append) - path := c.absPath(c.param) + if err != nil { + c.writeMessage(550, "Could not open file: "+err.Error()) + return + } if tr, err := c.TransferOpen(); err == nil { defer c.TransferClose() - if _, err := c.storeOrAppend(tr, path, append); err != nil && err != io.EOF { + if _, err := c.storeOrAppend(tr, file); err != nil && err != io.EOF { c.writeMessage(550, err.Error()) } } else { - c.writeMessage(550, err.Error()) + c.writeMessage(550, "Could not open transfer: "+err.Error()) + } +} + +func (c *clientHandler) openFile(path string, append bool) (FileStream, error) { + flag := os.O_WRONLY + if append { + flag |= os.O_APPEND } + + return c.driver.OpenFile(c, path, flag) } func (c *clientHandler) handleRETR() { @@ -81,18 +94,7 @@ func (c *clientHandler) handleCHMOD(params string) { c.writeMessage(200, "SITE CHMOD command successful") } -func (c *clientHandler) storeOrAppend(conn net.Conn, name string, append bool) (int64, error) { - flag := os.O_WRONLY - if append { - flag |= os.O_APPEND - } - - file, err := c.driver.OpenFile(c, name, flag) - - if err != nil { - return 0, err - } - +func (c *clientHandler) storeOrAppend(conn net.Conn, file FileStream) (int64, error) { if c.ctxRest != 0 { file.Seek(c.ctxRest, 0) c.ctxRest = 0 diff --git a/tests/misc_commands_test.go b/tests/misc_commands_test.go index 2cdf7f0b..29cefae9 100644 --- a/tests/misc_commands_test.go +++ b/tests/misc_commands_test.go @@ -1,8 +1,8 @@ package tests import ( - "github.com/secsy/goftp" "testing" + "github.com/secsy/goftp" ) func TestSiteCommand(t *testing.T) { From c3bf88160d910fb789779eec55781d892af70cba Mon Sep 17 00:00:00 2001 From: Florent Clairambault Date: Fri, 22 Sep 2017 01:20:15 +0200 Subject: [PATCH 2/3] Format fix --- tests/misc_commands_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/misc_commands_test.go b/tests/misc_commands_test.go index 29cefae9..5c9c1815 100644 --- a/tests/misc_commands_test.go +++ b/tests/misc_commands_test.go @@ -2,6 +2,7 @@ package tests import ( "testing" + "github.com/secsy/goftp" ) From b50d3755ae73664eca4ec08a22f20ca44181f733 Mon Sep 17 00:00:00 2001 From: Florent Clairambault Date: Sat, 23 Sep 2017 02:07:59 +0200 Subject: [PATCH 3/3] Added a test around file upload access issues --- tests/transfer_test.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/transfer_test.go b/tests/transfer_test.go index 5516ca2a..3382dd50 100644 --- a/tests/transfer_test.go +++ b/tests/transfer_test.go @@ -96,6 +96,7 @@ func ftpDelete(t *testing.T, ftp *goftp.Client, filename string) { } } +// TestTransfer validates the upload of file in both active and passive mode func TestTransfer(t *testing.T) { s := NewTestServer(true) s.Settings.NonStandardActiveDataPort = true @@ -137,3 +138,32 @@ func testTransferOnConnection(t *testing.T, server *server.FtpServer, active boo t.Fatal("The two files don't have the same hash:", hashUpload, "!=", hashDownload) } } + +// TestFailedTransfer validates the handling of failed transfer caused by file access issues +func TestFailedTransfer(t *testing.T) { + s := NewTestServer(true) + defer s.Stop() + + conf := goftp.Config{ + User: "test", + Password: "test", + } + + var err error + var c *goftp.Client + + if c, err = goftp.DialConfig(conf, s.Listener.Addr().String()); err != nil { + t.Fatal("Couldn't connect", err) + } + defer c.Close() + + // We create a 1KB file and upload it + file := createTemporaryFile(t, 1*1024) + if err = c.Store("/non/existing/path/file.bin", file); err == nil { + t.Fatal("This upload should have failed") + } + + if err = c.Store("file.bin", file); err != nil { + t.Fatal("This upload should have succeeded", err) + } +}