V2: Check for matching field numbers in StartGroup / EndGroup tags (#810)

Author: timostamm

packages/protobuf-bench/README.md

@@ -10,5 +10,5 @@ server would usually do.
 
 | code generator      | bundle size             | minified               | compressed         |
 |---------------------|------------------------:|-----------------------:|-------------------:|
-| protobuf-es         | 126,666 b      | 65,334 b | 15,946 b |
+| protobuf-es         | 126,944 b      | 65,418 b | 15,948 b |
 | protobuf-javascript | 394,384 b  | 288,654 b | 45,122 b |

packages/protobuf-test/src/wire/binary-encoding.test.ts

@@ -13,9 +13,9 @@
 // limitations under the License.
 
 import { describe, expect, it } from "@jest/globals";
-import { BinaryWriter, WireType } from "@bufbuild/protobuf/wire";
-import { UserDesc } from "../gen/ts/extra/example_pb.js";
 import { fromBinary } from "@bufbuild/protobuf";
+import { BinaryWriter, BinaryReader, WireType } from "@bufbuild/protobuf/wire";
+import { UserDesc } from "../gen/ts/extra/example_pb.js";
 
 describe("BinaryWriter example", () => {
   it("should work as expected", () => {
@@ -32,3 +32,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/extensions.ts

@@ -80,7 +80,7 @@ export function setExtension<Desc extends DescExtension>(
   const reader = new BinaryReader(writer.finish());
   while (reader.pos < reader.len) {
     const [no, wireType] = reader.tag();
-    const data = reader.skip(wireType);
+    const data = reader.skip(wireType, no);
     ufs.push({ no, wireType, data });
   }
   message.$unknown = ufs;

packages/protobuf/src/from-binary.ts

@@ -108,12 +108,12 @@ function readMessage(
   const unknownFields = message.getUnknown() ?? [];
   while (reader.pos < end) {
     [fieldNo, wireType] = reader.tag();
-    if (wireType == WireType.EndGroup) {
+    if (delimited && wireType == WireType.EndGroup) {
       break;
     }
     const field = message.findNumber(fieldNo);
     if (!field) {
-      const data = reader.skip(wireType);
+      const data = reader.skip(wireType, fieldNo);
       if (options.readUnknownFields) {
         unknownFields.push({ no: fieldNo, wireType, data });
       }

packages/protobuf/src/wire/binary-encoding.ts

@@ -374,10 +374,12 @@ export class BinaryReader {
   }
 
   /**
-   * 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:
@@ -398,10 +400,15 @@ export class BinaryReader {
         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: