Refuse a file entry with no hash when loading a manifest (closes #194) #202

Merged
clawbot merged 1 commits from issue-194-refuse-entry-with-no-hash into next 2026-10-13 18:04:48 +02:00
5 changed files with 66 additions and 14 deletions
Showing only changes of commit e779032dcd - Show all commits
+2 -2
View File
@@ -76,10 +76,10 @@ After decompressing `innerMessage`, the result is a serialized `MFFile`
Each file entry contains: Each file entry contains:
| Field | Number | Type | Description | | Field | Number | Type | Description |
| ---------- | ------ | ------------------------- | ----------------------------------- | | ---------- | ------ | ------------------------- | ------------------------------------------------------------- |
| `path` | 1 | string | Relative file path (see Path Rules) | | `path` | 1 | string | Relative file path (see Path Rules) |
| `size` | 2 | int64 | File size in bytes | | `size` | 2 | int64 | File size in bytes |
| `hashes` | 3 | repeated `MFFileChecksum` | At least one hash required | | `hashes` | 3 | repeated `MFFileChecksum` | At least one hash required; readers reject an entry with none |
| `mimeType` | 301 | string (optional) | MIME type | | `mimeType` | 301 | string (optional) | MIME type |
| `mtime` | 302 | Timestamp (optional) | Modification time | | `mtime` | 302 | Timestamp (optional) | Modification time |
| `ctime` | 303 | Timestamp (optional) | Change time (inode metadata change) | | `ctime` | 303 | Timestamp (optional) | Change time (inode metadata change) |
+10 -2
View File
@@ -28,9 +28,15 @@ const (
scanProgressInterval = 100 scanProgressInterval = 100
) )
var (
// errEntryMissingMtime indicates a manifest entry that carries no // errEntryMissingMtime indicates a manifest entry that carries no
// modification time where one is required to carry it forward unchanged. // modification time where one is required to carry it forward unchanged.
var errEntryMissingMtime = errors.New("manifest entry has no mtime") 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. // FreshenStatus contains progress information for the freshen operation.
type FreshenStatus struct { type FreshenStatus struct {
@@ -644,6 +650,8 @@ func addFileToBuilder(
// Entries reach this path only when recordEntry classified them as // Entries reach this path only when recordEntry classified them as
// unchanged, which requires a recorded mtime, so an absent mtime here is // unchanged, which requires a recorded mtime, so an absent mtime here is
// an error rather than something to paper over with the Unix epoch. // 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 { func addExistingToBuilder(b *mfer.Builder, entry *mfer.MFFilePath) error {
mtime, ok := entryMtime(entry) mtime, ok := entryMtime(entry)
if !ok { if !ok {
@@ -651,7 +659,7 @@ func addExistingToBuilder(b *mfer.Builder, entry *mfer.MFFilePath) error {
} }
if len(entry.GetHashes()) == 0 { if len(entry.GetHashes()) == 0 {
return nil return fmt.Errorf("%w: %s", errEntryMissingHash, entry.GetPath())
} }
err := b.AddFileWithHash(mfer.RelFilePath(entry.GetPath()), err := b.AddFileWithHash(mfer.RelFilePath(entry.GetPath()),
+18
View File
@@ -471,6 +471,24 @@ func TestFreshenAddExistingRejectsMissingMtime(t *testing.T) {
assert.Contains(t, err.Error(), "file1.txt") 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 // TestFreshenAddExistingRejectsShortHash pins that an existing manifest
// entry whose hash the builder refuses is reported as a problem with the // entry whose hash the builder refuses is reported as a problem with the
// manifest, with the command that fixes it. // manifest, with the command that fixes it.
+7 -1
View File
@@ -27,6 +27,7 @@ var (
errUUIDMismatch = errors.New("outer and inner UUID mismatch") errUUIDMismatch = errors.New("outer and inner UUID mismatch")
errInvalidFileFormat = errors.New("invalid file format") errInvalidFileFormat = errors.New("invalid file format")
errInvalidManifestPath = errors.New("invalid file entry") errInvalidManifestPath = errors.New("invalid file entry")
errEntryMissingHash = errors.New("file entry has no hash")
errDecodedTooLarge = errors.New("too much memory needed") errDecodedTooLarge = errors.New("too much memory needed")
errSignerNotSigningKey = errors.New( errSignerNotSigningKey = errors.New(
"signer is not the fingerprint of the key that made the signature") "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 // untrusted .mf. Reject loudly on the first offender rather than
// dropping entries, which would let a hostile manifest hide files from a // 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 // 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())) seen := make(map[string]bool, len(m.pbInner.GetFiles()))
for _, f := range 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) return fmt.Errorf("%w: %w", errInvalidManifestPath, err)
} }
if len(f.GetHashes()) == 0 {
return fmt.Errorf("%w: %q", errEntryMissingHash, f.GetPath())
}
if seen[f.GetPath()] { if seen[f.GetPath()] {
return fmt.Errorf("%w %q", errDuplicatePath, f.GetPath()) return fmt.Errorf("%w %q", errDuplicatePath, f.GetPath())
} }
+20
View File
@@ -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 // 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 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 // 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.AppendTag(nil, 1, protowire.BytesType) // MFFilePath.path
entry = protowire.AppendString(entry, "a") entry = protowire.AppendString(entry, "a")
entry = protowire.AppendTag(entry, 3, protowire.BytesType) // MFFilePath.hashes
entry = protowire.AppendBytes(entry, nil)
entry = append(entry, unknown...) entry = append(entry, unknown...)
id := uuid.NewV4() id := uuid.NewV4()