fix(communication): tighten ProtobufMessageParser gap-gate + wire-type validation (closes #189)

src/Daqifi.Core.Tests/Communication/Consumers/ProtobufMessageParserTests.cs

@@ -230,6 +230,145 @@ public void ProtobufMessageParser_ParseMessages_PreservesPartialFrameAfterGarbag
         Assert.Equal(junk.Length, consumedBytes);
     }
 
+    [Fact]
+    public void ProtobufMessageParser_ParseMessages_LegitimateMultiKbFrame_NotCorruptedByGapGate()
+    {
+        // Closes #189 Bug 1. The gap gate previously fired whenever
+        // missingPayload > MaxPartialFrameGapBytes regardless of how many
+        // body bytes had already arrived. A legitimate multi-KB frame
+        // arriving across multiple reads (e.g. a fat initial-status frame
+        // on a transport that returns smaller chunks) would be misclassified
+        // as garbage mid-frame: the parser would advance into the real body
+        // and corrupt it. The fix gates the suspicion check on
+        // availableBodyBytes <= 1 — once 2+ body bytes are buffered, the
+        // field-tag check has already structurally validated the start.
+        //
+        // Construct a real frame whose declared length leaves a gap larger
+        // than MaxPartialFrameGapBytes (1024) and feed only the prefix +
+        // a couple of body bytes. The parser must NOT advance into the
+        // body — it must wait for the rest.
+
+        // Arrange
+        var parser = new ProtobufMessageParser();
+
+        // Build a real frame ~2KB with DeviceFwRev padded to push the body
+        // past the gap threshold. WriteDelimitedTo prepends the varint
+        // length prefix.
+        var msg = new DaqifiOutMessage
+        {
+            MsgTimeStamp = 99,
+            DeviceFwRev = new string('x', 2000),
+        };
+        using var stream = new MemoryStream();
+        msg.WriteDelimitedTo(stream);
+        var fullFrame = stream.ToArray();
+        Assert.True(fullFrame.Length > 1100,
+            "Test precondition: frame must exceed MaxPartialFrameGapBytes (1024) so the gap gate would fire on a one-shot read.");
+
+        // Feed only prefix + first 4 body bytes — body is partially present
+        // but not complete. Parser must wait, not advance into the real body.
+        var prefixPlusFour = fullFrame.Take(GetVarintLength(fullFrame) + 4).ToArray();
+
+        // Act
+        var messages = parser.ParseMessages(prefixPlusFour, out var consumedBytes).ToList();
+
+        // Assert: parser waits — no message parsed, no bytes advanced.
+        // Callers retain the buffer for the next read; corrupting it here
+        // (advancing into the body) would lose the frame forever.
+        Assert.Empty(messages);
+        Assert.Equal(0, consumedBytes);
+    }
+
+    [Fact]
+    public void ProtobufMessageParser_ParseMessages_OneBodyByteOfMultiKbFrame_NotCorrupted()
+    {
+        // Edge case of #189 Bug 1: when EXACTLY one body byte of a real
+        // multi-KB frame has arrived (common with small chunked reads),
+        // the gap gate must not fire. The single body byte's field-tag
+        // check provides positive evidence the prefix is real; advancing
+        // even one byte here would unrecoverably corrupt the frame
+        // because StreamMessageConsumer trims consumedBytes from its
+        // buffer between reads.
+
+        // Arrange
+        var parser = new ProtobufMessageParser();
+
+        var msg = new DaqifiOutMessage
+        {
+            MsgTimeStamp = 99,
+            DeviceFwRev = new string('x', 2000),
+        };
+        using var stream = new MemoryStream();
+        msg.WriteDelimitedTo(stream);
+        var fullFrame = stream.ToArray();
+
+        // Sanity-check the regression precondition: this test is meaningless
+        // unless the frame body would have triggered the old gap gate. The
+        // sibling LegitimateMultiKbFrame test uses the same threshold; if the
+        // fixture message ever shrinks below it, the regression silently
+        // de-scopes — fail loudly here instead.
+        Assert.True(fullFrame.Length > 1100,
+            $"Test fixture too small to exercise the gap gate (got {fullFrame.Length}, expected > 1100). Increase DeviceFwRev length.");
+
+        // Feed prefix + exactly 1 body byte. The first body byte of a real
+        // DaqifiOutMessage is a valid field tag (passes IsPlausibleFieldTagByte).
+        var prefixPlusOne = fullFrame.Take(GetVarintLength(fullFrame) + 1).ToArray();
+
+        // Act
+        var messages = parser.ParseMessages(prefixPlusOne, out var consumedBytes).ToList();
+
+        // Assert: parser waits, does not advance into the legitimate body.
+        Assert.Empty(messages);
+        Assert.Equal(0, consumedBytes);
+    }
+
+    [Theory]
+    [InlineData((byte)0x83)] // continuation bit set, wire type 3 (deprecated SGROUP)
+    [InlineData((byte)0x84)] // continuation bit set, wire type 4 (deprecated EGROUP)
+    [InlineData((byte)0x86)] // continuation bit set, wire type 6 (undefined)
+    [InlineData((byte)0x87)] // continuation bit set, wire type 7 (undefined)
+    public void ProtobufMessageParser_ParseMessages_GarbageWithContinuationBit_StillRejected(byte garbageBodyByte)
+    {
+        // Closes #189 Bug 2. IsPlausibleFieldTagByte previously early-
+        // returned true for any byte with the continuation bit (0x80) set,
+        // bypassing wire-type validation. Wire type lives in the low 3 bits
+        // of the first byte regardless of multi-byte tags, so impossible
+        // wire types (3,4,6,7) MUST be rejected even on continuation bytes.
+        //
+        // Construct a frame with: a plausible-but-bogus length prefix +
+        // a body byte whose continuation bit is set AND whose low 3 bits
+        // form an impossible wire type. Without the fix, the parser
+        // accepts the body byte and stalls waiting for the bogus declared
+        // length to arrive. With the fix, the field-tag gate fires and
+        // the parser advances past the garbage.
+
+        // Arrange
+        var parser = new ProtobufMessageParser();
+
+        // Length prefix: 0x10 = 16 bytes (well under MaxPartialFrameGapBytes
+        // so the gap gate doesn't fire on its own — we want to isolate the
+        // wire-type check).
+        var data = new byte[] { 0x10, garbageBodyByte };
+
+        // Act
+        var messages = parser.ParseMessages(data, out var consumedBytes).ToList();
+
+        // Assert: parser advances past the bogus prefix instead of waiting
+        // for 16 never-coming bytes.
+        Assert.Empty(messages);
+        Assert.True(consumedBytes >= 1,
+            $"Expected parser to advance past garbage prefix, got consumedBytes={consumedBytes}");
+    }
+
+    private static int GetVarintLength(byte[] buffer)
+    {
+        for (var i = 0; i < buffer.Length && i < 5; i++)
+        {
+            if ((buffer[i] & 0x80) == 0) return i + 1;
+        }
+        throw new InvalidOperationException("Malformed varint in test fixture");
+    }
+
     [Fact]
     public void ProtobufMessageParser_ParseMessages_ReturnsCorrectMessageType()
     {

src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs

@@ -108,11 +108,26 @@ public IEnumerable<IInboundMessage<DaqifiOutMessage>> ParseMessages(byte[] data,
                 // Neither gate advances into a real frame's prefix or payload: only
                 // into bytes already identified as implausible. Callers that trim
                 // consumedBytes from their buffer never lose recoverable data.
-                var missingPayload = messageLength - (remainingData.Length - prefixBytes);
-                var firstBodyByteIsGarbage = remainingData.Length > prefixBytes
+                var availableBodyBytes = remainingData.Length - prefixBytes;
+                var missingPayload = messageLength - availableBodyBytes;
+                var firstBodyByteIsGarbage = availableBodyBytes > 0
                     && !IsPlausibleFieldTagByte(remainingData[prefixBytes]);
 
-                if (missingPayload > MaxPartialFrameGapBytes || firstBodyByteIsGarbage)
+                // Gap gate ONLY applies in the pure-prefix-no-body case
+                // (no body bytes arrived). Once any body byte is buffered,
+                // the field-tag gate above provides positive evidence that
+                // the prefix really is a frame start; a large declared
+                // length at that point is just a multi-chunk read of a
+                // legitimate frame (e.g. a multi-KB initial-status frame
+                // on a transport that returns smaller reads). Including
+                // availableBodyBytes == 1 here would corrupt real frames
+                // whose first body byte arrives alone — a single-byte
+                // mistaken advance is unrecoverable because callers trim
+                // consumedBytes from their buffer.
+                var gapIsSuspicious = availableBodyBytes == 0
+                    && missingPayload > MaxPartialFrameGapBytes;
+
+                if (gapIsSuspicious || firstBodyByteIsGarbage)
                 {
                     currentIndex++;
                     consumedBytes = currentIndex;
@@ -146,29 +161,32 @@ public IEnumerable<IInboundMessage<DaqifiOutMessage>> ParseMessages(byte[] data,
     /// Returns true if <paramref name="b"/> could plausibly be the first byte of a
     /// real DaqifiOutMessage body (a protobuf field tag). Used by the partial-frame
     /// wait path to reject garbage whose varint prefix happens to decode to a
-    /// plausible-looking length. Multi-byte tags (continuation bit set) can't be
-    /// fully validated from a single byte, so they pass this check.
+    /// plausible-looking length. Multi-byte tags (continuation bit set) can't have
+    /// their full field number validated from a single byte, but the wire type
+    /// always lives in the low 3 bits of the first byte regardless — so impossible
+    /// wire types are rejected even on continuation bytes.
     /// </summary>
     private static bool IsPlausibleFieldTagByte(byte b)
     {
-        // Continuation bit set → multi-byte tag. We can't fully validate without
-        // more bytes (which we may not have yet); don't reject on that alone.
-        if ((b & 0x80) != 0)
-        {
-            return true;
-        }
-
-        // Single-byte tag: low 3 bits are wire type, next 4 bits (within the low 7)
-        // are the field number. Valid wire types are VARINT(0), I64(1), LEN(2),
-        // I32(5). Wire types 3 (SGROUP) and 4 (EGROUP) are deprecated group types
-        // that DaqifiOutMessage does not use; 6/7 are undefined. Field number 0
-        // is reserved and never appears in a real message.
+        // Wire type lives in the low 3 bits regardless of whether the field
+        // number spans multiple bytes. Valid wire types are VARINT(0), I64(1),
+        // LEN(2), I32(5). Wire types 3 (SGROUP) and 4 (EGROUP) are deprecated
+        // group types that DaqifiOutMessage does not use; 6/7 are undefined.
         var wireType = b & 0x07;
         if (wireType is 3 or 4 or 6 or 7)
         {
             return false;
         }
 
+        // Continuation bit set → multi-byte tag. The field number spans more
+        // bytes which we may not have yet; accept once wire type passed.
+        if ((b & 0x80) != 0)
+        {
+            return true;
+        }
+
+        // Single-byte tag: bits 6..3 are the field number. Field number 0 is
+        // reserved and never appears in a real message.
         var fieldNumber = (b >> 3) & 0x0F;
         return fieldNumber != 0;
     }