fix(git,auth,remotes): sever ambient credential chain for push, tag, and delete ops
Brought to you by:
thebguy
Menu
▾
▴
#74 fix(git,auth,remotes): sever ambient credential chain for push, tag, and delete ops
Originally created by: theBGuy
Originally owned by: theBGuy
This change ensures that all network mutating Git operations (push, tag, remote branch/tag deletion, and fork PR pushes) reliably use the signed-in GitHub or GitLab CLI identity, never falling back to an outdated system credential that could cause "Repository not found" errors. This is accomplished by explicitly severing git's ambient credential helper chain and injecting the authenticated CLI helper for these operations, eliminating identity confusion caused by stale keychain entries on macOS/Windows.
Credential Isolation for Git Operations
- Adds credential chain severing and CLI injection logic for mutating network ops in:
src-tauri/src/git/branches.rs(remote branch delete uses provider-CLI credentials)src-tauri/src/git/ops.rs(tag push/delete uses provider-CLI credentials)src-tauri/src/github/pr.rs(PR fork pushes use provider-CLI credentials)- All now use
run_git_mutating_with_credsinstead ofrun_git_mutatingand pull config from the new logic
Provider Credential Configuration
- Refactors helper construction so each remote operation gets a
[reset, helper]credential pair, severing git’s accumulated helpers before installing the CLI (never letting ambient helpers shadow the intended identity) src-tauri/src/forge/github.rsdefinesgithub_credential_entries, adds memoized per-hostgh_authenticatedcheck and a TTL cache for host authentication statussrc-tauri/src/forge/gitlab.rsmirrors the entry structure withgitlab_credential_entries; its auth gate reads glab's confighosts:keys viaknown_hosts()— a per-call file read with no subprocess, so it needs no TTL cache- Both provider modules only inject helper entries when the respective CLI is both installed and holds a stored token/session for the repo’s host; otherwise, ambient credential helpers are left unchanged
- Clarifies credential config logic in
src-tauri/src/forge/mod.rsto reflect new credential sequestering strategy and accurate fail-open/closed behaviors
Git Command Execution
- Exposes
run_git_mutating_with_credsinsrc-tauri/src/git/remote.rs, enabling all network-modifying commands to use injected provider-specific credentials - Updates all call sites to use this function with the computed per-remote credential config
Testing and Documentation
- Strengthens helper entry format tests in both provider modules and adds tests for the GitHub TTL authentication cache in
src-tauri/src/forge/github.rs(GitLab's gate is an uncached config read, so it has no cache to test) - Adds detailed doc comments in all affected areas to record helper chain-resetting semantics and source of registered helpers
- Updates changelog (
changelog.d/fixed-credential-helper-severs-ambient-chain.md) with a clear note about the behavioral fix for network ops when stale credentials are present
Related
Originally posted by: cloudflare-workers-and-pages[bot]
Deploying gitdesktop with
Cloudflare Pages
41d4e2bView logs
Originally posted by: theBGuy
🤖 GitDesktop AI security audit ·
opus· automatedI traced the three sinks that matter in this change:
Token leakage —
gh_authenticatedrunsgh auth token --hostname <host>viarun_gh_raw, which captures stdout intoGhOutput.stdoutand returns it; the caller reads onlyout.codeand discards the output.run_gh_rawdoes not log stdout/stderr. The token never reaches a log or a formatted string. Safe.Config/command injection via
host—hostcomes fromremote_host(clone_url), which strips the scheme, dropsuser@, and takes the segment before the first/or:, then lowercases it. The result is interpolated intocredential.https://{host}.helper=…and passed to git as a separate argv element (with_credentialspushes"-c"then the string; no shell). There's no shell to break out of, and even a malicious remote host only scopes a credential-helper entry to that host — where the CLI helper returns a token only if the user is actually authenticated to it (and the newgh_authenticated/known_hostsgate suppresses injection otherwise). No token is placed in the URL or the-cvalue.Ambient-chain reset — the
[reset, helper]pair severs earlier helpers and installs the signed-in CLI as the sole helper; the auth gate fails open to git's ambient helpers only when the CLI isn't authenticated for that host. This is a hardening, and stale-positive cache entries (60s TTL) can't leak a token because the CLI helper itself returns nothing when signed out.No security issues in these changes.
Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedVerified:
run_git_mutatingremains imported and used inbranches.rs/ops.rs, andpr.rshas no remaining uses (so its import removal is clean — no build break).credential_config_for_remotealways returnsOk(...), so the new.await?at the call sites never fails the op. The core mechanism (blank-c credential.https://<host>.helper=reset clears the accumulated helper list, then the CLI helper is installed as sole helper) is correct per gitcredentials(7), and the-creset lands after all config files so it also clears system/globalosxkeychain/gcm entries.Summary
Sound, well-documented change that severs git's ambient credential chain and injects the signed-in
gh/glabhelper for push/tag/branch-delete/fork-push ops, gated so ambient behavior is preserved when the CLI is absent or not signed in. The pure format + TTL-cache logic is unit-tested and the plumbing is correct. Nothing here is merge-blocking; one behavioral edge deserves attention.Correctness / edge cases
forge/github.rs::gh_authenticated/forge/gitlab.rs::clone_credential_config(the auth gate) combined with the new reset entry. The gate confirms a credential exists locally, not that it works, and the reset removes the ambient fallback. Concrete trigger: a user whosegh/glabtoken has been revoked server-side or is an expired PAT, but who has a valid credential in git-credential-manager / osxkeychain for that host. Before this change the injected CLI helper was additive, so ambient answered and the push succeeded; nowcredential_config_for_remote(repo, "origin")→clone_credential_configreturns[reset, helper],run_git_mutating_with_credsprefixes-c credential.https://host.helper=(clearing ambient) then the CLI helper — the stale CLI token is the only helper, sogit_push_tag_core/git_delete_remote_branch_core/gh_pr_create_corenow hard-fail with "authentication failed" and no fallback. This is the exact inverse of the motivating bug.known_hosts().contains(host), i.e. mere presence of ahosts:key inconfig.yml(which persists after a token expires — seeglab.rs::known_hosts), so it never checks validity at all and the hard-fail is persistent for an expired-PAT user.gh auth token --hostnamestill returns exit 0 for a stored-but-revoked token, and the 60s TTL cache means a mid-sessiongh auth logoutleaves a staleauthed=truefor up to a minute, during which the reset severs ambient and the push hard-fails.gh/glabtoken for<host>may be stale — reconnect" hint rather than a bare git auth error. If you choose to accept the tradeoff, note it explicitly in the doc comment (the current comment frames the gate as "authenticated for host", which overstates whatgh auth token/known_hostsactually prove).Nits
forge/github.rs::gh_authenticated,Err(_) => false. A transientghtimeout (GH_TIMEOUT) or spawn hiccup returnsfalse→ inject nothing → ambient runs, which is precisely the stale-keychain path this PR fixes, so a flaky probe silently reopens the bug for that one op. It's the correct fail-open direction, but consider distinguishing "gh missing" (fail-open is right) from "gh present but probe errored" (you already know gh resolves, sinceresolve_namedsucceeded upstream) — e.g. treat a timeout on a resolvable gh as authenticated rather than unauthenticated. Low stakes given the 60s cache smooths most cases.Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
Round-1 triage (general review + security audit)
should-fix (stale CLI credential + valid ambient → hard-fail): confirmed, accepted — ambient-fallback retry in the next push. The finding is the exact inverse of the motivating bug, and the analysis is right: the gates prove a credential exists (
gh auth tokenis a local read;known_hosts()is a config-file read that persists past PAT expiry), not that it works. Fix landing in the next push:run_git_mutating_with_credsgains a one-shot ambient fallback: when injected entries are present and the run fails with an auth-class error (authentication failed/could not read Username/repository not found— the third because GitHub 404s a valid-but-unauthorized identity on a private repo), it re-runs once with no injected config so git's ambient chain gets its shot. Retry safety: HTTPS auth happens at ref negotiation, before any server-side ref update, so a failed-auth push has mutated nothing. This closes both directions — valid CLI wins deterministically; stale/revoked CLI + valid ambient falls back and succeeds; both-stale fails honestly (surfacing the ambient attempt's error, which names the true end state).gh auth logoutwindow and the persistent glab expired-PAT case are both covered by the same fallback. On the "targeted reconnect hint" alternative: the forge session-health surface shipped in [#60] already detects dead gh/glab sessions and offers reconnect — that existing surface serves the hint role.nit (
Err(_) => falseon a transient probe error): accepted, with the upgrade the review implies. Once the fallback exists the calculus flips: treating a probe error on a resolvable gh as authenticated (optimistic injection) is safe in both directions — if gh is actually signed out, the injected helper returns nothing and the fallback restores ambient; if gh is fine, we keep the deterministic identity instead of silently reopening the stale-keychain path for that one op. Changing to optimistic in the next push.Security audit: no action needed — the three traced sinks match the implementation's intent (exit-code-only read of
gh auth token, argv-separated-cvalues so no shell boundary, gate-suppressed injection).Posted by GitDesktop — automated agent comment, verify before acting on it.
Related
Tickets:
#60Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedVerified against the code:
credential_config_for_remotereturnsOk(...)on every path (mod.rs:171–201), so the new.await?at the tag/delete/fork call sites can never fail the op;with_credentialsprefixes-c <entry>pairs in Vec order (reset first, helper second), matching gitcredentials(7); andfetch --prune/pullalready route throughrun_git_mutating_with_creds(remote.rs:293, 435), so the changelog's "pull, fetch" claim is accurate rather than an overclaim.Summary
Sound, well-documented follow-up: it extends the reset+CLI-helper credential injection to tag push/delete, remote-branch delete, and fork PR pushes, and — the substantive new behavior — adds a one-shot ambient-credential fallback so that severing git's helper chain can't hard-fail a user whose CLI token is stale but whose keychain credential still works. The classifier is unit-tested and the retry is placed correctly. No blockers.
Resolved since last review
run_git_mutating_with_credsretries exactly once with no injected config whencredis non-empty and the first attempt failsis_auth_class_failure(remote.rs:99–105). The retry is correctly gated on!cred.is_empty()and onAppError::Git, uses the originalsubargs without the reset, and is safe for every op that reaches it — all are pushes/fetch/pull where HTTPS auth (and therepository not found404) resolves before any server-side ref update, so no double-apply. The doc comments now state what the gates actually prove ("has a STORED token/session", not "is authenticated").Err(_) => falsereopening the stale-keychain bug on a transient probe (prior nit). NowErr(_) => true(github.rs:828) — optimistic uncached inject, which the fallback makes safe both ways.Correctness
git/remote.rs::is_auth_class_failure, the"repository not found"signature. Including 404 in the fallback set is necessary for the primary fix (a CLI identity that lacks access to a private repo surfaces as not-found, not as an auth error), but it also means the retry fires on a genuinely-missing repo and, in the narrow window where the correct CLI identity hits a transient GitHub 404 (replication lag / incident) while an ambient credential holds access, the mutation completes under the ambient identity — the very identity this PR resets away. Blast radius is limited (commit authorship comes from author/committer config, not the pushing credential), so this is a considered tradeoff rather than a bug; worth a one-line code comment on therepository not foundarm calling out that the fallback can change which identity performs the op, so a future reader doesn't widen the classifier further.Everything else checks out. The
Err(_) => trueoptimistic inject is self-consistent with the fallback (a genuinely signed-out gh yields "could not read Username" → ambient retry), the reset/=-terminator format is byte-identical to the historical helper entry in entry[1], and the classifier's negative tests (merge conflict, DNS failure) correctly keep those out of the retry path.Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
Round-2 triage
nit (identity-switch caveat on the
repository not foundarm): accepted, applied in the next push — exactly the suggested shape: a comment on that classifier arm stating that on a transient not-found for the correct CLI identity, the ambient retry can complete the op under a different identity than the severed CLI one — accepted tradeoff (push identity ≠ commit authorship), with an explicit "don't widen this classifier further" so the boundary holds. No behavior change; 737 tests + clippy re-verified.All round-1 items and the three inline threads were confirmed resolved by this round — nothing else outstanding on our side.
Posted by GitDesktop — automated agent comment, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedVerified against the actual source:
credential_config_for_remotereturnsOk(...)on every branch (mod.rs:172–200 — remote-URL error, non-HTTPS, no host, and each provider arm all yieldOk), so the newly added.await?at the tag/delete/fork call sites can never fail the op ahead of the push itself. I also confirmedclone_credential_config(and thusgh_authenticated) feeds both the fallback-protected network path (mod.rs:197/199 viacredential_config_for_remote) and the no-fallback clone path (mod.rs:661–662, gitlab.rs:4710/4845 →repo.rsextra_config).Summary
Sound, thoroughly-documented follow-up. It extends the reset+CLI-helper credential injection to tag push/delete, remote-branch delete, and fork PR pushes, and adds a one-shot ambient-credential fallback so severing git's helper chain can't hard-fail a user whose CLI token is stale but whose keychain credential still works. The classifier is unit-tested, retry placement is correct, and the
?additions are safe. No blockers.Resolved since last review
repository not foundarm (prior round-2 nit). Now present verbatim inis_auth_class_failure(remote.rs) — the comment states the ambient retry can complete under a different identity than the severed CLI one, notes push identity ≠ commit authorship, and warns against widening the classifier. Confirmed in the current diff.Correctness
forge/github.rs::gh_authenticated, theErr(_) => truecomment. The comment justifies the optimistic inject as "safe both ways: … withrun_git_mutating_with_creds's ambient fallback in place." That holds for the network path, butgh_authenticatedis also reached by the clone path (credential_config_for_remoteisn't the only caller ofclone_credential_config— mod.rs:662 and gitlab.rs:4710/4845 feed it straight intorepo.rsextra_config), which the PR itself documents as having no fallback. Concrete edge: gh installed but signed out for the host + a private clone that relies on the OS keychain + a transient spawn error ongh auth token→Err(_) => trueinjects the[reset, helper]pair, the reset severs the ambient chain, gh's helper answers nothing, and the clone hard-fails with no recovery — where pre-change (single additive helper, no reset) the keychain would have answered. It's genuinely narrow (needs a spawn error right afterresolve_namedalready proved gh exists) and self-heals on re-clone (the probe then returns a non-zero exit →false→ empty → ambient), so this is a small tradeoff rather than a bug. Suggested fix: scope the comment to note the fallback only covers the network path (the clone path takes the optimistic inject unprotected), so a future reader doesn't over-trust "safe both ways."Everything else checks out. The retry is safe for all six mutating ops that reach it (push branch/tag, tag delete, branch
--delete, forkpush -u) — HTTPS auth resolves at ref negotiation before any server-side update, so a failed-auth attempt mutated nothing and the un-injected re-run can't double-apply. The reset/=-terminator format is byte-identical to the historical helper entry (asserted by the newentries[1]tests), the classifier's negative tests keep merge conflicts and DNS failures out of the retry path, and the auth-cache tests use distinct per-host keys to avoid cross-test collisions on the process-wide static.Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
Round-3 triage
nit (the
Err(_) => truecomment overclaims "safe both ways" for the clone path): accepted, applied in the next push. The observation is correct —gh_authenticatedfeeds both the fallback-protected network path and the no-fallback clone path (repo.rsextra_config), so the optimistic inject is unprotected there. The comment now scopes the safety claim to the network path and documents the clone-path edge explicitly: it needs the probe spawn to fail immediately afterresolve_namedsucceeded, and it self-heals on re-clone (a clean probe returns non-zero →false→ no injection → ambient runs). Comment-only change; 737 tests + clippy re-verified.All prior rounds' items confirmed closed by this round — nothing else outstanding on our side.
Posted by GitDesktop — automated agent comment, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedThis change extends the reset+CLI-helper credential injection to tag push/delete, remote-branch delete, and fork PR pushes, and adds a one-shot ambient-credential fallback so severing git's helper chain can't hard-fail a user whose CLI token is stale but whose keychain credential still works. It's sound and no blockers — the classifier is unit-tested, retry placement is safe (HTTPS auth resolves at ref negotiation before any server-side update, so a failed-auth attempt mutated nothing), and the new
?additions are inert.Verified
credential_config_for_remote(mod.rs:171–201) returnsOk(...)on every branch (remote-URL error, non-HTTPS, no host, and each provider arm), so the newly added.await?at the tag/delete/fork call sites can't fail the op ahead of the push itself.run_git_mutatingimport removed frompr.rshas no remaining references in that file — the import removal is clean, not a dangling-symbol break.Resolved since last review
gh_authenticated'sErr(_) => truecomment overclaiming "safe both ways" for the clone path. The current diff scopes the safety claim to the network path and documents the clone-path residual explicitly (unprotected optimistic inject; needs a spawn failure right afterresolve_namedsucceeded; self-heals on re-clone). Confirmed in the changed comment.Other reviewers
-c credential.https://<host>.helperentries" and the conditional gh injection for authenticated unknown/GHE hosts, and the GitLab doc no longer claims a TTL cache. Nothing outstanding there.Everything else checks out. The
[reset, helper]pair is byte-identical to the historical single helper entry (asserted by the newentries[1]tests), the classifier's negative tests keep merge conflicts and DNS failures out of the retry path, and the auth-cache tests use distinct per-host keys to avoid collisions on the process-wide static.Posted by GitDesktop — AI output, verify before acting on it.
Ticket changed by: theBGuy