From 61238e407183de883c2d1eb2a2b2222f5bb2d639 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 13 Oct 2026 18:04:47 +0200 Subject: [PATCH] Refuse a file entry with no hash when loading a manifest (closes #194) Loading accepted a file entry whose hashes list was empty. check and fetch then failed the file as a hash mismatch, and freshen left it out of the rewritten manifest without a word. NewManifestFromReader now refuses such an entry, naming its path, after checking the path itself. freshen's addExistingToBuilder returns an error for an entry with no hash instead of skipping it. The file entry table in docs/FORMAT.md says readers reject an entry with none. Model: opus-5-5 --- docs/FORMAT.md | 18 +++++++++--------- internal/cli/freshen.go | 16 ++++++++++++---- internal/cli/freshen_test.go | 18 ++++++++++++++++++ mfer/deserialize.go | 8 +++++++- mfer/deserialize_path_test.go | 20 ++++++++++++++++++++ 5 files changed, 66 insertions(+), 14 deletions(-) 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()