From dacc0543b47e7a1e3748842cd67f9d7a9e9f64e9 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Thu, 27 Aug 2026 20:54:14 -0700 Subject: [PATCH 1/2] PR6: harden the repo source cache with URL-hashed mirrors and self-contained checkouts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the basename-keyed mutable repo cache (~/.cache/harness-openshell/repos//) — which collided when two repos shared a basename and raced when two runs shared one repo — with: mirrors/.git bare, shallow, shared, updated in place checkouts/// real repo (own .git), per run The mirror is the only shared on-disk state; every write to it is serialized under a per-mirror flock held across the mirror update and the local object copy, so a concurrent run's shallow gc can't delete packs mid-read. Distinct repos that share a basename now hash to different mirrors; concurrent runs of the same repo get independent checkouts. Each per-run checkout is built with `git init` + `git fetch --depth 1 ` + `git checkout --detach FETCH_HEAD` rather than a linked worktree. A linked worktree's .git is a *file* pointing at a host path, which breaks once only the checkout is uploaded into the sandbox; a self-contained checkout carries its own objects (no alternates), so git keeps working there. The fetch runs against the local mirror path, so no repo URL or credentials leak into the checkout's .git/config. Failed prepares (bad ref, network blip) clean up their run dir instead of leaking it under checkouts/. Mirror creation is idempotent across a crash between init and remote setup. Lock files are intentionally never unlinked (deleting an flock'd file reintroduces the unlink race); one 0-byte file per repo, not per run. --- CHANGELOG.md | 12 ++ cmd/executor.go | 145 ++++-------------- internal/source/cache.go | 103 +++++++++++++ internal/source/checkout.go | 101 +++++++++++++ internal/source/mirror.go | 140 +++++++++++++++++ internal/source/source_test.go | 267 +++++++++++++++++++++++++++++++++ 6 files changed, 656 insertions(+), 112 deletions(-) create mode 100644 internal/source/cache.go create mode 100644 internal/source/checkout.go create mode 100644 internal/source/mirror.go create mode 100644 internal/source/source_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 20b3202..722aab8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ # Changelog +## [Unreleased] + +### Changed +- Cloned repos now use URL-hashed bare mirrors (`~/.cache/harness-openshell/mirrors/`) + plus per-run, self-contained checkouts (`~/.cache/harness-openshell/checkouts/`) + instead of the basename-keyed `repos/` cache. Distinct repositories that share a + basename no longer collide, and concurrent runs of the same repository no longer + share a working tree. Each checkout is a real repository with its own `.git`, so + git keeps working inside the sandbox after upload. The old + `~/.cache/harness-openshell/repos/` directory is orphaned and safe to delete + manually. + ## [0.3.0] - 2026-06-17 ### Added diff --git a/cmd/executor.go b/cmd/executor.go index 1a0ec73..165e2c8 100644 --- a/cmd/executor.go +++ b/cmd/executor.go @@ -4,10 +4,7 @@ import ( "context" "fmt" "os" - "os/exec" - "path" "path/filepath" - "strings" "time" "github.com/stackrox/harness-openshell/internal/agent" @@ -18,6 +15,7 @@ import ( "github.com/stackrox/harness-openshell/internal/plan" "github.com/stackrox/harness-openshell/internal/reconcile" "github.com/stackrox/harness-openshell/internal/run" + "github.com/stackrox/harness-openshell/internal/source" "github.com/stackrox/harness-openshell/internal/status" ) @@ -31,17 +29,17 @@ const reconcileTimeout = 60 * time.Second var DefaultAgentConfig []byte type upLocalOpts struct { - harnessDir string - gw gateway.Gateway - target openshell.Target - agentCfg *agent.AgentConfig - agentPath string - sandboxName string - noTTY bool - setupOnly bool - harness *agent.Harness - newClient openshell.Factory - retrySleep time.Duration + harnessDir string + gw gateway.Gateway + target openshell.Target + agentCfg *agent.AgentConfig + agentPath string + sandboxName string + noTTY bool + setupOnly bool + harness *agent.Harness + newClient openshell.Factory + retrySleep time.Duration } func upLocal(opts upLocalOpts) error { @@ -94,7 +92,11 @@ func upLocal(opts upLocalOpts) error { // Clone repo outside the sandbox so git credentials never enter it. var repoUpload *gateway.Upload if agentCfg.Repo != "" { - upload, cleanup, err := cloneRepo(agentCfg.Repo, agentCfg.RepoRef) + runID, err := source.NewRunID() + if err != nil { + return err + } + upload, cleanup, err := cloneRepo(agentCfg.Repo, agentCfg.RepoRef, runID) if err != nil { return fmt.Errorf("cloning repo: %w", err) } @@ -201,116 +203,35 @@ func upLocal(opts upLocalOpts) error { }) } -// cloneRepo clones or updates a cached git repository and returns an Upload -// that places it at /sandbox/. Repos are cached in -// ~/.cache/harness-openshell/repos// so subsequent runs only fetch -// deltas. The clone happens outside the sandbox so git credentials never enter -// it. Returns a cleanup function (no-op since the cache is persistent). -func cloneRepo(repo, ref string) (gateway.Upload, func(), error) { - repoName := strings.TrimSuffix(path.Base(repo), ".git") - +// cloneRepo prepares an isolated per-run checkout of repo at ref and returns an +// Upload that places it at /sandbox/, plus a cleanup that removes the +// checkout. The mirror + checkout are built on the host so git credentials never +// enter the sandbox; see internal/source for the URL-hashed mirror + per-run +// checkout layout that keeps distinct same-basename repos and concurrent runs +// from colliding. +func cloneRepo(repo, ref, runID string) (gateway.Upload, func(), error) { if ref != "" { status.Infof("Repo: %s (ref: %s)", repo, ref) } else { status.Infof("Repo: %s", repo) } - cacheDir, err := repoCacheDir(repoName) + cache, err := source.DefaultCache() if err != nil { return gateway.Upload{}, nil, err } - - if isGitRepo(cacheDir) { - if err := fetchRepo(cacheDir, ref); err != nil { - return gateway.Upload{}, nil, err - } - status.OKf("Updated %s (cached)", repoName) - } else { - if err := freshClone(repo, ref, cacheDir); err != nil { - return gateway.Upload{}, nil, fmt.Errorf("git clone %s: %w", repo, err) - } - status.OKf("Cloned %s", repoName) - } - - return gateway.Upload{Src: cacheDir, Dst: "/sandbox"}, func() {}, nil -} - -func repoCacheDir(repoName string) (string, error) { - home, err := os.UserHomeDir() + prepared, err := cache.Prepare(repo, ref, runID) if err != nil { - return "", fmt.Errorf("determining home dir: %w", err) - } - dir := filepath.Join(home, ".cache", "harness-openshell", "repos", repoName) - if err := os.MkdirAll(filepath.Dir(dir), 0o755); err != nil { - return "", fmt.Errorf("creating cache dir: %w", err) - } - return dir, nil -} - -func isGitRepo(dir string) bool { - _, err := os.Stat(filepath.Join(dir, ".git")) - return err == nil -} - -func freshClone(repo, ref, dest string) error { - args := []string{"clone", "--depth", "1"} - if ref != "" { - args = append(args, "--branch", ref) - } - args = append(args, repo, dest) - cmd := exec.Command("git", args...) - cmd.Stdout = os.Stderr - cmd.Stderr = os.Stderr - if err := cmd.Run(); err != nil { - return err - } - return initSubmodules(dest) -} - -func fetchRepo(dir, ref string) error { - fetchArgs := []string{"-C", dir, "fetch", "--depth", "1", "origin"} - if ref != "" { - fetchArgs = append(fetchArgs, ref) - } - cmd := exec.Command("git", fetchArgs...) - cmd.Stdout = os.Stderr - cmd.Stderr = os.Stderr - if err := cmd.Run(); err != nil { - return fmt.Errorf("git fetch: %w", err) - } - - target := "FETCH_HEAD" - if ref == "" { - target = "origin/HEAD" - } - cmd = exec.Command("git", "-C", dir, "checkout", target, "--force") - cmd.Stdout = os.Stderr - cmd.Stderr = os.Stderr - if err := cmd.Run(); err != nil { - return fmt.Errorf("git checkout %s: %w", target, err) - } - - if err := initSubmodules(dir); err != nil { - return err + return gateway.Upload{}, nil, fmt.Errorf("preparing repo %s: %w", repo, err) } + status.OKf("Prepared %s", source.RepoName(repo)) - // Clean untracked files from previous runs - cmd = exec.Command("git", "-C", dir, "clean", "-fdx") - cmd.Stdout = os.Stderr - cmd.Stderr = os.Stderr - cmd.Run() - - return nil -} - -func initSubmodules(dir string) error { - cmd := exec.Command("git", "-C", dir, "submodule", "update", "--init", "--depth", "1") - cmd.Stdout = os.Stderr - cmd.Stderr = os.Stderr - if err := cmd.Run(); err != nil { - return fmt.Errorf("git submodule update: %w", err) + cleanup := func() { + if cerr := prepared.Cleanup(); cerr != nil { + status.Warnf("cleaning up repo checkout: %v", cerr) + } } - return nil + return gateway.Upload{Src: prepared.Dir, Dst: "/sandbox"}, cleanup, nil } // reconcileGateway drives the gateway's providers and inference route to match diff --git a/internal/source/cache.go b/internal/source/cache.go new file mode 100644 index 0000000..9540367 --- /dev/null +++ b/internal/source/cache.go @@ -0,0 +1,103 @@ +// Package source manages the on-disk cache of git repositories cloned outside +// the sandbox for upload. +// +// It replaces the old basename-keyed cache (~/.cache/harness-openshell/repos/ +// /) — which collided when two repos shared a basename and raced when +// two runs shared one repo — with URL-hashed bare mirrors plus per-run, +// self-contained checkouts: +// +// ~/.cache/harness-openshell/ +// mirrors/.git bare, shallow, updated in place, shared +// checkouts/// real repo (own .git), per run, removed after run +// +// The mirror is the only shared state; every write to it is serialized under a +// per-mirror file lock. Checkouts are per-run, never shared, and hold their own +// objects (no alternates into the mirror) so git keeps working after only the +// checkout dir is uploaded into the sandbox. +package source + +import ( + "crypto/rand" + "crypto/sha256" + "encoding/hex" + "fmt" + "net/url" + "os" + "path" + "path/filepath" + "strings" +) + +// Cache locates the on-disk cache roots. The zero value is unusable; construct +// with DefaultCache or NewCache. +type Cache struct { + root string // ~/.cache/harness-openshell +} + +// DefaultCache resolves the cache under the user's home directory. +func DefaultCache() (*Cache, error) { + home, err := os.UserHomeDir() + if err != nil { + return nil, fmt.Errorf("determining home dir: %w", err) + } + return NewCache(filepath.Join(home, ".cache", "harness-openshell")), nil +} + +// NewCache builds a cache rooted at the given directory (used by tests). +func NewCache(root string) *Cache { return &Cache{root: root} } + +// CanonicalizeURL normalizes a repo URL into a stable key for "same repo". +// It trims surrounding space, strips a trailing slash and a ".git" suffix, and +// lowercases the scheme and host only — repository paths stay case-sensitive +// because many hosts treat them so. Non-URL inputs (e.g. scp-style +// git@host:org/repo) are returned trimmed of the same suffixes without further +// change, which is still stable per distinct spelling. +func CanonicalizeURL(raw string) string { + s := strings.TrimSpace(raw) + if u, err := url.Parse(s); err == nil && u.Host != "" { + u.Scheme = strings.ToLower(u.Scheme) + u.Host = strings.ToLower(u.Host) + s = u.String() + } + s = strings.TrimRight(s, "/") + s = strings.TrimSuffix(s, ".git") + return s +} + +// RepoName derives the directory basename a repo is uploaded under +// (/sandbox/), matching the old cache's behavior. +func RepoName(repoURL string) string { + return strings.TrimSuffix(path.Base(strings.TrimRight(strings.TrimSpace(repoURL), "/")), ".git") +} + +// MirrorPath is the bare-mirror directory for a repo URL, keyed by the sha256 of +// its canonical form so distinct repos with the same basename never collide. +func (c *Cache) MirrorPath(repoURL string) string { + sum := sha256.Sum256([]byte(CanonicalizeURL(repoURL))) + return filepath.Join(c.root, "mirrors", hex.EncodeToString(sum[:])+".git") +} + +// runDir is the per-run checkout parent (checkouts/), removed wholesale +// on cleanup. +func (c *Cache) runDir(runID string) string { + return filepath.Join(c.root, "checkouts", runID) +} + +// checkoutPath nests the checkout as checkouts// so its +// basename stays : `openshell --upload` copies the source dir by +// name, so this is what makes the tree land at /sandbox/ rather than +// /sandbox/. +func (c *Cache) checkoutPath(runID, repoName string) string { + return filepath.Join(c.runDir(runID), repoName) +} + +// NewRunID returns a random hex id identifying one run's checkout. 128 bits so +// concurrent runs never collide on a checkout path (a collision would let one +// run's cleanup delete another's tree). +func NewRunID() (string, error) { + var b [16]byte + if _, err := rand.Read(b[:]); err != nil { + return "", fmt.Errorf("generating run id: %w", err) + } + return hex.EncodeToString(b[:]), nil +} diff --git a/internal/source/checkout.go b/internal/source/checkout.go new file mode 100644 index 0000000..4a05f1d --- /dev/null +++ b/internal/source/checkout.go @@ -0,0 +1,101 @@ +package source + +import ( + "fmt" + "os" +) + +// Prepared is the result of Prepare: an isolated, self-contained per-run +// checkout ready to upload, plus a Cleanup that removes it. +type Prepared struct { + // Dir is the absolute path to the per-run checkout. Its basename is the repo + // name, so uploading it lands the tree at /sandbox/. The checkout + // has a real .git directory holding its own objects (no link back to the + // shared mirror), so git still works inside the sandbox after upload. + Dir string + // Cleanup removes the per-run checkout directory. Safe to call once; errors + // are returned for the caller to log, never fatal. + Cleanup func() error +} + +// Prepare updates the shared bare mirror for repoURL and builds a self-contained +// per-run checkout at ref (branch, tag, or the remote default when ref is ""), +// with submodules initialized. +// +// The mirror update and the local object copy into the checkout run under an +// exclusive per-mirror lock so concurrent runs of the same repo cannot corrupt +// the shared object store or race a shallow gc. Once the objects are copied the +// checkout is fully independent of the mirror, so the checkout and submodule init +// run outside the lock — unrelated runs are not serialized on network submodule +// fetches. +// +// The clone happens on the host; git credentials never enter the sandbox — only +// the returned checkout contents are uploaded. The checkout is a real repository +// (self-contained .git), so the agent can run git inside the sandbox. +func (c *Cache) Prepare(repoURL, ref, runID string) (Prepared, error) { + dir := c.checkoutPath(runID, RepoName(repoURL)) + + if err := c.fetchIntoCheckout(repoURL, ref, dir); err != nil { + // The checkout dir may have been created before the failing step (a bad + // ref or a network blip on fetch is expected); don't leak it. + _ = os.RemoveAll(c.runDir(runID)) + return Prepared{}, err + } + + // checkout + submodules run outside the mirror lock: the checkout already + // holds every object it needs, so it no longer touches the shared mirror. + if err := git(dir, "checkout", "--detach", "--quiet", "FETCH_HEAD"); err != nil { + _ = os.RemoveAll(c.runDir(runID)) + return Prepared{}, err + } + if err := git(dir, "submodule", "update", "--init", "--depth", "1"); err != nil { + _ = os.RemoveAll(c.runDir(runID)) + return Prepared{}, err + } + + cleanup := func() error { return os.RemoveAll(c.runDir(runID)) } + return Prepared{Dir: dir, Cleanup: cleanup}, nil +} + +// fetchIntoCheckout, under the per-mirror lock, updates the shared mirror for +// repoURL at ref and copies the resolved commit's objects into a fresh +// repository at dir, leaving the commit as the checkout's FETCH_HEAD. Holding the +// lock across both the mirror update and the local object copy keeps a concurrent +// run's shallow gc from deleting packs mid-read. +func (c *Cache) fetchIntoCheckout(repoURL, ref, dir string) error { + mirror := c.MirrorPath(repoURL) + + lock, err := lockMirror(mirror) + if err != nil { + return err + } + defer lock.unlock() + + if err := ensureMirror(mirror, repoURL); err != nil { + return err + } + commit, err := fetchRef(mirror, ref) + if err != nil { + return err + } + + // Fresh, isolated checkout dir (clear any stale remnant from a reused run id + // or a crashed prior run). + if err := os.RemoveAll(dir); err != nil { + return fmt.Errorf("clearing checkout dir: %w", err) + } + if err := os.MkdirAll(dir, 0o755); err != nil { + return fmt.Errorf("creating checkout dir: %w", err) + } + if err := git(dir, "init", "--quiet"); err != nil { + return err + } + // Copy just the resolved commit's objects from the local mirror into the + // checkout's own object store. No alternates are configured, so the checkout + // stays valid after the mirror (or the whole host cache) is gone — which is + // what makes git usable inside the sandbox once only this dir is uploaded. + if err := git(dir, "fetch", "--depth", "1", mirror, commit); err != nil { + return err + } + return nil +} diff --git a/internal/source/mirror.go b/internal/source/mirror.go new file mode 100644 index 0000000..8844787 --- /dev/null +++ b/internal/source/mirror.go @@ -0,0 +1,140 @@ +package source + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "syscall" +) + +// git runs a git command, streaming its output to stderr (so clone/fetch +// progress is visible), and wraps failures with the arguments for context. +func git(dir string, args ...string) error { + full := args + if dir != "" { + full = append([]string{"-C", dir}, args...) + } + cmd := exec.Command("git", full...) + cmd.Stdout = os.Stderr + cmd.Stderr = os.Stderr + if err := cmd.Run(); err != nil { + return fmt.Errorf("git %v: %w", args, err) + } + return nil +} + +// gitOutput runs a git command and returns its trimmed stdout (stderr still +// streams for diagnostics). +func gitOutput(dir string, args ...string) (string, error) { + full := args + if dir != "" { + full = append([]string{"-C", dir}, args...) + } + cmd := exec.Command("git", full...) + cmd.Stderr = os.Stderr + out, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("git %v: %w", args, err) + } + return strings.TrimSpace(string(out)), nil +} + +// mirrorLock is an exclusive advisory lock held over a mirror's fetch+worktree +// window. Concurrent runs of the same repo contend only here — the one piece of +// shared on-disk state — so serializing it keeps a fetch's shallow gc from +// racing a peer's worktree registration. +type mirrorLock struct{ f *os.File } + +// lockMirror acquires an exclusive flock on a sidecar lock file for the mirror. +// The lock file lives beside the mirror dir (.lock) so it exists before +// the mirror itself does. +// +// The lock file is intentionally never unlinked: deleting it on unlock would +// reintroduce the classic flock unlink race (a peer that already opened the file +// holds a lock on a now-orphaned inode while a new process creates a fresh file +// and locks that instead, so both believe they hold the lock). There is exactly +// one 0-byte lock file per distinct repo URL, so they do not accumulate per run. +func lockMirror(mirrorPath string) (*mirrorLock, error) { + if err := os.MkdirAll(filepath.Dir(mirrorPath), 0o755); err != nil { + return nil, fmt.Errorf("creating mirrors dir: %w", err) + } + f, err := os.OpenFile(mirrorPath+".lock", os.O_CREATE|os.O_RDWR, 0o644) + if err != nil { + return nil, fmt.Errorf("opening mirror lock: %w", err) + } + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil { + f.Close() + return nil, fmt.Errorf("locking mirror: %w", err) + } + return &mirrorLock{f: f}, nil +} + +func (l *mirrorLock) unlock() { + if l == nil || l.f == nil { + return + } + syscall.Flock(int(l.f.Fd()), syscall.LOCK_UN) + l.f.Close() +} + +// ensureMirror creates the bare mirror on first use and (re)points its origin +// remote at repoURL. A bare repo with a configured origin lets us shallow-fetch +// arbitrary refs on demand rather than mirroring every ref. It is fully +// idempotent — safe to re-run after a crash that left the bare repo created but +// the remote unconfigured (the origin step no longer sits behind the isGitDir +// early return). Callers hold the mirror lock, so this never races a peer. +func ensureMirror(mirrorPath, repoURL string) error { + if !isGitDir(mirrorPath) { + if err := os.MkdirAll(mirrorPath, 0o755); err != nil { + return fmt.Errorf("creating mirror dir: %w", err) + } + if err := git(mirrorPath, "init", "--bare", "--quiet"); err != nil { + return err + } + } + return ensureOrigin(mirrorPath, repoURL) +} + +// ensureOrigin idempotently points the mirror's origin remote at repoURL: it +// adds the remote when absent and updates the URL otherwise. Listing remotes +// first keeps the common first-clone path quiet (probing with `remote get-url` +// on a missing remote would print an error to stderr). +func ensureOrigin(mirrorPath, repoURL string) error { + remotes, err := gitOutput(mirrorPath, "remote") + if err != nil { + return err + } + for _, r := range strings.Split(remotes, "\n") { + if strings.TrimSpace(r) == "origin" { + return git(mirrorPath, "remote", "set-url", "origin", repoURL) + } + } + return git(mirrorPath, "remote", "add", "origin", repoURL) +} + +// fetchRef shallow-fetches the requested ref (or the remote default HEAD when +// ref is empty) into the mirror and resolves it to a concrete commit. The commit +// is what the per-run worktree is created from. +func fetchRef(mirrorPath, ref string) (commit string, err error) { + target := ref + if target == "" { + target = "HEAD" + } + if err := git(mirrorPath, "fetch", "--depth", "1", "origin", target); err != nil { + return "", err + } + commit, err = gitOutput(mirrorPath, "rev-parse", "FETCH_HEAD") + if err != nil { + return "", err + } + return commit, nil +} + +func isGitDir(dir string) bool { + // A bare repo has HEAD at its root; a non-bare has .git/HEAD. The mirror is + // bare, so check for HEAD directly. + _, err := os.Stat(filepath.Join(dir, "HEAD")) + return err == nil +} diff --git a/internal/source/source_test.go b/internal/source/source_test.go new file mode 100644 index 0000000..e51d635 --- /dev/null +++ b/internal/source/source_test.go @@ -0,0 +1,267 @@ +package source + +import ( + "os" + "os/exec" + "path/filepath" + "sync" + "testing" +) + +func TestCanonicalizeURL(t *testing.T) { + cases := []struct{ in, want string }{ + {"https://github.com/org/repo.git", "https://github.com/org/repo"}, + {"https://github.com/org/repo/", "https://github.com/org/repo"}, + {"https://GitHub.com/org/repo", "https://github.com/org/repo"}, + {"HTTPS://github.com/org/repo", "https://github.com/org/repo"}, + {" https://github.com/org/repo.git ", "https://github.com/org/repo"}, + {"git@github.com:org/repo.git", "git@github.com:org/repo"}, + } + for _, c := range cases { + if got := CanonicalizeURL(c.in); got != c.want { + t.Errorf("CanonicalizeURL(%q) = %q, want %q", c.in, got, c.want) + } + } +} + +func TestCanonicalizeURLVariantsShareKey(t *testing.T) { + // All spellings of the same repo must canonicalize identically so they hash + // to one mirror. + variants := []string{ + "https://github.com/org/repo", + "https://github.com/org/repo.git", + "https://github.com/org/repo/", + "https://GitHub.com/org/repo.git", + } + first := CanonicalizeURL(variants[0]) + for _, v := range variants[1:] { + if got := CanonicalizeURL(v); got != first { + t.Errorf("CanonicalizeURL(%q) = %q, want %q", v, got, first) + } + } +} + +func TestMirrorPathBasenameCollision(t *testing.T) { + // Two different repos that share a basename ("repo") must map to different + // mirrors — the whole point of URL-hashing instead of basename-keying. + c := NewCache(t.TempDir()) + a := c.MirrorPath("https://github.com/org-a/repo.git") + b := c.MirrorPath("https://github.com/org-b/repo.git") + if a == b { + t.Fatalf("same-basename repos collided on one mirror: %s", a) + } + // And URL variants of the SAME repo must map to one mirror. + if c.MirrorPath("https://github.com/org-a/repo") != a { + t.Errorf("URL variant did not reuse the mirror for org-a/repo") + } +} + +func TestRepoName(t *testing.T) { + cases := map[string]string{ + "https://github.com/org/repo.git": "repo", + "https://github.com/org/repo/": "repo", + "git@github.com:org/tools.git": "tools", + } + for in, want := range cases { + if got := RepoName(in); got != want { + t.Errorf("RepoName(%q) = %q, want %q", in, got, want) + } + } +} + +// makeRemote creates a local non-bare git repo with one commit on `main` and a +// file, then returns a file:// URL with the given basename so tests can exercise +// real git without a network. +func makeRemote(t *testing.T, basename, fileContent string) string { + t.Helper() + dir := filepath.Join(t.TempDir(), basename) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + runGit(t, dir, "init", "--quiet", "-b", "main") + runGit(t, dir, "config", "user.email", "t@example.com") + runGit(t, dir, "config", "user.name", "t") + if err := os.WriteFile(filepath.Join(dir, "marker.txt"), []byte(fileContent), 0o644); err != nil { + t.Fatal(err) + } + runGit(t, dir, "add", "-A") + runGit(t, dir, "commit", "--quiet", "-m", "init") + return "file://" + dir +} + +func runGit(t *testing.T, dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } +} + +func TestPrepareCheckoutContent(t *testing.T) { + remote := makeRemote(t, "repo", "hello") + c := NewCache(t.TempDir()) + + p, err := c.Prepare(remote, "main", "run1") + if err != nil { + t.Fatalf("Prepare: %v", err) + } + // The checkout basename must be the repo name (so it uploads to + // /sandbox/), not the run id. + if got := filepath.Base(p.Dir); got != "repo" { + t.Errorf("checkout basename = %q, want %q", got, "repo") + } + got, err := os.ReadFile(filepath.Join(p.Dir, "marker.txt")) + if err != nil { + t.Fatalf("reading checked-out file: %v", err) + } + if string(got) != "hello" { + t.Errorf("marker.txt = %q, want %q", got, "hello") + } + + if err := p.Cleanup(); err != nil { + t.Errorf("Cleanup: %v", err) + } + if _, err := os.Stat(p.Dir); !os.IsNotExist(err) { + t.Errorf("worktree still present after cleanup: %v", err) + } +} + +func TestPrepareCheckoutSelfContained(t *testing.T) { + // The uploaded checkout must be a real repo whose git works after the mirror + // (and the whole host cache) is gone — inside the sandbox only this dir + // exists. A linked worktree would leave a `.git` *file* pointing at a host + // path that breaks there; this pins that it is a real `.git` directory with + // its own objects. + remote := makeRemote(t, "repo", "portable") + cacheRoot := t.TempDir() + c := NewCache(cacheRoot) + + p, err := c.Prepare(remote, "main", "run1") + if err != nil { + t.Fatalf("Prepare: %v", err) + } + + info, err := os.Stat(filepath.Join(p.Dir, ".git")) + if err != nil || !info.IsDir() { + t.Fatalf(".git must be a real directory, got isDir=%v err=%v", info.IsDir(), err) + } + + // Copy the checkout elsewhere, then delete the entire cache (mirror + run + // dirs) to simulate uploading only the checkout into a fresh sandbox. + uploaded := filepath.Join(t.TempDir(), "repo") + if out, err := exec.Command("cp", "-R", p.Dir, uploaded).CombinedOutput(); err != nil { + t.Fatalf("copy checkout: %v\n%s", err, out) + } + if err := os.RemoveAll(cacheRoot); err != nil { + t.Fatalf("removing cache: %v", err) + } + + // git must still work against the copied checkout with no mirror present. + if out, err := exec.Command("git", "-C", uploaded, "status", "--porcelain").CombinedOutput(); err != nil { + t.Fatalf("git status in uploaded checkout: %v\n%s", err, out) + } + if out, err := exec.Command("git", "-C", uploaded, "log", "--oneline", "-1").CombinedOutput(); err != nil { + t.Fatalf("git log in uploaded checkout: %v\n%s", err, out) + } +} + +func TestPrepareDefaultRef(t *testing.T) { + // ref "" resolves the remote default branch. + remote := makeRemote(t, "repo", "default") + c := NewCache(t.TempDir()) + p, err := c.Prepare(remote, "", "run1") + if err != nil { + t.Fatalf("Prepare: %v", err) + } + defer p.Cleanup() + got, _ := os.ReadFile(filepath.Join(p.Dir, "marker.txt")) + if string(got) != "default" { + t.Errorf("marker.txt = %q, want %q", got, "default") + } +} + +func TestPrepareConcurrentSameRepo(t *testing.T) { + // Two simultaneous runs of the same repo must get independent worktrees with + // correct content and no error — the mirror lock serializes the shared fetch. + remote := makeRemote(t, "repo", "shared") + c := NewCache(t.TempDir()) + + const n = 6 + var wg sync.WaitGroup + results := make([]Prepared, n) + errs := make([]error, n) + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + runID, _ := NewRunID() + results[i], errs[i] = c.Prepare(remote, "main", runID) + }(i) + } + wg.Wait() + + seen := map[string]bool{} + for i := 0; i < n; i++ { + if errs[i] != nil { + t.Fatalf("run %d Prepare: %v", i, errs[i]) + } + if seen[results[i].Dir] { + t.Fatalf("run %d shared a checkout path: %s", i, results[i].Dir) + } + seen[results[i].Dir] = true + got, err := os.ReadFile(filepath.Join(results[i].Dir, "marker.txt")) + if err != nil || string(got) != "shared" { + t.Errorf("run %d content = %q (err %v), want %q", i, got, err, "shared") + } + } + for i := 0; i < n; i++ { + if err := results[i].Cleanup(); err != nil { + t.Errorf("run %d Cleanup: %v", i, err) + } + } +} + +func TestPrepareBadRefNoLeak(t *testing.T) { + // A failed prepare (here: a ref that does not exist) must not leave an + // orphaned checkout dir behind — network/ref failures are expected in + // production and would otherwise pile up under checkouts/. + remote := makeRemote(t, "repo", "x") + root := t.TempDir() + c := NewCache(root) + + if _, err := c.Prepare(remote, "no-such-branch", "run1"); err == nil { + t.Fatal("Prepare succeeded on a nonexistent ref, want error") + } + runDir := filepath.Join(root, "checkouts", "run1") + if _, err := os.Stat(runDir); !os.IsNotExist(err) { + t.Errorf("checkout run dir leaked after failed prepare: %v", err) + } +} + +func TestPrepareTwoBasenameReposNoCollision(t *testing.T) { + // Two distinct repos sharing basename "repo" prepared in the same cache must + // yield the content of their own remote, proving the mirrors are separate. + c := NewCache(t.TempDir()) + remoteA := makeRemote(t, "repo", "content-A") + remoteB := makeRemote(t, "repo", "content-B") + + pa, err := c.Prepare(remoteA, "main", "runA") + if err != nil { + t.Fatal(err) + } + defer pa.Cleanup() + pb, err := c.Prepare(remoteB, "main", "runB") + if err != nil { + t.Fatal(err) + } + defer pb.Cleanup() + + ga, _ := os.ReadFile(filepath.Join(pa.Dir, "marker.txt")) + gb, _ := os.ReadFile(filepath.Join(pb.Dir, "marker.txt")) + if string(ga) != "content-A" { + t.Errorf("repo A content = %q, want content-A", ga) + } + if string(gb) != "content-B" { + t.Errorf("repo B content = %q, want content-B", gb) + } +} From 60814b18f6de53653f3261f6b811558f89f46177 Mon Sep 17 00:00:00 2001 From: Robby Cochran Date: Mon, 31 Aug 2026 09:41:21 -0700 Subject: [PATCH 2/2] fix(source): exclude credentials from mirror state --- internal/source/cache.go | 24 +++++++++++++++++------ internal/source/mirror.go | 1 + internal/source/source_test.go | 36 ++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 6 deletions(-) diff --git a/internal/source/cache.go b/internal/source/cache.go index 9540367..d0ddd9b 100644 --- a/internal/source/cache.go +++ b/internal/source/cache.go @@ -47,13 +47,13 @@ func DefaultCache() (*Cache, error) { func NewCache(root string) *Cache { return &Cache{root: root} } // CanonicalizeURL normalizes a repo URL into a stable key for "same repo". -// It trims surrounding space, strips a trailing slash and a ".git" suffix, and -// lowercases the scheme and host only — repository paths stay case-sensitive -// because many hosts treat them so. Non-URL inputs (e.g. scp-style -// git@host:org/repo) are returned trimmed of the same suffixes without further -// change, which is still stable per distinct spelling. +// It trims surrounding space, removes URL userinfo, strips a trailing slash and +// a ".git" suffix, and lowercases the scheme and host only — repository paths +// stay case-sensitive because many hosts treat them so. Non-URL inputs (e.g. +// scp-style git@host:org/repo) are returned trimmed of the same suffixes without +// further change, which is still stable per distinct spelling. func CanonicalizeURL(raw string) string { - s := strings.TrimSpace(raw) + s := stripURLUserinfo(strings.TrimSpace(raw)) if u, err := url.Parse(s); err == nil && u.Host != "" { u.Scheme = strings.ToLower(u.Scheme) u.Host = strings.ToLower(u.Host) @@ -64,6 +64,18 @@ func CanonicalizeURL(raw string) string { return s } +// stripURLUserinfo removes embedded credentials from a URL before it is used +// as a cache identity or persisted in git configuration. Authentication is +// resolved by git's configured credential helper instead. +func stripURLUserinfo(raw string) string { + u, err := url.Parse(raw) + if err != nil || u.Host == "" { + return raw + } + u.User = nil + return u.String() +} + // RepoName derives the directory basename a repo is uploaded under // (/sandbox/), matching the old cache's behavior. func RepoName(repoURL string) string { diff --git a/internal/source/mirror.go b/internal/source/mirror.go index 8844787..ee0ed38 100644 --- a/internal/source/mirror.go +++ b/internal/source/mirror.go @@ -102,6 +102,7 @@ func ensureMirror(mirrorPath, repoURL string) error { // first keeps the common first-clone path quiet (probing with `remote get-url` // on a missing remote would print an error to stderr). func ensureOrigin(mirrorPath, repoURL string) error { + repoURL = stripURLUserinfo(repoURL) remotes, err := gitOutput(mirrorPath, "remote") if err != nil { return err diff --git a/internal/source/source_test.go b/internal/source/source_test.go index e51d635..e0faf1c 100644 --- a/internal/source/source_test.go +++ b/internal/source/source_test.go @@ -15,6 +15,7 @@ func TestCanonicalizeURL(t *testing.T) { {"https://GitHub.com/org/repo", "https://github.com/org/repo"}, {"HTTPS://github.com/org/repo", "https://github.com/org/repo"}, {" https://github.com/org/repo.git ", "https://github.com/org/repo"}, + {"https://user:secret@github.com/org/repo.git", "https://github.com/org/repo"}, {"git@github.com:org/repo.git", "git@github.com:org/repo"}, } for _, c := range cases { @@ -24,6 +25,41 @@ func TestCanonicalizeURL(t *testing.T) { } } +func TestCredentialsShareMirrorKey(t *testing.T) { + c := NewCache(t.TempDir()) + first := c.MirrorPath("https://alice:first-token@github.com/org/repo.git") + second := c.MirrorPath("https://bob:rotated-token@github.com/org/repo.git") + if first != second { + t.Fatalf("credential variants mapped to different mirrors: %q != %q", first, second) + } +} + +func TestEnsureOriginDoesNotPersistUserinfo(t *testing.T) { + for _, existingOrigin := range []bool{false, true} { + t.Run(map[bool]string{false: "add", true: "update"}[existingOrigin], func(t *testing.T) { + parent := t.TempDir() + mirror := filepath.Join(parent, "mirror.git") + runGit(t, parent, "init", "--bare", "--quiet", mirror) + if existingOrigin { + runGit(t, mirror, "remote", "add", "origin", "https://alice:first-token@github.com/org/repo.git") + } + if err := ensureOrigin(mirror, "https://bob:rotated-token@github.com/org/repo.git"); err != nil { + t.Fatalf("ensureOrigin: %v", err) + } + // Read the stored config value directly. `git remote get-url` + // applies the caller's global insteadOf rules and may report a + // rewritten transport. + got, err := gitOutput(mirror, "config", "--get", "remote.origin.url") + if err != nil { + t.Fatalf("get origin URL: %v", err) + } + if want := "https://github.com/org/repo.git"; got != want { + t.Fatalf("origin URL = %q, want %q", got, want) + } + }) + } +} + func TestCanonicalizeURLVariantsShareKey(t *testing.T) { // All spellings of the same repo must canonicalize identically so they hash // to one mirror.