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 19 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughOAuth keyring storage now uses individually encoded token 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. Refresh also preserves existing scopes. ChangesOAuth keyring storage
OAuth refresh scopes
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
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. |
ea5b965 to
98e1664
Compare
…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.
…ile config The cross-process lock guarding the keyring token index was keyed off ResolveStorePath (ZERO_OAUTH_TOKENS_PATH / XDG_CONFIG_HOME), not off the keyring's own identity (service + index account). Two zero processes with different config roots but pointed at the same underlying OS keyring entry got different lock files, so they could race a read-modify-write on the shared index and silently drop one process's token write. keyringLockPath now derives the lock file name from the keyring service and account instead, so the lock always matches the entry actually being touched regardless of caller config.
…ead error readLegacyTokens collapsed every failure reading the legacy combined keyring entry (a transient keyring error, undecodable base64, invalid JSON) into the same "no tokens" result as the entry genuinely not existing. During mixed-version reconciliation, write() used that result to decide what to merge before unconditionally deleting the legacy blob, so a transient read failure looked identical to "nothing to reconcile" and the blob got deleted anyway, permanently losing a credential that belonged to an older, still-installed zero binary. readLegacyTokens now returns an explicit error distinct from "not found." read()'s recovery fallback stays best-effort (a failure there just skips recovering one desynced key, since it doesn't delete anything), but write() now aborts the whole write and surfaces the error instead of reconciling against an empty map when the legacy blob can't actually be read.
…lookups readKeyIndex's decoded key list was fanned out into one keyring Get per key (Load, Status, Save, Delete all go through it) with no dedup or format check, even though the cap on the index is documented at 25,600 entries. A corrupted or adversarially crafted index that repeats the same key thousands of times still costs one blocking keyring lookup per repeat (each up to the 10s command timeout) while the store lock is held, reintroducing the fan-out DoS the index cap was meant to close. readKeyIndex now runs its result through dedupeValidKeys, which drops duplicate and malformed (non-ValidateKey-shaped) entries before any caller fans them out, so repeats in the index collapse to one lookup per distinct valid key.
…lock The keyring index lock was derived from os.UserCacheDir(), which reads XDG_CACHE_HOME (and falls back through os.TempDir()/TMPDIR) per process. Two same-user zero processes with different cache or temp roots picked different lock files, both read the shared keyring index, and could publish competing read-modify-write updates that hid one process's token. The lock now anchors on the user's home directory instead, which does not vary this way between processes of the same real user. Anchoring on the home directory also lines it up with where a pre-PR binary resolves its own lock (beside ResolveStorePath, which falls back to the home directory too). That pre-PR lock is respected directly now: write() additionally holds it for the whole reconcile-then-delete pass over the legacy combined entry, so a live old binary can't sneak in a fresh legacy login or refresh between this binary's reconciliation read and its legacy-blob delete and have that write silently discarded. Added TestKeyringLockPathIndependentOfCacheAndTempRoots, which varies XDG_CACHE_HOME/TMPDIR between two simulated processes and confirms they still resolve to the same lock, and TestStoreKeyringWriteWaitsForLegacyLockDuringReconciliation, which holds the legacy lock as a live old binary would and confirms Save blocks until it is released, with the seeded legacy token intact afterward.
037b307 to
9e3f52c
Compare
|
I rebased this branch onto upstream/main. That picked up #686 (fix(cron): reserve job IDs atomically), which fixes the atomic job-ID reservation finding at internal/cron/store.go:102 for free; that file now has no diff against upstream/main. For the other two findings, both store.go: The keyring lock was anchored on os.UserCacheDir(), which reads XDG_CACHE_HOME per process (falling back through os.TempDir()/TMPDIR). Two same-user processes with different cache or temp roots picked different lock files, both read the shared keyring index, and could race a read-modify-write that hid one process's token. I moved the anchor to the user's home directory instead, which does not vary this way between processes of the same real user. Added TestKeyringLockPathIndependentOfCacheAndTempRoots, which varies XDG_CACHE_HOME/TMPDIR between two simulated processes and confirms they still resolve to the same lock. Anchoring on the home directory also lines the new lock up with where a pre-PR binary resolves its own lock (beside ResolveStorePath, which falls back to home too). I made write() hold that legacy lock as well for the whole reconcile-then-delete pass over the legacy combined entry, so a live old binary can't write a fresh legacy login or refresh in the window between this binary's reconciliation read and its legacy-blob delete and have that write silently discarded. Added TestStoreKeyringWriteWaitsForLegacyLockDuringReconciliation, which holds the legacy lock the way a live old binary would and confirms Save blocks until it's released, with the seeded legacy token intact afterward. go build ./..., go vet, go test ./internal/oauth/... ./internal/cron/..., and gofmt are all clean. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/oauth/store_keyring_test.go (1)
1014-1029: 🩺 Stability & Availability | 🔵 TrivialMake sure this concurrent path actually runs under
-racein CI.This is the one test spawning a goroutine that races
Saveagainst a held legacy lock — precisely the cross-process hazard the PR exists to fix. The PR notes race testing was skipped where gcc/cgo was missing, which means the highest-value concurrency test can silently run without the detector. Ensure at least one CI lane builds with cgo and runsgo test -race ./internal/oauth/....As per coding guidelines: "run affected concurrent code under the race detector."
🤖 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 1014 - 1029, Update CI configuration to add at least one lane with cgo/gcc available that runs `go test -race ./internal/oauth/...`. Ensure the lane does not silently skip race testing when cgo is unavailable, so the concurrent Save test around the legacy lock executes under the race detector.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@internal/oauth/store_keyring_test.go`:
- Around line 1014-1029: Update CI configuration to add at least one lane with
cgo/gcc available that runs `go test -race ./internal/oauth/...`. Ensure the
lane does not silently skip race testing when cgo is unavailable, so the
concurrent Save test around the legacy lock executes under the race detector.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6cf993ee-5593-43dc-b059-d9516a070a64
📒 Files selected for processing (3)
internal/oauth/lock.gointernal/oauth/store.gointernal/oauth/store_keyring_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/oauth/lock.go
- internal/oauth/store.go
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] Coordinate the legacy migration lock across different config roots
internal/oauth/store.go:298
legacyKeyringLockPathis computed from this process'sResolveStorePath, while pre-PR binaries lock beside their configured file-store path even though both versions modify the same fixedzero/oauth-tokenskeyring item. With different supportedXDG_CONFIG_HOMEorZERO_OAUTH_TOKENS_PATHvalues, the locks differ: the new writer can reconcile the legacy blob, an old writer can save a credential, and the new writer then deletes that unseen credential at step 4. The new global lock cannot protect this because old binaries do not take it. -
[P1] Do not discard legacy token rotations with an unknown or unchanged expiry
internal/oauth/store.go:703
legacyIsFresheraccepts an old-binary update only when both expiries are nonzero and the legacy expiry is strictly later. Zero expiry is a supported token state, and a standards-valid refresh response that omitsexpires_inproduces exactly that state; equal expiries can also accompany a rotated access/refresh token. The next new-version save keeps the stale indexed credential and deletes the only fresh legacy copy, leaving the user with revoked credentials. -
[P1] Read a live legacy credential after an index already exists
internal/oauth/store.go:596
Once any index exists,readconsults the legacy blob only for keys already listed in that index whose individual entries are missing. A concurrently running old binary can therefore successfully log in to a new provider after migration (or after the new binary writes an empty index on logout), but new-versionLoadandStatusreturn no token indefinitely unless some unrelated new-version save happens to reconcile it. This breaks the advertised mixed-version window for normal read-only startup and request paths. -
[P2] Reduce the accepted index size to a practical bounded operation
internal/oauth/store.go:850
The new cap still accepts 25,600 distinct valid keys. EveryLoad,Status,Save, andDeletesubsequently performs one keyring lookup per key while holding both the store mutex and cross-process lock; each lookup may consume the keyring command's ten-second timeout. A damaged but in-range index can therefore block all OAuth operations for many hours, which defeats the corruption/fan-out protection this change is adding. Cap the number of credentials to a realistic bound (or otherwise bound the total lookup time/work) before fanning out.
…ock file components
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/oauth/flow_test.go (1)
303-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new empty-scope fallback branch.
This test covers non-empty
current.Scopes, but the change also adds thecurrent.Scopes == empty→cfg.Scopespath. Add a case with an empty current scope list, configured fallback scopes, and an omitted response scope, then assert the configured scopes are preserved.As per coding guidelines,
**/*_test.gofiles must add regression tests for behavior changes.🤖 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/flow_test.go` around lines 303 - 316, The existing TestRefreshPreservesScopesWhenOmitted only covers non-empty current.Scopes. Add a regression case covering an empty current scope list with configured cfg.Scopes and no scope in the refresh response, then assert Refresh preserves the configured fallback scopes.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@internal/oauth/flow_test.go`:
- Around line 303-316: The existing TestRefreshPreservesScopesWhenOmitted only
covers non-empty current.Scopes. Add a regression case covering an empty current
scope list with configured cfg.Scopes and no scope in the refresh response, then
assert Refresh preserves the configured fallback scopes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0444b2d3-7dca-4f68-99c2-6c2b66e81a9b
📒 Files selected for processing (3)
internal/oauth/flow.gointernal/oauth/flow_test.gointernal/oauth/store.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I rechecked the current head and the following issues remain unresolved.
Findings
-
[P1] Coordinate the legacy migration lock across configured roots
internal/oauth/store.go:299
legacyKeyringLockPathis computed from the new process'sResolveStorePath, while a pre-PR binary locks beside its own configured file-store path even though both versions modify the same fixedzero/oauth-tokenskeyring entry. With different supportedXDG_CONFIG_HOMEorZERO_OAUTH_TOKENS_PATHvalues, the locks differ: the new writer can reconcile the legacy blob, an old writer can save a credential, and the new writer then deletes that unseen credential at step 4. The global keyring lock cannot protect this because old binaries do not acquire it. -
[P1] Do not discard legacy token rotations with an unknown or unchanged expiry
internal/oauth/store.go:704
legacyIsFresheraccepts an old-binary update only when both expiries are nonzero and the legacy expiry is strictly later. Zero expiry is a supported token state, and a valid refresh response that omitsexpires_inproduces it; equal expiries can also accompany a rotated access or refresh token. The next new-version save keeps the stale indexed credential and deletes the only fresh legacy copy, leaving the user with revoked credentials. -
[P1] Read live legacy credentials after an index exists
internal/oauth/store.go:597
Once any index exists,readconsults the legacy blob only for keys already listed in that index whose individual entries are missing. A concurrently running old binary can therefore log in to a new provider after migration, but new-versionLoad,Status, andFirstStoredreturn no token unless an unrelated new-version save happens to reconcile it. A new-version logout of that key also takes the no-token early return and cannot clear the live legacy credential. -
[P2] Bound the accepted index to a practical amount of keyring work
internal/oauth/store.go:850
The 25,600-key cap still permits everyLoad,Status,Save, andDeleteto issue 25,600 serialized keyring lookups while holding the store mutex and cross-process lock. Since a lookup can consume the keyring command timeout, a damaged but in-range index can block all OAuth operations for many hours. Use a realistic credential cap or otherwise bound total lookup work before fanning out.
…ys, lock path, and keyring key cap
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] Make the duplicate-index regression tests pass
internal/oauth/store.go:946
readKeyIndexrejects the rawheader.Keysslice beforededupeValidKeysruns, but the new regression tests deliberately provide 2,000 and 3,000 copies of the same key and expect one deduplicated lookup. Consequently all three Smoke jobs fail ininternal/oauth:TestStoreKeyringReadIndexDedupesDuplicateKeysandTestStoreKeyringDuplicateIndexDoesNotFanOutPerEntryreturn the new 512-key-cap error. Apply the fan-out cap after deduplicating/validating (while retaining a bounded raw input), or make the intended corrupted-index behavior and tests consistent. -
[P1] Acquire the actual pre-migration lock
internal/oauth/store.go:300
legacyKeyringLockPathignoresenvand always selects the process home’s.configpath. The pre-PR binary instead locked besideResolveStorePath(options.Env), so an old process usingXDG_CONFIG_HOMEorZERO_OAUTH_TOKENS_PATHholds a different lock. It can save a login/refresh after the new process reconciles the legacy blob but before line 846 deletes it, permanently losing that credential. Derive this compatibility lock with the sameResolveStorePath(env)rule as the old binary and test a non-default configured root. -
[P1] Keep the index lock tied to the keyring identity, not
HOME
internal/oauth/store.go:281
The new-format lock still derives from caller-controlledHOME, although every process writes the fixedzero/oauth-tokens-indexkeyring entry. Two processes for the same OS keychain user with different home overrides (for example a sandboxed/launcher invocation) therefore take different locks, both perform the index read-modify-write, and one saved token can be left unindexed. Use a stable per-OS-user identity for this lock (and cover differingHOMEvalues), rather than assumingHOMEcannot vary for one keychain user. -
[P1] Do not resurrect a token the caller just logged out
internal/oauth/store.go:790
If an old binary addsbetaonly to the legacy blob after the new index already containsalpha,readexposesbeta, soDelete(beta)removes it fromstate. Duringwrite,betais not inprior, so this branch classifies that same legacy value as a fresh old-binary login and puts it back; the operation returnsremoved=truebut writesbetato the indexed store and deletes the legacy blob. Preserve deletion intent through reconciliation (or otherwise exclude the requested key) so logout cannot leave the credential usable. -
[P1] Do not discard valid refreshes that omit
expires_in
internal/oauth/store.go:719
OAuth responses may omitexpires_in, andRefreshdoes not carry the previous expiry into its base token, so an old-binary refresh can produce a zeroExpiresAtwith a new access/refresh token. When the indexed copy has any nonzero expiry,legacyIsFreshercalls this legacy refresh older andwritedeletes the legacy blob without persisting it. That loses a valid (and potentially rotation-required) refresh during the mixed-version window; use an ordering/reconciliation scheme that does not infer freshness solely from optional expiry metadata. -
[P1] Serve the fresher legacy credential during the compatibility window
internal/oauth/store.go:659
The read path adds legacy tokens only when the indexed map lacks the key. An old binary can refresh an already-indexed token in the legacy blob, soLoadcontinues returning the stale indexed access/refresh token until some later successful write happens to reconcile it. In particular, a rotated old refresh token can fail before that write is reached. Apply the same verified freshness/reconciliation rule on overlapping reads, or keep both schemas coherently readable until migration is complete.
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 662-676: Update the stale comment near the legacy-token loading
logic in read() to state that the legacy blob is also read after the indexed-key
loop when legacyLoaded remains false, including steady-state reads to discover
legacy-only keys from older binaries. Keep the explanation aligned with the
existing unconditional readLegacyTokens() behavior and mixed-version migration
support.
🪄 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 Plus
Run ID: 7c0c0f4c-a538-4167-bf4f-bb55cc4375be
📒 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_keyring_test.go
|
|
||
| if !legacyLoaded { | ||
| if lt, lerr := b.readLegacyTokens(); lerr == nil { | ||
| legacyTokens = lt | ||
| } | ||
| } | ||
| for key, legacyToken := range legacyTokens { | ||
| if ValidateKey(key) != nil { | ||
| continue | ||
| } | ||
| if _, exists := tokens[key]; !exists { | ||
| tokens[key] = legacyToken | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Stale comment: "legacy blob is never read" in steady state is no longer accurate.
The comment at lines 615-620 says the legacy combined entry is "consulted lazily... only when an indexed key's own entry is missing" and that "in steady state (all entries present) the legacy blob is never read." But the code added here at lines 663-667 unconditionally calls b.readLegacyTokens() whenever legacyLoaded is still false after the per-key loop — which is precisely the steady-state case (every indexed key resolved). So every read() call now does an extra kr.Get for the legacy account, even with no pending migration, to catch legacy-only keys written by an older binary.
That's presumably intentional (mixed-version support, per the PR's stated design and the test note about accounting for "the existing legacy-blob read"), but the inline comment directly contradicts what the code now does. Left as-is, this is the kind of comment that invites someone to "simplify" the merge step later, thinking it's dead/defensive code for a case that can't happen.
✏️ Suggested comment fix
// The legacy combined entry is consulted lazily (below) only when an indexed
// key's own entry is missing. write() publishes the index before the per-key
// entries and deletes the legacy blob only after every entry is written, so a
// crash partway through the initial legacy->indexed migration can leave a
// pre-existing credential readable solely in the still-present legacy blob.
- // In steady state (all entries present) the legacy blob is never read.
+ // Even when every indexed key resolves cleanly, the legacy blob is still
+ // consulted once afterward (see below) to recover keys an older binary may
+ // have written there but that aren't in this process's index yet.🤖 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 662 - 676, Update the stale comment
near the legacy-token loading logic in read() to state that the legacy blob is
also read after the indexed-key loop when legacyLoaded remains false, including
steady-state reads to discover legacy-only keys from older binaries. Keep the
explanation aligned with the existing unconditional readLegacyTokens() behavior
and mixed-version migration support.
- Fix undefined keys / declared-but-unused rawKeys in the chunk-read loop of readKeyIndex: the loop must append to rawKeys, and the deduplicated result is assigned to a new keys variable. - Remove the pre-dedup len(rawKeys) > maxKeyringIndexKeys*4 cap on the chunked-path (header+continuation-chunks) index read. The raw array size cap was firing before dedup, so a corrupted index with thousands of duplicate entries was wrongly rejected even though dedup reduced it to a tiny number of distinct keys. The real DoS protection is the post-dedup maxKeyringIndexKeys cap (one blocking keyring Get per distinct key while the store lock is held). - Update TestStoreKeyringDuplicateIndexDoesNotFanOutPerEntry to assert <= 3 keyring Gets (was <= 2). The extra Get is the pre-existing unconditional legacy-blob pre-fetch in read(), not part of the fan-out regression. Co-authored-by: cairn-code
d5f0dfb to
6ea4827
Compare
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 283-292: Update the keyring lock-path logic around the shown
home-directory branch to derive the lock directory from an OS-user lookup rather
than os.UserHomeDir or environment-dependent values, preserving the per-user
identity and existing temporary fallback when lookup fails. Extend
TestKeyringLockPathIsPerUser to verify that different home roots for the same OS
user produce the same lock path.
🪄 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 Plus
Run ID: 53297521-6b01-49b4-9526-227e6d27b2fa
📒 Files selected for processing (2)
internal/oauth/store.gointernal/oauth/store_keyring_test.go
…ex format The existing TestStoreKeyringSkipsIndexedKeyMissingItsEntry exercises the desync recovery only for the flat (single-chunk) index format. This gap means the chunked-path where a continuation chunk is missing (torn write by a killed process mid-write) and a key from the surviving chunk has no corresponding entry was never covered. Add TestStoreKeyringSkipsMissingChunkEntry which seeds a chunked index with two chunks, omits chunk 1 entirely, and places a key in chunk 0 whose own entry is also absent from the keyring. The test verifies that read() skips both missing sources and returns only the token that survives intact. Co-authored-by: cairn-code Co-Authored-By: cairn-code <282421612+cairn-code@users.noreply.github.com>
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