From 8ac504fc221d40a451c216770334fc69e0ee7c51 Mon Sep 17 00:00:00 2001 From: Edwin <18327273+thinkbig1979@users.noreply.github.com> Date: Fri, 31 Jul 2026 20:50:18 +0200 Subject: [PATCH] fix(git): supply the stored HTTPS token to every git invocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Capstan encrypted the git HTTPS token, stored it and reported hasHttpsToken=true, but never handed it to a git process. Every pull of a private HTTPS repository died with: fatal: could not read Username for 'https://github.com' gitCommand() exec'd plain git with no credential helper, no askpass, no authenticated URL and no environment (git.go:197). The credential now reaches git through an inline credential.helper snippet installed with -c for the duration of one invocation, reading the secret out of the child process environment. That keeps it out of argv (readable via /proc//cmdline), out of .git/config on disk — and therefore out of the stack's backup snapshot — and out of anything that prints the remote. Rewriting origin to https://user:token@host was rejected for exactly those reasons. gitCmd() is now the single place the backend executes git, so the credential applies to pull, fetch, status and every other subcommand without a hand-maintained list of the ones that reach the network. Settings win over the GIT_HTTPS_TOKEN environment variable; with nothing configured the invocation is unchanged apart from GIT_TERMINAL_PROMPT=0, which turns a would-be hang into an error. git output is scrubbed of the token before it reaches an error string, since that text travels into AppError.Details, the action log and the API response. Tests run git's own smart-HTTP server (git-http-backend over CGI) behind Basic auth, so the pull is proved against a remote that genuinely demands credentials. Verified to fail for the right reason against the unfixed code: "fatal: could not read Username for 'http://127.0.0.1:38441': terminal prompts disabled". agent-os-qqw --- backend/internal/services/git.go | 9 +- backend/internal/services/git_credentials.go | 112 +++++++ .../internal/services/git_credentials_test.go | 299 ++++++++++++++++++ 3 files changed, 415 insertions(+), 5 deletions(-) create mode 100644 backend/internal/services/git_credentials.go create mode 100644 backend/internal/services/git_credentials_test.go diff --git a/backend/internal/services/git.go b/backend/internal/services/git.go index ccc732c..db70f06 100644 --- a/backend/internal/services/git.go +++ b/backend/internal/services/git.go @@ -3,7 +3,6 @@ package services import ( "fmt" "log/slog" - "os/exec" "path/filepath" "runtime/debug" "strconv" @@ -195,13 +194,13 @@ func (s *GitService) getStatusCLI(dirPath string) (*models.GitStatusResult, erro } func (s *GitService) gitCommand(dirPath string, args ...string) (string, error) { - cmd := exec.Command("git", append([]string{"-c", "safe.directory=" + dirPath}, args...)...) - cmd.Dir = dirPath + cmd, token := s.gitCmd(dirPath, args...) output, err := cmd.CombinedOutput() if err != nil { - return "", fmt.Errorf("git %s: %w (%s)", args[0], err, strings.TrimSpace(string(output))) + return "", fmt.Errorf("git %s: %w (%s)", args[0], err, + redactToken(strings.TrimSpace(string(output)), token)) } - return strings.TrimSpace(string(output)), nil + return redactToken(strings.TrimSpace(string(output)), token), nil } func (s *GitService) Pull(dirPath string) (*models.PullResult, error) { diff --git a/backend/internal/services/git_credentials.go b/backend/internal/services/git_credentials.go new file mode 100644 index 0000000..6cc3ffb --- /dev/null +++ b/backend/internal/services/git_credentials.go @@ -0,0 +1,112 @@ +package services + +import ( + "os" + "os/exec" + "strings" +) + +// Environment variable names the credential helper below reads. They are set +// only on the git child process, never exported into capstan's own environment. +const ( + credentialEnvUser = "CAPSTAN_GIT_USERNAME" + credentialEnvToken = "CAPSTAN_GIT_PASSWORD" +) + +// gitCredentialHelper is a shell snippet installed as git's credential helper +// for the duration of a single invocation. It answers only the "get" operation +// and reads the secret out of the child environment. +// +// Everything else was rejected for leaking the token somewhere durable: +// - rewriting origin to https://user:token@host persists the credential into +// .git/config on disk, into that stack's backup snapshot, and into every log +// line or API response that prints the remote (agent-os-qqw); +// - -c http..extraheader="Authorization: Basic ..." puts it in argv, +// where any local process can read it from /proc//cmdline; +// - a GIT_ASKPASS script has to be written to disk and made executable. +// +// The snippet itself contains only variable *names*, so argv stays clean, and +// the -c flag means nothing is written to any config file. +const gitCredentialHelper = `!f() { test "$1" = get && printf 'username=%s\npassword=%s\n' ` + + `"$` + credentialEnvUser + `" "$` + credentialEnvToken + `"; }; f` + +// defaultGitHTTPSUser is used when neither the stored setting nor the +// GIT_HTTPS_USER environment variable names one. Forges that authenticate by +// token ignore the username, but it must not be empty or git re-prompts. +const defaultGitHTTPSUser = "oauth2" + +// httpsCredentials resolves the git HTTPS credential, preferring the value +// stored (encrypted) in settings over the GIT_HTTPS_TOKEN environment variable. +// It returns an empty token when none is configured, in which case git runs +// exactly as before. +func (s *GitService) httpsCredentials() (user, token string) { + if s.db != nil { + // A decrypt failure (wrong STORAGE_KEY) surfaces as an error here; treat + // it as "no credential" and let git report the auth failure, rather than + // failing every git command including the ones that need no remote. + if v, err := s.db.GetSetting("git_https_token"); err == nil { + token = v + } + if v, err := s.db.GetSetting("git_https_user"); err == nil { + user = v + } + } + if s.config != nil { + if token == "" { + token = s.config.GitHTTPSToken + } + if user == "" { + user = s.config.GitHTTPSUser + } + } + if user == "" { + user = defaultGitHTTPSUser + } + return user, token +} + +// gitCmd builds the git child process for dirPath with the HTTPS credential +// attached when one is configured. It also returns the token it used, so the +// caller can redact it from output without resolving (and decrypting) it twice. +// +// The credential is applied to every invocation rather than to a hand-picked +// list of remote-contacting subcommands: pull, fetch, clone, ls-remote and +// `status` with a configured upstream can all reach the network, and an +// omission from such a list is exactly the failure mode being fixed here. git +// only runs the helper when a remote actually challenges it. +func (s *GitService) gitCmd(dirPath string, args ...string) (*exec.Cmd, string) { + gitArgs := []string{"-c", "safe.directory=" + dirPath} + env := append(os.Environ(), "GIT_TERMINAL_PROMPT=0") + + user, token := s.httpsCredentials() + if token != "" { + // The empty assignment first clears any helper inherited from system or + // global config, so ours is the only one consulted. + gitArgs = append(gitArgs, + "-c", "credential.helper=", + "-c", "credential.helper="+gitCredentialHelper, + ) + env = append(env, + credentialEnvUser+"="+user, + credentialEnvToken+"="+token, + ) + } + + cmd := exec.Command("git", append(gitArgs, args...)...) + cmd.Dir = dirPath + cmd.Env = env + return cmd, token +} + +// redactToken replaces every occurrence of token in s with a placeholder. +// +// git never prints the password it received, so this is defence in depth: it +// also covers the case where the operator embedded the same token in the remote +// URL, which git *does* echo back in error messages. Those messages travel into +// AppError.Details, the action log and the API response. +func redactToken(s, token string) string { + if token == "" { + return s + } + return strings.ReplaceAll(s, token, "***") +} diff --git a/backend/internal/services/git_credentials_test.go b/backend/internal/services/git_credentials_test.go new file mode 100644 index 0000000..44959d8 --- /dev/null +++ b/backend/internal/services/git_credentials_test.go @@ -0,0 +1,299 @@ +package services + +import ( + "net/http" + "net/http/cgi" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/thinkbig1979/capstan/backend/internal/config" +) + +// The stored git HTTPS token was encrypted, persisted and reported as present, +// but never handed to any git process (agent-os-qqw). Every pull of a private +// HTTPS repository therefore died with "could not read Username". +// +// Reproducing that needs a remote that actually demands credentials, so these +// tests run git's own smart-HTTP server (git-http-backend, over CGI) behind +// Basic auth. No network leaves the machine and no real token is involved. + +const ( + testGitUser = "oauth2" + testGitToken = "test-pat-4f8c1e9b-do-not-log" +) + +// runGit executes git in dir and fails the test on a non-zero exit. +func runGit(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_TERMINAL_PROMPT=0", + "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null", + "GIT_AUTHOR_NAME=Test", "GIT_AUTHOR_EMAIL=test@test.invalid", + "GIT_COMMITTER_NAME=Test", "GIT_COMMITTER_EMAIL=test@test.invalid", + ) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } + return strings.TrimSpace(string(out)) +} + +// authenticatedHTTPRepo builds a bare repository, serves it over smart HTTP +// behind Basic auth, and returns a working clone whose origin URL carries NO +// embedded credentials. advance() adds a commit to the remote so a subsequent +// pull has real work to do and cannot pass as a no-op "Already up to date". +func authenticatedHTTPRepo(t *testing.T) (local string, advance func() string) { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not on PATH") + } + execPath, err := exec.Command("git", "--exec-path").Output() + if err != nil { + t.Skipf("git --exec-path failed: %v", err) + } + backend := filepath.Join(strings.TrimSpace(string(execPath)), "git-http-backend") + if _, err := os.Stat(backend); err != nil { + t.Skipf("git-http-backend not available: %v", err) + } + + base := t.TempDir() + root := filepath.Join(base, "srv") + bare := filepath.Join(root, "repo.git") + seed := filepath.Join(base, "seed") + local = filepath.Join(base, "local") + for _, d := range []string{root, seed} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatalf("mkdir %s: %v", d, err) + } + } + + runGit(t, root, "init", "--bare", "-b", "main", bare) + + runGit(t, seed, "init", "-b", "main") + if err := os.WriteFile(filepath.Join(seed, "docker-compose.yml"), []byte("services: {}\n"), 0o644); err != nil { + t.Fatalf("write seed file: %v", err) + } + runGit(t, seed, "add", "-A") + runGit(t, seed, "commit", "-m", "initial") + runGit(t, seed, "remote", "add", "origin", bare) + runGit(t, seed, "push", "-u", "origin", "main") + + // Serve the bare repo over smart HTTP, demanding Basic auth on every request. + handler := &cgi.Handler{ + Path: backend, + Env: []string{ + "GIT_PROJECT_ROOT=" + root, + "GIT_HTTP_EXPORT_ALL=1", + "PATH=" + os.Getenv("PATH"), + }, + } + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + user, pass, ok := r.BasicAuth() + if !ok || user != testGitUser || pass != testGitToken { + w.Header().Set("WWW-Authenticate", `Basic realm="capstan-test"`) + w.WriteHeader(http.StatusUnauthorized) + return + } + handler.ServeHTTP(w, r) + })) + t.Cleanup(srv.Close) + + // Clone from the filesystem, then point origin at the authenticated URL. The + // remote URL deliberately contains no credentials — supplying them is exactly + // what is under test. + runGit(t, base, "clone", bare, local) + runGit(t, local, "remote", "set-url", "origin", srv.URL+"/repo.git") + + advance = func() string { + if err := os.WriteFile(filepath.Join(seed, "post-restore-canary.env"), []byte("CANARY=1\n"), 0o644); err != nil { + t.Fatalf("write canary: %v", err) + } + runGit(t, seed, "add", "-A") + runGit(t, seed, "commit", "-m", "canary") + runGit(t, seed, "push", "origin", "main") + return runGit(t, seed, "rev-parse", "HEAD") + } + + return local, advance +} + +// gitServiceWithStoredToken returns a GitService whose database holds the git +// HTTPS credential exactly as the settings handler stores it: encrypted at rest +// under the git_https_token key. +func gitServiceWithStoredToken(t *testing.T) *GitService { + t.Helper() + db := newTestDBWithEncryptor(t) + if err := db.SetSetting("git_https_user", testGitUser); err != nil { + t.Fatalf("store git_https_user: %v", err) + } + if err := db.SetSetting("git_https_token", testGitToken); err != nil { + t.Fatalf("store git_https_token: %v", err) + } + return NewGitService(&config.Config{}, db) +} + +// TestPull_UsesStoredHTTPSToken is the regression test for agent-os-qqw. With a +// token stored in settings and none embedded in the remote URL, a pull of a +// credential-protected repository must fast-forward. +func TestPull_UsesStoredHTTPSToken(t *testing.T) { + local, advance := authenticatedHTTPRepo(t) + want := advance() + + svc := gitServiceWithStoredToken(t) + + result, err := svc.Pull(local) + if err != nil { + t.Fatalf("Pull with a stored HTTPS token failed: %v", err) + } + if result.CurrentCommit != want { + t.Errorf("HEAD after pull = %q, want the remote's new commit %q", result.CurrentCommit, want) + } + if result.PreviousCommit == result.CurrentCommit { + t.Error("pull reported no change; the remote had a new commit to fetch") + } +} + +// TestPull_WithoutStoredTokenStillFails guards the test harness itself: the +// remote really does demand credentials, so a service with no token configured +// must fail. Without this, TestPull_UsesStoredHTTPSToken would also pass against +// an unauthenticated remote and prove nothing. +func TestPull_WithoutStoredTokenStillFails(t *testing.T) { + local, advance := authenticatedHTTPRepo(t) + advance() + + svc := NewGitService(&config.Config{}, newTestDBWithEncryptor(t)) + + if _, err := svc.Pull(local); err == nil { + t.Fatal("expected the pull to fail without credentials, but it succeeded") + } +} + +// TestPull_TokenNeverPersistsToGitConfig covers the leak the fix must not +// introduce: rewriting origin to embed the token would put it in .git/config on +// disk, inside that stack's backup snapshot, and in every log printing the +// remote. +func TestPull_TokenNeverPersistsToGitConfig(t *testing.T) { + local, advance := authenticatedHTTPRepo(t) + advance() + + svc := gitServiceWithStoredToken(t) + if _, err := svc.Pull(local); err != nil { + t.Fatalf("Pull with a stored HTTPS token failed: %v", err) + } + + cfg, err := os.ReadFile(filepath.Join(local, ".git", "config")) + if err != nil { + t.Fatalf("read .git/config: %v", err) + } + if strings.Contains(string(cfg), testGitToken) { + t.Errorf(".git/config contains the token:\n%s", cfg) + } + + remote := runGit(t, local, "remote", "get-url", "origin") + if strings.Contains(remote, testGitToken) { + t.Errorf("origin URL contains the token: %q", remote) + } +} + +// TestGitCmd_TokenTravelsInEnvNotArgv pins down *how* the credential reaches +// git. argv is world-readable through /proc//cmdline, so the token must +// appear only in the child's environment. +func TestGitCmd_TokenTravelsInEnvNotArgv(t *testing.T) { + svc := gitServiceWithStoredToken(t) + + cmd, token := svc.gitCmd(t.TempDir(), "pull", "--ff-only") + + if token != testGitToken { + t.Errorf("gitCmd returned token %q, want the stored one", token) + } + for i, arg := range cmd.Args { + if strings.Contains(arg, testGitToken) { + t.Errorf("argv[%d] contains the token: %q", i, arg) + } + } + + var sawUser, sawToken bool + for _, kv := range cmd.Env { + switch kv { + case credentialEnvUser + "=" + testGitUser: + sawUser = true + case credentialEnvToken + "=" + testGitToken: + sawToken = true + } + } + if !sawUser || !sawToken { + t.Errorf("credential missing from the child environment (user=%v token=%v)", sawUser, sawToken) + } + + if !strings.Contains(strings.Join(cmd.Args, " "), "credential.helper="+gitCredentialHelper) { + t.Errorf("credential helper not installed; args = %v", cmd.Args) + } +} + +// TestGitCmd_NoCredentialWhenNoneConfigured keeps the change inert for the +// common case: with nothing stored, git is invoked exactly as before. +func TestGitCmd_NoCredentialWhenNoneConfigured(t *testing.T) { + svc := NewGitService(&config.Config{}, newTestDBWithEncryptor(t)) + + cmd, token := svc.gitCmd(t.TempDir(), "status", "--porcelain") + + if token != "" { + t.Errorf("expected no token, got %q", token) + } + if strings.Contains(strings.Join(cmd.Args, " "), "credential.helper") { + t.Errorf("credential helper installed with no credential configured: %v", cmd.Args) + } + for _, kv := range cmd.Env { + if strings.HasPrefix(kv, credentialEnvToken+"=") { + t.Errorf("credential env var set with no credential configured: %q", kv) + } + } +} + +// TestGitCommand_RedactsTokenFromOutput proves the redaction is wired into the +// real error path, not just available as a helper. git's error text flows into +// AppError.Details, the action log and the API response, so a token that +// reaches git's output would be persisted. +func TestGitCommand_RedactsTokenFromOutput(t *testing.T) { + local, _ := authenticatedHTTPRepo(t) + svc := gitServiceWithStoredToken(t) + + // rev-parse echoes an unknown revision back verbatim, which is the cheapest + // way to get the token into genuine git output. + _, err := svc.gitCommand(local, "rev-parse", testGitToken) + if err == nil { + t.Fatal("expected rev-parse of a bogus revision to fail") + } + if strings.Contains(err.Error(), testGitToken) { + t.Errorf("git error text leaks the token: %v", err) + } + if !strings.Contains(err.Error(), "***") { + t.Errorf("expected the token to be replaced by a placeholder, got: %v", err) + } +} + +// TestHTTPSCredentials_SettingsBeatEnvironment records the precedence: an +// operator who stores a token through Settings expects it to win over whatever +// GIT_HTTPS_TOKEN the container was started with. +func TestHTTPSCredentials_SettingsBeatEnvironment(t *testing.T) { + svc := gitServiceWithStoredToken(t) + svc.config = &config.Config{GitHTTPSUser: "env-user", GitHTTPSToken: "env-token"} + + user, token := svc.httpsCredentials() + if user != testGitUser || token != testGitToken { + t.Errorf("got %q/%q, want the stored settings values", user, token) + } + + // With nothing stored, the environment is the fallback rather than nothing. + bare := NewGitService(&config.Config{GitHTTPSUser: "env-user", GitHTTPSToken: "env-token"}, newTestDBWithEncryptor(t)) + user, token = bare.httpsCredentials() + if user != "env-user" || token != "env-token" { + t.Errorf("got %q/%q, want the environment values", user, token) + } +}