From ddb829d701b3d3bf5627d24401e79aeae4f9027e Mon Sep 17 00:00:00 2001 From: claude Date: Sun, 23 Aug 2026 11:29:30 -0400 Subject: [PATCH] fix(client): make our credential helper the only one the repo consults Live QA against the beta forge failed its first push with 'Failed to authenticate user' while the config held a valid token. Cause: credential.helper is a list accumulated across system/global/repo config, and this machine has osxkeychain (Xcode gitconfig) plus store (~/.gitconfig). A stale entry for the forge host answered before our helper. The same list is a token leak in the other direction: git calls approve on every helper after a successful auth, so 'store' writes the forge token into ~/.git-credentials in plaintext -- undoing the 0600 config and the no-token-in-URL rule. Confirmed accidentally during QA when a verification clone with a URL-embedded token re-created exactly that entry. Fix: set an empty credential.helper first (git reads that as 'forget the inherited list'), then add ours -- in install_credential_helper and in the git clone inside get. 2 regression tests, one of which drives 'git credential fill' against a poisoned outer helper. 143 tests. --- README.md | 31 ++++++++++++++++++++++++-- client/granthi_sync_client.py | 27 +++++++++++++++++++++- tests/test_client.py | 42 +++++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 487fe8d..3c58f44 100644 --- a/README.md +++ b/README.md @@ -116,6 +116,31 @@ restore point into a *new* directory and refuses a non-empty destination. Someone restoring a backup is already having a bad day; overwriting the files they still have would make the recovery tool the second disaster. +## The credential helper must be the ONLY helper (found by live QA) + +`credential.helper` is a list that accumulates across system, global and repo +config, and git asks every helper in it. A stock mac already has two — +`osxkeychain` from Xcode's gitconfig, and `store` from many people's +`~/.gitconfig` — and they lose in both directions: + +* **reading:** a stale entry for the forge host answers before our helper, so + pushes fail `remote: Failed to authenticate user` long after the token was + rotated, and nothing in this tool's config explains why. This is exactly + how the first live-QA run failed; +* **writing:** git calls `approve` on every helper after a successful auth, + so `store` copies the forge token into `~/.git-credentials` **in + plaintext**. Keeping the token in a 0600 file and out of remote URLs buys + nothing if git then hands it to a plaintext store. + +So `install_credential_helper` (and the `git clone` in `get`) sets an **empty** +`credential.helper` first, which resets the inherited list, then adds ours. +Exactly one helper serves this repo. + +Corollary worth remembering: a token embedded in a remote URL gets saved by +`store` on first use. During QA a verification clone with a URL-embedded +token re-created the very entry that had just been cleaned out. That is the +whole reason this client passes tokens through a helper and never a URL. + ## Device identity `link` mints a uuid on first run and persists it in `~/.granthi-sync/config.json` @@ -356,7 +381,7 @@ deleted it again, `DELETE …/tokens/{id}` returning 204 under basic auth): ## Tests -* `python3 -m unittest discover -s tests` — 141 tests. The v1.2 additions +* `python3 -m unittest discover -s tests` — 143 tests. The v1.2 additions cover: a snapshot capturing uncommitted work while HEAD, the index and the working tree stay byte-identical; snapshots landing outside `refs/heads`; an unchanged tree not being re-pushed; a diverged folder still being backed @@ -368,7 +393,9 @@ deleted it again, `DELETE …/tokens/{id}` returning 204 under basic auth): disk; mode detection; `list` filtering; `get --all` skipping what is already present, defaulting to snapshot mode, and shouting about truncation; `restore` writing a new folder, refusing a non-empty - destination, and leaving the working tree alone. + destination, and leaving the working tree alone; and the credential helper + being the only one the repo consults, proven by driving + `git credential fill` against a deliberately poisoned outer helper. * Earlier suite: autocommit/ff/diverged logic against real temp git repos (including "diverged never touches the remote"), config 0600 handling (including umask-proof creation and a diff --git a/client/granthi_sync_client.py b/client/granthi_sync_client.py index 5c101a7..953ee57 100644 --- a/client/granthi_sync_client.py +++ b/client/granthi_sync_client.py @@ -544,7 +544,27 @@ def credential_helper_value(): def install_credential_helper(folder): - git(folder, "config", "credential.helper", credential_helper_value()) + """Make OUR helper the only one this repo consults. + + credential.helper is a LIST that accumulates across system, global and + repo config, and git asks every helper in order. On a stock mac there are + already two (`osxkeychain` from Xcode's gitconfig, `store` from many + people's ~/.gitconfig), and they lose both ways: + + * reading -- a stale entry for the forge host answers first, so pushes + fail with "Failed to authenticate user" long after the token was + rotated, and nothing in this tool's config explains why; + * writing -- git calls `approve` on every helper after a successful + auth, so `store` copies the forge token into ~/.git-credentials in + PLAINTEXT. Keeping the token in a 0600 file and out of remote URLs + is pointless if git hands it to a plaintext store on first use. + + An empty value resets the inherited list, so replace-all-then-add leaves + exactly one helper: this script. + """ + git(folder, "config", "--replace-all", "credential.helper", "") + git(folder, "config", "--add", "credential.helper", + credential_helper_value()) # -------------------------------------------------------------------------- @@ -756,7 +776,12 @@ def clone_one(cfg, full_name, dest, mode=None): # persists it into the new repo's config. --origin names the remote # 'granthi' up front so `watch` picks the folder up without a rename. proc = subprocess.run( + # The empty -c resets the inherited helper list (see + # install_credential_helper) so a stale keychain/store entry cannot + # answer for the forge host during the clone, and the token cannot + # leak into a plaintext store afterwards. ["git", "clone", + "-c", "credential.helper=", "-c", f"credential.helper={credential_helper_value()}", "--origin", "granthi", clone_url, dest], capture_output=True, text=True) diff --git a/tests/test_client.py b/tests/test_client.py index 7d19213..d360328 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -858,5 +858,47 @@ class TestMarkerRoundTrip(GitScenarioBase): self.assertEqual(client.read_marker(self.local), {}) +class TestCredentialHelperIsolation(GitScenarioBase): + """A repo-local helper is not enough on a normal machine: git consults + system + global helpers too, and they both shadow us and copy the token + into plaintext. Found by live QA against the beta forge, not by a unit + test -- so it gets one now.""" + + def test_install_leaves_exactly_one_helper(self): + run_git(self.local, "config", "--add", "credential.helper", "store") + client.install_credential_helper(self.local) + # --get-all merges system + global + local, so entries inherited from + # the machine still appear. What matters is that the last two are the + # reset and ours: git reads an empty value as "forget every helper + # inherited so far", so nothing before it can answer. + helpers = run_git(self.local, "config", "--get-all", + "credential.helper").splitlines() + self.assertEqual(helpers[-2], "", helpers) + self.assertIn("git-credential", helpers[-1]) + # the repo-level 'store' this test added is gone, not merely outvoted + self.assertNotIn("store", helpers) + + def test_inherited_helper_cannot_answer_for_the_forge(self): + """The end-to-end property: with a poisoned outer helper configured, + the credential git actually resolves is ours.""" + fake = os.path.join(self.tmp, "poison.sh") + with open(fake, "w") as f: + f.write("#!/bin/sh\n" + "echo username=wrong-user\necho password=stale-token\n") + os.chmod(fake, 0o755) + run_git(self.local, "config", "--add", "credential.helper", + f"!{shlex.quote(fake)}") + client.save_config({"gitea_base": "http://forge.example:3041", + "login": "alice", "token": "the-right-token"}) + client.install_credential_helper(self.local) + out = subprocess.run( + ["git", "-C", self.local, "credential", "fill"], + input="protocol=http\nhost=forge.example:3041\n\n", + capture_output=True, text=True, env=dict( + GIT_ENV, GRANTHI_SYNC_HOME=os.environ["GRANTHI_SYNC_HOME"])) + self.assertIn("password=the-right-token", out.stdout) + self.assertNotIn("stale-token", out.stdout) + + if __name__ == "__main__": unittest.main()