Repository: netty/netty
Issue: codec.mqtt: wrong password and willMessage field types in MqttConnectPayload/MqttEncoder (should be byte[])

In the present implementation of MqttConnectPayload/MqttEncoder, willMessage and password fields are  strings. However, spec (3.1.3.5) clearly mandates that the password filed should support arbitrary binary data:

> The Password field contains 0 to 65535 bytes of binary data prefixed with a two byte length field which indicates the number of bytes used by the binary data (it does not include the two bytes taken up by the length field itself). 

Same for willMessage (3.1.3.3):

> This field consists of a two byte length followed by the payload for the Will Message expressed as a sequence of zero or more bytes.

Thus, the relevant field and encoder bits must treat the password and willMessage fields as byte[] or ByteBuf (the former is probably enough).

This has important practical implications and the fix is straightforward and simple.

Issue comments:
are you interested in submitting a PR to correct this issue?
Fixed by https://github.com/netty/netty/pull/681

Pull request review comments:
Please use the same approach (add a new method) as in https://github.com/netty/netty/pull/6769 and avoid breaking the public API.
can we remove `minBytes` and `maxBytes` parameters? `maxBytes` is always `MAX_VALUE` and `minBytes` is always `0`. can we instead just throw an exception (or return early) if `size < 0`?
should we deprecate this constructor in favor of the new constructor which takes `byte[]`?
Please deprecate this method and refer to the `willMessageInBytes` in the `@deprecated` javadocs.
Please deprecate this method and refer to the `passwordInBytes ` in the `@deprecated` javadocs.
this is invalid and will throw an exception.
do you really want to decrement `numberOfBytesConsumed`?
can you add a unit test which hits this code?
deprecate this method and reference the new method in javadocs?
deprecate this method and reference the new method in javadocs?
This part of the code will be hit only in a hypothetical situation when the encoded bytebuf have some issues like length of a message field, say password is set to -1 etc.

However, if the assumption is, the encoded bytebuf is created out of the MqttEncoder, then this part of the code can be safely commented out.
I still don't understand this line of code. 2 bytes have still been consumed either way even if the data is invalid. This value is ultimately set to `bytesRemainingInVariablePart` which may be used later during the decode process. You mentioned this was a [hypothetical situation](https://github.com/netty/netty/pull/6817#discussion_r120771580) but it is not clear what will happen if this situation does occur (if its possible at all). If this situation is possible please add a unit test, and if it isn't possible can we remove this line?
Now a variable `numberOfBytesConsumed` is not needed.
```java
return new Result<byte[]>(bytes, decodedSize.numberOfBytesConsumed + size);
```

Patch that resolved the issue:
diff --git a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttConnectPayload.java b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttConnectPayload.java
index 1e16acf4430..e274e3f29e6 100644
--- a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttConnectPayload.java
+++ b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttConnectPayload.java
@@ -16,6 +16,7 @@
 
 package io.netty.handler.codec.mqtt;
 
+import io.netty.util.CharsetUtil;
 import io.netty.util.internal.StringUtil;
 
 /**
@@ -25,16 +26,34 @@ public final class MqttConnectPayload {
 
     private final String clientIdentifier;
     private final String willTopic;
-    private final String willMessage;
+    private final byte[] willMessage;
     private final String userName;
-    private final String password;
+    private final byte[] password;
 
+    /**
+     * @deprecated use {@link MqttConnectPayload#MqttConnectPayload(String, String, byte[], String, byte[])} instead
+     */
+    @Deprecated
     public MqttConnectPayload(
             String clientIdentifier,
             String willTopic,
             String willMessage,
             String userName,
             String password) {
+        this(
+          clientIdentifier,
+          willTopic,
+          willMessage.getBytes(CharsetUtil.UTF_8),
+          userName,
+          password.getBytes(CharsetUtil.UTF_8));
+    }
+
+    public MqttConnectPayload(
+            String clientIdentifier,
+            String willTopic,
+            byte[] willMessage,
+            String userName,
+            byte[] password) {
         this.clientIdentifier = clientIdentifier;
         this.willTopic = willTopic;
         this.willMessage = willMessage;
@@ -50,7 +69,15 @@ public String willTopic() {
         return willTopic;
     }
 
+    /**
+     * @deprecated use {@link MqttConnectPayload#willMessageInBytes()} instead
+     */
+    @Deprecated
     public String willMessage() {
+        return new String(willMessage, CharsetUtil.UTF_8);
+    }
+
+    public byte[] willMessageInBytes() {
         return willMessage;
     }
 
@@ -58,7 +85,15 @@ public String userName() {
         return userName;
     }
 
+    /**
+     * @deprecated use {@link MqttConnectPayload#passwordInBytes()} instead
+     */
+    @Deprecated
     public String password() {
+        return new String(password, CharsetUtil.UTF_8);
+    }
+
+    public byte[] passwordInBytes() {
         return password;
     }
 
diff --git a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java
index b734c349a09..c8a3546e0ed 100644
--- a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java
+++ b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttDecoder.java
@@ -339,21 +339,21 @@ private static Result<MqttConnectPayload> decodeConnectionPayload(
         int numberOfBytesConsumed = decodedClientId.numberOfBytesConsumed;
 
         Result<String> decodedWillTopic = null;
-        Result<String> decodedWillMessage = null;
+        Result<byte[]> decodedWillMessage = null;
         if (mqttConnectVariableHeader.isWillFlag()) {
             decodedWillTopic = decodeString(buffer, 0, 32767);
             numberOfBytesConsumed += decodedWillTopic.numberOfBytesConsumed;
-            decodedWillMessage = decodeAsciiString(buffer);
+            decodedWillMessage = decodeByteArray(buffer);
             numberOfBytesConsumed += decodedWillMessage.numberOfBytesConsumed;
         }
         Result<String> decodedUserName = null;
-        Result<String> decodedPassword = null;
+        Result<byte[]> decodedPassword = null;
         if (mqttConnectVariableHeader.hasUserName()) {
             decodedUserName = decodeString(buffer);
             numberOfBytesConsumed += decodedUserName.numberOfBytesConsumed;
         }
         if (mqttConnectVariableHeader.hasPassword()) {
-            decodedPassword = decodeString(buffer);
+            decodedPassword = decodeByteArray(buffer);
             numberOfBytesConsumed += decodedPassword.numberOfBytesConsumed;
         }
 
@@ -419,17 +419,6 @@ private static Result<String> decodeString(ByteBuf buffer) {
         return decodeString(buffer, 0, Integer.MAX_VALUE);
     }
 
-    private static Result<String> decodeAsciiString(ByteBuf buffer) {
-        Result<String> result = decodeString(buffer, 0, Integer.MAX_VALUE);
-        final String s = result.value;
-        for (int i = 0; i < s.length(); i++) {
-            if (s.charAt(i) > 127) {
-                return new Result<String>(null, result.numberOfBytesConsumed);
-            }
-        }
-        return new Result<String>(s, result.numberOfBytesConsumed);
-    }
-
     private static Result<String> decodeString(ByteBuf buffer, int minBytes, int maxBytes) {
         final Result<Integer> decodedSize = decodeMsbLsb(buffer);
         int size = decodedSize.value;
@@ -445,6 +434,14 @@ private static Result<String> decodeString(ByteBuf buffer, int minBytes, int max
         return new Result<String>(s, numberOfBytesConsumed);
     }
 
+    private static Result<byte[]> decodeByteArray(ByteBuf buffer) {
+        final Result<Integer> decodedSize = decodeMsbLsb(buffer);
+        int size = decodedSize.value;
+        byte[] bytes = new byte[size];
+        buffer.readBytes(bytes);
+        return new Result<byte[]>(bytes, decodedSize.numberOfBytesConsumed + size);
+    }
+
     private static Result<Integer> decodeMsbLsb(ByteBuf buffer) {
         return decodeMsbLsb(buffer, 0, 65535);
     }
diff --git a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttEncoder.java b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttEncoder.java
index 4ee7293075e..967cdb844bb 100644
--- a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttEncoder.java
+++ b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttEncoder.java
@@ -113,8 +113,8 @@ private static ByteBuf encodeConnectMessage(
         // Will topic and message
         String willTopic = payload.willTopic();
         byte[] willTopicBytes = willTopic != null ? encodeStringUtf8(willTopic) : EmptyArrays.EMPTY_BYTES;
-        String willMessage = payload.willMessage();
-        byte[] willMessageBytes = willMessage != null ? encodeStringUtf8(willMessage) : EmptyArrays.EMPTY_BYTES;
+        byte[] willMessage = payload.willMessageInBytes();
+        byte[] willMessageBytes = willMessage != null ? willMessage : EmptyArrays.EMPTY_BYTES;
         if (variableHeader.isWillFlag()) {
             payloadBufferSize += 2 + willTopicBytes.length;
             payloadBufferSize += 2 + willMessageBytes.length;
@@ -126,8 +126,8 @@ private static ByteBuf encodeConnectMessage(
             payloadBufferSize += 2 + userNameBytes.length;
         }
 
-        String password = payload.password();
-        byte[] passwordBytes = password != null ? encodeStringUtf8(password) : EmptyArrays.EMPTY_BYTES;
+        byte[] password = payload.passwordInBytes();
+        byte[] passwordBytes = password != null ? password : EmptyArrays.EMPTY_BYTES;
         if (variableHeader.hasPassword()) {
             payloadBufferSize += 2 + passwordBytes.length;
         }
diff --git a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttMessageBuilders.java b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttMessageBuilders.java
index 51be0072b5b..b893205ee76 100644
--- a/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttMessageBuilders.java
+++ b/codec-mqtt/src/main/java/io/netty/handler/codec/mqtt/MqttMessageBuilders.java
@@ -17,6 +17,7 @@
 
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
+import io.netty.util.CharsetUtil;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -77,9 +78,9 @@ public static final class ConnectBuilder {
         private boolean willRetain;
         private MqttQoS willQos = MqttQoS.AT_MOST_ONCE;
         private String willTopic;
-        private String willMessage;
+        private byte[] willMessage;
         private String username;
-        private String password;
+        private byte[] password;
 
         ConnectBuilder() {
         }
@@ -119,7 +120,16 @@ public ConnectBuilder willTopic(String willTopic) {
             return this;
         }
 
+        /**
+         * @deprecated use {@link ConnectBuilder#willMessage(byte[])} instead
+         */
+        @Deprecated
         public ConnectBuilder willMessage(String willMessage) {
+            willMessage(willMessage.getBytes(CharsetUtil.UTF_8));
+            return this;
+        }
+
+        public ConnectBuilder willMessage(byte[] willMessage) {
             this.willMessage = willMessage;
             return this;
         }
@@ -145,7 +155,16 @@ public ConnectBuilder username(String username) {
             return this;
         }
 
+        /**
+         * @deprecated use {@link ConnectBuilder#password(byte[])} instead
+         */
+        @Deprecated
         public ConnectBuilder password(String password) {
+            password(password.getBytes(CharsetUtil.UTF_8));
+            return this;
+        }
+
+        public ConnectBuilder password(byte[] password) {
             this.hasPassword = true;
             this.password = password;
             return this;


Test patch:
diff --git a/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java b/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java
index 8bfd2fb95db..21a69055ed5 100644
--- a/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java
+++ b/codec-mqtt/src/test/java/io/netty/handler/codec/mqtt/MqttCodecTest.java
@@ -28,6 +28,7 @@
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 
+import java.util.Arrays;
 import java.util.LinkedList;
 import java.util.List;
 
@@ -395,7 +396,13 @@ private static void validateConnectPayload(MqttConnectPayload expected, MqttConn
                 actual.clientIdentifier());
         assertEquals("MqttConnectPayload UserName mismatch ", expected.userName(), actual.userName());
         assertEquals("MqttConnectPayload Password mismatch ", expected.password(), actual.password());
+        assertTrue(
+                "MqttConnectPayload Password bytes mismatch ",
+                Arrays.equals(expected.passwordInBytes(), actual.passwordInBytes()));
         assertEquals("MqttConnectPayload WillMessage mismatch ", expected.willMessage(), actual.willMessage());
+        assertTrue(
+                "MqttConnectPayload WillMessage bytes mismatch ",
+                Arrays.equals(expected.willMessageInBytes(), actual.willMessageInBytes()));
         assertEquals("MqttConnectPayload WillTopic mismatch ", expected.willTopic(), actual.willTopic());
     }