Limit how much fetch and check read for a manifest or a file (closes #168)

NewManifestFromReader reads at most one byte past MaxManifestSize, a new
constant of 258 MiB: the 256 MiB decompressed limit grown by zstd's worst
case of 1/256, plus 1 MiB for the signature, the signing key and the
other outer fields. It refuses a larger manifest. fetch, and check given
a URL, stop downloading a manifest one byte past the same size and report
it as too large; tests lower that size to keep their memory small. fetch
stops reading a file one byte past its listed size, so a longer body ends
in the size mismatch at once instead of filling the disk. docs/FORMAT.md
states the limit and gives the decompressed limit as 256 MiB, the size
the code uses.

Model: opus-5-5

internal/cli/check.go

@@ -113,12 +113,16 @@ func (mfa *CLIApp) fetchManifestToTemp(
 
 	tmpPath := tmpFile.Name()
 
-	// One byte past mfer.MaxManifestSize is enough for loading the
-	// manifest to refuse one that is too large.
-	_, cpErr := io.Copy(tmpFile, io.LimitReader(rc, mfer.MaxManifestSize+1))
+	// Copying 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)
+	}
+
 	if cpErr != nil {
 		_ = mfa.Fs.Remove(tmpPath)
 

internal/cli/entry.go

@@ -5,6 +5,7 @@ import (
 	"os"
 
 	"github.com/spf13/afero"
+	"sneak.berlin/go/mfer/mfer"
 )
 
 // NoColor disables colored output when set. Automatically true if the
@@ -56,14 +57,15 @@ func Run(appname, version, gitrev string) int {
 // RunWithOptions creates and runs the CLI application with the given options.
 func RunWithOptions(opts *RunOptions) int {
 	m := &CLIApp{
-		appname:  opts.Appname,
-		version:  opts.Version,
-		gitrev:   opts.Gitrev,
-		exitCode: 0,
-		Stdin:    opts.Stdin,
-		Stdout:   opts.Stdout,
-		Stderr:   opts.Stderr,
-		Fs:       opts.Fs,
+		appname:         opts.Appname,
+		version:         opts.Version,
+		gitrev:          opts.Gitrev,
+		exitCode:        0,
+		maxManifestSize: mfer.MaxManifestSize,
+		Stdin:           opts.Stdin,
+		Stdout:          opts.Stdout,
+		Stderr:          opts.Stderr,
+		Fs:              opts.Fs,
 	}
 
 	m.run(opts.Args)

internal/cli/fetch.go

@@ -361,7 +361,7 @@ func (mfa *CLIApp) fetchManifestOperation(
 		firstDelay: firstRetryDelay,
 	}
 
-	manifestData, files, err := fetchManifest(ctx, cmd, client, manifestURL)
+	manifestData, files, err := mfa.fetchManifest(ctx, cmd, client, manifestURL)
 	if err != nil {
 		return err
 	}
@@ -426,22 +426,22 @@ func (mfa *CLIApp) fetchManifestOperation(
 // that lists a file where fetch writes another or a mode outside 0777. It
 // returns the manifest as downloaded, to be saved once the files are in
 // place, and the files it lists.
-func fetchManifest(
+func (mfa *CLIApp) fetchManifest(
 	ctx context.Context, cmd *cli.Command, client retryingClient, manifestURL string,
 ) ([]byte, []*mfer.MFFilePath, error) {
 	log.Infof("fetching manifest from %s", manifestURL)
 
 	// Read the whole manifest before parsing it, so that a connection
 	// lost partway through is retried rather than reported as a bad
-	// manifest. One byte past mfer.MaxManifestSize is enough for
-	// mfer.NewManifestFromReader to refuse a manifest that is too large.
+	// manifest. Reading stops one byte past mfa.maxManifestSize, which is
+	// enough to tell that the manifest is too large.
 	var manifestData []byte
 
 	err := client.get(ctx, manifestURL, func(resp *http.Response) error {
 		var readErr error
 
 		manifestData, readErr = io.ReadAll(
-			io.LimitReader(resp.Body, mfer.MaxManifestSize+1))
+			io.LimitReader(resp.Body, mfa.maxManifestSize+1))
 
 		return readErr
 	})
@@ -449,6 +449,11 @@ func fetchManifest(
 		return nil, nil, fmt.Errorf("failed to fetch manifest: %w", err)
 	}
 
+	if int64(len(manifestData)) > mfa.maxManifestSize {
+		return nil, nil, fmt.Errorf("failed to fetch manifest: %w of %d bytes",
+			errManifestTooLarge, mfa.maxManifestSize)
+	}
+
 	// Parse manifest
 	//nolint:contextcheck // mfer loads a manifest without a context
 	manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData))

internal/cli/fetch_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	urfcli "github.com/urfave/cli/v3"
 	"google.golang.org/protobuf/proto"
 	"sneak.berlin/go/mfer/mfer"
 )
@@ -441,33 +442,6 @@ func TestFetchSizeMismatch(t *testing.T) {
 		"temp file should be cleaned up on size mismatch")
 }
 
-// TestFetchStopsReadingFilePastListedSize serves 1 MiB for a file the
-// manifest lists at 16 bytes. fetch must stop reading one byte past the
-// listed size, report the size mismatch and remove its temp file.
-//
-//nolint:paralleltest // changes the process-global working directory
-func TestFetchStopsReadingFilePastListedSize(t *testing.T) {
-	content := []byte("sixteen bytes...")
-
-	manifest, err := mfer.NewManifestFromReader(bytes.NewReader(
-		manifestOf(t, map[string][]byte{testFileTxt: content})))
-	require.NoError(t, err)
-
-	server := httptest.NewServer(
-		http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
-			_, _ = w.Write(make([]byte, 1<<20))
-		}))
-	defer server.Close()
-
-	chdirTemp(t)
-
-	err = downloadFile(context.Background(), testClient(),
-		server.URL+"/"+testFileTxt, ".", testFileTxt, manifest.Files()[0], nil)
-	require.ErrorIs(t, err, errSizeMismatch)
-	require.EqualError(t, err, "size mismatch: expected 16 bytes, got 17")
-	assert.NoFileExists(t, tempPathFor(testFileTxt))
-}
-
 // zeros is an io.Reader of zero bytes without end.
 type zeros struct{}
 
@@ -477,35 +451,59 @@ func (zeros) Read(p []byte) (int, error) {
 	return len(p), nil
 }
 
-// TestManifestDownloadStopsPastLimit serves a manifest 32 MiB larger than
-// mfer.MaxManifestSize to fetch and to check. Each must stop reading it
-// once it passes the maximum, fail with an error naming the maximum and
-// leave no temp file. The connection's buffers hold less than 32 MiB, so a
-// client that stops reading leaves the server unable to send all of it.
-func TestManifestDownloadStopsPastLimit(t *testing.T) {
-	size := mfer.MaxManifestSize + 32<<20
-	limit := strconv.FormatInt(mfer.MaxManifestSize, 10)
+// TestFetchStopsReadingFilePastListedSize serves a body that never ends
+// for a file listed at 16 bytes. fetch must stop reading one byte past
+// the listed size, report the size mismatch and remove its temp file.
+//
+//nolint:paralleltest // changes the process-global working directory
+func TestFetchStopsReadingFilePastListedSize(t *testing.T) {
+	server := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			_, _ = io.Copy(w, zeros{})
+		}))
+	defer server.Close()
+
+	chdirTemp(t)
+
+	err := downloadFile(context.Background(), testClient(),
+		server.URL+"/"+testFileTxt, ".", testFileTxt,
+		&mfer.MFFilePath{Path: testFileTxt, Size: 16}, nil)
+	require.ErrorIs(t, err, errSizeMismatch)
+	require.EqualError(t, err, "size mismatch: expected 16 bytes, got 17")
+	assert.NoFileExists(t, tempPathFor(testFileTxt))
+}
+
+// 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.
+func TestManifestDownloadStopsAtLimit(t *testing.T) {
+	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)
 
-	for _, command := range []string{cmdFetch, cmdCheck} {
-		var sent atomic.Int64
+	mfa := &CLIApp{Fs: afero.NewOsFs(), maxManifestSize: 64 << 10}
 
-		server := httptest.NewServer(
-			http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
-				n, _ := io.CopyN(w, zeros{}, size)
-				sent.Store(n)
-			}))
+	fetch := mfa.fetchCommand()
+	fetch.Action = mfa.fetchManifestOperation
 
-		opts := testOpts([]string{testApp, command, "-q", server.URL + "/index.mf"},
-			afero.NewOsFs())
-		assert.Equal(t, 1, runCLI(opts), command)
-		assert.Contains(t, testStderr(t, opts),
-			"maximum allowed size of "+limit+" bytes", command)
+	check := mfa.checkCommand()
+	check.Action = mfa.checkManifestOperation
 
-		server.Close()
-		assert.Less(t, sent.Load(), size, "%s read the whole manifest", command)
+	for _, cmd := range []*urfcli.Command{fetch, check} {
+		// Both operations log to the process-global logger.
+		err := runLocked(func() error {
+			return cmd.Run(context.Background(),
+				[]string{cmd.Name, server.URL + "/index.mf"})
+		})
+		require.ErrorIs(t, err, errManifestTooLarge, cmd.Name)
+		require.ErrorContains(t, err,
+			"maximum allowed size of 65536 bytes", cmd.Name)
 	}
 
 	leftover, err := os.ReadDir(tmpDir)

internal/cli/manifest_loader.go

@@ -23,6 +23,10 @@ const manifestFetchTimeout = 30 * time.Second
 // its message.
 var errHTTPStatus = errors.New("HTTP")
 
+// errManifestTooLarge indicates a manifest download that passed
+// CLIApp.maxManifestSize.
+var errManifestTooLarge = errors.New("manifest exceeds maximum allowed size")
+
 // isHTTPURL returns true if the string starts with http:// or https://.
 func isHTTPURL(s string) bool {
 	return strings.HasPrefix(s, "http://") || strings.HasPrefix(s, "https://")

internal/cli/mfer.go

@@ -59,6 +59,10 @@ type CLIApp struct {
 	exitCode    int
 	app         *cli.Command
 
+	// maxManifestSize is the most of a manifest that fetch, and check
+	// given a URL, download: mfer.MaxManifestSize, which tests lower.
+	maxManifestSize int64
+
 	Stdin  io.Reader // Standard input stream
 	Stdout io.Writer // Standard output stream for normal output
 	Stderr io.Writer // Standard error stream for diagnostics

mfer/constants.go

@@ -8,7 +8,7 @@ const (
 	ReleaseDate = "2025-12-17"
 
 	// MaxDecompressedSize is the maximum allowed size of decompressed manifest
-	// data (256 MB). This prevents decompression bombs from consuming excessive
+	// data (256 MiB). This prevents decompression bombs from consuming excessive
 	// memory.
 	MaxDecompressedSize int64 = 256 * 1024 * 1024
 

mfer/deserialize.go

@@ -316,15 +316,11 @@ func validateMagic(dat []byte) bool {
 func NewManifestFromReader(input io.Reader) (*manifest, error) {
 	m := &manifest{}
 
-	dat, err := io.ReadAll(io.LimitReader(input, MaxManifestSize+1))
+	dat, err := readAtMost(input, MaxManifestSize)
 	if err != nil {
 		return nil, err
 	}
 
-	if int64(len(dat)) > MaxManifestSize {
-		return nil, fmt.Errorf("%w of %d bytes", errManifestTooLarge, MaxManifestSize)
-	}
-
 	if !validateMagic(dat) {
 		return nil, errInvalidFileFormat
 	}
@@ -352,6 +348,21 @@ func NewManifestFromReader(input io.Reader) (*manifest, error) {
 	return m, nil
 }
 
+// readAtMost reads all of input, or refuses it with errManifestTooLarge
+// once it passes maxSize bytes, after reading one byte past maxSize.
+func readAtMost(input io.Reader, maxSize int64) ([]byte, error) {
+	dat, err := io.ReadAll(io.LimitReader(input, maxSize+1))
+	if err != nil {
+		return nil, err
+	}
+
+	if int64(len(dat)) > maxSize {
+		return nil, fmt.Errorf("%w of %d bytes", errManifestTooLarge, maxSize)
+	}
+
+	return dat, nil
+}
+
 // ManifestFromFileOptions configures NewManifestFromFile.
 type ManifestFromFileOptions struct {
 	// Path is the manifest file to read (required).

mfer/deserialize_test.go

@@ -2,32 +2,32 @@
 package mfer
 
 import (
-	"io"
+	"bytes"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-// zeros is an io.Reader of zero bytes without end.
-type zeros struct{}
+// TestReadAtMost gives readAtMost exactly its maximum, which it must
+// return whole, and twice its maximum, which it must refuse after reading
+// one byte past the maximum, and no more. NewManifestFromReader reads
+// through it with MaxManifestSize; the test uses 64 KiB, since reading
+// MaxManifestSize under the race detector takes gigabytes of memory.
+func TestReadAtMost(t *testing.T) {
+	t.Parallel()
 
-func (zeros) Read(p []byte) (int, error) {
-	clear(p)
+	const maxSize = 64 << 10
 
-	return len(p), nil
-}
+	dat, err := readAtMost(bytes.NewReader(make([]byte, maxSize)), maxSize)
+	require.NoError(t, err)
+	assert.Len(t, dat, maxSize)
 
-// TestNewManifestFromReaderRefusesTooLarge gives NewManifestFromReader
-// 1 MiB more than MaxManifestSize. It must refuse the manifest after
-// reading one byte past the maximum, and no more.
-//
-//nolint:paralleltest // holds 258 MiB; kept apart from other large reads
-func TestNewManifestFromReaderRefusesTooLarge(t *testing.T) {
-	size := MaxManifestSize + 1<<20
-	input := &io.LimitedReader{R: zeros{}, N: size}
+	input := bytes.NewReader(make([]byte, 2*maxSize))
 
-	_, err := NewManifestFromReader(input)
+	_, err = readAtMost(input, maxSize)
 	require.ErrorIs(t, err, errManifestTooLarge)
-	assert.Equal(t, MaxManifestSize+1, size-input.N, "bytes read")
+	require.EqualError(t, err,
+		"manifest exceeds maximum allowed size of 65536 bytes")
+	assert.Equal(t, maxSize-1, input.Len(), "bytes left unread")
 }