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