Load the manifest check downloads from memory, not a temp file (closes #223)

check given a URL wrote the manifest it downloaded to a file in the temp
directory before loading it, so it failed wherever that directory is
missing or cannot be written, such as the scratch image script/cibuild
builds. It now keeps the manifest in memory, as fetch, list and export
do. NewChecker reads a manifest handed to it as a reader through the new
CheckerOptions.ManifestReader field, in place of ManifestPath, because it
otherwise reads the manifest from the filesystem holding the files it
checks. fetch now hands its downloaded manifest to the Checker the same
way, not through an in-memory filesystem.

Model: opus-5-5

internal/cli/check.go

@@ -2,6 +2,7 @@
 package cli
 
 import (
+	"bytes"
 	"context"
 	"encoding/hex"
 	"errors"
@@ -94,42 +95,29 @@ func findManifest(fs afero.Fs, dir string) (string, error) {
 	return path, nil
 }
 
-// fetchManifestToTemp downloads a manifest URL to a temporary file and
-// returns the temp file path. The caller is responsible for removing it.
-func (mfa *CLIApp) fetchManifestToTemp(
-	ctx context.Context, url string,
-) (string, error) {
-	rc, fetchErr := mfa.openManifestReader(ctx, url)
-	if fetchErr != nil {
-		return "", fetchErr
+// downloadManifest downloads the manifest at url into memory. It refuses a
+// manifest larger than mfa.maxManifestSize.
+func (mfa *CLIApp) downloadManifest(ctx context.Context, url string) ([]byte, error) {
+	rc, err := mfa.openManifestReader(ctx, url)
+	if err != nil {
+		return nil, err
 	}
 
-	tmpFile, tmpErr := afero.TempFile(mfa.Fs, "", "mfer-manifest-*.mf")
-	if tmpErr != nil {
-		_ = rc.Close()
+	defer func() { _ = rc.Close() }()
 
-		return "", tmpErr
-	}
-
-	tmpPath := tmpFile.Name()
-
-	// Copying stops one byte past mfa.maxManifestSize, which is enough to
+	// Reading stops one byte past mfa.maxManifestSize, which is enough to
 	// tell that the manifest is too large.
-	written, cpErr := io.Copy(tmpFile, io.LimitReader(rc, mfa.maxManifestSize+1))
-	_ = rc.Close()
-	_ = tmpFile.Close()
-
-	if cpErr == nil && written > mfa.maxManifestSize {
-		cpErr = fmt.Errorf("%w of %d bytes", errManifestTooLarge, mfa.maxManifestSize)
+	manifestData, err := io.ReadAll(io.LimitReader(rc, mfa.maxManifestSize+1))
+	if err != nil {
+		return nil, fmt.Errorf("download manifest: %w", err)
 	}
 
-	if cpErr != nil {
-		_ = mfa.Fs.Remove(tmpPath)
-
-		return "", fmt.Errorf("download manifest: %w", cpErr)
+	if int64(len(manifestData)) > mfa.maxManifestSize {
+		return nil, fmt.Errorf("download manifest: %w of %d bytes",
+			errManifestTooLarge, mfa.maxManifestSize)
 	}
 
-	return tmpPath, nil
+	return manifestData, nil
 }
 
 // verifyRequiredSigner enforces the --require-signature fingerprint
@@ -299,32 +287,28 @@ func (mfa *CLIApp) checkManifestOperation(
 		return err
 	}
 
-	// Done before a URL is swapped for the temp file it is downloaded to,
-	// whose directory is not the base.
 	basePath := resolveBasePath(cmd, manifestPath)
 
-	// URL manifests need to be downloaded to a temp file for the checker
+	opts := &mfer.CheckerOptions{BasePath: basePath, Fs: mfa.Fs}
+
+	// A manifest given by URL is downloaded into memory and read from
+	// there.
 	if isHTTPURL(manifestPath) {
-		tmpPath, tmpErr := mfa.fetchManifestToTemp(ctx, manifestPath)
-		if tmpErr != nil {
-			return tmpErr
+		manifestData, downloadErr := mfa.downloadManifest(ctx, manifestPath)
+		if downloadErr != nil {
+			return downloadErr
 		}
 
-		defer func() { _ = mfa.Fs.Remove(tmpPath) }()
-
-		manifestPath = tmpPath
+		opts.ManifestReader = bytes.NewReader(manifestData)
+	} else {
+		opts.ManifestPath = manifestPath
 	}
 
 	showProgress := cmd.Bool("progress")
 
 	log.Infof("checking manifest %s with base %s", manifestPath, basePath)
 
-	// Create checker
-	chk, err := mfer.NewChecker(&mfer.CheckerOptions{
-		ManifestPath: manifestPath,
-		BasePath:     basePath,
-		Fs:           mfa.Fs,
-	})
+	chk, err := mfer.NewChecker(opts)
 	if err != nil {
 		return fmt.Errorf("load manifest: %w", err)
 	}

internal/cli/entry_test.go

@@ -1167,6 +1167,29 @@ func TestCheckURLManifestResolvesEntriesAgainstCurrentDirectory(t *testing.T) {
 	assert.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts))
 }
 
+// TestCheckURLManifestWithoutTempDirectory checks the tree under --base
+// against a manifest given by URL while TMPDIR names a directory that does
+// not exist. check keeps the manifest it downloads in memory, so it passes.
+// Setting TMPDIR keeps it from running in parallel with other tests.
+func TestCheckURLManifestWithoutTempDirectory(t *testing.T) {
+	files := map[string][]byte{testFileTxt: []byte("hello")}
+
+	server := httptest.NewServer(fetchTestHandler(manifestOf(t, files), files))
+	defer server.Close()
+
+	base := t.TempDir()
+	require.NoError(t,
+		os.WriteFile(filepath.Join(base, testFileTxt), files[testFileTxt], 0o600))
+
+	t.Setenv("TMPDIR", filepath.Join(t.TempDir(), "missing"))
+
+	opts := testOpts([]string{
+		testApp, cmdCheck, testFlagNoExtra, testFlagBase, base,
+		server.URL + "/" + defaultManifestName,
+	}, afero.NewOsFs())
+	assert.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts))
+}
+
 // unlistableDirFs is a filesystem on which one directory cannot be listed.
 type unlistableDirFs struct {
 	afero.Fs

internal/cli/fetch.go

@@ -19,7 +19,6 @@ import (
 
 	"github.com/dustin/go-humanize"
 	"github.com/multiformats/go-multihash"
-	"github.com/spf13/afero"
 	"github.com/urfave/cli/v3"
 	"sneak.berlin/go/mfer/internal/log"
 	"sneak.berlin/go/mfer/mfer"
@@ -554,22 +553,12 @@ func checkNoNameClash(files []*mfer.MFFilePath) error {
 }
 
 // verifyFetchedSigner enforces --require-signature on the fetched manifest
-// exactly as check does. verifyRequiredSigner takes a Checker, which loads
-// its manifest from a file, so the manifest is handed to it as a file in
-// memory.
+// exactly as check does. The Checker it builds is used only for the
+// manifest's signature, so its base path is never read.
 func verifyFetchedSigner(manifestData []byte, requiredSigner string) error {
-	memFs := afero.NewMemMapFs()
-	manifestPath := "/" + defaultManifestName
-
-	err := afero.WriteFile(memFs, manifestPath, manifestData, filePerms)
-	if err != nil {
-		return err
-	}
-
 	chk, err := mfer.NewChecker(&mfer.CheckerOptions{
-		ManifestPath: manifestPath,
-		BasePath:     "/",
-		Fs:           memFs,
+		ManifestReader: bytes.NewReader(manifestData),
+		BasePath:       "/",
 	})
 	if err != nil {
 		return fmt.Errorf("load manifest: %w", err)

internal/cli/fetch_test.go

@@ -489,18 +489,17 @@ func TestFetchStopsReadingFilePastListedSize(t *testing.T) {
 
 // TestManifestDownloadStopsAtLimit serves a manifest that never ends to
 // fetch and to check, with the most they download of a manifest lowered
-// to 64 KiB. Each must stop reading at that limit, fail with an error
-// naming it and leave no temp file.
+// to 64 KiB. Each must stop reading at that limit and fail with an error
+// naming it.
 func TestManifestDownloadStopsAtLimit(t *testing.T) {
+	t.Parallel()
+
 	server := httptest.NewServer(
 		http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 			_, _ = io.Copy(w, zeros{})
 		}))
 	defer server.Close()
 
-	tmpDir := t.TempDir()
-	t.Setenv("TMPDIR", tmpDir)
-
 	mfa := &CLIApp{Fs: afero.NewOsFs(), maxManifestSize: 64 << 10}
 
 	fetch := mfa.fetchCommand()
@@ -520,10 +519,6 @@ func TestManifestDownloadStopsAtLimit(t *testing.T) {
 			"download manifest: file exceeds maximum allowed size of 65536 bytes",
 			cmd.Name)
 	}
-
-	leftover, err := os.ReadDir(tmpDir)
-	require.NoError(t, err)
-	assert.Empty(t, leftover)
 }
 
 //nolint:paralleltest // changes the process-global working directory

mfer/checker.go

@@ -80,7 +80,8 @@ type Checker struct {
 	// manifestPaths is a set of paths in the manifest for quick lookup
 	manifestPaths map[RelFilePath]struct{}
 	// manifestInfo is the manifest file, which FindExtraFiles leaves out,
-	// matched with os.SameFile.
+	// matched with os.SameFile. It is nil for a manifest read from
+	// CheckerOptions.ManifestReader.
 	manifestInfo os.FileInfo
 	// signature info from the manifest
 	signature     []byte
@@ -90,8 +91,13 @@ type Checker struct {
 
 // CheckerOptions configures a Checker.
 type CheckerOptions struct {
-	// ManifestPath is the manifest file to check against (required).
+	// ManifestPath is the manifest file to check against. It is required
+	// unless ManifestReader is given.
 	ManifestPath string
+	// ManifestReader, if not nil, is read for the manifest to check against
+	// in place of ManifestPath, such as a manifest downloaded into memory.
+	// FindExtraFiles then has no manifest file to leave out.
+	ManifestReader io.Reader
 	// BasePath is the directory relative to which manifest paths are
 	// resolved (required).
 	BasePath string
@@ -100,9 +106,10 @@ type CheckerOptions struct {
 }
 
 // NewChecker creates a new Checker with the given options. It returns an
-// error if opts is nil or either path is empty.
+// error if opts is nil, the base path is empty, or neither a manifest path
+// nor a manifest reader is given.
 func NewChecker(opts *CheckerOptions) (*Checker, error) {
-	if opts == nil || opts.ManifestPath == "" {
+	if opts == nil || (opts.ManifestPath == "" && opts.ManifestReader == nil) {
 		return nil, errManifestPathEmpty
 	}
 
@@ -115,12 +122,30 @@ func NewChecker(opts *CheckerOptions) (*Checker, error) {
 		fs = afero.NewOsFs()
 	}
 
-	m, err := NewManifestFromFile(&ManifestFromFileOptions{
-		Path: opts.ManifestPath,
-		Fs:   fs,
-	})
-	if err != nil {
-		return nil, err
+	var (
+		m            *manifest
+		manifestInfo os.FileInfo
+		err          error
+	)
+
+	if opts.ManifestReader != nil {
+		m, err = NewManifestFromReader(opts.ManifestReader)
+		if err != nil {
+			return nil, err
+		}
+	} else {
+		m, err = NewManifestFromFile(&ManifestFromFileOptions{
+			Path: opts.ManifestPath,
+			Fs:   fs,
+		})
+		if err != nil {
+			return nil, err
+		}
+
+		manifestInfo, err = fs.Stat(opts.ManifestPath)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	abs, err := filepath.Abs(opts.BasePath)
@@ -135,11 +160,6 @@ func NewChecker(opts *CheckerOptions) (*Checker, error) {
 		manifestPaths[RelFilePath(f.GetPath())] = struct{}{}
 	}
 
-	manifestInfo, err := fs.Stat(opts.ManifestPath)
-	if err != nil {
-		return nil, err
-	}
-
 	return &Checker{
 		basePath:      AbsFilePath(abs),
 		files:         files,

mfer/checker_test.go

@@ -189,6 +189,38 @@ func TestNewCheckerNilFsUsesOsFs(t *testing.T) {
 	assert.Equal(t, StatusOK, (<-results).Status)
 }
 
+// TestNewCheckerReadsManifestFromReader gives NewChecker the manifest as a
+// reader and no manifest path, on a filesystem that holds only the files it
+// lists: it checks those files and finds nothing unlisted.
+func TestNewCheckerReadsManifestFromReader(t *testing.T) {
+	t.Parallel()
+
+	files := map[string][]byte{testFile1: []byte("hello")}
+
+	manifestFs := afero.NewMemMapFs()
+	createTestManifest(t, manifestFs, testManifestPath, files)
+	manifestData, err := afero.ReadFile(manifestFs, testManifestPath)
+	require.NoError(t, err)
+
+	fs := afero.NewMemMapFs()
+	createFilesOnDisk(t, fs, files)
+
+	chk, err := NewChecker(&CheckerOptions{
+		ManifestReader: bytes.NewReader(manifestData),
+		BasePath:       testDataDir,
+		Fs:             fs,
+	})
+	require.NoError(t, err)
+
+	results := make(chan Result, 1)
+	require.NoError(t, chk.Check(context.Background(), results, nil))
+	assert.Equal(t, Result{Path: testFile1, Status: StatusOK}, <-results)
+
+	extras := make(chan Result, len(files))
+	require.NoError(t, chk.FindExtraFiles(context.Background(), extras))
+	assert.Empty(t, extras)
+}
+
 func TestCheckerFileCountAndTotalBytes(t *testing.T) {
 	t.Parallel()