diff --git a/docs/FORMAT.md b/docs/FORMAT.md index 3d7e3d5..e693063 100644 --- a/docs/FORMAT.md +++ b/docs/FORMAT.md @@ -106,9 +106,12 @@ All `path` values must satisfy these invariants: - **No parent traversal**: no `..` path segments - **No empty segments**: no `//` sequences - **No trailing slash**: paths refer to files, not directories +- **Listed once**: each path appears at most once in a manifest, compared byte + for byte, so `A.txt` and `a.txt` are two paths Implementations must validate these invariants when reading and writing -manifests. Paths that violate these rules must be rejected. +manifests. Paths that violate these rules must be rejected, and a reader must +reject a manifest that lists a path more than once. ## Hash Format (`MFFileChecksum`) diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index aa5623a..f0a5854 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -354,6 +354,44 @@ func TestGenerateCommand(t *testing.T) { assert.True(t, exists) } +// TestGenerateRefusesTwoFilesAtOnePath runs gen on arguments whose files +// would share a path in the manifest: two directories that each hold a.txt, +// and one directory given twice. gen must fail while it lists the files, +// before it hashes any, naming the path, and write no manifest. +func TestGenerateRefusesTwoFilesAtOnePath(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + first, second string + }{ + {"two directories", "/one", "/two"}, + {"one directory twice", "/one", "/one"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + require.NoError(t, fs.MkdirAll("/one", 0o755)) + require.NoError(t, fs.MkdirAll("/two", 0o755)) + writeTestFile(t, fs, "/one/a.txt", "first") + writeTestFile(t, fs, "/two/a.txt", "second") + + opts := testOpts([]string{ + testApp, cmdGenerate, "-q", "-o", testOutput, tc.first, tc.second, + }, fs) + assert.Equal(t, 1, runCLI(opts)) + assert.Contains(t, testStderr(t, opts), + `generate: failed to enumerate paths: duplicate path "a.txt": `+ + tc.first+"/a.txt and "+tc.second+"/a.txt") + + exists, err := afero.Exists(fs, testOutput) + require.NoError(t, err) + assert.False(t, exists) + }) + } +} + // TestGenerateSeededManifestBytes pins the exact bytes `gen --seed` writes // for a fixed tree, so that a Go or dependency update that changes what // mfer writes fails here. testdata/seeded.mf was written by an mfer built diff --git a/mfer/builder.go b/mfer/builder.go index 827d430..ea99780 100644 --- a/mfer/builder.go +++ b/mfer/builder.go @@ -38,6 +38,7 @@ var ( errNegativeSize = errors.New("size cannot be negative") errHashNotMultihash = errors.New("hash is not a valid multihash") errHashTooShort = errors.New("hash digest is too short") + errDuplicatePath = errors.New("duplicate path") ) // ValidatePath checks that a file path conforms to manifest path invariants: @@ -115,6 +116,7 @@ type FileHashProgress struct { type Builder struct { mu sync.Mutex files []*MFFilePath + paths map[string]bool // the path of each entry in files createdAt time.Time includeTimestamps bool signingOptions *SigningOptions @@ -125,6 +127,7 @@ type Builder struct { func NewBuilder() *Builder { return &Builder{ files: make([]*MFFilePath, 0), + paths: make(map[string]bool), createdAt: time.Now(), } } @@ -138,6 +141,7 @@ func (b *Builder) SetSeed(seed string) { } // AddFile reads file content from reader, computes hashes, and adds to manifest. +// A path already added is refused once the file is read. // Only mode's permission bits (mode.Perm()) are recorded; 0 records none. // Progress updates are sent to the progress channel (if non-nil) without blocking. // Returns the number of bytes read. @@ -204,11 +208,23 @@ func (b *Builder) AddFile( Mode: uint32(mode.Perm()), } - b.mu.Lock() - b.files = append(b.files, entry) - b.mu.Unlock() + return totalRead, b.addEntry(entry) +} - return totalRead, nil +// addEntry adds entry to the manifest unless an entry with its path is +// already there. +func (b *Builder) addEntry(entry *MFFilePath) error { + b.mu.Lock() + defer b.mu.Unlock() + + if b.paths[entry.GetPath()] { + return fmt.Errorf("%w %q", errDuplicatePath, entry.GetPath()) + } + + b.paths[entry.GetPath()] = true + b.files = append(b.files, entry) + + return nil } // sendFileHashProgress sends a progress update without blocking. @@ -234,8 +250,9 @@ 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). // Only mode's permission bits (mode.Perm()) are recorded; 0 records none. -// 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. +// Returns an error if path is invalid or already added, 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, @@ -277,11 +294,7 @@ func (b *Builder) AddFileWithHash( Mode: uint32(mode.Perm()), } - b.mu.Lock() - b.files = append(b.files, entry) - b.mu.Unlock() - - return nil + return b.addEntry(entry) } // SetIncludeTimestamps controls whether the manifest includes a createdAt timestamp. diff --git a/mfer/builder_test.go b/mfer/builder_test.go index b8c6073..39dd96d 100644 --- a/mfer/builder_test.go +++ b/mfer/builder_test.go @@ -125,6 +125,32 @@ func TestBuilderAddFileWithHashRejectsBadHashes(t *testing.T) { } } +// TestBuilderRefusesPathAlreadyAdded adds a path, then adds it again with +// AddFile and with AddFileWithHash. Each must refuse it, naming it, and +// keep the one entry already added. +func TestBuilderRefusesPathAlreadyAdded(t *testing.T) { + t.Parallel() + + hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256) + require.NoError(t, err) + + b := NewBuilder() + require.NoError(t, b.AddFileWithHash("dir/a.txt", 4, ModTime{}, 0, hash)) + + content := []byte("data") + _, err = b.AddFile( + "dir/a.txt", FileSize(len(content)), ModTime{}, 0, bytes.NewReader(content), nil, + ) + require.ErrorIs(t, err, errDuplicatePath) + assert.EqualError(t, err, `duplicate path "dir/a.txt"`) + + err = b.AddFileWithHash("dir/a.txt", 4, ModTime{}, 0, hash) + require.ErrorIs(t, err, errDuplicatePath) + assert.EqualError(t, err, `duplicate path "dir/a.txt"`) + + assert.Equal(t, 1, b.FileCount()) +} + func TestBuilderBuild(t *testing.T) { t.Parallel() diff --git a/mfer/deserialize.go b/mfer/deserialize.go index 85ec2b6..15ba6b3 100644 --- a/mfer/deserialize.go +++ b/mfer/deserialize.go @@ -293,12 +293,21 @@ func (m *manifest) deserializeInner() error { // extract path tomorrow — acts on a traversal or absolute path from an // untrusted .mf. Reject loudly on the first offender rather than // dropping entries, which would let a hostile manifest hide files from a - // check. + // check. A path listed twice is refused too: check would check the one + // file against both entries. + seen := make(map[string]bool, len(m.pbInner.GetFiles())) + for _, f := range m.pbInner.GetFiles() { err = ValidatePath(f.GetPath()) if err != nil { return fmt.Errorf("%w: %w", errInvalidManifestPath, err) } + + if seen[f.GetPath()] { + return fmt.Errorf("%w %q", errDuplicatePath, f.GetPath()) + } + + seen[f.GetPath()] = true } log.Infof("loaded manifest with %d files", len(m.pbInner.GetFiles())) diff --git a/mfer/deserialize_path_test.go b/mfer/deserialize_path_test.go index 8572901..09dd342 100644 --- a/mfer/deserialize_path_test.go +++ b/mfer/deserialize_path_test.go @@ -7,7 +7,6 @@ import ( "crypto/sha256" "fmt" "strconv" - "strings" "testing" "time" "uuid" @@ -118,12 +117,61 @@ func TestDeserializeRejectsInvalidEntryPaths(t *testing.T) { } } +// A manifest that lists a path twice is refused as it is loaded, naming the +// path. Paths are compared byte for byte: two that differ only in letter case +// load, and fetch refuses those itself. Each entry has a hash, as entries mfer +// writes do; entries of a path alone would take too much memory to decode. +func TestDeserializeRefusesPathListedTwice(t *testing.T) { + t.Parallel() + + hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256) + require.NoError(t, err) + + tests := []struct { + name string + paths []string + refused bool + }{ + {"same path twice", []string{"dir/a.txt", "b.txt", "dir/a.txt"}, true}, + {"paths differing in letter case", []string{"dir/a.txt", "dir/A.txt"}, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + id := uuid.NewV4() + inner := &MFFile{Version: MFFile_VERSION_ONE, Uuid: id[:]} + + for _, p := range tt.paths { + inner.Files = append(inner.Files, &MFFilePath{ + Path: p, + Hashes: []*MFFileChecksum{{MultiHash: hash}}, + }) + } + + innerData, err := proto.Marshal(inner) + require.NoError(t, err) + + m, err := NewManifestFromReader(bytes.NewReader(wrapInner(t, id, innerData))) + if tt.refused { + require.ErrorIs(t, err, errDuplicatePath) + assert.EqualError(t, err, `duplicate path "dir/a.txt"`) + } else { + require.NoError(t, err) + assert.Len(t, m.Files(), len(tt.paths)) + } + }) + } +} + // 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 // 53 bytes, about 8.2 times: refused, and leaving any one of the five // uncounted, even the MIME type, brings it under 8. A 39-character path makes -// it 55 bytes, about 7.9 times: loaded. +// it 55 bytes, about 7.9 times: loaded. Each entry's path is its number, +// padded with zeros to that length, since a manifest lists a path only once. func TestDeserializeRefusesEntriesThatDecodeTooLarge(t *testing.T) { t.Parallel() @@ -139,22 +187,22 @@ func TestDeserializeRefusesEntriesThatDecodeTooLarge(t *testing.T) { t.Run(strconv.Itoa(tt.pathLen), func(t *testing.T) { t.Parallel() - entry := protowire.AppendTag(nil, 1, protowire.BytesType) // MFFilePath.path - entry = protowire.AppendString(entry, strings.Repeat("a", tt.pathLen)) - entry = protowire.AppendTag(entry, 3, protowire.BytesType) // MFFilePath.hashes - entry = protowire.AppendBytes(entry, nil) - entry = protowire.AppendTag(entry, 301, protowire.BytesType) // MFFilePath.mimeType - entry = protowire.AppendBytes(entry, nil) - entry = protowire.AppendTag(entry, 302, protowire.BytesType) // MFFilePath.mtime - entry = protowire.AppendBytes(entry, nil) - entry = protowire.AppendTag(entry, 303, protowire.BytesType) // MFFilePath.ctime - entry = protowire.AppendBytes(entry, nil) - id := uuid.NewV4() inner := protowire.AppendTag(nil, 102, protowire.BytesType) // MFFile.uuid inner = protowire.AppendBytes(inner, id[:]) - for range 1000 { + for i := range 1000 { + entry := protowire.AppendTag(nil, 1, protowire.BytesType) // MFFilePath.path + entry = protowire.AppendString(entry, fmt.Sprintf("%0*d", tt.pathLen, i)) + entry = protowire.AppendTag(entry, 3, protowire.BytesType) // MFFilePath.hashes + entry = protowire.AppendBytes(entry, nil) + entry = protowire.AppendTag(entry, 301, protowire.BytesType) // MFFilePath.mimeType + entry = protowire.AppendBytes(entry, nil) + entry = protowire.AppendTag(entry, 302, protowire.BytesType) // MFFilePath.mtime + entry = protowire.AppendBytes(entry, nil) + entry = protowire.AppendTag(entry, 303, protowire.BytesType) // MFFilePath.ctime + entry = protowire.AppendBytes(entry, nil) + inner = protowire.AppendTag(inner, 101, protowire.BytesType) // MFFile.files inner = protowire.AppendBytes(inner, entry) } diff --git a/mfer/scanner.go b/mfer/scanner.go index 129e503..92b5538 100644 --- a/mfer/scanner.go +++ b/mfer/scanner.go @@ -2,6 +2,7 @@ package mfer import ( "context" + "fmt" "io" "io/fs" "os" @@ -79,7 +80,8 @@ type FileEntry struct { type Scanner struct { mu sync.RWMutex files []*FileEntry - totalBytes FileSize // cached sum of all file sizes + paths map[RelFilePath]AbsFilePath // the file at each path in files + totalBytes FileSize // cached sum of all file sizes options *ScannerOptions fs afero.Fs excluded []fs.FileInfo // the files named in ExcludePaths that exist @@ -103,6 +105,7 @@ func NewScannerWithOptions(opts *ScannerOptions) *Scanner { s := &Scanner{ files: make([]*FileEntry, 0), + paths: make(map[RelFilePath]AbsFilePath), options: opts, fs: fs, } @@ -478,6 +481,18 @@ func (s *Scanner) enumerateFileWithInfo( } s.mu.Lock() + + // Each path is relative to the input path it was found under, so files + // under two input paths can share one. + first, ok := s.paths[entry.Path] + if ok { + s.mu.Unlock() + + return fmt.Errorf("%w %q: %s and %s", + errDuplicatePath, entry.Path, first, entry.AbsPath) + } + + s.paths[entry.Path] = entry.AbsPath s.files = append(s.files, entry) s.totalBytes += entry.Size filesFound := FileCount(len(s.files))