feat(auth): store acquired OAuth tokens in the secret store; clobber-safe oauth.json writes by BobDickinson · Pull Request #2482 · modelcontextprotocol/inspector

BobDickinson · 2026-09-24T05:15:18Z

Two changes to the shared oauth.json persistence, 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 through POST /api/storage/oauth?sections=…; sectioned writes on other stores are rejected. 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>

Copilot

Copilot review overview

🟡 Changes recommended

The current migration, key namespace, and non-durable-store behavior can overwrite or lose credentials and leave keychain secrets undeleted.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 3 · 1

Open (4)

Non-durable store drops unchanged plaintext OAuth secrets · New
OAuth deletion is not serialized with file-locking · New
Colon-containing IDs prevent OAuth credential deletion · New
Custom OAuth store IDs fail sectioned writes · New

What changed in this PR

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.

File	Description
`README.md`	Updates secret-storage warning.
`docs/secret-storage.md`	Documents OAuth secret migration.
`docs/environment-variables.md`	Documents token persistence policy.
`docs/docker.md`	Updates container security guidance.
`core/mcp/remote/node/server.ts`	Adds OAuth-specific storage routes.
`core/auth/remote/storage-remote.ts`	Uses the shared OAuth store ID.
`core/auth/oauth-storage.ts`	Scopes persistence by mutated entries.
`core/auth/oauth-persist.ts`	Defines section merge and transport behavior.
`core/auth/node/storage-node.ts`	Injects secret stores into Node storage.
`core/auth/node/oauth-secrets.ts`	Implements secret splitting and joining.
`core/auth/node/oauth-persist-file.ts`	Adds locking, migration, and secret persistence.
`clients/web/vite.config.ts`	Pins tests to memory secret storage.
`clients/web/src/test/integration/storage/oauth-secret-split.test.ts`	Tests splitting, migration, and policies.
`clients/web/src/test/integration/storage/adapters.test.ts`	Updates persistence adapter coverage.
`clients/web/src/test/integration/mcp/remote/transport.test.ts`	Tests sectioned storage routes.
`clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts`	Verifies joined OAuth reads.
`clients/web/src/test/integration/mcp/inspectorClient-ema-e2e.test.ts`	Verifies EMA secret splitting.
`clients/web/src/test/integration/auth/node/storage.test.ts`	Updates custom-path persistence assertions.
`clients/web/src/test/core/auth/oauth-storage-sections.test.ts`	Tests mutation section descriptors.
`clients/web/src/test/core/auth/oauth-secrets.test.ts`	Tests split/join helpers and policy.
`clients/web/src/test/core/auth/oauth-persist.test.ts`	Tests section parsing and merging.
`clients/web/src/test/core/auth/oauth-persist-file.test.ts`	Tests lock error handling.
`clients/cli/vitest.config.ts`	Prevents keychain use during CLI tests.
`clients/cli/src/cli.ts`	Uses shared joined and sectioned persistence.
`clients/cli/README.md`	Documents shared secret-backed OAuth state.
`clients/cli/__tests__/stored-auth.test.ts`	Updates stored-auth persistence tests.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

…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>

BobDickinson · 2026-09-24T06:06:21Z

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.

Copilot

Copilot review overview

🟡 Changes recommended

Token migration, store isolation, and failed secret deletion can currently lose, leak across profiles, or resurrect credentials.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 4

Open (4)

Prevent stale secrets after failed deletion · New
Preserve allowed access tokens during policy downgrade · New
Preserve persisted tokens during read migration · New
Scope secret IDs by OAuth state path · New

Resolved since last review (4)

Colon-containing IDs prevent OAuth credential deletion
OAuth deletion is not serialized with file-locking
Non-durable store drops unchanged plaintext OAuth secrets
Custom OAuth store IDs fail sectioned writes

…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>

BobDickinson · 2026-09-24T08:26:46Z

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.

Copilot

Copilot review overview

🟡 Changes recommended

Secret-store failure and migration paths can retain, overwrite, orphan, or misreport persisted credentials.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 3

Open (4)

FileSecretStore hides failed confirmed deletions · New
Secret-store failures are misreported as lock failures · New
OAuth secret-store failures return generic 500 errors · New
OAuth store removal masks service-unavailable errors · New

Resolved since last review (4)

Scope secret IDs by OAuth state path
Preserve persisted tokens during read migration
Preserve allowed access tokens during policy downgrade
Prevent stale secrets after failed deletion

+  /**
+   * No-op if no entry exists; throws when the store cannot confirm the
+   * entry is gone (e.g. the keychain is unavailable). Reporting success
+   * for an unconfirmed delete would let callers commit state that assumes
+   * the credential is gone, and a later read would resurrect it.
+   */
  delete(serverId: string, field: string): Promise<void>;
-  /** Remove every secret stored for this server id (called on DELETE /api/servers/:id). */
+  /**
+   * Remove every secret stored for this server id (called on DELETE
+   * /api/servers/:id). Same contract as {@link delete}: throws when the
+   * sweep cannot be confirmed.
+   */
  deleteAllForServer(serverId: string): Promise<void>;


+/** Rethrow the lock's "secrets file" wording as OAuth wording (same file). */
+function rethrowLockError(filePath: string, error: unknown): never {
+  if (error instanceof SecretStoreUnavailableError) {
+    throw new Error(
+      `Could not save OAuth state: the state file at ${filePath} is locked by another Inspector process and did not become available.`,
+      { cause: error },
+    );
+  }
+  throw error;


+            400,
+          );
+        }
+        await writeOAuthSections(filePath, snapshot, sections, secretStore);


      }

+      if (storeId === OAUTH_STORE_ID) {
+        await removeOAuthStore(filePath, secretStore);


BobDickinson and others added 3 commits September 23, 2026 17:03

BobDickinson added the v2 Issues and PRs for v2 label Sep 24, 2026

BobDickinson requested a balanced review from Copilot September 24, 2026 05:15

Copilot started reviewing on behalf of BobDickinson September 24, 2026 05:15 View session

cliffhall linked an issue Sep 24, 2026 that may be closed by this pull request

Store acquired OAuth tokens in the secret store; stop stale-snapshot clients from clobbering oauth.json #2481

Open

Copilot AI reviewed Sep 24, 2026

View reviewed changes

Comment thread core/auth/node/oauth-persist-file.ts

Comment thread core/auth/node/oauth-persist-file.ts Outdated

Comment thread core/auth/node/oauth-secrets.ts Outdated

Comment thread core/auth/remote/storage-remote.ts Outdated

BobDickinson requested a balanced review from Copilot September 24, 2026 06:06

Copilot started reviewing on behalf of BobDickinson September 24, 2026 06:07 View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes

Comment thread core/auth/node/oauth-persist-file.ts Outdated

Comment thread core/auth/node/oauth-persist-file.ts

Comment thread core/auth/node/oauth-persist-file.ts Outdated

Comment thread core/auth/node/oauth-secrets.ts

BobDickinson requested a balanced review from Copilot September 24, 2026 08:26

Copilot started reviewing on behalf of BobDickinson September 24, 2026 08:27 View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes

Navigation Menu

feat(auth): store acquired OAuth tokens in the secret store; clobber-safe oauth.json writes - #2482

feat(auth): store acquired OAuth tokens in the secret store; clobber-safe oauth.json writes#2482
BobDickinson wants to merge 5 commits into
v2/mainfrom
v2/feat/2481-oauth-tokens-secret-store

BobDickinson commented Sep 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BobDickinson commented Sep 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BobDickinson commented Sep 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

BobDickinson commented Sep 24, 2026

1. Clobber-safe writes (cd4ccf9)

2. Tokens and client secrets move to the secret store (8549ef0)

Docs

Out of scope

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BobDickinson commented Sep 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BobDickinson commented Sep 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

1. Clobber-safe writes (`cd4ccf9`)

2. Tokens and client secrets move to the secret store (`8549ef0`)