diff --git a/TODO.md b/TODO.md index 06f9dba..cea66c6 100644 --- a/TODO.md +++ b/TODO.md @@ -24,6 +24,9 @@ only thing left of the `chore/align-repo-policies` branch is the list below. # Completed Steps +- 2026-09-21: pinned CLI error messages by driving their real call sites in + `internal/cli/errmsg_test.go`, and made the freshen mtime-presence test + distinguish an absent mtime from the epoch (#87) - 2026-08-09: added `.prettierrc`/`.prettierignore`, gave `script/fmt` and `script/fmt-check` one shared prettier file set via `script/prettier`, dropped the `|| true` that hid prettier failures, and added a node-based Dockerfile diff --git a/internal/cli/errmsg_test.go b/internal/cli/errmsg_test.go index 873658a..8ad2875 100644 --- a/internal/cli/errmsg_test.go +++ b/internal/cli/errmsg_test.go @@ -2,167 +2,326 @@ package cli import ( - "fmt" + "bytes" + "context" + "flag" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" "testing" + "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + urfcli "github.com/urfave/cli/v2" + "sneak.berlin/go/mfer/mfer" ) -// errMsgCase is one pinned user-visible error message. -type errMsgCase struct { - name string - err error - want string -} +// These tests pin the exact rendered text of the CLI's user-visible error +// messages. The messages are grepped for in CI pipelines and quoted in bug +// reports, so a reword is a deliberate change, never a refactoring side +// effect. +// +// Every case drives the real function that emits the message and asserts on +// what it returns. No production format string is restated here: a test that +// only re-rendered a copied format string would keep passing after the real +// message changed, which is exactly the regression these tests exist to +// catch. +// Full 40-hex fingerprints used where a message embeds one. const ( msgFpA = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" msgFpB = "BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB" ) -func checkErrMsgCases(t *testing.T, cases []errMsgCase) { +// runLocked runs fn while holding runMu, so operations that write to the +// process-global logger do not race the other CLI runs. +func runLocked(fn func() error) error { + runMu.Lock() + defer runMu.Unlock() + + return fn() +} + +// unsignedChecker builds a Checker over a freshly scanned, unsigned manifest. +func unsignedChecker(t *testing.T) *mfer.Checker { t.Helper() - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - assert.Equal(t, tc.want, tc.err.Error()) - }) - } + fs := afero.NewMemMapFs() + require.NoError(t, fs.MkdirAll("/d", 0o755)) + require.NoError(t, afero.WriteFile(fs, "/d/f.txt", []byte("hi"), 0o644)) + + s := mfer.NewScannerWithOptions(&mfer.ScannerOptions{Fs: fs}) + require.NoError(t, s.EnumeratePath("/d", nil)) + + var buf bytes.Buffer + + require.NoError(t, s.ToManifest(context.Background(), &buf, nil)) + require.NoError(t, afero.WriteFile(fs, "/d/index.mf", buf.Bytes(), 0o644)) + + chk, err := mfer.NewChecker("/d/index.mf", "/d", fs) + require.NoError(t, err) + require.False(t, chk.IsSigned()) + + return chk } -// TestErrorMessagesVerbatim pins the exact rendered text of the CLI's -// user-visible error messages. +func TestNoManifestFoundMessage(t *testing.T) { + t.Parallel() + + _, err := findManifest(afero.NewMemMapFs(), "/tmp/x") + require.ErrorIs(t, err, errNoManifestFound) + assert.EqualError(t, err, + "no manifest found in /tmp/x (looked for index.mf and .index.mf)") +} + +func TestVerifyRequiredSignerMessages(t *testing.T) { + t.Parallel() + + t.Run("invalid fingerprint length", func(t *testing.T) { + t.Parallel() + + err := verifyRequiredSigner(unsignedChecker(t), "12345678") + require.ErrorIs(t, err, errInvalidFingerprint) + assert.EqualError(t, err, + "invalid fingerprint: must be exactly 40 hex characters, got 8") + }) + + t.Run("manifest not signed", func(t *testing.T) { + t.Parallel() + + err := verifyRequiredSigner(unsignedChecker(t), msgFpA) + require.ErrorIs(t, err, errManifestNotSigned) + assert.EqualError(t, err, + "manifest is not signed, but signature from "+msgFpA+" is required") + }) +} + +// TestSignerMismatchMessage drives verifyRequiredSigner against a real signed +// manifest. The embedded fingerprint is whatever the generated key produced, +// so it is read back from the checker and substituted into the expected +// string; the required signer is a fixed value that cannot match it. Requires +// gpg and is skipped where it is absent, as the other signing tests are. // -// These strings are an interface: they are grepped for in CI pipelines -// and quoted in bug reports. The messages are assembled by wrapping -// static sentinels, and it is easy to change what a user sees while -// only meaning to make an error matchable with errors.Is - which is -// precisely what happened once already. Any change to a string below is -// therefore a deliberate, separately stated change, never a side effect -// of a refactor. -func TestErrorMessagesVerbatim(t *testing.T) { +//nolint:paralleltest // signedChecker calls t.Setenv, which bars t.Parallel +func TestSignerMismatchMessage(t *testing.T) { + chk := signedChecker(t) + + embeddedFP, err := chk.ExtractEmbeddedSigningKeyFP() + require.NoError(t, err) + + err = verifyRequiredSigner(chk, msgFpB) + require.ErrorIs(t, err, errSignerMismatch) + assert.EqualError(t, err, + "embedded signing key fingerprint "+embeddedFP+ + " does not match required "+msgFpB) +} + +// signedChecker builds a Checker over a manifest signed by a throwaway GPG +// key generated in a temporary GNUPGHOME. +func signedChecker(t *testing.T) *mfer.Checker { + t.Helper() + + _, err := exec.LookPath("gpg") + if err != nil { + t.Skip("gpg not installed, skipping signing test") + } + + gpgHome := t.TempDir() + params := "%no-protection\n" + + "Key-Type: RSA\nKey-Length: 2048\n" + + "Name-Real: MFER Test Key\nName-Email: test@mfer.test\n" + + "Expire-Date: 0\n%commit\n" + paramsFile := filepath.Join(gpgHome, "key-params") + require.NoError(t, os.WriteFile(paramsFile, []byte(params), 0o600)) + + //nolint:gosec // paramsFile is a test-controlled path inside t.TempDir() + cmd := exec.CommandContext(context.Background(), "gpg", + "--batch", "--gen-key", paramsFile) + cmd.Env = append(os.Environ(), "GNUPGHOME="+gpgHome) + + out, err := cmd.CombinedOutput() + if err != nil { + t.Skipf("failed to generate test GPG key: %v: %s", err, out) + } + + t.Setenv("GNUPGHOME", gpgHome) + + b := mfer.NewBuilder() + b.SetSigningOptions(&mfer.SigningOptions{KeyID: mfer.GPGKeyID("test@mfer.test")}) + + content := []byte("signed file") + _, err = b.AddFile("f.txt", mfer.FileSize(len(content)), mfer.ModTime{}, + bytes.NewReader(content), nil) + require.NoError(t, err) + + var buf bytes.Buffer + + require.NoError(t, b.Build(&buf)) + + fs := afero.NewMemMapFs() + require.NoError(t, afero.WriteFile(fs, "/index.mf", buf.Bytes(), 0o644)) + + chk, err := mfer.NewChecker("/index.mf", "/", fs) + require.NoError(t, err) + require.True(t, chk.IsSigned()) + + return chk +} + +func TestPathDoesNotExistMessage(t *testing.T) { t.Parallel() - checkErrMsgCases(t, []errMsgCase{ - { - name: "check: no manifest found", - err: fmt.Errorf("%w in %s (looked for index.mf and .index.mf)", - errNoManifestFound, "/tmp/x"), - want: "no manifest found in /tmp/x " + - "(looked for index.mf and .index.mf)", - }, - { - name: "check: invalid fingerprint length", - err: fmt.Errorf("%w, got %d", errInvalidFingerprint, 8), - want: "invalid fingerprint: must be exactly 40 hex characters, got 8", - }, - { - name: "check: manifest not signed", - err: fmt.Errorf("%w, but signature from %s is required", - errManifestNotSigned, msgFpA), - want: "manifest is not signed, but signature from " + msgFpA + - " is required", - }, - { - name: "check: signer mismatch", - err: fmt.Errorf("embedded signing key fingerprint %s %w %s", - msgFpA, errSignerMismatch, msgFpB), - want: "embedded signing key fingerprint " + msgFpA + - " does not match required " + msgFpB, - }, - { - name: "gen: path does not exist", - err: fmt.Errorf("%w: %s", errPathNotExist, "nope"), - want: "path does not exist: nope", - }, - { - name: "gen: output file exists", - err: fmt.Errorf("output file %s %w", "index.mf", errOutputExists), - want: "output file index.mf already exists " + - "(use --force to overwrite)", - }, - { - name: "mfer: unknown command", - err: fmt.Errorf("%w %q", errUnknownCommand, "bogus"), - want: `unknown command "bogus"`, - }, + set := flag.NewFlagSet("gen", flag.ContinueOnError) + require.NoError(t, set.Parse([]string{"nope"})) + + mfa := &CLIApp{Fs: afero.NewMemMapFs()} + ctx := urfcli.NewContext(nil, set, nil) + + _, err := mfa.collectInputPaths(ctx.Args()) + require.ErrorIs(t, err, errPathNotExist) + assert.EqualError(t, err, "path does not exist: nope") +} + +func TestOutputFileExistsMessage(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + require.NoError(t, fs.MkdirAll("/d", 0o755)) + require.NoError(t, afero.WriteFile(fs, "/d/f.txt", []byte("hi"), 0o644)) + require.NoError(t, afero.WriteFile(fs, "/out.mf", []byte("old"), 0o644)) + + set := flag.NewFlagSet("gen", flag.ContinueOnError) + set.String("output", "", "") + set.Bool("force", false, "") + require.NoError(t, set.Parse([]string{"/d"})) + require.NoError(t, set.Set("output", "/out.mf")) + + mfa := &CLIApp{Fs: fs} + ctx := urfcli.NewContext(nil, set, nil) + + // generateManifestOperation writes to the process-global logger during + // enumeration, so serialize with the other CLI runs. + err := runLocked(func() error { return mfa.generateManifestOperation(ctx) }) + require.ErrorIs(t, err, errOutputExists) + assert.EqualError(t, err, + "output file /out.mf already exists (use --force to overwrite)") +} + +func TestManifestLoaderHTTPStatusMessage(t *testing.T) { + t.Parallel() + + server := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer server.Close() + + mfa := &CLIApp{Fs: afero.NewMemMapFs()} + + _, err := mfa.openManifestReader(server.URL + "/foo.mf") + require.ErrorIs(t, err, errHTTPStatus) + assert.EqualError(t, err, + "failed to fetch "+server.URL+"/foo.mf: HTTP 404") +} + +func TestFetchManifestHTTPStatusMessage(t *testing.T) { + t.Parallel() + + server := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer server.Close() + + set := flag.NewFlagSet("fetch", flag.ContinueOnError) + require.NoError(t, set.Parse([]string{server.URL})) + + mfa := &CLIApp{Fs: afero.NewMemMapFs()} + ctx := urfcli.NewContext(nil, set, nil) + + // fetchManifestOperation logs to the process-global logger. + err := runLocked(func() error { return mfa.fetchManifestOperation(ctx) }) + require.ErrorIs(t, err, errHTTPStatus) + assert.EqualError(t, err, "failed to fetch manifest: HTTP 404") +} + +func TestFetchFileHTTPStatusMessage(t *testing.T) { + t.Parallel() + + server := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() + + err := downloadFile(context.Background(), server.URL+"/x", "x", + &mfer.MFFilePath{}, nil) + require.ErrorIs(t, err, errHTTPStatus) + assert.EqualError(t, err, "HTTP 500") +} + +func TestURLRequiredMessage(t *testing.T) { + t.Parallel() + + set := flag.NewFlagSet("fetch", flag.ContinueOnError) + require.NoError(t, set.Parse([]string{})) + + mfa := &CLIApp{Fs: afero.NewMemMapFs()} + ctx := urfcli.NewContext(nil, set, nil) + + // fetchManifestOperation logs to the process-global logger. + err := runLocked(func() error { return mfa.fetchManifestOperation(ctx) }) + require.ErrorIs(t, err, errURLRequired) + assert.EqualError(t, err, "URL argument required") +} + +func TestSanitizePathMessages(t *testing.T) { + t.Parallel() + + t.Run("empty", func(t *testing.T) { + t.Parallel() + + _, err := sanitizePath("") + require.ErrorIs(t, err, errEmptyPath) + assert.EqualError(t, err, "empty path") + }) + + t.Run("absolute", func(t *testing.T) { + t.Parallel() + + _, err := sanitizePath("/etc/passwd") + require.ErrorIs(t, err, errAbsolutePath) + assert.EqualError(t, err, "absolute path not allowed: /etc/passwd") + }) + + t.Run("traversal", func(t *testing.T) { + t.Parallel() + + _, err := sanitizePath("../x") + require.ErrorIs(t, err, errPathTraversal) + assert.EqualError(t, err, "path traversal not allowed: ../x") }) } -// TestFetchErrorMessagesVerbatim pins the fetch and manifest-loader -// messages; see TestErrorMessagesVerbatim for why. -func TestFetchErrorMessagesVerbatim(t *testing.T) { +func TestSizeMismatchMessage(t *testing.T) { t.Parallel() - checkErrMsgCases(t, []errMsgCase{ - { - name: "manifest_loader: http status", - err: fmt.Errorf("failed to fetch %s: %w %d", - "https://example.com/index.mf", errHTTPStatus, 404), - want: "failed to fetch https://example.com/index.mf: HTTP 404", - }, - { - name: "fetch: manifest http status", - err: fmt.Errorf("failed to fetch manifest: %w %d", - errHTTPStatus, 404), - want: "failed to fetch manifest: HTTP 404", - }, - { - name: "fetch: file http status", - err: fmt.Errorf("%w %d", errHTTPStatus, 500), - want: "HTTP 500", - }, - { - name: "fetch: empty path", - err: errEmptyPath, - want: "empty path", - }, - { - name: "fetch: absolute path", - err: fmt.Errorf("%w: %s", errAbsolutePath, "/etc/passwd"), - want: "absolute path not allowed: /etc/passwd", - }, - { - name: "fetch: path traversal", - err: fmt.Errorf("%w: %s", errPathTraversal, "../x"), - want: "path traversal not allowed: ../x", - }, - { - name: "fetch: size mismatch", - err: fmt.Errorf("%w: expected %d bytes, got %d", - errSizeMismatch, 10, 9), - want: "size mismatch: expected 10 bytes, got 9", - }, - { - name: "fetch: url required", - err: errURLRequired, - want: "URL argument required", - }, - { - name: "fetch: hash mismatch", - err: errHashMismatch, - want: "hash mismatch", - }, - }) + // finishDownload returns the size-mismatch error before it touches the + // paths, digest, or entry, so those can be zero here. + err := finishDownload("", "", 9, 10, nil, nil, nil, nil) + require.ErrorIs(t, err, errSizeMismatch) + assert.EqualError(t, err, "size mismatch: expected 10 bytes, got 9") } -// TestSentinelsAreMatchable checks that the wrapped forms of the -// messages above remain matchable with errors.Is, which is the reason -// the sentinels exist at all. -func TestSentinelsAreMatchable(t *testing.T) { +func TestHashMismatchMessage(t *testing.T) { t.Parallel() - wrapped := fmt.Errorf("embedded signing key fingerprint %s %w %s", - "a", errSignerMismatch, "b") - require.ErrorIs(t, wrapped, errSignerMismatch) - - wrapped = fmt.Errorf("output file %s %w", "index.mf", errOutputExists) - require.ErrorIs(t, wrapped, errOutputExists) - - wrapped = fmt.Errorf("failed to fetch manifest: %w %d", errHTTPStatus, 404) - require.ErrorIs(t, wrapped, errHTTPStatus) - - assert.NotErrorIs(t, errHashMismatch, errSizeMismatch) + // A 32-byte digest that matches none of the (empty) manifest hashes. + err := verifyDownloadedHash(make([]byte, 32), &mfer.MFFilePath{}) + require.ErrorIs(t, err, errHashMismatch) + require.NotErrorIs(t, err, errSizeMismatch) + assert.EqualError(t, err, "hash mismatch") } diff --git a/internal/cli/freshen_test.go b/internal/cli/freshen_test.go index e5d844d..0679729 100644 --- a/internal/cli/freshen_test.go +++ b/internal/cli/freshen_test.go @@ -110,7 +110,10 @@ func TestFreshenRecordEntryMtimePresence(t *testing.T) { const relPath = "file1.txt" - mtime := time.Unix(1_700_000_000, 0) + // The scanned file's mtime is the Unix epoch. If recordEntry ever misreads + // an absent manifest mtime as the epoch, the "absent" case below would + // compare equal to this and be classified unchanged, so the test fails. + mtime := time.Unix(0, 0) info := stubFileInfo{size: 8, mtime: mtime} for _, tc := range []struct {