From 1c5f54603a5f0693f1ef093b3c8a25b18587dc77 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Thu, 15 Oct 2026 15:52:34 +0000 Subject: [PATCH] Set the time zone in main and run Entry in its own test process (closes #192) cli.Entry() set time.Local to UTC and ended with memguard.Purge(), and TestEntryWipesBuffersAndReturnsExitCode ran it in the same process as the other tests. A timer left by an earlier test could read time.Local as it changed, which the race detector reported on some runs, and the purge could destroy a buffer still in use, the likeliest cause of a crash seen on another run. main() in cmd/secret now sets time.Local before it calls cli.Entry(); the test that runs the built binary with TZ=America/New_York still covers it. The Entry test runs cli.Entry() in a copy of the test binary started with SECRET_TEST_RUN_ENTRY set, which init picks up so that TestMain does not run there. Model: opus-5-5 --- TODO.md | 9 ++++++ cmd/secret/main.go | 6 ++++ internal/cli/entry_test.go | 64 ++++++++++++++++++++++++++++++-------- internal/cli/root.go | 5 --- 4 files changed, 66 insertions(+), 18 deletions(-) diff --git a/TODO.md b/TODO.md index 42302ae..033f414 100644 --- a/TODO.md +++ b/TODO.md @@ -18,6 +18,15 @@ https://git.eeqj.de/sneak/secret/milestone/12 # Completed Steps +- 2026-10-15: `make test` no longer fails on some runs in `internal/cli` + (https://git.eeqj.de/sneak/secret/issues/192). `cli.Entry()` set `time.Local` + to UTC and ended by wiping every memguard buffer, and a test ran it in the + same process as the other tests. A timer left by an earlier test could read + `time.Local` as it changed, which the race detector reported, and the wipe + could destroy a buffer still in use, the likeliest cause of a crash of the + test process on another run. `main()` in `cmd/secret` now sets `time.Local` + before it calls `cli.Entry()`, and that test runs `cli.Entry()` in a process + of its own. - 2026-10-14: A wrong passphrase, from `SB_UNLOCK_PASSPHRASE` or typed at the prompt, gives the error `wrong passphrase for unlocker ` (https://git.eeqj.de/sneak/secret/issues/157). It used to be age's "no diff --git a/cmd/secret/main.go b/cmd/secret/main.go index e2b9ab7..e780426 100644 --- a/cmd/secret/main.go +++ b/cmd/secret/main.go @@ -3,10 +3,16 @@ package main import ( "os" + "time" "sneak.berlin/go/secret/internal/cli" ) func main() { + // Every time secret prints or writes is in UTC, whatever TZ or the + // system's time zone is set to. It is set here, before cli.Entry starts + // any goroutine that could read time.Local while it changes. + time.Local = time.UTC //nolint:gosmopolitan // this makes local time UTC + os.Exit(cli.Entry()) } diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index 3889936..08ebe7d 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -3,6 +3,7 @@ package cli_test import ( "bufio" "context" + "fmt" "os" "os/exec" "path/filepath" @@ -16,29 +17,66 @@ import ( "sneak.berlin/go/secret/internal/secret" ) -// Entry must return its exit code rather than exit, so that its deferred -// memguard purge runs on the success and the error path alike. -// -//nolint:paralleltest // sets os.Args, and Entry wipes every buffer in the process -func TestEntryWipesBuffersAndReturnsExitCode(t *testing.T) { - savedArgs := os.Args +const ( + // envRunEntry, when set, makes this test binary run cli.Entry with its + // own arguments and exit, instead of running the tests. + envRunEntry = "SECRET_TEST_RUN_ENTRY" - t.Cleanup(func() { os.Args = savedArgs }) + // bufferWiped is what the test binary prints, run with envRunEntry set, + // when cli.Entry wiped the buffer made before it ran. + bufferWiped = "Entry wiped the buffer made before it ran" +) + +// With envRunEntry set, the test binary makes a buffer, runs cli.Entry, +// says whether the buffer is wiped, and exits with Entry's exit code. This +// is done in init, not in a test, so that TestMain does not run in that +// process: it would build the secret binary again, and remove it at the end, +// while the other tests run it. +// +//nolint:gochecknoinits // must run before TestMain, as above +func init() { + if os.Getenv(envRunEntry) == "" { + return + } + + buf := memguard.NewBufferFromBytes([]byte("key material")) + code := cli.Entry() + + if !buf.IsAlive() { + fmt.Fprintln(os.Stderr, bufferWiped) + } + + os.Exit(code) +} + +// Entry must return its exit code rather than exit, so that its deferred +// memguard purge runs on the success and the error path alike. Entry wipes +// every memguard buffer in the process, including those other tests are +// using, so it runs in a process of its own: this test binary, run again +// with envRunEntry set. +func TestEntryWipesBuffersAndReturnsExitCode(t *testing.T) { + t.Parallel() tests := []struct { args []string exitCode int }{ - {args: []string{"secret", "--help"}, exitCode: 0}, - {args: []string{"secret", "no-such-command"}, exitCode: 1}, + {args: []string{"--help"}, exitCode: 0}, + {args: []string{"no-such-command"}, exitCode: 1}, } for _, tt := range tests { - buf := memguard.NewBufferFromBytes([]byte("key material")) - os.Args = tt.args + //nolint:gosec // G204: test runs its own test binary + cmd := exec.CommandContext(t.Context(), os.Args[0], tt.args...) + cmd.Env = append(os.Environ(), envRunEntry+"=1") - assert.Equal(t, tt.exitCode, cli.Entry(), "exit code for %v", tt.args) - assert.False(t, buf.IsAlive(), "Entry left a buffer unwiped for %v", tt.args) + output, err := cmd.CombinedOutput() + require.NotNil(t, cmd.ProcessState, "%v did not run: %v", tt.args, err) + + assert.Equal(t, tt.exitCode, cmd.ProcessState.ExitCode(), + "exit code for %v: %s", tt.args, output) + assert.Contains(t, string(output), bufferWiped, + "Entry left a buffer unwiped for %v", tt.args) } } diff --git a/internal/cli/root.go b/internal/cli/root.go index ee2e536..65d1620 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -2,7 +2,6 @@ package cli import ( "os" - "time" "github.com/awnumar/memguard" "github.com/spf13/cobra" @@ -15,10 +14,6 @@ import ( // every memguard buffer before it returns, so the caller must do nothing // but exit with the code. func Entry() int { - // Every time secret prints or writes is in UTC, whatever TZ or the - // system's time zone is set to. - time.Local = time.UTC //nolint:gosmopolitan // this makes local time UTC - raiseLockedMemoryLimit() // On SIGINT or SIGTERM memguard runs this function, wipes every buffer