Repository navigation
auth: hand Git fresh credentials [5/10] - #22940
Merged
Merged
Conversation
This was referenced Sep 28, 2026
sergiou87
added this pull request to stack #22944
September 28, 2026 09:00
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The security-sensitive debug-log redaction lacks regression coverage.
Review effort: Balanced
Findings: 1
What changed in this PR
Routes Git and Git LFS authentication through fresh AccountsStore tokens while protecting refreshable credentials.
Changes:
- Renews credentials before returning them to Git.
- Uses ephemeral Basic credentials when supported, with legacy fallback.
- Redacts additional secrets from debug logs and adds credential tests.
| File | Description |
|---|---|
app/src/lib/trampoline/trampoline-credential-helper.ts |
Adds fresh, ephemeral credentials and log redaction. |
app/test/unit/refresh-token-consumers-test.ts |
Tests renewal and credential formats. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
sergiou87
changed the title
sergiou87
marked this pull request as ready for review
September 28, 2026 15:13
sergiou87
force-pushed
the
short-lived-tokens-5-git
branch
from
October 2, 2026 11:28
55d5555 to
9d5009a
Compare
sergiou87
force-pushed
the
short-lived-tokens-5-git
branch
from
October 2, 2026 11:42
9d5009a to
1660bf1
Compare
sergiou87
changed the title
sergiou87
force-pushed
the
short-lived-tokens-5-git
branch
from
October 2, 2026 12:18
1660bf1 to
e377ca7
Compare
niik
requested changes
Oct 2, 2026
niik
left a comment
niik
left a comment
Member
There was a problem hiding this comment.
The final delta is nicely minimal and correctly asks AccountsStore for a fresh credential before returning it to Git. I see two integration issues that I think need resolution before this layer lands.
First, Git caches credentials returned by a helper and may reuse them for later HTTP requests without invoking the helper again. Refreshing for a second Git/LFS operation immediately invalidates the old access token. A command that received a token just outside the ten-minute margin can therefore be broken when a concurrent operation or pre-push LFS work crosses the margin and rotates it. The original command’s next authenticated request uses its cached token, receives a 401, and Git does not necessarily reacquire credentials; a subprocess reproduction exited 128 in this situation. This does not require an eight-hour operation, only overlap near the renewal boundary. The direct one-shot helper test cannot expose it.
I think Git credentials need coordination with active consumers rather than independent per-helper freshness checks—likely a lease/pin associated with the trampoline environment or Git process lifecycle. Renew before issuing a near-expiry credential, then avoid rotating that credential while active Git consumers still hold it. Please add a subprocess-level regression covering discovery with one token, rotation by another consumer before the next authenticated request, and the resulting push/LFS behavior.
Second, when renewal retires the account or otherwise requires sign-in, the exception reaches the trampoline handler’s outer catch. That returns no credential but bypasses the existing no-account path: no interactive GitHub sign-in prompt, no setHasRejectedCredentialsForEndpoint marker, and no translation to the recognized HTTPSAuthenticationFailed error for background tasks. The user instead gets a cryptic terminal-prompts-disabled Git error. Please handle the retired-account case in or below getGitHubCredential so it re-enters the existing sign-in/background-authentication behavior.
It would also be valuable to cover plain tokens, sufficiently valid refreshable tokens, renewal rejection, and transient renewal failure in the integration tests.
Git and Git LFS get GitHub credentials from Desktop's credential helper trampoline. With short-lived tokens, the token stored on the Account can have expired, and Git has no way to renew it. Have the trampoline ask AccountsStore for a fresh token first. For every GitHub account the helper now calls `getAccountWithFreshToken`, which renews the access token if it expires within ten minutes. Plain (non-refreshable) tokens are returned unchanged, so Git behaves as before for existing accounts. For refreshable accounts, if Git says it supports the `authtype` capability (`capability[]=authtype`), the helper replies with a pre-encoded `Authorization: Basic` credential and `ephemeral=1` instead of a username and password. Git then uses the token for this request only and doesn't pass it to other credential helpers to store, so an expiring token never ends up saved in the system keychain or a credential file. Git versions and Git LFS without the capability still get the username and password form. The helper's debug logging now redacts `credential=` and `oauth_refresh_token=` lines as well as passwords. When no GitHub account matches the endpoint, the helper returns undefined explicitly instead of passing an undefined account to `credWithAccount`, which returned undefined anyway. Tests cover renewal before replying in both the `authtype` and the username/password forms, without leaking the old access token or any refresh token. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
In development builds the credential helper trampoline logs the raw stdin Git sends it. This branch added a second `replaceAll` that replaces `credential=` (the base64 Basic credential sent with the `authtype` capability) and `oauth_refresh_token=` lines with `=[redacted]`, next to the existing `password=***` masking. Nothing covered it, so dropping or breaking that line would silently start writing live credentials to the development log. The new test "redacts secrets from development debug logs" in `refresh-token-consumers-test.ts`: - Sets `__DEV__` to true for the duration of the test (tests run with it false, see `app/test/globals.mts`) and restores it in `t.after`. - Captures `log.debug` with `t.mock.method`. - Sends an `erase` command for `github.com` whose stdin contains `password=`, `credential=` and `oauth_refresh_token=` lines, each with a distinct `secret-` value. `erase` for a GitHub host returns before touching any credential store, so the call has no side effects. - Asserts the log still contains the non-secret `host=github.com` line (so the test would notice if logging stopped altogether), shows `credential=[redacted]` and `oauth_refresh_token=[redacted]`, and contains none of the three secret values. Verified that the test fails when the new redaction line is removed and passes with it. No source changes. Addresses review feedback on #22940. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Simplify the Git credential helper change to the one thing short-lived tokens need: ask AccountsStore for a fresh token before replying. `getGitHubCredential` now returns `credWithAccount(cred, await store.getAccountWithFreshToken(account))` for a matching GitHub account and is otherwise unchanged from `development`. Removed: - The `capability[]=authtype` branch that, for refreshable accounts, replied with `authtype=Basic`, a base64 `credential=` and `ephemeral=1` instead of a username and password. It produced the same `Authorization: Basic` header Git builds from a username and password. `ephemeral=1` only stops Git from handing the credential to other helpers' `store`, but Desktop already runs Git with `credential.helper=` followed by `credential.helper=desktop`, which clears every system, global and repository helper, and Desktop's own `store`/`erase` return early for GitHub endpoints. So nothing persisted the token before, and the extra response format was defense against a configuration Desktop does not use. - The extra debug-log redaction of `credential=` and `oauth_refresh_token=` lines, which only existed for that format. Password masking is unchanged. - The test "redacts secrets from development debug logs", which covered that redaction. - The explicit `return undefined` for the no-account case; the original `return credWithAccount(cred, account)` already returns undefined when there is no account. The Git test is now a single case, "refreshes before returning credentials": Git asks for github.com credentials with an expiring refreshable account, and the helper renews once and replies with the account login and the renewed token as username and password, without the old access token or any refresh token. It fails against the previous helper, which returned the stored token without renewing. Git and Git LFS both keep receiving username and password, exactly as on `development`; only the token is now guaranteed fresh. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review feedback on #22940 found two problems with handing refreshable credentials to Git. 1. Renewal broke Git processes that already held the previous token. Git asks the credential helper once per process and reuses the credential for every later HTTP request that process makes; Git LFS, which Git spawns, does the same. GitHub revokes the previous access token as soon as a refresh token is used ("Once you use a refresh token, that refresh token and the old user access token will no longer work"). So when any other consumer renewed the token while a Git process was running (an API request once the token entered the ten-minute renewal window, a background fetch, another Git operation, Copilot), that process's next authenticated request got a 401 and Git failed with "RPC failed; HTTP 401". This did not need a long operation, only overlap with the moment of renewal. Tokens can now be leased by consumers that keep using them: - CredentialSessions.leaseToken(account, holder) returns an access token and a release function. Leases are recorded per access token and holder on the session. - Whenever anyone needs a token renewed (API clients, Git, Copilot, images; through getFreshToken, resolveToken or leaseToken) and that token is leased, they wait until every holder releases it, then renew it as usual. The wait also ends when the session is retired (the caller then gets AccountRequiresSignInError) or when the token comes within one minute of expiring (leasedRenewalMargin), at which point it stops working for its holders anyway and is renewed regardless. - A holder that already leases the token gets the same token again instead of waiting or renewing. Git LFS asks the credential helper under its parent Git process's trampoline token; waiting would deadlock on the parent, and renewing would revoke the token the parent is still using. - leaseToken only takes the lease when no renewal is in flight and the token is still the session's current one. If a renewal started while the token was being handed out, it waits for it and leases the new token instead, so it never hands out a token that is about to be revoked. Conversely, the check for leases and the start of a renewal happen in the same synchronous step, so a lease cannot be taken on a token whose renewal has begun; if a new lease is taken while a caller is waking up, the caller waits again. - AccountsStore.leaseAccountToken wraps it with the same account checks as getAccountWithFreshToken. - The credential helper leases every GitHub credential it returns, using the trampoline token as the holder, and registers the release with holdCredentialLease. withTrampolineEnv already brackets the whole Git process (and the LFS processes it spawns), so it now releases those leases in its finally block, next to the rest of the per-token cleanup. A lease registered for a trampoline token that is no longer valid is released immediately. The trade-off is that during the last ten minutes of a token's lifetime, work that needs it renewed can wait behind a long-running Git operation, at most until a minute before the token expires. 2. A signed-out account bypassed the existing sign-in path. When renewal was rejected, the account was signed out and the helper's getAccountWithFreshToken call threw. The error reached the handler's outer catch, which returned no credential without prompting for sign-in, without recording rejected credentials for the endpoint, and therefore without translating Git's "terminal prompts disabled" failure into the usual authentication error for background tasks. getGitHubCredential now treats AccountRequiresSignInError as "no account for this host", so getCredential continues down the same path as when no account exists: it prompts to sign in for foreground operations, or returns nothing for background tasks and lets the handler mark the endpoint as rejected. leaseToken reports rejected renewal as AccountRequiresSignInError once the session has been retired. If renewal fails transiently (network or server errors) and the account is still signed in, leaseToken now falls back to the current access token while it has not expired, so a short outage does not fail Git operations that could still authenticate. Other consumers keep their existing behavior. Tests: - app/test/unit/git/refreshable-credentials-test.ts runs a real git push through Desktop's credential helper binary against a smart HTTP server backed by git http-backend, which only accepts the current token and revokes old ones on renewal, as GitHub does. Between Git's first authenticated request and the next one, the token moves inside the renewal window and an API consumer asks for a fresh token. The push must succeed, and the consumer must receive the renewed token only after Git exits. Without the wait the push fails with "RPC failed; HTTP 401". The server runs as an HTTP proxy so the remote can use a host without a port, which is how Desktop matches accounts to remotes. The remote is read with --git-dir so the test also works with safe.bareRepository=explicit. Git LFS is not covered. - credential-sessions-test: renewal waits for every holder and releasing twice is harmless; a holder gets the token it already holds without renewal; a leased token is renewed when about to expire, including by a waiting caller once that point is reached; sign-out ends the wait; a renewal that starts while a token is being leased results in leasing the renewed token; transient failure falls back to an unexpired token but not an expired one; rejected renewal requires sign-in. - refresh-token-consumers-test: plain tokens are returned unchanged, sufficiently valid refreshable tokens are returned without renewing, transient renewal failure returns the current token and keeps the account, and rejected renewal signs out, prompts for GitHub sign-in and marks the endpoint as rejected. Each new test fails without the corresponding change; without the holder exception, the test for a holder asking again hangs waiting for itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sergiou87
force-pushed
the
short-lived-tokens-5-git
branch
from
October 5, 2026 11:35
f16a017 to
3125643
Compare
The refreshable credential push test failed on Windows with "Cannot access URL ... return code 22" followed by "fatal: git-http-push failed". This was a test-server failure, not a credential lease failure. Desktop's Windows Git distribution comes from MinGit. The exact bundled Windows x64 archive in dugite 3.2.3 contains receive-pack but does not ship git-http-backend.exe. The test attempted to launch http-backend anyway, ignored its nonzero exit status and stderr, and translated its empty stdout into an HTTP 200 with no smart-HTTP advertisement. Git then fell back to the unsupported dumb-HTTP push protocol and failed. Serve the two requests needed by this regression directly with the bundled Git receive-pack plumbing instead of relying on an unavailable CGI program: - Discovery uses receive-pack --stateless-rpc --advertise-refs and wraps its output with the smart-HTTP service announcement and advertisement MIME type. - The push POST pipes its request into receive-pack --stateless-rpc and returns the resulting packet stream with the RPC result MIME type. - Both invocations use an explicit operand boundary and the absolute bare repository path. Unknown routes return HTTP 404. - Capture stderr, check the subprocess exit status, and return HTTP 500 on backend failure instead of a success-shaped empty response. A launch error closes the HTTP response before propagating the original error. The test still runs a real Git push through Desktop's credential helper, authenticates each HTTP request against the current token, verifies the remote ref, and verifies that the other credential consumer waits until Git exits before renewing. No production code, lease semantics, dependencies, platform skips, or workflow configuration change. Validation: - Confirmed the missing backend in the bundled Windows x64 release archive. - Reproduced the exact Windows failure on macOS by pointing the old backend invocation's GIT_EXEC_PATH at a directory without external Git helpers. - The replacement server passes with that same restriction, using built-in Git commands only. The restriction was temporary and is not committed. - Temporarily disabling the lease wait still makes the real push fail with "RPC failed; HTTP 401", preserving the original regression coverage. - yarn test app/test/unit/git/refreshable-credentials-test.ts app/test/unit/credential-sessions-test.ts app/test/unit/refresh-token-consumers-test.ts: 21 passed. - yarn -s lint:fix: passed. - npx tsc -p . --noEmit --skipLibCheck: passed. The unfiltered check reports four existing WeakMap declaration conflicts in dependency type files. Windows execution requires the next CI run; it cannot be run on this macOS host. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a comment to parseOAuthToken explaining why a refresh token can be accepted without expiry metadata. GitHub documents expires_in and refresh_token_expires_in for refreshable OAuth credentials, so this parser behavior is defensive tolerance for an unexpected response, not recognition of a documented GitHub response shape. This clarifies the distinction raised in review 5413998626 on #22940: accepting optional metadata in our parser does not establish that GitHub legitimately omits it from refreshable-token responses. The existing comment still explains that absent lifetimes remain unknown rather than being invented, while zero means the token is already expired. No parsing rules, credential fallback behavior, public types, or tests change. Validation: - yarn -s lint:fix: passed. - git diff --check: passed. - Unit tests not rerun because the change is documentation-only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
Thanks @niik! Both were hopefully addressed in f16a017. 1. Renewal revoking a token a running Git process still uses. Tokens can now be leased. The credential helper leases every GitHub credential it hands out, keyed by the trampoline token. The lease is released in 2. Tests:
LFS isn't covered end to end, since standing up an LFS server in the tests felt too heavy. It runs under its parent Git process's trampoline token, so it shares that lease. |
niik
approved these changes
Oct 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Git and Git LFS get GitHub credentials from Desktop's credential helper trampoline. The helper now asks AccountsStore for a fresh token before replying, so Git never receives an expired short-lived token. Plain tokens come back unchanged, and Git still gets the usual username/password form.
Stack
Merge in order, starting with 1. Each PR builds, passes tests, and keeps production behavior on its own.
Validation
Release notes
Notes: no-notes