From 28a5203d0165b091493a4820596c79aaaa8c3d15 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 5 Oct 2026 21:38:24 +0200 Subject: [PATCH] R-616 (on top of R-615): the catalog clone stores no credentials Replaces 365eff6 for main, which now carries R-615 (0b349e2). The plain URL is cloned; the Basic credentials ride per git command as http..extraHeader via GIT_CONFIG_COUNT. R-615's origin compare now compares credential-free forms against the configured URL, so a set token never re-clones; a stored origin with user:token@ is rewritten without it. Pinned by TestR616_* incl. TestR616_TokenSetSameRepoNoRecloneOriginClean. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS --- .../internal/sync/r616_credentials_test.go | 172 ++++++++++++++++++ controller/internal/sync/sync.go | 88 ++++++--- 2 files changed, 239 insertions(+), 21 deletions(-) create mode 100644 controller/internal/sync/r616_credentials_test.go diff --git a/controller/internal/sync/r616_credentials_test.go b/controller/internal/sync/r616_credentials_test.go new file mode 100644 index 0000000..0e36228 --- /dev/null +++ b/controller/internal/sync/r616_credentials_test.go @@ -0,0 +1,172 @@ +package sync + +import ( + "bytes" + "encoding/base64" + "log" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// R-616: the catalog credentials must never be stored in the clone. A fake https remote is served from +// a local repository through git's own url..insteadOf (in a test-only HOME), so the real clone +// and fetch paths run with no network: git REWRITES the URL it dials but STORES the URL it was given — +// which is exactly the URL the product chose, credentialed or not. +func r616Fixture(t *testing.T) (s *Syncer, logs *bytes.Buffer, src string) { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not on PATH") + } + root := t.TempDir() + src = filepath.Join(root, "src") + run := func(dir string, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + home := filepath.Join(root, "home") + os.MkdirAll(home, 0o755) + t.Setenv("HOME", home) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) + t.Setenv("GIT_CONFIG_NOSYSTEM", "1") + gitcfg := "[user]\n\tname = t\n\temail = t@example.invalid\n" + + "[url \"file://" + src + "\"]\n" + + "\tinsteadOf = https://user:tok-SECRET@catalog.example.invalid/admin/catalog.git\n" + + "\tinsteadOf = https://catalog.example.invalid/admin/catalog.git\n" + if err := os.WriteFile(filepath.Join(home, ".gitconfig"), []byte(gitcfg), 0o644); err != nil { + t.Fatal(err) + } + os.MkdirAll(src, 0o755) + run(src, "init", "-q", "-b", "main") + os.WriteFile(filepath.Join(src, "README"), []byte("x\n"), 0o644) + run(src, "add", "README") + run(src, "commit", "-q", "-m", "init") + + s = newTestSyncer(t, "https://catalog.example.invalid/admin/catalog.git") + s.cfg.Git.Username = "user" + s.cfg.Git.Token = "tok-SECRET" + logs = &bytes.Buffer{} + s.logger = log.New(logs, "", 0) + return s, logs, src +} + +func storedOrigin(t *testing.T, dir string) string { + t.Helper() + out, err := exec.Command("git", "-C", dir, "config", "--get", "remote.origin.url").Output() + if err != nil { + t.Fatalf("read origin: %v", err) + } + return strings.TrimSpace(string(out)) +} + +func TestR616_CloneStoresNoCredentials(t *testing.T) { + s, logs, _ := r616Fixture(t) + if err := s.gitCloneOrPull(); err != nil { + t.Fatalf("clone: %v", err) + } + origin := storedOrigin(t, s.cacheDir) + if strings.Contains(origin, "@") || strings.Contains(origin, "tok-SECRET") { + t.Errorf("the clone's stored origin carries credentials: %q", origin) + } + cfgBytes, _ := os.ReadFile(filepath.Join(s.cacheDir, ".git", "config")) + if strings.Contains(string(cfgBytes), "tok-SECRET") { + t.Errorf(".git/config holds the token in the clear:\n%s", cfgBytes) + } + // A pull (fetch + reset) still works on the clean clone. + if err := s.gitCloneOrPull(); err != nil { + t.Fatalf("pull: %v", err) + } + if strings.Contains(logs.String(), "tok-SECRET") { + t.Errorf("the token reached the log:\n%s", logs.String()) + } +} + +// A clone made BEFORE the fix (origin with userinfo) is scrubbed on the next pull. +func TestR616_PreFixCloneIsScrubbedOnPull(t *testing.T) { + s, logs, _ := r616Fixture(t) + cmd := exec.Command("git", "clone", "-q", "--depth", "1", "--branch", "main", + "https://user:tok-SECRET@catalog.example.invalid/admin/catalog.git", s.cacheDir) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("pre-fix clone: %v\n%s", err, out) + } + if !strings.Contains(storedOrigin(t, s.cacheDir), "tok-SECRET") { + t.Fatal("fixture: the pre-fix clone should carry the token") + } + if err := s.gitCloneOrPull(); err != nil { + t.Fatalf("pull: %v", err) + } + if got := storedOrigin(t, s.cacheDir); got != "https://catalog.example.invalid/admin/catalog.git" { + t.Errorf("stored origin not scrubbed: %q", got) + } + if strings.Contains(logs.String(), "tok-SECRET") { + t.Errorf("the token reached the log:\n%s", logs.String()) + } +} + +// The credentials still reach git: the header is scoped to the remote's origin and carries the same +// Basic pair the URL form sent. Checked with git's own URL matcher, no network. +func TestR616_AuthHeaderScopedToTheRemote(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not on PATH") + } + s := newTestSyncer(t, "https://catalog.example.invalid/admin/catalog.git") + if env := s.gitAuthEnv(); env != nil { + t.Errorf("no credentials configured -> no auth env, got %v", env) + } + s.cfg.Git.Username, s.cfg.Git.Token = "user", "tok-SECRET" + env := s.gitAuthEnv() + want := "Authorization: Basic " + base64.StdEncoding.EncodeToString([]byte("user:tok-SECRET")) + match := func(url string) string { + cmd := exec.Command("git", "config", "--get-urlmatch", "http.extraHeader", url) + cmd.Env = append(os.Environ(), env...) + out, _ := cmd.Output() + return strings.TrimSpace(string(out)) + } + t.Setenv("GIT_CONFIG_NOSYSTEM", "1") + t.Setenv("HOME", t.TempDir()) + if got := match("https://catalog.example.invalid/admin/catalog.git"); got != want { + t.Errorf("header for the remote = %q, want the Basic pair", got) + } + if got := match("https://other.example.invalid/x.git"); got != "" { + t.Errorf("the header must not reach another host, got %q", got) + } + s.cfg.Git.RepoURL = "/local/path/catalog" + if env := s.gitAuthEnv(); env != nil { + t.Errorf("a non-https remote gets no auth env, got %v", env) + } +} + +// R-616 × R-615: with a token set and the SAME repository configured, repeated syncs never re-clone +// (the configured URL carries no credentials to differ from the stored origin) and the stored origin +// stays free of '@'. A re-clone is detected by a marker file planted inside the cache: RemoveAll would +// take it with it. +func TestR616_TokenSetSameRepoNoRecloneOriginClean(t *testing.T) { + s, logs, _ := r616Fixture(t) + if err := s.gitCloneOrPull(); err != nil { + t.Fatalf("clone: %v", err) + } + marker := filepath.Join(s.cacheDir, ".git", "r616-no-reclone") + if err := os.WriteFile(marker, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + for i := 0; i < 3; i++ { + if err := s.gitCloneOrPull(); err != nil { + t.Fatalf("pull %d: %v", i, err) + } + } + if _, err := os.Stat(marker); err != nil { + t.Error("the cache was RE-CLONED on a same-repo sync with a token set") + } + if o := storedOrigin(t, s.cacheDir); strings.Contains(o, "@") { + t.Errorf("stored origin carries credentials: %q", o) + } + if strings.Contains(logs.String(), "re-cloning") || strings.Contains(logs.String(), "tok-SECRET") { + t.Errorf("unexpected re-clone or token in the log:\n%s", logs.String()) + } +} diff --git a/controller/internal/sync/sync.go b/controller/internal/sync/sync.go index 0457eea..df4027e 100644 --- a/controller/internal/sync/sync.go +++ b/controller/internal/sync/sync.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "crypto/sha256" + "encoding/base64" "encoding/hex" "fmt" "io" @@ -300,7 +301,10 @@ func (s *Syncer) gitCloneOrPull() error { // Clone s.logger.Printf("[INFO] [sync] Cloning %s → %s", s.cfg.Git.RepoURL, s.cacheDir) args := []string{"clone", "--depth", "1", "--branch", s.cfg.Git.Branch} - repoURL := s.buildRepoURL() + // R-616: clone the PLAIN URL — git persists the clone URL as `origin`, so a credentialed URL + // left the token readable in .git/config and printed by any `git remote -v`. The credentials + // ride per command in the environment instead (gitAuthEnv). + repoURL := s.cfg.Git.RepoURL args = append(args, repoURL, s.cacheDir) if s.isDebug() { s.logger.Printf("[DEBUG] [sync] git clone URL: %s, branch: %s, cacheDir: %s", maskRepoURL(repoURL), s.cfg.Git.Branch, s.cacheDir) @@ -312,22 +316,25 @@ func (s *Syncer) gitCloneOrPull() error { s.removeGitLockFiles() // R-615: the clone remembers the repository it was made from. A changed `git.repo_url` used to be - // INERT — every later fetch went to the stored origin and reported success. Compare and follow: - // a different repository (credentials aside) → drop the cache and clone the new one; the same - // repository with different credentials (a rotated token) → point origin at the new URL. + // INERT — every later fetch went to the stored origin and reported success. Compare and follow: a + // different repository (credentials aside) → drop the cache and clone the new one. + // R-616: credentials are NEVER part of the stored origin (they ride per command, gitAuthEnv), so the + // comparison is between credential-free forms, and a stored origin that still carries a + // `user:token@` (a clone made before R-616) is rewritten without it. A rotated token therefore needs + // no origin change at all. Pinned by TestR616_TokenSetSameRepoNoRecloneOriginClean. if cur, err := s.gitOutput(s.cacheDir, "config", "--get", "remote.origin.url"); err != nil { s.logger.Printf("[WARN] [sync] cannot read the catalog cache's origin (%v) — fetching from it as before", err) - } else if want := s.buildRepoURL(); cur != want { - if stripURLCreds(cur) != stripURLCreds(want) { - s.logger.Printf("[WARN] [sync] git.repo_url changed (cache was cloned from %s, config says %s) — re-cloning the catalog cache from the configured repository (R-615)", maskRepoURL(cur), maskRepoURL(want)) - if err := os.RemoveAll(s.cacheDir); err != nil { - return fmt.Errorf("removing the catalog cache for a re-clone: %w", err) - } - return s.gitCloneOrPull() + } else if want := s.cfg.Git.RepoURL; stripURLCreds(cur) != stripURLCreds(want) { + s.logger.Printf("[WARN] [sync] git.repo_url changed (cache was cloned from %s, config says %s) — re-cloning the catalog cache from the configured repository (R-615)", maskRepoURL(cur), maskRepoURL(want)) + if err := os.RemoveAll(s.cacheDir); err != nil { + return fmt.Errorf("removing the catalog cache for a re-clone: %w", err) } - s.logger.Printf("[INFO] [sync] catalog repository credentials changed — updating the cache's origin (R-615)") - if err := s.gitCmd(s.cacheDir, "remote", "set-url", "origin", want); err != nil { - return fmt.Errorf("git remote set-url: %w", err) + return s.gitCloneOrPull() + } else if clean := stripURLCreds(cur); clean != cur { + if err := s.gitCmd(s.cacheDir, "remote", "set-url", "origin", clean); err != nil { + s.logger.Printf("[WARN] [sync] could not remove stored credentials from the catalog clone origin (%s): %v", maskRepoURL(cur), err) + } else { + s.logger.Printf("[INFO] [sync] removed stored credentials from the catalog clone origin (R-616): now %s", clean) } } @@ -362,14 +369,50 @@ func (s *Syncer) removeGitLockFiles() { } } -// buildRepoURL constructs the repo URL with optional auth credentials. -func (s *Syncer) buildRepoURL() string { - url := s.cfg.Git.RepoURL - if s.cfg.Git.Username != "" && s.cfg.Git.Token != "" { - // Inject credentials into HTTPS URL: https://user:token@host/path - url = strings.Replace(url, "https://", fmt.Sprintf("https://%s:%s@", s.cfg.Git.Username, s.cfg.Git.Token), 1) +// gitAuthEnv returns the environment that authenticates one git command, or nil when no credentials +// are configured or the remote is not HTTPS. +// +// R-616: the credentials used to be injected into the clone URL (https://user:token@host/…), which +// git then stored as the clone's `origin` — a plaintext token in /catalog-cache/.git/config, +// printed by every `git remote -v`. They now travel as an `http..extraHeader` +// carrying the same HTTP Basic credentials the URL form sent, set through GIT_CONFIG_COUNT (git ≥ +// 2.31; the runtime image is bookworm, 2.39). The environment is neither persisted by git nor in +// the process argv nor in any log line here. The header is scoped to the remote's own origin so a +// redirect to another host never receives it. Pinned by TestR616_CloneStoresNoCredentials. +func (s *Syncer) gitAuthEnv() []string { + u, p := s.cfg.Git.Username, s.cfg.Git.Token + if u == "" || p == "" { + return nil } - return url + origin := httpsOrigin(s.cfg.Git.RepoURL) + if origin == "" { + return nil + } + basic := base64.StdEncoding.EncodeToString([]byte(u + ":" + p)) + return []string{ + "GIT_CONFIG_COUNT=1", + "GIT_CONFIG_KEY_0=http." + origin + ".extraHeader", + "GIT_CONFIG_VALUE_0=Authorization: Basic " + basic, + } +} + +// httpsOrigin returns "https://host[:port]/" for an https URL (userinfo dropped), or "" otherwise. +func httpsOrigin(raw string) string { + rest, ok := strings.CutPrefix(raw, "https://") + if !ok { + return "" + } + host := rest + if i := strings.IndexByte(rest, '/'); i >= 0 { + host = rest[:i] + } + if i := strings.LastIndexByte(host, '@'); i >= 0 { + host = host[i+1:] + } + if host == "" { + return "" + } + return "https://" + host + "/" } // copyTemplates copies docker-compose.yml and .felhom.yml from the catalog cache @@ -696,6 +739,9 @@ func (s *Syncer) runGitInDir(ctx context.Context, dir string, args ...string) er if dir != "" { cmd.Dir = dir } + if env := s.gitAuthEnv(); env != nil { + cmd.Env = append(os.Environ(), env...) + } var stderr bytes.Buffer cmd.Stdout = io.Discard