From 7122b56a51eb885b38970d3089244e3bde64856e Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 4 Oct 2026 16:37:58 +0000 Subject: [PATCH] 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 --- README.md | 9 ++-- internal/cli/fetch.go | 54 ++++++++++++++------- internal/cli/fetch_test.go | 97 +++++++++++++++++++++++++++++++------- 3 files changed, 121 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index a826bf2..407cd89 100644 --- a/README.md +++ b/README.md @@ -265,9 +265,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 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 diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index 2b61c7f..ad1c0a2 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -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) + } } } diff --git a/internal/cli/fetch_test.go b/internal/cli/fetch_test.go index d9e0a20..52427a8 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -1166,33 +1166,94 @@ func TestFetchRequireSignature(t *testing.T) { func TestFetchRefusesListedManifestName(t *testing.T) { t.Parallel() - for _, listed := range []string{ - defaultManifestName, - tempPathFor(defaultManifestName), - defaultManifestName + "/" + testFileTxt, - "INDEX.MF", - "./" + defaultManifestName, + for listed, writtenThere := range map[string]string{ + defaultManifestName: "the saved manifest", + tempPathFor(defaultManifestName): "the saved manifest's temp file", + defaultManifestName + "/" + testFileTxt: "the saved manifest", + "INDEX.MF": "the saved manifest", + "./" + defaultManifestName: "the saved manifest", } { 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+" ("+writtenThere+")") }) } } +// 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. 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/"} { + t.Run(dir+"a.txt and "+dir+".a.txt.tmp", func(t *testing.T) { + t.Parallel() + + files := map[string][]byte{ + dir + "a.txt": []byte("a file"), + dir + ".a.txt.tmp": []byte("a file at its temp name"), + } + + assertFetchRefused(t, builtManifest(t, files), files, + "manifest lists a file where fetch writes another file: "+ + dir+".a.txt.tmp (the temp file for "+dir+"a.txt)") + }) + } + + for _, listed := range []string{"a.txt", ".a.txt.tmp"} { + t.Run("only "+listed, func(t *testing.T) { + t.Parallel() + + files := map[string][]byte{listed: []byte("listed")} + 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