From 46babc89c82dd3ae4f45fbffb84ace8b75de1a7c Mon Sep 17 00:00:00 2001
From: JB <28275108+mochi-co@users.noreply.github.com>
Date: Sat, 25 Feb 2023 01:37:54 +0000
Subject: [PATCH] Allow 0 byte usernames if correctly formed (#181)

* Allow 0 byte usernames if correctly formed

* Allow 0 byte usernames if correctly formed
---
 packets/packets.go  |  8 ++---
 packets/tpackets.go | 71 +++++++++++++++++++++++++++++++++++----------
 server_test.go      | 27 +++++++++++++++++
 3 files changed, 87 insertions(+), 19 deletions(-)

diff --git a/packets/packets.go b/packets/packets.go
index 36125d73..0e067170 100644
--- a/packets/packets.go
+++ b/packets/packets.go
@@ -412,6 +412,10 @@ func (pk *Packet) ConnectDecode(buf []byte) error {
 	}
 
 	if pk.Connect.UsernameFlag { // [MQTT-3.1.3-12]
+		if offset >= len(buf) { // we are at the end of the packet
+			return ErrProtocolViolationFlagNoUsername // [MQTT-3.1.2-17]
+		}
+
 		pk.Connect.Username, offset, err = decodeBytes(buf, offset)
 		if err != nil {
 			return ErrMalformedUsername
@@ -451,10 +455,6 @@ func (pk *Packet) ConnectValidate() Code {
 		return ErrProtocolViolationUsernameTooLong
 	}
 
-	if pk.Connect.UsernameFlag && len(pk.Connect.Username) == 0 {
-		return ErrProtocolViolationFlagNoUsername // [MQTT-3.1.2-17]
-	}
-
 	if !pk.Connect.UsernameFlag && len(pk.Connect.Username) > 0 {
 		return ErrProtocolViolationUsernameNoFlag // [MQTT-3.1.2-16]
 	}
diff --git a/packets/tpackets.go b/packets/tpackets.go
index d8bc0cac..934ce05e 100644
--- a/packets/tpackets.go
+++ b/packets/tpackets.go
@@ -71,7 +71,7 @@ const (
 	TConnectInvalidWillFlagNoPayload
 	TConnectInvalidWillFlagQosOutOfRange
 	TConnectInvalidWillSurplusRetain
-	TConnectNotCleanNoClientID
+	TConnectZeroByteUsername
 	TConnectSpecInvalidUTF8D800
 	TConnectSpecInvalidUTF8DFFF
 	TConnectSpecInvalidUTF80000
@@ -498,6 +498,43 @@ var TPacketData = map[byte]TPacketCases{
 				},
 			},
 		},
+		{
+			Case:  TConnectZeroByteUsername,
+			Desc:  "username flag but 0 byte username",
+			Group: "decode",
+			RawBytes: []byte{
+				Connect << 4, 23, // Fixed header
+				0, 4, // Protocol Name - MSB+LSB
+				'M', 'Q', 'T', 'T', // Protocol Name
+				5,     // Protocol Version
+				130,   // Packet Flags
+				0, 30, // Keepalive
+				5,                // length
+				17, 0, 0, 0, 120, // Session Expiry Interval (17)
+				0, 3, // Client ID - MSB+LSB
+				'z', 'e', 'n', // Client ID "zen"
+				0, 0, // Username MSB+LSB
+			},
+			Packet: &Packet{
+				FixedHeader: FixedHeader{
+					Type:      Connect,
+					Remaining: 23,
+				},
+				ProtocolVersion: 5,
+				Connect: ConnectParams{
+					ProtocolName:     []byte("MQTT"),
+					Clean:            true,
+					Keepalive:        30,
+					ClientIdentifier: "zen",
+					Username:         []byte{},
+					UsernameFlag:     true,
+				},
+				Properties: Properties{
+					SessionExpiryInterval:     uint32(120),
+					SessionExpiryIntervalFlag: true,
+				},
+			},
+		},
 
 		// Fail States
 		{
@@ -624,6 +661,24 @@ var TPacketData = map[byte]TPacketCases{
 				'm', 'o', 'c',
 			},
 		},
+
+		{
+			Case:      TConnectInvalidFlagNoUsername,
+			Desc:      "username flag with no username bytes",
+			Group:     "decode",
+			FailFirst: ErrProtocolViolationFlagNoUsername,
+			RawBytes: []byte{
+				Connect << 4, 17, // Fixed header
+				0, 4, // Protocol Name - MSB+LSB
+				'M', 'Q', 'T', 'T', // Protocol Name
+				5,     // Protocol Version
+				130,   // Flags
+				0, 20, // Keepalive
+				0,
+				0, 3, // Client ID - MSB+LSB
+				'z', 'e', 'n', // Client ID "zen"
+			},
+		},
 		{
 			Case:      TConnectMalPassword,
 			Desc:      "malformed password",
@@ -784,20 +839,6 @@ var TPacketData = map[byte]TPacketCases{
 				},
 			},
 		},
-		{
-			Case:   TConnectInvalidFlagNoUsername,
-			Desc:   "has username flag but no username",
-			Group:  "validate",
-			Expect: ErrProtocolViolationFlagNoUsername,
-			Packet: &Packet{
-				FixedHeader:     FixedHeader{Type: Connect},
-				ProtocolVersion: 4,
-				Connect: ConnectParams{
-					ProtocolName: []byte("MQTT"),
-					UsernameFlag: true,
-				},
-			},
-		},
 		{
 			Case:   TConnectInvalidUsernameNoFlag,
 			Desc:   "has username but no flag",
diff --git a/server_test.go b/server_test.go
index d6fdaabf..31905ea1 100644
--- a/server_test.go
+++ b/server_test.go
@@ -748,6 +748,33 @@ func TestServerEstablishConnectionInvalidConnect(t *testing.T) {
 	r.Close()
 }
 
+// See https://github.com/mochi-co/mqtt/issues/178
+func TestServerEstablishConnectionZeroByteUsernameIsValid(t *testing.T) {
+	s := newServer()
+
+	r, w := net.Pipe()
+	o := make(chan error)
+	go func() {
+		o <- s.EstablishConnection("tcp", r)
+	}()
+
+	go func() {
+		w.Write(packets.TPacketData[packets.Connect].Get(packets.TConnectZeroByteUsername).RawBytes)
+		w.Write(packets.TPacketData[packets.Disconnect].Get(packets.TDisconnect).RawBytes)
+	}()
+
+	// receive the connack error
+	go func() {
+		_, err := io.ReadAll(w)
+		require.NoError(t, err)
+	}()
+
+	err := <-o
+	require.NoError(t, err)
+
+	r.Close()
+}
+
 func TestServerEstablishConnectionInvalidConnectAckFailure(t *testing.T) {
 	s := newServer()