AddFileWithHash rejects hashes that are not multihashes (closes #129)

AddFileWithHash took any non-empty bytes as a hash, so the builder could
write a manifest that mfer refuses to load. It now decodes the hash with
go-multihash and also requires a digest of at least 32 bytes, the SHA-256
length the reader's decoding-cost limit assumes: a valid but shorter
multihash, such as an empty identity hash or SHA-1, still makes a
manifest of one-character paths too costly to load. When mfer freshen
meets such a hash in an existing manifest, its error names the manifest
entry and says to regenerate the manifest with mfer generate. Test
fixtures whose stand-in hashes were not valid multihashes now use a
SHA-256 multihash.

Model: opus-5-5

Dockerfile

@@ -8,9 +8,6 @@ RUN go mod download
 
 COPY . .
 
-# Touch .pb.go so make does not try to regenerate via protoc (file is committed)
-RUN touch mfer/mf.pb.go
-
 # Go half of fmt-check only: this image has no node, so no prettier. The
 # markdown half runs in the mdfmt stage below.
 RUN make fmt-check-go
@@ -46,9 +43,6 @@ RUN go mod download
 
 COPY . .
 
-# Touch .pb.go so make does not try to regenerate via protoc (file is committed)
-RUN touch mfer/mf.pb.go
-
 RUN make test
 
 # A build context sent as a tar archive, as upaas sends it, keeps its files'

Makefile

@@ -2,7 +2,6 @@ export DOCKER_BUILDKIT := 1
 export PROGRESS_NO_TRUNC := 1
 GOPATH := $(shell go env GOPATH)
 export PATH := $(PATH):$(GOPATH)/bin
-PROTOC_GEN_GO := $(GOPATH)/bin/protoc-gen-go
 SOURCEFILES := mfer/*.go mfer/*.proto internal/*/*.go cmd/*/*.go go.mod go.sum
 ARCH := $(shell uname -m)
 GITREV_BUILD := $(shell bash $(PWD)/bin/gitrev.sh 2>/dev/null || echo unknown)
@@ -13,7 +12,7 @@ GOLDFLAGS += -X main.Version=$(VERSION)
 GOLDFLAGS += -X main.Gitrev=$(GITREV_BUILD)
 GOFLAGS := -ldflags "$(GOLDFLAGS)"
 
-.PHONY: bootstrap setup docker default run ci test fuzz check lint fmt fmt-check fmt-check-go fmt-check-md hooks fixme
+.PHONY: bootstrap setup docker default run ci test fuzz check lint fmt fmt-check fmt-check-go fmt-check-md hooks fixme generate
 
 default: fmt test
 
@@ -35,9 +34,6 @@ test:
 fuzz:
 	@script/fuzz
 
-$(PROTOC_GEN_GO):
-	test -e $(PROTOC_GEN_GO) || go install -v google.golang.org/protobuf/cmd/protoc-gen-go@v1.28.1
-
 fixme:
 	@grep -nir fixme . | grep -v Makefile
 
@@ -58,15 +54,14 @@ fmt-check-md:
 hooks:
 	@script/install-precommit
 
-mfer/mf.pb.go: mfer/mf.proto
+generate:
-	cd mfer && go generate .
+	@script/generate
 
-bin/mfer: $(SOURCEFILES) mfer/mf.pb.go
+bin/mfer: $(SOURCEFILES)
-	protoc --version
 	cd cmd/mfer && go build -tags urfave_cli_no_docs -o ../../bin/mfer $(GOFLAGS) .
 
 clean:
-	rm -rfv mfer/*.pb.go bin/mfer cmd/mfer/mfer *.dockerimage
+	rm -rfv bin/mfer cmd/mfer/mfer *.dockerimage
 
 fmt:
 	@script/fmt

README.md

@@ -72,8 +72,19 @@ provide:
   `script/bootstrap`, then `script/install-precommit`
 - `script/projectname` — output the project name (`mfer`); used by other scripts
   such as `script/docker`
-- `script/test` — run the test suite (`go test`), regenerating the protobuf code
+- `script/test` — run the test suite (`go test`); one test fails when
-  first if it is stale
+  `mfer/mf.proto` no longer matches the hash `script/generate` recorded
+- `script/generate` (`make generate`) — regenerate `mfer/mf.pb.go` from
+  `mfer/mf.proto` and record the hash of that `mfer/mf.proto` in
+  `mfer/mf.proto.sha256`; the only thing that regenerates the committed
+  `mfer/mf.pb.go`. It needs the exact versions that wrote the committed file,
+  and refuses to run with any other: `protoc` 33.4 (unpack
+  `protoc-33.4-<platform>.zip` from
+  [its release](https://github.com/protocolbuffers/protobuf/releases/tag/v33.4)
+  and put its `bin/protoc` on `PATH`) and `protoc-gen-go` v1.36.11
+  (`go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.36.11`, which
+  installs it in `$(go env GOPATH)/bin`; `make generate` adds that directory to
+  `PATH`)
 - `script/fuzz` — fuzz the manifest parser for one minute; run by hand
   (`make fuzz`), never by CI, while `script/test` runs its committed seed corpus
   as ordinary tests

internal/cli/freshen.go

@@ -615,7 +615,15 @@ func addExistingToBuilder(b *mfer.Builder, entry *mfer.MFFilePath) error {
 		return nil
 	}
 
-	return b.AddFileWithHash(mfer.RelFilePath(entry.GetPath()),
+	err := b.AddFileWithHash(mfer.RelFilePath(entry.GetPath()),
 		mfer.FileSize(entry.GetSize()), mfer.ModTime(mtime),
 		entry.GetHashes()[0].GetMultiHash())
+	if err != nil {
+		return fmt.Errorf(
+			"manifest entry %s: %w (regenerate the manifest with mfer generate)",
+			entry.GetPath(), err,
+		)
+	}
+
+	return nil
 }

internal/cli/freshen_test.go

@@ -4,10 +4,12 @@ package cli
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
 	"os"
 	"testing"
 	"time"
 
+	"github.com/multiformats/go-multihash"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -175,21 +177,50 @@ func TestFreshenRecordEntryMtimePresence(t *testing.T) {
 func TestFreshenAddExistingRejectsMissingMtime(t *testing.T) {
 	t.Parallel()
 
+	hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256)
+	require.NoError(t, err)
+
 	b := mfer.NewBuilder()
 	entry := &mfer.MFFilePath{
 		Path:  "file1.txt",
 		Size:  8,
 		Mtime: nil,
 		Hashes: []*mfer.MFFileChecksum{
-			{MultiHash: []byte{0x12, 0x20}},
+			{MultiHash: hash},
 		},
 	}
 
-	err := addExistingToBuilder(b, entry)
+	err = addExistingToBuilder(b, entry)
 	require.ErrorIs(t, err, errEntryMissingMtime)
 	assert.Contains(t, err.Error(), "file1.txt")
 }
 
+// TestFreshenAddExistingRejectsShortHash pins that an existing manifest
+// entry whose hash the builder refuses is reported as a problem with the
+// manifest, with the command that fixes it.
+func TestFreshenAddExistingRejectsShortHash(t *testing.T) {
+	t.Parallel()
+
+	sha1Hash, err := multihash.Encode(make([]byte, 20), multihash.SHA1)
+	require.NoError(t, err)
+
+	b := mfer.NewBuilder()
+	entry := &mfer.MFFilePath{
+		Path:  "old.txt",
+		Size:  8,
+		Mtime: &mfer.Timestamp{Seconds: 1_700_000_000},
+		Hashes: []*mfer.MFFileChecksum{
+			{MultiHash: sha1Hash},
+		},
+	}
+
+	err = addExistingToBuilder(b, entry)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "manifest entry old.txt")
+	assert.Contains(t, err.Error(), "mfer generate")
+	assert.Zero(t, b.FileCount())
+}
+
 // TestEntryMtime pins the presence semantics the callers depend on.
 func TestEntryMtime(t *testing.T) {
 	t.Parallel()

mfer/builder.go

@@ -35,7 +35,8 @@ var (
 	errPathDotDot       = errors.New("contains '..' segment")
 	errSizeMismatch     = errors.New("size mismatch")
 	errNegativeSize     = errors.New("size cannot be negative")
-	errEmptyHash        = errors.New("hash cannot be nil or empty")
+	errHashNotMultihash = errors.New("hash is not a valid multihash")
+	errHashTooShort     = errors.New("hash digest is too short")
 )
 
 // ValidatePath checks that a file path conforms to manifest path invariants:
@@ -228,7 +229,8 @@ func (b *Builder) FileCount() int {
 
 // AddFileWithHash adds a file entry with a pre-computed hash.
 // This is useful when the hash is already known (e.g., from an existing manifest).
-// Returns an error if path is empty, size is negative, or hash is nil/empty.
+// Returns an error if path is invalid, size is negative, or hash is not a
+// multihash with a digest of at least 32 bytes, as long as SHA-256's.
 func (b *Builder) AddFileWithHash(
 	path RelFilePath,
 	size FileSize,
@@ -244,8 +246,19 @@ func (b *Builder) AddFileWithHash(
 		return errNegativeSize
 	}
 
-	if len(hash) == 0 {
+	decoded, err := multihash.Decode(hash)
-		return errEmptyHash
+	if err != nil {
+		return fmt.Errorf("%w: %w", errHashNotMultihash, err)
+	}
+
+	// The reader's limit on decoding cost (maxDecodedGrowth) assumes every
+	// hash is at least as long as a SHA-256 multihash, so a manifest of
+	// shorter ones could fail to load.
+	if len(decoded.Digest) < sha256.Size {
+		return fmt.Errorf(
+			"%w: %d bytes, at least %d needed",
+			errHashTooShort, len(decoded.Digest), sha256.Size,
+		)
 	}
 
 	entry := &MFFilePath{

mfer/builder_test.go

@@ -4,12 +4,14 @@ package mfer
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
 	"fmt"
 	"path/filepath"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/multiformats/go-multihash"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -44,9 +46,10 @@ func TestBuilderAddFileWithHash(t *testing.T) {
 	t.Parallel()
 
 	b := NewBuilder()
-	hash := make([]byte, 34) // SHA256 multihash is 34 bytes
+	hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256)
+	require.NoError(t, err)
 
-	err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), hash)
+	err = b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), hash)
 	require.NoError(t, err)
 	assert.Equal(t, 1, b.FileCount())
 }
@@ -54,12 +57,14 @@ func TestBuilderAddFileWithHash(t *testing.T) {
 func TestBuilderAddFileWithHashValidation(t *testing.T) {
 	t.Parallel()
 
+	sha256Hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256)
+	require.NoError(t, err)
+
 	t.Run("empty path", func(t *testing.T) {
 		t.Parallel()
 
 		b := NewBuilder()
-		hash := make([]byte, 34)
+		err := b.AddFileWithHash("", 100, ModTime(time.Now()), sha256Hash)
-		err := b.AddFileWithHash("", 100, ModTime(time.Now()), hash)
 		require.Error(t, err)
 		assert.Contains(t, err.Error(), "path")
 	})
@@ -68,41 +73,53 @@ func TestBuilderAddFileWithHashValidation(t *testing.T) {
 		t.Parallel()
 
 		b := NewBuilder()
-		hash := make([]byte, 34)
+		err := b.AddFileWithHash("test.txt", -1, ModTime(time.Now()), sha256Hash)
-		err := b.AddFileWithHash("test.txt", -1, ModTime(time.Now()), hash)
 		require.Error(t, err)
 		assert.Contains(t, err.Error(), "size")
 	})
 
-	t.Run("nil hash", func(t *testing.T) {
-		t.Parallel()
-
-		b := NewBuilder()
-		err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), nil)
-		require.Error(t, err)
-		assert.Contains(t, err.Error(), "hash")
-	})
-
-	t.Run("empty hash", func(t *testing.T) {
-		t.Parallel()
-
-		b := NewBuilder()
-		err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), []byte{})
-		require.Error(t, err)
-		assert.Contains(t, err.Error(), "hash")
-	})
-
 	t.Run("valid inputs", func(t *testing.T) {
 		t.Parallel()
 
 		b := NewBuilder()
-		hash := make([]byte, 34)
+		err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), sha256Hash)
-		err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), hash)
 		require.NoError(t, err)
 		assert.Equal(t, 1, b.FileCount())
 	})
 }
 
+func TestBuilderAddFileWithHashRejectsBadHashes(t *testing.T) {
+	t.Parallel()
+
+	sha1Hash, err := multihash.Encode(make([]byte, 20), multihash.SHA1)
+	require.NoError(t, err)
+
+	tests := []struct {
+		name string
+		hash Multihash
+		want error
+	}{
+		{"nil hash", nil, errHashNotMultihash},
+		{"empty hash", []byte{}, errHashNotMultihash},
+		{"one-byte hash", []byte{0x12}, errHashNotMultihash},
+		// A SHA-256 code and 32-byte length, then only two bytes of digest.
+		{"malformed multihash", []byte{0x12, 0x20, 0x01, 0x02}, errHashNotMultihash},
+		// A valid multihash, but its 20-byte SHA-1 digest is too short.
+		{"SHA-1 multihash", sha1Hash, errHashTooShort},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			b := NewBuilder()
+			err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), tt.hash)
+			require.ErrorIs(t, err, tt.want)
+			assert.Equal(t, 0, b.FileCount())
+		})
+	}
+}
+
 func TestBuilderBuild(t *testing.T) {
 	t.Parallel()
 
@@ -358,7 +375,8 @@ func TestBuilderBuildRoundTrip(t *testing.T) {
 func TestBuilderBuildRoundTripLargeManifest(t *testing.T) {
 	t.Parallel()
 
-	hash := make([]byte, 34) // multihash: 2-byte prefix + 32-byte SHA-256
+	hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256)
+	require.NoError(t, err)
 
 	b := NewBuilder()
 

mfer/constants.go

@@ -36,7 +36,8 @@ const (
 	decodedMIMETypeSize  = 16
 
 	// Each file entry mfer writes holds a path of at least one byte, a
-	// 34-byte SHA-256 multihash and a modification time: at least 47 bytes,
+	// multihash at least as long as SHA-256's 34 bytes (AddFileWithHash
+	// refuses shorter ones) and a modification time: at least 47 bytes,
 	// counted at 336. So its manifests add up to at most about 7.15 times
 	// their size, and this limit is about 12% above that.
 	maxDecodedGrowth = 8

mfer/deserialize_path_test.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/google/uuid"
 	"github.com/klauspost/compress/zstd"
+	"github.com/multiformats/go-multihash"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"google.golang.org/protobuf/encoding/protowire"
@@ -205,7 +206,8 @@ func TestDeserializeDropsUnknownFields(t *testing.T) {
 func TestDeserializeLoadsDensestManifest(t *testing.T) {
 	t.Parallel()
 
-	hash := make([]byte, 34) // multihash: 2-byte prefix + 32-byte SHA-256
+	hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256)
+	require.NoError(t, err)
 
 	b := NewBuilder()
 	b.SetIncludeTimestamps(true)
@@ -227,7 +229,8 @@ func TestDeserializeLoadsDensestManifest(t *testing.T) {
 func TestDeserializeValidManifestRoundTrips(t *testing.T) {
 	t.Parallel()
 
-	hash := make([]byte, 34) // multihash: 2-byte prefix + 32-byte SHA-256
+	hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256)
+	require.NoError(t, err)
 
 	b := NewBuilder()
 	require.NoError(t, b.AddFileWithHash("dir/file.txt", 123, ModTime{}, hash))

mfer/mf.proto.sha256

@@ -0,0 +1 @@
+103901c42b94396aa7ae128fd503ef693a4b7a03b2169481f25fda3d2c254e00  mf.proto

mfer/mf_test.go

@@ -0,0 +1,29 @@
+package mfer_test
+
+import (
+	"crypto/sha256"
+	"encoding/hex"
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// mf.pb.go is generated from mf.proto and committed. `make generate`
+// records the hash of the mf.proto it generated from in mf.proto.sha256.
+func TestGeneratedCodeMatchesProto(t *testing.T) {
+	t.Parallel()
+
+	proto, err := os.ReadFile("mf.proto")
+	require.NoError(t, err)
+
+	recorded, err := os.ReadFile("mf.proto.sha256")
+	require.NoError(t, err)
+
+	recordedHash, _, _ := strings.Cut(string(recorded), " ")
+	sum := sha256.Sum256(proto)
+	require.Equal(t, recordedHash, hex.EncodeToString(sum[:]),
+		"mfer/mf.proto has changed since mfer/mf.pb.go was generated "+
+			"from it: run `make generate` and commit the result")
+}

script/fmt

@@ -5,19 +5,8 @@ set -eu
 SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)"
 ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)"
 
-# Regenerate mfer/mf.pb.go from mfer/mf.proto if it is missing or stale
-# (mirrors the old Makefile prerequisite; the generated file is
-# committed, so this is normally a no-op).
-ensure_pb() {
-    if [ ! -f mfer/mf.pb.go ] ||
-        [ -n "$(find mfer/mf.proto -newer mfer/mf.pb.go 2>/dev/null)" ]; then
-        (cd mfer && go generate .)
-    fi
-}
-
 main() {
     cd "$ROOT"
-    ensure_pb
     gofumpt -l -w mfer internal cmd
     # Markdown and JSON, over the same file set script/fmt-check verifies.
     "$SCRIPT_DIR/prettier" --write

script/fmt-check-go

@@ -6,19 +6,8 @@ set -eu
 
 ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
 
-# Regenerate mfer/mf.pb.go from mfer/mf.proto if it is missing or stale
-# (mirrors the old Makefile prerequisite; the generated file is
-# committed, so this is normally a no-op).
-ensure_pb() {
-    if [ ! -f mfer/mf.pb.go ] ||
-        [ -n "$(find mfer/mf.proto -newer mfer/mf.pb.go 2>/dev/null)" ]; then
-        (cd mfer && go generate .)
-    fi
-}
-
 main() {
     cd "$ROOT"
-    ensure_pb
     if [ -n "$(gofmt -l .)" ]; then
         echo "gofmt: files need formatting:" >&2
         gofmt -l . >&2

script/generate

@@ -0,0 +1,52 @@
+#!/bin/sh
+# script/generate: regenerate mfer/mf.pb.go from mfer/mf.proto, and record
+# the hash of that mf.proto in mfer/mf.proto.sha256. Nothing else
+# regenerates mf.pb.go: it is committed, so building and checking need no
+# protoc. A test fails while mf.proto no longer matches the recorded hash.
+#
+# Needs exactly the protoc and protoc-gen-go versions named in the header of
+# the committed mf.pb.go (README.md says how to install them). Another
+# version writes a different mf.pb.go, so the script refuses to run.
+set -eu
+
+ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
+
+# protoc 33.4 names itself v6.33.4 in the mf.pb.go header.
+PROTOC_VERSION="33.4"
+PROTOC_GEN_GO_VERSION="v1.36.11"
+
+# require_version <command> <its exact --version output>
+require_version() {
+    actual="$("$1" --version 2>/dev/null || true)"
+    if [ "$actual" != "$2" ]; then
+        echo "generate: needs $2 on PATH, found: ${actual:-none}" >&2
+        echo "  README.md says how to install it." >&2
+        exit 1
+    fi
+}
+
+# sha256 <file>: print "<hash>  <file>", with sha256sum, or with shasum
+# where there is no sha256sum.
+sha256() {
+    if command -v sha256sum >/dev/null 2>&1; then
+        sha256sum "$1"
+    elif command -v shasum >/dev/null 2>&1; then
+        shasum -a 256 "$1"
+    else
+        echo "generate: needs sha256sum or shasum on PATH" >&2
+        exit 1
+    fi
+}
+
+main() {
+    cd "$ROOT/mfer"
+    require_version protoc "libprotoc $PROTOC_VERSION"
+    require_version protoc-gen-go "protoc-gen-go $PROTOC_GEN_GO_VERSION"
+    # Hashed before regenerating, so a missing hash tool stops the script
+    # before it changes anything. Regenerating leaves mf.proto as it is.
+    proto_hash="$(sha256 mf.proto)"
+    go generate .
+    echo "$proto_hash" >mf.proto.sha256
+}
+
+main "$@"

script/test

@@ -4,19 +4,8 @@ set -eu
 
 ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
 
-# Regenerate mfer/mf.pb.go from mfer/mf.proto if it is missing or stale
-# (mirrors the old Makefile prerequisite; the generated file is
-# committed, so this is normally a no-op).
-ensure_pb() {
-    if [ ! -f mfer/mf.pb.go ] ||
-        [ -n "$(find mfer/mf.proto -newer mfer/mf.pb.go 2>/dev/null)" ]; then
-        (cd mfer && go generate .)
-    fi
-}
-
 main() {
     cd "$ROOT"
-    ensure_pb
     go test -timeout 30s -race -cover ./... ||
         {
             echo "--- Rerunning with -v for details ---"