fetch refuses a manifest that lists another file's temp name (closes #151 )

fetch downloads each file to a temp name beside it and first removes whatever is there. A manifest listing both a.txt and .a.txt.tmp had fetch delete the second while fetching the first, then exit 0 with a tree check rejects. The refusal of a manifest that lists the saved manifest's own name or temp name now covers this too: a listed file, or a directory a listed file is in, may not sit at any name fetch writes besides the listed files themselves. Temp names come from tempPathFor, names are compared ignoring case as before, and the refusal still happens before the destination is created or any file requested. Model: opus-5-5
2026-10-04 20:02:17 +02:00
3 changed files with 120 additions and 33 deletions
@@ -268,9 +268,12 @@ are now tracked only in the [issues](https://git.eeqj.de/sneak/mfer/issues).
      cryptographic integrity of downloaded files. A file already there with the
      size and hash the manifest lists is skipped. Once every file is in place,
      the manifest is saved there as `index.mf`, so `mfer check` can verify the
-      tree later. A manifest that lists `index.mf` (in any letter case) or
-      `.index.mf.tmp` at the top of the tree is refused before any file is
-      downloaded, since saving the manifest would replace it.
+      tree later. Each file is downloaded to a temp file beside it, such as
+      `.a.txt.tmp` for `a.txt`, then moved into place. A manifest is refused
+      before any file is downloaded if it lists a file where fetch writes
+      another: at the temp file of a listed file, or at `index.mf` or
+      `.index.mf.tmp` at the top of the tree. Names are compared in any letter
+      case.
 - `mfer fetch --require-signature <fingerprint> https://example.com/stuff/`
    - as above, but first refuses a manifest not signed by the key with that
      fingerprint, as `mfer check --require-signature` does, before downloading
@@ -91,10 +91,10 @@ var (
 	// errHashMismatch indicates a downloaded file whose hash matches no
 	// manifest hash.
 	errHashMismatch = errors.New("hash mismatch")
-	// errManifestNameListed indicates a manifest that lists a file where
-	// fetch saves the manifest.
-	errManifestNameListed = errors.New(
-		"manifest lists a file where fetch saves the manifest")
+	// errNameClash indicates a manifest that lists a file where fetch
+	// writes another file.
+	errNameClash = errors.New(
+		"manifest lists a file where fetch writes another file")
 )

 // DownloadProgress reports the progress of a single file download.
@@ -412,7 +412,7 @@ func (mfa *CLIApp) fetchManifestOperation(ctx *cli.Context) error {

 // fetchManifest downloads the manifest at manifestURL and parses it,
 // enforcing --require-signature if it is given and refusing a manifest
-// that lists a file where it will be saved. It returns the manifest as
+// that lists a file where fetch writes another. It returns the manifest as
 // downloaded, to be saved once the files are in place, and the files it
 // lists.
 func fetchManifest(
@@ -452,7 +452,7 @@ func fetchManifest(

 	files := manifest.Files()

-	err = checkManifestNameUnlisted(files)
+	err = checkNoNameClash(files)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -462,19 +462,37 @@ func fetchManifest(
 	return manifestData, files, nil
 }

-// checkManifestNameUnlisted returns an error if files lists a file or
-// directory at the top of the tree under the name fetch saves the
-// manifest as, or under that name's temp file. Saving the manifest would
-// replace or remove it, or fail once every file was downloaded, leaving a
-// tree check rejects. Names are compared ignoring case, since on a
-// case-insensitive filesystem INDEX.MF and index.mf are one file.
-func checkManifestNameUnlisted(files []*mfer.MFFilePath) error {
-	for _, f := range files {
-		top, _, _ := strings.Cut(filepath.Clean(f.GetPath()), string(filepath.Separator))
+// checkNoNameClash returns an error if files lists a file, or a directory
+// a file is in, under a name where fetch writes another file: the temp
+// file it downloads a listed file to, or, at the top of the tree, the
+// saved manifest or its temp file. fetch would remove or replace what is
+// listed there, or fail partway, leaving a tree check rejects. Names are
+// compared ignoring case, since on a case-insensitive filesystem INDEX.MF
+// and index.mf are one file.
+func checkNoNameClash(files []*mfer.MFFilePath) error {
+	sep := string(filepath.Separator)

-		if strings.EqualFold(top, defaultManifestName) ||
-			strings.EqualFold(top, tempPathFor(defaultManifestName)) {
-			return fmt.Errorf("%w: %s", errManifestNameListed, f.GetPath())
+	// written maps each name fetch writes, other than the listed files
+	// themselves, in lower case, to the file it writes there.
+	written := map[string]string{
+		defaultManifestName:              "the saved manifest",
+		tempPathFor(defaultManifestName): "the saved manifest's temp file",
+	}
+
+	for _, f := range files {
+		tmpPath := tempPathFor(filepath.Clean(f.GetPath()))
+		written[strings.ToLower(tmpPath)] = "the temp file for " + f.GetPath()
+	}
+
+	for _, f := range files {
+		// Look up each directory on the file's path, then the file itself.
+		parts := strings.Split(strings.ToLower(filepath.Clean(f.GetPath())), sep)
+
+		for i := range parts {
+			what, ok := written[strings.Join(parts[:i+1], sep)]
+			if ok {
+				return fmt.Errorf("%w: %s (%s)", errNameClash, f.GetPath(), what)
+			}
 		}
 	}

@@ -1176,23 +1176,89 @@ func TestFetchRefusesListedManifestName(t *testing.T) {
 		t.Run(listed, func(t *testing.T) {
 			t.Parallel()

-			// Built directly rather than scanned, since a scan lists no
-			// hidden files and never a path starting with "./".
-			content := []byte("listed")
-			builder := mfer.NewBuilder()
-			_, err := builder.AddFile(mfer.RelFilePath(listed), mfer.FileSize(len(content)),
-				mfer.ModTime(time.Now()), bytes.NewReader(content), nil)
-			require.NoError(t, err)
+			files := map[string][]byte{listed: []byte("listed")}

-			var manifest bytes.Buffer
-			require.NoError(t, builder.Build(context.Background(), &manifest))
-
-			assertFetchRefused(t, manifest.Bytes(), map[string][]byte{listed: content},
-				"manifest lists a file where fetch saves the manifest: "+listed)
+			assertFetchRefused(t, builtManifest(t, files), files,
+				"manifest lists a file where fetch writes another file: "+listed)
 		})
 	}
 }

+// TestFetchRefusesListedTempName fetches manifests that list a.txt and
+// .a.txt.tmp, the temp file fetch downloads a.txt to, at the top of the
+// tree and in a directory. Downloading a.txt would remove .a.txt.tmp, so
+// fetch must refuse the manifest before it creates the destination or
+// requests any file. README with .README.tmp checks that temp names are
+// compared ignoring case. A manifest that lists only one of the two is
+// fetched in full.
+func TestFetchRefusesListedTempName(t *testing.T) {
+	t.Parallel()
+
+	for _, dir := range []string{"", "sub/"} {
+		for file, tmp := range map[string]string{
+			dir + "a.txt":  dir + ".a.txt.tmp",
+			dir + "README": dir + ".README.tmp",
+		} {
+			both := map[string][]byte{
+				file: []byte("a file"),
+				tmp:  []byte("a file at its temp name"),
+			}
+
+			t.Run(file+" and "+tmp, func(t *testing.T) {
+				t.Parallel()
+
+				assertFetchRefused(t, builtManifest(t, both), both,
+					"manifest lists a file where fetch writes another file: "+
+						tmp+" (the temp file for "+file+")")
+			})
+
+			for listed, content := range both {
+				t.Run("only "+listed, func(t *testing.T) {
+					t.Parallel()
+
+					files := map[string][]byte{listed: content}
+					manifest := builtManifest(t, files)
+
+					server := httptest.NewServer(fetchTestHandler(manifest, files))
+					defer server.Close()
+
+					dest := t.TempDir()
+
+					opts := testOpts([]string{
+						testApp, cmdFetch, "-q", "--" + flagDest, dest, server.URL,
+					}, afero.NewOsFs())
+					require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
+
+					want := maps.Clone(files)
+					want[defaultManifestName] = manifest
+					assert.Equal(t, want, filesUnder(t, dest))
+				})
+			}
+		}
+	}
+}
+
+// builtManifest returns a manifest of files, built directly rather than
+// scanned, since a scan lists no hidden files and never a path starting
+// with "./".
+func builtManifest(t *testing.T, files map[string][]byte) []byte {
+	t.Helper()
+
+	builder := mfer.NewBuilder()
+
+	for p, content := range files {
+		_, err := builder.AddFile(mfer.RelFilePath(p), mfer.FileSize(len(content)),
+			mfer.ModTime(time.Now()), bytes.NewReader(content), nil)
+		require.NoError(t, err)
+	}
+
+	var manifest bytes.Buffer
+
+	require.NoError(t, builder.Build(context.Background(), &manifest))
+
+	return manifest.Bytes()
+}
+
 // assertFetchRefused serves manifest, a manifest of files, and fetches it
 // with flags into a directory that does not exist yet. fetch must fail
 // with message after requesting only the manifest, and must not create