Leave no package-registry credential or clone userinfo in the workspace a check mounts (issue 462)

A build wrote .npmrc into its source tree and never removed it, and its clone recorded the URL's userinfo; a later check's container mounts the workspace as HOME. The .npmrc and the tree now go when the build ends, clones record their URL without userinfo, and a check first removes any .npmrc an older builder left.
2026-10-11 11:24:58 +02:00
parent c494ed03a7
commit 5c4fa43f8b
4 changed files with 129 additions and 14 deletions
@@ -10,6 +10,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"os/exec"
 	"path"
@@ -169,6 +170,9 @@ func build(ctx context.Context, run Runner, publish Publisher,
 	if err := os.RemoveAll(tree); err != nil {
 		return Result{}, err
 	}
+	// Removed when the build ends, whatever happens: the tree holds the .npmrc a build may be handed, in the
+	// workspace a later check's container mounts as its HOME (novox/hq issue 462).
+	defer os.RemoveAll(tree)
 	// A fresh clone every time rather than a fetch into a tree that is already there. A build
 	// that reuses a working tree can succeed because of something a previous build left behind,
 	// and that is a build nobody can reproduce.
@@ -176,6 +180,9 @@ func build(ctx context.Context, run Runner, publish Publisher,
 		say("clone", "FAILED: %v", err)
 		return Result{}, fmt.Errorf("cannot clone %s: %w", repository, err)
 	}
+	if err := recordedWithoutUserinfo(ctx, run, tree, repository); err != nil {
+		return Result{}, err
+	}
 	say("clone", "done")
 	if ref != "" {
 		if _, err := run(ctx, tree, "git", "checkout", "--quiet", ref); err != nil {
@@ -247,6 +254,8 @@ func build(ctx context.Context, run Runner, publish Publisher,
 		if err := os.WriteFile(npmrcPath, []byte(content), 0o600); err != nil {
 			return Result{}, fmt.Errorf("cannot write the package-registry credential for the build: %w", err)
 		}
+		// Only for as long as the build: a later check's container mounts this workspace (novox/hq issue 462).
+		defer os.Remove(npmrcPath)
 		say("packages", "resolving %s from the mesh's package registry", npmrc.Scope)
 		src.notPinned("it resolves packages from the mesh's registry at build time")
 	}
@@ -440,6 +449,9 @@ func contextFrom(ctx context.Context, run Runner, workspace, artifact, credentia
 	if _, err := run(ctx, workspace, "git", cloneWith(credentials, "clone", "--quiet", url, dir)...); err != nil {
 		return "", fmt.Errorf("cannot clone %s: %w", url, err)
 	}
+	if err := recordedWithoutUserinfo(ctx, run, dir, url); err != nil {
+		return "", err
+	}
 	if from.Ref != "" {
 		if _, err := run(ctx, dir, "git", "checkout", "--quiet", from.Ref); err != nil {
 			return "", fmt.Errorf("%s has no %s: %w", from.Repository, from.Ref, err)
@@ -498,6 +510,47 @@ func storeCredential(workspace string, forge GitCredential) (string, func(), err
 	return path, forget, nil
 }

+// recordedWithoutUserinfo makes a clone record the URL it came from without userinfo: git keeps it as given in
+// the clone's .git/config, inside the workspace a check's container mounts (novox/hq issue 462).
+func recordedWithoutUserinfo(ctx context.Context, run Runner, clone, repository string) error {
+	bare, carried := withoutUserinfo(repository)
+	if !carried {
+		return nil
+	}
+	if _, err := run(ctx, clone, "git", "remote", "set-url", "origin", bare); err != nil {
+		return fmt.Errorf("the clone of %s keeps its credential: %w", bare, err)
+	}
+	return nil
+}
+
+// forgetLeftCredentials removes what an earlier build or an older builder left in the workspace that holds
+// a credential: the forge's git-credentials, and every .npmrc a build wrote into a tree it cloned. Run
+// before a check, whose container mounts the workspace as its HOME (novox/hq issue 462). The Go caches are
+// not walked: no build writes a credential there, and they hold more files than everything else.
+func forgetLeftCredentials(workspace string) error {
+	if err := os.Remove(filepath.Join(workspace, "git-credentials")); err != nil && !errors.Is(err, os.ErrNotExist) {
+		return err
+	}
+	return filepath.WalkDir(workspace, func(p string, d fs.DirEntry, err error) error {
+		if err != nil {
+			if errors.Is(err, os.ErrNotExist) {
+				return nil
+			}
+			return err
+		}
+		if d.IsDir() && p != workspace && (d.Name() == "go-cache" || d.Name() == "go-modules") &&
+			filepath.Dir(p) == workspace {
+			return filepath.SkipDir
+		}
+		if !d.IsDir() && d.Name() == ".npmrc" {
+			if err := os.Remove(p); err != nil && !errors.Is(err, os.ErrNotExist) {
+				return fmt.Errorf("a package-registry credential left at %s cannot be removed: %w", p, err)
+			}
+		}
+		return nil
+	})
+}
+
 // withinDir is whether path is dir or under it, both made absolute and resolved.
 func withinDir(dir, path string) bool {
 	d, err1 := filepath.Abs(dir)
@@ -255,16 +255,15 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry
 		return CheckVerdict{}, err
 	}
 	defer forget()
+	if err := forgetLeftCredentials(workspace); err != nil {
+		return CheckVerdict{}, err
+	}
 	clone := func(repository, ref, dir string) error {
 		if _, err := run(ctx, root, "git", cloneWith(credentials, "clone", "--quiet", repository, dir)...); err != nil {
 			return fmt.Errorf("cannot clone %s: %s", redact.redact(repository), redact.redact(err.Error()))
 		}
-		// Git records the URL cloned from as given, in the clone's .git/config the container reads: one
-		// carrying userinfo is recorded without it (novox/hq issue 462).
-		if bare, ok := withoutUserinfo(repository); ok {
-			if _, err := run(ctx, filepath.Join(root, dir), "git", "remote", "set-url", "origin", bare); err != nil {
-				return fmt.Errorf("the clone of %s keeps its credential: %w", bare, err)
-			}
+		if err := recordedWithoutUserinfo(ctx, run, filepath.Join(root, dir), repository); err != nil {
+			return err
 		}
 		if ref != "" {
 			if _, err := run(ctx, filepath.Join(root, dir), "git", "checkout", "--quiet", ref); err != nil {
@@ -28,6 +28,7 @@ const (
 	forgeSecret  = "sw0rdfi5h-forge"
 	forgeURL     = "http://mesh_novox_builder:" + forgeSecret + "@forge.invalid:20000"
 	besideSecret = "b3side-t0ken"
+	npmSecret    = "npm-s3cret-t0ken"
 )

 // aFactsRegistry is an artifact store holding the facts snapshot, and nothing else.
@@ -116,7 +117,8 @@ func TestACheckContainerSeesNoForgeCredential(t *testing.T) {
 					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") {
+						strings.Contains(s, npmSecret) || d.Name() == "git-credentials" || d.Name() == ".npmrc" ||
+						strings.Contains(s, "credential.helper") {
 						leaks = append(leaks, path)
 					}
 					return nil
@@ -137,10 +139,20 @@ func TestACheckContainerSeesNoForgeCredential(t *testing.T) {
 		}
 		return "", errors.New("unexpected command " + name)
 	}
-	// A credential an older builder left in the workspace is removed too.
+	// A credential an older builder left in the workspace is removed too: the forge's, and the .npmrc a
+	// build wrote into the tree it cloned.
 	if err := os.WriteFile(filepath.Join(workspace, "git-credentials"), []byte(forgeURL+"\n"), 0o600); err != nil {
 		t.Fatal(err)
 	}
+	for _, dir := range []string{"source/x", "context-server"} {
+		if err := os.MkdirAll(filepath.Join(workspace, dir), 0o755); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.WriteFile(filepath.Join(workspace, dir, ".npmrc"),
+			[]byte("//forge.invalid/api/packages/novox/npm/:_authToken="+npmSecret+"\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),
@@ -198,3 +210,42 @@ func TestTheRedactorHidesTheForgeCredentialAndShapes(t *testing.T) {
 		}
 	}
 }
+
+// A build's clone of a URL carrying userinfo records it without, so no build leaves a credential in
+// workspace/source/.git/config while it runs either (novox/hq issue 462); the tree itself is gone when the
+// build ends.
+func TestABuildsCloneRecordsNoUserinfo(t *testing.T) {
+	repo, _ := aCheckedRepository(t, map[string]string{ManifestName: `{"module":"plain","version":"1"}`})
+	source := "file://build-user:" + besideSecret + "@" + repo
+	workspace := t.TempDir()
+	tree := filepath.Join(workspace, "source")
+	var configs []string
+	run := func(ctx context.Context, dir, name string, args ...string) (string, error) {
+		if name == "git" && hasString(args, "clone") && args[len(args)-2] == source {
+			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, args[len(args)-1], "git", "remote", "set-url", "origin", source)
+		}
+		if name == "git" && len(args) > 0 && args[0] == "rev-parse" {
+			raw, _ := os.ReadFile(filepath.Join(tree, ".git", "config"))
+			configs = append(configs, string(raw))
+		}
+		return Command(ctx, dir, name, args...)
+	}
+	if _, err := Build(t.Context(), run, &recorded{}, source, "", "", workspace, nil, Npmrc{}, GitCredential{}, nil); err != nil {
+		t.Fatal(err)
+	}
+	if len(configs) == 0 {
+		t.Fatal("the build never read its clone")
+	}
+	for _, c := range configs {
+		if strings.Contains(c, besideSecret) || strings.Contains(c, "build-user") {
+			t.Fatalf("the build's clone records the credential it was cloned with:\n%s", c)
+		}
+	}
+	if _, err := os.Stat(tree); !os.IsNotExist(err) {
+		t.Fatalf("the build's tree outlives the build: %v", err)
+	}
+}
@@ -70,7 +70,16 @@ func TestNpmrcDisabledUntilThereIsARegistry(t *testing.T) {
 func TestAnImageBuildGetsTheCredentialInTheContextAndHostNetwork(t *testing.T) {
 	r, workspace := aRepository(t, withBoth, map[string]string{"Dockerfile": "FROM scratch\nCOPY .npmrc ./", "files/x": "y"})
 	n := Npmrc{Scope: "@novox", Registry: "https://forge.invalid/api/packages/novox/npm/", Token: "t"}
-	if _, err := Build(context.Background(), r.run, r,
+	npmrc := filepath.Join(workspace, "source", ".npmrc")
+	inContext := false
+	run := func(ctx context.Context, dir, name string, args ...string) (string, error) {
+		if name == "docker" && len(args) > 0 && args[0] == "build" {
+			_, err := os.Stat(npmrc)
+			inContext = err == nil
+		}
+		return r.run(ctx, dir, name, args...)
+	}
+	if _, err := Build(context.Background(), run, r,
 		"https://forge.invalid/meshboard.git", "", "", workspace, nil, n, GitCredential{}, nil); err != nil {
 		t.Fatalf("the build failed: %v", err)
 	}
@@ -90,11 +99,14 @@ func TestAnImageBuildGetsTheCredentialInTheContextAndHostNetwork(t *testing.T) {
 	if !strings.Contains(build, "--network host") {
 		t.Fatalf("the build was not given the host network to reach the registry: %s", build)
 	}
-	// The .npmrc is written into the build context (the source tree), where a Dockerfile COPYs it.
-	tree := filepath.Join(workspace, "source")
-	npmrc := filepath.Join(tree, ".npmrc")
-	if _, err := os.Stat(npmrc); err != nil {
-		t.Fatalf("the credential was not written into the build context: %v", err)
+	// The .npmrc is written into the build context (the source tree), where a Dockerfile COPYs it, and
+	// removed with the tree when the build ends: a later check's container mounts the workspace (novox/hq
+	// issue 462).
+	if !inContext {
+		t.Fatal("the credential was not in the build context when the image was built")
+	}
+	if _, err := os.Stat(npmrc); !os.IsNotExist(err) {
+		t.Fatalf("the credential outlives the build in the workspace: %v", err)
 	}
 }