Refuse a path listed twice in a manifest (closes #170)

gen given arguments whose files share a path, such as gen a b with a.txt
in both, or gen . ., now fails while listing the files, before hashing
any, naming the path and both files. Builder.AddFile and
Builder.AddFileWithHash refuse a path already added. Loading refuses a
manifest that lists a path twice, compared byte for byte; fetch keeps
its own letter-case check. The Path Rules in docs/FORMAT.md say each
path appears at most once. The decode-size test listed one path 1000
times; each entry now has its own path of the same length.

Model: opus-5-5

docs/FORMAT.md

@@ -109,9 +109,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`)
 

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", testDir, "/other"},
+		{"one directory twice", testDir, testDir},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			fs := afero.NewMemMapFs()
+			require.NoError(t, fs.MkdirAll(testDir, 0o755))
+			require.NoError(t, fs.MkdirAll("/other", 0o755))
+			writeTestFile(t, fs, "/testdir/a.txt", "first")
+			writeTestFile(t, fs, "/other/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

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,7 @@ func (b *Builder) AddFile(
 		Mode:  uint32(mode.Perm()),
 	}
 
-	b.mu.Lock()
-	b.files = append(b.files, entry)
-	b.mu.Unlock()
-
-	return totalRead, nil
+	return totalRead, b.addEntry(entry)
 }
 
 // sendFileHashProgress sends a progress update without blocking.
@@ -234,8 +234,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 +278,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.
@@ -349,3 +346,19 @@ func (b *Builder) Build(ctx context.Context, w io.Writer) error {
 
 	return 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
+}

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)
+	require.EqualError(t, err, `duplicate path "dir/a.txt"`)
+
+	err = b.AddFileWithHash("dir/a.txt", 4, ModTime{}, 0, hash)
+	require.ErrorIs(t, err, errDuplicatePath)
+	require.EqualError(t, err, `duplicate path "dir/a.txt"`)
+
+	assert.Equal(t, 1, b.FileCount())
+}
+
 func TestBuilderBuild(t *testing.T) {
 	t.Parallel()
 

mfer/deserialize.go

@@ -294,12 +294,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()))

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", "other.txt", "dir/a.txt"}, true},
+		{"paths differing in letter case", []string{"dir/b.txt", "dir/B.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)
+				require.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)
 			}

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))