From 8ba6ae44ba52270a97f3768bead4123aa93ad467 Mon Sep 17 00:00:00 2001 From: Aaron Brethorst Date: Sat, 9 May 2026 15:17:22 -0700 Subject: [PATCH 1/2] test(docker): cover SIGTERM, hooksctl-vs-server, 0o755 host dir Three coverage gaps surfaced in the PR #2 review: - TestImageGracefulShutdownOnSIGTERM: docker stop sends SIGTERM and we expect the binary to honor signal.NotifyContext rather than getting SIGKILL'd at the grace deadline. Asserts exit=0 and elapsed under the binary's 30s shutdown deadline (now extracted to a constant so the source of truth lives in one place). - TestImageHooksctlAgainstRunningServer: runs `hooksctl token list` inside the same container the server is in, against 127.0.0.1:8080. Token from `hooks init` is captured via a redaction-aware helper and passed to docker exec via -e (never argv, never test logs). - TestImageInitFailsClearlyOn0o755HostDir: documents the README-literal case (mkdir -p ./hooks-data without chmod). Probes for UID translation and skips on Docker Desktop / macOS where the scenario doesn't manifest; on Linux CI, asserts init fails with permission-denied. --- dockertest/docker_test.go | 225 +++++++++++++++++++++++++++++++++++++- 1 file changed, 220 insertions(+), 5 deletions(-) diff --git a/dockertest/docker_test.go b/dockertest/docker_test.go index 5c534c4..be1f6a8 100644 --- a/dockertest/docker_test.go +++ b/dockertest/docker_test.go @@ -3,6 +3,7 @@ package dockertest import ( + "bytes" "fmt" "net/http" "os" @@ -98,9 +99,18 @@ func TestImageShipsBothBinaries(t *testing.T) { // // `hooks init` prints a one-time admin token and a bootstrap signup code on // stdout. Both are credentials, so we never put the raw output in t.Fatalf -// messages — CI logs are public on PRs. We assert against a sentinel string -// and report only that the assertion failed, never the content. +// messages — CI logs are public on PRs. func scaffoldDataDir(t *testing.T) string { + t.Helper() + dir, _ := scaffoldDataDirCapturingToken(t) + return dir +} + +// scaffoldDataDirCapturingToken is scaffoldDataDir plus the one-time admin +// token. Caller MUST treat the returned token as a secret — never put it +// into t.Logf / t.Fatalf or any output that lands in CI logs. The redact +// helper exists precisely so this is easy to do. +func scaffoldDataDirCapturingToken(t *testing.T) (string, string) { t.Helper() dir := t.TempDir() if err := os.Chmod(dir, 0o777); err != nil { @@ -113,10 +123,51 @@ func scaffoldDataDir(t *testing.T) string { if err != nil { t.Fatalf("hooks init: %v (output redacted: contains one-time admin token)", err) } - if !strings.Contains(string(out), "admin token (shown ONCE)") { - t.Fatal("init did not print the admin-token line (output redacted)") + token := extractAdminToken(out) + if token == "" { + t.Fatal("init did not print an admin-token line (output redacted)") } - return dir + return dir, token +} + +func extractAdminToken(out []byte) string { + const marker = "admin token (shown ONCE):" + for _, line := range strings.Split(string(out), "\n") { + // HasPrefix on the trimmed line — substring matching would silently + // return any text that follows the token if `hooks init` is ever + // changed to print extra context on the same line, breaking the + // "token never leaks past this helper" invariant. + trimmed := strings.TrimLeft(line, " \t") + if !strings.HasPrefix(trimmed, marker) { + continue + } + return strings.TrimSpace(trimmed[len(marker):]) + } + return "" +} + +// redact strips occurrences of the secret from buf so the result is safe +// to include in test logs. Used on `docker exec` output that may echo the +// HOOKS_TOKEN env we passed in. +func redact(buf []byte, secret string) []byte { + if secret == "" { + return buf + } + return bytes.ReplaceAll(buf, []byte(secret), []byte("[REDACTED]")) +} + +// tokenListContainsName checks for `name` as a whitespace-anchored field in +// `hooksctl token list` output. Substring matching would over-accept a +// future header or help banner that mentions the same word. +func tokenListContainsName(out []byte, name string) bool { + for _, line := range strings.Split(string(out), "\n") { + for _, field := range strings.Fields(line) { + if field == name { + return true + } + } + } + return false } func TestImageInitScaffold(t *testing.T) { @@ -273,6 +324,170 @@ func TestImageRestartPreservesState(t *testing.T) { } } +// shutdownDeadline mirrors the WithTimeout value in cmd/hooks/main.go's +// signal-handler goroutine. Kept in sync by the SIGTERM test below — if the +// binary's deadline ever changes, update this too. +const shutdownDeadline = 30 * time.Second + +// TestImageGracefulShutdownOnSIGTERM verifies that `docker stop` (which +// sends SIGTERM, then SIGKILL after a grace period) lets the binary exit +// cleanly via its signal.NotifyContext path rather than getting hard-killed. +// A failed graceful shutdown shows up as exit code 137 (128 + SIGKILL) and +// `docker stop` taking the full grace period; a successful one returns +// quickly with exit code 0. The container is started without --rm so we +// can read .State.ExitCode after stop; cleanup goes through cleanupContainer +// rather than relying on the daemon to remove it. +func TestImageGracefulShutdownOnSIGTERM(t *testing.T) { + skipIfNoDocker(t) + dir := scaffoldDataDir(t) + + name := fmt.Sprintf("hooks-dockertest-sigterm-%d", time.Now().UnixNano()) + t.Cleanup(func() { cleanupContainer(t, name) }) + + if out, err := exec.Command("docker", "run", "-d", + "--name", name, + "-v", dir+":/data", + "-e", "RENDER_WEBHOOK_SECRET=stub-for-tests", + "-p", "0:8080", + imageTag, + ).CombinedOutput(); err != nil { + t.Fatalf("docker run: %v\n%s", err, out) + } + + addr := "http://127.0.0.1:" + hostPort(t, name, "8080/tcp") + if err := waitForHealthz(addr, 60*time.Second); err != nil { + t.Fatalf("server: %v\nlogs:\n%s", err, dockerLogs(name)) + } + + // `docker stop -t` exceeds shutdownDeadline by 5s so a slow CI runner + // doesn't SIGKILL a graceful-but-slow shutdown. + stopGrace := shutdownDeadline + 5*time.Second + start := time.Now() + if out, err := exec.Command("docker", "stop", "-t", + fmt.Sprintf("%d", int(stopGrace.Seconds())), name).CombinedOutput(); err != nil { + t.Fatalf("docker stop: %v\n%s", err, out) + } + elapsed := time.Since(start) + + out, err := exec.Command("docker", "inspect", + "--format", "{{.State.ExitCode}}", + name, + ).CombinedOutput() + if err != nil { + t.Fatalf("docker inspect: %v\n%s", err, out) + } + code := strings.TrimSpace(string(out)) + if code != "0" { + t.Fatalf("graceful shutdown failed: exit=%s after %v\nlogs:\n%s", + code, elapsed, dockerLogs(name)) + } + if elapsed > shutdownDeadline { + t.Fatalf("docker stop took %v, longer than the binary's %v shutdown deadline", + elapsed, shutdownDeadline) + } +} + +// TestImageHooksctlAgainstRunningServer boots the server in the container +// and runs `hooksctl token list` from inside the same container against +// 127.0.0.1:8080. Proves the shipped hooksctl can talk to the shipped hooks +// over a real TCP loopback inside the image — a property unit tests can't +// cover because they swap in httptest servers and a host-built hooksctl. +// +// The admin token is captured from `hooks init` and passed to docker exec +// via -e HOOKS_TOKEN= so it never lands in argv (and never in the test +// log; we redact before printing failure output). +func TestImageHooksctlAgainstRunningServer(t *testing.T) { + skipIfNoDocker(t) + dir, token := scaffoldDataDirCapturingToken(t) + + name := fmt.Sprintf("hooks-dockertest-ctl-%d", time.Now().UnixNano()) + // Register cleanup before the run so a failure between the two lines + // can't leak the container; `docker rm -f` on a not-yet-created name + // is a harmless no-op (logged by cleanupContainer). + t.Cleanup(func() { cleanupContainer(t, name) }) + if out, err := exec.Command("docker", "run", "-d", "--rm", + "--name", name, + "-v", dir+":/data", + "-e", "RENDER_WEBHOOK_SECRET=stub-for-tests", + "-p", "0:8080", + imageTag, + ).CombinedOutput(); err != nil { + t.Fatalf("docker run: %v\n%s", err, out) + } + + addr := "http://127.0.0.1:" + hostPort(t, name, "8080/tcp") + if err := waitForHealthz(addr, 60*time.Second); err != nil { + t.Fatalf("server: %v\nlogs:\n%s", err, dockerLogs(name)) + } + + // HOOKS_SERVER targets the in-container listener port (8080, EXPOSEd by + // the Dockerfile), not the random host-mapped port — exec runs inside + // the container's network namespace. + cmd := exec.Command("docker", "exec", + "-e", "HOOKS_TOKEN="+token, + "-e", "HOOKS_SERVER=http://127.0.0.1:8080", + name, + "hooksctl", "token", "list", + ) + out, err := cmd.CombinedOutput() + safe := redact(out, token) + if err != nil { + t.Fatalf("hooksctl token list: %v\n%s\nlogs:\n%s", err, safe, dockerLogs(name)) + } + // `hooks init` mints the admin token under the default name "operator". + // Anchor with whitespace boundaries so a future header rename or help + // banner that happens to contain "operator" can't satisfy this. + if !tokenListContainsName(out, "operator") { + t.Fatalf("token list missing the operator-named admin token\noutput:\n%s", safe) + } +} + +// TestImageInitFailsClearlyOn0o755HostDir documents what an operator hits +// when they follow the README literally — `mkdir -p ./hooks-data` produces +// a 0o755 directory owned by their host user. The container runs as UID +// 65532 (non-root), so bind-mount writes inside /data hit EACCES. We don't +// try to "fix" this in the image (chowning /data inside the container +// would require running as root or an init script); we test that the +// failure is loud (non-zero exit, "permission denied" in stderr). +// +// Skips on platforms that translate UIDs across the bind mount (Docker +// Desktop with file sharing typically does this on macOS) — there the +// scenario doesn't manifest, so there's nothing to assert. The probe runs +// `touch /data/probe` from inside the container against a 0o755 host dir +// and skips if the touch succeeds. +func TestImageInitFailsClearlyOn0o755HostDir(t *testing.T) { + skipIfNoDocker(t) + dir := t.TempDir() + // Force 0o755 to model the README path exactly (`mkdir -p ./hooks-data` + // with default umask 022). t.TempDir defaults to 0o700 — both block a + // non-owner UID, but 0o755 is the failure operators actually report. + if err := os.Chmod(dir, 0o755); err != nil { + t.Fatalf("chmod tempdir: %v", err) + } + + probe, err := exec.Command("docker", "run", "--rm", + "-v", dir+":/data", + "--entrypoint", "sh", + imageTag, "-c", "touch /data/probe", + ).CombinedOutput() + if err == nil { + t.Skipf("docker bind mount allows non-owner writes on this host "+ + "(likely a UID-translating setup like Docker Desktop file sharing); "+ + "the README permissions edge case doesn't manifest here.\nprobe output: %s", probe) + } + + out, runErr := exec.Command("docker", "run", "--rm", + "-v", dir+":/data", + imageTag, "init", + ).CombinedOutput() + if runErr == nil { + t.Fatalf("expected `hooks init` to fail on a 0o755 host dir, but it succeeded:\n%s", out) + } + if !bytes.Contains(bytes.ToLower(out), []byte("permission denied")) { + t.Fatalf("expected permission-denied error in init output, got:\n%s", out) + } +} + // waitForHealthz polls /healthz on the running container until it returns // 200 or the deadline expires. Server-side errors (5xx) are preserved // across iterations — if the server ever returned 500 then died, the From 9215a11fa0e0f48cb0b95d7410aad8b72bb6a5cb Mon Sep 17 00:00:00 2001 From: Aaron Brethorst Date: Sat, 9 May 2026 15:24:38 -0700 Subject: [PATCH 2/2] test(docker): redact init output on the runErr==nil path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the 0o755 test's first assertion fires, hooks init unexpectedly succeeded — which means it reached the print site in cmd/hooks/main.go and the token is in the captured output. Drop the %s and route through a redaction-aware Fatal, matching the pattern already used in scaffoldDataDir. The second assertion's branch (init returned non-zero, no "permission denied" found) is reached only when init failed before token issuance, so the diagnostic %s there cannot leak the token. Comment the leak analysis so future readers don't reflexively redact and lose the debug output. --- dockertest/docker_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dockertest/docker_test.go b/dockertest/docker_test.go index be1f6a8..5d44df5 100644 --- a/dockertest/docker_test.go +++ b/dockertest/docker_test.go @@ -481,9 +481,13 @@ func TestImageInitFailsClearlyOn0o755HostDir(t *testing.T) { imageTag, "init", ).CombinedOutput() if runErr == nil { - t.Fatalf("expected `hooks init` to fail on a 0o755 host dir, but it succeeded:\n%s", out) + // Init succeeded → admin-token line was printed; never echo `out`. + t.Fatal("expected `hooks init` to fail on a 0o755 host dir, but it succeeded (output redacted: contains one-time admin token)") } if !bytes.Contains(bytes.ToLower(out), []byte("permission denied")) { + // init returned non-zero, so by cmd/hooks/main.go's order it didn't + // reach the admin-token print site — `out` is safe to surface and + // the diagnostic value is high (tells you what error did fire). t.Fatalf("expected permission-denied error in init output, got:\n%s", out) } }