fix(oauth): store keyring tokens as one entry per provider#3
Closed
euxaristia wants to merge 1 commit into
Closed
Conversation
The keyring backend combined every provider and MCP token into a single JSON blob under one keyring entry. On macOS, add-generic-password now writes through security -i, whose command parser caps a single write at 4095 bytes (Gitlawb#574). The combined blob has no such bound: three or more logged-in providers routinely exceeds it, so Set() starts failing for every provider, not just the one that pushed it over. Split storage into one keyring entry per token key, plus a small index entry listing which keys exist (KeyringClient has no list operation). Each write is now bounded to a single token's size, well under the line cap regardless of how many providers are logged in. Installs on the old combined-entry format keep reading correctly via a legacy fallback, and get migrated to per-key entries on the next save.
📝 WalkthroughWalkthroughThe OAuth keyring backend now stores each token in a separate entry, maintains an encoded index, removes deleted entries, and migrates legacy combined storage. Tests cover round trips, size limits, deletion, and migration. ChangesKeyring storage migration
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
Owner
Author
|
Opened against upstream instead: Gitlawb#668. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/oauth/store.go`:
- Around line 465-504: Update internal/oauth/store.go lines 465-504 in
keyringBlob.read to skip corrupted index or per-token base64/JSON decode
failures like missing entries, allowing valid tokens to load; update lines
521-560 in keyringBlob.write to treat indexedKeys decode failures as no known
prior keys and continue Save/Delete instead of returning an error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 77b954a3-b167-42c9-8702-54387ad5b31f
📒 Files selected for processing (2)
internal/oauth/store.gointernal/oauth/store_keyring_test.go
Comment on lines
465
to
+504
| func (b keyringBlob) read() ([]byte, bool, error) { | ||
| enc, ok, err := b.kr.Get(b.service, b.account) | ||
| indexEnc, ok, err := b.kr.Get(b.service, b.indexAccount) | ||
| if err != nil { | ||
| return nil, false, err | ||
| } | ||
| if !ok { | ||
| return b.readLegacy() | ||
| } | ||
| keys, err := decodeKeyIndex(indexEnc) | ||
| if err != nil { | ||
| return nil, false, fmt.Errorf("oauth: decode keyring token index: %w", err) | ||
| } | ||
| tokens := make(map[string]Token, len(keys)) | ||
| for _, key := range keys { | ||
| enc, ok, err := b.kr.Get(b.service, key) | ||
| if err != nil { | ||
| return nil, false, err | ||
| } | ||
| if !ok { | ||
| // Index and entries fell out of sync (e.g. a killed process between | ||
| // writing an entry and updating the index); skip rather than fail the | ||
| // whole read, since the next Save/Delete will reconcile the index. | ||
| continue | ||
| } | ||
| raw, err := base64.StdEncoding.DecodeString(strings.TrimSpace(enc)) | ||
| if err != nil { | ||
| return nil, false, fmt.Errorf("oauth: decode keyring token entry %q: %w", key, err) | ||
| } | ||
| var token Token | ||
| if err := json.Unmarshal(raw, &token); err != nil { | ||
| return nil, false, fmt.Errorf("oauth: invalid keyring token entry %q: %w", key, err) | ||
| } | ||
| tokens[key] = token | ||
| } | ||
| data, err := json.Marshal(storeFile{SchemaVersion: storeSchemaVersion, Tokens: tokens}) | ||
| if err != nil { | ||
| return nil, false, err | ||
| } | ||
| return data, true, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Decode/unmarshal errors for keyring index or per-token data abort the whole operation, instead of self-healing like "not found" already does. Both keyringBlob.read() and keyringBlob.write() (via indexedKeys()) treat a corrupted index or per-token entry as a fatal error for the entire call, even though the migration's whole purpose is to stop one bad entry from affecting every other provider/MCP token.
internal/oauth/store.go#L465-L504: inread(), skip (rather than error out on) a base64/JSON decode failure for the index (lines 473-476) or for an individual per-token entry (lines 489-496), the same way the!ok/"not found" branch already does, so one corrupted provider doesn't blockLoad()for the rest.internal/oauth/store.go#L521-L560: inwrite(), treat a decode failure fromindexedKeys()(line 526-529) as "no known prior keys" instead of aborting, so a corrupted index doesn't block every futureSave/Deletefor every provider/MCP token until it's cleared out-of-band.
📍 Affects 1 file
internal/oauth/store.go#L465-L504(this comment)internal/oauth/store.go#L521-L560
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/oauth/store.go` around lines 465 - 504, Update
internal/oauth/store.go lines 465-504 in keyringBlob.read to skip corrupted
index or per-token base64/JSON decode failures like missing entries, allowing
valid tokens to load; update lines 521-560 in keyringBlob.write to treat
indexedKeys decode failures as no known prior keys and continue Save/Delete
instead of returning an error.
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
1 participant
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.
Summary
PR Gitlawb#574 moved the macOS keyring write path to
security -i, which isnecessary to keep the secret out of the process list and out of a
getpass(3) /dev/tty prompt. But
security -i's command parser caps asingle write at 4095 bytes, and the oauth store combines every
provider and MCP token into one JSON blob under a single keyring
entry. That blob has no size bound of its own: three or more
logged-in providers routinely exceeds the cap, so Set() starts failing
for every provider, not just the one that pushed it over the line.
This splits keyring storage into one entry per token key (account =
key), plus a small index entry listing which keys currently exist,
since KeyringClient has no list operation. Each write is now bounded
to a single token's size, comfortably under the 4095-byte cap
regardless of how many providers are logged in.
Installs still on the old combined-entry format keep reading
correctly through a legacy fallback, and get migrated to per-key
entries (with the legacy entry removed) the next time anything is
saved.
Test plan
go build ./...make lint(fmt-check + vet)go test ./internal/oauth/... ./internal/keyring/... ./internal/credstore/... -racetokens and asserts no single keyring entry exceeds a 3000-byte
margin under the line cap
securityCLI in a throwawaytest keychain that the underlying
security -iwrite/readmechanism this builds on works correctly (quoting, round-trip,
length guard)