Warn about the keyring fallback on the first credential read, not only on login - #664
Conversation
…login The credential store printed its "system keyring unavailable, credentials stored in plaintext" warning only on Save. Every other command merely reads, so a process whose keyring probe failed served whatever an earlier fallback had left in credentials.json — on one machine today, tokens that expired months ago and profiles that no longer existed — with no word about why. The failing probe was invisible until the next login. Store.Load now runs the same once-per-process warning as Save, so the first read after a fallen-back probe says so on stderr, and once credstore is bumped past basecamp/cli#70 the warning and the Load error also name the probe failure itself. Hosts that mean to use file storage set BASECAMP_NO_KEYRING, which skips the probe and never warns. The wrapper's inner store is now the credStore interface rather than the concrete *credstore.Store, so the test can stand in a fallen-back store without failing a real keyring probe.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Surfaces credstore keyring-fallback warnings on the first credential read, preventing silent use of stale plaintext credentials.
Changes:
- Warns once on the first credential read or write.
- Adds an interface seam and regression test for fallback behavior.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
internal/auth/keyring.go |
Adds read-time fallback warnings and a testable store interface. |
internal/auth/keyring_test.go |
Verifies warnings occur once across reads and writes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
`basecamp people me` stores the fetched email through SetUserEmail, the one read path with no BASECAMP_TOKEN guard: IsAuthenticated and AuthorizationEndpoint short-circuit on the env token, then SetUserEmail loads the stored credentials anyway. That was wrong twice over. The email names the env token's user, not whoever the stored credentials belong to, so writing it there mislabeled them. And the load ran the keyring probe — now that a fallback read warns, a CI host with a locked keychain and only BASECAMP_TOKEN warned about plaintext credentials it neither stored nor read. BASECAMP_TOKEN wins, matching AccessToken() and AccountID(): SetUserEmail returns without touching the store.
|
Since 6208afd (one commit,
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Pulls in basecamp/cli#70: the keyring availability probe now writes a per-probe keychain item (service credstore.probe.<name>, account __probe__.<pid>.<n>), so concurrent CLI processes no longer share one probe entry and race each other's read-back. With the previous pin, 20 parallel invocations against the real macOS keychain returned 19 file-store fallbacks reading stale or missing credentials; with this pin all 20 read the keyring. FallbackWarning() now carries the probe failure reason, so the fallback warning added in #664 names its cause. vendorHash recomputed and the Nix build verified via make update-nix-hash.
Pulls in basecamp/cli#70: the keyring availability probe now writes a per-probe keychain item (service credstore.probe.<name>, account __probe__.<pid>.<n>), so concurrent CLI processes no longer share one probe entry and race each other's read-back. With the previous pin, 20 parallel invocations against the real macOS keychain returned 19 file-store fallbacks reading stale or missing credentials; with this pin all 20 read the keyring. FallbackWarning() now carries the probe failure reason, so the fallback warning added in #664 names its cause. vendorHash recomputed and the Nix build verified via make update-nix-hash.
The failure
The credential store printed its
warning: system keyring unavailable, credentials stored in plaintext at …only onSave. Every other command merely reads, so a process whose keyring probe failed served whatever an earlier fallback had left incredentials.json— on one machine today, tokens that expired months ago and profiles that no longer existed — with no word about why. Under the concurrent-probe race fixed in basecamp/cli#70, 19 of 20 parallelbasecamp auth status -jreturned that stale file silently, andbasecamp me --profile clawditosaid "Not authenticated" 19/20.The change
Store.Loadruns the same once-per-processwarnFallbackasSave, so the first read after a fallen-back probe says so on stderr. The inner store becomes a smallcredStoreinterface so the test can stand in a fallen-back store without failing a real keyring probe;swapNewCredStore's signature follows.Manager.SetUserEmail— the one credential read aBASECAMP_TOKENsession could reach (basecamp people mestores the fetched email) — now returns without touching the store when the env token is set, matchingAccessToken()/AccountID()precedence. The email names the env token's user, not whoever the stored credentials belong to, so writing it there mislabeled them; and the load ran the keyring probe, which with this PR would have warned a CI host with a locked keychain about plaintext credentials it neither stored nor read.TestSetUserEmailSkipsStoreOnEnvTokenfails with the guard removed.TestLoadWarnsOnceWhenKeyringFellBackasserts the warning appears on the first of two reads and that a later write stays quiet. Checked that it fails with only theLoadcall removed (an earlier draft passed for the wrong reason — theSavein the same capture window warned — which is why the test is split into a read phase and a write phase).Verified end to end against basecamp/cli#70 with a temporary local
replace(not committed): a bounded probe that fails yieldsThis PR works against the currently pinned credstore too — the warning just lacks the reason until the bump.
Noise
Hosts with no keyring at all (Linux without a secret service) will now see the warning on every invocation, not only at login. That is the honest state — their probe fails every run — and hosts that mean to use file storage set
BASECAMP_NO_KEYRING=1, which skips the probe and never warns. CI and agents onBASECAMP_TOKENnever read or write the store:AccessToken,IsAuthenticated,AccountID,AuthorizationEndpoint,auth status, anddoctorshort-circuit on the env token, andSetUserEmailnow does too, so the warning cannot fire there.Declined
FallbackWarning()carries the reason and its docstring asks callers to surface it on reads.Delete/MigrateToKeyringtoo. Both are preceded by a read or a login in every path that reaches them.SetUserIdentitythe sameBASECAMP_TOKENguard. Its callers are the login flows (auth login,profile add, the setup wizard), which have just saved fresh credentials and fetch the profile to label them; skipping the write there would leave a login done under an exported token unlabeled. Whether that fetch should use the fresh token rather than the env one is a separate, pre-existing question.GetUserEmail/GetOAuthTypetoo. Neither is reached from a token session —AuthorizationEndpointconsultsGetOAuthTypeonly after its env-token branch — and both are display-only reads whose stored answer some callers may want alongside an env token.people.goinstead of the setter. The precedence belongs whereAccessTokenandAccountIDalready keep it, so every caller gets it.Follow-up
Bump credstore once basecamp/cli#70 merges:
go get github.com/basecamp/cli@main && go mod tidy.