Check for matching field numbers in StartGroup / EndGroup tags (#816)

This is a port of #810 from
the v2 branch into the main (v1) branch.

---------

Co-authored-by: Timo Stamm <ts@timostamm.de>

packages/protobuf-bench/README.md

@@ -10,5 +10,5 @@ server would usually do.
 
 | code generator      | bundle size             | minified               | compressed         |
 |---------------------|------------------------:|-----------------------:|-------------------:|
-| protobuf-es         | 97,764 b      | 41,777 b | 10,850 b |
+| protobuf-es         | 98,070 b      | 41,868 b | 10,882 b |
 | protobuf-javascript | 394,384 b  | 288,654 b | 45,122 b |

packages/protobuf-test/src/binary-reader-writer.test.ts

@@ -13,7 +13,7 @@
 // limitations under the License.
 
 import { describe, expect, it } from "@jest/globals";
-import { BinaryWriter, WireType } from "@bufbuild/protobuf";
+import { BinaryReader, BinaryWriter, WireType } from "@bufbuild/protobuf";
 import { User } from "./gen/ts/extra/example_pb.js";
 
 describe("BinaryWriter example", () => {
@@ -31,3 +31,78 @@ describe("BinaryWriter example", () => {
     expect(user.active).toBe(true);
   });
 });
+
+describe("BinaryReader", () => {
+  describe("skip", () => {
+    it("should skip group", () => {
+      const reader = new BinaryReader(
+        new BinaryWriter()
+          .tag(1, WireType.StartGroup)
+          .tag(33, WireType.Varint)
+          .bool(true)
+          .tag(1, WireType.EndGroup)
+          .finish(),
+      );
+      const [fieldNo, wireType] = reader.tag();
+      expect(fieldNo).toBe(1);
+      expect(wireType).toBe(WireType.StartGroup);
+      reader.skip(WireType.StartGroup, 1);
+      expect(reader.pos).toBe(reader.len);
+    });
+    it("should skip nested group", () => {
+      const reader = new BinaryReader(
+        new BinaryWriter()
+          .tag(1, WireType.StartGroup)
+          .tag(1, WireType.StartGroup)
+          .tag(1, WireType.EndGroup)
+          .tag(1, WireType.EndGroup)
+          .finish(),
+      );
+      const [fieldNo, wireType] = reader.tag();
+      expect(fieldNo).toBe(1);
+      expect(wireType).toBe(WireType.StartGroup);
+      reader.skip(WireType.StartGroup, 1);
+      expect(reader.pos).toBe(reader.len);
+    });
+    it("should error on unexpected end group field number", () => {
+      const reader = new BinaryReader(
+        new BinaryWriter()
+          .tag(1, WireType.StartGroup)
+          .tag(2, WireType.EndGroup)
+          .finish(),
+      );
+      const [fieldNo, wireType] = reader.tag();
+      expect(fieldNo).toBe(1);
+      expect(wireType).toBe(WireType.StartGroup);
+      expect(() => {
+        reader.skip(WireType.StartGroup, 1);
+      }).toThrow(/^invalid end group tag$/);
+    });
+    it("should return skipped group data", () => {
+      const reader = new BinaryReader(
+        new BinaryWriter()
+          .tag(1, WireType.StartGroup)
+          .tag(33, WireType.Varint)
+          .bool(true)
+          .tag(1, WireType.EndGroup)
+          .finish(),
+      );
+      reader.tag();
+      const skipped = reader.skip(WireType.StartGroup, 1);
+      const sr = new BinaryReader(skipped);
+      {
+        const [fieldNo, wireType] = sr.tag();
+        expect(fieldNo).toBe(33);
+        expect(wireType).toBe(WireType.Varint);
+        const bool = sr.bool();
+        expect(bool).toBe(true);
+      }
+      {
+        const [fieldNo, wireType] = sr.tag();
+        expect(fieldNo).toBe(1);
+        expect(wireType).toBe(WireType.EndGroup);
+        expect(sr.pos).toBe(sr.len);
+      }
+    });
+  });
+});

packages/protobuf/src/binary-encoding.ts

@@ -92,7 +92,7 @@ export interface IBinaryReader {
   /**
    * Skip one element on the wire and return the skipped data.
    */
-  skip(wireType: WireType): Uint8Array;
+  skip(wireType: WireType, fieldNo?: number): Uint8Array;
 
   /**
    * Read a `uint32` field, an unsigned 32 bit varint.
@@ -582,10 +582,12 @@ export class BinaryReader implements IBinaryReader {
   }
 
   /**
-   * Skip one element on the wire and return the skipped data.
-   * Supports WireType.StartGroup since v2.0.0-alpha.23.
+   * Skip one element and return the skipped data.
+   *
+   * When skipping StartGroup, provide the tags field number to check for
+   * matching field number in the EndGroup tag.
    */
-  skip(wireType: WireType): Uint8Array {
+  skip(wireType: WireType, fieldNo?: number): Uint8Array {
     let start = this.pos;
     switch (wireType) {
       case WireType.Varint:
@@ -607,10 +609,15 @@ export class BinaryReader implements IBinaryReader {
         this.pos += len;
         break;
       case WireType.StartGroup:
-        // TODO check for matching field numbers in StartGroup / EndGroup tags
-        let t: WireType;
-        while ((t = this.tag()[1]) !== WireType.EndGroup) {
-          this.skip(t);
+        for (;;) {
+          const [fn, wt] = this.tag();
+          if (wt === WireType.EndGroup) {
+            if (fieldNo !== undefined && fn !== fieldNo) {
+              throw new Error("invalid end group tag");
+            }
+            break;
+          }
+          this.skip(wt, fn);
         }
         break;
       default:

packages/protobuf/src/extension-accessor.ts

@@ -98,7 +98,7 @@ export function setExtension<E extends Message<E>, V>(
   const reader = readOpt.readerFactory(writer.finish());
   while (reader.pos < reader.len) {
     const [no, wireType] = reader.tag();
-    const data = reader.skip(wireType);
+    const data = reader.skip(wireType, no);
     message.getType().runtime.bin.onUnknownField(message, no, wireType, data);
   }
 }

packages/protobuf/src/private/binary-format.ts

@@ -105,12 +105,15 @@ export function makeBinaryFormat(): BinaryFormat {
       let fieldNo: number | undefined, wireType: WireType | undefined;
       while (reader.pos < end) {
         [fieldNo, wireType] = reader.tag();
-        if (wireType == WireType.EndGroup) {
+        if (
+          delimitedMessageEncoding === true &&
+          wireType == WireType.EndGroup
+        ) {
           break;
         }
         const field = type.fields.find(fieldNo);
         if (!field) {
-          const data = reader.skip(wireType);
+          const data = reader.skip(wireType, fieldNo);
           if (options.readUnknownFields) {
             this.onUnknownField(message, fieldNo, wireType, data);
           }