Fix for passiveListenIP. Currently the function returns different (#70

)

formats depending if the value is retrieved from the underlying
connection or from the value set by given parameters.
If the value is taken from the parameters then the value is returned as
it was set.
If it is retrieved from the underlying connection, then `Addr.String()`
is used which appends the port number.
The code using the value returned from the function always assume that
the format is `<ip>:<port`. Which is not the case for a listen ip set
by the user.
I decided to change the behavior and make `passiveListenIP` always
return an IP address, as it names suggest. This fixes the issue and
removed the need of checking for the format on the calling side.

cmd.go

@@ -312,13 +312,7 @@ func (cmd commandEpsv) RequireAuth() bool {
 
 func (cmd commandEpsv) Execute(conn *Conn, param string) {
 	addr := conn.passiveListenIP()
-	lastIdx := strings.LastIndex(addr, ":")
-	if lastIdx <= 0 {
-		conn.writeMessage(425, "Data connection failed")
-		return
-	}
-
-	socket, err := newPassiveSocket(addr[:lastIdx], conn.PassivePort, conn.logger, conn.sessionID, conn.tlsConfig)
+	socket, err := newPassiveSocket(addr, conn.PassivePort, conn.logger, conn.sessionID, conn.tlsConfig)
 	if err != nil {
 		log.Println(err)
 		conn.writeMessage(425, "Data connection failed")
@@ -587,20 +581,15 @@ func (cmd commandPasv) RequireAuth() bool {
 
 func (cmd commandPasv) Execute(conn *Conn, param string) {
 	listenIP := conn.passiveListenIP()
-	lastIdx := strings.LastIndex(listenIP, ":")
-	if lastIdx <= 0 {
-		conn.writeMessage(425, "Data connection failed")
-		return
-	}
-	socket, err := newPassiveSocket(listenIP[:lastIdx], conn.PassivePort, conn.logger, conn.sessionID, conn.tlsConfig)
+	socket, err := newPassiveSocket(listenIP, conn.PassivePort, conn.logger, conn.sessionID, conn.tlsConfig)
 	if err != nil {
 		conn.writeMessage(425, "Data connection failed")
 		return
 	}
 	conn.dataConn = socket
 	p1 := socket.Port() / 256
 	p2 := socket.Port() - (p1 * 256)
-	quads := strings.Split(listenIP[:lastIdx], ".")
+	quads := strings.Split(listenIP, ".")
 	target := fmt.Sprintf("(%s,%s,%s,%s,%d,%d)", quads[0], quads[1], quads[2], quads[3], p1, p2)
 	msg := "Entering Passive Mode " + target
 	conn.writeMessage(227, msg)

conn.go

@@ -61,7 +61,12 @@ func (conn *Conn) passiveListenIP() string {
 	if len(conn.PublicIp()) > 0 {
 		return conn.PublicIp()
 	}
-	return conn.conn.LocalAddr().String()
+	listenIP := conn.conn.LocalAddr().String()
+	lastIdx := strings.LastIndex(listenIP, ":")
+	if lastIdx <= 0 {
+		return listenIP
+	}
+	return listenIP[:lastIdx]
 }
 
 func (conn *Conn) PassivePort() int {

conn_test.go

@@ -5,7 +5,9 @@
 package server
 
 import (
+	"net"
 	"testing"
+	"time"
 )
 
 func TestConnBuildPath(t *testing.T) {
@@ -32,3 +34,60 @@ func TestConnBuildPath(t *testing.T) {
 		})
 	}
 }
+
+type mockConn struct {
+	ip   net.IP
+	port int
+}
+
+func (m mockConn) Read(b []byte) (n int, err error) {
+	return 0, nil
+}
+func (m mockConn) Write(b []byte) (n int, err error) {
+	return 0, nil
+}
+func (m mockConn) Close() error {
+	return nil
+}
+func (m mockConn) LocalAddr() net.Addr {
+	return &net.TCPAddr{
+		IP:   m.ip,
+		Port: m.port,
+	}
+}
+func (m mockConn) RemoteAddr() net.Addr {
+	return nil
+}
+func (m mockConn) SetDeadline(t time.Time) error {
+	return nil
+}
+func (m mockConn) SetReadDeadline(t time.Time) error {
+	return nil
+}
+func (m mockConn) SetWriteDeadline(t time.Time) error {
+	return nil
+}
+func TestPassiveListenIP(t *testing.T) {
+	c := &Conn{
+		server: &Server{
+			ServerOpts: &ServerOpts{
+				PublicIp: "1.1.1.1",
+			},
+		},
+	}
+	if c.passiveListenIP() != "1.1.1.1" {
+		t.Fatalf("Expected passive listen IP to be 1.1.1.1 but got %s", c.passiveListenIP())
+	}
+
+	c = &Conn{
+		conn: mockConn{
+			ip: net.IPv4(1, 1, 1, 1),
+		},
+		server: &Server{
+			ServerOpts: &ServerOpts{},
+		},
+	}
+	if c.passiveListenIP() != "1.1.1.1" {
+		t.Fatalf("Expected passive listen IP to be 1.1.1.1 but got %s", c.passiveListenIP())
+	}
+}