Repository navigation
Conversation
Refresh an expired or near-expiry refreshable token before printing it, and add --no-refresh to opt out. Fail with a re-authentication notice when the refresh token is rejected. Under --secure-storage a refreshable token is returned regardless of where it is stored, since gh owns its lifecycle and a go-gh based caller cannot obtain it otherwise; the strict keyring-only behavior is preserved for non-refreshable tokens. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2dc3f23c-61a6-43a7-85cd-90aa872a7fc6
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The secure-storage paths regress legacy host-level keyring lookup when an active user is recorded.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Updates gh auth token to refresh short-lived credentials while preserving secure-storage compatibility.
Changes:
- Adds
--no-refreshand automatic token refresh. - Implements four secure-storage/refresh combinations.
- Expands unit coverage for token sources and refresh outcomes.
| File | Description |
|---|---|
pkg/cmd/auth/token/token.go |
Adds refresh and secure-storage selection logic. |
pkg/cmd/auth/token/token_test.go |
Tests flags, storage modes, and refresh outcomes. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| val = cred.Token | ||
| } else { | ||
| cred, _ := authCfg.TokenForUser(hostname, opts.Username) | ||
| val = getNonRefreshableTokenFromKeyring(authCfg, hostname, user) |
There was a problem hiding this comment.
I don't think this is really an issue. That is incredibly old code.
There was a problem hiding this comment.
It does highlight that the unkeyed-slot fallback in ActiveToken is dead weight now though. We should clean that legacy path up in a follow-up, at which point this and ActiveToken will agree again.
| } else { | ||
| if opts.Username == "" { | ||
| val = authCfg.ActiveToken(hostname).Token | ||
| if cred.IsRefreshable() || refreshStatus == gh.RefreshStatusDone { |
There was a problem hiding this comment.
Question
Under what circumstances would cred.IsRefreshable be false but refreshStatus == gh.RefreshStatusDone be true?
| val = cred.Token | ||
| } else { | ||
| cred, _ := authCfg.TokenForUser(hostname, opts.Username) | ||
| val = getNonRefreshableTokenFromKeyring(authCfg, hostname, user) |
There was a problem hiding this comment.
It does highlight that the unkeyed-slot fallback in ActiveToken is dead weight now though. We should clean that legacy path up in a follow-up, at which point this and ActiveToken will agree again.
williammartin
left a comment
williammartin
left a comment
There was a problem hiding this comment.
I'm still pretty unsure about --secure-storage refreshing tokens stored insecurely. I think there's a reasonable case to be made for the user opting into --short-lived, requiring extension authors to upgrade. Pretty torn on the right option here.
Part of #14449. Based on #14452 (login, refresh, git credential).
Description
Makes
gh auth tokenrefresh a short-lived token before printing it, and handles the hidden--secure-storageflag correctly for refreshable credentials.gh auth tokenis not only a user-facing command; it is also how go-gh (and therefore every gh extension) fetches a stored token. go-gh invokes it with a hidden--secure-storageflag, whose historical meaning is "look only in the OS keyring, ignore environment variables and the plain config file." That contract made sense when the only tokens gh stored were non-expiring: go-gh could read a config-file token itself, so--secure-storagewas just asking for the keyring copy specifically.Refreshable tokens break that assumption in two ways: they are stored under a separate config key (and a separate keyring service) that go-gh does not know about, so a go-gh caller cannot obtain a refreshable token by any means other than shelling out to
gh auth token; and gh, not the caller, owns their lifecycle (expiry, rotation, re-approval), so handing a caller a refreshable token means gh must first make it valid.How did you test this change?
This feature is verified end to end as a whole rather than per PR. End-to-end tests should pass.
Key points
Because the command now varies along two independent axes, whether to refresh (
--no-refresh) and whether to restrict to secure storage (--secure-storage), the run function is written as an explicit four-case switch rather than a cascade of conditionals. The four cases are:--no-refreshonly: return the stored active token as is, still surfacing a refreshable token's current access token, but never contacting the token endpoint.--secure-storageonly (the go-gh compatibility path): if the resolved credential is refreshable or was just refreshed, return it regardless of where it is stored; otherwise fall back to the original strict behavior and read a non-refreshable token straight from the keyring.--secure-storage --no-refresh: a rare combination with no known caller; honor--secure-storagestrictly and return the token only when it actually came from the keyring.Why case 3 diverges from the historical
--secure-storagesemanticsThe tempting simplification is to treat
--secure-storageas an absolute "keyring only" filter in every case. That would be wrong for refreshable tokens: a refreshable token stored in the plain config file would then be invisible to go-gh even though go-gh has no other way to reach it, so every extension would break for users on short-lived credentials whose token happens to live in the config file. So case 3 deliberately ignores the keyring-only constraint when the token to return is refreshable (or was just refreshed): gh owns the lifecycle, the caller cannot get the token any other way, and dropping a freshly minted token merely because of where it is stored would be a silent failure.We keep the divergence as narrow as possible. When the credential is not refreshable, case 3 falls straight back to the original strict keyring read, so non-refreshable callers see exactly the old behavior. Case 4 (
--secure-storage --no-refresh) is stricter still: with refresh declined there is no lifecycle to honor, so we respect--secure-storageliterally and return only a genuinely keyring-sourced token.We are intentionally not proposing to remove or rename
--secure-storagehere. The flag is part of the go-gh contract and other callers may rely on it; the point is only to record why the handling is more intricate than a single boolean filter and why case 3 knowingly departs from the literal meaning of the flag for refreshable tokens.Notes for reviewers
The four-case switch in the run function is the heart of the change; read it against the
--secure-storagecontract described above, and check that the non-refreshable fallbacks in cases 3 and 4 reproduce the exact prior behavior.Commit:
fix(auth token): refresh short-lived tokens before printingAuthorship and follow-up
Who wrote this:
Who answers review comments: