Surface keyring access failures from ActiveToken by Iron-Ham · Pull Request #13318 · cli/cli

Iron-Ham · 2026-04-29T21:59:48Z

Summary

gh api and gh api graphql were silently sending requests without an Authorization header when gh's keyring lookup failed — typically when the 3-second timeout in internal/keyring/keyring.go fired against a macOS Keychain entry whose ACL had drifted (e.g. an entry written by an older gh that now requires an interactive approval the non-TTY child process can't surface). Users saw only HTTP 401 (REST) or HTTP 403 "API rate limit exceeded for <IP>" (GraphQL) with no signal that the local keyring was the cause.

The bug was a silent error discard at two layers:

(*config.AuthConfig).ActiveToken swallowed keyring errors from TokenFromKeyringForUser / TokenFromKeyring and returned ("", source).
AddAuthTokenHeader.RoundTrip then saw an empty token and proceeded with no header — indistinguishable from a legitimate anonymous request.

What changed

Added ActiveTokenWithError(hostname) (token, source, err) to gh.AuthConfig and *config.AuthConfig that surfaces non-ErrNotFound keyring errors.
Refactored (*AuthConfig).ActiveToken as a thin wrapper that calls ActiveTokenWithError and discards the error, preserving its existing signature for every current caller.
Required ActiveTokenWithError on api/http_client.go's local tokenGetter interface; updated the cfg adapter in internal/authflow/flow.go and the tinyConfig test mock to match.
AddAuthTokenHeader.RoundTrip now calls ActiveTokenWithError. On non-nil error it fails the request with that error; on nil error + empty token it proceeds anonymously (preserves anonymous-endpoint behavior); on nil error + non-empty token it attaches the Authorization header as before.
keyring.ErrNotFound continues to be the legitimate "no token configured for this host" signal and is not surfaced as an error, so anonymous requests against unconfigured hosts keep working unchanged.

What stays the same

The gh.AuthConfig.ActiveToken signature and behavior for every existing caller.
Test mocks that don't directly satisfy tokenGetter (e.g. stubAuthConfig in pkg/cmd/attestation/trustedroot/trustedroot_test.go) — stubAuthConfig embeds config.AuthConfig so it inherits the new method automatically.
Anonymous-endpoint behavior (gh api zen against a host with no configured user still proceeds without an Authorization header).

Alternatives considered

Opt-in type-assertion shim (avoid changing gh.AuthConfig)

Initially this PR added an unexported errorTokenGetter interface in api/http_client.go that RoundTrip would type-assert against, with *config.AuthConfig opportunistically satisfying it. The advantage was zero churn to gh.AuthConfig — no interface change, no mock updates for downstream consumers.

Rejected because the new error-aware behavior would only fire by accident of static type propagation, the contract would be untestable from the interface seam (any future code constructing HTTPClientOptions from a gh.AuthConfig value rather than the concrete type would silently get the legacy path), and an opt-in shim is brittle for a fix to a silent-failure bug. Adding ActiveTokenWithError to the interface makes the contract explicit and verifiable.

Naming: ActiveTokenE vs. ActiveTokenWithError vs. ResolveActiveToken

ActiveTokenE (single-letter E suffix) is concise and used in some Go projects, but has no precedent in cli/cli — would be inventing a convention.

ResolveActiveToken discriminates well from ActiveToken, but pairs awkwardly with the existing method-on-noun naming throughout gh.AuthConfig (ActiveUser, TokenFromKeyring, etc.).

Chose ActiveTokenWithError because it composes naturally with ActiveToken (clear sibling relationship), is self-documenting in error-checking call sites, and matches the existing explicit-suffix precedent in this codebase (e.g. keyring.MockInitWithError in internal/keyring/keyring.go).

Logging from inside ActiveToken (surface error without signature change)

A smaller-footprint alternative: keep ActiveToken unchanged but write a warning to os.Stderr when both keyring lookups fail with non-ErrNotFound errors. The user would see a "warning: keyring access failed" line followed by the downstream 401/403, which is at least diagnosable.

Rejected because (1) logging from a config-layer getter is the wrong architectural seam — it doesn't compose with structured loggers or with cli/cli's --verbose plumbing, (2) the request still goes out unauthenticated, so scripts and CI consumers still get misleading exit codes, and (3) it leaves the underlying contract bug in place for every future caller. Returning the error is the right fix; logging would be a band-aid.

Marking ActiveToken as // Deprecated: in this PR

Could be added as a one-line annotation on the existing method to steer new code toward ActiveTokenWithError.

Deferred to a follow-up PR because adding the marker now would generate SA1019 warnings on every existing call site (pkg/cmd/auth/status/status.go:238, pkg/cmd/auth/refresh/refresh.go:178,229, pkg/cmd/auth/token/token.go:81, pkg/cmd/auth/shared/writeable.go:10, pkg/cmd/config/get/get.go:58, plus internal usages) without giving them a migration path in the same PR. Better to land this fix first, then a follow-up that migrates the call sites and adds the marker together. Happy to do both in one PR if maintainers prefer.

Follow-ups (not in this PR)

Other call sites of ActiveToken would benefit from migrating to ActiveTokenWithError so the same class of silent failure can't recur in those code paths. Left for a follow-up to keep this PR focused:

pkg/cmd/auth/status/status.go:238 — gh auth status can mis-report keyring failures as "no token"
pkg/cmd/auth/refresh/refresh.go:178,229
pkg/cmd/auth/token/token.go:81
pkg/cmd/auth/shared/writeable.go:10
pkg/cmd/config/get/get.go:58

Adding // Deprecated: to ActiveToken is also deferred to that follow-up — adding it now would generate SA1019 warnings on every existing call site without giving users a migration path.

Test plan

go vet ./... clean.
go test ./... -short passes (full suite).
New tests in internal/config/auth_config_test.go:
- TestActiveTokenWithErrorSurfacesKeyringFailure — keyring error is wrapped and returned.
- TestActiveTokenSilentlyDiscardsKeyringFailure — ActiveToken (the wrapper) preserves its no-error contract.
- TestActiveTokenWithErrorDoesNotSurfaceErrNotFound — ErrNotFound returns (token="", err=nil).
- TestActiveTokenWithErrorReturnsEnvTokenWithoutKeyring — env-var path short-circuits keyring entirely.
- TestActiveTokenWithErrorReturnsKeyringToken — successful keyring resolution returns (token, "keyring", nil).
New table-driven TestAddAuthTokenHeaderResolutionPaths in api/http_client_test.go covering: error fails the request, resolved token is attached, empty token without error proceeds anonymously.
Manually verified the patched binary built from this branch continues to authenticate correctly against a healthy macOS Keychain (gh api user returns HTTP/2 200, x-ratelimit-limit: 5000, resource: core; same for GraphQL).

Iron-Ham · 2026-05-26T21:32:09Z

cc: @babakks kindly asking for a review as y'all are gearing up for 2.93.0

this is a change that i genuinely need in order to use the gh cli, and i don't want to have to maintain a long-term fork of gh since… well, i live entirely in the cli and primarily use it over the web interface these days.

babakks · 2026-05-26T22:31:29Z

Hey @Iron-Ham! 👋 Sorry for the delay on this one. This has been on my list but had to punt it a bit due to other priorities. I'll try to have a look this week and will let you know. Need to deep dive for which your thorough investigation is going to be super helpful.

Iron-Ham · 2026-05-26T22:32:21Z

Totally understandable, just wanted to make sure it hadn't fallen off the list.

Hope you and the team have been well!

babakks · 2026-05-26T22:32:45Z

By the way, I don't think it'll get into the next release.

williammartin · 2026-08-04T16:54:09Z

@Iron-Ham, @LaurMo said you are having to work off a long lived branch due to this? The impression I got first time I glanced at this was that it was a one off/rare occurrence that was suppressing the error and causing an issue an you had to address. It seemed like this PR didn't actually address the issue, but did make you more aware of the step you needed to take to resolve it. Am I missing something? Would help me prioritise this higher if I understood there was more friction. Thanks.

williammartin · 2026-08-05T11:06:13Z

I left a longer comment here: #13317 (comment)

When ActiveToken's keyring lookup fails with a non-ErrNotFound error (typically *keyring.TimeoutError on macOS Keychain), the error was silently discarded and AddAuthTokenHeader.RoundTrip then sent the request without an Authorization header. Users saw a confusing HTTP 401 (REST) or HTTP 403 "API rate limit exceeded for <IP>" (GraphQL) with no signal that the local keyring was the cause. Add ActiveTokenWithError to gh.AuthConfig that surfaces these errors. Refactor ActiveToken as a thin wrapper that calls ActiveTokenWithError and discards the error so existing callers keep their signature. Require the new method on api/http_client.go's tokenGetter interface and update RoundTrip to fail the request with the resolution error rather than send unauthenticated. When the user-scoped keyring lookup fails with a real error and the legacy unkeyed fallback then returns ErrNotFound, preserve the original error rather than letting the fallback's "not found" mask it. Otherwise a timeout or permission failure on the canonical lookup would be reported as if no token were configured. ErrNotFound continues to be the legitimate "no token configured for this host" signal and is not surfaced as an error so genuinely anonymous requests against unconfigured hosts keep working. Fixes cli#13317

…omment (#46) review-craft ruled PR descriptions out of scope on two reasons. The first was sound: a description is a durable record and must not inherit the 150-word review budget. The second was false — it said a description "cannot collapse anything". GitHub renders <details><summary> in PR bodies and in every kind of comment; the probe is dated in house-style.md (cli/cli#13318, whose body_html returns real <details> elements). The false reason did the damage. It ruled out the one mechanism that reconciles complete with short, so "never budget a durable record" stood alone and was read as "length does not matter". By 18 Sep 2026 that had produced eleven agent-authored PRs on Trilogy-Care/tc-portal carrying 9,000-39,000 character descriptions with zero folds — 307,000 characters against a budget of 41,000, in a repo that squash-merges with PR_BODY as the commit message. The worst, #9309, was 17,638 characters for 50 changed lines, one of them application code. A human reviewer flagged it; nothing in the skill would have. What replaces it is not a shorter description. It is a destination: - The description is the record of the decision — why, what changed in shape, what a reviewer should look at, risks, references — budgeted at min(4000, max(800, 12 x changed lines)) RAW characters of the body as stored, excluding the Claude Code attribution footer. Raw, because the squash copies the body verbatim and because that is the unit all three constants were measured in. - The working — the evidence census, the queries and their output, the method, the alternatives rejected — moves whole and verbatim into a PR comment headed "## Working notes", which the description links to in one line. Identified by that heading and never by position: the working moves out late, so it is the newest comment. Nothing is deleted; it moves. The constants are measured, not chosen: slope 12 from the 10.6-character median across 45 recent human-authored tc-portal PRs, ceiling 4,000 from the p89 of 5,557 PR bodies, floor 800 from just above the p25. The file says plainly where the ceiling is tighter than current practice rather than hiding it. issue-craft carried the same false claim inside the paraphrase scar. The scar and its rule stand — budget artefacts consumed in a feed, never budget a durable record — with a dated correction: the scar's real lesson was that the detail had nowhere to go, and a budget with a destination is a move while a budget without one is a paraphrase machine. Adds skills/review-craft/pr_lint.py beside review_lint.py: takes --repo/--pr via gh or a local draft with an explicit --changed-lines (it will not guess a diff size), and checks the body against the budget for that diff, a long body with no headings, unfilled template placeholders, rotting relative-date words, oversized <details> a squash will copy raw, and that a comment headed "## Working notes" exists when the body links to one. Mechanics only; exit 0 pass, 1 errors, 2 could not run. scripts/test-pr-lint.sh covers it in 43 cases. SKILL.md, dispatcher workers implement.md and review.md, and agents/engineer.md now carry the rule and require pr_lint to pass before a PR is called ready. Stack children inherit it through implement.md. Co-authored-by: Claude Opus 5 (1M context) <[email protected]>

github-actions Bot added external pull request originating outside of the CLI core team needs-triage needs to be reviewed labels Apr 29, 2026

Iron-Ham marked this pull request as ready for review April 29, 2026 22:01

Iron-Ham requested a review from a team as a code owner April 29, 2026 22:01

Iron-Ham requested a review from babakks April 29, 2026 22:01

Iron-Ham force-pushed the Iron-Ham/fix-silent-keyring-token-failures branch 2 times, most recently from 9d71383 to 5213965 Compare April 30, 2026 20:50

niik added ready-for-review and removed needs-triage needs to be reviewed labels May 29, 2026

zxvchaos mentioned this pull request Jun 10, 2026

Desktop: false "GitHub CLI authentication expired" toast — any gh auth status failure (incl. its 5s timeout) is classified as expired credentials, frequent during multi-agent workloads anthropics/claude-code#67055

Closed

2 tasks done

coderabbitai Bot mentioned this pull request Jun 30, 2026

ci: restore release label preflight GainForest/hyperindex#114

Merged

coderabbitai Bot mentioned this pull request Jul 10, 2026

fix: fall back to gh keyring auth, qualify skill references and correct validators spencerbeggs/design-docs-plugin#56

Merged

coderabbitai Bot mentioned this pull request Jul 26, 2026

fix: zsh起動時のキーリングパスフレーズ要求を回避するためGITHUB_TOKENの非同期更新処理を最適化 yohi/dotfiles-zsh#13

Merged

Iron-Ham force-pushed the Iron-Ham/fix-silent-keyring-token-failures branch from 5213965 to 7e8a674 Compare August 5, 2026 12:52

saltedzucchini mentioned this pull request Aug 27, 2026

Fail closed on Automic Vault credential resolution errors automic-vault/gh-cli#3

Closed

2 of 6 tasks

patrickdorival mentioned this pull request Sep 19, 2026

feat(review-craft): the description is the record, the working is a comment devergehq/locus#46

Merged

Iron-Ham closed this Oct 9, 2026

Navigation Menu

Repository navigation

Surface keyring access failures from ActiveToken - #13318

Surface keyring access failures from ActiveToken#13318
Iron-Ham wants to merge 1 commit into
cli:trunkfrom
Iron-Ham:Iron-Ham/fix-silent-keyring-token-failures

Iron-Ham commented Apr 29, 2026 •

edited

Loading

Uh oh!

Iron-Ham commented May 26, 2026

Uh oh!

babakks commented May 26, 2026

Uh oh!

Iron-Ham commented May 26, 2026 •

edited

Loading

Uh oh!

babakks commented May 26, 2026

Uh oh!

williammartin commented Aug 4, 2026

Uh oh!

williammartin commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Iron-Ham commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changed

What stays the same

Alternatives considered

Follow-ups (not in this PR)

Test plan

Uh oh!

Iron-Ham commented May 26, 2026

Uh oh!

babakks commented May 26, 2026

Uh oh!

Iron-Ham commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babakks commented May 26, 2026

Uh oh!

williammartin commented Aug 4, 2026

Uh oh!

williammartin commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Iron-Ham commented Apr 29, 2026 •

edited

Loading

Iron-Ham commented May 26, 2026 •

edited

Loading