allow decoding PBNode fields in any order

We recently updated the spec to say that decoders "should" allow
decoding a PBNode in either order of its fields. That is because some
IPFS data out in the wild is already encoded in the non-canonical
format, which is what Protobuf uses as a default, too.

This change makes the decoder here comply with the spec change, adding a
test with the encoded block that caused this entire spec change in the
first place: go-ipfs/test/sharness/t0110-gateway-data/foofoo.block.

The change to the decoder is slightly subtle, because the decoder used
to leverage the knowledge that Links must come before Data to start the
Links list early, and finish it as soon as Data arrives.

Since that order is now unknown, we must have some extra logic to
support either order. We also need special code to make sure Links is
always set, just like before.

We also add a test to double check that Data between Links is rejected.

basics_test.go

@@ -280,3 +280,43 @@ func TestNodeWithNamedLinks(t *testing.T) {
 		"12390a221220b4397c02da5513563d33eef894bf68f2ccdf1bdfc14a976956ab3d1c72f735a0120e617564696f5f6f6e6c792e6d346118cda88f0b12310a221220025c13fcd1a885df444f64a4a82a26aea867b1148c68cb671e83589f971149321208636861742e74787418e40712340a2212205d44a305b9b328ab80451d0daa72a12a7bf2763c5f8bbe327597a31ee40d1e48120c706c61796261636b2e6d3375187412360a2212202539ed6e85f2a6f9097db9d76cffd49bf3042eb2e3e8e9af4a3ce842d49dea22120a7a6f6f6d5f302e6d70341897fb8592010a020801",
 		expected)
 }
+
+func TestNodeAlternativeOrder(t *testing.T) {
+	// This input has Data before Links.
+	input, err := hex.DecodeString("0a040802180612240a221220cf92fdefcdc34cac009c8b05eb662be0618db9de55ecd42785e9ec6712f8df6512240a221220cf92fdefcdc34cac009c8b05eb662be0618db9de55ecd42785e9ec6712f8df65")
+	if err != nil {
+		t.Fatal(err)
+	}
+	nb := Type.PBNode.NewBuilder()
+	if err := DecodeBytes(nb, input); err != nil {
+		t.Fatal(err)
+	}
+	node := nb.Build()
+
+	// We don't really care what the node looks like, or what the exact
+	// re-encode bytes are. But we do care that it's not exactly the same,
+	// but still the same length.
+	output, err := AppendEncode(nil, node)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if bytes.Equal(input, output) {
+		t.Fatal("re-encoding should use canonical PBNode field order")
+	}
+	if len(input) != len(output) {
+		t.Fatal("re-encoding should have the same number of bytes")
+	}
+}
+
+func TestNodeWithDataBetweenLinks(t *testing.T) {
+	// This input has one Links element, then Data, then another Links element.
+	// As per the spec, this should fail as a duplicate Links field.
+	input, err := hex.DecodeString("12240a221220cf92fdefcdc34cac009c8b05eb662be0618db9de55ecd42785e9ec6712f8df650a040802180612240a221220cf92fdefcdc34cac009c8b05eb662be0618db9de55ecd42785e9ec6712f8df65")
+	if err != nil {
+		t.Fatal(err)
+	}
+	nb := Type.PBNode.NewBuilder()
+	if err := DecodeBytes(nb, input); err == nil {
+		t.Fatal("expected Decode to fail")
+	}
+}

unmarshal.go

@@ -45,16 +45,10 @@ func DecodeBytes(na ipld.NodeAssembler, src []byte) error {
 	if err != nil {
 		return err
 	}
-	// always make "Links", even if we don't use it
-	if err := ma.AssembleKey().AssignString("Links"); err != nil {
-		return err
-	}
-	links, err := ma.AssembleValue().BeginList(0)
-	if err != nil {
-		return err
-	}
+	var links ipld.ListAssembler
 
 	haveData := false
+	haveLinks := false
 	for {
 		if len(remaining) == 0 {
 			break
@@ -70,6 +64,10 @@ func DecodeBytes(na ipld.NodeAssembler, src []byte) error {
 			return fmt.Errorf("protobuf: (PBNode) invalid wireType, expected 2, got %d", wireType)
 		}
 
+		// Note that we allow Data and Links to come in either order,
+		// since the spec defines that decoding "should" accept either form.
+		// This is for backwards compatibility with older IPFS data.
+
 		switch fieldNum {
 		case 1:
 			if haveData {
@@ -82,12 +80,15 @@ func DecodeBytes(na ipld.NodeAssembler, src []byte) error {
 			}
 			remaining = remaining[n:]
 
-			// Data must come after Links, so it's safe to close this here even if we
-			// didn't use it
+			if links != nil {
+				// Links came before Data.
+				// Finish them before we start Data.
 				if err := links.Finish(); err != nil {
 					return err
 				}
 				links = nil
+			}
+
 			if err := ma.AssembleKey().AssignString("Data"); err != nil {
 				return err
 			}
@@ -97,16 +98,27 @@ func DecodeBytes(na ipld.NodeAssembler, src []byte) error {
 			haveData = true
 
 		case 2:
-			if haveData {
-				return fmt.Errorf("protobuf: (PBNode) invalid order, found Data before Links content")
-			}
-
 			bytesLen, n := protowire.ConsumeVarint(remaining)
 			if n < 0 {
 				return protowire.ParseError(n)
 			}
 			remaining = remaining[n:]
 
+			if links == nil {
+				if haveLinks {
+					return fmt.Errorf("protobuf: (PBNode) duplicate Links section")
+				}
+
+				// The repeated "Links" part begins.
+				if err := ma.AssembleKey().AssignString("Links"); err != nil {
+					return err
+				}
+				links, err = ma.AssembleValue().BeginList(0)
+				if err != nil {
+					return err
+				}
+			}
+
 			curLink, err := links.AssembleValue().BeginMap(3)
 			if err != nil {
 				return err
@@ -118,6 +130,7 @@ func DecodeBytes(na ipld.NodeAssembler, src []byte) error {
 			if err := curLink.Finish(); err != nil {
 				return err
 			}
+			haveLinks = true
 
 		default:
 			return fmt.Errorf("protobuf: (PBNode) invalid fieldNumber, expected 1 or 2, got %d", fieldNum)
@@ -125,6 +138,21 @@ func DecodeBytes(na ipld.NodeAssembler, src []byte) error {
 	}
 
 	if links != nil {
+		// We had some links at the end, so finish them.
+		if err := links.Finish(); err != nil {
+			return err
+		}
+
+	} else if !haveLinks {
+		// We didn't have any links.
+		// Since we always want a Links field, add one here.
+		if err := ma.AssembleKey().AssignString("Links"); err != nil {
+			return err
+		}
+		links, err := ma.AssembleValue().BeginList(0)
+		if err != nil {
+			return err
+		}
 		if err := links.Finish(); err != nil {
 			return err
 		}