fetch: destination directory, skip files already present, save the manifest, require a signer (closes #101)

fetch takes --dest (default .) and writes every file there through the
existing symlink and hard-link guards, which now work relative to that
directory. A file already there with the listed size and hash is
skipped; a leftover temp file is still replaced. Once every file
verifies, the manifest is saved as index.mf through the same temp file
and rename, so check runs on the result; a manifest that lists index.mf
or its temp name at the top of the tree is refused, since saving would
replace that file. --require-signature is shared with check and
enforced through verifyRequiredSigner. Both refusals come before any
file is downloaded or anything is written.

Model: opus-5-5

internal/cli/fetch_test.go

@@ -13,6 +13,7 @@ import (
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"slices"
 	"strconv"
 	"sync"
 	"sync/atomic"
@@ -338,7 +339,7 @@ func TestFetchFromHTTP(t *testing.T) {
 
 		fileURL := baseURL + f.GetPath()
 		err = downloadFile(context.Background(), testClient(),
-			fileURL, localPath, f, progress)
+			fileURL, ".", localPath, f, progress)
 		require.NoError(t, err, "failed to download %s", f.GetPath())
 	}
 
@@ -386,7 +387,7 @@ func TestFetchHashMismatch(t *testing.T) {
 
 	// Try to download - should fail with hash mismatch
 	err = downloadFile(context.Background(), testClient(),
-		server.URL+"/file.txt", testFileTxt, files[0], nil)
+		server.URL+"/file.txt", ".", testFileTxt, files[0], nil)
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "mismatch")
 
@@ -432,7 +433,7 @@ func TestFetchSizeMismatch(t *testing.T) {
 
 	// Try to download - should fail with size mismatch
 	err = downloadFile(context.Background(), testClient(),
-		server.URL+"/file.txt", testFileTxt, files[0], nil)
+		server.URL+"/file.txt", ".", testFileTxt, files[0], nil)
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "size mismatch")
 
@@ -490,7 +491,7 @@ func TestFetchProgress(t *testing.T) {
 
 	// Download
 	err = downloadFile(context.Background(), testClient(),
-		server.URL+"/large.txt", "large.txt", files[0], progress)
+		server.URL+"/large.txt", ".", "large.txt", files[0], progress)
 	close(progress)
 	<-done
 
@@ -513,24 +514,51 @@ func TestFetchProgress(t *testing.T) {
 	assert.Equal(t, content, downloaded)
 }
 
-// TestFetchRefusesSymlinks runs fetch into a destination directory that
-// holds a symlink pointing outside it, in each of the three places fetch
-// writes: a parent directory, the temp file, and the file itself, which
-// the temp file is renamed onto; and once as a directory inside a plain
-// directory. The fetch must fail and nothing outside may change.
+// TestFetchRefusesSymlinks runs fetch with --dest naming a directory other
+// than the current one, which holds a symlink pointing outside it, in each
+// place fetch writes: a parent directory, the temp file, the file itself,
+// which the temp file is renamed onto, and the saved manifest's temp file
+// and final name; and once as a directory inside a plain directory. The
+// fetch must fail, and neither the outside directory nor the current one
+// may change.
 //
 //nolint:paralleltest // changes the process-global working directory
 func TestFetchRefusesSymlinks(t *testing.T) {
+	// What a link standing for a file points to: a file outside that does
+	// not exist yet.
+	const newFile = "new.txt"
+
 	tests := []struct {
-		name   string
-		entry  string // the manifest's only file
-		link   string // symlink placed in the destination directory
-		target string // what link points to, relative to the outside directory
+		name    string
+		entry   string // the manifest's only file
+		link    string // symlink placed in the destination directory
+		target  string // what link points to, relative to the outside directory
+		failure string // what fetch reports it was doing when it found link
 	}{
-		{"parent directory", "sub/deeper/file.txt", "sub", "."},
-		{"directory inside a plain directory", "docs/data/passwd", "docs/data", "."},
-		{"temp file", testFileTxt, ".file.txt.tmp", "new.txt"},
-		{"file", testFileTxt, testFileTxt, "new.txt"},
+		{
+			"parent directory", "sub/deeper/file.txt", "sub", ".",
+			"failed to download sub/deeper/file.txt",
+		},
+		{
+			"directory inside a plain directory", "docs/data/passwd", "docs/data", ".",
+			"failed to download docs/data/passwd",
+		},
+		{
+			"temp file", testFileTxt, ".file.txt.tmp", newFile,
+			"failed to download " + testFileTxt,
+		},
+		{
+			"file", testFileTxt, testFileTxt, newFile,
+			"failed to download " + testFileTxt,
+		},
+		{
+			"manifest temp file", testFileTxt, tempPathFor(defaultManifestName), newFile,
+			"failed to save manifest",
+		},
+		{
+			"manifest", testFileTxt, defaultManifestName, newFile,
+			"failed to save manifest",
+		},
 	}
 
 	for _, tt := range tests {
@@ -545,23 +573,61 @@ func TestFetchRefusesSymlinks(t *testing.T) {
 			defer server.Close()
 
 			outside := t.TempDir()
+			cwd := chdirTemp(t)
+			dest := t.TempDir()
+			link := filepath.Join(dest, tt.link)
 
-			chdirTemp(t)
-			require.NoError(t, os.MkdirAll(filepath.Dir(tt.link), 0o750))
-			require.NoError(t, os.Symlink(filepath.Join(outside, tt.target), tt.link))
+			require.NoError(t, os.MkdirAll(filepath.Dir(link), 0o750))
+			require.NoError(t, os.Symlink(filepath.Join(outside, tt.target), link))
 
-			opts := testOpts([]string{testApp, cmdFetch, "-q", server.URL}, afero.NewOsFs())
+			opts := testOpts([]string{
+				testApp, cmdFetch, "-q", "--" + flagDest, dest, server.URL,
+			}, afero.NewOsFs())
 			assert.Equal(t, 1, runCLI(opts))
-			assert.Contains(t, testStderr(t, opts), "failed to download "+tt.entry+
-				": symlink in path not allowed: "+tt.link)
+			assert.Contains(t, testStderr(t, opts),
+				tt.failure+": symlink in path not allowed: "+link)
 
 			written, err := os.ReadDir(outside)
 			require.NoError(t, err)
 			assert.Empty(t, written, "fetch wrote outside the destination")
+
+			written, err = os.ReadDir(cwd)
+			require.NoError(t, err)
+			assert.Empty(t, written, "fetch wrote to the current directory")
 		})
 	}
 }
 
+// TestFetchDoesNotSkipThroughSymlink runs fetch with --dest holding a
+// symlink to a directory outside it, where the file the manifest lists
+// through that symlink already sits with the listed content. fetch must
+// not take that file as already present: it must fail on the symlink, as
+// the download would, and leave the outside file alone.
+func TestFetchDoesNotSkipThroughSymlink(t *testing.T) {
+	t.Parallel()
+
+	content := []byte("fetched")
+	files := map[string][]byte{"sub/" + testFileTxt: content}
+
+	server := httptest.NewServer(fetchTestHandler(manifestOf(t, files), files))
+	defer server.Close()
+
+	outside := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(outside, testFileTxt), content, 0o600))
+
+	dest := t.TempDir()
+	link := filepath.Join(dest, "sub")
+	require.NoError(t, os.Symlink(outside, link))
+
+	opts := testOpts([]string{
+		testApp, cmdFetch, "-q", "--" + flagDest, dest, server.URL,
+	}, afero.NewOsFs())
+	assert.Equal(t, 1, runCLI(opts))
+	assert.Contains(t, testStderr(t, opts),
+		"failed to download sub/"+testFileTxt+": symlink in path not allowed: "+link)
+	assert.Equal(t, map[string][]byte{testFileTxt: content}, filesUnder(t, outside))
+}
+
 // TestFetchReplacesHardLinkAtTempName runs fetch into a destination
 // directory that holds, at the temp file's name, a hard link to a file
 // outside it. To fetch that is an ordinary leftover from an interrupted
@@ -787,7 +853,7 @@ func TestDownloadFileRetriesToSuccess(t *testing.T) {
 	chdirTemp(t)
 
 	err = downloadFile(context.Background(), testClient(),
-		server.URL+"/"+testFileTxt, testFileTxt, manifest.Files()[0], nil)
+		server.URL+"/"+testFileTxt, ".", testFileTxt, manifest.Files()[0], nil)
 	require.NoError(t, err)
 	assert.Equal(t, int32(3), requests.Load())
 
@@ -878,7 +944,7 @@ func TestFetchEscapedPaths(t *testing.T) {
 
 // TestFetchTree runs fetch on a tree with nested directories. Every file
 // the manifest lists must land under its own path with its own content,
-// and nothing else may be left in the destination.
+// beside the manifest, and nothing else may be left in the destination.
 //
 //nolint:paralleltest // changes the process-global working directory
 func TestFetchTree(t *testing.T) {
@@ -889,7 +955,9 @@ func TestFetchTree(t *testing.T) {
 		"other/deep/est.txt": []byte("in a second directory"),
 	}
 
-	server := httptest.NewServer(fetchTestHandler(manifestOf(t, files), files))
+	manifest := manifestOf(t, files)
+
+	server := httptest.NewServer(fetchTestHandler(manifest, files))
 	defer server.Close()
 
 	dest := chdirTemp(t)
@@ -897,7 +965,9 @@ func TestFetchTree(t *testing.T) {
 	opts := testOpts([]string{testApp, cmdFetch, "-q", server.URL}, afero.NewOsFs())
 	require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
 
-	assert.Equal(t, files, filesUnder(t, dest))
+	want := maps.Clone(files)
+	want[defaultManifestName] = manifest
+	assert.Equal(t, want, filesUnder(t, dest))
 }
 
 // TestFetchFailsOnHashMismatch runs fetch against a server that serves a
@@ -922,9 +992,10 @@ func TestFetchFailsOnHashMismatch(t *testing.T) {
 // TestFetchIntoPartlyFilledDestination runs fetch where an interrupted
 // fetch of an older version of the tree left one file current, one file
 // out of date and one half written to its temp file, beside a file the
-// manifest does not list. fetch downloads every file the manifest lists,
-// those already present included, and replaces what is there; the file
-// the manifest does not list is left alone.
+// manifest does not list. fetch skips the current file and downloads the
+// other two. The out-of-date file has the same size as the new version,
+// so only its hash shows that it must be replaced. The file the manifest
+// does not list is left alone, and the manifest is saved beside the files.
 //
 //nolint:paralleltest // changes the process-global working directory
 func TestFetchIntoPartlyFilledDestination(t *testing.T) {
@@ -935,7 +1006,8 @@ func TestFetchIntoPartlyFilledDestination(t *testing.T) {
 	}
 	unlisted := []byte("not in the manifest")
 
-	tree := fetchTestHandler(manifestOf(t, files), files)
+	manifest := manifestOf(t, files)
+	tree := fetchTestHandler(manifest, files)
 
 	var (
 		mu        sync.Mutex
@@ -963,21 +1035,208 @@ func TestFetchIntoPartlyFilledDestination(t *testing.T) {
 		os.WriteFile(tempPathFor("sub/partial.txt"), []byte("cut off"), 0o600))
 	require.NoError(t, os.WriteFile("unlisted.txt", unlisted, 0o600))
 
-	opts := testOpts([]string{testApp, cmdFetch, "-q", server.URL}, afero.NewOsFs())
+	opts := testOpts([]string{testApp, cmdFetch, server.URL}, afero.NewOsFs())
 	require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
+	assert.Contains(t, testStderr(t, opts), "skipping current.txt: already present")
 
 	want := maps.Clone(files)
 	want["unlisted.txt"] = unlisted
+	want[defaultManifestName] = manifest
 	assert.Equal(t, want, filesUnder(t, dest))
 
 	mu.Lock()
 	defer mu.Unlock()
 
 	assert.ElementsMatch(t, []string{
-		"/" + defaultManifestName, "/current.txt", "/sub/changed.txt", "/sub/partial.txt",
+		"/" + defaultManifestName, "/sub/changed.txt", "/sub/partial.txt",
 	}, requested)
 }
 
+// TestFetchIntoDest fetches a tree with --dest into a directory that does
+// not exist yet. The files and the manifest must land there and nowhere
+// else, and check must pass on the result with no extra files. A second
+// fetch into the same directory must download nothing but the manifest.
+//
+//nolint:paralleltest // changes the process-global working directory
+func TestFetchIntoDest(t *testing.T) {
+	files := map[string][]byte{
+		"top.txt":     []byte("at the top"),
+		"sub/one.txt": []byte("one level down"),
+	}
+
+	manifest := manifestOf(t, files)
+	tree := fetchTestHandler(manifest, files)
+
+	var (
+		mu        sync.Mutex
+		requested []string
+	)
+
+	server := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			mu.Lock()
+
+			requested = append(requested, r.URL.Path)
+
+			mu.Unlock()
+
+			tree.ServeHTTP(w, r)
+		}))
+	defer server.Close()
+
+	cwd := chdirTemp(t)
+	dest := filepath.Join(t.TempDir(), "mirror")
+	fetch := []string{testApp, cmdFetch, "-q", "--" + flagDest, dest, server.URL}
+
+	opts := testOpts(fetch, 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))
+	assert.Empty(t, filesUnder(t, cwd), "fetch wrote outside --dest")
+
+	check := testOpts([]string{
+		testApp, cmdCheck, "-q", testFlagBase, dest, testFlagNoExtra,
+		filepath.Join(dest, defaultManifestName),
+	}, afero.NewOsFs())
+	require.Equal(t, 0, runCLI(check), testStderr(t, check))
+
+	mu.Lock()
+	requested = nil
+	mu.Unlock()
+
+	opts = testOpts(fetch, afero.NewOsFs())
+	require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
+	assert.Equal(t, want, filesUnder(t, dest))
+
+	mu.Lock()
+	defer mu.Unlock()
+
+	assert.Equal(t, []string{"/" + defaultManifestName}, requested)
+}
+
+// TestFetchRequireSignature runs fetch with --require-signature. A
+// manifest that is unsigned, or signed by another key, must stop fetch
+// with check's message before it downloads or writes anything; the
+// required key lets it through. The signed cases need gpg and are skipped
+// without it, as the other signing tests are.
+//
+//nolint:paralleltest // signedManifest calls t.Setenv, which bars t.Parallel
+func TestFetchRequireSignature(t *testing.T) {
+	files := map[string][]byte{testFileTxt: []byte("signed file")}
+
+	t.Run("unsigned", func(t *testing.T) {
+		assertFetchRefused(t, manifestOf(t, files), files,
+			"manifest is not signed, but signature from "+msgFpA+" is required",
+			"--"+flagRequireSignature, msgFpA)
+	})
+
+	t.Run("signed", func(t *testing.T) {
+		manifest := signedManifest(t, files)
+
+		signer, err := signedChecker(t, manifest).
+			ExtractEmbeddedSigningKeyFP(context.Background())
+		require.NoError(t, err)
+
+		assertFetchRefused(t, manifest, files,
+			"embedded signing key fingerprint "+signer+" does not match required "+msgFpB,
+			"--"+flagRequireSignature, msgFpB)
+
+		server := httptest.NewServer(fetchTestHandler(manifest, files))
+		defer server.Close()
+
+		dest := t.TempDir()
+
+		opts := testOpts([]string{
+			testApp, cmdFetch, "-q", "--" + flagDest, dest,
+			"--" + flagRequireSignature, signer, server.URL,
+		}, afero.NewOsFs())
+		require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
+		assert.Equal(t, files[testFileTxt], filesUnder(t, dest)[testFileTxt])
+	})
+}
+
+// TestFetchRefusesListedManifestName fetches manifests that list, at the
+// top of the tree, the name fetch saves the manifest under or that name's
+// temp file: as a file, as a directory, in capitals, and with a leading
+// "./". Saving the manifest would replace or remove what is listed there,
+// so fetch must refuse the manifest before it creates the destination or
+// requests any file.
+func TestFetchRefusesListedManifestName(t *testing.T) {
+	t.Parallel()
+
+	for _, listed := range []string{
+		defaultManifestName,
+		tempPathFor(defaultManifestName),
+		defaultManifestName + "/" + testFileTxt,
+		"INDEX.MF",
+		"./" + defaultManifestName,
+	} {
+		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)
+
+			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 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
+// the directory.
+func assertFetchRefused(
+	t *testing.T, manifest []byte, files map[string][]byte,
+	message string, flags ...string,
+) {
+	t.Helper()
+
+	tree := fetchTestHandler(manifest, files)
+
+	var (
+		mu        sync.Mutex
+		requested []string
+	)
+
+	server := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			mu.Lock()
+
+			requested = append(requested, r.URL.Path)
+
+			mu.Unlock()
+
+			tree.ServeHTTP(w, r)
+		}))
+	defer server.Close()
+
+	dest := filepath.Join(t.TempDir(), "mirror")
+
+	opts := testOpts(slices.Concat(
+		[]string{testApp, cmdFetch, "-q", "--" + flagDest, dest}, flags, []string{server.URL},
+	), afero.NewOsFs())
+	assert.Equal(t, 1, runCLI(opts))
+	assert.Contains(t, testStderr(t, opts), message)
+	assert.NoDirExists(t, dest, "fetch created the destination before refusing")
+
+	mu.Lock()
+	defer mu.Unlock()
+
+	assert.Equal(t, []string{"/" + defaultManifestName}, requested)
+}
+
 // TestFetchTimeoutFlag runs fetch with --timeout against a server that
 // never answers. Without the flag's limit the request would wait forever;
 // once fetch gives up on it, the server cancels fetch's context so that