From e736166a393b03d308fc8ebb44ee38f8c24a27d2 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 2 Nov 2026 11:45:42 +0000 Subject: [PATCH] Hide credentials in checkout preparation errors --- cmd/releasekit/main_test.go | 23 ++++++++++++++++++++++ docs/cli.md | 1 + docs/roadmap/remediation.md | 26 ++++++++++++++++++++++++- internal/git/prepare.go | 2 +- preparation_test.go | 38 +++++++++++++++++++++++++++++++++++++ 5 files changed, 88 insertions(+), 2 deletions(-) diff --git a/cmd/releasekit/main_test.go b/cmd/releasekit/main_test.go index 93766b9..cf111a4 100644 --- a/cmd/releasekit/main_test.go +++ b/cmd/releasekit/main_test.go @@ -37,6 +37,29 @@ func TestVersionCommand(t *testing.T) { } } +func TestCIDeliverRedactsRemoteCredentialsOnPreparationFailure(t *testing.T) { + t.Setenv("NO_PROXY", "*") + t.Setenv("no_proxy", "*") + t.Setenv("GIT_CONFIG_GLOBAL", os.DevNull) + t.Setenv("GIT_CONFIG_NOSYSTEM", "1") + t.Setenv("GIT_TERMINAL_PROMPT", "0") + repository := newCLIRepository(t) + commit := cliGitOutput(t, repository, "rev-parse", "HEAD") + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + })) + defer server.Close() + remote := strings.Replace(server.URL, "http://", "http://review-user:review-password@", 1) + "/repo.git" + var stdout, stderr bytes.Buffer + status := run(t.Context(), []string{"ci-deliver", "--repo", repository, "--remote", remote, "--commit", commit, "v1.2.3"}, &stdout, &stderr) + if status != 1 || stdout.Len() != 0 || !strings.Contains(stderr.String(), "prepare tagged checkout") || !strings.Contains(stderr.String(), "repo.git") { + t.Fatalf("status = %d, stdout = %q, stderr = %q", status, stdout.String(), stderr.String()) + } + if strings.Contains(stderr.String(), "review-user") || strings.Contains(stderr.String(), "review-password") { + t.Fatalf("CLI preparation diagnostic exposes credentials: %q", stderr.String()) + } +} + func TestPublishCommandReportsUnknownForPeeledOnlyRecovery(t *testing.T) { repository := newCLIRepository(t) writeRepositoryFile(t, repository, "docs/releases/v1.2.3.md", "# Example Product v1.2.3\n\nRelease note.\n") diff --git a/docs/cli.md b/docs/cli.md index b85249c..d6fbd5b 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -244,6 +244,7 @@ the selected Git remote (default `origin`), requires its peeled commit to match the required full `--commit` object ID, checks out that commit detached, then verifies the tag, HEAD, and clean worktree. With `--prepared`, it performs those checks read-only against the existing checkout and does not fetch or check out. +Preparation diagnostics mask userinfo in HTTP(S) remote URLs. In either mode, it loads the profile from the verified source. `--expected-module` optionally checks that the profile selects the module chosen by a launcher; module selection itself remains profile-owned. The command compares the selected diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index a3377df..95c8a37 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -109,9 +109,16 @@ public publication regression using a local hook from another caller directory. ## R03. Checkout preparation reintroduces credentials after Git redaction -**Priority:** P2. **Status:** Pending. This is an A04 integration gap in the +**Priority:** P2. **Status:** Complete. This addressed an A04 integration gap in the existing preparation workflow, separate from R01's URL-span parsing defect. +**Completion:** Fetch-failure errors render the remote with `DisplayURL`, while +the fetch retains its original URL and wrapped Git cause. Public preparation +and CLI regressions failed before the fix and now pass with local HTTP servers; +the public test also verifies authentication with the configured credentials. +The remaining owned Git URL renderers were reviewed, and the CLI guide describes +the implemented diagnostic behavior. Combined verification below passed. + **Relevant code:** `PrepareTaggedCheckout` in [internal/git/prepare.go](../../internal/git/prepare.go), lines 18–20; the public [preparation facade](../../preparation.go); and error presentation in @@ -205,3 +212,20 @@ using the earlier Git environment behavior passed. Both audits left production code and existing tests unchanged. The full-suite, static, and compiled API/dependency results above record the audit baseline; completion notes record verification of the subsequent repairs. + +## Completed remediation verification + +All three findings are complete. Each fix includes regressions that reproduced +the defect before the production change, and each unit has its own commit with +the corresponding completion marker. + +On Linux/amd64 with Go 1.26.8, combined verification passed with +`GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off`, the prepared build cache, and +`TMPDIR=/var/tmp` for test fixtures: + +- `go test ./...` +- `go test -race ./...` +- `go run ./internal/apicheck` + +Changed Go files passed formatting inspection. Local links and fragments in +this document and the CLI guide resolved, and the diff passed whitespace checks. diff --git a/internal/git/prepare.go b/internal/git/prepare.go index 5549e24..981ded0 100644 --- a/internal/git/prepare.go +++ b/internal/git/prepare.go @@ -17,7 +17,7 @@ func PrepareTaggedCheckout(ctx context.Context, repositoryDir, tag, remote, expe } ref := "refs/tags/" + tag if _, err := run(ctx, repositoryDir, nil, "fetch", "--no-tags", remote, ref+":"+ref); err != nil { - return fmt.Errorf("fetch tag %s from remote %s: %w", tag, remote, err) + return fmt.Errorf("fetch tag %s from remote %s: %w", tag, DisplayURL(remote), err) } source, err := InspectTaggedSource(ctx, repositoryDir, tag) if err != nil { diff --git a/preparation_test.go b/preparation_test.go index 25a69cd..9f8bfdc 100644 --- a/preparation_test.go +++ b/preparation_test.go @@ -3,17 +3,55 @@ package releasekit_test import ( "context" "errors" + "net/http" + "net/http/httptest" "os" "os/exec" "path/filepath" "strconv" "strings" + "sync/atomic" "testing" "time" releasekit "go.maximumdirect.net/releasekit" ) +func TestPrepareTaggedCheckoutRedactsRemoteCredentials(t *testing.T) { + t.Setenv("NO_PROXY", "*") + t.Setenv("no_proxy", "*") + t.Setenv("GIT_CONFIG_GLOBAL", os.DevNull) + t.Setenv("GIT_CONFIG_NOSYSTEM", "1") + t.Setenv("GIT_TERMINAL_PROMPT", "0") + repo := newPublishFixture(t) + var authenticated atomic.Bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + user, password, ok := r.BasicAuth() + if !ok { + w.Header().Set("WWW-Authenticate", `Basic realm="fixture"`) + w.WriteHeader(http.StatusUnauthorized) + return + } + if user != "review-user" || password != "review-password" { + t.Error("fetch did not preserve the configured credentials") + } + authenticated.Store(true) + w.WriteHeader(http.StatusServiceUnavailable) + })) + defer server.Close() + remote := strings.Replace(server.URL, "http://", "http://review-user:review-password@", 1) + "/repo.git" + err := releasekit.PrepareTaggedCheckout(t.Context(), releasekit.TaggedCheckoutRequest{ + RepositoryDir: repo.work, Tag: "v1.2.3", Remote: remote, ExpectedCommit: repo.commit, + }) + var exit *exec.ExitError + if !errors.As(err, &exit) || !authenticated.Load() || !strings.Contains(err.Error(), "v1.2.3") || !strings.Contains(err.Error(), "repo.git") { + t.Fatalf("preparation failure = %v, authenticated = %t", err, authenticated.Load()) + } + if strings.Contains(err.Error(), "review-user") || strings.Contains(err.Error(), "review-password") { + t.Fatalf("preparation diagnostic exposes credentials: %v", err) + } +} + func TestPrepareBuildDependenciesCancellationReleasesInheritedOutput(t *testing.T) { directory := t.TempDir() ready := filepath.Join(t.TempDir(), "descendant")