Keep the forge credential out of what a check's container sees, and redact what a check publishes (issue 462)

The toolchain container mounts the workspace as HOME, so the credential kept there, and a clone's recorded userinfo, were readable by any pull request; its output is kept on the bus for days. The credential now lives in a private directory outside the workspace only while cloning, clones record their URL without userinfo, and every line said to the build's log and the verdict passes a redactor copied from the journal tool (sharing it: issue 471).
2026-10-11 11:20:10 +02:00
parent 3b6b54a501
commit c494ed03a7
5 changed files with 497 additions and 33 deletions
@@ -0,0 +1,200 @@
+package builder
+
+import (
+	"context"
+	"crypto/sha256"
+	"encoding/hex"
+	"encoding/json"
+	"errors"
+	"io/fs"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/novox/mesh-controller/internal/facts"
+)
+
+// **A check's container never sees the forge credential** (novox/hq issue 462): the toolchain container
+// mounts the workspace as HOME, so a credential kept there — or a clone's .git/config carrying one — is
+// readable by any pull request's merge-check.sh, and printed, kept on the bus for days.
+
+const (
+	forgeSecret  = "sw0rdfi5h-forge"
+	forgeURL     = "http://mesh_novox_builder:" + forgeSecret + "@forge.invalid:20000"
+	besideSecret = "b3side-t0ken"
+)
+
+// aFactsRegistry is an artifact store holding the facts snapshot, and nothing else.
+func aFactsRegistry(t *testing.T) string {
+	t.Helper()
+	body, err := json.Marshal(facts.Facts{Format: facts.Format, Taken: time.Now().UTC(),
+		Versions: facts.Versions{Bus: "2.11.17", Store: "17.11"}, Machines: []facts.Machine{{Name: "abcdef", Length: 6}}})
+	if err != nil {
+		t.Fatal(err)
+	}
+	sum := sha256.Sum256(body)
+	digest := "sha256:" + hex.EncodeToString(sum[:])
+	manifest, _ := json.Marshal(map[string]any{"schemaVersion": 2, "layers": []map[string]any{
+		{"mediaType": facts.MediaType, "digest": digest, "size": len(body)}}})
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case strings.Contains(r.URL.Path, "/manifests/"):
+			w.Write(manifest)
+		case strings.HasSuffix(r.URL.Path, "/blobs/"+digest):
+			w.Write(body)
+		default:
+			http.NotFound(w, r)
+		}
+	}))
+	t.Cleanup(srv.Close)
+	return strings.TrimPrefix(srv.URL, "http://")
+}
+
+// bareURL is a URL with its userinfo left out.
+func bareURL(t *testing.T, raw string) string {
+	u, err := url.Parse(raw)
+	if err != nil {
+		t.Fatal(err)
+	}
+	u.User = nil
+	return u.String()
+}
+
+func TestACheckContainerSeesNoForgeCredential(t *testing.T) {
+	repo, head := aCheckedRepository(t, map[string]string{CheckScript: "echo checked\n"})
+	besideRepo, besideHead := aCheckedRepository(t, map[string]string{"README": "beside"})
+	// A clone source that carries userinfo, as a forge's clone URL may: git records it as given in the
+	// clone's .git/config, which the container reads.
+	besideURL := "file://beside-user:" + besideSecret + "@" + besideRepo
+	workspace := t.TempDir()
+
+	var stores []string
+	reached := false
+	var leaks []string
+	run := func(ctx context.Context, dir, name string, args ...string) (string, error) {
+		switch name {
+		case "git":
+			for _, a := range args {
+				if f, ok := strings.CutPrefix(a, "credential.helper=store --file="); ok {
+					stores = append(stores, f)
+					raw, err := os.ReadFile(f)
+					if err != nil || strings.TrimSpace(string(raw)) != forgeURL {
+						t.Errorf("git is offered a store that does not hold the credential as given: %q, %v", raw, err)
+					}
+					if info, err := os.Stat(f); err == nil && info.Mode().Perm() != 0o600 {
+						t.Errorf("the credential store is readable beyond its owner: %v", info.Mode())
+					}
+				}
+			}
+			if len(args) >= 2 && args[len(args)-3] == "--quiet" && hasString(args, "clone") {
+				source := args[len(args)-2]
+				if u, err := url.Parse(source); err == nil && u.User != nil {
+					// Git cannot reach a file:// URL with userinfo; clone it without, then record it as git
+					// would have: as given.
+					clone := append(append([]string{}, args[:len(args)-2]...), bareURL(t, source), args[len(args)-1])
+					if out, err := Command(ctx, dir, "git", clone...); err != nil {
+						return out, err
+					}
+					return Command(ctx, filepath.Join(dir, args[len(args)-1]), "git", "remote", "set-url", "origin", source)
+				}
+			}
+			return Command(ctx, dir, name, args...)
+		case "docker":
+			if !reached {
+				reached = true
+				// The first container: everything the workspace holds is what the toolchain container sees.
+				filepath.WalkDir(workspace, func(path string, d fs.DirEntry, err error) error {
+					if err != nil || d.IsDir() {
+						return nil
+					}
+					raw, _ := os.ReadFile(path)
+					s := string(raw)
+					if strings.Contains(s, forgeSecret) || strings.Contains(s, besideSecret) ||
+						d.Name() == "git-credentials" || strings.Contains(s, "credential.helper") {
+						leaks = append(leaks, path)
+					}
+					return nil
+				})
+				for _, f := range stores {
+					if _, err := os.Stat(f); !errors.Is(err, os.ErrNotExist) {
+						leaks = append(leaks, f+" (still there when the first container runs)")
+					}
+					if rel, err := filepath.Rel(workspace, f); err == nil && !strings.HasPrefix(rel, "..") {
+						leaks = append(leaks, f+" (inside the workspace the container mounts)")
+					}
+				}
+			}
+			if len(args) > 0 && args[0] == "ps" {
+				return "", nil
+			}
+			return "", errors.New("no container runtime in this test")
+		}
+		return "", errors.New("unexpected command " + name)
+	}
+	// A credential an older builder left in the workspace is removed too.
+	if err := os.WriteFile(filepath.Join(workspace, "git-credentials"), []byte(forgeURL+"\n"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	_, err := Check(t.Context(), run, CheckSpec{ID: "check-462", Repository: repo, Ref: head, Owner: "novox",
+		Repo: "mesh-controller", Number: 1, Toolchain: "golang", Beside: map[string]Beside{
+			"mesh-catalog": {Repository: besideURL, Ref: besideHead}}}, workspace, aFactsRegistry(t),
+		GitCredential{URL: forgeURL}, nil)
+	if err == nil {
+		t.Fatal("the check ran past its first container in a test with none")
+	}
+	if !reached {
+		t.Fatalf("the check never reached its first container: %v", err)
+	}
+	if len(stores) == 0 {
+		t.Fatal("no clone was offered the forge credential")
+	}
+	if len(leaks) > 0 {
+		t.Fatalf("the check's container sees the credential:\n%s", strings.Join(leaks, "\n"))
+	}
+}
+
+// Every line a repository's own check prints is published to the build's log redacted.
+func TestACheckLinePublishedToTheLogIsRedacted(t *testing.T) {
+	var said []string
+	say := func(step, format string, args ...any) {
+		if step == "output" && len(args) > 0 {
+			said = append(said, args[0].(string))
+		}
+	}
+	var out tail
+	layer := ownCheck(t.Context(), CheckSpec{Toolchain: "golang"}, []ScriptPart{{Toolchain: "go", Script: CheckScript}},
+		t.TempDir(), &out, func() bool { return false }, func(string, string) *exec.Cmd {
+			return exec.CommandContext(t.Context(), "sh", "-c", "echo cloning http://mesh_builder:t0ps3cret-forge@forge.invalid/novox/x.git; "+
+				"echo token ghp_abcdefghijklmnopqrstuvwxyz0123456789")
+		}, say)
+	if layer == nil || layer.Verdict != "pass" {
+		t.Fatalf("the check answered %+v\n%s", layer, out.String())
+	}
+	joined := strings.Join(said, "\n")
+	if strings.Contains(joined, "t0ps3cret-forge") || strings.Contains(joined, "ghp_abcdef") {
+		t.Fatalf("a secret the check printed is published to the build's log:\n%s", joined)
+	}
+	if !strings.Contains(joined, "http://mesh_builder:[redacted: a password in a URI]@forge.invalid/novox/x.git") {
+		t.Fatalf("the line is not said with what was there named:\n%s", joined)
+	}
+}
+
+func TestTheRedactorHidesTheForgeCredentialAndShapes(t *testing.T) {
+	r := redactorFor(GitCredential{URL: forgeURL})
+	for in, want := range map[string]string{
+		"the secret alone: " + forgeSecret:    "the secret alone: [redacted: the forge credential]",
+		"go test ./... ok":                    "go test ./... ok",
+		"--password hunter22 and done":        "--password [redacted: the word after --password] and done",
+		"commit 3b6b54a0c1d2e3f4a5b6c7d8e9f0": "commit 3b6b54a0c1d2e3f4a5b6c7d8e9f0",
+	} {
+		if got := r.redact(in); got != want {
+			t.Errorf("%q redacted as %q, want %q", in, got, want)
+		}
+	}
+}