fix(pop3): reject deleted messages and failed TLS handshakes (#620)

- RETR and TOP now respond "-ERR You deleted message N" for messages
  marked deleted, as RFC 1939 sections 5 (RETR) and 7 (TOP) require;
  previously the message content was still sent.
- STLS: a failed TLS handshake previously continued the session with
  the dead TLS connection installed as s.conn and a bogus tlsState,
  producing confusing follow-on errors. RFC 2595 section 4 requires
  the negotiation to complete before further commands, so a failure
  is now fatal: log it and terminate the session.

Updates the scripted tests that previously pinned the old behavior,
and adds a STLS handshake failure regression test.

Signed-off-by: James Hillyerd <james@hillyerd.com>

pkg/server/pop3/handler.go

@@ -235,8 +235,11 @@ func (s *Session) authorizationHandler(ctx context.Context, cmd string, args []s
 		s.send("+OK Begin TLS Negotiation")
 		tlsConn := tls.Server(s.conn, s.tlsConfig)
 		if err := tlsConn.HandshakeContext(tlsCtx); err != nil {
-			s.logger.Error().Msgf("-ERR TLS handshake failed %v", err)
+			// RFC 2595 §4: the negotiation must complete before any further
-			s.ooSeq(cmd)
+			// commands are sent, so a failure is fatal to the session.
+			s.logger.Error().Msgf("TLS handshake failed: %v", err)
+			s.enterState(QUIT)
+			return
 		}
 		s.conn = tlsConn
 		s.reader = bufio.NewReader(tlsConn)
@@ -328,6 +331,11 @@ func (s *Session) transactionHandler(cmd string, args []string) {
 		if !ok {
 			return
 		}
+		if !s.retain[msgNum-1] {
+			s.logger.Warn().Msgf("Client tried to RETR a message it had deleted")
+			s.send(fmt.Sprintf("-ERR You deleted message %v", msgNum))
+			return
+		}
 		s.send(fmt.Sprintf("+OK %v bytes follows", s.messages[msgNum-1].Size()))
 		s.sendMessageLines(s.messages[msgNum-1], -1)
 	case "TOP":
@@ -353,6 +361,11 @@ func (s *Session) transactionHandler(cmd string, args []string) {
 			s.send("-ERR TOP second argument must be non-negative")
 			return
 		}
+		if !s.retain[msgNum-1] {
+			s.logger.Warn().Msgf("Client tried to TOP a message it had deleted")
+			s.send(fmt.Sprintf("-ERR You deleted message %v", msgNum))
+			return
+		}
 		s.send("+OK Top of message follows")
 		s.sendMessageLines(s.messages[msgNum-1], int(lines))
 	case "QUIT":

pkg/server/pop3/handler_test.go

@@ -288,6 +288,51 @@ func TestForceTLS(t *testing.T) {
 	}
 }
 
+// RFC 2595 §4: after the +OK response to STLS, a TLS negotiation begins
+// immediately and the client must not issue further commands until it
+// completes. A failed negotiation is therefore fatal to the session: the
+// server must not keep the session running on the dead connection.
+func TestSTLSHandshakeFailure(t *testing.T) {
+	ds := test.NewStore()
+	server := setupPOPServer(t, ds, true, false)
+	pipe := setupPOPSession(t, server)
+	c := textproto.NewConn(pipe)
+	defer func() {
+		_ = c.Close()
+		server.Drain()
+	}()
+
+	reply, err := c.ReadLine()
+	if err != nil {
+		t.Fatalf("Reading initial line failed %v", err)
+	}
+	if !strings.HasPrefix(reply, "+OK") {
+		t.Fatalf("Initial line is not +OK")
+	}
+
+	if err := c.PrintfLine("STLS"); err != nil {
+		t.Fatalf("Failed to send STLS; %v.", err)
+	}
+	reply, err = c.ReadLine()
+	if err != nil {
+		t.Fatalf("Reading STLS reply line failed %v", err)
+	}
+	if !strings.HasPrefix(reply, "+OK") {
+		t.Fatalf("STLS failed: %s", reply)
+	}
+
+	// Send bytes that cannot begin a TLS handshake.
+	if _, err := pipe.Write([]byte("this is not a TLS handshake\r\n")); err != nil {
+		t.Fatalf("Failed to write garbage; %v.", err)
+	}
+
+	// The server must terminate the session without further responses.
+	_, err = c.ReadLine()
+	if err == nil {
+		t.Fatal("Expected connection close after failed handshake")
+	}
+}
+
 // net.Pipe does not implement deadlines
 type mockConn struct {
 	net.Conn

pkg/server/pop3/transaction_test.go

@@ -452,19 +452,20 @@ func TestTransactionRetrArgumentErrors(t *testing.T) {
 	}
 }
 
-// Documents current behavior: RFC 1939 requires a negative status indicator
+// RFC 1939 §5: a RETR message-number may NOT refer to a message marked as
-// when RETR refers to a message marked as deleted, but the handler does not
+// deleted; the server responds with a negative status indicator.
-// check the retain flag. Flagged for a follow-up fix.
+func TestTransactionRetrDeletedMessageRejected(t *testing.T) {
-func TestTransactionRetrDeletedMessage(t *testing.T) {
 	ds := newMemStore(t)
-	test.DeliverToStore(t, ds, "mailbox", "One", time.Now())
+	_, size1 := test.DeliverToStore(t, ds, "mailbox", "One", time.Now())
-	_, size2 := test.DeliverToStore(t, ds, "mailbox", "Two", time.Now())
+	test.DeliverToStore(t, ds, "mailbox", "Two", time.Now())
 	server := setupPOPServer(t, ds, false, false)
 
 	script := slices.Concat(loginSteps(2), []scriptStep{
 		{"DELE 2", []string{"+OK Deleted message 2"}},
-		{"RETR 2", []string{fmt.Sprintf("+OK %v bytes follows", size2),
+		{"RETR 2", []string{"-ERR You deleted message 2"}},
-			"To: somebody@host", "From: somebodyelse@host", "Subject: Two", "", "Test Body", "."}},
+		// A retained message still downloads fine.
+		{"RETR 1", []string{fmt.Sprintf("+OK %v bytes follows", size1),
+			"To: somebody@host", "From: somebodyelse@host", "Subject: One", "", "Test Body", "."}},
 		{"QUIT", []string{"+OK We will process your deletes"}},
 	})
 	playPOP3Session(t, server, script)
@@ -514,18 +515,16 @@ func TestTransactionTopArgumentErrors(t *testing.T) {
 	}
 }
 
-// Documents current behavior: RFC 1939 (TOP, section 7) requires a negative
+// RFC 1939 §7 (TOP): a message-number may NOT refer to a message marked as
-// status indicator when TOP refers to a message marked as deleted, but the
+// deleted.
-// handler does not check the retain flag. Flagged for a follow-up fix.
+func TestTransactionTopDeletedMessageRejected(t *testing.T) {
-func TestTransactionTopDeletedMessage(t *testing.T) {
 	ds := newMemStore(t)
 	test.DeliverToStore(t, ds, "mailbox", "One", time.Now())
 	server := setupPOPServer(t, ds, false, false)
 
 	script := slices.Concat(loginSteps(1), []scriptStep{
 		{"DELE 1", []string{"+OK Deleted message 1"}},
-		{"TOP 1 0", []string{"+OK Top of message follows",
+		{"TOP 1 0", []string{"-ERR You deleted message 1"}},
-			"To: somebody@host", "From: somebodyelse@host", "Subject: One", "", "."}},
 		{"QUIT", []string{"+OK We will process your deletes"}},
 	})
 	playPOP3Session(t, server, script)