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 26 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 recover legacy and partially written data, while writes reconcile migrations, interruptions, deletions, and mixed-version updates. Refresh preserves existing scopes when responses omit them. 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
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
- 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>
Co-Authored-By: cairn-code <282421612+cairn-code@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/oauth/store.go (1)
281-292: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not derive the shared lock identity from
HOME/USERPROFILE.
os.UserHomeDir()uses those environment values, so two processes for one OS user with different home overrides can still acquire different locks and race the shared keyring index. This repeats the previously reported issue despite being marked addressed. Use an OS-account-derived identity and add a regression varying both home variables.🤖 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 281 - 292, The keyringLockPath function still derives the shared lock location from HOME/USERPROFILE via os.UserHomeDir, allowing one OS account to use different locks. Replace that identity source with an OS-account-derived stable value, retain the existing temp fallback semantics, and add a regression test that varies both home environment variables while verifying the lock identity remains the same.
🤖 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.
Duplicate comments:
In `@internal/oauth/store.go`:
- Around line 281-292: The keyringLockPath function still derives the shared
lock location from HOME/USERPROFILE via os.UserHomeDir, allowing one OS account
to use different locks. Replace that identity source with an OS-account-derived
stable value, retain the existing temp fallback semantics, and add a regression
test that varies both home environment variables while verifying the lock
identity remains the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a731bf4-3939-4b5f-ad2b-ee5906d2984e
📒 Files selected for processing (2)
internal/oauth/store.gointernal/oauth/store_keyring_test.go
There was a problem hiding this comment.
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_keyring_test.go (1)
35-38: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThese tests now write lock files into the real
$HOME, and the comment is stale.keyringLockPathdeliberately ignores the env map and usesos.UserHomeDir(), sot.Setenv("XDG_CONFIG_HOME", ...)no longer redirects the keyring lock — every keyring test in this file acquires~/.cache/zero/oauth-keyring-zero-oauth-tokens-index.lockfileon the machine running the suite. Sequential package execution hides it today, but two concurrently running test binaries (or a leftover lock from a killed run) will hit the 5s acquisition timeout, and it pollutes a contributor's home directory. SettingHOMEper test keeps it hermetic and also fixes the comment.🧪 Proposed fix (apply to each keyring test that sets XDG_CONFIG_HOME)
- // Keep the cross-process keyring lock file inside a temp config dir. - t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + // Keep both the legacy (config-anchored) and keyring (home-anchored) + // lock files inside temp dirs so the suite never touches the real $HOME. + t.Setenv("HOME", t.TempDir()) + t.Setenv("USERPROFILE", t.TempDir()) + t.Setenv("XDG_CONFIG_HOME", t.TempDir())A shared
newKeyringTestStore(t)/isolateLockPaths(t)helper would avoid repeating this in ~15 tests. NoteTestKeyringLockPathIsPerUserintentionally asserts against the realos.UserHomeDir(), so it should compute the expectation after the override rather than being skipped.🤖 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 35 - 38, Update all keyring tests that currently set only XDG_CONFIG_HOME to override HOME with the test-specific temporary directory, preferably through a shared helper such as newKeyringTestStore or isolateLockPaths. Ensure keyringLockPath resolves lock files within the temporary home, update the stale comment, and adjust TestKeyringLockPathIsPerUser to compute its expected path after the HOME override.
🧹 Nitpick comments (4)
internal/oauth/flow_test.go (1)
303-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the configured-scope fallback branch.
This regression test verifies non-empty
current.Scopes, but not the newlen(current.Scopes) == 0path that falls back tocfg.Scopes. Add a case with empty current scopes and assert that the returned token preservesfallback-scopewhen the response omitsscope.Suggested test case
+func TestRefreshFallsBackToConfiguredScopesWhenCurrentScopesAreEmpty(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"access_token":"new-at","expires_in":3600}`)) + })) + defer server.Close() + + cfg := Config{ClientID: "c", TokenEndpoint: server.URL, Scopes: []string{"fallback-scope"}} + tok, err := Refresh(context.Background(), server.Client(), cfg, Token{RefreshToken: "keep-me"}, nil) + if err != nil { + t.Fatalf("Refresh: %v", err) + } + if len(tok.Scopes) != 1 || tok.Scopes[0] != "fallback-scope" { + t.Fatalf("refresh should fall back to configured scopes, got %v", tok.Scopes) + } +}🤖 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, Extend TestRefreshPreservesScopesWhenOmitted with a case using a token whose Scopes is empty, while keeping cfg.Scopes set to fallback-scope and the response without scope. Assert Refresh returns fallback-scope in the token’s Scopes, covering the configured-scope fallback branch alongside the existing current-scope case.Source: Coding guidelines
internal/oauth/store.go (3)
896-899: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth logging and returning the same message double-reports. Every caller already wraps/propagates this error;
log.Printffrom a library function also writes to whatever stderr the CLI owns. Consider dropping the log and letting the caller decide.♻️ Proposed cleanup
func errKeyringIndexTooManyKeys(count int) error { - log.Printf("warning: oauth: keyring token index lists %d keys, over the %d-key cap", count, maxKeyringIndexKeys) return fmt.Errorf("oauth: keyring token index lists %d keys, over the %d-key cap", count, maxKeyringIndexKeys) }(and drop the now-unused
logimport if nothing else uses it)🤖 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 896 - 899, Remove the log.Printf call from errKeyringIndexTooManyKeys and retain only the formatted error return, allowing callers to control reporting. Remove the log import if no other code in the file uses it.
1082-1132: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA panic inside
fnstrands the lock files and the lease goroutine.close(stop)and the unlock loop only run on normal return, so a panic anywhere in the keyring read/write path (a nil map, a bad type assertion) leaves a lock file behind that other processes must wait ~30s to reclaim, plus a live ticker goroutine.defermakes this unconditional at no cost.🛡️ Proposed fix
if len(held) == 0 { return fn() } + defer func() { + for i := len(unlocks) - 1; i >= 0; i-- { + unlocks[i]() + } + }() stop := make(chan struct{}) done := make(chan struct{}) go func() { @@ }() + defer func() { + close(stop) + <-done + }() - err := fn() - close(stop) - <-done - for i := len(unlocks) - 1; i >= 0; i-- { - unlocks[i]() - } - return err + return fn() }Note the deferred order: the lease-stop defer must run before the unlock defer, which the ordering above gives you.
🤖 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 1082 - 1132, Update withLeasedLocks to defer cleanup immediately after starting the lease goroutine so it runs during both normal returns and panics from fn. Ensure the defer order stops and joins the lease goroutine before releasing locks, while preserving reverse-order unlock behavior and returning fn’s error normally.
283-292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment misdescribes
resolveHomeDir. It honors onlyHOME/USERPROFILE(lines 179-188), notZERO_OAUTH_TOKENS_PATH/XDG_CONFIG_HOME. The doc block above (lines 266-280) also says the path is anchored "see resolveHomeDir" while the body deliberately usesos.UserHomeDir. Worth aligning both so the next reader doesn't "simplify" this back toresolveHomeDir.✏️ Suggested comment fix
- // Use OS.UserHomeDir (not resolveHomeDir) for a stable per-OS-user - // identity. resolveHomeDir honors ZERO_OAUTH_TOKENS_PATH and - // XDG_CONFIG_HOME overrides; using those for the lock path means - // two processes for the same keychain user with different HOME - // values take different locks, both read-modify-write the shared - // keyring index, and one saved token can be left unindexed. + // Use os.UserHomeDir (not resolveHomeDir) for a stable per-OS-user + // identity: resolveHomeDir honors the caller-supplied HOME/USERPROFILE + // env map, so two processes for the same keychain user with different + // HOME values would take different locks, both read-modify-write the + // shared keyring index, and one saved token could be left unindexed.🤖 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 283 - 292, Correct the comments above the lock-path logic to accurately describe resolveHomeDir as honoring HOME/USERPROFILE, not ZERO_OAUTH_TOKENS_PATH or XDG_CONFIG_HOME. Update the doc block and inline comment around the os.UserHomeDir-based path to explain that the lock is anchored to the OS user’s home directory and must remain distinct from resolveHomeDir.
🤖 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.
Outside diff comments:
In `@internal/oauth/store_keyring_test.go`:
- Around line 35-38: Update all keyring tests that currently set only
XDG_CONFIG_HOME to override HOME with the test-specific temporary directory,
preferably through a shared helper such as newKeyringTestStore or
isolateLockPaths. Ensure keyringLockPath resolves lock files within the
temporary home, update the stale comment, and adjust
TestKeyringLockPathIsPerUser to compute its expected path after the HOME
override.
---
Nitpick comments:
In `@internal/oauth/flow_test.go`:
- Around line 303-316: Extend TestRefreshPreservesScopesWhenOmitted with a case
using a token whose Scopes is empty, while keeping cfg.Scopes set to
fallback-scope and the response without scope. Assert Refresh returns
fallback-scope in the token’s Scopes, covering the configured-scope fallback
branch alongside the existing current-scope case.
In `@internal/oauth/store.go`:
- Around line 896-899: Remove the log.Printf call from
errKeyringIndexTooManyKeys and retain only the formatted error return, allowing
callers to control reporting. Remove the log import if no other code in the file
uses it.
- Around line 1082-1132: Update withLeasedLocks to defer cleanup immediately
after starting the lease goroutine so it runs during both normal returns and
panics from fn. Ensure the defer order stops and joins the lease goroutine
before releasing locks, while preserving reverse-order unlock behavior and
returning fn’s error normally.
- Around line 283-292: Correct the comments above the lock-path logic to
accurately describe resolveHomeDir as honoring HOME/USERPROFILE, not
ZERO_OAUTH_TOKENS_PATH or XDG_CONFIG_HOME. Update the doc block and inline
comment around the os.UserHomeDir-based path to explain that the lock is
anchored to the OS user’s home directory and must remain distinct from
resolveHomeDir.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 623e9dc2-db0f-4014-bec5-9d222a636152
📒 Files selected for processing (5)
internal/oauth/flow.gointernal/oauth/flow_test.gointernal/oauth/lock.gointernal/oauth/store.gointernal/oauth/store_keyring_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I rechecked the current head against the prior review. The chunked-index cap, dedupe-before-fan-out, env-aware legacy lock path, lock-free file reads, and interruption tests look addressed. The mixed-version and logout gaps below are still reproducible in the code on this head.
Findings
-
[P1] Keep the index lock tied to a stable per-keychain identity, not per-process
$HOME
internal/oauth/store.go:281
keyringLockPathignores itsenvargument and anchors onos.UserHomeDir(), which on Unix follows each process’s$HOME. Twozeroprocesses for the same OS keychain user but with different home overrides (sandbox wrappers, CI harnesses, launcher profiles) therefore take different lock files under$HOME/.cache/zero/…while still read-modifying-writing the fixedzero/oauth-tokens-indexentry. That revives the cross-process index race this PR added locking to prevent: one process’sSavecan be dropped from the published index while its per-key entry still exists, or vice versa. This is unlikely in a single desktop shell with one stable$HOME, but it is real for the sandbox/CI cases this lock design already discusses. Please derive the lock from a stable per-OS-user identity that does not vary with caller-controlledHOME, and add a regression test with two stores whose processes use different$HOMEvalues but share one injected keyring client. -
[P1] Serve the fresher legacy credential during the compatibility window on read paths
internal/oauth/store.go:624
Once the index exists,read()always keeps the indexed per-key entry whenkr.Get(service, key)succeeds. The trailing legacy merge only adds keys that are absent from the assembled map; it never runslegacyIsFresheragainst overlapping keys. During mixed-version rollout an old binary refreshes a provider only in the legacy combined entry, soLoad,Status, andGetFreshkeep returning the stale indexed access/refresh token until some unrelatedSave/Deletetriggerswrite()reconciliation. That can surface expired credentials, skip needed refresh, or fail with a rotated refresh token before any write occurs.TestStoreKeyringMergesFreshLegacyRefreshOfIndexedKeyonly covers reconciliation onSave, notLoadalone. Please apply the same verified freshness rule on read (or keep both representations coherently readable until migration completes), and add a test that seeds a fresher legacy refresh for an already-indexed key then assertsLoadreturns it without a subsequentSave. -
[P1] Do not discard valid refreshes that omit
expires_in
internal/oauth/store.go:728
OAuth refresh responses may omitexpires_in, andRefreshdoes not carry the previous expiry into its base token when building the refreshed result. During mixed-version reconciliation, an old-binary legacy refresh can therefore land withExpiresAtzero while the indexed copy still carries a non-zero expiry.legacyIsFresherthen returns false without comparing access/refresh material, sowrite()keeps the stale indexed credential and deletes the legacy blob in step 4. That loses a valid (and potentially rotation-required) refresh during the supported mixed-version window. Please reconcile overlapping keys using token material and explicit expiry semantics consistent withToken.Expired(zero expiry means “unknown”, not “older”), not expiry ordering alone. -
[P1] Do not resurrect a token the caller just logged out
internal/oauth/store.go:789
If an old binary addsbetaonly to the legacy blob after the new index already containsalpha,read()exposesbeta, soDelete(beta)removes it from the in-memory state. Duringwrite()reconciliation,betais not inprior(it was never indexed), so the branch at lines 799–802 classifies that same legacy value as a fresh old-binary login and puts it back; the operation can returnremoved=truewhilebetais written into the indexed store and the legacy blob is deleted. Please preserve deletion intent through reconciliation (for example by tracking keys removed in the current operation, or by refusing to merge legacy keys that were present in the read state but absent from the write state) so logout cannot leave the credential usable. -
[P1] Do not re-expose logged-out credentials through the read-side legacy merge
internal/oauth/store.go:668
write()refuses to resurrect deliberately removed keys whenprior[key]is set, butread()unconditionally merges every legacy token whose key is absent from the indexed map. If logout’s multi-stepwrite()deletes the per-key entry and shrinks the in-memory state but fails while deleting the legacy blob (step 4), or if a still-running pre-PR binary repopulates the legacy combined entry after a successful logout, the nextLoad/Statusrebuilds the credential from legacy with nopriorguard. A later unrelatedSavecallsreadState()first and can re-persist that credential viawrite()step 2. Please mirror the logout-preservation rule on the read path (or block legacy consultation under the same lock contract as mixed-version writes), and cover both “failed legacy delete after entry delete” and “old binary rewrites legacy after shrink” with tests that assertLoadstays empty without retryingDelete. -
[P3] Narrow the per-token size claim or add an explicit single-entry bound
internal/oauth/store.go:831
This PR correctly fixes the reported regression: many providers in one combined blob exceeded the macOSsecurity -iline cap. Per-key storage removes that aggregation failure. Eachkr.Setstill ships one marshaled token as a singlesecurity -iline, andinternal/keyringrejects lines longer than 4095 bytes including service/account framing. A single login with very large JWT payloads could still fail on macOS even with one provider logged in. That is a separate edge case from the multi-provider bug, and the regression tests only cap encoded secret length infakeKR(~3000 bytes), not the full command line. Please either tighten the PR/release wording to “bounded per provider count” or add a pre-write size check with a clear error for oversized single-token payloads. -
[P3] Consider whether the five-second lock acquisition timeout is too tight under slow keyring I/O
internal/oauth/lock.go:15
fileLockTimeoutis five seconds, while each keyring subprocess can run up to ten seconds and a contestedLoad/SaveunderwithLeasedLocksmay wait behind a holder doing several operations. A spuriousoauth: timed out acquiring token lockis possible under heavy contention or a slow keychain, though CI smoke on this head passed and typical installs have few providers. This looks latent rather than a main-path regression introduced here. If you keep the current timeout, a short comment documenting the assumed upper bound on held critical sections would help; otherwise align the waiter timeout with worst-case keyring latency.
…ries Address requested review changes for keyring per-provider entries: - Anchor keyring lock path on user.Current() home directory so it stays stable across per-process HOME overrides. - Serve fresher legacy credentials directly on read paths without requiring a Save. - Evaluate token material changes in legacyIsFresher when expiries are zero (omitted expires_in). - Preserve deletion intent through reconciliation so logged-out tokens are never resurrected. - Add maxKeyringSingleEntryBytes bound check (3800 bytes) before keyring Set. - Document fileLockTimeout critical section upper bounds. Refs Gitlawb#668
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 legacy migration across configured roots
internal/oauth/store.go:296
legacyKeyringLockPathselects the lock from the new process'sZERO_OAUTH_TOKENS_PATH/XDG_CONFIG_HOME, while a pre-PR binary selects it from its own environment; both still mutate the fixedzero/oauth-tokenskeyring item. If the old process uses root A and the new one uses root B, the new writer can reconcile the legacy blob, the old writer can save a login or rotated refresh under A's lock, and the new writer then deletes that unseen blob under B's lock. This loses a successful credential update during the explicitly supported mixed-version window. The current test only holds the new store's computed legacy lock; preserve the legacy data or use a migration protocol that remains safe when the two processes cannot share a configuration-root lock. -
[P1] Do not hide legacy-only credentials after publishing the index
internal/oauth/store.go:614
Once an index exists,readiterates only its keys and consults the legacy blob only for an overlapping/missing indexed key. An old binary can therefore log into a new provider after migration, leaving (for example)provider:betaonly inoauth-tokens;Load,Status, andFirstStoredall report it absent until an unrelated new-version write happens.Delete(beta)then returnsfalseand leaves the credential behind, which a laterSavemerges at line 786. Keep legacy-only keys visible—and make their deletion durable—through the compatibility window rather than making the index authoritative before old writers are gone. -
[P1] Do not use expiry as the ordering protocol for rotated credentials
internal/oauth/store.go:719
legacyIsFreshertreats a legacy token with an earlier non-zero expiry as stale even when its access/refresh material has rotated, and treats any material difference as legacy-newer when either expiry is zero. OAuth responses may legitimately omitexpires_in(the newRefreshpath leavesExpiresAtzero), and a successful refresh can receive a shorter TTL. Consequently an old legacy copy can overwrite a just-refreshed indexed token, or a fresh old-binary rotation can be discarded, before line 856 deletes the only other copy. The two representations need a monotonic migration/revision rule; expiry is not a write-order signal. -
[P2] Cap raw index input before deduplicating it
internal/oauth/store.go:936
The 512-key limit is applied only afterjson.Unmarshalhas materialized the entire header/chunk array anddedupeValidKeyshas preallocated a map and result slice sized to that raw array. A corrupted Linux keyring entry containing millions of copies of one valid key deduplicates to one and passes the cap, but consumes unbounded memory and CPU while every OAuth operation holds the store lock. Apply a byte/raw-element limit before unmarshalling or preallocation; the duplicate regression tests currently exercise exactly the unbounded-input shape but only at a few thousand entries.
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] Serialize migration with legacy writers using other config roots
internal/oauth/store.go:296
legacyKeyringLockPathis calculated only from the new process'sZERO_OAUTH_TOKENS_PATH/XDG_CONFIG_HOME, while a still-running pre-PR binary calculates its sole lock from its own root even though both write the fixedzero/oauth-tokenskeyring account. With old process A and new process B using different roots, A can save a login or rotated refresh after B has reconciled the legacy blob but before B deletes it at line 861; B then deletes A's successful update. The compatibility test shares one environment, so it misses this supported mixed-version path. Use a migration protocol that cannot delete an uncoordinated legacy write, or retain the legacy representation until old writers are excluded. -
[P1] Give mixed-version copies a real ordering rule before merging them
internal/oauth/store.go:736
legacyIsFreshertreats any different access or refresh string as newer and ignores every other field. An old binary can save a stale whole-blob snapshot while a new binary has already refreshedalpha; bothLoadand the next unrelatedSavereplace the newer indexed alpha with that stale legacy value and then delete the only newer copy. Conversely, a refresh that reuses the token strings but extendsexpires_inis discarded. A migration revision/tombstone protocol is needed here—expiry and token material are not a safe cross-writer ordering signal. -
[P1] Preserve logout semantics while reconciling the two formats
internal/oauth/store.go:791
The migration has no durable deletion signal. If the index containsalphaand an old binary writes legacy-onlybeta,readexposes beta;Delete(beta)removes it from memory, but this loop sees beta absent fromprior, classifies the same legacy value as a fresh login, and writes it back. The call returnsremoved=truealthoughLoad(beta)still succeeds. The opposite direction is also lost: when an old binary deletes indexed alpha from its legacy blob, this loop never sees an alpha entry to reconcile, so new processes continue serving alpha. Preserve deletion intent/tombstones across reconciliation instead of treating an absent key as an unchanged stale copy. -
[P2] Bound the index payload before decoding it
internal/oauth/store.go:941
The new 16,384-element guard runs only after base64 decoding andjson.Unmarshalhave fully allocated the keyring-controlled header or chunk. A corrupted Linux keyring entry can therefore contain a huge JSON payload (or a small number of enormous strings) and consume memory/CPU on every Load, Status, Save, or Delete while the store lock is held, before the count check rejects it. Cap encoded/decoded bytes before unmarshalling (valid writer output is already bounded to 2700 raw bytes per chunk). -
[P2] Make lock acquisition cover a healthy keyring pass
internal/oauth/lock.go:19
A contender gives up after five seconds, but oneinternal/keyringGet/Set/Delete is allowed to run for ten seconds and the new read/write path performs one such call per indexed entry while holding this global lock. A healthy slow keychain operation therefore makes concurrent ordinary Load/Status/Save/Delete fail with a timeout; lease refresh only prevents unsafe reclamation. Align the wait timeout with the bounded critical-section latency (or otherwise avoid failing concurrent callers behind a live holder).
Preserve logout intent through legacy reconciliation so a legacy-only credential is not reclassified as a fresh old-binary login after Delete. Restore hybrid legacyIsFresher ordering (expiry when both known, else token material) so zero-expiry refreshes are not discarded, and cover the remaining mixed-version lock and delete edge cases with regression tests. Refs Gitlawb#668
|
Addressed remaining keyring migration P1s:
Earlier tip already had: fan-out cap after dedupe, |
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] Validate token entries before publishing their index keys
internal/oauth/store.go:842
writecommits the union index at line 856, but only marshals and checks the new values at lines 861-868. A rejectedSave(provider:x, hugeToken)therefore leavesprovider:xin the authoritative index without an entry.readdeliberately skips that missing entry, soDelete(provider:x)returns false without rewriting the index. After 512 such rejected, unique saves, the next normal save constructs a 513-key union and fails the reader cap before it can repair the index; the keyring store remains unable to save credentials until the user manually edits the keychain. Preflight and size-check the complete desired token set before publishing any index, and make interrupted writes recoverable with explicit operation state (or a verified missing-entry pruning path) so unmaterialized index keys cannot consume permanent capacity. This needs to coverKeyringClient.Setfailures too, not only oversized payloads. -
[P1] Serialize migration with legacy writers using other config roots
internal/oauth/store.go:296
The compatibility lock cannot work across process-local config roots:legacyKeyringLockPathderives B's lock from the new process'sZERO_OAUTH_TOKENS_PATH/XDG_CONFIG_HOME, whereas a pre-PR process with root A takes only A's legacy lock. Both nonetheless mutate the fixedzero/oauth-tokenskeyring account. New B can read/reconcile the legacy blob, old A can save a login or refresh, and B then deletes that unseen update at line 889. The credential is never indexed and is permanently lost. Since a new binary cannot discover every old process's configured root, adding another new-schema lock is not sufficient. Keep/dual-write the legacy representation for the supported coexistence period, or use a migration protocol that never deletes it until old writers are known to be excluded; add a regression test with two distinct config roots. -
[P1] Do not use token contents or expiry as a cross-version write order
internal/oauth/store.go:746
legacyIsFreshertreats a later expiry, or any changed access/refresh string when either expiry is absent, as proof that the legacy copy was written later. Neither property establishes causal order. For example, an old process can leave account A's token expiring tomorrow, then a user explicitly logs in through the new binary as account B with a one-hour token. The reconciliation loop replaces the just-supplied B token at line 825 with A, writes A, and deletes the only B copy. OAuth also permitsexpires_into be absent, so different token material alone makes either stale no-expiry copy win. This can silently retain revoked credentials or authenticate as the wrong account. A correct fix needs a durable ordering/deletion protocol (or a coexistence strategy that preserves both formats); expiry and token strings cannot serve as a version vector. At minimum, do not let legacy reconciliation overwrite an explicit currentSavewithout evidence it was written later, and test competing login/refresh writes in both orders. -
[P2] Bound keyring index bytes before decoding JSON
internal/oauth/store.go:969
The raw-key limit is checked only after arbitrary keyring data has been base64-decoded and unmarshaled into[]stringorkeyIndexHeader. A damaged index or chunk can contain a huge payload, or a few huge strings, and consume unbounded memory/CPU before the count guard runs; every OAuth operation reaches this while holding the global lock. The valid writer already limits chunks to 2700 raw bytes, so reject an encoded value whose decoded length exceeds a small header/chunk bound before callingDecodeStringorjson.Unmarshal, and apply the same bound to every continuation chunk. The current element-count cap is complementary, not a substitute: it does not bound the size of an individual JSON string. -
[P2] Let lock acquisition cover a healthy keyring operation
internal/oauth/lock.go:19
Contenders give up after five seconds, but one OS-keyring command is allowed to take ten seconds and the new multi-entry read/write path performs several commands while holding the lease. A healthyGetthat takes six seconds is enough for a concurrentLoad,Status,Save, orDeleteto fail withtimed out acquiring token lock; a multi-provider pass can legitimately be much longer. Lease refresh prevents unsafe stale reclamation but does not help waiting callers. The root problem is a fixed acquisition deadline below the possible critical-section duration, not the stale-lock threshold. Either make waiting cancellation-aware and wait while a holder's lease remains healthy, or redesign the multi-entry read/write around a committed generation/snapshot so readers can retry without waiting for an unbounded pass. Simply raising five seconds to ten seconds is insufficient when the locked operation contains multiple ten-second commands.
Address jatmn review on PR Gitlawb#668: preflight token size before publishing index keys and prune phantoms so failed Saves cannot brick the store; dual-write the legacy blob without deleting it (size-capped) so cross-root old writers cannot permanently lose credentials; stop using expiry/token material as causal order when reconciling; bound index payloads before base64/JSON decode; wait while a lock lease stays healthy. Refs Gitlawb#668
|
Addressed the latest review:
Remaining coexistence gaps (same-key concurrent old-binary refresh, dual-write size cap, true causal order) are noted in the commit; full generation/snapshot redesign deferred. |
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 an old-writer update that lands during legacy reconciliation
internal/oauth/store.go:770
The two versions cannot sharelegacyLockPathwhen their config roots differ, but they still both read-modify-write the single fixedzero/oauth-tokensaccount. In the failing order, new B (root B) snapshots the legacy map here, old A (root A) saves a new provider or rotated credential to that account, then B reachesdualWriteLegacyand writes its stale state at line 896. A's credential was never indexed and B has overwritten its only copy, so neither version can discover it. Retaining the legacy entry rather than deleting it does not make a snapshot-then-Setoperation safe.The root cause is treating two independently writable representations as though a per-config-root lock provides a global serialization or as though the token values can supply write order. Please redesign the coexistence protocol around a durable ordering/ownership rule: for example, keep one representation authoritative and writable until old writers are excluded, or use a persistent generation/tombstone protocol that prevents an unobserved legacy write from being overwritten. Do not try to repair this with another root-specific lock or by comparing expiry/token contents; neither can order these writes. Add a deterministic interleaving test that pauses B after the legacy read, lets A write, then resumes B and proves every credential remains readable.
-
[P1] Keep a logout durable when an uncoordinated old binary rewrites its stale snapshot
internal/oauth/store.go:664
A new-versionDelete(alpha)removes the indexed entry and rewrites its legacy view withoutalpha. An old process using another config root can nevertheless finish a read-modify-write that began before the delete and put its stalealphaback into the shared legacy account.readimports every legacy-only key at this loop, so the nextLoad(alpha)authenticates with the supposedly logged-out token; a later unrelatedSavereindexes it through the merge at line 775.omitFromLegacyapplies only to the one delete call, and there is no persistent deletion marker visible to that later old-writer snapshot.This is the deletion half of the same split-authority protocol problem. A logout cannot be made durable merely by removing a key from whichever representation the current process read: an old snapshot has no way to distinguish a deliberate deletion from a key that it should restore. Address it with the same durable migration state as the write race—e.g. a monotonic generation plus tombstones, or an explicit period in which old writers are no longer supported—and make both reads and reconciliation honor that state. Add a regression where the old writer reads
alpha, the new writer deletes it, the old writer saves another key from its stale snapshot, and bothLoad(alpha)and a later new-versionSavekeepalphaabsent. -
[P1] Do not replace a valid oversized legacy blob with an arbitrary subset
internal/oauth/store.go:917
When the reconciled combined representation exceeds the new 3800-byte write budget, this loop keeps only map entries that fit inpatchTokensand overwrites the legacy account with that subset at line 946. Map iteration makes the retained set nondeterministic. Pre-PR Linux keyring storage has no corresponding single-secret limit, so an existing valid legacy map with several provider logins can be truncated on the first new-version save; a concurrently running or downgraded old binary then silently loses whichever credentials were omitted. The finallen(enc)guard cannot protect this case because each individual addition has already been capped at line 931.The root cause is using the new macOS-safe write bound as a lossy migration transform for data the old backend legitimately stored on other platforms. Never replace a complete legacy representation with a partial one. If a complete compatibility write cannot fit, preserve the existing legacy value unchanged and make the migration state explicitly represent that old readers can no longer receive new changes, or adopt a lossless versioned migration protocol. Add a Linux-compatible fixture whose legacy payload exceeds 3800 bytes, migrate it, and verify that an old-format reader never observes a subset.
-
[P1] Renew the index lease while waiting for the legacy lock
internal/oauth/store.go:1215
withLeasedLocksacquires the shared index lock first, then can block acquiring the config-root-specific legacy lock, but it only starts refreshing mtimes after all acquisitions succeed. If that second lock is stale or held for 30 seconds, another new process on a different config root can reclaim the unrefreshed shared index lock and enter its own keyring update. When the first waiter finally acquires its legacy lock, both processes execute the index read-modify-write concurrently, reintroducing the lost-update race thatlockPathis meant to prevent.The root cause is separating lock acquisition from lease ownership: a lock is already logically owned while the function waits for the next one. Start the lease lifecycle immediately after each successful acquisition (and stop/release it on every error or panic), or change the lock ordering/protocol so the shared lock is never held while a second acquisition can block. Add a controlled test that holds or leaves stale the second lock beyond
fileLockStaleAfter, verifies a competing root cannot reclaim the first lock, and then verifies the two writes remain serialized.
Stop dual-writing the shared legacy blob so uncoordinated old writers cannot be clobbered or truncated, record chunked tombstones for logout durability, and renew each multi-lock lease as soon as it is acquired. Add regressions for the remaining migration coexistence P1s.
|
Addressed the latest review (legacy coexistence P1s). Changes
Test plan
|
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 tombstones authoritative across interrupted writes
internal/oauth/store.go:627
Consider a legacy-only installation containingalpha:Delete(alpha)first reads it through this branch, addsalphatotombstones, and successfully writes that marker at line 881. If the firstwriteKeyIndexcall at line 898 fails (or the process is interrupted), no live index exists. A laterLoad(alpha)returns the untouched legacy blob directly from this branch and never reads the already-durable tombstone, so the supposedly logged-out access/refresh token becomes usable again. This is not merely an error-return retry case: process interruption after the tombstone write produces persistent contradictory state.The inverse transition is also unsafe. After that logout,
Save(alpha, replacement)removes the tombstone at lines 797-800 and persists its removal before either the replacement index or token entry is durable. IfwriteKeyIndexorkr.Setfails, the missing-entry fallback at lines 665-678 imports the previous legacyalpha; a failed re-login has silently restored a credential the user deliberately revoked.The root cause is treating a tombstone, index, and token entry as independently committed state despite readers interpreting them as one authorization decision. Make this a single recoverable transition: all legacy reads, including the no-index path, must honor tombstones; retain a tombstone until a replacement entry and its authoritative index have committed; and add failure-injection tests at every boundary of initial migration, delete, and re-login.
-
[P2] Do not make historical logouts a permanent 512-key quota
internal/oauth/store.go:973
The frozen legacy account means tombstones never naturally expire, yetwriteTombstonesrejects the 513th distinct key beforewritereaches either the live-entry delete or index shrink. A user who has logged out of 512 valid provider/MCP keys over time can still save a 513th credential, butDeletefor it returns an error and leaves its live token in place. This is reachable through ordinary namespaced MCP keys, not malformed keyring data, and survives restart because the tombstone index itself is persistent.The root cause is reusing the live-index capacity limit for unbounded deletion history while retaining the legacy representation indefinitely. Either give deletion markers a representation and lifecycle that can outgrow live credentials (for example, a compact/generation-based migration protocol), or establish a bounded compatibility-retirement point after which tombstones can be safely collected. Please add a regression that creates more than
maxKeyringIndexKeysdistinct login/logout cycles and verifies every later logout still removes its credential. -
[P2] Keep the keyring lock stable when user lookup is unavailable
internal/oauth/store.go:293
Ifuser.Current()fails, this fallback usesos.UserHomeDir(), which on Unix can resolve the process's ambientHOME. Two same-user sandboxed or launcher processes with differentHOMEvalues can therefore compute separate paths, acquire both locks successfully, read the same fixedzero/oauth-tokens-indexaccount, and each publish an index containing only its own update. The later writer then hides the other process's credential even though both operations reported success.The root cause is deriving the lock from process-local environment state after the code correctly identifies the keyring account as process-independent. Use a UID-stable fallback that does not consult
HOME(the existing UID-scoped temporary naming is one viable building block), and exercise theuser.Currentfailure path with different ambient home directories to prove concurrent stores choose one lock. -
[P2] Do not time out behind a healthy large keyring operation
internal/oauth/lock.go:119
A live holder refreshes its lease, but contenders still hit the unconditional two-minute ceiling. The new format permits 512 indexed keys;readvisits each entry, and a save can read the set before writing every desired entry. Each real keyring command is allowed ten seconds ininternal/keyring/keyring.go, so even a small fraction of slow commands can make a healthy pass exceed two minutes. At that point every concurrentLoad,Status,Save, orDeletereturnsoauth: timed out acquiring token lockdespite observing a continuously refreshed lease.The root cause is having two incompatible definitions of a healthy holder: lease renewal tells the waiter the operation is making progress, while the fixed absolute deadline rejects that same operation without considering bounded work or caller cancellation. Make waiting context/cancellation-aware and continue behind a healthy lease, or derive a defensible bound from the maximum indexed work and per-command timeout. Add a controlled slow-keyring test that holds a lease beyond two minutes' scaled equivalent and verifies a concurrent operation waits or receives caller cancellation rather than a spurious store timeout.
Keep tombstones authoritative until replacement credentials commit, allow bounded deletion history beyond the live-token cap, stabilize fallback lock identity, and wait behind healthy renewed leases. Refs Gitlawb#668
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