diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 080faa67..76070fd5 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -6,7 +6,7 @@ on: push: branches: - main - - chore/lint-* + - chore/lint* pull_request: permissions: @@ -26,7 +26,7 @@ jobs: strategy: matrix: - go: [ '1.22', '1.21', '1.20' ] + go: [ '1.22', '1.21' ] include: - go: '1.22' lint: true @@ -54,11 +54,7 @@ jobs: run: go build -v ./... - name: Test - # We need GCC because of the "go test -race" - # env: - # CGO_ENABLED: 0 run: | - apt-get update && apt-get install gcc -y go test -parallel 20 -v -race -coverprofile=coverage.txt -covermode=atomic ./... bash <(curl -s https://codecov.io/bash) diff --git a/.gitignore b/.gitignore index e284d6cd..51cf6852 100644 --- a/.gitignore +++ b/.gitignore @@ -3,5 +3,5 @@ ftpserver .idea .vscode debug -__debug_bin +__debug_bin* *.toml \ No newline at end of file diff --git a/.golangci.yml b/.golangci.yml index 6db607f1..0535eb8d 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -160,61 +160,128 @@ linters-settings: linters: disable-all: true enable: + # https://golangci-lint.run/usage/linters/ - asciicheck + - asasalint + - bidichk - bodyclose # - deadcode -> unused + - containedctx + # - contextcheck -> creates an odd error here: https://github.com/fclairamb/ftpserverlib/blob/4d7c663e9e0b2650673fc2e0fcdb443895f2a1b9/server.go#L234 + # - copyloopvar -> unknown in v1.56.2 ??? + # - cyclop -> Delaying it for now (too much work) + - decorder - depguard - dogsled - dupl + - dupword + - durationcheck - errcheck - exhaustive + - errchkjson + - errname + - errorlint + - execinquery + - exhaustive + # - exhaustruct --> Not convinced it's useful # - exhaustivestruct - exportloopref - funlen - # - gci + - forbidigo + - forcetypeassert + - gci + - ginkgolinter - gochecknoinits - # - gochecknoglobals + - gochecksumtype + - gochecknoglobals - gocognit - goconst - gocritic - gocyclo + # - godot --> lots of not so useful changes - godox - goerr113 - gofmt + # - gofumpt -> conflicts with wsl - goimports + - gosimple # - golint --> revive - revive - # - gomnd + # - gomnd --> too much work + # - gomoddirectives + # - gomodguard - goprintffuncname - gosec + - gosmopolitan - gosimple - govet + - grouper - ineffassign + - importas + - inamedparam + # - intrange --> upcoming + # - interfacebloat # - interfacer --> (deprecated) + # - ireturn --> I can't even see how to fix those like ClientHandler::getFileHandle - lll + - loggercheck + - maintidx + - makezero + - mirror # - maligned --> govet:fieldalignment - megacheck - misspell + - musttag - nakedret # - nestif - nlreturn - prealloc + - nestif - nilerr + - nilnil - nolintlint + - nlreturn - rowserrcheck + - noctx + - nonamedreturns # - scopelint --> exportloopref + - nosprintfhostport - exportloopref - staticcheck # - structcheck -> unused - stylecheck + # - paralleltest -> buggy, doesn't work with subtests - typecheck + - perfsprint + - prealloc + - predeclared + - reassign + - promlinter + - protogetter + - rowserrcheck + - sloglint + - spancheck + - sqlclosecheck + - stylecheck + - tagalign + - tagliatelle + - tenv + - testableexamples + - testifylint + # - testpackage -> too late for that + - thelper + - tparallel - unconvert - unparam - unused + - usestdlibvars + - varnamelen + - wastedassign # - varcheck -> unused - whitespace - # - wrapcheck + # - wrapcheck -> too much effort for now - wsl + # - zerologlint -> Will most probably never use it fast: false issues: @@ -224,3 +291,7 @@ issues: # Default value for this option is true. exclude-use-default: false + exclude-rules: + - path: _test\.go + linters: + - gochecknoglobals diff --git a/README.md b/README.md index cdf1768c..4df112da 100644 --- a/README.md +++ b/README.md @@ -47,7 +47,7 @@ If you're interested in a fully featured FTP server, you should use [sftpgo](htt The easiest way to test this library is to use [ftpserver](https://github.com/fclairamb/ftpserver). ## The driver -The simplest way to get a good understanding of how the driver shall be implemented, you can have a look at the [tests driver](https://github.com/fclairamb/ftpserverlib/blob/master/driver_test.go). +The simplest way to get a good understanding of how the driver shall be implemented is to look at the [tests driver](https://github.com/fclairamb/ftpserverlib/blob/master/driver_test.go). ### The base API diff --git a/asciiconverter.go b/asciiconverter.go index 675aac82..69929714 100644 --- a/asciiconverter.go +++ b/asciiconverter.go @@ -10,6 +10,8 @@ type convertMode int8 const ( convertModeToCRLF convertMode = iota convertModeToLF + + bufferSize = 4096 ) type asciiConverter struct { @@ -19,7 +21,7 @@ type asciiConverter struct { } func newASCIIConverter(r io.Reader, mode convertMode) *asciiConverter { - reader := bufio.NewReaderSize(r, 4096) + reader := bufio.NewReaderSize(r, bufferSize) return &asciiConverter{ reader: reader, @@ -28,8 +30,10 @@ func newASCIIConverter(r io.Reader, mode convertMode) *asciiConverter { } } -func (c *asciiConverter) Read(p []byte) (n int, err error) { +func (c *asciiConverter) Read(bytes []byte) (int, error) { var data []byte + var readBytes int + var err error if len(c.remaining) > 0 { data = c.remaining @@ -37,21 +41,21 @@ func (c *asciiConverter) Read(p []byte) (n int, err error) { } else { data, _, err = c.reader.ReadLine() if err != nil { - return n, err + return readBytes, err } } - n = len(data) - if n > 0 { - maxSize := len(p) - 2 - if n > maxSize { - copy(p, data[:maxSize]) + readBytes = len(data) + if readBytes > 0 { + maxSize := len(bytes) - 2 + if readBytes > maxSize { + copy(bytes, data[:maxSize]) c.remaining = data[maxSize:] return maxSize, nil } - copy(p[:n], data[:n]) + copy(bytes[:readBytes], data[:readBytes]) } // we can have a partial read if the line is too long @@ -64,7 +68,7 @@ func (c *asciiConverter) Read(p []byte) (n int, err error) { // client transfers it in ASCII mode err = c.reader.UnreadByte() if err != nil { - return n, err + return readBytes, err } lastByte, err := c.reader.ReadByte() @@ -72,14 +76,14 @@ func (c *asciiConverter) Read(p []byte) (n int, err error) { if err == nil && lastByte == '\n' { switch c.mode { case convertModeToCRLF: - p[n] = '\r' - p[n+1] = '\n' - n += 2 + bytes[readBytes] = '\r' + bytes[readBytes+1] = '\n' + readBytes += 2 case convertModeToLF: - p[n] = '\n' - n++ + bytes[readBytes] = '\n' + readBytes++ } } - return n, err + return readBytes, err //nolint:wrapcheck // here wrapping errors brings nothing } diff --git a/asciiconverter_test.go b/asciiconverter_test.go index 0ba2ce81..cfe95da0 100644 --- a/asciiconverter_test.go +++ b/asciiconverter_test.go @@ -12,15 +12,15 @@ func TestASCIIConvert(t *testing.T) { lines := []byte("line1\r\nline2\r\n\r\nline4") src := bytes.NewBuffer(lines) dst := bytes.NewBuffer(nil) - c := newASCIIConverter(src, convertModeToLF) - _, err := io.Copy(dst, c) + converter := newASCIIConverter(src, convertModeToLF) + _, err := io.Copy(dst, converter) require.NoError(t, err) require.Equal(t, []byte("line1\nline2\n\nline4"), dst.Bytes()) lines = []byte("line1\nline2\n\nline4") dst = bytes.NewBuffer(nil) - c = newASCIIConverter(bytes.NewBuffer(lines), convertModeToCRLF) - _, err = io.Copy(dst, c) + converter = newASCIIConverter(bytes.NewBuffer(lines), convertModeToCRLF) + _, err = io.Copy(dst, converter) require.NoError(t, err) require.Equal(t, []byte("line1\r\nline2\r\n\r\nline4"), dst.Bytes()) @@ -31,8 +31,8 @@ func TestASCIIConvert(t *testing.T) { } dst = bytes.NewBuffer(nil) - c = newASCIIConverter(bytes.NewBuffer(buf), convertModeToCRLF) - _, err = io.Copy(dst, c) + converter = newASCIIConverter(bytes.NewBuffer(buf), convertModeToCRLF) + _, err = io.Copy(dst, converter) require.NoError(t, err) require.Equal(t, buf, dst.Bytes()) } diff --git a/client_handler.go b/client_handler.go index 18f22e12..011ddf6c 100644 --- a/client_handler.go +++ b/client_handler.go @@ -13,7 +13,7 @@ import ( log "github.com/fclairamb/go-log" ) -// HASHAlgo is the enumerable that represents the supported HASH algorithms +// HASHAlgo is the enumerable that represents the supported HASH algorithms. type HASHAlgo int8 // Supported hash algorithms @@ -25,7 +25,7 @@ const ( HASHAlgoSHA512 ) -// TransferType is the enumerable that represents the supported transfer types +// TransferType is the enumerable that represents the supported transfer types. type TransferType int8 // Supported transfer type @@ -111,21 +111,23 @@ type clientHandler struct { } // newClientHandler initializes a client handler when someone connects -func (server *FtpServer) newClientHandler(connection net.Conn, id uint32, transferType TransferType) *clientHandler { - p := &clientHandler{ +func (server *FtpServer) newClientHandler( + connection net.Conn, + clientID uint32, + transferType TransferType, +) *clientHandler { + return &clientHandler{ server: server, conn: connection, - id: id, + id: clientID, writer: bufio.NewWriter(connection), reader: bufio.NewReaderSize(connection, maxCommandSize), connectedAt: time.Now().UTC(), path: "/", selectedHashAlgo: HASHAlgoSHA256, currentTransferType: transferType, - logger: server.Logger.With("clientId", id), + logger: server.Logger.With("clientId", clientID), } - - return p } func (c *clientHandler) disconnect() { @@ -328,6 +330,10 @@ func (c *clientHandler) closeTransfer() error { } } + if err != nil { + err = fmt.Errorf("error closing transfer connection: %w", err) + } + return err } @@ -354,7 +360,12 @@ func (c *clientHandler) Close() error { // 2) the client could wait for another response and so we break the protocol // // closing the connection from a different goroutine should be safe - return c.conn.Close() + err := c.conn.Close() + if err != nil { + err = newNetworkError("error closing control connection", err) + } + + return err } func (c *clientHandler) end() { @@ -379,13 +390,11 @@ func (c *clientHandler) end() { } } -func (c *clientHandler) isCommandAborted() (aborted bool) { +func (c *clientHandler) isCommandAborted() bool { c.transferMu.Lock() defer c.transferMu.Unlock() - aborted = c.isTransferAborted - - return + return c.isTransferAborted } // HandleCommands reads the stream of commands @@ -401,57 +410,65 @@ func (c *clientHandler) HandleCommands() { } for { - if c.reader == nil { - if c.debug { - c.logger.Debug("Client disconnected", "clean", true) - } - + if c.readCommand() { return } + } +} - // florent(2018-01-14): #58: IDLE timeout: Preparing the deadline before we read - if c.server.settings.IdleTimeout > 0 { - if err := c.conn.SetDeadline( - time.Now().Add(time.Duration(time.Second.Nanoseconds() * int64(c.server.settings.IdleTimeout)))); err != nil { - c.logger.Error("Network error", "err", err) - } +func (c *clientHandler) readCommand() bool { + if c.reader == nil { + if c.debug { + c.logger.Debug("Client disconnected", "clean", true) } - lineSlice, isPrefix, err := c.reader.ReadLine() - - if isPrefix { - if c.debug { - c.logger.Warn("Received line too long, disconnecting client", - "size", len(lineSlice)) - } + return true + } - return + // florent(2018-01-14): #58: IDLE timeout: Preparing the deadline before we read + if c.server.settings.IdleTimeout > 0 { + if err := c.conn.SetDeadline( + time.Now().Add(time.Duration(time.Second.Nanoseconds() * int64(c.server.settings.IdleTimeout)))); err != nil { + c.logger.Error("Network error", "err", err) } + } - if err != nil { - c.handleCommandsStreamError(err) + lineSlice, isPrefix, err := c.reader.ReadLine() - return + if isPrefix { + if c.debug { + c.logger.Warn("Received line too long, disconnecting client", + "size", len(lineSlice)) } - line := string(lineSlice) + return true + } - if c.debug { - c.logger.Debug("Received line", "line", line) - } + if err != nil { + c.handleCommandsStreamError(err) - c.handleCommand(line) + return true } + + line := string(lineSlice) + + if c.debug { + c.logger.Debug("Received line", "line", line) + } + + c.handleCommand(line) + + return false } func (c *clientHandler) handleCommandsStreamError(err error) { // florent(2018-01-14): #58: IDLE timeout: Adding some code to deal with the deadline - switch err := err.(type) { - case net.Error: - if err.Timeout() { + var errNetError net.Error + if errors.As(err, &errNetError) { //nolint:nestif // too much effort to change for now + if errNetError.Timeout() { // We have to extend the deadline now - if err := c.conn.SetDeadline(time.Now().Add(time.Minute)); err != nil { - c.logger.Error("Could not set read deadline", "err", err) + if errSet := c.conn.SetDeadline(time.Now().Add(time.Minute)); errSet != nil { + c.logger.Error("Could not set read deadline", "err", errSet) } c.logger.Info("Client IDLE timeout", "err", err) @@ -459,16 +476,16 @@ func (c *clientHandler) handleCommandsStreamError(err error) { StatusServiceNotAvailable, fmt.Sprintf("command timeout (%d seconds): closing control connection", c.server.settings.IdleTimeout)) - if err := c.writer.Flush(); err != nil { - c.logger.Error("Flush error", "err", err) + if errFlush := c.writer.Flush(); errFlush != nil { + c.logger.Error("Flush error", "err", errFlush) } - break + return } c.logger.Error("Network error", "err", err) - default: - if err == io.EOF { + } else { + if errors.Is(err, io.EOF) { if c.debug { c.logger.Debug("Client disconnected", "clean", false) } @@ -641,6 +658,8 @@ func (c *clientHandler) TransferOpen(info string) (net.Conn, error) { c.writeMessage(StatusCannotOpenDataConnection, err.Error()) + err = newNetworkError("Unable to open transfer", err) + return nil, err } @@ -656,7 +675,7 @@ func (c *clientHandler) TransferOpen(info string) (net.Conn, error) { "localAddr", conn.LocalAddr().String()) } - return conn, err + return conn, nil } func (c *clientHandler) TransferClose(err error) { @@ -725,14 +744,14 @@ func getIPFromRemoteAddr(remoteAddr net.Addr) (net.IP, error) { return nil, &ipValidationError{error: "nil remote address"} } - ip, _, err := net.SplitHostPort(remoteAddr.String()) + ipAddress, _, err := net.SplitHostPort(remoteAddr.String()) if err != nil { - return nil, err + return nil, fmt.Errorf("error parsing remote address: %w", err) } - remoteIP := net.ParseIP(ip) + remoteIP := net.ParseIP(ipAddress) if remoteIP == nil { - return nil, &ipValidationError{error: fmt.Sprintf("invalid remote IP: %v", ip)} + return nil, &ipValidationError{error: fmt.Sprintf("invalid remote IP: %v", ipAddress)} } return remoteIP, nil diff --git a/client_handler_test.go b/client_handler_test.go index ae8738c6..56c74957 100644 --- a/client_handler_test.go +++ b/client_handler_test.go @@ -27,18 +27,16 @@ func TestConcurrency(t *testing.T) { Password: authPass, } - var err error - var c *goftp.Client - - if c, err = goftp.DialConfig(conf, server.Addr()); err != nil { + client, err := goftp.DialConfig(conf, server.Addr()) + if err != nil { panic(fmt.Sprintf("Couldn't connect: %v", err)) } - if _, err = c.ReadDir("/"); err != nil { + if _, err = client.ReadDir("/"); err != nil { panic(fmt.Sprintf("Couldn't list dir: %v", err)) } - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() waitGroup.Done() }() @@ -48,8 +46,8 @@ func TestConcurrency(t *testing.T) { } func TestDOS(t *testing.T) { - s := NewTestServer(t, true) - conn, err := net.DialTimeout("tcp", s.Addr(), 5*time.Second) + server := NewTestServer(t, true) + conn, err := net.DialTimeout("tcp", server.Addr(), 5*time.Second) require.NoError(t, err) defer func() { @@ -58,24 +56,24 @@ func TestDOS(t *testing.T) { }() buf := make([]byte, 128) - n, err := conn.Read(buf) + readBytes, err := conn.Read(buf) require.NoError(t, err) - response := string(buf[:n]) + response := string(buf[:readBytes]) require.Equal(t, "220 TEST Server\r\n", response) written := 0 for { - n, err = conn.Write([]byte("some text without line ending")) - written += n + readBytes, err = conn.Write([]byte("some text without line ending")) + written += readBytes if err != nil { break } if written > 4096 { - s.Logger.Warn("test DOS", + server.Logger.Warn("test DOS", "bytes written", written) } } @@ -92,18 +90,18 @@ func TestLastDataChannel(t *testing.T) { } func TestTransferOpenError(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() @@ -116,7 +114,10 @@ func TestTransferOpenError(t *testing.T) { } func TestTLSMethods(t *testing.T) { + t.Parallel() + t.Run("without-tls", func(t *testing.T) { + t.Parallel() cc := clientHandler{ server: NewTestServer(t, false), } @@ -125,7 +126,8 @@ func TestTLSMethods(t *testing.T) { }) t.Run("with-implicit-tls", func(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + t.Parallel() + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Settings: &Settings{ TLSRequired: ImplicitEncryption, }, @@ -133,7 +135,7 @@ func TestTLSMethods(t *testing.T) { Debug: false, }) cc := clientHandler{ - server: s, + server: server, } require.True(t, cc.HasTLSForControl()) require.True(t, cc.HasTLSForTransfers()) @@ -173,24 +175,24 @@ func TestCloseConnection(t *testing.T) { driver := &TestServerDriver{ Debug: false, } - s := NewTestServerWithTestDriver(t, driver) + server := NewTestServerWithTestDriver(t, driver) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - ftpUpload(t, c, createTemporaryFile(t, 1024*1024), "file.bin") + ftpUpload(t, client, createTemporaryFile(t, 1024*1024), "file.bin") require.Len(t, driver.GetClientsInfo(), 1) - err = c.Rename("file.bin", "delay-io.bin") + err = client.Rename("file.bin", "delay-io.bin") require.NoError(t, err) - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err) defer func() { require.NoError(t, raw.Close()) }() @@ -214,30 +216,30 @@ func TestCloseConnection(t *testing.T) { func TestClientContextConcurrency(t *testing.T) { driver := &TestServerDriver{} - s := NewTestServerWithTestDriver(t, driver) + server := NewTestServerWithTestDriver(t, driver) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() done := make(chan bool, 1) connected := make(chan bool, 1) go func() { - _, err := c.Getwd() + _, err := client.Getwd() assert.NoError(t, err) connected <- true counter := 0 for counter < 100 { - _, err := c.Getwd() + _, err := client.Getwd() assert.NoError(t, err) counter++ @@ -323,13 +325,13 @@ func isStringInSlice(s string, list []string) bool { } func TestUnknownCommand(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + c, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") defer func() { panicOnError(c.Close()) }() @@ -351,12 +353,12 @@ type testNetConn struct { remoteAddr net.Addr } -func (*testNetConn) Read(_ []byte) (n int, err error) { - return +func (*testNetConn) Read(_ []byte) (int, error) { + return 0, nil } -func (*testNetConn) Write(_ []byte) (n int, err error) { - return +func (*testNetConn) Write(_ []byte) (int, error) { + return 0, nil } func (*testNetConn) Close() error { @@ -405,9 +407,10 @@ func (*testNetListener) Addr() net.Addr { } func TestDataConnectionRequirements(t *testing.T) { + req := require.New(t) controlConnIP := net.ParseIP("192.168.1.1") - c := clientHandler{ + cltHandler := clientHandler{ conn: &testNetConn{ remoteAddr: &net.TCPAddr{IP: controlConnIP, Port: 21}, }, @@ -419,39 +422,39 @@ func TestDataConnectionRequirements(t *testing.T) { }, } - err := c.checkDataConnectionRequirement(controlConnIP, DataChannelPassive) - assert.NoError(t, err) // ip match + err := cltHandler.checkDataConnectionRequirement(controlConnIP, DataChannelPassive) + req.NoError(err) // ip match - err = c.checkDataConnectionRequirement(net.ParseIP("192.168.1.2"), DataChannelActive) + err = cltHandler.checkDataConnectionRequirement(net.ParseIP("192.168.1.2"), DataChannelActive) if assert.Error(t, err) { assert.Contains(t, err.Error(), "does not match control connection ip address") } - c.conn = &testNetConn{ + cltHandler.conn = &testNetConn{ remoteAddr: &net.IPAddr{IP: controlConnIP}, } - err = c.checkDataConnectionRequirement(controlConnIP, DataChannelPassive) - assert.Error(t, err) + err = cltHandler.checkDataConnectionRequirement(controlConnIP, DataChannelPassive) + req.Error(err) // nil remote address - c.conn = &testNetConn{} - err = c.checkDataConnectionRequirement(controlConnIP, DataChannelActive) - assert.Error(t, err) + cltHandler.conn = &testNetConn{} + err = cltHandler.checkDataConnectionRequirement(controlConnIP, DataChannelActive) + req.Error(err) // invalid IP - c.conn = &testNetConn{ + cltHandler.conn = &testNetConn{ remoteAddr: &net.TCPAddr{IP: nil, Port: 21}, } - err = c.checkDataConnectionRequirement(controlConnIP, DataChannelPassive) + err = cltHandler.checkDataConnectionRequirement(controlConnIP, DataChannelPassive) if assert.Error(t, err) { assert.Contains(t, err.Error(), "invalid remote IP") } // invalid setting - c.server.settings.PasvConnectionsCheck = 100 - err = c.checkDataConnectionRequirement(controlConnIP, DataChannelPassive) + cltHandler.server.settings.PasvConnectionsCheck = 100 + err = cltHandler.checkDataConnectionRequirement(controlConnIP, DataChannelPassive) if assert.Error(t, err) { assert.Contains(t, err.Error(), "unhandled data connection requirement") @@ -462,14 +465,14 @@ func TestExtraData(t *testing.T) { driver := &TestServerDriver{ Debug: false, } - s := NewTestServerWithTestDriver(t, driver) + server := NewTestServerWithTestDriver(t, driver) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + c, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") defer func() { panicOnError(c.Close()) }() diff --git a/control_unix.go b/control_unix.go index 6ab45323..c15f19c9 100644 --- a/control_unix.go +++ b/control_unix.go @@ -4,6 +4,7 @@ package ftpserver import ( + "fmt" "syscall" "golang.org/x/sys/unix" @@ -13,19 +14,23 @@ import ( func Control(_, _ string, c syscall.RawConn) error { var errSetOpts error - err := c.Control(func(fd uintptr) { - errSetOpts = unix.SetsockoptInt(int(fd), unix.SOL_SOCKET, unix.SO_REUSEADDR, 1) + err := c.Control(func(unixFd uintptr) { + errSetOpts = unix.SetsockoptInt(int(unixFd), unix.SOL_SOCKET, unix.SO_REUSEADDR, 1) if errSetOpts != nil { return } - errSetOpts = unix.SetsockoptInt(int(fd), unix.SOL_SOCKET, unix.SO_REUSEPORT, 1) + errSetOpts = unix.SetsockoptInt(int(unixFd), unix.SOL_SOCKET, unix.SO_REUSEPORT, 1) if errSetOpts != nil { return } }) if err != nil { - return err + return fmt.Errorf("unable to set control options: %w", err) + } + + if errSetOpts != nil { + errSetOpts = fmt.Errorf("unable to set control options: %w", errSetOpts) } return errSetOpts diff --git a/driver.go b/driver.go index 7229b957..80316a77 100644 --- a/driver.go +++ b/driver.go @@ -78,23 +78,12 @@ type ClientDriver interface { // ClientDriverExtensionAllocate is an extension to support the "ALLO" - file allocation - command type ClientDriverExtensionAllocate interface { - // AllocateSpace reserves the space necessary to upload files AllocateSpace(size int) error } -/* -// ClientDriverExtensionChown is an extension to support the "CHOWN" - owner change - command -type ClientDriverExtensionChown interface { - - // Chown changes the owner of a file - Chown(name string, user string, group string) error -} -*/ - // ClientDriverExtensionSymlink is an extension to support the "SITE SYMLINK" - symbolic link creation - command type ClientDriverExtensionSymlink interface { - // Symlink creates a symlink Symlink(oldname, newname string) error @@ -105,7 +94,6 @@ type ClientDriverExtensionSymlink interface { // ClientDriverExtensionFileList is a convenience extension to allow to return file listing // without requiring to implement the methods Open/Readdir for your custom afero.File type ClientDriverExtensionFileList interface { - // ReadDir reads the directory named by name and return a list of directory entries. ReadDir(name string) ([]os.FileInfo, error) } @@ -113,7 +101,6 @@ type ClientDriverExtensionFileList interface { // ClientDriverExtentionFileTransfer is a convenience extension to allow to transfer files // without requiring to implement the methods Create/Open/OpenFile for your custom afero.File. type ClientDriverExtentionFileTransfer interface { - // GetHandle return an handle to upload or download a file based on flags: // os.O_RDONLY indicates a download // os.O_WRONLY indicates an upload and can be combined with os.O_APPEND (resume) or diff --git a/driver_test.go b/driver_test.go index 2365a1b3..1e11572d 100644 --- a/driver_test.go +++ b/driver_test.go @@ -39,6 +39,8 @@ var errInvalidTLSCertificate = errors.New("invalid TLS certificate") // NewTestServer provides a test server with or without debugging func NewTestServer(t *testing.T, debug bool) *FtpServer { + t.Helper() + return NewTestServerWithTestDriver(t, &TestServerDriver{Debug: debug}) } @@ -55,7 +57,7 @@ func (driver *TestServerDriver) Init() { { dir, _ := os.MkdirTemp("", "example") - if err := os.MkdirAll(dir, 0750); err != nil { + if err := os.MkdirAll(dir, 0o750); err != nil { panic(err) } @@ -64,7 +66,7 @@ func (driver *TestServerDriver) Init() { } func NewTestServerWithTestDriver(t *testing.T, driver *TestServerDriver) *FtpServer { - t.Parallel() + t.Helper() driver.Init() @@ -86,30 +88,34 @@ func NewTestServerWithTestDriver(t *testing.T, driver *TestServerDriver) *FtpSer // NewTestServerWithTestDriver provides a server instantiated with some settings func NewTestServerWithDriverAndLogger(t *testing.T, driver MainDriver, logger log.Logger) *FtpServer { - s := NewFtpServer(driver) + t.Helper() + + server := NewFtpServer(driver) if logger != nil { - s.Logger = logger + server.Logger = logger } - if err := s.Listen(); err != nil { + if err := server.Listen(); err != nil { return nil } t.Cleanup(func() { - mustStopServer(s) + mustStopServer(server) }) go func() { - if err := s.Serve(); err != nil && err != io.EOF { - s.Logger.Error("problem serving", "err", err) + if err := server.Serve(); err != nil && errors.Is(err, io.EOF) { + server.Logger.Error("problem serving", "err", err) } }() - return s + return server } func NewTestServerWithDriver(t *testing.T, driver MainDriver) *FtpServer { + t.Helper() + return NewTestServerWithDriverAndLogger(t, driver, nil) } @@ -146,16 +152,16 @@ var ( errFailOpen = errors.New("couldn't open") ) -func (f *testFile) Read(b []byte) (int, error) { +func (f *testFile) Read(out []byte) (int, error) { // simulating a slow reading allows us to test ABOR if strings.Contains(f.File.Name(), "delay-io") { time.Sleep(500 * time.Millisecond) } - return f.File.Read(b) + return f.File.Read(out) } -func (f *testFile) Write(b []byte) (int, error) { +func (f *testFile) Write(out []byte) (int, error) { if strings.Contains(f.File.Name(), "fail-to-write") { return 0, errFailWrite } @@ -165,7 +171,7 @@ func (f *testFile) Write(b []byte) (int, error) { time.Sleep(500 * time.Millisecond) } - return f.File.Write(b) + return f.File.Write(out) } func (f *testFile) Close() error { @@ -226,7 +232,7 @@ func mustStopServer(server *FtpServer) { var errConnectionNotAllowed = errors.New("connection not allowed") // ClientConnected is the very first message people will see -func (driver *TestServerDriver) ClientConnected(cc ClientContext) (string, error) { +func (driver *TestServerDriver) ClientConnected(cltContext ClientContext) (string, error) { driver.clientMU.Lock() defer driver.clientMU.Unlock() @@ -236,10 +242,10 @@ func (driver *TestServerDriver) ClientConnected(cc ClientContext) (string, error err = errConnectionNotAllowed } - cc.SetDebug(driver.Debug) + cltContext.SetDebug(driver.Debug) // we set the client id as extra data just for testing - cc.SetExtra(cc.ID()) - driver.Clients = append(driver.Clients, cc) + cltContext.SetExtra(cltContext.ID()) + driver.Clients = append(driver.Clients, cltContext) // This will remain the official name for now return "TEST Server", err } @@ -254,7 +260,7 @@ func (driver *TestServerDriver) AuthUser(_ ClientContext, user, pass string) (Cl return clientdriver, nil } else if user == "nil" && pass == "nil" { // Definitely a bad behavior (but can be done on the driver side) - return nil, nil + return nil, nil //nolint:nilnil } return nil, errBadUserNameOrPassword @@ -302,20 +308,20 @@ func (driver *TestServerDriver) GetClientsInfo() map[uint32]interface{} { info := make(map[uint32]interface{}) - for _, cc := range driver.Clients { + for _, clientContext := range driver.Clients { ccInfo := make(map[string]interface{}) - ccInfo["localAddr"] = cc.LocalAddr() - ccInfo["remoteAddr"] = cc.RemoteAddr() - ccInfo["clientVersion"] = cc.GetClientVersion() - ccInfo["path"] = cc.Path() - ccInfo["hasTLSForControl"] = cc.HasTLSForControl() - ccInfo["hasTLSForTransfers"] = cc.HasTLSForTransfers() - ccInfo["lastCommand"] = cc.GetLastCommand() - ccInfo["debug"] = cc.Debug() - ccInfo["extra"] = cc.Extra() + ccInfo["localAddr"] = clientContext.LocalAddr() + ccInfo["remoteAddr"] = clientContext.RemoteAddr() + ccInfo["clientVersion"] = clientContext.GetClientVersion() + ccInfo["path"] = clientContext.Path() + ccInfo["hasTLSForControl"] = clientContext.HasTLSForControl() + ccInfo["hasTLSForTransfers"] = clientContext.HasTLSForTransfers() + ccInfo["lastCommand"] = clientContext.GetLastCommand() + ccInfo["debug"] = clientContext.Debug() + ccInfo["extra"] = clientContext.Extra() - info[cc.ID()] = ccInfo + info[clientContext.ID()] = ccInfo } return info @@ -364,7 +370,8 @@ func (driver *TestServerDriver) PreAuthUser(cc ClientContext, _ string) error { } func (driver *TestServerDriver) VerifyConnection(_ ClientContext, _ string, - _ *tls.Conn) (ClientDriver, error) { + _ *tls.Conn, +) (ClientDriver, error) { switch driver.TLSVerificationReply { case tlsVerificationFailed: return nil, errInvalidTLSCertificate @@ -373,10 +380,10 @@ func (driver *TestServerDriver) VerifyConnection(_ ClientContext, _ string, return clientdriver, nil case tlsVerificationOK: - return nil, nil + return nil, nil //nolint:nilnil } - return nil, nil + return nil, nil //nolint:nilnil } func (driver *TestServerDriver) WrapPassiveListener(listener net.Listener) (net.Listener, error) { @@ -452,8 +459,10 @@ func (driver *TestClientDriver) GetAvailableSpace(dirName string) (int64, error) return int64(123), nil } -var errInvalidChownUser = errors.New("invalid chown on user") -var errInvalidChownGroup = errors.New("invalid chown on group") +var ( + errInvalidChownUser = errors.New("invalid chown on user") + errInvalidChownGroup = errors.New("invalid chown on group") +) func (driver *TestClientDriver) Chown(name string, uid int, gid int) error { if uid != 0 && uid != authUserID { diff --git a/errors.go b/errors.go index 8023d9e0..6319bba5 100644 --- a/errors.go +++ b/errors.go @@ -1,6 +1,9 @@ package ftpserver -import "errors" +import ( + "errors" + "fmt" +) var ( // ErrStorageExceeded defines the error mapped to the FTP 552 reply code. @@ -21,3 +24,57 @@ func getErrorCode(err error, defaultCode int) int { return defaultCode } } + +// DriverError is a wrapper is for any error that occur while contacting the drivers +type DriverError struct { + str string + err error +} + +func newDriverError(str string, err error) DriverError { + return DriverError{str: str, err: err} +} + +func (e DriverError) Error() string { + return fmt.Sprintf("driver error: %s: %v", e.str, e.err) +} + +func (e DriverError) Unwrap() error { + return e.err +} + +// NetworkError is a wrapper for any error that occur while contacting the network +type NetworkError struct { + str string + err error +} + +func newNetworkError(str string, err error) NetworkError { + return NetworkError{str: str, err: err} +} + +func (e NetworkError) Error() string { + return fmt.Sprintf("network error: %s: %v", e.str, e.err) +} + +func (e NetworkError) Unwrap() error { + return e.err +} + +// FileAccessError is a wrapper for any error that occur while accessing the file system +type FileAccessError struct { + str string + err error +} + +func newFileAccessError(str string, err error) FileAccessError { + return FileAccessError{str: str, err: err} +} + +func (e FileAccessError) Error() string { + return fmt.Sprintf("file access error: %s: %v", e.str, e.err) +} + +func (e FileAccessError) Unwrap() error { + return e.err +} diff --git a/errors_test.go b/errors_test.go index 20b70f14..f789ac16 100644 --- a/errors_test.go +++ b/errors_test.go @@ -27,3 +27,38 @@ func TestTransferCloseStorageExceeded(t *testing.T) { h.TransferClose(ErrStorageExceeded) require.Equal(t, "552 Issue during transfer: storage limit exceeded\r\n", buf.String()) } + +func TestErrorTypes(t *testing.T) { + // a := assert.New(t) + t.Run("DriverError", func(t *testing.T) { + req := require.New(t) + var err error = newDriverError("test", os.ErrPermission) + req.Equal("driver error: test: permission denied", err.Error()) + req.ErrorIs(err, os.ErrPermission) + + var specificError DriverError + req.ErrorAs(err, &specificError) + req.Equal("test", specificError.str) + }) + + t.Run("NetworkError", func(t *testing.T) { + req := require.New(t) + var err error = newNetworkError("test", os.ErrPermission) + req.Equal("network error: test: permission denied", err.Error()) + req.ErrorIs(err, os.ErrPermission) + + var specificError NetworkError + req.ErrorAs(err, &specificError) + req.Equal("test", specificError.str) + }) + + t.Run("FileAccessError", func(t *testing.T) { + req := require.New(t) + var err error = newFileAccessError("test", os.ErrPermission) + req.Equal("file access error: test: permission denied", err.Error()) + req.ErrorIs(err, os.ErrPermission) + var specificError FileAccessError + req.ErrorAs(err, &specificError) + req.Equal("test", specificError.str) + }) +} diff --git a/handle_auth.go b/handle_auth.go index bcb4d3d5..7c200abd 100644 --- a/handle_auth.go +++ b/handle_auth.go @@ -6,10 +6,9 @@ import ( ) // Handle the "USER" command -func (c *clientHandler) handleUSER(param string) error { +func (c *clientHandler) handleUSER(user string) error { if verifier, ok := c.server.driver.(MainDriverExtensionUserVerifier); ok { - err := verifier.PreAuthUser(c, param) - + err := verifier.PreAuthUser(c, user) if err != nil { c.writeMessage(StatusNotLoggedIn, fmt.Sprintf("User rejected: %v", err)) c.disconnect() @@ -26,32 +25,47 @@ func (c *clientHandler) handleUSER(param string) error { } if c.HasTLSForControl() { - if verifier, ok := c.server.driver.(MainDriverExtensionTLSVerifier); ok { - if tlsConn, ok := c.conn.(*tls.Conn); ok { - driver, err := verifier.VerifyConnection(c, param, tlsConn) + if c.handleUserTLS(user) { + return nil + } + } - if err != nil { - c.writeMessage(StatusNotLoggedIn, fmt.Sprintf("TLS verification failed: %v", err)) - c.disconnect() + c.user = user + c.writeMessage(StatusUserOK, "OK") - return nil - } + return nil +} - if driver != nil { - c.user = param - c.driver = driver - c.writeMessage(StatusUserLoggedIn, "TLS certificate ok, continue") +func (c *clientHandler) handleUserTLS(user string) bool { + verifier, interfaceFound := c.server.driver.(MainDriverExtensionTLSVerifier) - return nil - } - } - } + if !interfaceFound { + return false } - c.user = param - c.writeMessage(StatusUserOK, "OK") + tlsConn, interfaceFound := c.conn.(*tls.Conn) - return nil + if !interfaceFound { + return false + } + + driver, err := verifier.VerifyConnection(c, user, tlsConn) + if err != nil { + c.writeMessage(StatusNotLoggedIn, fmt.Sprintf("TLS verification failed: %v", err)) + c.disconnect() + + return true + } + + if driver != nil { + c.user = user + c.driver = driver + c.writeMessage(StatusUserLoggedIn, "TLS certificate ok, continue") + + return true + } + + return false } // Handle the "PASS" command diff --git a/handle_auth_test.go b/handle_auth_test.go index 5186a4ca..feae7899 100644 --- a/handle_auth_test.go +++ b/handle_auth_test.go @@ -17,9 +17,9 @@ func panicOnError(err error) { } func TestLoginSuccess(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) // send a NOOP before the login, this doesn't seems possible using secsy/goftp so use the old way ... - conn, err := net.DialTimeout("tcp", s.Addr(), 5*time.Second) + conn, err := net.DialTimeout("tcp", server.Addr(), 5*time.Second) require.NoError(t, err) defer func() { @@ -28,19 +28,19 @@ func TestLoginSuccess(t *testing.T) { }() buf := make([]byte, 1024) - n, err := conn.Read(buf) + readBytes, err := conn.Read(buf) require.NoError(t, err) - response := string(buf[:n]) + response := string(buf[:readBytes]) require.Equal(t, "220 TEST Server\r\n", response) _, err = conn.Write([]byte("NOOP\r\n")) require.NoError(t, err) - n, err = conn.Read(buf) + readBytes, err = conn.Read(buf) require.NoError(t, err) - response = string(buf[:n]) + response = string(buf[:readBytes]) require.Equal(t, "200 OK\r\n", response) conf := goftp.Config{ @@ -48,7 +48,7 @@ func TestLoginSuccess(t *testing.T) { Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + c, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") defer func() { panicOnError(c.Close()) }() @@ -58,78 +58,78 @@ func TestLoginSuccess(t *testing.T) { defer func() { require.NoError(t, raw.Close()) }() - rc, _, err := raw.SendCommand("NOOP") + returnCode, _, err := raw.SendCommand("NOOP") require.NoError(t, err) - require.Equal(t, StatusOK, rc, "Couldn't NOOP") + require.Equal(t, StatusOK, returnCode, "Couldn't NOOP") - rc, response, err = raw.SendCommand("SYST") + returnCode, response, err = raw.SendCommand("SYST") require.NoError(t, err) - require.Equal(t, StatusSystemType, rc) + require.Equal(t, StatusSystemType, returnCode) require.Equal(t, "UNIX Type: L8", response) - s.settings.DisableSYST = true - rc, response, err = raw.SendCommand("SYST") + server.settings.DisableSYST = true + returnCode, response, err = raw.SendCommand("SYST") require.NoError(t, err) - require.Equal(t, StatusCommandNotImplemented, rc, response) + require.Equal(t, StatusCommandNotImplemented, returnCode, response) } func TestLoginFailure(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass + "_wrong", } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - _, err = c.OpenRawConn() + _, err = client.OpenRawConn() require.Error(t, err, "We should have failed to login") } func TestLoginCustom(t *testing.T) { + req := require.New(t) driver := &MesssageDriver{} driver.Init() - s := NewTestServerWithDriver(t, driver) - r := require.New(t) + server := NewTestServerWithDriver(t, driver) conf := goftp.Config{ User: authUser, Password: authPass + "_wrong", } - c, err := goftp.DialConfig(conf, s.Addr()) - r.NoError(err, "Couldn't connect") + client, err := goftp.DialConfig(conf, server.Addr()) + req.NoError(err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - _, err = c.OpenRawConn() - r.Error(err, "We should have failed to login") + _, err = client.OpenRawConn() + req.Error(err, "We should have failed to login") } func TestLoginNil(t *testing.T) { - s := NewTestServer(t, true) - r := require.New(t) + server := NewTestServer(t, true) + req := require.New(t) conf := goftp.Config{ User: "nil", Password: "nil", } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - _, err = c.OpenRawConn() - r.Error(err) + _, err = client.OpenRawConn() + req.Error(err) } func TestAuthTLS(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Debug: false, TLS: true, }) @@ -144,12 +144,12 @@ func TestAuthTLS(t *testing.T) { TLSMode: goftp.TLSExplicit, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't upgrade connection to TLS") err = raw.Close() @@ -157,7 +157,7 @@ func TestAuthTLS(t *testing.T) { } func TestAuthExplicitTLSFailure(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, @@ -169,7 +169,7 @@ func TestAuthExplicitTLSFailure(t *testing.T) { TLSMode: goftp.TLSExplicit, } - c, err := goftp.DialConfig(conf, s.Addr()) + c, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") defer func() { panicOnError(c.Close()) }() @@ -179,23 +179,23 @@ func TestAuthExplicitTLSFailure(t *testing.T) { } func TestAuthTLSRequired(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Debug: false, TLS: true, }) - s.settings.TLSRequired = MandatoryEncryption + server.settings.TLSRequired = MandatoryEncryption conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - _, err = c.OpenRawConn() + _, err = client.OpenRawConn() require.Error(t, err, "Plain text login must fail, TLS is required") require.EqualError(t, err, "unexpected response: 421-TLS is required") @@ -205,10 +205,10 @@ func TestAuthTLSRequired(t *testing.T) { } conf.TLSMode = goftp.TLSExplicit - c, err = goftp.DialConfig(conf, s.Addr()) + client, err = goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() @@ -219,7 +219,7 @@ func TestAuthTLSRequired(t *testing.T) { } func TestAuthTLSVerificationFailed(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Debug: true, TLS: true, TLSVerificationReply: tlsVerificationFailed, @@ -235,17 +235,17 @@ func TestAuthTLSVerificationFailed(t *testing.T) { TLSMode: goftp.TLSExplicit, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - _, err = c.OpenRawConn() + _, err = client.OpenRawConn() require.Error(t, err, "TLS verification shoul fail") } func TestAuthTLSCertificate(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Debug: true, TLS: true, TLSVerificationReply: tlsVerificationAuthenticated, @@ -260,7 +260,7 @@ func TestAuthTLSCertificate(t *testing.T) { TLSMode: goftp.TLSExplicit, } - c, err := goftp.DialConfig(conf, s.Addr()) + c, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") defer func() { panicOnError(c.Close()) }() @@ -276,7 +276,7 @@ func TestAuthTLSCertificate(t *testing.T) { } func TestAuthPerClientTLSRequired(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Debug: true, TLS: true, TLSRequirement: MandatoryEncryption, @@ -287,12 +287,12 @@ func TestAuthPerClientTLSRequired(t *testing.T) { Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - _, err = c.OpenRawConn() + _, err = client.OpenRawConn() require.Error(t, err, "Plain text login must fail, TLS is required") require.EqualError(t, err, "unexpected response: 421-TLS is required") @@ -301,10 +301,10 @@ func TestAuthPerClientTLSRequired(t *testing.T) { } conf.TLSMode = goftp.TLSExplicit - c, err = goftp.DialConfig(conf, s.Addr()) + client, err = goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() @@ -315,7 +315,7 @@ func TestAuthPerClientTLSRequired(t *testing.T) { } func TestUserVerifierError(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Debug: false, TLS: true, // setting an invalid TLS requirement will cause the test driver @@ -332,7 +332,7 @@ func TestUserVerifierError(t *testing.T) { TLSMode: goftp.TLSExplicit, } - c, err := goftp.DialConfig(conf, s.Addr()) + c, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") defer func() { panicOnError(c.Close()) }() diff --git a/handle_dirs.go b/handle_dirs.go index a832f457..d3473e58 100644 --- a/handle_dirs.go +++ b/handle_dirs.go @@ -17,7 +17,7 @@ import ( var errFileList = errors.New("listing a file isn't allowed") // the order matter, put parameters with more characters first -var supportedlistArgs = []string{"-al", "-la", "-a", "-l"} +var supportedlistArgs = []string{"-al", "-la", "-a", "-l"} //nolint:gochecknoglobals func (c *clientHandler) absPath(p string) string { if path.IsAbs(p) { @@ -29,44 +29,44 @@ func (c *clientHandler) absPath(p string) string { // getRelativePath returns the specified path as relative to the // current working directory. The specified path must be cleaned -func (c *clientHandler) getRelativePath(p string) string { - var sb strings.Builder +func (c *clientHandler) getRelativePath(inputPath string) string { + var builder strings.Builder base := c.Path() for { - if base == p { - return sb.String() + if base == inputPath { + return builder.String() } if !strings.HasSuffix(base, "/") { base += "/" } - if strings.HasPrefix(p, base) { - sb.WriteString(strings.TrimPrefix(p, base)) + if strings.HasPrefix(inputPath, base) { + builder.WriteString(strings.TrimPrefix(inputPath, base)) - return sb.String() + return builder.String() } if base == "/" || base == "./" { - return p + return inputPath } - sb.WriteString("../") + builder.WriteString("../") base = path.Dir(path.Clean(base)) } } func (c *clientHandler) handleCWD(param string) error { - p := c.absPath(param) + pathAbsolute := c.absPath(param) - if stat, err := c.driver.Stat(p); err == nil { + if stat, err := c.driver.Stat(pathAbsolute); err == nil { if stat.IsDir() { - c.SetPath(p) - c.writeMessage(StatusFileOK, fmt.Sprintf("CD worked on %s", p)) + c.SetPath(pathAbsolute) + c.writeMessage(StatusFileOK, "CD worked on "+pathAbsolute) } else { - c.writeMessage(StatusActionNotTaken, fmt.Sprintf("Can't change directory to %s: Not a Directory", p)) + c.writeMessage(StatusActionNotTaken, fmt.Sprintf("Can't change directory to %s: Not a Directory", pathAbsolute)) } } else { c.writeMessage(StatusActionNotTaken, fmt.Sprintf("CD issue: %v", err)) @@ -76,13 +76,13 @@ func (c *clientHandler) handleCWD(param string) error { } func (c *clientHandler) handleMKD(param string) error { - p := c.absPath(param) - if err := c.driver.Mkdir(p, 0755); err == nil { + pathAbsolute := c.absPath(param) + if err := c.driver.Mkdir(pathAbsolute, 0o755); err == nil { // handleMKD confirms to "quote-doubling" // https://tools.ietf.org/html/rfc959 , page 63 - c.writeMessage(StatusPathCreated, fmt.Sprintf(`Created dir "%s"`, quoteDoubling(p))) + c.writeMessage(StatusPathCreated, fmt.Sprintf(`Created dir "%s"`, quoteDoubling(pathAbsolute))) } else { - c.writeMessage(StatusActionNotTaken, fmt.Sprintf(`Could not create "%s" : %v`, quoteDoubling(p), err)) + c.writeMessage(StatusActionNotTaken, fmt.Sprintf(`Could not create "%s" : %v`, quoteDoubling(pathAbsolute), err)) } return nil @@ -97,8 +97,8 @@ func (c *clientHandler) handleMKDIR(params string) { p := c.absPath(params) - if err := c.driver.MkdirAll(p, 0755); err == nil { - c.writeMessage(StatusFileOK, fmt.Sprintf("Created dir %s", p)) + if err := c.driver.MkdirAll(p, 0o755); err == nil { + c.writeMessage(StatusFileOK, "Created dir "+p) } else { c.writeMessage(StatusActionNotTaken, fmt.Sprintf("Couldn't create dir %s: %v", p, err)) } @@ -107,18 +107,18 @@ func (c *clientHandler) handleMKDIR(params string) { func (c *clientHandler) handleRMD(param string) error { var err error - p := c.absPath(param) + pathAbsolute := c.absPath(param) if rmd, ok := c.driver.(ClientDriverExtensionRemoveDir); ok { - err = rmd.RemoveDir(p) + err = rmd.RemoveDir(pathAbsolute) } else { - err = c.driver.Remove(p) + err = c.driver.Remove(pathAbsolute) } if err == nil { - c.writeMessage(StatusFileOK, fmt.Sprintf("Deleted dir %s", p)) + c.writeMessage(StatusFileOK, "Deleted dir "+pathAbsolute) } else { - c.writeMessage(StatusActionNotTaken, fmt.Sprintf("Could not delete dir %s: %v", p, err)) + c.writeMessage(StatusActionNotTaken, fmt.Sprintf("Could not delete dir %s: %v", pathAbsolute, err)) } return nil @@ -134,7 +134,7 @@ func (c *clientHandler) handleRMDIR(params string) { p := c.absPath(params) if err := c.driver.RemoveAll(p); err == nil { - c.writeMessage(StatusFileOK, fmt.Sprintf("Removed dir %s", p)) + c.writeMessage(StatusFileOK, "Removed dir "+p) } else { c.writeMessage(StatusActionNotTaken, fmt.Sprintf("Couldn't remove dir %s: %v", p, err)) } @@ -148,7 +148,7 @@ func (c *clientHandler) handleCDUP(_ string) error { if _, err := c.driver.Stat(parent); err == nil { c.SetPath(parent) - c.writeMessage(StatusFileOK, fmt.Sprintf("CDUP worked on %s", parent)) + c.writeMessage(StatusFileOK, "CDUP worked on "+parent) } else { c.writeMessage(StatusActionNotTaken, fmt.Sprintf("CDUP issue: %v", err)) } @@ -188,7 +188,7 @@ func (c *clientHandler) checkLISTArgs(args string) string { func (c *clientHandler) handleLIST(param string) error { info := fmt.Sprintf("LIST %v", param) - if files, _, err := c.getFileList(param, true); err == nil || err == io.EOF { + if files, _, err := c.getFileList(param, true); err == nil || errors.Is(err, io.EOF) { if tr, errTr := c.TransferOpen(info); errTr == nil { err = c.dirTransferLIST(tr, files) c.TransferClose(err) @@ -207,7 +207,7 @@ func (c *clientHandler) handleLIST(param string) error { func (c *clientHandler) handleNLST(param string) error { info := fmt.Sprintf("NLST %v", param) - if files, parentDir, err := c.getFileList(param, true); err == nil || err == io.EOF { + if files, parentDir, err := c.getFileList(param, true); err == nil || errors.Is(err, io.EOF) { if tr, errTrOpen := c.TransferOpen(info); errTrOpen == nil { err = c.dirTransferNLST(tr, files, parentDir) c.TransferClose(err) @@ -223,9 +223,12 @@ func (c *clientHandler) handleNLST(param string) error { return nil } -func (c *clientHandler) dirTransferNLST(w io.Writer, files []os.FileInfo, parentDir string) error { +func (c *clientHandler) dirTransferNLST(writer io.Writer, files []os.FileInfo, parentDir string) error { if len(files) == 0 { - _, err := w.Write([]byte("")) + _, err := writer.Write([]byte("")) + if err != nil { + err = newNetworkError("couldn't send NLST data", err) + } return err } @@ -234,8 +237,8 @@ func (c *clientHandler) dirTransferNLST(w io.Writer, files []os.FileInfo, parent // Based on RFC 959 NLST is intended to return information that can be used // by a program to further process the files automatically. // So we return paths relative to the current working directory - if _, err := fmt.Fprintf(w, "%s\r\n", path.Join(c.getRelativePath(parentDir), file.Name())); err != nil { - return err + if _, err := fmt.Fprintf(writer, "%s\r\n", path.Join(c.getRelativePath(parentDir), file.Name())); err != nil { + return newNetworkError("couldn't send NLST data", err) } } @@ -251,7 +254,7 @@ func (c *clientHandler) handleMLSD(param string) error { info := fmt.Sprintf("MLSD %v", param) - if files, _, err := c.getFileList(param, false); err == nil || err == io.EOF { + if files, _, err := c.getFileList(param, false); err == nil || errors.Is(err, io.EOF) { if tr, errTr := c.TransferOpen(info); errTr == nil { err = c.dirTransferMLSD(tr, files) c.TransferClose(err) @@ -272,6 +275,8 @@ const ( dateFormatStatYear = "Jan _2 2006" // LIST date formatting with year dateFormatStatOldSwitch = time.Hour * 24 * 30 * 6 // 6 months ago dateFormatMLSD = "20060102150405" // MLSD date formatting + fakeUser = "ftp" + fakeGroup = "ftp" ) func (c *clientHandler) fileStat(file os.FileInfo) string { @@ -286,8 +291,10 @@ func (c *clientHandler) fileStat(file os.FileInfo) string { } return fmt.Sprintf( - "%s 1 ftp ftp %12d %s %s", + "%s 1 %s %s %12d %s %s", file.Mode(), + fakeUser, + fakeGroup, file.Size(), file.ModTime().Format(dateFormat), file.Name(), @@ -295,16 +302,19 @@ func (c *clientHandler) fileStat(file os.FileInfo) string { } // fclairamb (2018-02-13): #64: Removed extra empty line -func (c *clientHandler) dirTransferLIST(w io.Writer, files []os.FileInfo) error { +func (c *clientHandler) dirTransferLIST(writer io.Writer, files []os.FileInfo) error { if len(files) == 0 { - _, err := w.Write([]byte("")) + _, err := writer.Write([]byte("")) + if err != nil { + err = newNetworkError("error writing LIST entry", err) + } return err } for _, file := range files { - if _, err := fmt.Fprintf(w, "%s\r\n", c.fileStat(file)); err != nil { - return err + if _, err := fmt.Fprintf(writer, "%s\r\n", c.fileStat(file)); err != nil { + return fmt.Errorf("error writing LIST entry: %w", err) } } @@ -312,22 +322,26 @@ func (c *clientHandler) dirTransferLIST(w io.Writer, files []os.FileInfo) error } // fclairamb (2018-02-13): #64: Removed extra empty line -func (c *clientHandler) dirTransferMLSD(w io.Writer, files []os.FileInfo) error { +func (c *clientHandler) dirTransferMLSD(writer io.Writer, files []os.FileInfo) error { if len(files) == 0 { - _, err := w.Write([]byte("")) + _, err := writer.Write([]byte("")) + if err != nil { + err = newNetworkError("error writing MLSD entry", err) + } return err } for _, file := range files { - if err := c.writeMLSxEntry(w, file); err != nil { + if err := c.writeMLSxEntry(writer, file); err != nil { return err } } return nil } -func (c *clientHandler) writeMLSxEntry(w io.Writer, file os.FileInfo) error { + +func (c *clientHandler) writeMLSxEntry(writer io.Writer, file os.FileInfo) error { var listType string if file.IsDir() { listType = "dir" @@ -336,13 +350,16 @@ func (c *clientHandler) writeMLSxEntry(w io.Writer, file os.FileInfo) error { } _, err := fmt.Fprintf( - w, + writer, "Type=%s;Size=%d;Modify=%s; %s\r\n", listType, file.Size(), file.ModTime().UTC().Format(dateFormatMLSD), file.Name(), ) + if err != nil { + err = fmt.Errorf("error writing MLSD entry: %w", err) + } return err } @@ -358,7 +375,7 @@ func (c *clientHandler) getFileList(param string, filePathAllowed bool) ([]os.Fi // return list of single file if directoryPath points to file and filePathAllowed info, err := c.driver.Stat(listPath) if err != nil { - return nil, "", err + return nil, "", newFileAccessError("couldn't stat", err) } if !info.IsDir() { @@ -379,7 +396,7 @@ func (c *clientHandler) getFileList(param string, filePathAllowed bool) ([]os.Fi directory, errOpenFile := c.driver.Open(listPath) if errOpenFile != nil { - return nil, "", errOpenFile + return nil, "", newFileAccessError("couldn't open directory", errOpenFile) } defer c.closeDirectory(listPath, directory) diff --git a/handle_dirs_test.go b/handle_dirs_test.go index 7043fad7..ac69c3ba 100644 --- a/handle_dirs_test.go +++ b/handle_dirs_test.go @@ -19,7 +19,7 @@ func TestGetRelativePaths(t *testing.T) { type relativePathTest struct { workingDir, path, result string } - var tests = []relativePathTest{ + tests := []relativePathTest{ {"/", "/p", "p"}, {"/", "/", ""}, {"/p", "/p", ""}, @@ -40,22 +40,22 @@ func TestGetRelativePaths(t *testing.T) { func TestDirListing(t *testing.T) { // MLSD is disabled we relies on LIST of files listing - s := NewTestServerWithTestDriver(t, &TestServerDriver{Debug: false, Settings: &Settings{DisableMLSD: true}}) + server := NewTestServerWithTestDriver(t, &TestServerDriver{Debug: false, Settings: &Settings{DisableMLSD: true}}) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - dirName, err := c.Mkdir(DirKnown) + dirName, err := client.Mkdir(DirKnown) require.NoError(t, err, "Couldn't create dir") require.Equal(t, path.Join("/", DirKnown), dirName) - contents, err := c.ReadDir("/") + contents, err := client.ReadDir("/") require.NoError(t, err) require.Len(t, contents, 1) require.Equal(t, DirKnown, contents[0].Name()) @@ -63,47 +63,47 @@ func TestDirListing(t *testing.T) { // LIST also works for filePath fileName := "testfile.ext" - _, err = c.ReadDir(fileName) + _, err = client.ReadDir(fileName) require.Error(t, err, "LIST for not existing filePath must fail") - ftpUpload(t, c, createTemporaryFile(t, 10), fileName) + ftpUpload(t, client, createTemporaryFile(t, 10), fileName) - fileContents, err := c.ReadDir(fileName) + fileContents, err := client.ReadDir(fileName) require.NoError(t, err) require.Len(t, fileContents, 1) require.Equal(t, fileName, fileContents[0].Name()) // the test driver will fail to open this dir - dirName, err = c.Mkdir("fail-to-open-dir") + dirName, err = client.Mkdir("fail-to-open-dir") require.NoError(t, err) - _, err = c.ReadDir(dirName) + _, err = client.ReadDir(dirName) require.Error(t, err) } func TestDirListingPathArg(t *testing.T) { // MLSD is disabled we relies on LIST of files listing - s := NewTestServerWithTestDriver(t, &TestServerDriver{Debug: false, Settings: &Settings{DisableMLSD: true}}) + server := NewTestServerWithTestDriver(t, &TestServerDriver{Debug: false, Settings: &Settings{DisableMLSD: true}}) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() for _, dir := range []string{"/" + DirKnown, "/" + DirKnown + "/1"} { - _, err = c.Mkdir(dir) + _, err = client.Mkdir(dir) require.NoError(t, err, "Couldn't create dir") } - contents, err := c.ReadDir(DirKnown) + contents, err := client.ReadDir(DirKnown) require.NoError(t, err) require.Len(t, contents, 1) require.Equal(t, "1", contents[0].Name()) - contents, err = c.ReadDir("") + contents, err = client.ReadDir("") require.NoError(t, err) require.Len(t, contents, 1) require.Equal(t, DirKnown, contents[0].Name()) @@ -112,86 +112,86 @@ func TestDirListingPathArg(t *testing.T) { func TestDirHandling(t *testing.T) { s := NewTestServer(t, false) - c, err := goftp.DialConfig(goftp.Config{ + client, err := goftp.DialConfig(goftp.Config{ User: authUser, Password: authPass, }, s.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - rc, _, err := raw.SendCommand("CWD /unknown") + returnCode, _, err := raw.SendCommand("CWD /unknown") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc) + require.Equal(t, StatusActionNotTaken, returnCode) - _, err = c.Mkdir("/" + DirKnown) + _, err = client.Mkdir("/" + DirKnown) require.NoError(t, err) - contents, err := c.ReadDir("/") + contents, err := client.ReadDir("/") require.NoError(t, err) require.Len(t, contents, 1) - rc, _, err = raw.SendCommand("CWD /" + DirKnown) + returnCode, _, err = raw.SendCommand("CWD /" + DirKnown) require.NoError(t, err) - require.Equal(t, StatusFileOK, rc) + require.Equal(t, StatusFileOK, returnCode) testSubdir := ` strange\\ sub" dìr` - rc, _, err = raw.SendCommand(fmt.Sprintf("MKD %v", testSubdir)) + returnCode, _, err = raw.SendCommand(fmt.Sprintf("MKD %v", testSubdir)) require.NoError(t, err) - require.Equal(t, StatusPathCreated, rc) + require.Equal(t, StatusPathCreated, returnCode) - rc, response, err := raw.SendCommand(fmt.Sprintf("CWD %v", testSubdir)) + returnCode, response, err := raw.SendCommand(fmt.Sprintf("CWD %v", testSubdir)) require.NoError(t, err) - require.Equal(t, StatusFileOK, rc, response) + require.Equal(t, StatusFileOK, returnCode, response) - rc, response, err = raw.SendCommand(fmt.Sprintf("PWD %v", testSubdir)) + returnCode, response, err = raw.SendCommand(fmt.Sprintf("PWD %v", testSubdir)) require.NoError(t, err) - require.Equal(t, StatusPathCreated, rc, response) + require.Equal(t, StatusPathCreated, returnCode, response) require.Equal(t, `"/known/ strange\\ sub"" dìr" is the current directory`, response) - rc, response, err = raw.SendCommand("CDUP") + returnCode, response, err = raw.SendCommand("CDUP") require.NoError(t, err) - require.Equal(t, StatusFileOK, rc, response) + require.Equal(t, StatusFileOK, returnCode, response) - err = c.Rmdir(path.Join("/", DirKnown, testSubdir)) + err = client.Rmdir(path.Join("/", DirKnown, testSubdir)) require.NoError(t, err) - err = c.Rmdir(path.Join("/", DirKnown)) + err = client.Rmdir(path.Join("/", DirKnown)) require.NoError(t, err) - err = c.Rmdir(path.Join("/", DirKnown)) + err = client.Rmdir(path.Join("/", DirKnown)) require.Error(t, err, "We shouldn't have been able to ftpDelete known again") } func TestCWDToRegularFile(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() // Getwd will send a PWD command - p, err := c.Getwd() + p, err := client.Getwd() require.NoError(t, err) require.Equal(t, "/", p, "Bad path") - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() // Creating a tiny file - ftpUpload(t, c, createTemporaryFile(t, 10), "file.txt") + ftpUpload(t, client, createTemporaryFile(t, 10), "file.txt") rc, msg, err := raw.SendCommand("CWD /file.txt") require.NoError(t, err) @@ -200,114 +200,115 @@ func TestCWDToRegularFile(t *testing.T) { } func TestMkdirRmDir(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) + req := require.New(t) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) - require.NoError(t, err, "Couldn't connect") + client, err := goftp.DialConfig(conf, server.Addr()) + req.NoError(err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() - require.NoError(t, err, "Couldn't open raw connection") + raw, err := client.OpenRawConn() + req.NoError(err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() t.Run("standard", func(t *testing.T) { - rc, _, err := raw.SendCommand("SITE MKDIR /dir1/dir2/dir3") + returnCode, _, err := raw.SendCommand("SITE MKDIR /dir1/dir2/dir3") require.NoError(t, err) - require.Equal(t, StatusFileOK, rc) + require.Equal(t, StatusFileOK, returnCode) for _, d := range []string{"/dir1", "/dir1/dir2", "/dir1/dir2/dir3"} { - stat, errStat := c.Stat(d) + stat, errStat := client.Stat(d) require.NoError(t, errStat) require.True(t, stat.IsDir()) } - rc, _, err = raw.SendCommand("SITE RMDIR /dir1") + returnCode, _, err = raw.SendCommand("SITE RMDIR /dir1") require.NoError(t, err) - require.Equal(t, StatusFileOK, rc) + require.Equal(t, StatusFileOK, returnCode) for _, d := range []string{"/dir1", "/dir1/dir2", "/dir1/dir2/dir3"} { - stat, errStat := c.Stat(d) + stat, errStat := client.Stat(d) require.Error(t, errStat) require.Nil(t, stat) } - _, err = c.Mkdir("/missing/path") + _, err = client.Mkdir("/missing/path") require.Error(t, err) }) t.Run("syntax error", func(t *testing.T) { - rc, _, err := raw.SendCommand("SITE MKDIR") + returnCode, _, err := raw.SendCommand("SITE MKDIR") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorNotRecognised, rc) + require.Equal(t, StatusSyntaxErrorNotRecognised, returnCode) - rc, _, err = raw.SendCommand("SITE RMDIR") + returnCode, _, err = raw.SendCommand("SITE RMDIR") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorNotRecognised, rc) + require.Equal(t, StatusSyntaxErrorNotRecognised, returnCode) }) t.Run("spaces", func(t *testing.T) { - rc, _, err := raw.SendCommand("SITE MKDIR /dir1 /dir2") + returnCode, _, err := raw.SendCommand("SITE MKDIR /dir1 /dir2") require.NoError(t, err) - require.Equal(t, StatusFileOK, rc) + require.Equal(t, StatusFileOK, returnCode) { dir := "/dir1 /dir2" - stat, errStat := c.Stat(dir) + stat, errStat := client.Stat(dir) require.NoError(t, errStat) require.True(t, stat.IsDir()) } - rc, _, err = raw.SendCommand("SITE RMDIR /dir1 /dir2") + returnCode, _, err = raw.SendCommand("SITE RMDIR /dir1 /dir2") require.NoError(t, err) - require.Equal(t, StatusFileOK, rc) + require.Equal(t, StatusFileOK, returnCode) }) } // TestDirListingWithSpace uses the MLSD for files listing func TestDirListingWithSpace(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() dirName := " with spaces " - _, err = c.Mkdir(dirName) + _, err = client.Mkdir(dirName) require.NoError(t, err, "Couldn't create dir") - contents, err := c.ReadDir("/") + contents, err := client.ReadDir("/") require.NoError(t, err) require.Len(t, contents, 1) require.Equal(t, dirName, contents[0].Name()) - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - rc, response, err := raw.SendCommand(fmt.Sprintf("CWD /%s", dirName)) + returnCode, response, err := raw.SendCommand("CWD /" + dirName) require.NoError(t, err) - require.Equal(t, StatusFileOK, rc) - require.Equal(t, fmt.Sprintf("CD worked on /%s", dirName), response) + require.Equal(t, StatusFileOK, returnCode) + require.Equal(t, "CD worked on /"+dirName, response) dcGetter, err := raw.PrepareDataConn() require.NoError(t, err) - rc, response, err = raw.SendCommand("NLST /") + returnCode, response, err = raw.SendCommand("NLST /") require.NoError(t, err) - require.Equal(t, StatusFileStatusOK, rc, response) + require.Equal(t, StatusFileStatusOK, returnCode, response) dc, err := dcGetter() require.NoError(t, err) @@ -315,31 +316,31 @@ func TestDirListingWithSpace(t *testing.T) { require.NoError(t, err) require.Equal(t, "../"+dirName+"\r\n", string(resp)) - rc, _, err = raw.ReadResponse() + returnCode, _, err = raw.ReadResponse() require.NoError(t, err) - require.Equal(t, StatusClosingDataConn, rc) + require.Equal(t, StatusClosingDataConn, returnCode) _, err = raw.PrepareDataConn() require.NoError(t, err) - rc, response, err = raw.SendCommand("NLST /missingpath") + returnCode, response, err = raw.SendCommand("NLST /missingpath") require.NoError(t, err) - require.Equal(t, StatusFileActionNotTaken, rc, response) + require.Equal(t, StatusFileActionNotTaken, returnCode, response) } func TestCleanPath(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() @@ -354,23 +355,24 @@ func TestCleanPath(t *testing.T) { "/./", "/././.", } { - rc, response, err := raw.SendCommand(fmt.Sprintf("CWD %s", dir)) + rc, response, err := raw.SendCommand("CWD " + dir) require.NoError(t, err) require.Equal(t, StatusFileOK, rc) require.Equal(t, "CD worked on /", response) - p, err := c.Getwd() + p, err := client.Getwd() require.NoError(t, err) require.Equal(t, "/", p) } } func TestTLSTransfer(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + req := require.New(t) + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Debug: false, TLS: true, }) - s.settings.TLSRequired = MandatoryEncryption + server.settings.TLSRequired = MandatoryEncryption conf := goftp.Config{ User: authUser, @@ -382,37 +384,37 @@ func TestTLSTransfer(t *testing.T) { TLSMode: goftp.TLSExplicit, } - c, err := goftp.DialConfig(conf, s.Addr()) - require.NoError(t, err, "Couldn't connect") + client, err := goftp.DialConfig(conf, server.Addr()) + req.NoError(err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - contents, err := c.ReadDir("/") - require.NoError(t, err) - require.Len(t, contents, 0) + contents, err := client.ReadDir("/") + req.NoError(err) + req.Empty(contents) - raw, err := c.OpenRawConn() - require.NoError(t, err, "Couldn't open raw connection") + raw, err := client.OpenRawConn() + req.NoError(err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - rc, response, err := raw.SendCommand("PROT C") - require.NoError(t, err) - require.Equal(t, StatusOK, rc) - require.Equal(t, "OK", response) + returnCode, response, err := raw.SendCommand("PROT C") + req.NoError(err) + req.Equal(StatusOK, returnCode) + req.Equal("OK", response) - rc, _, err = raw.SendCommand("PASV") - require.NoError(t, err) - require.Equal(t, StatusEnteringPASV, rc) + returnCode, _, err = raw.SendCommand("PASV") + req.NoError(err) + req.Equal(StatusEnteringPASV, returnCode) - rc, response, err = raw.SendCommand("MLSD /") - require.NoError(t, err) - require.Equal(t, StatusServiceNotAvailable, rc) - require.Equal(t, "unable to open transfer: TLS is required", response) + returnCode, response, err = raw.SendCommand("MLSD /") + req.NoError(err) + req.Equal(StatusServiceNotAvailable, returnCode) + req.Equal("unable to open transfer: TLS is required", response) } func TestPerClientTLSTransfer(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Debug: true, TLS: true, TLSRequirement: MandatoryEncryption, @@ -427,32 +429,32 @@ func TestPerClientTLSTransfer(t *testing.T) { TLSMode: goftp.TLSExplicit, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - _, err = c.ReadDir("/") + _, err = client.ReadDir("/") require.NoError(t, err) // now switch to unencrypted data connection - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - rc, resp, err := raw.SendCommand("PROT C") + returnCode, resp, err := raw.SendCommand("PROT C") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) require.Equal(t, "OK", resp) - rc, _, err = raw.SendCommand("PASV") + returnCode, _, err = raw.SendCommand("PASV") require.NoError(t, err) - require.Equal(t, StatusEnteringPASV, rc) + require.Equal(t, StatusEnteringPASV, returnCode) - rc, response, err := raw.SendCommand("MLSD /") + returnCode, response, err := raw.SendCommand("MLSD /") require.NoError(t, err) - require.Equal(t, StatusServiceNotAvailable, rc) + require.Equal(t, StatusServiceNotAvailable, returnCode) require.Equal(t, "unable to open transfer: TLS is required", response) } @@ -467,24 +469,27 @@ func TestDirListingBeforeLogin(t *testing.T) { }() buf := make([]byte, 1024) - n, err := conn.Read(buf) + readBytes, err := conn.Read(buf) require.NoError(t, err) - response := string(buf[:n]) + response := string(buf[:readBytes]) require.Equal(t, "220 TEST Server\r\n", response) _, err = conn.Write([]byte("LIST\r\n")) require.NoError(t, err) - n, err = conn.Read(buf) + readBytes, err = conn.Read(buf) require.NoError(t, err) - response = string(buf[:n]) + response = string(buf[:readBytes]) require.Equal(t, "530 Please login with USER and PASS\r\n", response) } func TestListArgs(t *testing.T) { + t.Parallel() + t.Run("with-mlsd", func(t *testing.T) { + t.Parallel() testListDirArgs( t, NewTestServer(t, false), @@ -492,6 +497,7 @@ func TestListArgs(t *testing.T) { }) t.Run("without-mlsd", func(t *testing.T) { + t.Parallel() testListDirArgs( t, NewTestServerWithTestDriver(t, &TestServerDriver{Debug: false, Settings: &Settings{DisableMLSD: true}}), @@ -499,69 +505,72 @@ func TestListArgs(t *testing.T) { }) } -func testListDirArgs(t *testing.T, s *FtpServer) { +func testListDirArgs(t *testing.T, server *FtpServer) { + t.Helper() + req := require.New(t) + conf := goftp.Config{ User: authUser, Password: authPass, } testDir := "testdir" - c, err := goftp.DialConfig(conf, s.Addr()) - require.NoError(t, err, "Couldn't connect") + client, err := goftp.DialConfig(conf, server.Addr()) + req.NoError(err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() for _, arg := range supportedlistArgs { - s.settings.DisableLISTArgs = true + server.settings.DisableLISTArgs = true - _, err = c.ReadDir(arg) + _, err = client.ReadDir(arg) require.Error(t, err, fmt.Sprintf("list args are disabled \"list %v\" must fail", arg)) - s.settings.DisableLISTArgs = false + server.settings.DisableLISTArgs = false - contents, err := c.ReadDir(arg) - require.NoError(t, err) - require.Len(t, contents, 0) + contents, err := client.ReadDir(arg) + req.NoError(err) + req.Empty(contents) - _, err = c.Mkdir(arg) - require.NoError(t, err) + _, err = client.Mkdir(arg) + req.NoError(err) - _, err = c.Mkdir(fmt.Sprintf("%v/%v", arg, testDir)) - require.NoError(t, err) + _, err = client.Mkdir(fmt.Sprintf("%v/%v", arg, testDir)) + req.NoError(err) - contents, err = c.ReadDir(arg) - require.NoError(t, err) - require.Len(t, contents, 1) - require.Equal(t, contents[0].Name(), testDir) + contents, err = client.ReadDir(arg) + req.NoError(err) + req.Len(contents, 1) + req.Equal(contents[0].Name(), testDir) - contents, err = c.ReadDir(fmt.Sprintf("%v %v", arg, arg)) - require.NoError(t, err) - require.Len(t, contents, 1) - require.Equal(t, contents[0].Name(), testDir) + contents, err = client.ReadDir(fmt.Sprintf("%v %v", arg, arg)) + req.NoError(err) + req.Len(contents, 1) + req.Equal(contents[0].Name(), testDir) // cleanup - err = c.Rmdir(fmt.Sprintf("%v/%v", arg, testDir)) - require.NoError(t, err) + err = client.Rmdir(fmt.Sprintf("%v/%v", arg, testDir)) + req.NoError(err) - err = c.Rmdir(arg) - require.NoError(t, err) + err = client.Rmdir(arg) + req.NoError(err) } } func TestMLSDTimezone(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - ftpUpload(t, c, createTemporaryFile(t, 10), "file") - contents, err := c.ReadDir("/") + ftpUpload(t, client, createTemporaryFile(t, 10), "file") + contents, err := client.ReadDir("/") require.NoError(t, err) require.Len(t, contents, 1) require.Equal(t, "file", contents[0].Name()) @@ -569,30 +578,30 @@ func TestMLSDTimezone(t *testing.T) { } func TestMLSDAndNLSTFilePathError(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() // MLSD shouldn't work for filePaths - var fileName = "testfile.ext" + fileName := "testfile.ext" - _, err = c.ReadDir(fileName) + _, err = client.ReadDir(fileName) require.Error(t, err, "MLSD for not existing filePath must fail") - ftpUpload(t, c, createTemporaryFile(t, 10), fileName) + ftpUpload(t, client, createTemporaryFile(t, 10), fileName) - _, err = c.ReadDir(fileName) + _, err = client.ReadDir(fileName) require.Error(t, err, "MLSD is enabled, MLSD for filePath must fail") // NLST should work for filePath - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() diff --git a/handle_files.go b/handle_files.go index c371d592..bbf895ff 100644 --- a/handle_files.go +++ b/handle_files.go @@ -43,7 +43,7 @@ func (c *clientHandler) handleRETR(param string) error { // File transfer, read or write, seek or not, is basically the same. // To make sure we don't miss any step, we execute everything in order -func (c *clientHandler) transferFile(write bool, append bool, param, info string) { +func (c *clientHandler) transferFile(write bool, appendFile bool, param, info string) { var file FileTransfer var err error var fileFlag int @@ -51,9 +51,9 @@ func (c *clientHandler) transferFile(write bool, append bool, param, info string path := c.absPath(param) // We try to open the file - if write { + if write { //nolint:nestif // too much effort to change for now fileFlag = os.O_WRONLY - if append { + if appendFile { fileFlag |= os.O_CREATE | os.O_APPEND // ignore the seek position for append mode c.ctxRest = 0 @@ -70,7 +70,6 @@ func (c *clientHandler) transferFile(write bool, append bool, param, info string } file, err = c.getFileHandle(path, fileFlag, c.ctxRest) - // If this fail, can stop right here and reset the seek position if err != nil { if !c.isCommandAborted() { @@ -100,7 +99,7 @@ func (c *clientHandler) transferFile(write bool, append bool, param, info string } } - tr, err := c.TransferOpen(info) + fileTransferConn, err := c.TransferOpen(info) if err != nil { if fileTransferError, ok := file.(FileTransferError); ok { fileTransferError.TransferError(err) @@ -112,7 +111,7 @@ func (c *clientHandler) transferFile(write bool, append bool, param, info string return } - err = c.doFileTransfer(tr, file, write) + err = c.doFileTransfer(fileTransferConn, file, write) // we ignore close error for reads if errClose := file.Close(); errClose != nil && err == nil && write { err = errClose @@ -122,32 +121,32 @@ func (c *clientHandler) transferFile(write bool, append bool, param, info string c.TransferClose(err) } -func (c *clientHandler) doFileTransfer(tr net.Conn, file io.ReadWriter, write bool) error { +func (c *clientHandler) doFileTransfer(transferConn net.Conn, file io.ReadWriter, write bool) error { var err error - var in io.Reader - var out io.Writer + var reader io.Reader + var writer io.Writer conversionMode := convertModeToCRLF // Copy the data if write { // ... from the connection to the file - in = tr - out = file + reader = transferConn + writer = file if runtime.GOOS != "windows" { conversionMode = convertModeToLF } } else { // ... from the file to the connection - in = file - out = tr + reader = file + writer = transferConn } if c.currentTransferType == TransferTypeASCII { - in = newASCIIConverter(in, conversionMode) + reader = newASCIIConverter(reader, conversionMode) } // for reads io.EOF isn't an error, for writes it must be considered an error - if written, errCopy := io.Copy(out, in); errCopy != nil && (errCopy != io.EOF || write) { + if written, errCopy := io.Copy(writer, reader); errCopy != nil && (errors.Is(errCopy, io.EOF) || write) { err = errCopy } else { c.logger.Debug( @@ -156,7 +155,7 @@ func (c *clientHandler) doFileTransfer(tr net.Conn, file io.ReadWriter, write bo ) if written == 0 { - _, err = out.Write([]byte{}) + _, err = writer.Write([]byte{}) } } @@ -164,6 +163,8 @@ func (c *clientHandler) doFileTransfer(tr net.Conn, file io.ReadWriter, write bo if fileTransferError, ok := file.(FileTransferError); ok { fileTransferError.TransferError(err) } + + err = newNetworkError("error transferring data", err) } return err @@ -351,7 +352,7 @@ func (c *clientHandler) handleSYMLINK(params string) { func (c *clientHandler) handleDELE(param string) error { path := c.absPath(param) if err := c.driver.Remove(path); err == nil { - c.writeMessage(StatusFileOK, fmt.Sprintf("Removed file %s", path)) + c.writeMessage(StatusFileOK, "Removed file "+path) } else { c.writeMessage(StatusActionNotTaken, fmt.Sprintf("Couldn't delete %s: %v", path, err)) } @@ -406,7 +407,7 @@ func (c *clientHandler) handleSIZE(param string) error { path := c.absPath(param) if info, err := c.driver.Stat(path); err == nil { - c.writeMessage(StatusFileStatus, fmt.Sprintf("%d", info.Size())) + c.writeMessage(StatusFileStatus, strconv.FormatInt(info.Size(), 10)) } else { c.writeMessage(StatusActionNotTaken, fmt.Sprintf("Couldn't access %s: %v", path, err)) } @@ -417,44 +418,49 @@ func (c *clientHandler) handleSIZE(param string) error { func (c *clientHandler) handleSTATFile(param string) error { path := c.absPath(param) - if info, err := c.driver.Stat(path); err == nil { - if info.IsDir() { - var files []os.FileInfo - var errList error + info, err := c.driver.Stat(path) + if err != nil { + c.writeMessage(StatusFileActionNotTaken, fmt.Sprintf("Could not STAT: %v", err)) - directoryPath := c.absPath(param) + return nil + } - if fileList, ok := c.driver.(ClientDriverExtensionFileList); ok { - files, errList = fileList.ReadDir(directoryPath) - } else { - directory, errOpenFile := c.driver.Open(c.absPath(param)) + if !info.IsDir() { + defer c.multilineAnswer(StatusFileStatus, fmt.Sprintf("STAT %v", param))() - if errOpenFile != nil { - c.writeMessage(StatusFileActionNotTaken, fmt.Sprintf("Could not list: %v", errOpenFile)) + c.writeLine(" " + c.fileStat(info)) - return nil - } + return nil + } - files, errList = directory.Readdir(-1) - c.closeDirectory(directoryPath, directory) - } + var files []os.FileInfo + var errList error - if errList == nil { - defer c.multilineAnswer(StatusDirectoryStatus, fmt.Sprintf("STAT %v", param))() + directoryPath := c.absPath(param) - for _, f := range files { - c.writeLine(fmt.Sprintf(" %s", c.fileStat(f))) - } - } else { - c.writeMessage(StatusFileActionNotTaken, fmt.Sprintf("Could not list: %v", errList)) - } - } else { - defer c.multilineAnswer(StatusFileStatus, fmt.Sprintf("STAT %v", param))() + if fileList, ok := c.driver.(ClientDriverExtensionFileList); ok { + files, errList = fileList.ReadDir(directoryPath) + } else { + directory, errOpenFile := c.driver.Open(c.absPath(param)) + + if errOpenFile != nil { + c.writeMessage(StatusFileActionNotTaken, fmt.Sprintf("Could not list: %v", errOpenFile)) + + return nil + } + + files, errList = directory.Readdir(-1) + c.closeDirectory(directoryPath, directory) + } - c.writeLine(fmt.Sprintf(" %s", c.fileStat(info))) + if errList == nil { + defer c.multilineAnswer(StatusDirectoryStatus, fmt.Sprintf("STAT %v", param))() + + for _, f := range files { + c.writeLine(" %s" + c.fileStat(f)) } } else { - c.writeMessage(StatusFileActionNotTaken, fmt.Sprintf("Could not STAT: %v", err)) + c.writeMessage(StatusFileActionNotTaken, fmt.Sprintf("Could not list: %v", errList)) } return nil @@ -487,18 +493,21 @@ func (c *clientHandler) handleMLST(param string) error { func (c *clientHandler) handleALLO(param string) error { // We should probably add a method in the driver - if size, err := strconv.Atoi(param); err == nil { - if alloInt, ok := c.driver.(ClientDriverExtensionAllocate); !ok { - c.writeMessage(StatusNotImplemented, "This extension hasn't been implemented !") + size, err := strconv.Atoi(param) + if err != nil { + c.writeMessage(StatusSyntaxErrorParameters, fmt.Sprintf("Couldn't parse size: %v", err)) + + return nil + } + + if alloInt, ok := c.driver.(ClientDriverExtensionAllocate); !ok { + c.writeMessage(StatusNotImplemented, "This extension hasn't been implemented !") + } else { + if errAllocate := alloInt.AllocateSpace(size); errAllocate != nil { + c.writeMessage(StatusActionNotTaken, fmt.Sprintf("Couldn't alloInt: %v", errAllocate)) } else { - if errAllocate := alloInt.AllocateSpace(size); errAllocate != nil { - c.writeMessage(StatusActionNotTaken, fmt.Sprintf("Couldn't alloInt: %v", errAllocate)) - } else { - c.writeMessage(StatusOK, "Done !") - } + c.writeMessage(StatusOK, "Done !") } - } else { - c.writeMessage(StatusSyntaxErrorParameters, fmt.Sprintf("Couldn't parse size: %v", err)) } return nil @@ -536,8 +545,9 @@ func (c *clientHandler) handleMDTM(param string) error { func (c *clientHandler) handleMFMT(param string) error { params := strings.SplitN(param, " ", 2) if len(params) != 2 { - c.writeMessage(StatusSyntaxErrorNotRecognised, fmt.Sprintf( - "Couldn't set mtime, not enough params, given: %s", param)) + c.writeMessage(StatusSyntaxErrorNotRecognised, + "Couldn't set mtime, not enough params, given: "+param, + ) return nil } @@ -617,7 +627,7 @@ func (c *clientHandler) handleGenericHash(param string, algo HASHAlgo, isCustomM // to support partial hash also for the HASH command, we should implement RANG, // but it applies also to uploads/downloads and so it complicates their handling, // we'll add this support in future improvements - if isCustomMode { + if isCustomMode { //nolint:nestif // too much effort to change for now // for custom command the range can be specified in this way: // XSHA1 if len(args) > 1 { @@ -668,27 +678,26 @@ func (c *clientHandler) handleGenericHash(param string, algo HASHAlgo, isCustomM } func (c *clientHandler) computeHashForFile(filePath string, algo HASHAlgo, start, end int64) (string, error) { - var h hash.Hash + var chosenHashAlgo hash.Hash var file FileTransfer var err error switch algo { case HASHAlgoCRC32: - h = crc32.NewIEEE() + chosenHashAlgo = crc32.NewIEEE() case HASHAlgoMD5: - h = md5.New() //nolint:gosec + chosenHashAlgo = md5.New() //nolint:gosec case HASHAlgoSHA1: - h = sha1.New() //nolint:gosec + chosenHashAlgo = sha1.New() //nolint:gosec case HASHAlgoSHA256: - h = sha256.New() + chosenHashAlgo = sha256.New() case HASHAlgoSHA512: - h = sha512.New() + chosenHashAlgo = sha512.New() default: return "", errUnknowHash } file, err = c.getFileHandle(filePath, os.O_RDONLY, start) - if err != nil { return "", err } @@ -698,25 +707,35 @@ func (c *clientHandler) computeHashForFile(filePath string, algo HASHAlgo, start if start > 0 { _, err = file.Seek(start, io.SeekStart) if err != nil { - return "", err + return "", newFileAccessError("couldn't seek file", err) } } - _, err = io.CopyN(h, file, end-start) + _, err = io.CopyN(chosenHashAlgo, file, end-start) - if err != nil && err != io.EOF { - return "", err + if err != nil && errors.Is(err, io.EOF) { + return "", newFileAccessError("couldn't read file", err) } - return hex.EncodeToString(h.Sum(nil)), nil + return hex.EncodeToString(chosenHashAlgo.Sum(nil)), nil } func (c *clientHandler) getFileHandle(name string, flags int, offset int64) (FileTransfer, error) { if fileTransfer, ok := c.driver.(ClientDriverExtentionFileTransfer); ok { - return fileTransfer.GetHandle(name, flags, offset) + ft, err := fileTransfer.GetHandle(name, flags, offset) + if err != nil { + err = newDriverError("calling GetHandle", err) + } + + return ft, err } - return c.driver.OpenFile(name, flags, os.ModePerm) + file, err := c.driver.OpenFile(name, flags, os.ModePerm) + if err != nil { + err = newDriverError("calling OpenFile", err) + } + + return file, err } func (c *clientHandler) closeUnchecked(file io.Closer) { @@ -740,5 +759,10 @@ func unquoteSpaceSeparatedParams(params string) ([]string, error) { reader := csv.NewReader(strings.NewReader(params)) reader.Comma = ' ' // space - return reader.Read() + spl, err := reader.Read() + if err != nil { + return nil, fmt.Errorf("error parsing params: %w", err) + } + + return spl, nil } diff --git a/handle_files_test.go b/handle_files_test.go index f6b059d8..ad57f686 100644 --- a/handle_files_test.go +++ b/handle_files_test.go @@ -8,6 +8,7 @@ import ( "io" "os" "regexp" + "strconv" "strings" "testing" @@ -100,23 +101,23 @@ func TestALLO(t *testing.T) { defer func() { require.NoError(t, raw.Close()) }() // Asking for too much (2MB) - rc, _, err := raw.SendCommand("ALLO 2000000") + returnCode, _, err := raw.SendCommand("ALLO 2000000") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc, "Should have been refused") + require.Equal(t, StatusActionNotTaken, returnCode, "Should have been refused") // Asking for the right amount of space (500KB) - rc, _, err = raw.SendCommand("ALLO 500000") + returnCode, _, err = raw.SendCommand("ALLO 500000") require.NoError(t, err) - require.Equal(t, StatusOK, rc, "Should have been accepted") + require.Equal(t, StatusOK, returnCode, "Should have been accepted") // Wrong size - rc, _, err = raw.SendCommand("ALLO 500000a") + returnCode, _, err = raw.SendCommand("ALLO 500000a") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorParameters, rc, "Should have been refused") + require.Equal(t, StatusSyntaxErrorParameters, returnCode, "Should have been refused") } func TestCHMOD(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Debug: false, TLS: true, }) @@ -129,78 +130,78 @@ func TestCHMOD(t *testing.T) { }, TLSMode: goftp.TLSExplicit, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") // Creating a tiny file - ftpUpload(t, c, createTemporaryFile(t, 10), "file") + ftpUpload(t, client, createTemporaryFile(t, 10), "file") - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - rc, _, err := raw.SendCommand("SITE CHMOD a file") + returnCode, _, err := raw.SendCommand("SITE CHMOD a file") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc, "Should have been refused") + require.Equal(t, StatusActionNotTaken, returnCode, "Should have been refused") - rc, _, err = raw.SendCommand("SITE CHMOD 600 file") + returnCode, _, err = raw.SendCommand("SITE CHMOD 600 file") require.NoError(t, err) - require.Equal(t, StatusOK, rc, "Should have been accepted") + require.Equal(t, StatusOK, returnCode, "Should have been accepted") } func TestCHOWN(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() // Creating a tiny file - ftpUpload(t, c, createTemporaryFile(t, 10), "file") + ftpUpload(t, client, createTemporaryFile(t, 10), "file") - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() // Asking for a chown user change that isn't authorized - rc, _, err := raw.SendCommand("SITE CHOWN 1001:500 file") + returnCode, _, err := raw.SendCommand("SITE CHOWN 1001:500 file") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc, "Should have been refused") + require.Equal(t, StatusActionNotTaken, returnCode, "Should have been refused") // Asking for a chown user change that isn't authorized - rc, _, err = raw.SendCommand("SITE CHOWN 1001 file") + returnCode, _, err = raw.SendCommand("SITE CHOWN 1001 file") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc, "Should have been refused") + require.Equal(t, StatusActionNotTaken, returnCode, "Should have been refused") // Asking for the right chown user - rc, _, err = raw.SendCommand("SITE CHOWN 1000:500 file") + returnCode, _, err = raw.SendCommand("SITE CHOWN 1000:500 file") require.NoError(t, err) - require.Equal(t, StatusOK, rc, "Should have been accepted") + require.Equal(t, StatusOK, returnCode, "Should have been accepted") // Asking for the right chown user - rc, _, err = raw.SendCommand("SITE CHOWN 1000 file") + returnCode, _, err = raw.SendCommand("SITE CHOWN 1000 file") require.NoError(t, err) - require.Equal(t, StatusOK, rc, "Should have been accepted") + require.Equal(t, StatusOK, returnCode, "Should have been accepted") // Asking for a chown on a file that doesn't exist - rc, _, err = raw.SendCommand("SITE CHOWN test file2") + returnCode, _, err = raw.SendCommand("SITE CHOWN test file2") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc, "Should NOT have been accepted") + require.Equal(t, StatusActionNotTaken, returnCode, "Should NOT have been accepted") // Asking for a chown with a missing parameter - rc, _, err = raw.SendCommand("SITE CHOWN 1000") + returnCode, _, err = raw.SendCommand("SITE CHOWN 1000") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorParameters, rc, "Should NOT have been accepted") + require.Equal(t, StatusSyntaxErrorParameters, returnCode, "Should NOT have been accepted") } func TestMFMT(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Debug: false, TLS: true, }) @@ -213,179 +214,180 @@ func TestMFMT(t *testing.T) { }, TLSMode: goftp.TLSExplicit, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() // Creating a tiny file - ftpUpload(t, c, createTemporaryFile(t, 10), "file") + ftpUpload(t, client, createTemporaryFile(t, 10), "file") - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() // Good - rc, _, err := raw.SendCommand("MFMT 20201209211059 file") + returnCode, _, err := raw.SendCommand("MFMT 20201209211059 file") require.NoError(t, err) - require.Equal(t, StatusFileStatus, rc, "Should have succeeded") + require.Equal(t, StatusFileStatus, returnCode, "Should have succeeded") // 3 params instead of 2 - rc, _, err = raw.SendCommand("MFMT 20201209211059 file somethingelse") + returnCode, _, err = raw.SendCommand("MFMT 20201209211059 file somethingelse") require.NoError(t, err) - require.NotEqual(t, StatusFileStatus, rc, "Should have failed") + require.NotEqual(t, StatusFileStatus, returnCode, "Should have failed") // 1 param instead of 2 - rc, _, err = raw.SendCommand("MFMT 202012092110 file") + returnCode, _, err = raw.SendCommand("MFMT 202012092110 file") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorParameters, rc, "Should have failed") + require.Equal(t, StatusSyntaxErrorParameters, returnCode, "Should have failed") // no parameters - rc, _, err = raw.SendCommand("MFMT") + returnCode, _, err = raw.SendCommand("MFMT") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorNotRecognised, rc, "Should have failed") + require.Equal(t, StatusSyntaxErrorNotRecognised, returnCode, "Should have failed") // Good (to make sure we are still in sync) - rc, _, err = raw.SendCommand("MFMT 20201209211059 file") + returnCode, _, err = raw.SendCommand("MFMT 20201209211059 file") require.NoError(t, err) - require.Equal(t, StatusFileStatus, rc, "Should have succeeded") + require.Equal(t, StatusFileStatus, returnCode, "Should have succeeded") } func TestSYMLINK(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() // Creating a tiny file - ftpUpload(t, c, createTemporaryFile(t, 10), "file") + ftpUpload(t, client, createTemporaryFile(t, 10), "file") - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() // Bad syntaxes - rc, _, err := raw.SendCommand("SITE SYMLINK") + returnCode, _, err := raw.SendCommand("SITE SYMLINK") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorParameters, rc, "Should have been refused") + require.Equal(t, StatusSyntaxErrorParameters, returnCode, "Should have been refused") - rc, _, err = raw.SendCommand("SITE SYMLINK ") + returnCode, _, err = raw.SendCommand("SITE SYMLINK ") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorParameters, rc, "Should have been refused") + require.Equal(t, StatusSyntaxErrorParameters, returnCode, "Should have been refused") - rc, _, err = raw.SendCommand("SITE SYMLINK file1") + returnCode, _, err = raw.SendCommand("SITE SYMLINK file1") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorParameters, rc, "Should have been refused") + require.Equal(t, StatusSyntaxErrorParameters, returnCode, "Should have been refused") - rc, _, err = raw.SendCommand("SITE SYMLINK file1 file2 file3") + returnCode, _, err = raw.SendCommand("SITE SYMLINK file1 file2 file3") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorParameters, rc, "Should have been refused") + require.Equal(t, StatusSyntaxErrorParameters, returnCode, "Should have been refused") // Creating a bad symlink is authorized - rc, _, err = raw.SendCommand("SITE SYMLINK file3 file4") + returnCode, _, err = raw.SendCommand("SITE SYMLINK file3 file4") require.NoError(t, err) - require.Equal(t, StatusOK, rc, "Should have been accepted") + require.Equal(t, StatusOK, returnCode, "Should have been accepted") // Overwriting a file is not authorized - rc, _, err = raw.SendCommand("SITE SYMLINK file5 file") + returnCode, _, err = raw.SendCommand("SITE SYMLINK file5 file") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc, "Should have been refused") + require.Equal(t, StatusActionNotTaken, returnCode, "Should have been refused") // disable SITE - s.settings.DisableSite = true + server.settings.DisableSite = true - rc, _, err = raw.SendCommand("SITE SYMLINK file test") + returnCode, _, err = raw.SendCommand("SITE SYMLINK file test") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorNotRecognised, rc, "Should have been refused") + require.Equal(t, StatusSyntaxErrorNotRecognised, returnCode, "Should have been refused") - s.settings.DisableSite = false + server.settings.DisableSite = false // Good symlink - rc, _, err = raw.SendCommand("SITE SYMLINK file test") + returnCode, _, err = raw.SendCommand("SITE SYMLINK file test") require.NoError(t, err) - require.Equal(t, StatusOK, rc, "Should have been accepted") + require.Equal(t, StatusOK, returnCode, "Should have been accepted") } func TestSTATFile(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() // Creating a tiny file - ftpUpload(t, c, createTemporaryFile(t, 10), "file") + ftpUpload(t, client, createTemporaryFile(t, 10), "file") // Create a directory with a subdir - _, err = c.Mkdir("dir") + _, err = client.Mkdir("dir") require.NoError(t, err) - _, err = c.Mkdir("/dir/sub") + _, err = client.Mkdir("/dir/sub") require.NoError(t, err) - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - rc, _, err := raw.SendCommand("STAT file") + returnCode, _, err := raw.SendCommand("STAT file") require.NoError(t, err) - require.Equal(t, StatusFileStatus, rc) + require.Equal(t, StatusFileStatus, returnCode) - rc, _, err = raw.SendCommand("STAT dir") + returnCode, _, err = raw.SendCommand("STAT dir") require.NoError(t, err) - require.Equal(t, StatusDirectoryStatus, rc) + require.Equal(t, StatusDirectoryStatus, returnCode) // finally stat a missing file dir - rc, _, err = raw.SendCommand("STAT missing") + returnCode, _, err = raw.SendCommand("STAT missing") require.NoError(t, err) - require.Equal(t, StatusFileActionNotTaken, rc) + require.Equal(t, StatusFileActionNotTaken, returnCode) // the test driver will fail to open this dir - dirName, err := c.Mkdir("fail-to-open") + dirName, err := client.Mkdir("fail-to-open") require.NoError(t, err) - rc, _, err = raw.SendCommand(fmt.Sprintf("STAT %v", dirName)) + returnCode, _, err = raw.SendCommand(fmt.Sprintf("STAT %v", dirName)) require.NoError(t, err) - require.Equal(t, StatusFileActionNotTaken, rc) + require.Equal(t, StatusFileActionNotTaken, returnCode) } func TestMLST(t *testing.T) { - s := NewTestServer(t, false) + req := require.New(t) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) - require.NoError(t, err, "Couldn't connect") + client, err := goftp.DialConfig(conf, server.Addr()) + req.NoError(err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() // Creating a tiny file - ftpUpload(t, c, createTemporaryFile(t, 10), "file") + ftpUpload(t, client, createTemporaryFile(t, 10), "file") - raw, err := c.OpenRawConn() - require.NoError(t, err, "Couldn't open raw connection") + raw, err := client.OpenRawConn() + req.NoError(err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() rc, rsp, err := raw.SendCommand("MLST file") - require.NoError(t, err) - require.Equal(t, StatusFileOK, rc) + req.NoError(err) + req.Equal(StatusFileOK, rc) lines := strings.Split(rsp, "\n") - require.Len(t, lines, 3) + req.Len(lines, 3) path := validMLSxEntryPattern.FindStringSubmatch(lines[1] + "\r\n") if len(path) != 2 { @@ -401,26 +403,26 @@ func TestMDTM(t *testing.T) { User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, s.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() // Creating a tiny file - ftpUpload(t, c, createTemporaryFile(t, 10), "file") + ftpUpload(t, client, createTemporaryFile(t, 10), "file") - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - rc, _, err := raw.SendCommand("MDTM file") + returnCode, _, err := raw.SendCommand("MDTM file") require.NoError(t, err) - require.Equal(t, StatusFileStatus, rc) + require.Equal(t, StatusFileStatus, returnCode) - rc, _, err = raw.SendCommand("MDTM missing") + returnCode, _, err = raw.SendCommand("MDTM missing") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc) + require.Equal(t, StatusActionNotTaken, returnCode) } func TestRename(t *testing.T) { @@ -429,29 +431,29 @@ func TestRename(t *testing.T) { User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, s.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - ftpUpload(t, c, createTemporaryFile(t, 10), "file") + ftpUpload(t, client, createTemporaryFile(t, 10), "file") - err = c.Rename("file", "file1") + err = client.Rename("file", "file1") require.NoError(t, err) // the test driver returns FileNameNotAllowedError in this case, the error code should be 553 instead of 550 - err = c.Rename("file1", "not-allowed") + err = client.Rename("file1", "not-allowed") if assert.Error(t, err) { assert.True(t, strings.Contains(err.Error(), "553-Couldn't rename"), err.Error()) } // renaming a missing file must fail - err = c.Rename("missingfile", "file1") + err = client.Rename("missingfile", "file1") if assert.Error(t, err) { assert.True(t, strings.Contains(err.Error(), "550-Couldn't access"), err.Error()) } - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() @@ -467,23 +469,23 @@ func TestUploadErrorCodes(t *testing.T) { User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, s.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - f := createTemporaryFile(t, 10) - _, err = f.Seek(0, 0) + tempFile := createTemporaryFile(t, 10) + _, err = tempFile.Seek(0, 0) require.NoError(t, err, "Couldn't seek") - err = c.Store("quota-exceeded", f) + err = client.Store("quota-exceeded", tempFile) if assert.Error(t, err) { assert.Contains(t, err.Error(), "552-Could not access file") } - _, err = f.Seek(0, 0) + _, err = tempFile.Seek(0, 0) require.NoError(t, err, "Couldn't seek") - err = c.Store("not-allowed", f) + err = client.Store("not-allowed", tempFile) if assert.Error(t, err) { assert.Contains(t, err.Error(), "553-Could not access file") @@ -491,18 +493,18 @@ func TestUploadErrorCodes(t *testing.T) { } func TestHASHDisabled(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() @@ -513,7 +515,7 @@ func TestHASHDisabled(t *testing.T) { } func TestHASHCommand(t *testing.T) { - s := NewTestServerWithTestDriver( + server := NewTestServerWithTestDriver( t, &TestServerDriver{ Debug: false, @@ -527,12 +529,12 @@ func TestHASHCommand(t *testing.T) { Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - dir, err := c.Mkdir("testdir") + dir, err := client.Mkdir("testdir") require.NoError(t, err) tempFile, err := os.CreateTemp("", "ftpserver") @@ -543,115 +545,115 @@ func TestHASHCommand(t *testing.T) { crc32Sum := "21b0f382" sha256Hash := "ceee704dd96e2b8c2ceca59c4c697bc01123fb9e66a1a3ac34dbdd2d6da9659b" - ftpUpload(t, c, tempFile, "file.txt") + ftpUpload(t, client, tempFile, "file.txt") - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() // ask hash for a directory - rc, _, err := raw.SendCommand(fmt.Sprintf("XSHA256 %v", dir)) + returnCode, _, err := raw.SendCommand(fmt.Sprintf("XSHA256 %v", dir)) require.NoError(t, err) - require.Equal(t, StatusActionNotTakenNoFile, rc) + require.Equal(t, StatusActionNotTakenNoFile, returnCode) // test the HASH command - rc, message, err := raw.SendCommand("HASH file.txt") + returnCode, message, err := raw.SendCommand("HASH file.txt") require.NoError(t, err) - require.Equal(t, StatusFileStatus, rc) + require.Equal(t, StatusFileStatus, returnCode) require.True(t, strings.HasSuffix(message, fmt.Sprintf("SHA-256 0-36 %v file.txt", sha256Hash))) // change algo and request the hash again - rc, message, err = raw.SendCommand("OPTS HASH CRC32") + returnCode, message, err = raw.SendCommand("OPTS HASH CRC32") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) require.Equal(t, "CRC32", message) - rc, message, err = raw.SendCommand("HASH file.txt") + returnCode, message, err = raw.SendCommand("HASH file.txt") require.NoError(t, err) - require.Equal(t, StatusFileStatus, rc) + require.Equal(t, StatusFileStatus, returnCode) require.True(t, strings.HasSuffix(message, fmt.Sprintf("CRC32 0-36 %v file.txt", crc32Sum))) } func TestCustomHASHCommands(t *testing.T) { - s := NewTestServer(t, false) - s.settings.EnableHASH = true + server := NewTestServer(t, false) + server.settings.EnableHASH = true conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() tempFile, err := os.CreateTemp("", "ftpserver") require.NoError(t, err) _, err = tempFile.WriteString("sample data with know checksum/hash\n") require.NoError(t, err) - ftpUpload(t, c, tempFile, "file.txt") + ftpUpload(t, client, tempFile, "file.txt") err = tempFile.Close() require.NoError(t, err) - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() hashMapping := getKnownHASHMappings() - var rc int + var returnCode int var message string for cmd, expected := range hashMapping { - rc, message, err = raw.SendCommand(fmt.Sprintf("%v file.txt", cmd)) + returnCode, message, err = raw.SendCommand(fmt.Sprintf("%v file.txt", cmd)) require.NoError(t, err) - require.Equal(t, StatusFileOK, rc) + require.Equal(t, StatusFileOK, returnCode) require.True(t, strings.HasSuffix(message, expected)) } // now a partial hash - rc, message, err = raw.SendCommand("XSHA256 file.txt 7 11") + returnCode, message, err = raw.SendCommand("XSHA256 file.txt 7 11") require.NoError(t, err) - require.Equal(t, StatusFileOK, rc) + require.Equal(t, StatusFileOK, returnCode) require.True(t, strings.HasSuffix(message, "3a6eb0790f39ac87c94f3856b2dd2c5d110e6811602261a9a923d3bb23adc8b7")) // invalid start - rc, _, err = raw.SendCommand("XSHA256 file.txt a 11") + returnCode, _, err = raw.SendCommand("XSHA256 file.txt a 11") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorParameters, rc) + require.Equal(t, StatusSyntaxErrorParameters, returnCode) // invalid end - rc, _, err = raw.SendCommand("XSHA256 file.txt 7 a") + returnCode, _, err = raw.SendCommand("XSHA256 file.txt 7 a") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorParameters, rc) + require.Equal(t, StatusSyntaxErrorParameters, returnCode) } func TestCOMB(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - rc, message, err := raw.SendCommand("COMB file.bin 1 2") + returnCode, message, err := raw.SendCommand("COMB file.bin 1 2") require.NoError(t, err) - require.Equal(t, StatusCommandNotImplemented, rc, message) + require.Equal(t, StatusCommandNotImplemented, returnCode, message) - s.settings.EnableCOMB = true + server.settings.EnableCOMB = true var parts []*os.File @@ -662,33 +664,33 @@ func TestCOMB(t *testing.T) { createTemporaryFile(t, partSize), createTemporaryFile(t, partSize)) for idx, part := range parts { - ftpUpload(t, c, part, fmt.Sprintf("%d", idx)) + ftpUpload(t, client, part, strconv.Itoa(idx)) _, err = part.Seek(0, io.SeekStart) require.NoError(t, err) _, err = io.Copy(hasher, part) require.NoError(t, err) } - rc, message, err = raw.SendCommand("COMB file.bin 0 1 2 3") + returnCode, message, err = raw.SendCommand("COMB file.bin 0 1 2 3") require.NoError(t, err) - require.Equal(t, StatusFileOK, rc, message) + require.Equal(t, StatusFileOK, returnCode, message) require.Equal(t, "COMB succeeded!", message) - info, err := c.Stat("file.bin") + info, err := client.Stat("file.bin") require.NoError(t, err) require.Equal(t, int64(partSize*4), info.Size()) hashParts := hex.EncodeToString(hasher.Sum(nil)) - hashCombined := ftpDownloadAndHash(t, c, "file.bin") + hashCombined := ftpDownloadAndHash(t, client, "file.bin") require.Equal(t, hashParts, hashCombined) - contents, err := c.ReadDir("/") + contents, err := client.ReadDir("/") require.NoError(t, err) require.Len(t, contents, 1) } func TestCOMBAppend(t *testing.T) { - s := NewTestServerWithTestDriver( + server := NewTestServerWithTestDriver( t, &TestServerDriver{ Debug: false, @@ -702,16 +704,16 @@ func TestCOMBAppend(t *testing.T) { Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() partSize := 1024 hasher := sha256.New() initialFile := createTemporaryFile(t, partSize) - ftpUpload(t, c, initialFile, "file.bin") + ftpUpload(t, client, initialFile, "file.bin") _, err = initialFile.Seek(0, io.SeekStart) require.NoError(t, err) @@ -723,14 +725,14 @@ func TestCOMBAppend(t *testing.T) { parts = append(parts, createTemporaryFile(t, partSize), createTemporaryFile(t, partSize)) for idx, part := range parts { - ftpUpload(t, c, part, fmt.Sprintf(" %d ", idx)) + ftpUpload(t, client, part, fmt.Sprintf(" %d ", idx)) _, err = part.Seek(0, io.SeekStart) require.NoError(t, err) _, err = io.Copy(hasher, part) require.NoError(t, err) } - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() @@ -740,39 +742,39 @@ func TestCOMBAppend(t *testing.T) { require.Equal(t, StatusFileOK, rc, message) require.Equal(t, "COMB succeeded!", message) - info, err := c.Stat("file.bin") + info, err := client.Stat("file.bin") require.NoError(t, err) require.Equal(t, int64(partSize*3), info.Size()) hashParts := hex.EncodeToString(hasher.Sum(nil)) - hashCombined := ftpDownloadAndHash(t, c, "file.bin") + hashCombined := ftpDownloadAndHash(t, client, "file.bin") require.Equal(t, hashParts, hashCombined) - contents, err := c.ReadDir("/") + contents, err := client.ReadDir("/") require.NoError(t, err) require.Len(t, contents, 1) } func TestCOMBCloseError(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - s.settings.EnableCOMB = true + server.settings.EnableCOMB = true - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - ftpUpload(t, c, createTemporaryFile(t, 10), "1.bin") - ftpUpload(t, c, createTemporaryFile(t, 10), "2.bin") + ftpUpload(t, client, createTemporaryFile(t, 10), "1.bin") + ftpUpload(t, client, createTemporaryFile(t, 10), "2.bin") rc, message, err := raw.SendCommand("COMB fail-to-close.bin 1.bin 2.bin") require.NoError(t, err) @@ -781,116 +783,116 @@ func TestCOMBCloseError(t *testing.T) { } func TestREST(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - rc, response, err := raw.SendCommand("TYPE A") + returnCode, response, err := raw.SendCommand("TYPE A") require.NoError(t, err) - require.Equal(t, StatusOK, rc, response) + require.Equal(t, StatusOK, returnCode, response) - rc, response, err = raw.SendCommand("REST 10") + returnCode, response, err = raw.SendCommand("REST 10") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorParameters, rc, response) + require.Equal(t, StatusSyntaxErrorParameters, returnCode, response) - rc, response, err = raw.SendCommand("TYPE I") + returnCode, response, err = raw.SendCommand("TYPE I") require.NoError(t, err) - require.Equal(t, StatusOK, rc, response) + require.Equal(t, StatusOK, returnCode, response) - rc, response, err = raw.SendCommand("REST a") + returnCode, response, err = raw.SendCommand("REST a") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc, response) + require.Equal(t, StatusActionNotTaken, returnCode, response) require.True(t, strings.HasPrefix(response, "Couldn't parse size")) } func TestSIZE(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - rc, response, err := raw.SendCommand("SIZE file.bin") + returnCode, response, err := raw.SendCommand("SIZE file.bin") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc, response) + require.Equal(t, StatusActionNotTaken, returnCode, response) require.True(t, strings.HasPrefix(response, "Couldn't access")) - ftpUpload(t, c, createTemporaryFile(t, 10), "file.bin") + ftpUpload(t, client, createTemporaryFile(t, 10), "file.bin") - rc, response, err = raw.SendCommand("SIZE file.bin") + returnCode, response, err = raw.SendCommand("SIZE file.bin") require.NoError(t, err) - require.Equal(t, StatusFileStatus, rc, response) + require.Equal(t, StatusFileStatus, returnCode, response) require.Equal(t, "10", response) - rc, response, err = raw.SendCommand("TYPE A") + returnCode, response, err = raw.SendCommand("TYPE A") require.NoError(t, err) - require.Equal(t, StatusOK, rc, response) + require.Equal(t, StatusOK, returnCode, response) - rc, response, err = raw.SendCommand("SIZE file.bin") + returnCode, response, err = raw.SendCommand("SIZE file.bin") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc, response) + require.Equal(t, StatusActionNotTaken, returnCode, response) require.Equal(t, "SIZE not allowed in ASCII mode", response) } func TestCOMBErrors(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - s.settings.EnableCOMB = true + server.settings.EnableCOMB = true - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - rc, message, err := raw.SendCommand("COMB") + returnCode, message, err := raw.SendCommand("COMB") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorParameters, rc, message) + require.Equal(t, StatusSyntaxErrorParameters, returnCode, message) - rc, message, err = raw.SendCommand("COMB file.bin") + returnCode, message, err = raw.SendCommand("COMB file.bin") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorParameters, rc, message) + require.Equal(t, StatusSyntaxErrorParameters, returnCode, message) - rc, message, err = raw.SendCommand("COMB file.bin missing") + returnCode, message, err = raw.SendCommand("COMB file.bin missing") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc, message) + require.Equal(t, StatusActionNotTaken, returnCode, message) - rc, message, err = raw.SendCommand("COMB /missing/file.bin file.bin") + returnCode, message, err = raw.SendCommand("COMB /missing/file.bin file.bin") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc, message) + require.Equal(t, StatusActionNotTaken, returnCode, message) - rc, message, err = raw.SendCommand("COMB file.bin \"\"") + returnCode, message, err = raw.SendCommand("COMB file.bin \"\"") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc, message) + require.Equal(t, StatusActionNotTaken, returnCode, message) } type quotedParams struct { @@ -928,14 +930,14 @@ func TestUnquoteCOMBParams(t *testing.T) { }, } - for _, p := range testQuotedParams { - parsed, err := unquoteSpaceSeparatedParams(p.params) + for _, params := range testQuotedParams { + parsed, err := unquoteSpaceSeparatedParams(params.params) - if p.wantError { + if params.wantError { require.Error(t, err) } else { require.NoError(t, err) - require.Equal(t, p.parsed, parsed) + require.Equal(t, params.parsed, parsed) } } } diff --git a/handle_misc.go b/handle_misc.go index ffb8ae6e..154da82a 100644 --- a/handle_misc.go +++ b/handle_misc.go @@ -5,6 +5,7 @@ import ( "crypto/tls" "errors" "fmt" + "strconv" "strings" "time" ) @@ -89,7 +90,7 @@ func (c *clientHandler) handleSITE(param string) error { case "RMDIR": c.handleRMDIR(params) default: - c.writeMessage(StatusSyntaxErrorNotRecognised, fmt.Sprintf("Unknown SITE subcommand: %s", cmd)) + c.writeMessage(StatusSyntaxErrorNotRecognised, "Unknown SITE subcommand: "+cmd) } return nil @@ -121,7 +122,7 @@ func (c *clientHandler) handleSTATServer() error { )) if c.user != "" { - c.writeLine(fmt.Sprintf("Logged in as %s", c.user)) + c.writeLine("Logged in as " + c.user) } else { c.writeLine("Not logged in yet") } @@ -136,40 +137,50 @@ func (c *clientHandler) handleSTATServer() error { return nil } -func (c *clientHandler) handleOPTS(param string) error { - args := strings.SplitN(param, " ", 2) - if strings.EqualFold(args[0], "UTF8") { - c.writeMessage(StatusOK, "I'm in UTF8 only anyway") +func (c *clientHandler) handleOptsUtf8() error { + c.writeMessage(StatusOK, "I'm in UTF8 only anyway") - return nil - } - - if strings.EqualFold(args[0], "HASH") && c.server.settings.EnableHASH { - hashMapping := getHashMapping() + return nil +} - if len(args) > 1 { - // try to change the current hash algorithm to the requested one - if value, ok := hashMapping[args[1]]; ok { - c.selectedHashAlgo = value - c.writeMessage(StatusOK, args[1]) - } else { - c.writeMessage(StatusSyntaxErrorParameters, "Unknown algorithm, current selection not changed") - } +func (c *clientHandler) handleOptsHash(args []string) error { + hashMapping := getHashMapping() - return nil + if len(args) > 0 { + // try to change the current hash algorithm to the requested one + if value, ok := hashMapping[args[0]]; ok { + c.selectedHashAlgo = value + c.writeMessage(StatusOK, args[0]) + } else { + c.writeMessage(StatusSyntaxErrorParameters, "Unknown algorithm, current selection not changed") } - // return the current hash algorithm - var currentHash string - for k, v := range hashMapping { - if v == c.selectedHashAlgo { - currentHash = k - } + return nil + } + // return the current hash algorithm + var currentHash string + + for k, v := range hashMapping { + if v == c.selectedHashAlgo { + currentHash = k } + } - c.writeMessage(StatusOK, currentHash) + c.writeMessage(StatusOK, currentHash) - return nil + return nil +} + +func (c *clientHandler) handleOPTS(param string) error { + args := strings.SplitN(param, " ", 2) + + switch strings.ToUpper(args[0]) { + case "UTF8": + return c.handleOptsUtf8() + case "HASH": + if c.server.settings.EnableHASH { + return c.handleOptsHash(args[1:]) + } } c.writeMessage(StatusSyntaxErrorNotRecognised, "Don't know this option") @@ -334,7 +345,7 @@ func (c *clientHandler) handleAVBL(param string) error { } if !info.IsDir() { - c.writeMessage(StatusActionNotTaken, fmt.Sprintf("%s: is not a directory", path)) + c.writeMessage(StatusActionNotTaken, path+": is not a directory") return nil } @@ -346,7 +357,7 @@ func (c *clientHandler) handleAVBL(param string) error { return nil } - c.writeMessage(StatusFileStatus, fmt.Sprintf("%d", available)) + c.writeMessage(StatusFileStatus, strconv.FormatInt(available, 10)) } else { c.writeMessage(StatusNotImplemented, "This extension hasn't been implemented !") } diff --git a/handle_misc_test.go b/handle_misc_test.go index ad9dcd92..8f5448ac 100644 --- a/handle_misc_test.go +++ b/handle_misc_test.go @@ -7,27 +7,25 @@ import ( "path/filepath" "strings" "testing" - "time" "github.com/secsy/goftp" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestSiteCommand(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() @@ -44,13 +42,13 @@ func TestSiteCommand(t *testing.T) { // will timeout. I handle idle timeout myself in SFTPGo but you could be // interested to fix this bug func TestIdleTimeout(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{Debug: false, Settings: &Settings{IdleTimeout: 2}}) + server := NewTestServerWithTestDriver(t, &TestServerDriver{Debug: false, Settings: &Settings{IdleTimeout: 2}}) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + c, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") defer func() { panicOnError(c.Close()) }() @@ -62,55 +60,55 @@ func TestIdleTimeout(t *testing.T) { time.Sleep(time.Second * 1) // < 2s : OK - rc, _, err := raw.SendCommand("NOOP") + returnCode, _, err := raw.SendCommand("NOOP") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) time.Sleep(time.Second * 3) // > 2s : Timeout - rc, _, err = raw.SendCommand("NOOP") + returnCode, _, err = raw.SendCommand("NOOP") require.NoError(t, err) - require.Equal(t, StatusServiceNotAvailable, rc) + require.Equal(t, StatusServiceNotAvailable, returnCode) } func TestStat(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") - rc, str, err := raw.SendCommand("STAT") + returnCode, str, err := raw.SendCommand("STAT") require.NoError(t, err) - require.Equal(t, StatusSystemStatus, rc) + require.Equal(t, StatusSystemStatus, returnCode) count := strings.Count(str, "\n") require.GreaterOrEqual(t, count, 4) require.NotEqual(t, ' ', str[0]) - s.settings.DisableSTAT = true + server.settings.DisableSTAT = true - rc, str, err = raw.SendCommand("STAT") + returnCode, str, err = raw.SendCommand("STAT") require.NoError(t, err) - require.Equal(t, StatusCommandNotImplemented, rc, str) + require.Equal(t, StatusCommandNotImplemented, returnCode, str) } func TestCLNT(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + c, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") defer func() { panicOnError(c.Close()) }() @@ -126,18 +124,18 @@ func TestCLNT(t *testing.T) { } func TestOPTSUTF8(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() @@ -151,7 +149,7 @@ func TestOPTSUTF8(t *testing.T) { } func TestOPTSHASH(t *testing.T) { - s := NewTestServerWithTestDriver( + server := NewTestServerWithTestDriver( t, &TestServerDriver{ Debug: false, @@ -165,89 +163,89 @@ func TestOPTSHASH(t *testing.T) { Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - rc, message, err := raw.SendCommand("OPTS HASH") + returnCode, message, err := raw.SendCommand("OPTS HASH") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode, message) require.Equal(t, "SHA-256", message) - rc, message, err = raw.SendCommand("OPTS HASH MD5") + returnCode, message, err = raw.SendCommand("OPTS HASH MD5") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) require.Equal(t, "MD5", message) - rc, message, err = raw.SendCommand("OPTS HASH CRC-37") + returnCode, message, err = raw.SendCommand("OPTS HASH CRC-37") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorParameters, rc) + require.Equal(t, StatusSyntaxErrorParameters, returnCode) require.Equal(t, "Unknown algorithm, current selection not changed", message) - rc, message, err = raw.SendCommand("OPTS HASH") + returnCode, message, err = raw.SendCommand("OPTS HASH") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) require.Equal(t, "MD5", message) // now disable hash support - s.settings.EnableHASH = false + server.settings.EnableHASH = false - rc, _, err = raw.SendCommand("OPTS HASH") + returnCode, _, err = raw.SendCommand("OPTS HASH") require.NoError(t, err) - require.Equal(t, StatusSyntaxErrorNotRecognised, rc) + require.Equal(t, StatusSyntaxErrorNotRecognised, returnCode) } func TestAVBL(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - rc, response, err := raw.SendCommand("AVBL") + returnCode, response, err := raw.SendCommand("AVBL") require.NoError(t, err) - require.Equal(t, StatusFileStatus, rc) + require.Equal(t, StatusFileStatus, returnCode) require.Equal(t, "123", response) // a missing dir - rc, _, err = raw.SendCommand("AVBL missing") + returnCode, _, err = raw.SendCommand("AVBL missing") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc) + require.Equal(t, StatusActionNotTaken, returnCode) // AVBL on a file path - ftpUpload(t, c, createTemporaryFile(t, 10), "file") + ftpUpload(t, client, createTemporaryFile(t, 10), "file") - rc, response, err = raw.SendCommand("AVBL file") + returnCode, response, err = raw.SendCommand("AVBL file") require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc) + require.Equal(t, StatusActionNotTaken, returnCode) require.Equal(t, "/file: is not a directory", response) - noavblDir, err := c.Mkdir("noavbl") + noavblDir, err := client.Mkdir("noavbl") require.NoError(t, err) - rc, response, err = raw.SendCommand(fmt.Sprintf("AVBL %v", noavblDir)) + returnCode, response, err = raw.SendCommand(fmt.Sprintf("AVBL %v", noavblDir)) require.NoError(t, err) - require.Equal(t, StatusActionNotTaken, rc) + require.Equal(t, StatusActionNotTaken, returnCode) require.Equal(t, fmt.Sprintf("Couldn't get space for path %v: %v", noavblDir, errAvblNotPermitted.Error()), response) } func TestQuit(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Debug: false, TLS: true, }) @@ -260,12 +258,12 @@ func TestQuit(t *testing.T) { }, TLSMode: goftp.TLSExplicit, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() @@ -275,16 +273,16 @@ func TestQuit(t *testing.T) { require.Equal(t, StatusClosingControlConn, rc) } -func TestQuitWithCustomMessage(_t *testing.T) { - d := &MesssageDriver{ +func TestQuitWithCustomMessage(t *testing.T) { + driver := &MesssageDriver{ TestServerDriver{ Debug: true, TLS: true, }, } - d.Init() - s := NewTestServerWithDriver(_t, d) - t := require.New(_t) + driver.Init() + server := NewTestServerWithDriver(t, driver) + req := require.New(t) conf := goftp.Config{ User: authUser, Password: authPass, @@ -294,123 +292,129 @@ func TestQuitWithCustomMessage(_t *testing.T) { }, TLSMode: goftp.TLSExplicit, } - c, err := goftp.DialConfig(conf, s.Addr()) - t.NoError(err, "Couldn't connect") + c, err := goftp.DialConfig(conf, server.Addr()) + req.NoError(err, "Couldn't connect") defer func() { panicOnError(c.Close()) }() raw, err := c.OpenRawConn() - t.NoError(err, "Couldn't open raw connection") + req.NoError(err, "Couldn't open raw connection") rc, msg, err := raw.SendCommand("QUIT") - t.NoError(err) - t.Equal(StatusClosingControlConn, rc) - t.Equal("Sayonara, bye bye!", msg) + req.NoError(err) + req.Equal(StatusClosingControlConn, rc) + req.Equal("Sayonara, bye bye!", msg) } func TestQuitWithTransferInProgress(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + req := require.New(t) + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Debug: false, }) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - ch := make(chan struct{}, 1) - go func() { - defer close(ch) + syncChannel := make(chan struct{}, 1) + go + // I don't see a pragmatic/good way to test this without forwarding the errors to the channel, + // and thus losing the convenience of testify. + //nolint:testifylint + func() { + defer close(syncChannel) dcGetter, err := raw.PrepareDataConn() //nolint:govet - require.NoError(t, err) + req.NoError(err) + file := createTemporaryFile(t, 256*1024) fileName := filepath.Base(file.Name()) rc, response, err := raw.SendCommand(fmt.Sprintf("%s %s", "STOR", fileName)) - require.NoError(t, err) - require.Equal(t, StatusFileStatusOK, rc, response) + req.NoError(err) + req.Equal(StatusFileStatusOK, rc, response) - dc, err := dcGetter() - assert.NoError(t, err) + dataConn, err := dcGetter() + req.NoError(err) - ch <- struct{}{} + syncChannel <- struct{}{} // wait some more time to be sure we send the QUIT command before starting the file copy time.Sleep(100 * time.Millisecond) - _, err = io.Copy(dc, file) - assert.NoError(t, err) + _, err = io.Copy(dataConn, file) + req.NoError(err) - err = dc.Close() - assert.NoError(t, err) + err = dataConn.Close() + req.NoError(err) }() - // wait for the trasfer to start - <-ch + // wait for the transfer to start + <-syncChannel // we send a QUIT command after sending STOR and before the transfer ends. // We expect the transfer close response and then the QUIT response - rc, _, err := raw.SendCommand("QUIT") - require.NoError(t, err) - require.Equal(t, StatusClosingDataConn, rc) + returnCode, _, err := raw.SendCommand("QUIT") + req.NoError(err) + req.Equal(StatusClosingDataConn, returnCode) - rc, _, err = raw.ReadResponse() - require.NoError(t, err) - require.Equal(t, StatusClosingControlConn, rc) + returnCode, _, err = raw.ReadResponse() + req.NoError(err) + req.Equal(StatusClosingControlConn, returnCode) } func TestTYPE(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() - rc, _, err := raw.SendCommand("TYPE I") + returnCode, _, err := raw.SendCommand("TYPE I") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) - rc, _, err = raw.SendCommand("TYPE A") + returnCode, _, err = raw.SendCommand("TYPE A") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) - rc, _, err = raw.SendCommand("TYPE A N") + returnCode, _, err = raw.SendCommand("TYPE A N") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) - rc, _, err = raw.SendCommand("TYPE i") + returnCode, _, err = raw.SendCommand("TYPE i") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) - rc, _, err = raw.SendCommand("TYPE a") + returnCode, _, err = raw.SendCommand("TYPE a") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) - rc, _, err = raw.SendCommand("TYPE l 8") + returnCode, _, err = raw.SendCommand("TYPE l 8") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) - rc, _, err = raw.SendCommand("TYPE l 7") + returnCode, _, err = raw.SendCommand("TYPE l 7") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) - rc, _, err = raw.SendCommand("TYPE wrong") + returnCode, _, err = raw.SendCommand("TYPE wrong") require.NoError(t, err) - require.Equal(t, StatusNotImplementedParam, rc) + require.Equal(t, StatusNotImplementedParam, returnCode) } diff --git a/server.go b/server.go index 7180ab96..91b4b0a5 100644 --- a/server.go +++ b/server.go @@ -6,7 +6,6 @@ import ( "errors" "fmt" "net" - "os" "syscall" "time" @@ -14,10 +13,8 @@ import ( lognoop "github.com/fclairamb/go-log/noop" ) -var ( - // ErrNotListening is returned when we are performing an action that is only valid while listening - ErrNotListening = errors.New("we aren't listening") -) +// ErrNotListening is returned when we are performing an action that is only valid while listening +var ErrNotListening = errors.New("we aren't listening") // CommandDescription defines which function should be used and if it should be open to anyone or only logged in users type CommandDescription struct { @@ -29,7 +26,7 @@ type CommandDescription struct { // This is shared between FtpServer instances as there's no point in making the FTP commands behave differently // between them. -var commandsMap = map[string]*CommandDescription{ +var commandsMap = map[string]*CommandDescription{ //nolint:gochecknoglobals // Authentication "USER": {Fn: (*clientHandler).handleUSER, Open: true}, "PASS": {Fn: (*clientHandler).handlePASS, Open: true}, @@ -96,9 +93,7 @@ var commandsMap = map[string]*CommandDescription{ "EPRT": {Fn: (*clientHandler).handlePORT}, } -var ( - specialAttentionCommands = []string{"ABOR", "STAT", "QUIT"} -) +var specialAttentionCommands = []string{"ABOR", "STAT", "QUIT"} //nolint:gochecknoglobals // FtpServer is where everything is stored // We want to keep it as simple as possible @@ -111,53 +106,59 @@ type FtpServer struct { } func (server *FtpServer) loadSettings() error { - s, err := server.driver.GetSettings() + settings, err := server.driver.GetSettings() - if err != nil { - return err + if err != nil || settings == nil { + return newDriverError("couldn't load settings", err) } - if s.PublicHost != "" { - parsedIP := net.ParseIP(s.PublicHost) - if parsedIP == nil { - return &ipValidationError{error: fmt.Sprintf("invalid passive IP %#v", s.PublicHost)} - } - - parsedIP = parsedIP.To4() - if parsedIP == nil { - return &ipValidationError{error: fmt.Sprintf("invalid IPv4 passive IP %#v", s.PublicHost)} + if settings.PublicHost != "" { + settings.PublicHost, err = parseIPv4(settings.PublicHost) + if err != nil { + return err } - - s.PublicHost = parsedIP.String() } - if s.Listener == nil && s.ListenAddr == "" { - s.ListenAddr = "0.0.0.0:2121" + if settings.Listener == nil && settings.ListenAddr == "" { + settings.ListenAddr = "0.0.0.0:2121" } // florent(2018-01-14): #58: IDLE timeout: Default idle timeout will be set at 900 seconds - if s.IdleTimeout == 0 { - s.IdleTimeout = 900 + if settings.IdleTimeout == 0 { + settings.IdleTimeout = 900 } - if s.ConnectionTimeout == 0 { - s.ConnectionTimeout = 30 + if settings.ConnectionTimeout == 0 { + settings.ConnectionTimeout = 30 } - if s.Banner == "" { - s.Banner = "ftpserver - golang FTP server" + if settings.Banner == "" { + settings.Banner = "ftpserver - golang FTP server" } - server.settings = s + server.settings = settings return nil } +func parseIPv4(publicHost string) (string, error) { + parsedIP := net.ParseIP(publicHost) + if parsedIP == nil { + return "", &ipValidationError{error: fmt.Sprintf("invalid passive IP %#v", publicHost)} + } + + parsedIP = parsedIP.To4() + if parsedIP == nil { + return "", &ipValidationError{error: fmt.Sprintf("invalid IPv4 passive IP %#v", publicHost)} + } + + return parsedIP.String(), nil +} + // Listen starts the listening // It's not a blocking call func (server *FtpServer) Listen() error { err := server.loadSettings() - if err != nil { return fmt.Errorf("could not load settings: %w", err) } @@ -167,43 +168,46 @@ func (server *FtpServer) Listen() error { server.listener = server.settings.Listener } else { // Otherwise, it's what we currently use - server.listener, err = net.Listen("tcp", server.settings.ListenAddr) - + server.listener, err = server.createListener() if err != nil { - server.Logger.Error("Cannot listen", "err", err) - - return err + return fmt.Errorf("could not create listener: %w", err) } + } - if server.settings.TLSRequired == ImplicitEncryption { - // implicit TLS - var tlsConfig *tls.Config + server.Logger.Info("Listening...", "address", server.listener.Addr()) - tlsConfig, err = server.driver.GetTLSConfig() - if err != nil { - server.Logger.Error("Cannot get tls config", "err", err) + return nil +} - return err - } +func (server *FtpServer) createListener() (net.Listener, error) { + listener, err := net.Listen("tcp", server.settings.ListenAddr) + if err != nil { + server.Logger.Error("cannot listen on main port", "err", err, "listenAddr", server.settings.ListenAddr) - server.listener = tls.NewListener(server.listener, tlsConfig) - } + return nil, newNetworkError("cannot listen on main port", err) } - server.Logger.Info("Listening...", "address", server.listener.Addr()) + if server.settings.TLSRequired == ImplicitEncryption { + // implicit TLS + var tlsConfig *tls.Config + + tlsConfig, err = server.driver.GetTLSConfig() + if err != nil || tlsConfig == nil { + server.Logger.Error("Cannot get tls config", "err", err) + + return nil, newDriverError("cannot get tls config", err) + } + + listener = tls.NewListener(listener, tlsConfig) + } - return err + return listener, nil } func temporaryError(err net.Error) bool { - if opErr, ok := err.(*net.OpError); ok { - if sysErr, ok := opErr.Err.(*os.SyscallError); ok { - if errno, ok := sysErr.Err.(syscall.Errno); ok { - if errno == syscall.ECONNABORTED || - errno == syscall.ECONNRESET { - return true - } - } + if syscallErrNo := new(syscall.Errno); errors.As(err, syscallErrNo) { + if *syscallErrNo == syscall.ECONNABORTED || *syscallErrNo == syscall.ECONNRESET { + return true } } @@ -216,43 +220,12 @@ func (server *FtpServer) Serve() error { for { connection, err := server.listener.Accept() - if err != nil { - if errOp, ok := err.(*net.OpError); ok { - // This means we just closed the connection and it's OK - if errOp.Err.Error() == "use of closed network connection" { - server.listener = nil - - return nil - } - } - - // see https://github.com/golang/go/blob/4aa1efed4853ea067d665a952eee77c52faac774/src/net/http/server.go#L3046 - // & https://github.com/fclairamb/ftpserverlib/pull/352#pullrequestreview-1077459896 - // The temporaryError method should replace net.Error.Temporary() when the go team - // will have provided us a better way to detect temporary errors. - if ne, ok := err.(net.Error); ok && ne.Temporary() { //nolint:staticcheck - if tempDelay == 0 { - tempDelay = 5 * time.Millisecond - } else { - tempDelay *= 2 - } - - if max := 1 * time.Second; tempDelay > max { - tempDelay = max - } - - server.Logger.Warn( - "accept error", err, - "retry delay", tempDelay) - time.Sleep(tempDelay) - - continue + if ok, finalErr := server.handleAcceptError(err, &tempDelay); ok { + return finalErr } - server.Logger.Error("Listener accept error", "err", err) - - return err + continue } tempDelay = 0 @@ -261,6 +234,50 @@ func (server *FtpServer) Serve() error { } } +// handleAcceptError handles the error that occurred when accepting a new connection +// It returns a boolean indicating if the error should stop the server and the error itself or none if it's a standard +// scenario (e.g. a closed listener) +func (server *FtpServer) handleAcceptError(err error, tempDelay *time.Duration) (bool, error) { + server.Logger.Error("Serve error", "err", err) + + if errOp := (&net.OpError{}); errors.As(err, &errOp) { + // This means we just closed the connection and it's OK + if errOp.Err.Error() == "use of closed network connection" { + server.listener = nil + + return true, nil + } + } + + // see https://github.com/golang/go/blob/4aa1efed4853ea067d665a952eee77c52faac774/src/net/http/server.go#L3046 + // & https://github.com/fclairamb/ftpserverlib/pull/352#pullrequestreview-1077459896 + // The temporaryError method should replace net.Error.Temporary() when the go team + // will have provided us a better way to detect temporary errors. + var ne net.Error + if errors.Is(err, ne) && ne.Temporary() { //nolint:staticcheck + if *tempDelay == 0 { + *tempDelay = 5 * time.Millisecond + } else { + *tempDelay *= 2 + } + + if max := 1 * time.Second; *tempDelay > max { + *tempDelay = max + } + + server.Logger.Warn( + "accept error", err, + "retry delay", tempDelay) + time.Sleep(*tempDelay) + + return false, nil + } + + server.Logger.Error("Listener accept error", "err", err) + + return true, newNetworkError("listener accept error", err) +} + // ListenAndServe simply chains the Listen and Serve method calls func (server *FtpServer) ListenAndServe() error { if err := server.Listen(); err != nil { @@ -295,15 +312,16 @@ func (server *FtpServer) Stop() error { return ErrNotListening } - err := server.listener.Close() - if err != nil { + if err := server.listener.Close(); err != nil { server.Logger.Warn( "Could not close listener", "err", err, ) + + return newNetworkError("couln't close listener", err) } - return err + return nil } // When a client connects, the server could refuse the connection diff --git a/server_test.go b/server_test.go index 14ac52bd..d441ae5a 100644 --- a/server_test.go +++ b/server_test.go @@ -2,25 +2,21 @@ package ftpserver import ( "errors" - "fmt" "net" "os" "syscall" "testing" "time" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - lognoop "github.com/fclairamb/go-log/noop" + "github.com/stretchr/testify/require" ) func TestMain(m *testing.M) { // we change the timezone to be able to test that MLSD/MLST commands write UTC timestamps loc, err := time.LoadLocation("America/New_York") if err != nil { - fmt.Printf("unable to set timezone: %v\n", err) - os.Exit(1) + panic(err) } time.Local = loc @@ -84,6 +80,53 @@ func newFakeListener(err error) net.Listener { } } +func TestCannotListen(t *testing.T) { + req := require.New(t) + + portBlockerListener, err := net.Listen("tcp", "127.0.0.1:0") + req.NoError(err) + + defer func() { req.NoError(portBlockerListener.Close()) }() + + server := FtpServer{ + Logger: lognoop.NewNoOpLogger(), + driver: &TestServerDriver{ + Settings: &Settings{ + ListenAddr: portBlockerListener.Addr().String(), + }, + }, + } + + err = server.Listen() + var ne NetworkError + req.ErrorAs(err, &ne) + req.Equal("cannot listen on main port", ne.str) +} + +func TestListenWithBadTLSSettings(t *testing.T) { + req := require.New(t) + + portBlockerListener, err := net.Listen("tcp", "127.0.0.1:0") + req.NoError(err) + + defer func() { req.NoError(portBlockerListener.Close()) }() + + server := FtpServer{ + Logger: lognoop.NewNoOpLogger(), + driver: &TestServerDriver{ + Settings: &Settings{ + TLSRequired: ImplicitEncryption, + }, + TLS: false, + }, + } + + err = server.Listen() + var drvErr DriverError + req.ErrorAs(err, &drvErr) + req.Equal("cannot get tls config", drvErr.str) +} + func TestListenerAcceptErrors(t *testing.T) { errNetFake := &fakeNetError{error: errListenerAccept} @@ -92,7 +135,7 @@ func TestListenerAcceptErrors(t *testing.T) { Logger: lognoop.NewNoOpLogger(), } err := server.Serve() - require.EqualError(t, err, errListenerAccept.Error()) + require.ErrorContains(t, err, errListenerAccept.Error()) } func TestPortCommandFormatOK(t *testing.T) { @@ -123,7 +166,6 @@ func TestQuoteDoubling(t *testing.T) { args args want string }{ - // TODO: Add test cases. {"1", args{" white space"}, " white space"}, {"1", args{` one" quote`}, ` one"" quote`}, {"1", args{` two"" quote`}, ` two"""" quote`}, @@ -137,51 +179,77 @@ func TestQuoteDoubling(t *testing.T) { } } -func TestServerSettings(t *testing.T) { +func TestServerSettingsIPError(t *testing.T) { server := FtpServer{ Logger: lognoop.NewNoOpLogger(), - driver: &TestServerDriver{ + } + + t.Run("IPv4 with 3 numbers", func(t *testing.T) { + server.driver = &TestServerDriver{ Settings: &Settings{ PublicHost: "127.0.0", }, - }, - } - err := server.loadSettings() - _, ok := err.(*ipValidationError) - require.True(t, ok) + } - server.driver = &TestServerDriver{ - Settings: &Settings{ - PublicHost: "::1", - }, - } - err = server.loadSettings() - _, ok = err.(*ipValidationError) - require.True(t, ok) + err := server.loadSettings() + _, ok := err.(*ipValidationError) //nolint:errorlint // Here we want to test the exact error match + require.True(t, ok) + }) - server.driver = &TestServerDriver{ - Settings: &Settings{ - PublicHost: "::ffff:192.168.1.1", + t.Run("localhost public host", func(t *testing.T) { + server.driver = &TestServerDriver{ + Settings: &Settings{ + PublicHost: "::1", + }, + } + + err := server.loadSettings() + _, ok := err.(*ipValidationError) //nolint:errorlint // Here we want to test the exact error match + require.True(t, ok) + }) + + t.Run("Strangely looking IPv6/IPv4 address", func(t *testing.T) { + server.driver = &TestServerDriver{ + Settings: &Settings{ + PublicHost: "::ffff:192.168.1.1", + }, + } + err := server.loadSettings() + require.NoError(t, err) + require.Equal(t, "192.168.1.1", server.settings.PublicHost) + }) +} + +func TestServerSettingsNilSettings(t *testing.T) { + req := require.New(t) + server := FtpServer{ + Logger: lognoop.NewNoOpLogger(), + driver: &TestServerDriver{ + Settings: nil, }, } - err = server.loadSettings() - require.NoError(t, err) - require.Equal(t, "192.168.1.1", server.settings.PublicHost) + + err := server.loadSettings() + req.Error(err) + + drvErr := DriverError{} + req.ErrorAs(err, &drvErr) + req.ErrorContains(drvErr, "couldn't load settings") } func TestTemporaryError(t *testing.T) { - a := assert.New(t) + req := require.New(t) // Test the temporaryError function - a.False(temporaryError(nil)) - a.False(temporaryError(&fakeNetError{error: errListenerAccept})) - a.False(temporaryError(&net.OpError{ + req.False(temporaryError(nil)) + req.False(temporaryError(&fakeNetError{error: errListenerAccept})) + req.False(temporaryError(&net.OpError{ Err: &fakeNetError{error: errListenerAccept}, })) for _, serr := range []syscall.Errno{syscall.ECONNABORTED, syscall.ECONNRESET} { - a.True(temporaryError(&net.OpError{Err: &os.SyscallError{Err: serr}})) + req.True(temporaryError(&net.OpError{Err: &os.SyscallError{Err: serr}})) } - a.False(temporaryError(&net.OpError{Err: &os.SyscallError{Err: syscall.EAGAIN}})) + req.False(temporaryError(&net.OpError{Err: &os.SyscallError{Err: syscall.EAGAIN}})) } diff --git a/transfer_active.go b/transfer_active.go index 8e8346fc..8ba853a6 100644 --- a/transfer_active.go +++ b/transfer_active.go @@ -97,14 +97,10 @@ func (a *activeTransferHandler) Open() (net.Conn, error) { dialer.LocalAddr, _ = net.ResolveTCPAddr("tcp", ":20") dialer.Control = Control } - // TODO(mgenov): support dialing with timeout - // Issues: - // https://github.com/golang/go/issues/3097 - // https://github.com/golang/go/issues/4842 - conn, err := dialer.Dial("tcp", a.raddr.String()) + conn, err := dialer.Dial("tcp", a.raddr.String()) if err != nil { - return nil, fmt.Errorf("could not establish active connection: %w", err) + return nil, newNetworkError("could not establish active connection", err) } if a.tlsConfig != nil { @@ -120,7 +116,9 @@ func (a *activeTransferHandler) Open() (net.Conn, error) { // Close closes only if connection is established func (a *activeTransferHandler) Close() error { if a.conn != nil { - return a.conn.Close() + if err := a.conn.Close(); err != nil { + return newNetworkError("could not close active connection", err) + } } return nil @@ -144,28 +142,35 @@ func parsePORTAddr(param string) (*net.TCPAddr, error) { params := strings.Split(param, ",") - ip := strings.Join(params[0:4], ".") + ipParts := strings.Join(params[0:4], ".") - p1, err := strconv.Atoi(params[4]) + portByte1, err := strconv.Atoi(params[4]) if err != nil { - return nil, err + return nil, ErrRemoteAddrFormat } - p2, err := strconv.Atoi(params[5]) - + portByte2, err := strconv.Atoi(params[5]) if err != nil { - return nil, err + return nil, ErrRemoteAddrFormat } - port := p1<<8 + p2 + port := portByte1<<8 + portByte2 + + addr, err := net.ResolveTCPAddr("tcp", fmt.Sprintf("%s:%d", ipParts, port)) + if err != nil { + err = newNetworkError("could not resolve "+param, err) + } - return net.ResolveTCPAddr("tcp", fmt.Sprintf("%s:%d", ip, port)) + return addr, err } // Parse EPRT parameter. Full EPRT command format: // - IPv4 : "EPRT |1|h1.h2.h3.h4|port|\r\n" // - IPv6 : "EPRT |2|h1::h2:h3:h4:h5|port|\r\n" -func parseEPRTAddr(param string) (addr *net.TCPAddr, err error) { +func parseEPRTAddr(param string) (*net.TCPAddr, error) { + var addr *net.TCPAddr + var err error + params := strings.Split(param, "|") if len(params) != 5 { return nil, ErrRemoteAddrFormat @@ -181,13 +186,13 @@ func parseEPRTAddr(param string) (addr *net.TCPAddr, err error) { return nil, ErrRemoteAddrFormat } - var ip net.IP + var ipAddress net.IP switch netProtocol { case "1", "2": // use protocol 1 means IPv4. 2 means IPv6 // net.ParseIP for validate IP - if ip = net.ParseIP(remoteIP); ip == nil { + if ipAddress = net.ParseIP(remoteIP); ipAddress == nil { return nil, ErrRemoteAddrFormat } default: @@ -195,5 +200,10 @@ func parseEPRTAddr(param string) (addr *net.TCPAddr, err error) { return nil, ErrRemoteAddrFormat } - return net.ResolveTCPAddr("tcp", net.JoinHostPort(ip.String(), strconv.Itoa(portI))) + addr, err = net.ResolveTCPAddr("tcp", net.JoinHostPort(ipAddress.String(), strconv.Itoa(portI))) + if err != nil { + err = newNetworkError(fmt.Sprintf("could not resolve addr %v:%v", ipAddress, portI), err) + } + + return addr, err } diff --git a/transfer_active_test.go b/transfer_active_test.go index 19bff436..6f60164b 100644 --- a/transfer_active_test.go +++ b/transfer_active_test.go @@ -11,6 +11,8 @@ import ( ) func testRegexMatch(t *testing.T, regexp *regexp.Regexp, strings []string, expectedMatch bool) { + t.Helper() + for _, s := range strings { if regexp.MatchString(s) != expectedMatch { t.Errorf("Invalid match result: %s", s) @@ -44,15 +46,15 @@ func TestActiveTransferFromPort20(t *testing.T) { Password: authPass, ActiveTransfers: true, } - c, err := goftp.DialConfig(conf, server.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - _, err = c.ReadDir("/") + _, err = client.ReadDir("/") require.NoError(t, err) // the second ReadDir fails if we don't se the SO_REUSEPORT/SO_REUSEADDR socket options - _, err = c.ReadDir("/") + _, err = client.ReadDir("/") require.NoError(t, err) } diff --git a/transfer_pasv.go b/transfer_pasv.go index 76253103..bd0c0324 100644 --- a/transfer_pasv.go +++ b/transfer_pasv.go @@ -21,7 +21,7 @@ type transferHandler interface { Close() error // Set info about the transfer to return in STAT response - SetInfo(string) + SetInfo(info string) // Info about the transfer to return in STAT response GetInfo() string } @@ -49,28 +49,28 @@ func (e *ipValidationError) Error() string { func (c *clientHandler) getCurrentIP() ([]string, error) { // Provide our external IP address so the ftp client can connect back to us - ip := c.server.settings.PublicHost + ipParts := c.server.settings.PublicHost // If we don't have an IP address, we can take the one that was used for the current connection - if ip == "" { + if ipParts == "" { // Defer to the user-provided resolver. if c.server.settings.PublicIPResolver != nil { var err error - ip, err = c.server.settings.PublicIPResolver(c) + ipParts, err = c.server.settings.PublicIPResolver(c) if err != nil { return nil, fmt.Errorf("couldn't fetch public IP: %w", err) } } else { - ip = strings.Split(c.conn.LocalAddr().String(), ":")[0] + ipParts = strings.Split(c.conn.LocalAddr().String(), ":")[0] } } - quads := strings.Split(ip, ".") + quads := strings.Split(ipParts, ".") if len(quads) != 4 { - c.logger.Warn("Invalid passive IP", "IP", ip) + c.logger.Warn("Invalid passive IP", "IP", ipParts) - return nil, &ipValidationError{error: fmt.Sprintf("invalid passive IP %#v", ip)} + return nil, &ipValidationError{error: fmt.Sprintf("invalid passive IP %#v", ipParts)} } return quads, nil @@ -79,14 +79,19 @@ func (c *clientHandler) getCurrentIP() ([]string, error) { // ErrNoAvailableListeningPort is returned when no port could be found to accept incoming connection var ErrNoAvailableListeningPort = errors.New("could not find any port to listen to") +const ( + portSearchMinAttempts = 10 + portSearchMaxAttempts = 1000 +) + func (c *clientHandler) findListenerWithinPortRange(portRange *PortRange) (*net.TCPListener, error) { nbAttempts := portRange.End - portRange.Start // Making sure we trying a reasonable amount of ports before giving up - if nbAttempts < 10 { - nbAttempts = 10 - } else if nbAttempts > 1000 { - nbAttempts = 1000 + if nbAttempts < portSearchMinAttempts { + nbAttempts = portSearchMinAttempts + } else if nbAttempts > portSearchMaxAttempts { + nbAttempts = portSearchMaxAttempts } for i := 0; i < nbAttempts; i++ { @@ -97,7 +102,7 @@ func (c *clientHandler) findListenerWithinPortRange(portRange *PortRange) (*net. if errResolve != nil { c.logger.Error("Problem resolving local port", "err", errResolve, "port", port) - return nil, fmt.Errorf("could not resolve port %d: %w", port, errResolve) + return nil, newNetworkError(fmt.Sprintf("could not resolve port %d", port), errResolve) } tcpListener, errListen := net.ListenTCP("tcp", laddr) @@ -159,7 +164,7 @@ func (c *clientHandler) handlePASV(_ string) error { } } - p := &passiveTransferHandler{ + transferHandler := &passiveTransferHandler{ //nolint:forcetypeassert tcpListener: tcpListener, listener: listener, Port: tcpListener.Addr().(*net.TCPAddr).Port, @@ -170,21 +175,11 @@ func (c *clientHandler) handlePASV(_ string) error { // We should rewrite this part if command == "PASV" { - p1 := p.Port / 256 - p2 := p.Port - (p1 * 256) - quads, err2 := c.getCurrentIP() - - if err2 != nil { - c.writeMessage(StatusServiceNotAvailable, fmt.Sprintf("Could not listen for passive connection: %v", err2)) - + if c.handlePassivePASV(transferHandler) { return nil } - - c.writeMessage( - StatusEnteringPASV, - fmt.Sprintf("Entering Passive Mode (%s,%s,%s,%s,%d,%d)", quads[0], quads[1], quads[2], quads[3], p1, p2)) } else { - c.writeMessage(StatusEnteringEPSV, fmt.Sprintf("Entering Extended Passive Mode (|||%d|)", p.Port)) + c.writeMessage(StatusEnteringEPSV, fmt.Sprintf("Entering Extended Passive Mode (|||%d|)", transferHandler.Port)) } c.transferMu.Lock() @@ -192,13 +187,36 @@ func (c *clientHandler) handlePASV(_ string) error { c.transfer.Close() //nolint:errcheck,gosec } - c.transfer = p + c.transfer = transferHandler c.transferMu.Unlock() c.setLastDataChannel(DataChannelPassive) return nil } +func (c *clientHandler) handlePassivePASV(transferHandler *passiveTransferHandler) bool { + portByte1 := transferHandler.Port / 256 + portByte2 := transferHandler.Port - (portByte1 * 256) + quads, err2 := c.getCurrentIP() + + if err2 != nil { + c.writeMessage(StatusServiceNotAvailable, fmt.Sprintf("Could not listen for passive connection: %v", err2)) + + return true + } + + c.writeMessage( + StatusEnteringPASV, + fmt.Sprintf( + "Entering Passive Mode (%s,%s,%s,%s,%d,%d)", + quads[0], quads[1], quads[2], quads[3], + portByte1, portByte2, + ), + ) + + return false +} + func (p *passiveTransferHandler) ConnectionWait(wait time.Duration) (net.Conn, error) { if p.connection == nil { var err error @@ -207,19 +225,18 @@ func (p *passiveTransferHandler) ConnectionWait(wait time.Duration) (net.Conn, e } p.connection, err = p.listener.Accept() - if err != nil { - return nil, err + return nil, fmt.Errorf("failed to accept passive transfer connection: %w", err) } - ip, err := getIPFromRemoteAddr(p.connection.RemoteAddr()) + ipAddress, err := getIPFromRemoteAddr(p.connection.RemoteAddr()) if err != nil { p.logger.Warn("Could get remote passive IP address", "err", err) return nil, err } - if err := p.checkDataConn(ip, DataChannelPassive); err != nil { + if err := p.checkDataConn(ipAddress, DataChannelPassive); err != nil { // we don't want to expose the full error to the client, we just log it p.logger.Warn("Could not validate passive data connection requirement", "err", err) @@ -248,19 +265,14 @@ func (p *passiveTransferHandler) Open() (net.Conn, error) { func (p *passiveTransferHandler) Close() error { if p.tcpListener != nil { if err := p.tcpListener.Close(); err != nil { - p.logger.Warn( - "Problem closing passive listener", - "err", err, - ) + p.logger.Warn("Problem closing passive listener", "err", err) } } if p.connection != nil { if err := p.connection.Close(); err != nil { p.logger.Warn( - "Problem closing passive connection", - "err", err, - ) + "Problem closing passive connection", "err", err) } } diff --git a/transfer_test.go b/transfer_test.go index 80345c5c..20aee28f 100644 --- a/transfer_test.go +++ b/transfer_test.go @@ -32,6 +32,8 @@ func getABORCmd() string { } func createTemporaryFile(t *testing.T, targetSize int) *os.File { + t.Helper() + var file *os.File var fileErr error @@ -55,6 +57,8 @@ func createTemporaryFile(t *testing.T, targetSize int) *os.File { } func hashFile(t *testing.T, file *os.File) string { + t.Helper() + _, err := file.Seek(0, 0) require.NoError(t, err, "Couldn't seek") @@ -71,6 +75,8 @@ func hashFile(t *testing.T, file *os.File) string { } func ftpUpload(t *testing.T, ftp *goftp.Client, file io.ReadSeeker, filename string) { + t.Helper() + _, err := file.Seek(0, 0) require.NoError(t, err, "Couldn't seek") @@ -99,6 +105,8 @@ func ftpUpload(t *testing.T, ftp *goftp.Client, file io.ReadSeeker, filename str } func ftpDownloadAndHash(t *testing.T, ftp *goftp.Client, filename string) string { + t.Helper() + hasher := sha256.New() err := ftp.Retrieve(filename, hasher) require.NoError(t, err, "Couldn't fetch file") @@ -107,59 +115,67 @@ func ftpDownloadAndHash(t *testing.T, ftp *goftp.Client, filename string) string } func ftpDownloadAndHashWithRawConnection(t *testing.T, raw goftp.RawConn, fileName string) string { + t.Helper() + + req := require.New(t) hasher := sha256.New() dcGetter, err := raw.PrepareDataConn() - assert.NoError(t, err) + req.NoError(err) - rc, response, err := raw.SendCommand(fmt.Sprintf("RETR %v", fileName)) + returnCode, response, err := raw.SendCommand(fmt.Sprintf("RETR %v", fileName)) require.NoError(t, err) - require.Equal(t, StatusFileStatusOK, rc, response) + req.Equal(StatusFileStatusOK, returnCode, response) - dc, err := dcGetter() - assert.NoError(t, err) + dataConn, err := dcGetter() + req.NoError(err) - _, err = io.Copy(hasher, dc) - assert.NoError(t, err) + _, err = io.Copy(hasher, dataConn) + req.NoError(err) - err = dc.Close() - assert.NoError(t, err) + err = dataConn.Close() + req.NoError(err) - rc, response, err = raw.ReadResponse() - assert.NoError(t, err) - assert.Equal(t, StatusClosingDataConn, rc, response) + returnCode, response, err = raw.ReadResponse() + req.NoError(err) + req.Equal(StatusClosingDataConn, returnCode, response) return hex.EncodeToString(hasher.Sum(nil)) } -func ftpUploadWithRawConnection(t *testing.T, raw goftp.RawConn, file io.Reader, fileName string, append bool) { +func ftpUploadWithRawConnection(t *testing.T, raw goftp.RawConn, file io.Reader, fileName string, appendFile bool) { + t.Helper() + + req := require.New(t) dcGetter, err := raw.PrepareDataConn() - assert.NoError(t, err) + req.NoError(err) cmd := "STOR" - if append { + if appendFile { cmd = "APPE" } - rc, response, err := raw.SendCommand(fmt.Sprintf("%v %v", cmd, fileName)) + returnCode, response, err := raw.SendCommand(fmt.Sprintf("%v %v", cmd, fileName)) require.NoError(t, err) - require.Equal(t, StatusFileStatusOK, rc, response) + require.Equal(t, StatusFileStatusOK, returnCode, response) - dc, err := dcGetter() - assert.NoError(t, err) + dataConn, err := dcGetter() + req.NoError(err) - _, err = io.Copy(dc, file) - assert.NoError(t, err) + _, err = io.Copy(dataConn, file) + req.NoError(err) - err = dc.Close() - assert.NoError(t, err) + err = dataConn.Close() + req.NoError(err) - rc, response, err = raw.ReadResponse() - assert.NoError(t, err) - assert.Equal(t, StatusClosingDataConn, rc, response) + returnCode, response, err = raw.ReadResponse() + req.NoError(err) + assert.Equal(t, StatusClosingDataConn, returnCode, response) } func ftpDelete(t *testing.T, ftp *goftp.Client, filename string) { + t.Helper() + err := ftp.Delete(filename) require.NoError(t, err, "Couldn't delete file "+filename) @@ -168,29 +184,48 @@ func ftpDelete(t *testing.T, ftp *goftp.Client, filename string) { } func TestTransferIPv6(t *testing.T) { - s := NewTestServerWithTestDriver( - t, - &TestServerDriver{ - Debug: false, - Settings: &Settings{ - ActiveTransferPortNon20: true, - ListenAddr: "[::1]:0", + t.Parallel() + + createServer := func() *FtpServer { + server := NewTestServerWithTestDriver( + t, + &TestServerDriver{ + Debug: false, + Settings: &Settings{ + ActiveTransferPortNon20: true, + ListenAddr: "[::1]:0", + }, }, - }, - ) + ) - if s == nil { - t.Skip("IPv6 is not supported here") + if server == nil { + t.Skip("IPv6 is not supported here") + } + + return server } - t.Run("active", func(t *testing.T) { testTransferOnConnection(t, s, true, false, false) }) - t.Run("passive", func(t *testing.T) { testTransferOnConnection(t, s, false, false, false) }) + t.Run("active", func(t *testing.T) { + t.Parallel() + + s := createServer() + testTransferOnConnection(t, s, true, false, false) + }) + t.Run("passive", func(t *testing.T) { + t.Parallel() + + s := createServer() + testTransferOnConnection(t, s, false, false, false) + }) } // TestTransfer validates the upload of file in both active and passive mode func TestTransfer(t *testing.T) { + t.Parallel() + t.Run("without-tls", func(t *testing.T) { - s := NewTestServerWithTestDriver( + t.Parallel() + server := NewTestServerWithTestDriver( t, &TestServerDriver{ Debug: false, @@ -200,11 +235,12 @@ func TestTransfer(t *testing.T) { }, ) - testTransferOnConnection(t, s, false, false, false) - testTransferOnConnection(t, s, true, false, false) + testTransferOnConnection(t, server, false, false, false) + testTransferOnConnection(t, server, true, false, false) }) t.Run("with-tls", func(t *testing.T) { - s := NewTestServerWithTestDriver( + t.Parallel() + server := NewTestServerWithTestDriver( t, &TestServerDriver{ Debug: false, @@ -215,25 +251,29 @@ func TestTransfer(t *testing.T) { }, ) - testTransferOnConnection(t, s, false, true, false) - testTransferOnConnection(t, s, true, true, false) + testTransferOnConnection(t, server, false, true, false) + testTransferOnConnection(t, server, true, true, false) }) t.Run("with-implicit-tls", func(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + t.Parallel() + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Debug: false, TLS: true, Settings: &Settings{ ActiveTransferPortNon20: true, TLSRequired: ImplicitEncryption, - }}) + }, + }) - testTransferOnConnection(t, s, false, true, true) - testTransferOnConnection(t, s, true, true, true) + testTransferOnConnection(t, server, false, true, true) + testTransferOnConnection(t, server, true, true, true) }) } func testTransferOnConnection(t *testing.T, server *FtpServer, active, enableTLS, implicitTLS bool) { + t.Helper() + conf := goftp.Config{ User: authUser, Password: authPass, @@ -251,21 +291,21 @@ func testTransferOnConnection(t *testing.T, server *FtpServer, active, enableTLS } } - c, err := goftp.DialConfig(conf, server.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() var hashUpload, hashDownload string { // We create a 10MB file and upload it file := createTemporaryFile(t, 10*1024*1024) hashUpload = hashFile(t, file) - ftpUpload(t, c, file, "file.bin") + ftpUpload(t, client, file, "file.bin") } { // We download the file we just uploaded - hashDownload = ftpDownloadAndHash(t, c, "file.bin") - ftpDelete(t, c, "file.bin") + hashDownload = ftpDownloadAndHash(t, client, "file.bin") + ftpDelete(t, client, "file.bin") } // We make sure the hashes of the two files match @@ -286,128 +326,142 @@ func TestActiveModeDisabled(t *testing.T) { Password: authPass, ActiveTransfers: true, } - c, err := goftp.DialConfig(conf, server.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() file := createTemporaryFile(t, 10*1024) - err = c.Store("file.bin", file) + err = client.Store("file.bin", file) require.Error(t, err, "active mode is disabled, upload must fail") require.True(t, strings.Contains(err.Error(), "421-PORT command is disabled")) } // TestFailedTransfer validates the handling of failed transfer caused by file access issues func TestFailedTransfer(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() // We create a 1KB file and upload it file := createTemporaryFile(t, 1*1024) - err = c.Store("/non/existing/path/file.bin", file) + err = client.Store("/non/existing/path/file.bin", file) require.Error(t, err, "This upload should have failed") - err = c.Store("file.bin", file) + err = client.Store("file.bin", file) require.NoError(t, err, "This upload should have succeeded") } func TestBogusTransferStart(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - rc, err := c.OpenRawConn() + returnCode, err := client.OpenRawConn() require.NoError(t, err) - defer func() { require.NoError(t, rc.Close()) }() + defer func() { require.NoError(t, returnCode.Close()) }() { // Completely bogus port declaration - status, resp, err := rc.SendCommand("PORT something") + status, resp, err := returnCode.SendCommand("PORT something") require.NoError(t, err) require.Equal(t, StatusSyntaxErrorParameters, status, resp) } { // Completely bogus port declaration - status, resp, err := rc.SendCommand("EPRT something") + status, resp, err := returnCode.SendCommand("EPRT something") require.NoError(t, err) require.Equal(t, StatusSyntaxErrorParameters, status, resp) } { // Bad port number: 0 - status, resp, err := rc.SendCommand("EPRT |2|::1|0|") + status, resp, err := returnCode.SendCommand("EPRT |2|::1|0|") require.NoError(t, err) require.Equal(t, StatusSyntaxErrorParameters, status, resp) } { // Bad IP - status, resp, err := rc.SendCommand("EPRT |1|253.254.255.256|2000|") + status, resp, err := returnCode.SendCommand("EPRT |1|253.254.255.256|2000|") require.NoError(t, err) require.Equal(t, StatusSyntaxErrorParameters, status, resp) } { // Bad protocol type: 3 - status, resp, err := rc.SendCommand("EPRT |3|::1|2000|") + status, resp, err := returnCode.SendCommand("EPRT |3|::1|2000|") require.NoError(t, err) require.Equal(t, StatusSyntaxErrorParameters, status, resp) } { // good request but unacceptable ip address - status, resp, err := rc.SendCommand("EPRT |1|::1|2000|") + status, resp, err := returnCode.SendCommand("EPRT |1|::1|2000|") require.NoError(t, err) require.Equal(t, StatusSyntaxErrorParameters, status, resp) require.Contains(t, resp, "Your request does not meet the configured security requirements") } - s.settings.ActiveConnectionsCheck = IPMatchDisabled + server.settings.ActiveConnectionsCheck = IPMatchDisabled { // We end-up on a positive note - status, resp, err := rc.SendCommand("EPRT |1|::1|2000|") + status, resp, err := returnCode.SendCommand("EPRT |1|::1|2000|") require.NoError(t, err) require.Equal(t, StatusOK, status, resp) } } func TestFailingFileTransfer(t *testing.T) { - driver := &TestServerDriver{ - Debug: false, - } - s := NewTestServerWithTestDriver(t, driver) - conf := goftp.Config{ - User: authUser, - Password: authPass, - } + t.Parallel() - file := createTemporaryFile(t, 1*1024) + createClientOnServer := func(t *testing.T) (*goftp.Client, *os.File) { + t.Helper() - c, err := goftp.DialConfig(conf, s.Addr()) - require.NoError(t, err) + driver := &TestServerDriver{ + Debug: false, + } - defer func() { require.NoError(t, c.Close()) }() + server := NewTestServerWithTestDriver(t, driver) + conf := goftp.Config{ + User: authUser, + Password: authPass, + } + + file := createTemporaryFile(t, 1*1024) + + client, err := goftp.DialConfig(conf, server.Addr()) + require.NoError(t, err) + t.Cleanup(func() { panicOnError(client.Close()) }) + + return client, file + } t.Run("on write", func(t *testing.T) { - err = c.Store("fail-to-write.bin", file) + t.Parallel() + c, file := createClientOnServer(t) + err := c.Store("fail-to-write.bin", file) require.Error(t, err) require.True(t, strings.Contains(err.Error(), errFailWrite.Error()), err) }) t.Run("on close", func(t *testing.T) { - err = c.Store("fail-to-close.bin", file) + t.Parallel() + c, file := createClientOnServer(t) + err := c.Store("fail-to-close.bin", file) require.Error(t, err) require.True(t, strings.Contains(err.Error(), errFailClose.Error()), err) }) t.Run("on seek", func(t *testing.T) { + t.Parallel() + client, _ := createClientOnServer(t) initialData := []byte("initial data") appendFile, err := os.CreateTemp("", "ftpserver") require.NoError(t, err) @@ -415,7 +469,7 @@ func TestFailingFileTransfer(t *testing.T) { _, err = appendFile.Write(initialData) require.NoError(t, err) - err = c.Store("fail-to-seek.bin", appendFile) + err = client.Store("fail-to-seek.bin", appendFile) require.NoError(t, err) err = appendFile.Close() @@ -430,7 +484,7 @@ func TestFailingFileTransfer(t *testing.T) { info, err := appendFile.Stat() require.NoError(t, err) require.Equal(t, int64(len(initialData)+len(data)), info.Size()) - _, err = c.TransferFromOffset("fail-to-seek.bin", nil, appendFile, int64(len(initialData))) + _, err = client.TransferFromOffset("fail-to-seek.bin", nil, appendFile, int64(len(initialData))) require.Error(t, err) require.True(t, strings.Contains(err.Error(), errFailSeek.Error()), err) err = appendFile.Close() @@ -438,6 +492,8 @@ func TestFailingFileTransfer(t *testing.T) { }) t.Run("check for sync", func(t *testing.T) { + t.Parallel() + c, file := createClientOnServer(t) require.NoError(t, c.Store("ok", file)) }) } @@ -446,18 +502,18 @@ func TestAPPEExistingFile(t *testing.T) { driver := &TestServerDriver{ Debug: false, } - s := NewTestServerWithTestDriver(t, driver) + server := NewTestServerWithTestDriver(t, driver) conf := goftp.Config{ User: authUser, Password: authPass, } file := createTemporaryFile(t, 1*1024) - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err) - defer func() { require.NoError(t, c.Close()) }() + defer func() { require.NoError(t, client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err) defer func() { require.NoError(t, raw.Close()) }() @@ -467,7 +523,7 @@ func TestAPPEExistingFile(t *testing.T) { fileName := filepath.Base(file.Name()) - err = c.Store(fileName, file) + err = client.Store(fileName, file) require.NoError(t, err) _, err = file.Seek(0, io.SeekEnd) @@ -482,12 +538,12 @@ func TestAPPEExistingFile(t *testing.T) { ftpUploadWithRawConnection(t, raw, file, fileName, true) - info, err := c.Stat(fileName) + info, err := client.Stat(fileName) require.NoError(t, err) require.Equal(t, int64(1024+len(data)), info.Size()) localHash := hashFile(t, file) - remoteHash := ftpDownloadAndHash(t, c, fileName) + remoteHash := ftpDownloadAndHash(t, client, fileName) require.Equal(t, localHash, remoteHash) } @@ -495,7 +551,7 @@ func TestAPPENewFile(t *testing.T) { driver := &TestServerDriver{ Debug: false, } - s := NewTestServerWithTestDriver(t, driver) + server := NewTestServerWithTestDriver(t, driver) conf := goftp.Config{ User: authUser, Password: authPass, @@ -504,12 +560,12 @@ func TestAPPENewFile(t *testing.T) { _, err := file.Seek(0, io.SeekStart) require.NoError(t, err) - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err) - defer func() { require.NoError(t, c.Close()) }() + defer func() { require.NoError(t, client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err) defer func() { require.NoError(t, raw.Close()) }() @@ -519,7 +575,7 @@ func TestAPPENewFile(t *testing.T) { ftpUploadWithRawConnection(t, raw, file, fileName, true) localHash := hashFile(t, file) - remoteHash := ftpDownloadAndHash(t, c, fileName) + remoteHash := ftpDownloadAndHash(t, client, fileName) require.Equal(t, localHash, remoteHash) } @@ -527,21 +583,21 @@ func TestTransfersFromOffset(t *testing.T) { driver := &TestServerDriver{ Debug: false, } - s := NewTestServerWithTestDriver(t, driver) + server := NewTestServerWithTestDriver(t, driver) conf := goftp.Config{ User: authUser, Password: authPass, } file := createTemporaryFile(t, 1*1024) - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err) - defer func() { require.NoError(t, c.Close()) }() + defer func() { require.NoError(t, client.Close()) }() _, err = file.Seek(0, io.SeekStart) require.NoError(t, err) - err = c.Store("file", file) + err = client.Store("file", file) require.NoError(t, err) _, err = file.Seek(0, io.SeekEnd) @@ -554,79 +610,79 @@ func TestTransfersFromOffset(t *testing.T) { _, err = file.Seek(1024, io.SeekStart) require.NoError(t, err) - _, err = c.TransferFromOffset("file", nil, file, 1024) + _, err = client.TransferFromOffset("file", nil, file, 1024) require.NoError(t, err) - info, err := c.Stat("file") + info, err := client.Stat("file") require.NoError(t, err) require.Equal(t, int64(1024+len(data)), info.Size()) localHash := hashFile(t, file) - remoteHash := ftpDownloadAndHash(t, c, "file") + remoteHash := ftpDownloadAndHash(t, client, "file") require.Equal(t, localHash, remoteHash) // finally test a partial RETR buf := bytes.NewBuffer(nil) - _, err = c.TransferFromOffset("file", buf, nil, 1024) + _, err = client.TransferFromOffset("file", buf, nil, 1024) require.NoError(t, err) require.Equal(t, string(data), buf.String()) } func TestBasicABOR(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { require.NoError(t, c.Close()) }() + defer func() { require.NoError(t, client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err) defer func() { require.NoError(t, raw.Close()) }() - rc, _, err := raw.SendCommand("EPSV") + returnCode, _, err := raw.SendCommand("EPSV") require.NoError(t, err) - require.Equal(t, StatusEnteringEPSV, rc) + require.Equal(t, StatusEnteringEPSV, returnCode) - rc, _, err = raw.SendCommand(getABORCmd()) + returnCode, _, err = raw.SendCommand(getABORCmd()) require.NoError(t, err) - require.Equal(t, StatusClosingDataConn, rc) + require.Equal(t, StatusClosingDataConn, returnCode) // verify we are in sync - rc, _, err = raw.SendCommand("NOOP") + returnCode, _, err = raw.SendCommand("NOOP") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) _, err = raw.PrepareDataConn() require.NoError(t, err) - rc, _, err = raw.SendCommand("NLST") + returnCode, _, err = raw.SendCommand("NLST") require.NoError(t, err) - require.Equal(t, StatusFileStatusOK, rc) + require.Equal(t, StatusFileStatusOK, returnCode) - rc, _, err = raw.ReadResponse() + returnCode, _, err = raw.ReadResponse() require.NoError(t, err) - require.Equal(t, StatusClosingDataConn, rc) + require.Equal(t, StatusClosingDataConn, returnCode) // test ABOR cmd without special attention chars - rc, _, err = raw.SendCommand("ABOR") + returnCode, _, err = raw.SendCommand("ABOR") require.NoError(t, err) - require.Equal(t, StatusClosingDataConn, rc) + require.Equal(t, StatusClosingDataConn, returnCode) // verify we are in sync - rc, _, err = raw.SendCommand("NOOP") + returnCode, _, err = raw.SendCommand("NOOP") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) } func TestTransferABOR(t *testing.T) { t.Run("passive-mode", func(t *testing.T) { - s := NewTestServer(t, false) - s.settings.PassiveTransferPortRange = &PortRange{ + server := NewTestServer(t, false) + server.settings.PassiveTransferPortRange = &PortRange{ Start: 49152, End: 65535, } @@ -634,53 +690,53 @@ func TestTransferABOR(t *testing.T) { User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { require.NoError(t, c.Close()) }() + defer func() { require.NoError(t, client.Close()) }() - aborTransfer(t, c) + aborTransfer(t, client) }) t.Run("active-mode", func(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, ActiveTransfers: true, } - s.settings.ActiveTransferPortNon20 = true - c, err := goftp.DialConfig(conf, s.Addr()) + server.settings.ActiveTransferPortNon20 = true + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { require.NoError(t, c.Close()) }() + defer func() { require.NoError(t, client.Close()) }() - aborTransfer(t, c) + aborTransfer(t, client) }) } func TestABORWithoutOpenTransfer(t *testing.T) { - s := NewTestServer(t, true) + server := NewTestServer(t, true) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { require.NoError(t, c.Close()) }() + defer func() { require.NoError(t, client.Close()) }() file := createTemporaryFile(t, 1*1024) - err = c.Store("file.bin", file) + err = client.Store("file.bin", file) require.NoError(t, err) - err = c.Rename("file.bin", "delay-io-fail-to-seek.bin") + err = client.Rename("file.bin", "delay-io-fail-to-seek.bin") require.NoError(t, err) - _, err = c.Mkdir("delay-io-fail-to-readdir") + _, err = client.Mkdir("delay-io-fail-to-readdir") require.NoError(t, err) - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err) defer func() { require.NoError(t, raw.Close()) }() @@ -688,76 +744,80 @@ func TestABORWithoutOpenTransfer(t *testing.T) { _, err = file.Seek(1, io.SeekStart) require.NoError(t, err) - rc, response, err := raw.SendCommand("REST 1") + returnCode, response, err := raw.SendCommand("REST 1") require.NoError(t, err) - require.Equal(t, StatusFileActionPending, rc, response) + require.Equal(t, StatusFileActionPending, returnCode, response) - for _, cmd := range []string{"RETR delay-io-fail-to-seek.bin", "LIST delay-io-fail-to-readdir", - "NLST delay-io-fail-to-readdir", "MLSD delay-io-fail-to-readdir"} { + for _, cmd := range []string{ + "RETR delay-io-fail-to-seek.bin", "LIST delay-io-fail-to-readdir", + "NLST delay-io-fail-to-readdir", "MLSD delay-io-fail-to-readdir", + } { _, err = raw.PrepareDataConn() require.NoError(t, err) err = raw.SendCommandNoWaitResponse(cmd) require.NoError(t, err) - rc, response, err = raw.SendCommand(getABORCmd()) + returnCode, response, err = raw.SendCommand(getABORCmd()) require.NoError(t, err) - require.Equal(t, StatusClosingDataConn, rc, response) + require.Equal(t, StatusClosingDataConn, returnCode, response) require.Equal(t, "ABOR successful; closing transfer connection", response) // verify we are in sync - rc, _, err = raw.SendCommand("NOOP") + returnCode, _, err = raw.SendCommand("NOOP") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) } - rc, _, err = raw.SendCommand("QUIT") + returnCode, _, err = raw.SendCommand("QUIT") require.NoError(t, err) - require.Equal(t, StatusClosingControlConn, rc) + require.Equal(t, StatusClosingControlConn, returnCode) } func TestABORBeforeOpenTransfer(t *testing.T) { t.Run("passive-mode", func(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - s.settings.ActiveTransferPortNon20 = true - c, err := goftp.DialConfig(conf, s.Addr()) + server.settings.ActiveTransferPortNon20 = true + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { require.NoError(t, c.Close()) }() + defer func() { require.NoError(t, client.Close()) }() - aborBeforeOpenTransfer(t, c) + aborBeforeOpenTransfer(t, client) }) t.Run("active-mode", func(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, ActiveTransfers: true, } - s.settings.ActiveTransferPortNon20 = true - c, err := goftp.DialConfig(conf, s.Addr()) + server.settings.ActiveTransferPortNon20 = true + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { require.NoError(t, c.Close()) }() + defer func() { require.NoError(t, client.Close()) }() - aborBeforeOpenTransfer(t, c) + aborBeforeOpenTransfer(t, client) }) } -func aborTransfer(t *testing.T, c *goftp.Client) { +func aborTransfer(t *testing.T, client *goftp.Client) { + t.Helper() + file := createTemporaryFile(t, 1*1024) - err := c.Store("file.bin", file) + err := client.Store("file.bin", file) require.NoError(t, err) - err = c.Rename("file.bin", "delay-io.bin") + err = client.Rename("file.bin", "delay-io.bin") require.NoError(t, err) - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err) defer func() { require.NoError(t, raw.Close()) }() @@ -768,43 +828,45 @@ func aborTransfer(t *testing.T, c *goftp.Client) { _, err = raw.PrepareDataConn() require.NoError(t, err) - rc, response, err := raw.SendCommand("RETR delay-io.bin") + returnCode, response, err := raw.SendCommand("RETR delay-io.bin") require.NoError(t, err) - require.Equal(t, StatusFileStatusOK, rc, response) + require.Equal(t, StatusFileStatusOK, returnCode, response) require.Equal(t, "Using transfer connection", response) - rc, response, err = raw.SendCommand("STAT") + returnCode, response, err = raw.SendCommand("STAT") require.NoError(t, err) - require.Equal(t, StatusSystemStatus, rc, response) + require.Equal(t, StatusSystemStatus, returnCode, response) require.Contains(t, response, "RETR delay-io.bin") require.NotContains(t, response, "Using transfer connection") require.NotContains(t, response, "Closing transfer connection") - rc, response, err = raw.SendCommand(getABORCmd()) + returnCode, response, err = raw.SendCommand(getABORCmd()) require.NoError(t, err) - require.Equal(t, StatusTransferAborted, rc, response) + require.Equal(t, StatusTransferAborted, returnCode, response) require.Equal(t, "Connection closed; transfer aborted", response) - rc, response, err = raw.ReadResponse() + returnCode, response, err = raw.ReadResponse() require.NoError(t, err) - require.Equal(t, StatusClosingDataConn, rc, response) + require.Equal(t, StatusClosingDataConn, returnCode, response) require.Equal(t, "ABOR successful; closing transfer connection", response) // verify we are in sync - rc, _, err = raw.SendCommand("NOOP") + returnCode, _, err = raw.SendCommand("NOOP") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) } -func aborBeforeOpenTransfer(t *testing.T, c *goftp.Client) { +func aborBeforeOpenTransfer(t *testing.T, client *goftp.Client) { + t.Helper() + file := createTemporaryFile(t, 1*1024) - err := c.Store("file.bin", file) + err := client.Store("file.bin", file) require.NoError(t, err) - err = c.Rename("file.bin", "delay-io.bin") + err = client.Rename("file.bin", "delay-io.bin") require.NoError(t, err) - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err) defer func() { require.NoError(t, raw.Close()) }() @@ -812,9 +874,9 @@ func aborBeforeOpenTransfer(t *testing.T, c *goftp.Client) { _, err = file.Seek(1, io.SeekStart) require.NoError(t, err) - rc, response, err := raw.SendCommand("REST 1") + returnCode, response, err := raw.SendCommand("REST 1") require.NoError(t, err) - require.Equal(t, StatusFileActionPending, rc, response) + require.Equal(t, StatusFileActionPending, returnCode, response) _, err = raw.PrepareDataConn() require.NoError(t, err) @@ -822,15 +884,15 @@ func aborBeforeOpenTransfer(t *testing.T, c *goftp.Client) { err = raw.SendCommandNoWaitResponse("RETR delay-io.bin") require.NoError(t, err) - rc, response, err = raw.SendCommand(getABORCmd()) + returnCode, response, err = raw.SendCommand(getABORCmd()) require.NoError(t, err) - require.Equal(t, StatusClosingDataConn, rc, response) + require.Equal(t, StatusClosingDataConn, returnCode, response) require.Equal(t, "ABOR successful; closing transfer connection", response) // verify we are in sync - rc, _, err = raw.SendCommand("NOOP") + returnCode, _, err = raw.SendCommand("NOOP") require.NoError(t, err) - require.Equal(t, StatusOK, rc) + require.Equal(t, StatusOK, returnCode) } func TestASCIITransfers(t *testing.T) { @@ -839,12 +901,12 @@ func TestASCIITransfers(t *testing.T) { User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, s.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { require.NoError(t, c.Close()) }() + defer func() { require.NoError(t, client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err) defer func() { require.NoError(t, raw.Close()) }() @@ -867,7 +929,7 @@ func TestASCIITransfers(t *testing.T) { ftpUploadWithRawConnection(t, raw, file, "file.txt", false) - files, err := c.ReadDir("/") + files, err := client.ReadDir("/") require.NoError(t, err) require.Len(t, files, 1) @@ -924,7 +986,7 @@ func TestASCIITransfersInvalidFiles(t *testing.T) { } func TestPASVWrappedListenerError(t *testing.T) { - s := NewTestServerWithTestDriver(t, &TestServerDriver{ + server := NewTestServerWithTestDriver(t, &TestServerDriver{ Debug: true, errPassiveListener: os.ErrClosed, }) @@ -932,50 +994,50 @@ func TestPASVWrappedListenerError(t *testing.T) { User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { require.NoError(t, c.Close()) }() + defer func() { require.NoError(t, client.Close()) }() - _, err = c.ReadDir("/") + _, err = client.ReadDir("/") if assert.Error(t, err) { assert.Contains(t, err.Error(), "421-Could not listen for passive connection") } } func TestPASVPublicIPResolver(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) conf := goftp.Config{ User: authUser, Password: authPass, } - c, err := goftp.DialConfig(conf, s.Addr()) + client, err := goftp.DialConfig(conf, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { require.NoError(t, c.Close()) }() + defer func() { require.NoError(t, client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") - s.settings.PublicHost = "" - s.settings.PublicIPResolver = func(_ ClientContext) (string, error) { + server.settings.PublicHost = "" + server.settings.PublicIPResolver = func(_ ClientContext) (string, error) { return "127.0.0", nil } // we crash if the PublicIPResolver returns an invalid IP, this must be fixed outside the lib - rc, resp, err := raw.SendCommand("PASV") + returnCode, resp, err := raw.SendCommand("PASV") require.NoError(t, err) - require.Equal(t, StatusServiceNotAvailable, rc) + require.Equal(t, StatusServiceNotAvailable, returnCode) require.Contains(t, resp, "invalid passive IP") - s.settings.PublicIPResolver = func(_ ClientContext) (string, error) { + server.settings.PublicIPResolver = func(_ ClientContext) (string, error) { return "", errConnectionNotAllowed } - rc, resp, err = raw.SendCommand("PASV") + returnCode, resp, err = raw.SendCommand("PASV") require.NoError(t, err) - require.Equal(t, StatusServiceNotAvailable, rc) + require.Equal(t, StatusServiceNotAvailable, returnCode) require.Contains(t, resp, "couldn't fetch public IP") } @@ -986,7 +1048,7 @@ func TestPASVConnectionWait(t *testing.T) { tcpListener, err := net.ListenTCP("tcp", addr) require.NoError(t, err) - c := clientHandler{ + cltHandler := clientHandler{ conn: &testNetConn{ remoteAddr: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 21}, }, @@ -998,7 +1060,7 @@ func TestPASVConnectionWait(t *testing.T) { }, } - p := passiveTransferHandler{ + transferHandler := passiveTransferHandler{ //nolint:forcetypeassert listener: &testNetListener{ conn: &testNetConn{ remoteAddr: &net.TCPAddr{IP: nil, Port: 21}, // invalid IP @@ -1006,37 +1068,37 @@ func TestPASVConnectionWait(t *testing.T) { }, tcpListener: tcpListener, Port: tcpListener.Addr().(*net.TCPAddr).Port, - settings: c.server.settings, + settings: cltHandler.server.settings, logger: lognoop.NewNoOpLogger(), - checkDataConn: c.checkDataConnectionRequirement, + checkDataConn: cltHandler.checkDataConnectionRequirement, } defer func() { - err = p.Close() + err = transferHandler.Close() assert.NoError(t, err) }() - _, err = p.ConnectionWait(1 * time.Second) + _, err = transferHandler.ConnectionWait(1 * time.Second) if assert.Error(t, err) { assert.Contains(t, err.Error(), "invalid remote IP") } - p.listener = &testNetListener{ + transferHandler.listener = &testNetListener{ conn: &testNetConn{ remoteAddr: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 21}, }, } - _, err = p.ConnectionWait(1 * time.Second) + _, err = transferHandler.ConnectionWait(1 * time.Second) assert.NoError(t, err) } // On Mac Os X, this requires to issue the following command: // sudo ifconfig lo0 alias 127.0.1.1 up func TestPASVIPMatch(t *testing.T) { - s := NewTestServer(t, false) + server := NewTestServer(t, false) - conn, err := net.DialTimeout("tcp", s.Addr(), 5*time.Second) + conn, err := net.DialTimeout("tcp", server.Addr(), 5*time.Second) require.NoError(t, err) defer func() { @@ -1054,15 +1116,15 @@ func TestPASVIPMatch(t *testing.T) { loginConnection(t, conn) for _, mode := range []DataConnectionRequirement{IPMatchRequired, IPMatchDisabled} { - s.settings.PasvConnectionsCheck = mode + server.settings.PasvConnectionsCheck = mode _, err = conn.Write([]byte("PASV\r\n")) require.NoError(t, err) - n, err := conn.Read(buf) + readBytes, err := conn.Read(buf) require.NoError(t, err) - resp := string(buf[:n]) + resp := string(buf[:readBytes]) port := getPortFromPASVResponse(t, resp) assert.NotEqual(t, 0, port) @@ -1071,14 +1133,14 @@ func TestPASVIPMatch(t *testing.T) { addr := net.JoinHostPort("127.0.0.1", strconv.Itoa(port)) // now dial from 127.0.1.1 instead of 127.0.0.1 - d := net.Dialer{ + dialer := net.Dialer{ LocalAddr: &net.TCPAddr{ IP: net.ParseIP("127.0.1.1"), Port: 0, }, Timeout: 5 * time.Second, } - dataConn, err := d.Dial("tcp", addr) + dataConn, err := dialer.Dial("tcp", addr) require.NoError(t, err) defer func() { @@ -1086,10 +1148,10 @@ func TestPASVIPMatch(t *testing.T) { assert.NoError(t, err) }() - n, err = conn.Read(buf) + readBytes, err = conn.Read(buf) require.NoError(t, err) - resp = string(buf[:n]) + resp = string(buf[:readBytes]) if mode == IPMatchRequired { require.Equal(t, "425 data connection security requirements not met", strings.TrimSpace(resp)) @@ -1100,21 +1162,21 @@ func TestPASVIPMatch(t *testing.T) { } func TestPassivePortExhaustion(t *testing.T) { - s := NewTestServer(t, false) - s.settings.PassiveTransferPortRange = &PortRange{ + server := NewTestServer(t, false) + server.settings.PassiveTransferPortRange = &PortRange{ Start: 40000, End: 40005, } - c, err := goftp.DialConfig(goftp.Config{ + client, err := goftp.DialConfig(goftp.Config{ User: authUser, Password: authPass, - }, s.Addr()) + }, server.Addr()) require.NoError(t, err, "Couldn't connect") - defer func() { panicOnError(c.Close()) }() + defer func() { panicOnError(client.Close()) }() - raw, err := c.OpenRawConn() + raw, err := client.OpenRawConn() require.NoError(t, err, "Couldn't open raw connection") defer func() { require.NoError(t, raw.Close()) }() @@ -1127,27 +1189,31 @@ func TestPassivePortExhaustion(t *testing.T) { } func loginConnection(t *testing.T, conn net.Conn) { + t.Helper() + buf := make([]byte, 1024) _, err := fmt.Fprintf(conn, "USER %v\r\n", authUser) require.NoError(t, err) - n, err := conn.Read(buf) + readBytes, err := conn.Read(buf) require.NoError(t, err) - resp := string(buf[:n]) + resp := string(buf[:readBytes]) require.True(t, strings.HasPrefix(resp, "331")) _, err = fmt.Fprintf(conn, "PASS %v\r\n", authPass) require.NoError(t, err) - n, err = conn.Read(buf) + readBytes, err = conn.Read(buf) require.NoError(t, err) - resp = string(buf[:n]) + resp = string(buf[:readBytes]) require.True(t, strings.HasPrefix(resp, "230")) } func getPortFromPASVResponse(t *testing.T, resp string) int { + t.Helper() + port := 0 resp = strings.Replace(resp, "227 Entering Passive Mode", "", 1) resp = strings.Replace(resp, "(", "", 1)