fix(ipns): reading records with raw []byte Value

CHANGELOG.md

@@ -22,6 +22,8 @@ The following emojis are used to highlight certain changes:
 
 ### Fixed
 
+- `ipns`: Improved interop with legacy clients by restoring support for `[]byte` CID in `Value` field. `Value()` will convert it to a valid `path.Path`. Empty `Value()` will produce `NoopPath` (`/ipfs/bafkqaaa`) to avoid breaking existing code that expects a valid record to always produce a valid content path. [#830](https://github.com/ipfs/boxo/pull/830)
+
 ### Security
 
 

ipns/record.go

@@ -12,6 +12,7 @@ import (
 	ipns_pb "github.com/ipfs/boxo/ipns/pb"
 	"github.com/ipfs/boxo/path"
 	"github.com/ipfs/boxo/util"
+	"github.com/ipfs/go-cid"
 	logging "github.com/ipfs/go-log/v2"
 	"github.com/ipld/go-ipld-prime"
 	"github.com/ipld/go-ipld-prime/codec/dagcbor"
@@ -31,6 +32,9 @@ type ValidityType int64
 // the only supported Validity type.
 const ValidityEOL ValidityType = 0
 
+// NoopValue is an identity CID that points at zero bytes.
+const NoopValue = "/ipfs/bafkqaaa"
+
 // Record represents an [IPNS Record].
 //
 // [IPNS Record]: https://specs.ipfs.tech/ipns/ipns-record/
@@ -81,18 +85,39 @@ func MarshalRecord(rec *Record) ([]byte, error) {
 	return proto.Marshal(rec.pb)
 }
 
-// Value returns the [path.Path] that is embedded in this IPNS Record. If the
-// path is invalid, an [ErrInvalidPath] is returned.
+// Value returns the [path.Path] that is embedded in this IPNS Record.
+// If the path is invalid, an error is returned.
+// If the value is a binary CID, it is converted to a [path.Path].
+// If the value is empty, a [NoopValue] is used instead.
 func (rec *Record) Value() (path.Path, error) {
 	value, err := rec.getBytesValue(cborValueKey)
 	if err != nil {
 		return nil, err
 	}
 
-	p, err := path.NewPath(string(value))
+	if len(value) == 0 {
+		// To maximize interop across implementations, turn empty value into zero-length identity CID.
+		// This is a convenience placeholder used when the value in record is empty or does
+		// not matter because IPNS is used for custom CBOR in the Data field.
+		value = []byte(NoopValue)
+	}
+
+	// parse as a string with content path
+	if value[0] == '/' {
+		p, err := path.NewPath(string(value))
+		if err != nil {
+			return nil, multierr.Combine(ErrInvalidPath, err)
+		}
+		// done, finish fast
+		return p, nil
+	}
+
+	// fallback: for legacy/optimization reasons, the value could be a valid CID in byte form
+	binaryCid, err := cid.Cast(value)
 	if err != nil {
 		return nil, multierr.Combine(ErrInvalidPath, err)
 	}
+	p := path.FromCid(binaryCid)
 
 	return p, nil
 }
@@ -232,6 +257,11 @@ func processOptions(opts ...Option) *options {
 // option [WithPublicKey]. In addition, records are, by default created with V1
 // compatibility.
 func NewRecord(sk ic.PrivKey, value path.Path, seq uint64, eol time.Time, ttl time.Duration, opts ...Option) (*Record, error) {
+	return newRecord(sk, []byte(value.String()), seq, eol, ttl, opts...)
+}
+
+// newRecord is a private version of NewRecord that allows arbitrary []byte values (used internally for testing)
+func newRecord(sk ic.PrivKey, value []byte, seq uint64, eol time.Time, ttl time.Duration, opts ...Option) (*Record, error) {
 	options := processOptions(opts...)
 
 	node, err := createNode(value, seq, eol, ttl)
@@ -260,7 +290,7 @@ func NewRecord(sk ic.PrivKey, value path.Path, seq uint64, eol time.Time, ttl ti
 	}
 
 	if options.v1Compatibility {
-		pb.Value = []byte(value.String())
+		pb.Value = value
 		typ := ipns_pb.IpnsRecord_EOL
 		pb.ValidityType = &typ
 		pb.Sequence = &seq
@@ -303,11 +333,11 @@ func NewRecord(sk ic.PrivKey, value path.Path, seq uint64, eol time.Time, ttl ti
 	}, nil
 }
 
-func createNode(value path.Path, seq uint64, eol time.Time, ttl time.Duration) (datamodel.Node, error) {
+func createNode(value []byte, seq uint64, eol time.Time, ttl time.Duration) (datamodel.Node, error) {
 	m := make(map[string]ipld.Node)
 	var keys []string
 
-	m[cborValueKey] = basicnode.NewBytes([]byte(value.String()))
+	m[cborValueKey] = basicnode.NewBytes(value)
 	keys = append(keys, cborValueKey)
 
 	m[cborValidityKey] = basicnode.NewBytes([]byte(util.FormatRFC3339(eol)))

ipns/record_test.go

@@ -9,6 +9,7 @@ import (
 	ipns_pb "github.com/ipfs/boxo/ipns/pb"
 	"github.com/ipfs/boxo/path"
 	"github.com/ipfs/boxo/util"
+	"github.com/ipfs/go-cid"
 	"github.com/ipld/go-ipld-prime/codec/dagcbor"
 	basicnode "github.com/ipld/go-ipld-prime/node/basic"
 	ic "github.com/libp2p/go-libp2p/core/crypto"
@@ -45,6 +46,13 @@ func mustNewRecord(t *testing.T, sk ic.PrivKey, value path.Path, seq uint64, eol
 	return rec
 }
 
+func mustNewRawRecord(t *testing.T, sk ic.PrivKey, value []byte, seq uint64, eol time.Time, ttl time.Duration, opts ...Option) *Record {
+	rec, err := newRecord(sk, value, seq, eol, ttl, opts...)
+	require.NoError(t, err)
+	require.NoError(t, Validate(rec, sk.GetPublic()))
+	return rec
+}
+
 func mustMarshal(t *testing.T, entry *Record) []byte {
 	data, err := MarshalRecord(entry)
 	require.NoError(t, err)
@@ -259,6 +267,12 @@ func TestCBORDataSerialization(t *testing.T) {
 func TestUnmarshal(t *testing.T) {
 	t.Parallel()
 
+	sk, _, _ := mustKeyPair(t, ic.Ed25519)
+
+	seq := uint64(0)
+	eol := time.Now().Add(time.Hour)
+	ttl := time.Minute * 10
+
 	t.Run("Errors on invalid bytes", func(t *testing.T) {
 		_, err := UnmarshalRecord([]byte("blah blah blah"))
 		require.ErrorIs(t, err, ErrInvalidRecord)
@@ -287,4 +301,65 @@ func TestUnmarshal(t *testing.T) {
 		_, err = UnmarshalRecord(data)
 		require.ErrorIs(t, err, ErrInvalidRecord)
 	})
+
+	t.Run("Errors on bad binary Value() unable to represent as a Path (interop)", func(t *testing.T) {
+		t.Parallel()
+
+		// create record with non-empty byte value that is not []byte CID nor a string with /ipfs/cid content path
+		rawByteValue := []byte{0x4A, 0x1B, 0x3C, 0x8D, 0x2E}
+		rec := mustNewRawRecord(t, sk, rawByteValue, seq, eol, ttl)
+
+		// confirm raw record has same bytes
+		require.Equal(t, rawByteValue, rec.pb.GetValue())
+		cborValue, err := rec.getBytesValue(cborValueKey)
+		require.NoError(t, err)
+		require.Equal(t, rawByteValue, cborValue)
+
+		// confirm rec.Value() returns error
+		recPath, err := rec.Value()
+		require.ErrorIs(t, err, ErrInvalidPath)
+		require.Empty(t, recPath)
+	})
+
+	t.Run("Reads record with empty Value() as zero-length identity CID (interop)", func(t *testing.T) {
+		t.Parallel()
+
+		// create record with empty byte value
+		rawByteValue := []byte{}
+		rec := mustNewRawRecord(t, sk, rawByteValue, seq, eol, ttl)
+
+		// confirm raw record has empty (0-length []byte) Value
+		require.Empty(t, rec.pb.GetValue())
+		cborValue, err := rec.getBytesValue(cborValueKey)
+		require.NoError(t, err)
+		require.Empty(t, cborValue)
+
+		// confirm rec.Value() returns NooPath for interop
+		recPath, err := rec.Value()
+		require.NoError(t, err)
+		require.Equal(t, NoopValue, recPath.String())
+	})
+
+	t.Run("Reads record with a []byte CID as Value() (interop)", func(t *testing.T) {
+		t.Parallel()
+
+		// create record with a valid CID in binary form
+		// instead of /ipfs/cid string
+		// (we need to support this because this was allowed since <2018)
+		testCid := "bafkqablimvwgy3y"
+		rawCidValue := cid.MustParse(testCid).Bytes()
+
+		rec := mustNewRawRecord(t, sk, rawCidValue, seq, eol, ttl)
+
+		// confirm raw record has same bytes
+		require.Equal(t, rawCidValue, rec.pb.GetValue())
+		cborValue, err := rec.getBytesValue(cborValueKey)
+		require.NoError(t, err)
+		require.Equal(t, rawCidValue, cborValue)
+
+		// confirm rec.Value() returns path for raw CID
+		recPath, err := rec.Value()
+		require.NoError(t, err)
+		require.Equal(t, "/ipfs/"+testCid, recPath.String())
+	})
 }