You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Every OAuth state mutation used to flush the client's entire in-memory snapshot over the shared file, so a long-lived client holding a stale snapshot (web page, TUI) erased entries other processes wrote since it loaded — an IdP login completed in the CLI was destroyed by any later web-client auth write.
Now each mutation names the sections it touched (server entries by URL, IdP sessions by issuer), and the file backend overlays only those sections onto a fresh read under a lock file. Remote (web) writes carry the same sections in the POST /api/storage/oauth request body as a { sections, snapshot } envelope (the descriptor can name many server URLs, so it must be bounded by the body limit, not the request-line limit); the legacy ?sections= query form is rejected with 400 rather than silently treated as a full replacement. The cli.ts refresh workaround that pre-dated this fix is retired.
2. Tokens and client secrets move to the secret store (8549ef0)
oauth.json keeps only non-secret residue (flow bookkeeping, discovered metadata, public client ids) and acts as the index; acquired access/refresh/ID tokens, IdP session tokens (EMA), and DCR/preregistered client secrets live in the secret store (OS keychain, with the existing secrets.json/memory fallbacks), joined back on read — store wins over file plaintext.
Migration: a pre-existing plaintext oauth.json is migrated lazily on first read, under the file lock, only when the store is durable; under the memory store the file stays authoritative (it is the only durable copy). If moving secrets into the store fails, the strip is aborted — plaintext keeps working rather than losing tokens.
MCP_INSPECTOR_PERSIST_TOKENS=all|access|none: write-side policy for which acquired tokens persist. access drops refresh tokens (and IdP refresh tokens), none drops all acquired tokens; registration client secrets are config, not acquired tokens, and persist regardless. Invalid values warn once and fall back to all.
Degradation: store write failures degrade to memory-only for the secret fields with a once-per-reason warning — secrets are never written back to the file.
Test safety: vitest configs pin MCP_INSPECTOR_SECRET_STORE=memory so no test touches a real OS keychain.
Docs
docs/secret-storage.md (what counts as a secret, the oauth.json split and migration), docs/environment-variables.md (MCP_INSPECTOR_PERSIST_TOKENS), and the README / Docker / CLI-README security wording now lead with token storage.
Out of scope
Read-side staleness (a long-lived client not seeing other clients' writes) is deliberately deferred until the daemon-cli work merges — see #2481.
npm run local:gate green (format, lint, typecheck, tests, coverage thresholds).
Every process (web backend, daemon, CLI) that persists OAuth state used to
flush its whole in-memory snapshot over the shared oauth.json — so a writer
holding a stale snapshot erased entries other processes wrote after it last
read the file (observed live: a background EMA flow wiping a fresh login).
Every write already enters through a mutation scoped to named entries, so
persistence now names what changed and merges only that:
- oauth-persist.ts: OAuthPersistSections + pure mergeOAuthSections (named
servers/idpSessions keys overlaid onto a fresh read; absent = deletion),
parseOAuthPersistSections for the wire form; backends accept an optional
sections arg; the remote backend forwards it as a ?sections= query param.
- oauth-storage.ts: persist(sections) snapshots inside the queued closure
(fresh at write time); every mutation passes its sections, with the
enterprise-managed sweep capturing its URLs before clearing them.
- oauth-persist-file.ts: shared writeOAuthSections = cross-process file
lock -> fresh read -> merge -> atomic write; lock failures rethrown with
OAuth wording and the original as cause.
- remote server storage route: sectioned POSTs apply the same shared locked
merge (400 on bad descriptors or non-OAuth bodies); plain POSTs and the
client store are unchanged.
- cli.ts refreshStoredAuthToken: its hand-rolled read-modify-write now
persists through writeOAuthSections — same lock, same merge, merged
against the file at write time.
Memory is deliberately not refreshed from the merged result: overwriting it
could revert concurrent in-process mutations, and reads staying cached is
fine — correctness comes from the per-mutation read-modify-write.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Split oauth.json persistence so secret material (acquired tokens,
client secrets, IdP session tokens) is written to the OS secret store
while non-secret residue stays in the file:
- New core/auth/node/oauth-secrets.ts: pure split/join/policy module
mapping server entries to oauth:<serverUrl> fields (per-issuer and
legacy tokens/client-secret/prereg-client-secret) and IdP sessions
to oauth-idp:<issuer>.
- Rewrite oauth-persist-file.ts: writeOAuthSections splits secrets to
the store, readOAuthStore joins them back (store wins over file
plaintext) and lazily migrates plaintext secrets when the store is
durable, removeOAuthStore purges store entries. Store write failures
degrade to memory-only with a once-per-reason warning; secrets are
never written back to the file.
- New MCP_INSPECTOR_PERSIST_TOKENS=all|access|none knob controlling
which acquired tokens persist (write-side; registration client
secrets always persist). Invalid values warn and default to all.
- Remote storage routes special-case the oauth store (sectioned and
full-replace writes via locked merge+split, purge on DELETE);
sectioned writes on other stores are rejected.
- CLI reads stored auth via the joined readOAuthStore so migrated
tokens remain visible to --wait-for-auth and refresh.
- Pin MCP_INSPECTOR_SECRET_STORE=memory in web/cli test configs so
tests never touch the real OS keychain.
- Docs: environment-variables.md and secret-storage.md updated.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Reframe the README security warning around what the Inspector stores
(OAuth tokens, OAuth client secrets, stdio env values) and where (OS
keychain by default, secrets.json fallback). Include acquired tokens in
the Docker guide's secret enumeration and plaintext-volume warning, and
update the CLI README's stored-auth wording: oauth.json is the index,
the tokens themselves are read from the secret store.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Moves OAuth credentials into the secret store and introduces section-scoped, lock-protected persistence to prevent stale clients from clobbering unrelated state.
Changes:
Splits OAuth tokens and client secrets from oauth.json.
Adds sectioned persistence, migration, and token-retention policy.
Updates documentation and comprehensive unit/integration coverage.
…ation, locked removal
- Secret store ids no longer contain colons (oauth+<enc-url> /
oauth-idp+<enc-issuer>): the keyring backend parses accounts at the
first colon, so URL-based ids broke deleteAllForServer purges and
prefix matching risked cross-server collisions.
- writeOAuthSections under a non-durable store now preserves
unchanged-from-disk plaintext secrets in the file instead of silently
demoting them to memory-only; new/changed secrets stay session-only.
- removeOAuthStore now runs under the cross-process file lock,
serializing its read→purge→unlink with concurrent writes/migration.
- RemoteOAuthStorage no longer accepts a storeId option; it always
targets the shared oauth store (custom ids fail sectioned writes).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Addressed all 4 findings from Copilot review round 1 in 65a31f4:
Colon-containing store ids → ids are now oauth+<encodeURIComponent(url)> / oauth-idp+<encodeURIComponent(issuer)> (colon-free, prefix-unambiguous); keychain purge works again.
Non-durable store stripping file plaintext → unchanged-from-disk secrets are preserved in the file; new/changed secrets stay session-only. (Lines 282/293 were already durable-gated via readOAuthStore.)
removeOAuthStore not lock-protected → now runs under withSecretFileLock.
Dead storeId option on RemoteOAuthStorage → option removed; always targets the shared oauth store.
New unit + integration tests for each behavioral fix; full local gate (typecheck, lint, tests, coverage ≥90%) passes.
…ompare, policy-free migration
- Secret store deletes now hard-fail when they cannot be confirmed:
KeyringSecretStore.delete/deleteAllForServer throw
KeychainUnavailableError on an unavailable keychain (a missing entry,
including NoEntry-style errors, is still success). Previously a
swallowed delete let the write commit residue that omitted a secret
the store still held, so the next read resurrected cleared or
policy-downgraded credentials.
- writeOAuthSections: a failed set still degrades to memory-only, but a
failed delete/purge now aborts the locked write before the residue is
committed. removeOAuthStore propagates purge failures and leaves the
file in place — it is the only index of the store entries.
- preserveNonDurableSecrets now splits the disk value with the active
policy: under `access` the raw disk blob still carries its refresh
token, so the old compare treated an unchanged access token as changed
and stripped the only durable copy.
- migratePlaintextSecrets splits with "all": migration moves existing
credentials, the persist-tokens policy applies on the next save —
matching the documented write-side policy contract.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Addressed Copilot review round 2 in 10bc31a (3 fixed, 1 declined with rationale):
Swallowed delete failures could resurrect secrets → fixed: keyring deletes throw KeychainUnavailableError when unconfirmed (missing entry = success); in writeOAuthSections a failed delete/purge aborts the locked write (failed set still degrades to memory-only); removeOAuthStore propagates purge failures and keeps the file as the index.
Non-durable preserve compare not policy-aware → fixed: disk side is split with the active policy, so an unchanged access token under access stays durable.
Migration applied the write-side policy → fixed: migration splits with "all" — it moves existing credentials; the policy trims them on the next save, per docs.
Secret ids not namespaced by state-file path → declined: embedding the path would invalidate all credentials whenever the file legitimately moves (renamed home dir, relocated storage dir), and the exposure requires two isolated state files on the same machine/user holding the same server URL. Accepted as a known limitation; see thread.
Full local gate passes; keyring delete contract tests updated and new tests added for each fix.
…k-error typing, route 503s
- FileSecretStore.deleteWhere now propagates SecretStoreUnavailableError
instead of swallowing every failure: the file store is the normal
durable fallback, so an unconfirmed delete there had the same
resurrection hazard the keyring fix closed.
- New SecretFileLockHeldError (thrown only by lock acquisition);
rethrowLockError matches it specifically, so a KeychainUnavailableError
thrown inside a locked callback is no longer misreported as 'the state
file is locked' and keeps the type the HTTP layer maps to 503.
- POST /api/storage maps secret-store failures to the actionable 503 for
every store (previously only client); DELETE /api/storage gains the
same mapping so removeOAuthStore purge failures no longer 500.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Addressed Copilot review round 3 in a837158 (all 4 fixed):
FileSecretStore hid failed deletions → deleteWhere propagates SecretStoreUnavailableError, completing the confirmed-delete contract across both durable stores.
Store failures misreported as lock failures → new SecretFileLockHeldError thrown only by lock acquisition; rethrowLockError matches only it, so store failures inside the locked callback keep their type and message.
POST /api/storage 500 for OAuth store failures → keychainErrorResponse now runs for all stores → 503.
DELETE /api/storage 500 → same mapping added → 503.
Full local gate passes; contract tests updated (undecryptable file, unavailable key, held lock now assert rejection + entry intact).
…ails
The oauth.json file is the only index of the secret store's OAuth
entries. If the atomic residue write failed after the store writes had
committed, a brand-new server's or issuer's secrets were stranded where
removeOAuthStore could never enumerate them. writeOAuthSections now
tracks ids whose secrets had no prior disk entry and best-effort purges
them when the file write throws, before rethrowing the original error.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
File-write failure strands unindexed store credentials → fixed: writeOAuthSections rolls back (best-effort purge) store secrets for entries no disk entry indexed yet before rethrowing the write error; already-indexed entries keep their file index and newer store secrets. Test added (forced write failure → rollback verified).
"FileSecretStore hides failed confirmed deletions" (re-listed as open) → this was already fixed in a837158, which is in the reviewed diff; see thread for specifics.
…ails
The round-4 rollback only deleted store entries for brand-new (unindexed)
server ids. For an already-indexed entry, a store update that succeeded
followed by a residue file-write failure left the store ahead of the file:
the next joined read paired the old residue (e.g. the previous client_id)
with the new secrets (the re-registered client_secret).
writeOAuthSections now snapshots every touched secret field's pre-write
store value (strict reads, so an unreadable store aborts before mutating
rather than masquerading as absence) and, when anything fails after store
mutations begin, best-effort restores those values — set back what existed,
delete what did not — before rethrowing the original error. This subsumes
the unindexed-id rollback (all-null priors become deletes) and also covers
entry removals and mid-loop aborts.
Test: update an indexed entry with a read-only state directory so the
residue write fails, then assert the store again holds the old secrets and
the joined read returns the original consistent entry.
Addresses Copilot review round 5 on #2482.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Rollback fails to restore existing indexed entries → fixed: rollback generalized from "delete unindexed ids" to full snapshot/restore — pre-write store values of every touched secret field are captured (strict reads) and restored on any failure after mutations begin, so an old on-disk residue always rejoins its old secrets. Test added (forced residue-write failure on an indexed entry → prior secrets restored, joined read consistent).
"FileSecretStore hides failed confirmed deletions" → fixed since a837158 (see thread); resolved the thread as the bot kept re-listing it despite the fix being in the reviewed diff.
The retry loop's rollback restored each failing attempt's own priors, but
from the second attempt on those priors observe the values *earlier
attempts* wrote — restoring them treats an intermediate attempt as
committed, stranding a new entry's secrets with no file index after a
clobbered first attempt.
Replace the per-attempt restore with a baseline holding, per touched
(server, field), the latest store value NOT written by this call. This is
decidable because what this call writes is constant across attempts (the
split derives from the caller's snapshot and the policy, never from disk;
candidates outside the split are deletions): a prior equal to our own
write is an earlier attempt's work and keeps the baseline's older value,
anything else is a concurrent writer's newer state and supersedes it — so
the rollback also no longer clobbers a foreign value that landed between
attempts.
The same fold applies to two more retry exits found by adversarial
review: the per-attempt disk read now sits inside the try (a concurrent
writer replacing the file with unrecognized content no longer escapes the
loop without rollback), and the entry-level degrade path receives
baseline-adjusted priors (its compensating restore no longer re-commits
an earlier attempt's own writes under the pre-write residue it keeps in
the file).
Also document honestly in readOAuthStore that on lock-unavailable
filesystems torn reads remain possible: no read-side verification can
observe the store-write-to-file-write window, the tear is transient, and
the write path's convergence loop bounds what persists.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Rollback restoring a failed attempt's own writes (oauth-persist-file.ts:528) — confirmed and fixed with a fold-rule restore baseline: per touched field, the latest value not written by this call (pre-operation value, or a concurrent writer's newer value when one lands between attempts — covering the concurrent-writer caveat in the finding). Decidable because this call's per-field writes are constant across attempts (split derives from snapshot + policy, never disk).
Adversarial review of all exit paths found two more instances of the same class, fixed in the same commit: the retry's disk read escaping the loop without rollback on an unrecognized-file clobber, and the entry-level degrade path re-committing an earlier attempt's writes under old residue.
Correct healing claim for persistent double-failure mismatch
specification/v2_servers_file.md:258
The final healing claim is incorrect. If compensation also fails, the next read only joins whatever value remains in the secret store onto the unchanged file; it does not restore an orphaned secret, recreate a deleted secret, or reconcile companion public fields such as client_id. The new rollback-failure test likewise treats this state as requiring re-authorization (oauth-secret-split.test.ts:317-321). Please document that the double-failure mismatch persists and requires user action instead of promising automatic repair.
The write-ordering bullet claimed a failed-write-plus-failed-compensation
mismatch is healed by the store-wins migration on the next read. That
migration only lifts plaintext still on disk; a stripped file gives the
next read nothing to reconcile with — it joins whatever the store holds
onto the unchanged file, and cannot restore an orphaned secret, recreate
a deleted one, or fix companion public fields. State that recovery is
user action (re-save / re-authorize), matching what the rollback-failure
warning already tells the user.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Healing claim in specification/v2_servers_file.md:258 — confirmed against the tree: the store-wins migration only lifts plaintext still on disk, and post-#1356 the stripped file gives the next read nothing to reconcile with — it joins whatever the store still holds onto the unchanged file, and cannot restore an orphaned secret, recreate a deleted one, or fix companion public fields like client_id. The rollback-failure warning (pinned by oauth-secret-split.test.ts "warns that the store may be inconsistent when the rollback itself fails") already points at re-authorization. The spec now states the double-failure mismatch persists and that recovery is user action (re-save the entry / re-authorize) instead of promising automatic repair.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
Malformed token payloads can be silently discarded, and a consecutive convergence read failure can leave file residue inconsistent with restored secrets.
…payloads
Restructure writeOAuthSections' failure handling around a single
reconciling exit instead of scattered per-site rollbacks:
- Track the last unconfirmed file write together with the store writes
its attempt actually committed. Every failure exit re-reads the file:
if it still holds that write the save is committed, so the store is
re-pointed at exactly the pairing the file holds (baseline values for
fields the committed attempt degraded, its own writes for the rest)
and the call reports success. A file confirmed changed, or never
written, restores the fold baseline; an unreadable file restores too
(biasing store-behind over store-ahead) and warns honestly.
- Allow the memory-only degrade in persistEntrySecrets on the first
attempt only, where the disk entry really is the pre-call state its
contract pairs with. On retries a store failure escalates into the
reconciling exit instead; the baselinePriors substitution this
replaces is deleted.
- Validate token and IdP-session payloads at the API write boundary
with the partial token schema (present fields must be well-typed,
none required — refresh-only entries are legitimate persisted state),
so an untrusted write can no longer accept a payload whose secrets
the next read would silently drop. File reads stay tolerant on
purpose: a corrupt token entry must remain clearable, not brick every
mutation of oauth.json; the store is never at risk because token
payloads are JSON-stringified, unlike verbatim client_secret fields,
which stay strict everywhere.
Covers both round-34 review findings plus two self-found degrade/retry
interactions; new convergence tests are mutation-verified.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Round 34 addressed in d542c83 — a structural pass rather than two spot fixes.
What changed
One reconciling failure exit (finding 1): every failure path in writeOAuthSections now funnels through a single decision procedure that re-reads oauth.json and settles the store against whatever the file is confirmed to hold — committed unverified write ⇒ re-point store + report success; confirmed changed ⇒ restore fold baseline; unreadable ⇒ restore (store-behind bias) + honest warning. This replaces the scattered per-site rollbacks that each previous round has been picking at.
Degrade is attempt-0-only: the memory-only degrade contract is only sound when the disk entry is the pre-call state; on retries a store failure escalates into the reconciling exit. This came out of a pre-push adversarial review of the entire diff, which found two degrade×retry interactions beyond the bot findings — both fixed structurally and locked in with mutation-verified tests.
Write-boundary payload validation (finding 2): type-corrupt token/IdP payloads are rejected at the API write boundary (400) instead of being silently dropped on the next read; file parsing stays tolerant so corrupt entries remain clearable, and verbatim secret fields stay strict everywhere (store-poisoning class).
Full local gate green (typecheck, lint, unit + integration suites across core/web/cli).
The migration path copied a legacy plaintext token payload into the
secret store and stripped it from oauth.json unconditionally, but the
read-side join only serves payloads that pass the full OAuthTokensSchema
- a partial-but-legitimate entry (say refresh-only) was stored with
apparent success and silently destroyed on the very next durable read.
The same loss existed on every ordinary save (legacy and byIssuer slot
tokens) and splitIdpSession stringified non-string junk into the store.
Root cause: the split layer extracted values the join could not serve.
Fix at that layer with one rule - the store only receives what the join
can serve back:
- New splitTokens() helper: post-policy payload goes to the store only
if OAuthTokensSchema.safeParse passes; otherwise it stays plaintext in
the residue (the only place it can survive). A post-policy payload
with no secret-bearing field at all is dropped, not kept, so no
secretless tokens artifact lingers. Applied to legacy and byIssuer
call sites; splitIdpSession stores string-typed fields only.
- migratePlaintextSecrets skips its final write when the residue
serializes byte-identical to the file it read, so kept-plaintext files
are not rewritten on every read.
- getTokens now safeParses and reports a non-servable payload as no
usable tokens instead of throwing: preserved partial tokens reach
mainline durable flows, and a ZodError would brick connection state,
the SDK provider tokens() callback, and every oauthManager path for
that server instead of prompting re-authorization. The CLI stored
token refresh reads the plaintext refresh_token directly and is
unaffected.
All changes are covered by mutation-verified tests, including migration
byte-stability and partial-payload round trips.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Round 35 addressed in b560321. The migration data-loss finding was verified real and fixed at the class root rather than the reported site: the split layer now stores only token payloads the read-side join can serve (splitTokens() helper, inherited by saves, migration, and the byIssuer/IdP variants — an audit found the identical loss on all of them). Unservable-but-legitimate partial payloads stay plaintext in oauth.json; secretless post-policy artifacts are dropped; migration is byte-stable for kept-plaintext files; and getTokens now reports a non-servable payload as no usable tokens instead of throwing. Full local gate and mutation-verified regression tests pass. Note: the reply also corrects an inaccurate claim I made in round 34 about isUsableStoredSecret guarding the migration strip.
splitTokens deliberately keeps a partial-but-legitimate token payload
plaintext in the residue, so the SplitResult docblock's "never carries
a secret value" claim is stale, and future persistence code trusting it
could expose credentials. Document the exception on SplitResult.residue
and in the oauth-persist-file secret-split overview.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Round 36: the one new finding (Low — stale "residue is secret-free" docblock) is fixed in f5b232a, plus the same stale claim in the oauth-persist-file module overview found by a class sweep. Doc-only; prettier/tsc/eslint clean. The High listed as Open was the round-35 thread — its fix shipped in b560321 and this round found no new issues with it; both threads are now resolved.
The store's write and read gates disagreed: writes stored only payloads
passing the full OAuthTokensSchema, so a partial-but-legitimate payload
(say a refresh-only entry inherited from a legacy plaintext file) had to
stay plaintext in oauth.json to survive - leaving a bearer-grade
refresh_token in the file indefinitely, against the point of this
change. Every earlier fix worked around that mismatch instead of fixing
it.
Align both sides on one store contract: a tokens payload is storable iff
the partial token schema accepts it (every present field well-typed,
none required - the same contract the API write boundary enforces) and
it carries at least one secret-bearing field (access_token,
refresh_token, id_token). splitTokens and parseStoredTokens share the
definition, so exactly what the split stores is served back:
- Partial payloads now move to the secret store like any full token set
and rejoin into state.tokens; the CLI stored-token refresh reads the
rejoined refresh_token, and getTokens still withholds partial sets
from the SDK ("no usable tokens" prompts re-auth).
- Only a type-corrupt payload (possible in hand-edited files; the API
rejects it with 400) stays in oauth.json, keeping the entry visible
and clearable. The residue otherwise carries no usable secret.
- A storable but secretless payload is dropped on write and unusable on
read, so no artifact lingers and an empty object cannot win over
plaintext in the join.
- Migration store-wins now honors a partial stored value over stale
plaintext (the store is at least as fresh); type-corrupt store junk is
still replaced by valid plaintext.
- Revocation reads grants with the same contract: parseTokens validates
with the partial schema, so a refresh-only grant is revoked at the
authorization server instead of being cleared locally and reported
unreadable while the live token survived at the AS.
Round-35 tests asserting partials stay plaintext are inverted to the new
contract; new mutation-verified coverage: partial round-trips through
the store on saves and migration (byte-stable), type-corrupt payloads
stay clearable and byte-stable, refresh-only grants revoke with a
refresh_token hint.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Round 37 fixed in 6b7c8ea, at the root: the store write gate (full schema) and the legitimacy of partial token payloads were in conflict, and prior rounds worked around that mismatch — first by destroying partials, then by keeping them plaintext. Both store gates now share one contract (partial schema + ≥1 secret-bearing field), so partial payloads live in the secret store, rejoin for the CLI refresh path, and never sit plaintext in oauth.json; only type-corrupt hand-edited junk stays in the file, where it remains clearable. A contract audit also aligned revocation (parseTokens), which would otherwise clear a refresh-only grant locally while leaving the live token unrevoked at the AS. Full local gate green; mutation-verified round-trip, byte-stability, and revocation tests included.
Every section save passed the full post-split secret set to
persistEntrySecrets, so unchanged credentials were rewritten on every
save. If such a redundant setMany failed (a keychain that turned
read-only between saves), the whole entry degraded to memory-only and a
non-secret update (saveScope, saveCodeVerifier) silently failed to
persist while the call reported success. The delete side carried the
same class, worse: serverSecretFields always lists the legacy candidate
fields, so every save issued redundant deletes for fields the store
never held - and a failed delete aborts the save outright, failing even
saves that needed no store change at all.
persistEntrySecrets now touches only deltas against the strict
per-field prior snapshot taken under the same lock: values that differ
are written, fields the store actually holds are deleted, everything
already in the desired state is left alone. The degrade compensation
restores exactly the fields the filtered batch could have touched. The
reconcile machinery is unaffected: attemptWrites/ourWrites record the
desired value for every candidate, and a skipped field already equals
both its baseline and its desired value.
New mutation-verified integration test: a scope-only save persists
against a store that turned read-only, with the entry not reverted and
the stored credentials untouched (kills removal of either filter). The
retry-escalation test's foreign clobber now restores the store as well
as the file - under delta persistence its old scenario genuinely
converges, which is the correct outcome; the store restore recreates a
real retry delta so escalation stays exercised end to end.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Round 38 fixed in a1914bc: section saves now persist only secret-field deltas against the per-attempt strict snapshot, so a redundant rewrite (or redundant delete — the same class on the other side, which previously made any save fail against a read-only keychain) can no longer degrade or abort a save whose store state was already correct. Reconcile/degrade machinery unaffected (desired values still recorded for every candidate; skipped fields equal baseline). Mutation-verified test included; full local gate green. Thread resolved.
Both result maps need own-property-safe assignment, as the generic getManyStrict fallback now uses. A requested field or server ID named "__proto__" invokes the inherited prototype setter instead of creating an entry, so this FileSecretStore override silently omits requested results and violates the bulk-read contract.
…-safe
Two review findings (round 39):
client.json serialization: the combined file/store writers ran their
compensated snapshot -> store mutate -> file write -> restore sequences
with no serialization, unlike every other combined writer. Two
overlapping saves both snapshot the same prior secret, and the failing
one then restores that stale snapshot over the succeeding one's
committed value, leaving client.json describing one client while the
keychain holds another's secret. writeClientConfigStore,
deleteClientConfigStore and the read-path plaintext migration now run
their whole sequence under the file's lock (withClientConfigLock,
mirroring withOAuthStateLock: acquisition failures are reworded to name
client.json, mid-body lock errors from the nested secret store pass
through). The migration also re-reads the file under the lock, since
stripping from the caller's unlocked read could write a stale config
over a concurrent writer's newer file; a held lock skips the migration
and serves the unlocked read rather than failing it.
Prototype-named keys: FileSecretStore.getMany/getManyStrict built their
result maps with plain assignment, so a requested field or server id
named "__proto__" invoked the inherited setter and was silently omitted
from the result, violating the bulk-read contract that later drives
store deletions. All four sites now use setOwnEntry, as do the inner
field maps of the generic secretStoreGetMany/secretStoreGetManyStrict
fallbacks, whose outer maps were already own-property-safe.
Tests: an interleaving test that parks one writer mid-mutation inside
its critical section while a second writer commits (fails without the
lock), and own-entry assertions for prototype-named fields and server
ids across the file store and both generic fallbacks (fail with plain
assignment). All mutation-verified.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
1. Serialize secret and file mutations per resolved path (thread above, resolved): client.json's combined file/store writers now run their whole compensated sequence — snapshot, store mutation, file write, restore — under the same per-resolved-path lock every other combined writer takes (withSecretFileLock, via a withClientConfigLock mirroring withOAuthStateLock). The class audit also brought the read-path plaintext migration under the lock with a fresh re-read, since it mutates from what was previously an unlocked read. Interleaving test added and mutation-verified (removing the lock reproduces the clobber).
2. Own-property-safe assignment for result maps (file-secret-store.ts:975, body-only finding): both getMany and getManyStrict built their result maps with plain assignment, so a field or server id named __proto__ invoked the inherited setter and was silently omitted. All four sites now use setOwnEntry — and the class audit found the generic secretStoreGetMany/secretStoreGetManyStrict fallbacks had the same defect on their inner field maps (their outer maps were already safe); fixed both. The remaining dynamic-key assignments in the persistence layer were audited and are safe (fixed field-name candidates, prefixed env keys, or allow-list-filtered). Own-entry tests added for the file store and both fallbacks, each mutation-verified.
Full local gate green (typecheck, lint, coverage thresholds, web + CLI suites).
I did a review of the final as-built design (it grew in scope during the PR prosection). I also manually tested all clients accessing/sharing the same shared auth tokens, and validated the physical storage (including that the access and resource tokens were only present in the MacOs keychain).
Every behavior this PR claims held up in a live smoke of the built clients at fc55e11a against real OAuth test servers. The key clobber scenario fails on v2/main and passes here, so the test really does tell the two builds apart. CI is green, the branch is up to date with v2/main (merge state CLEAN), and nothing I found is a regression. One bug surfaced that also reproduces on v2/main. It is unrelated to this PR, should not block it, and is now tracked in #2517.
Setup
PR head fc55e11a and a baseline v2/main (30a9a897), each built from a clean install in its own worktree.
Two composable OAuth servers (oauth-revocation-http.json with DCR, refresh and revocation) on :8093 (A) and :8094 (B).
Every run used its own MCP_STORAGE_DIR. Most used MCP_INSPECTOR_SECRET_STORE=file so the physical split could be read from disk. One run used the default macOS keychain, and its entries were deleted afterwards.
CLI logins were driven headlessly: a pty on stdin, the printed authorize URL scraped, the consent form submitted, and the browser redirect followed to the loopback callback. Web logins used the prod --web bundle under Playwright.
Results
#
Scenario
PR
v2/main
1
CLI interactive login (file store): oauth.json holds only residue (discovery, public client_id, bookkeeping). tokens (access + refresh) and the DCR client_secret are in secrets.json, under oauth+<encoded url>:<field>:<issuer>
✅
n/a
2
Reuse: --stored-auth-only tools/list joins residue + store and succeeds
✅ exit 0
✅
3
Lazy migration: a plaintext oauth.json written by the v2/main CLI, then read by the PR CLI (file store). Tokens and client secret move to the store, the file ends with 0 plaintext secrets, and the call succeeds
✅
n/a
4
Migration under memory store: the file is left byte-identical (same md5) and stays authoritative. The call succeeds and the memory-store warning is printed
✅
n/a
5
MCP_INSPECTOR_PERSIST_TOKENS=access: store has access_token, no refresh_token; client_secret still persisted
✅
n/a
6
…=none: no tokens entry at all; client_secret still persisted
✅
n/a
7
…=bogus: warns once (Ignoring invalid MCP_INSPECTOR_PERSIST_TOKENS="bogus" … persisting all tokens.) on the writing run and persists everything
✅
n/a
8
Clobber-safety (the #2481 bug). Web page logs into A via deep link + consent. The CLI then logs into B while the page keeps its now-stale snapshot. From that page, with no reload, Connection Info → Clear OAuth state and disconnect on A
✅ A removed, B's residue and both B secrets survive, and --stored-auth-only on B still works
❌ oauth.json emptied: B erased, and the CLI on B fails with exit 3
9
The clear's request is POST /api/storage/oauth with body { sections: { servers: ["…8093/mcp"] }, snapshot: { servers: {} } }, i.e. the sectioned envelope in the body
✅
n/a (full-snapshot body)
10
Legacy POST /api/storage/oauth?sections=x
✅ 400The sections descriptor moved from the ?sections query parameter to the request body, file untouched
200, full overwrite
11
Web-written OAuth state (file store): residue clean, secrets in secrets.json; the startup banner names the store (Secrets: File (unencrypted) at …)
✅
n/a (plaintext in oauth.json)
12
Default macOS keychain: CLI login with no store override. No secrets.json is created, oauth.json has 0 plaintext secrets, both …:tokens:… and …:client-secret:… items exist in login.keychain-db under service mcp-inspector, and --stored-auth-only reuses them
✅
n/a
Notes (none of them blocking)
Migration is one-way. After the PR build migrates a directory, a v2/main build pointed at it no longer finds the tokens and has to re-authorize (tested: auth_required, exit 3). That is inherent to moving secrets out of the file and matches the design, but it is worth a line in the release notes.
Browser → backend POST /api/storage/oauth bodies still carry tokens in snapshot, which is expected, since the backend is where the split happens. What lands on disk is clean either way (rows 1, 11).
Pre-existing bug found (reproduces on v2/main, not caused by this PR) → #2517
--use-stored-auth exits 3 with no_stored_token (while listing the very URL under "Stored keys"), and --list-stored-auth returns storedServerUrls: [], even though a valid token is stored. clients/cli/src/cli.ts reads state.tokens at the server level (L323, L386, L1069, L1179). Since the byIssuer storage shape, tokens live at byIssuer[activeIssuer].tokens, so neither the plaintext file on v2/main nor the joined view here is ever matched. --stored-auth-only is unaffected because it goes through the normal provider path. Filed as #2517 (v2.9.0, Todo). It is out of scope here. The fix should read the joined residue + secret-store view this PR introduces.
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2481
Two changes to the shared
oauth.jsonpersistence, in dependency order:1. Clobber-safe writes (cd4ccf9)
Every OAuth state mutation used to flush the client's entire in-memory snapshot over the shared file, so a long-lived client holding a stale snapshot (web page, TUI) erased entries other processes wrote since it loaded — an IdP login completed in the CLI was destroyed by any later web-client auth write.
Now each mutation names the sections it touched (server entries by URL, IdP sessions by issuer), and the file backend overlays only those sections onto a fresh read under a lock file. Remote (web) writes carry the same sections in the
POST /api/storage/oauthrequest body as a{ sections, snapshot }envelope (the descriptor can name many server URLs, so it must be bounded by the body limit, not the request-line limit); the legacy?sections=query form is rejected with 400 rather than silently treated as a full replacement. The cli.ts refresh workaround that pre-dated this fix is retired.2. Tokens and client secrets move to the secret store (8549ef0)
oauth.jsonkeeps only non-secret residue (flow bookkeeping, discovered metadata, public client ids) and acts as the index; acquired access/refresh/ID tokens, IdP session tokens (EMA), and DCR/preregistered client secrets live in the secret store (OS keychain, with the existingsecrets.json/memory fallbacks), joined back on read — store wins over file plaintext.oauth.jsonis migrated lazily on first read, under the file lock, only when the store is durable; under the memory store the file stays authoritative (it is the only durable copy). If moving secrets into the store fails, the strip is aborted — plaintext keeps working rather than losing tokens.MCP_INSPECTOR_PERSIST_TOKENS=all|access|none: write-side policy for which acquired tokens persist.accessdrops refresh tokens (and IdP refresh tokens),nonedrops all acquired tokens; registration client secrets are config, not acquired tokens, and persist regardless. Invalid values warn once and fall back toall.MCP_INSPECTOR_SECRET_STORE=memoryso no test touches a real OS keychain.Docs
docs/secret-storage.md(what counts as a secret, the oauth.json split and migration),docs/environment-variables.md(MCP_INSPECTOR_PERSIST_TOKENS), and the README / Docker / CLI-README security wording now lead with token storage.Out of scope
Read-side staleness (a long-lived client not seeing other clients' writes) is deliberately deferred until the daemon-cli work merges — see #2481.
npm run local:gategreen (format, lint, typecheck, tests, coverage thresholds).