diff --git a/internal/cli/freshen.go b/internal/cli/freshen.go index 72b9f7d..e50d803 100644 --- a/internal/cli/freshen.go +++ b/internal/cli/freshen.go @@ -629,7 +629,15 @@ func addExistingToBuilder(b *mfer.Builder, entry *mfer.MFFilePath) error { return nil } - return b.AddFileWithHash(mfer.RelFilePath(entry.GetPath()), + err := b.AddFileWithHash(mfer.RelFilePath(entry.GetPath()), mfer.FileSize(entry.GetSize()), mfer.ModTime(mtime), entry.GetHashes()[0].GetMultiHash()) + if err != nil { + return fmt.Errorf( + "manifest entry %s: %w (regenerate the manifest with mfer generate)", + entry.GetPath(), err, + ) + } + + return nil } diff --git a/internal/cli/freshen_test.go b/internal/cli/freshen_test.go index cd83e15..eeb9ee3 100644 --- a/internal/cli/freshen_test.go +++ b/internal/cli/freshen_test.go @@ -4,11 +4,13 @@ package cli import ( "bytes" "context" + "crypto/sha256" "os" "path/filepath" "testing" "time" + "github.com/multiformats/go-multihash" "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -234,21 +236,50 @@ func TestFreshenRecordEntryMtimePresence(t *testing.T) { func TestFreshenAddExistingRejectsMissingMtime(t *testing.T) { t.Parallel() + hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256) + require.NoError(t, err) + b := mfer.NewBuilder() entry := &mfer.MFFilePath{ Path: "file1.txt", Size: 8, Mtime: nil, Hashes: []*mfer.MFFileChecksum{ - {MultiHash: []byte{0x12, 0x20}}, + {MultiHash: hash}, }, } - err := addExistingToBuilder(b, entry) + err = addExistingToBuilder(b, entry) require.ErrorIs(t, err, errEntryMissingMtime) assert.Contains(t, err.Error(), "file1.txt") } +// TestFreshenAddExistingRejectsShortHash pins that an existing manifest +// entry whose hash the builder refuses is reported as a problem with the +// manifest, with the command that fixes it. +func TestFreshenAddExistingRejectsShortHash(t *testing.T) { + t.Parallel() + + sha1Hash, err := multihash.Encode(make([]byte, 20), multihash.SHA1) + require.NoError(t, err) + + b := mfer.NewBuilder() + entry := &mfer.MFFilePath{ + Path: "old.txt", + Size: 8, + Mtime: &mfer.Timestamp{Seconds: 1_700_000_000}, + Hashes: []*mfer.MFFileChecksum{ + {MultiHash: sha1Hash}, + }, + } + + err = addExistingToBuilder(b, entry) + require.Error(t, err) + assert.Contains(t, err.Error(), "manifest entry old.txt") + assert.Contains(t, err.Error(), "mfer generate") + assert.Zero(t, b.FileCount()) +} + // TestEntryMtime pins the presence semantics the callers depend on. func TestEntryMtime(t *testing.T) { t.Parallel() diff --git a/mfer/builder.go b/mfer/builder.go index c2d7856..610c750 100644 --- a/mfer/builder.go +++ b/mfer/builder.go @@ -35,7 +35,8 @@ var ( errPathDotDot = errors.New("contains '..' segment") errSizeMismatch = errors.New("size mismatch") errNegativeSize = errors.New("size cannot be negative") - errEmptyHash = errors.New("hash cannot be nil or empty") + errHashNotMultihash = errors.New("hash is not a valid multihash") + errHashTooShort = errors.New("hash digest is too short") ) // ValidatePath checks that a file path conforms to manifest path invariants: @@ -228,7 +229,8 @@ func (b *Builder) FileCount() int { // AddFileWithHash adds a file entry with a pre-computed hash. // This is useful when the hash is already known (e.g., from an existing manifest). -// Returns an error if path is empty, size is negative, or hash is nil/empty. +// Returns an error if path is invalid, size is negative, or hash is not a +// multihash with a digest of at least 32 bytes, as long as SHA-256's. func (b *Builder) AddFileWithHash( path RelFilePath, size FileSize, @@ -244,8 +246,19 @@ func (b *Builder) AddFileWithHash( return errNegativeSize } - if len(hash) == 0 { - return errEmptyHash + decoded, err := multihash.Decode(hash) + if err != nil { + return fmt.Errorf("%w: %w", errHashNotMultihash, err) + } + + // The reader's limit on decoding cost (maxDecodedGrowth) assumes every + // hash is at least as long as a SHA-256 multihash, so a manifest of + // shorter ones could fail to load. + if len(decoded.Digest) < sha256.Size { + return fmt.Errorf( + "%w: %d bytes, at least %d needed", + errHashTooShort, len(decoded.Digest), sha256.Size, + ) } entry := &MFFilePath{ diff --git a/mfer/builder_test.go b/mfer/builder_test.go index 4b720c4..275c747 100644 --- a/mfer/builder_test.go +++ b/mfer/builder_test.go @@ -4,12 +4,14 @@ package mfer import ( "bytes" "context" + "crypto/sha256" "fmt" "path/filepath" "strings" "testing" "time" + "github.com/multiformats/go-multihash" "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -44,9 +46,10 @@ func TestBuilderAddFileWithHash(t *testing.T) { t.Parallel() b := NewBuilder() - hash := make([]byte, 34) // SHA256 multihash is 34 bytes + hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256) + require.NoError(t, err) - err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), hash) + err = b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), hash) require.NoError(t, err) assert.Equal(t, 1, b.FileCount()) } @@ -54,12 +57,14 @@ func TestBuilderAddFileWithHash(t *testing.T) { func TestBuilderAddFileWithHashValidation(t *testing.T) { t.Parallel() + sha256Hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256) + require.NoError(t, err) + t.Run("empty path", func(t *testing.T) { t.Parallel() b := NewBuilder() - hash := make([]byte, 34) - err := b.AddFileWithHash("", 100, ModTime(time.Now()), hash) + err := b.AddFileWithHash("", 100, ModTime(time.Now()), sha256Hash) require.Error(t, err) assert.Contains(t, err.Error(), "path") }) @@ -68,41 +73,58 @@ func TestBuilderAddFileWithHashValidation(t *testing.T) { t.Parallel() b := NewBuilder() - hash := make([]byte, 34) - err := b.AddFileWithHash("test.txt", -1, ModTime(time.Now()), hash) + err := b.AddFileWithHash("test.txt", -1, ModTime(time.Now()), sha256Hash) require.Error(t, err) assert.Contains(t, err.Error(), "size") }) - t.Run("nil hash", func(t *testing.T) { - t.Parallel() - - b := NewBuilder() - err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), nil) - require.Error(t, err) - assert.Contains(t, err.Error(), "hash") - }) - - t.Run("empty hash", func(t *testing.T) { - t.Parallel() - - b := NewBuilder() - err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), []byte{}) - require.Error(t, err) - assert.Contains(t, err.Error(), "hash") - }) - t.Run("valid inputs", func(t *testing.T) { t.Parallel() b := NewBuilder() - hash := make([]byte, 34) - err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), hash) + err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), sha256Hash) require.NoError(t, err) assert.Equal(t, 1, b.FileCount()) }) } +func TestBuilderAddFileWithHashRejectsBadHashes(t *testing.T) { + t.Parallel() + + sha1Hash, err := multihash.Encode(make([]byte, 20), multihash.SHA1) + require.NoError(t, err) + + truncatedHash, err := multihash.Encode(make([]byte, sha256.Size-1), multihash.SHA2_256) + require.NoError(t, err) + + tests := []struct { + name string + hash Multihash + want error + }{ + {"nil hash", nil, errHashNotMultihash}, + {"empty hash", []byte{}, errHashNotMultihash}, + {"one-byte hash", []byte{0x12}, errHashNotMultihash}, + // A SHA-256 code and 32-byte length, then only two bytes of digest. + {"malformed multihash", []byte{0x12, 0x20, 0x01, 0x02}, errHashNotMultihash}, + // A valid multihash, but its 20-byte SHA-1 digest is too short. + {"SHA-1 multihash", sha1Hash, errHashTooShort}, + // A valid multihash whose 31-byte digest is one byte short. + {"31-byte digest", truncatedHash, errHashTooShort}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + b := NewBuilder() + err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), tt.hash) + require.ErrorIs(t, err, tt.want) + assert.Equal(t, 0, b.FileCount()) + }) + } +} + func TestBuilderBuild(t *testing.T) { t.Parallel() @@ -358,7 +380,8 @@ func TestBuilderBuildRoundTrip(t *testing.T) { func TestBuilderBuildRoundTripLargeManifest(t *testing.T) { t.Parallel() - hash := make([]byte, 34) // multihash: 2-byte prefix + 32-byte SHA-256 + hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256) + require.NoError(t, err) b := NewBuilder() diff --git a/mfer/constants.go b/mfer/constants.go index 519d21b..6d8267e 100644 --- a/mfer/constants.go +++ b/mfer/constants.go @@ -36,7 +36,8 @@ const ( decodedMIMETypeSize = 16 // Each file entry mfer writes holds a path of at least one byte, a - // 34-byte SHA-256 multihash and a modification time: at least 47 bytes, + // multihash at least as long as SHA-256's 34 bytes (AddFileWithHash + // refuses shorter ones) and a modification time: at least 47 bytes, // counted at 336. So its manifests add up to at most about 7.15 times // their size, and this limit is about 12% above that. maxDecodedGrowth = 8 diff --git a/mfer/deserialize_path_test.go b/mfer/deserialize_path_test.go index 8b7286b..7ac7221 100644 --- a/mfer/deserialize_path_test.go +++ b/mfer/deserialize_path_test.go @@ -13,6 +13,7 @@ import ( "github.com/google/uuid" "github.com/klauspost/compress/zstd" + "github.com/multiformats/go-multihash" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/protobuf/encoding/protowire" @@ -205,7 +206,8 @@ func TestDeserializeDropsUnknownFields(t *testing.T) { func TestDeserializeLoadsDensestManifest(t *testing.T) { t.Parallel() - hash := make([]byte, 34) // multihash: 2-byte prefix + 32-byte SHA-256 + hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256) + require.NoError(t, err) b := NewBuilder() b.SetIncludeTimestamps(true) @@ -227,7 +229,8 @@ func TestDeserializeLoadsDensestManifest(t *testing.T) { func TestDeserializeValidManifestRoundTrips(t *testing.T) { t.Parallel() - hash := make([]byte, 34) // multihash: 2-byte prefix + 32-byte SHA-256 + hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256) + require.NoError(t, err) b := NewBuilder() require.NoError(t, b.AddFileWithHash("dir/file.txt", 123, ModTime{}, hash))