From 5c4fa43f8babb6d191dd82b1ceb0f6b88d46a8d9 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 11:24:58 +0200 Subject: [PATCH] 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. --- internal/builder/builder.go | 53 ++++++++++++++++++++++ internal/builder/check.go | 11 +++-- internal/builder/check_credential_test.go | 55 ++++++++++++++++++++++- internal/builder/packages_test.go | 24 +++++++--- 4 files changed, 129 insertions(+), 14 deletions(-) diff --git a/internal/builder/builder.go b/internal/builder/builder.go index 3f8a3ba2..e142ef29 100644 --- a/internal/builder/builder.go +++ b/internal/builder/builder.go @@ -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) diff --git a/internal/builder/check.go b/internal/builder/check.go index fb154a6b..50b3a476 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -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 { diff --git a/internal/builder/check_credential_test.go b/internal/builder/check_credential_test.go index f61a6474..7fc52aa9 100644 --- a/internal/builder/check_credential_test.go +++ b/internal/builder/check_credential_test.go @@ -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) + } +} diff --git a/internal/builder/packages_test.go b/internal/builder/packages_test.go index a87f19fd..7cccbf16 100644 --- a/internal/builder/packages_test.go +++ b/internal/builder/packages_test.go @@ -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) } }