Fix: POP3 end of file reached error (#315)

* Changed POP3 size output to show compatible size

* Setting POP3 10 minutes timeout according to RFC1939

* fixed issue with unauthorized commands access, refactor

* readded package description

* fixes error strings should not be capitalized (ST1005)go-staticcheck

server/pop3/functions.go

@@ -72,5 +72,5 @@ func getSafeArg(args []string, nr int) (string, error) {
 	if nr < len(args) {
 		return args[nr], nil
 	}
-	return "", errors.New("Out of range")
+	return "", errors.New("-ERR out of range")
 }

server/pop3/pop3.go

@@ -9,6 +9,7 @@ import (
 	"bufio"
 	"crypto/tls"
 	"fmt"
+	"io"
 	"net"
 	"strconv"
 	"strings"
@@ -22,15 +23,12 @@ import (
 )
 
 const (
-	// UNAUTHORIZED state
+	AUTHORIZATION = 1
-	UNAUTHORIZED = 1
-	// TRANSACTION state
 	TRANSACTION   = 2
-	// UPDATE state
 	UPDATE        = 3
 )
 
-// Run will start the pop3 server if enabled
+// Run will start the POP3 server if enabled
 func Run() {
 	if auth.POP3Credentials == nil || config.POP3Listen == "" {
 		// POP3 server is disabled without authentication
@@ -68,6 +66,7 @@ func Run() {
 	for {
 		conn, err := listener.Accept()
 		if err != nil {
+			logger.Log().Errorf("[pop3] accept error: %s", err.Error())
 			continue
 		}
 
@@ -84,8 +83,9 @@ type message struct {
 func handleClient(conn net.Conn) {
 	var (
 		user     = ""
-		state    = 1
+		state    = AUTHORIZATION // Start with AUTHORIZATION state
-		toDelete = []string{}
+		toDelete []string        // Track messages marked for deletion
+		messages []message
 	)
 
 	defer func() {
@@ -94,7 +94,7 @@ func handleClient(conn net.Conn) {
 				_ = storage.DeleteMessages([]string{id})
 			}
 			if len(toDelete) > 0 {
-				// update web UI to remove deleted messages
+				// Update web UI to remove deleted messages
 				websockets.Broadcast("prune", nil)
 			}
 		}
@@ -106,23 +106,17 @@ func handleClient(conn net.Conn) {
 
 	reader := bufio.NewReader(conn)
 
-	messages := []message{}
-
-	// State
-	// 1 = Unauthorized
-	// 2 = Transaction mode
-	// 3 = update mode
-
 	logger.Log().Debugf("[pop3] connection opened by %s", conn.RemoteAddr().String())
 
 	// First welcome the new connection
 	sendResponse(conn, "+OK Mailpit POP3 server")
 
+	// Set 10 minutes timeout according to RFC1939
 	timeoutDuration := 600 * time.Second
 
 	for {
-		// POP3 server enforced a timeout of 30 seconds
+		// Set read deadline
-		if err := conn.SetDeadline(time.Now().Add(timeoutDuration)); err != nil {
+		if err := conn.SetReadDeadline(time.Now().Add(timeoutDuration)); err != nil {
 			logger.Log().Errorf("[pop3] %s", err.Error())
 			return
 		}
@@ -130,7 +124,11 @@ func handleClient(conn net.Conn) {
 		// Reads a line from the client
 		rawLine, err := reader.ReadString('\n')
 		if err != nil {
-			logger.Log().Errorf("[pop3] %s", err.Error())
+			if err == io.EOF {
+				logger.Log().Debugf("[pop3] client disconnected: %s", conn.RemoteAddr().String())
+			} else {
+				logger.Log().Errorf("[pop3] read error: %s", err.Error())
+			}
 			return
 		}
 
@@ -139,7 +137,8 @@ func handleClient(conn net.Conn) {
 
 		logger.Log().Debugf("[pop3] received: %s (%s)", strings.TrimSpace(rawLine), conn.RemoteAddr().String())
 
-		if cmd == "CAPA" {
+		switch cmd {
+		case "CAPA":
 			// List our capabilities per RFC2449
 			sendResponse(conn, "+OK Capability list follows")
 			sendResponse(conn, "TOP")
@@ -147,17 +146,19 @@ func handleClient(conn net.Conn) {
 			sendResponse(conn, "UIDL")
 			sendResponse(conn, "IMPLEMENTATION Mailpit")
 			sendResponse(conn, ".")
-			continue
+		case "USER":
-		} else if cmd == "USER" && state == UNAUTHORIZED {
+			if state == AUTHORIZATION {
 				if len(args) != 1 {
 					sendResponse(conn, "-ERR must supply a user")
 					return
 				}
-			// always true - stash for PASS
 				sendResponse(conn, "+OK")
 				user = args[0]
-
+			} else {
-		} else if cmd == "PASS" && state == UNAUTHORIZED {
+				sendResponse(conn, "-ERR user already specified")
+			}
+		case "PASS":
+			if state == AUTHORIZATION {
 				if len(args) != 1 {
 					sendResponse(conn, "-ERR must supply a password")
 					return
@@ -166,66 +167,68 @@ func handleClient(conn net.Conn) {
 				pass := args[0]
 				if authUser(user, pass) {
 					sendResponse(conn, "+OK signed in")
+					var err error
 					messages, err = getMessages()
 					if err != nil {
 						logger.Log().Errorf("[pop3] %s", err.Error())
 					}
-				state = 2
+					state = TRANSACTION
 				} else {
 					sendResponse(conn, "-ERR invalid password")
 					logger.Log().Warnf("[pop3] failed login: %s", user)
 				}
-
+			} else {
-		} else if cmd == "STAT" && state == TRANSACTION {
+				sendResponse(conn, "-ERR user not specified")
-			totalSize := float64(0)
+			}
-			for _, m := range messages {
+		case "STAT", "LIST", "UIDL", "RETR", "TOP", "NOOP", "DELE":
-				totalSize = totalSize + m.Size
+			if state == TRANSACTION {
+				handleTransactionCommand(conn, cmd, args, messages, &toDelete)
+			} else {
+				sendResponse(conn, "-ERR user not authenticated")
+			}
+		case "QUIT":
+			sendResponse(conn, "+OK Goodbye")
+			state = UPDATE
+			return
+		default:
+			sendResponse(conn, "-ERR unknown command")
+		}
+	}
 }
 
+func handleTransactionCommand(conn net.Conn, cmd string, args []string, messages []message, toDelete *[]string) {
+	switch cmd {
+	case "STAT":
+		totalSize := float64(0)
+		for _, m := range messages {
+			totalSize += m.Size
+		}
 		sendResponse(conn, fmt.Sprintf("+OK %d %d", len(messages), int64(totalSize)))
-
+	case "LIST":
-		} else if cmd == "LIST" && state == TRANSACTION {
 		totalSize := float64(0)
 		for _, m := range messages {
-				totalSize = totalSize + m.Size
+			totalSize += m.Size
 		}
-			sendData(conn, fmt.Sprintf("+OK %d messages (%d octets)", len(messages), int64(totalSize)))
+		sendResponse(conn, fmt.Sprintf("+OK %d messages (%d octets)", len(messages), int64(totalSize)))
 
-			// print all sizes
 		for row, m := range messages {
-				sendData(conn, fmt.Sprintf("%d %d", row+1, int64(m.Size))) // Convert Size to int64 when printing
+			sendResponse(conn, fmt.Sprintf("%d %d", row+1, int64(m.Size))) // Convert Size to int64 when printing
 		}
-			// end
+		sendResponse(conn, ".")
-			sendData(conn, ".")
+	case "UIDL":
-
+		sendResponse(conn, "+OK unique-id listing follows")
-		} else if cmd == "UIDL" && state == TRANSACTION {
-			totalSize := float64(0)
-			for _, m := range messages {
-				totalSize = totalSize + m.Size
-			}
-
-			sendData(conn, "+OK unique-id listing follows")
-
-			// print all message IDS
 		for row, m := range messages {
-				sendData(conn, fmt.Sprintf("%d %s", row+1, m.ID))
+			sendResponse(conn, fmt.Sprintf("%d %s", row+1, m.ID))
 		}
-			// end
+		sendResponse(conn, ".")
-			sendData(conn, ".")
+	case "RETR":
-
-		} else if cmd == "RETR" && state == TRANSACTION {
 		if len(args) != 1 {
 			sendResponse(conn, "-ERR no such message")
 			return
 		}
 
 		nr, err := strconv.Atoi(args[0])
-			if err != nil {
+		if err != nil || nr < 1 || nr > len(messages) {
-				sendResponse(conn, "-ERR no such message")
-				return
-			}
-
-			if nr < 1 || nr > len(messages) {
 			sendResponse(conn, "-ERR no such message")
 			return
 		}
@@ -236,9 +239,8 @@ func handleClient(conn net.Conn) {
 			sendResponse(conn, "-ERR no such message")
 			return
 		}
-
 		size := len(raw)
-			sendData(conn, fmt.Sprintf("+OK %d octets", size))
+		sendResponse(conn, fmt.Sprintf("+OK %d octets", size))
 
 		// When all lines of the response have been sent, a
 		// final line is sent, consisting of a termination octet (decimal code
@@ -247,24 +249,19 @@ func handleClient(conn net.Conn) {
 		// pre-pending the termination octet to that line of the response.
 		// @see: https://www.ietf.org/rfc/rfc1939.txt
 		sendData(conn, strings.Replace(string(raw), "\n.", "\n..", -1))
-			sendData(conn, ".")
+		sendResponse(conn, ".")
-
+	case "TOP":
-		} else if cmd == "TOP" && state == TRANSACTION {
 		arg, err := getSafeArg(args, 0)
 		if err != nil {
 			sendResponse(conn, "-ERR TOP requires two arguments")
 			return
 		}
 		nr, err := strconv.Atoi(arg)
-			if err != nil {
+		if err != nil || nr < 1 || nr > len(messages) {
-				sendResponse(conn, "-ERR TOP requires two arguments")
-				return
-			}
-
-			if nr < 1 || nr > len(messages) {
 			sendResponse(conn, "-ERR no such message")
 			return
 		}
+
 		arg2, err := getSafeArg(args, 1)
 		if err != nil {
 			sendResponse(conn, "-ERR TOP requires two arguments")
@@ -284,41 +281,24 @@ func handleClient(conn net.Conn) {
 			return
 		}
 
-			sendData(conn, "+OK Top of message follows")
+		sendResponse(conn, "+OK Top of message follows")
 		sendData(conn, headers+"\r\n")
 		sendData(conn, body)
-			sendData(conn, ".")
+		sendResponse(conn, ".")
-
+	case "NOOP":
-		} else if cmd == "NOOP" && state == TRANSACTION {
+		sendResponse(conn, "+OK")
-			sendData(conn, "+OK")
+	case "DELE":
-		} else if cmd == "DELE" && state == TRANSACTION {
 		arg, _ := getSafeArg(args, 0)
 		nr, err := strconv.Atoi(arg)
-			if err != nil {
+		if err != nil || nr < 1 || nr > len(messages) {
-				logger.Log().Warnf("[pop3] -ERR invalid DELETE integer: %s", arg)
-				sendResponse(conn, "-ERR invalid integer")
-				return
-			}
-
-			if nr < 1 || nr > len(messages) {
-				logger.Log().Warnf("[pop3] -ERR no such message")
 			sendResponse(conn, "-ERR no such message")
 			return
 		}
-			toDelete = append(toDelete, messages[nr-1].ID)
 
-			sendResponse(conn, "+OK")
+		m := messages[nr-1]
-
+		*toDelete = append(*toDelete, m.ID)
-		} else if cmd == "RSET" && state == TRANSACTION {
+		sendResponse(conn, "+OK message marked for deletion")
-			toDelete = []string{}
+	default:
-			sendData(conn, "+OK")
+		sendResponse(conn, "-ERR unknown command")
-
-		} else if cmd == "QUIT" {
-			state = UPDATE
-			return
-		} else {
-			logger.Log().Warnf("[pop3] -ERR %s not implemented", cmd)
-			sendResponse(conn, fmt.Sprintf("-ERR %s not implemented", cmd))
-		}
 	}
 }