diff --git a/docs/FORMAT.md b/docs/FORMAT.md index d792527..20062a5 100644 --- a/docs/FORMAT.md +++ b/docs/FORMAT.md @@ -75,15 +75,15 @@ After decompressing `innerMessage`, the result is a serialized `MFFile` Each file entry contains: -| Field | Number | Type | Description | -| ---------- | ------ | ------------------------- | ----------------------------------- | -| `path` | 1 | string | Relative file path (see Path Rules) | -| `size` | 2 | int64 | File size in bytes | -| `hashes` | 3 | repeated `MFFileChecksum` | At least one hash required | -| `mimeType` | 301 | string (optional) | MIME type | -| `mtime` | 302 | Timestamp (optional) | Modification time | -| `ctime` | 303 | Timestamp (optional) | Change time (inode metadata change) | -| `mode` | 304 | uint32 | Permission bits (see File Mode) | +| Field | Number | Type | Description | +| ---------- | ------ | ------------------------- | ------------------------------------------------------------- | +| `path` | 1 | string | Relative file path (see Path Rules) | +| `size` | 2 | int64 | File size in bytes | +| `hashes` | 3 | repeated `MFFileChecksum` | At least one hash required; readers reject an entry with none | +| `mimeType` | 301 | string (optional) | MIME type | +| `mtime` | 302 | Timestamp (optional) | Modification time | +| `ctime` | 303 | Timestamp (optional) | Change time (inode metadata change) | +| `mode` | 304 | uint32 | Permission bits (see File Mode) | ## File Mode diff --git a/internal/cli/freshen.go b/internal/cli/freshen.go index 4a9b246..cb37312 100644 --- a/internal/cli/freshen.go +++ b/internal/cli/freshen.go @@ -28,9 +28,15 @@ const ( scanProgressInterval = 100 ) -// errEntryMissingMtime indicates a manifest entry that carries no -// modification time where one is required to carry it forward unchanged. -var errEntryMissingMtime = errors.New("manifest entry has no mtime") +var ( + // errEntryMissingMtime indicates a manifest entry that carries no + // modification time where one is required to carry it forward unchanged. + errEntryMissingMtime = errors.New("manifest entry has no mtime") + + // errEntryMissingHash indicates a manifest entry that carries no hash + // to carry forward. + errEntryMissingHash = errors.New("manifest entry has no hash") +) // FreshenStatus contains progress information for the freshen operation. type FreshenStatus struct { @@ -644,6 +650,8 @@ func addFileToBuilder( // Entries reach this path only when recordEntry classified them as // unchanged, which requires a recorded mtime, so an absent mtime here is // an error rather than something to paper over with the Unix epoch. +// Loading refuses an entry with no hash; one that got here anyway is an +// error too, not a file to leave out of the manifest. func addExistingToBuilder(b *mfer.Builder, entry *mfer.MFFilePath) error { mtime, ok := entryMtime(entry) if !ok { @@ -651,7 +659,7 @@ func addExistingToBuilder(b *mfer.Builder, entry *mfer.MFFilePath) error { } if len(entry.GetHashes()) == 0 { - return nil + return fmt.Errorf("%w: %s", errEntryMissingHash, entry.GetPath()) } err := b.AddFileWithHash(mfer.RelFilePath(entry.GetPath()), diff --git a/internal/cli/freshen_test.go b/internal/cli/freshen_test.go index d6fbfe2..bde7f52 100644 --- a/internal/cli/freshen_test.go +++ b/internal/cli/freshen_test.go @@ -471,6 +471,24 @@ func TestFreshenAddExistingRejectsMissingMtime(t *testing.T) { assert.Contains(t, err.Error(), "file1.txt") } +// TestFreshenAddExistingRejectsMissingHash pins that an entry with no hash +// is an error rather than a file left out of the rebuilt manifest. +func TestFreshenAddExistingRejectsMissingHash(t *testing.T) { + t.Parallel() + + b := mfer.NewBuilder() + entry := &mfer.MFFilePath{ + Path: "data.bin", + Size: 8, + Mtime: &mfer.Timestamp{Seconds: 1_700_000_000}, + } + + err := addExistingToBuilder(b, entry) + require.ErrorIs(t, err, errEntryMissingHash) + assert.Contains(t, err.Error(), "data.bin") + assert.Zero(t, b.FileCount()) +} + // 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. diff --git a/mfer/deserialize.go b/mfer/deserialize.go index 0d3b95c..6a13cdf 100644 --- a/mfer/deserialize.go +++ b/mfer/deserialize.go @@ -27,6 +27,7 @@ var ( errUUIDMismatch = errors.New("outer and inner UUID mismatch") errInvalidFileFormat = errors.New("invalid file format") errInvalidManifestPath = errors.New("invalid file entry") + errEntryMissingHash = errors.New("file entry has no hash") errDecodedTooLarge = errors.New("too much memory needed") errSignerNotSigningKey = errors.New( "signer is not the fingerprint of the key that made the signature") @@ -293,7 +294,8 @@ func (m *manifest) deserializeInner() error { // untrusted .mf. Reject loudly on the first offender rather than // dropping entries, which would let a hostile manifest hide files from a // check. A path listed twice is refused too: check would check the one - // file against both entries. + // file against both entries. So is an entry with no hash: there is + // nothing to check the file against. seen := make(map[string]bool, len(m.pbInner.GetFiles())) for _, f := range m.pbInner.GetFiles() { @@ -302,6 +304,10 @@ func (m *manifest) deserializeInner() error { return fmt.Errorf("%w: %w", errInvalidManifestPath, err) } + if len(f.GetHashes()) == 0 { + return fmt.Errorf("%w: %q", errEntryMissingHash, f.GetPath()) + } + if seen[f.GetPath()] { return fmt.Errorf("%w %q", errDuplicatePath, f.GetPath()) } diff --git a/mfer/deserialize_path_test.go b/mfer/deserialize_path_test.go index a95ebea..b722f15 100644 --- a/mfer/deserialize_path_test.go +++ b/mfer/deserialize_path_test.go @@ -165,6 +165,24 @@ func TestDeserializeRefusesPathListedTwice(t *testing.T) { } } +// A manifest holding a file entry with no hash is refused as it is loaded, +// naming the path: there is nothing to check the file against. +func TestDeserializeRefusesEntryWithNoHash(t *testing.T) { + t.Parallel() + + id := uuid.NewV4() + inner, err := proto.Marshal(&MFFile{ + Version: MFFile_VERSION_ONE, + Files: []*MFFilePath{{Path: "x", Size: 1}}, + Uuid: id[:], + }) + require.NoError(t, err) + + _, err = NewManifestFromReader(bytes.NewReader(wrapInner(t, id, inner))) + require.ErrorIs(t, err, errEntryMissingHash) + require.EqualError(t, err, `file entry has no hash: "x"`) +} + // Entries of a path, an empty hash, an empty MIME type and empty modification // and change times are counted at 432 bytes each (176 + 112 + 16 + 64 + 64) // and take 16 bytes plus the path to encode. A 37-character path makes that @@ -229,6 +247,8 @@ func TestDeserializeDropsUnknownFields(t *testing.T) { entry := protowire.AppendTag(nil, 1, protowire.BytesType) // MFFilePath.path entry = protowire.AppendString(entry, "a") + entry = protowire.AppendTag(entry, 3, protowire.BytesType) // MFFilePath.hashes + entry = protowire.AppendBytes(entry, nil) entry = append(entry, unknown...) id := uuid.NewV4()