fix(oauth): store keyring tokens as one entry per provider#668
fix(oauth): store keyring tokens as one entry per provider#668euxaristia wants to merge 11 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughOAuth tokens are stored as individually encoded keyring entries coordinated by a bounded chunked index. Reads support legacy recovery and serialized multi-entry access, while writes reconcile migrations, interruptions, deletions, and mixed-version updates. Tests cover limits, corruption, migration, and locking. ChangesOAuth keyring storage
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Store
participant keyringBlob
participant keyIndex
participant tokenEntries
participant legacyEntry
Store->>keyringBlob: Load or Status under read lock
keyringBlob->>keyIndex: Read and validate index
keyIndex-->>keyringBlob: Return token keys
keyringBlob->>tokenEntries: Read encoded token entries
tokenEntries-->>keyringBlob: Return available tokens
Store->>keyringBlob: Save updated store under write lock
keyringBlob->>legacyEntry: Reconcile legacy tokens
keyringBlob->>keyIndex: Publish union key set
keyringBlob->>tokenEntries: Write or delete token entries
keyringBlob->>legacyEntry: Delete legacy whole-blob entry
keyringBlob->>keyIndex: Publish exact key set
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (1)
internal/oauth/store_keyring_test.go (1)
131-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider testing the index/entry desync recovery path.
Migration and deletion-reconciliation coverage look solid. One gap:
read()'scontinuewhen a key is listed in the index but its entry is missing (store.go Lines 483-488) — the mechanism the design relies on for surviving interrupted writes — isn't exercised anywhere. A small test seeding an index that references a key with no corresponding entry (mimicking a killed process mid-write) would lock in that behavior.🤖 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_keyring_test.go` around lines 131 - 172, Add a focused test for the keyring store’s index/entry desynchronization recovery in the read path, such as the relevant Store read method. Seed the fake keyring with an index referencing one missing entry and at least one valid entry, then verify reading skips the missing key without returning an error and still returns the valid token. Model the setup after TestStoreKeyringMigratesLegacyCombinedEntry and use the existing fake keyring helpers and keyring symbols.
🤖 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 447-510: Update Store.Load’s keyring path to execute
keyringBlob.read() through blob.withLock, using the same cross-process locking
established for Save/Delete. Ensure the lock covers the complete index-and-entry
read so concurrent writes cannot produce a stale or incomplete result; preserve
existing read errors and returned data.
---
Nitpick comments:
In `@internal/oauth/store_keyring_test.go`:
- Around line 131-172: Add a focused test for the keyring store’s index/entry
desynchronization recovery in the read path, such as the relevant Store read
method. Seed the fake keyring with an index referencing one missing entry and at
least one valid entry, then verify reading skips the missing key without
returning an error and still returns the valid token. Model the setup after
TestStoreKeyringMigratesLegacyCombinedEntry and use the existing fake keyring
helpers and keyring symbols.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23533b19-6702-40c4-8c89-873eaeb55792
📒 Files selected for processing (2)
internal/oauth/store.gointernal/oauth/store_keyring_test.go
|
Addressed CodeRabbit's review.
go build, go vet, and go test -race -count=1 ./internal/oauth/... ./internal/keyring/... ./internal/credstore/... are all clean. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Bound the key index as well as each token entry
internal/oauth/store.go:560
The newoauth-tokens-indexis still one base64-encoded value written throughKeyring.Set, and therefore through macOSsecurity -i's 4095-byte command-line cap.ValidateKeypermits 137-byte provider keys; an index containing 22 such keys serializes to 3,081 bytes and base64-expands to 4,108 bytes before thesecuritycommand framing, so the index write fails even when every token is tiny. The token entry has already been written, but it is not indexed and cannot be loaded. Use a bounded/chunked enumeration format (or another bounded scheme) and add a cap-aware regression test. -
[P1] Make the multi-entry update recoverable instead of leaving invisible credentials behind
internal/oauth/store.go:549
A write now has several externally visible steps with no durable recovery state. If the process dies after a per-tokenSetbut before the index write, the new credential is unindexed forever; if it dies orDeletefails after line 564 publishes a reduced index, a logged-out credential remains in the OS keychain forever because future cleanup only consults that reduced index. An index-write failure can also return an error after replacing an already-indexed token. This is especially harmful for refresh tokens: the caller can be told login/logout failed while the secret has already changed or remains resident but is unreachable through Zero. Add transactional/recovery metadata (or order the operations with a recoverable invariant) and failure-injection coverage for every interruption boundary. -
[P1] Keep the lock valid for the longer multi-key keyring operation
internal/oauth/store.go:609
The split format holdsoauth-keyring.lockfilewhile it runs one external keyring command per entry, butacquireFileLockreclaims any lock older than 30 seconds and never refreshes its mtime. Each command may legitimately take up to 10 seconds, so a normal read or write with only a few slow entries can exceed 30 seconds; another process then reclaims the live lock and resumes a concurrent read-modify-write, allowing the token-loss race this lock is intended to prevent. Refresh/lease the lock during the operation or use a stale timeout that safely covers the bounded keyring work. -
[P1] Preserve tokens when old and new binaries run during an upgrade or downgrade
internal/oauth/store.go:488
Once a new binary creates the index it ignores the legacy combined entry, while an older running binary continues to read and write only that legacy entry. After migration, an old process can save token C to the legacy blob; the next new-process save reads only the index, rewrites it without C, and deletes the legacy blob, silently losing C. The shared lock serializes the two processes but cannot reconcile the two schemas. Provide a compatibility/dual-write transition or otherwise detect and merge legacy writes before removing the legacy entry. -
[P2] Do not make file-backed reads fail behind a crashed writer's fresh lock
internal/oauth/store.go:279
LoadandStatusnow unconditionally acquire the blob lock, including for the file backend. File writes are atomic renames and reads were intentionally lock-free; after a writer crash leaves its lock file, the new read waits only five seconds and errors while the stale-lock threshold remains 30 seconds. That turns a recoverable process crash into roughly 30 seconds of OAuth read failures even though the last complete token file is readable. Restrict the read-side lock to the multi-get keyring backend, or adjust the file-lock recovery behavior so reads retain the former crash tolerance.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Local review: built and ran go test ./internal/oauth on darwin/arm64; all pass. One real correctness concern worth confirming.
|
Pushed 0b616f7 for jatmn's five findings and gnanam1990's lock-path comment.
go build, go vet, and go test ./internal/oauth pass locally. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/oauth/store.go (1)
212-222: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winPrefer a per-user fallback before
/tmp.
acquireFileLockalready usesO_EXCLand0600, so symlink-following isn’t the issue, butos.TempDir()still gives a fixed lock path in a shared directory. On multi-user hosts another local user can pre-create or hold that file and make keyring saves time out.os.UserCacheDir()would avoid the shared-path DoS; add a test for the fallback branch.🤖 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 212 - 222, Update the fallback lock-path logic in the Store construction to prefer a per-user directory from os.UserCacheDir(), creating or selecting an appropriate cache location before falling back to os.TempDir() only if the user cache directory cannot be resolved. Preserve the resolved store-path behavior, and add coverage for the fallback branch verifying the per-user lock location.
🤖 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_keyring_test.go`:
- Around line 570-573: Extend the crashed-writer lock test around Status to
verify that Status remains lock-free, rather than merely accepting a successful
call after stale-lock expiry. Add a bounded timing or immediate-return assertion
for s.Status("") while preserving the existing error and single-entry checks;
keep Load’s existing timing assertion unchanged.
---
Outside diff comments:
In `@internal/oauth/store.go`:
- Around line 212-222: Update the fallback lock-path logic in the Store
construction to prefer a per-user directory from os.UserCacheDir(), creating or
selecting an appropriate cache location before falling back to os.TempDir() only
if the user cache directory cannot be resolved. Preserve the resolved store-path
behavior, and add coverage for the fallback branch verifying the per-user lock
location.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6bf70feb-0af6-48ed-a808-dbcac2b2f97f
📒 Files selected for processing (2)
internal/oauth/store.gointernal/oauth/store_keyring_test.go
There was a problem hiding this comment.
Clean approve from me this latest commit addresses the earlier collaborator P1s (chunked index, recoverable multi-step write, lock-lease refresh, mixed-version legacy merge, lock-free file reads), and the interruption-boundary tests cover every write/delete step; build, vet, gofmt, and the oauth suite pass. Two minor things worth a glance, neither blocking: when ResolveStorePath fails the keyring lock falls back to a shared os.TempDir() file, which on a shared /tmp multi-user host could cross users (a per-user cache dir would be safer); and in the mixed-version window a token an old binary saves to the legacy entry stays invisible to new-binary Load until the next new-binary Save (the write path merges it, so it's not lost, just temporarily unreadable).
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep the legacy blob readable until a migration has copied every entry
internal/oauth/store.go:620
On a legacy-only installation,Savefirst publishes the indexed header and only then writes the per-token entries. If the process dies orkr.Setfails after that header write,readsees an index and stops consulting the still-intact legacy blob, so the old credentials disappear. A later save treats the incomplete indexed view as authoritative, removes the missing keys, and deletes the legacy blob, making the logout permanent. The interruption tests seed an already-indexed store and do not cover this legacy-to-indexed boundary. Keep a committed migration marker/legacy fallback until all entries are present, and add a failure-at-each-step migration test. -
[P1] Do not discard a same-key refresh made by a concurrently running old binary
internal/oauth/store.go:591
Once the new index exists, an old binary can still refreshprovider:alphain the legacy combined entry. The next new-binary save reads the stale indexed alpha, skips the newer legacy value solely becauseprior[alpha]is true, rewrites the stale value, and deletes the legacy entry. This silently loses a successful refresh and leads to later authentication failures; the added mixed-version test covers only a newly introduced key, not an update to an already-indexed one. Reconcile same-key legacy updates (or retain/dual-write the legacy data for the compatibility window) rather than treating their presence as a deliberate delete. -
[P2] Refresh lock leases with wall-clock time, not the injectable token-expiry clock
internal/oauth/store.go:811
StoreOptions.Nowis a public option and may be a fixed/stale clock. After the first refresh, this sets the live lock mtime to that old value, whileacquireFileLockjudges staleness with realtime.Since; another process can immediately reclaim the live lock and re-enter the keyring read-modify-write path concurrently. That revives the credential-loss race the lease is meant to prevent. Usetime.Now()for the filesystem lease (or make both sides use the same clock) and cover a fixed custom clock. -
[P2] Bound the advertised index chunk count before issuing keyring reads
internal/oauth/store.go:703
The chunked-index parser accepts any integer inheader.Chunksand performs one OS-keyring lookup for every value before returning. A corrupt index such as{"v":1,"chunks":1000000000,"keys":[]}makes every Load, Status, Save, and Delete loop through up to a billion lookups while holding the store lock; a real lookup can itself wait ten seconds. Reject unsupported headers and impose a sane maximum before the loop so damaged keyring state fails promptly instead of wedging OAuth operations. -
[P2] Do not put a per-user keyring lock at a predictable shared-temp path
internal/oauth/store.go:218
WhenResolveStorePathfails, every user falls back to the same${TMPDIR}/zero-oauth-keyring.lockfile. On a multi-user host another account can pre-create or keep refreshing that path; the victim then times out acquiring the lock for Load, Status, Save, and Delete despite using a separate OS keychain. The 0600 mode only protects the file after its creator wins. Prefer a user-private cache/state directory for the fallback and add coverage for this path.
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_keyring_test.go`:
- Around line 774-781: Strengthen the unsupported-version test around
blob.readKeyIndex by resetting the keyring get-call tracker before invoking it
and asserting exactly one lookup occurred afterward. This must verify that only
the index header is read and no advertised chunk is fetched before rejecting
version 2.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae205459-3f0d-4de9-ab6d-908745acb243
📒 Files selected for processing (2)
internal/oauth/store.gointernal/oauth/store_keyring_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/oauth/store.go
|
Pushed fixes for the review findings:
|
7ddca09 to
11d038d
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Preserve mixed-version writes on the read path
internal/oauth/store.go:527
Once the index exists,readconsults the legacy entry only for keys already named by the index. An old binary that is still running can therefore write a newprovider:carollogin to the legacy blob, but a new binary'sLoad,Status, andFirstStoredreport it absent until an unrelated new-binarySavehappens. The same one-way merge cannot observe an old-binary logout of an indexed key, so that credential remains usable. This is an explicitly supported upgrade window; retain/reconcile the legacy state on reads (with a protocol that can distinguish stale state from writes) until old writers cannot exist. -
[P1] Do not discard a refresh whose response has no expiry
internal/oauth/store.go:612
legacyIsFresheraccepts a legacy update only when both expirations are non-zero and the legacy value is later.PostTokenintentionally leavesExpiresAtzero when an OAuth response omits the optionalexpires_in, so an old binary can write a newly rotated access/refresh token that the next new-binary save treats as stale. It then overwrites the fresh value and deletes the legacy entry, producing authentication failures. Use a migration/write protocol that preserves such updates rather than using expiry as the sole version signal. -
[P1] Do not ignore failure to remove the legacy credential blob
internal/oauth/store.go:727
ADelete(alpha)can removealphafrom the indexed store and return success even when deletion of a leftover legacy blob fails. On a later save,alphais no longer inprior, so the stale legacy value is classified as a fresh old-binary login and is written back. The user has successfully logged out according to the API but silently becomes logged in again; propagate/handle that failure or record reconciliation state that prevents resurrection. -
[P1] Bound a single token entry before sending it to the macOS keyring
internal/oauth/store.go:703
Splitting the blob does not ensure that one complete JSONToken, after base64 expansion andsecurity -iframing, fits macOS's 4095-byte command-line limit. Large but valid JWT access/ID tokens or refresh tokens still makeKeyringClient.Setfail, so the login path retains the very failure this PR claims to eliminate for that provider. Enforce a clear per-token limit with an actionable fallback, or chunk the token representation; the five-provider test only covers smaller synthetic values. -
[P2] Reject an index that the reader cannot reopen
internal/oauth/store.go:826
writeKeyIndexwill publish any number of chunks, whilereadKeyIndexrejects headers abovemaxKeyringIndexChunks(128). Enough valid provider/MCP keys therefore letsSavesucceed and persist a 129-chunk header, after which everyLoad,Status,Save, andDeletefails before it can recover the store. Enforce the same maximum before publishing (or make the on-disk format readable beyond it).
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Holding at changes requested, but this is close. The latest push handles both of my earlier notes: the per-user lock fallback (UserCacheDir, uid-scoped temp as last resort) is exactly what I had in mind, and the read-side legacy recovery plus the wall-clock lease refresh are good hardening.
Out of jatmn's latest round, one finding is a real bug I want fixed before merge: write() throws away the error from the final legacy-blob delete (store.go:727). After a logout, if that delete fails, Delete still reports success while the secret stays resident in the keychain — and worse, the next Save sees the leftover legacy key with prior[key] now false and writes it back as a "fresh old-binary login". A logged-out user silently becomes logged in again, and even a retried logout hits the same merge. Propagate that error and add a failure-injection case at that boundary like the ones you already built. While you're in there, enforce maxKeyringIndexChunks on the write side too — writeKeyIndex will happily publish a header that readKeyIndex then refuses, and the guard is a couple of lines.
The rest I'm deliberately not gating on: the read-path visibility gap and the zero-expiry freshness heuristic are confined to the transient old-binary window and documented as best-effort, which I'm fine with — I don't want more protocol thrown at that window. And a single token blowing the 4095-byte cap predates this PR (the combined blob hit that wall sooner), so a clearer error there can be a follow-up. Fix the two items above and I'll approve.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_keyring_test.go`:
- Around line 471-475: Strengthen the legacy merge regression assertions: at
internal/oauth/store_keyring_test.go lines 471-475, load carol and verify its
access and refresh tokens match the expected values, not just presence; at lines
693-699, also assert the loaded credential’s ExpiresAt.Equal(fresh). Preserve
the existing checks for the other legacy values.
- Around line 408-412: Update the fault-injection tests in
internal/oauth/store_keyring_test.go at lines 408-412 and 627-635: capture the
Delete error in the first test and assert that a fired injection is returned;
capture the Save error in the second, assert it is returned, and verify seeded
tokens remain readable before reconciliation. Use the existing test helpers and
symbols, keeping reconciliation after these assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6ae3098-ce1c-449e-9a50-629517bc5d25
📒 Files selected for processing (2)
internal/oauth/store.gointernal/oauth/store_keyring_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/oauth/store.go
|
Pushed 496070e for the two gating items: the final legacy-blob delete's failure now propagates (a logout can no longer report success with the secret resident), and it runs while the union index still lists removed keys — so a retried logout reconciles instead of the stale legacy key being resurrected as a "fresh old-binary login" on the next save. writeKeyIndex now enforces maxKeyringIndexChunks before publishing anything, so Save can't persist a header readKeyIndex refuses. Added a failure-injection test at the legacy-delete boundary (including the no-resurrection-after-retry assertion) and a write-side cap test; the existing write-interruption test now asserts every mutating boundary surfaces its injected failure. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Bound the decoded index keys before iterating them
internal/oauth/store.go:785
The new guard only limitsheader.Chunks; it accepts an arbitrarily largeheader.Keysarray (and the supported legacy bare-array format), thenreadinvokeskr.Getonce per element while holding the cross-process lock. A corrupt index containing thousands of duplicate or invalid keys therefore serializes thousands of OS-keyring calls—each can wait up to ten seconds—and wedgesLoad,Status,Save, andDelete. Cap the raw/decoded index and total unique valid keys before the fan-out, including every chunk and the legacy-array branch. -
[P1] Use a wall-clock deadline when taking the keyring read lock
internal/oauth/store.go:933
withReadLocknow callswithLock, which passesStoreOptions.NowtoacquireFileLock. That helper derives both its deadline and timeout check from the supplied clock; with a legitimate fixed test/embedded clock,now().After(now().Add(5s))can never become true. A fresh peer or orphan lock then makes keyringLoadandStatusretry forever instead of returning a lock-timeout error. Use wall/monotonic time for the acquisition deadline, as the lease refresher already does.
|
Pushed 7b22465. Fixed:
Not fixing, per the sign-off already on this thread:
-race wasn't runnable in my environment (no gcc for cgo), worth running before merge. |
|
Pushed ea5b965. Hardening on top of 7b22465 / 496070e for the remaining write/read symmetry gap around the key-count cap:
Human findings already on this PR, with the commits that closed them:
Still not fixing, per the existing sign-off on this PR:
go fmt, go vet, golangci-lint (unused/ineffassign/staticcheck), govulncheck, and go test ./internal/oauth/... ./internal/keyring/... ./internal/credstore/... are clean. -race needs gcc/cgo and was not runnable in this environment. |
|
@Vasanthdev2004 Ready for re-review on head ea5b965. Both gating items are in:
jatmn's two P1s from the same round (bounding decoded index keys in readKeyIndex and the wall-clock lock deadline) are also fixed, with tests. CI is green on all jobs. |
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.
Store.Load and Status read the keyring blob via several separate Get calls (index, then each entry), not one atomic snapshot, but only Save/Delete ran that read-modify-write under blob.withLock. An unlocked Load/Status could run concurrently with another process's Save/Delete mid write and observe a torn state. Route both through withLock like Save/Delete already do. Also add coverage for read()'s index/entry desync recovery: a key listed in the index whose own entry is missing must be skipped, not fail the whole read.
…xed-version safe - The key index is chunked: continuation entries hold overflow keys and are written before the header that references them, so every index entry stays under the macOS security -i 4095-byte line cap regardless of how many providers are logged in, and a torn chunk write is skipped on read like a missing token entry. - Writes follow a recoverable ordering: the union of the prior and new key sets is published first, token entries are written next, removed entries are deleted while the index still lists them, and only then does the index shrink. Every token entry that exists at any instant is listed in the published index, so an interrupted login/logout can never strand an invisible credential in the OS keychain; the next write reconciles. - A held lock's mtime is refreshed every 10s while the multi-command keyring operation runs, so the 30s stale-reclaim threshold only ever fires for a genuinely crashed holder, not a legitimately slow healthy one. - A legacy combined entry that reappears after migration was written by an old binary still running; its unseen keys are merged before the entry is deleted, so mixed old/new versions during an upgrade no longer lose freshly saved tokens. - Read-side locking is scoped to the keyring backend via a new blob.withReadLock: file-backend reads stay lock-free (writes are atomic renames), restoring crash tolerance when a writer dies holding the lock. - The cross-process lock path falls back to the OS temp directory when no config location resolves, instead of silently degrading to in-process serialization only.
…tion During the initial legacy->indexed migration, write() publishes the index before the per-key entries and deletes the legacy combined entry only as the final step. A crash after the index appears but before an entry is written previously left that pre-existing credential unreadable: the index listed the key, but its own entry did not exist yet. read() now falls back to the still-present legacy blob for any indexed key whose own entry is missing, so a migration interrupted at any point keeps every token readable, and a following unimpeded save completes the migration. write() also reconciles a concurrent old-binary refresh: when the legacy blob holds a strictly later expiry for an already-indexed key, that fresher value wins instead of being overwritten by the stale indexed copy and then deleted.
The lock lease renewal stamped the live lock file using the injectable StoreOptions.Now, but a caller may fix or freeze that clock (for example to drive token-expiry tests). acquireFileLock judges lock staleness with real time.Since(mtime), so leasing with a frozen or backdated clock let another process treat a held lock as stale and reclaim it mid-operation, reviving the token-loss race the lease exists to prevent. Lease with time.Now() regardless of the store clock.
readKeyIndex trusted the stored header's advertised chunk count and issued one
keyring lookup per chunk. A corrupt or hostile header claiming a huge count
(for example {"v":1,"chunks":1000000000}) would fan out into that many blocking
keyring lookups, each up to the command timeout, while holding the store lock,
wedging every Load/Status/Save/Delete instead of failing promptly. Reject an
unsupported index version or an out-of-range chunk count (1..128) up front,
before the read loop.
When no config location resolves, the keyring lock fell back to a single shared
${TMPDIR}/zero-oauth-keyring.lockfile. On a multi-user host any other account
could pre-create or keep refreshing that path and time out the victim's
Load/Status/Save/Delete, even though each user has a separate OS keychain.
Prefer the per-user OS cache directory, and scope the last-resort temp file by
uid so two different users never collide on one lock path.
…ks on write Two review findings: - write() swallowed the final legacy combined-entry delete error, so a logout could report success while the secret stayed resident, and the next save would classify the leftover legacy key as a fresh old-binary login and silently log the user back in. The delete now runs while the union index still lists removed keys and its failure propagates, so a retried logout reconciles instead of resurrecting the credential. - writeKeyIndex could publish a header with more chunks than maxKeyringIndexChunks, which readKeyIndex then refuses — bricking every later Load/Status/Save/Delete. The write side now enforces the same cap before publishing anything. Adds failure-injection coverage for the legacy-delete boundary and a write-side cap regression test; the write-interruption test now asserts every mutating boundary surfaces its injected failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
acquireFileLock computed its deadline using the injectable clock, so a fixed test clock could make lock contention retry forever instead of timing out. Compute the deadline against wall-clock time instead, keeping the injectable clock for token generation only. Cap the number of keys a single keyring index chunk can claim, so a corrupted index can no longer drive an unbounded lookup fan-out while holding the store lock. Strengthen several existing tests that were marked addressed but weren't fully.
writeKeyIndex already refused over-cap chunk counts, but a large set of short keys can stay under the chunk limit while exceeding maxKeyringIndexKeys. Cap the key count before publishing so Save cannot persist an index that readKeyIndex then rejects. Cover the write-side path and multi-chunk accumulation on the reader.
ea5b965 to
98e1664
Compare
|
Addressed both P1s from the latest CHANGES_REQUESTED review (and rebased onto current main). [P1] Bound decoded index keys before iterating them
[P1] Use wall-clock deadline when taking the keyring read lock
Verification
|
4 participants
Summary
PR #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)
Summary by CodeRabbit