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
Secret-store ids were derived from the server URL alone, so two OAuth state files (profiles via MCP_INSPECTOR_OAUTH_STATE_PATH) that authorize against the same server shared one entry in any shared backend (OS keychain, shared secrets file) — each profile's save silently overwrote the other's tokens, and removeOAuthStore on one profile deleted the other's credentials.
Fix
Each state file now carries a top-level secretsNamespace UUID, minted and stamped on its first write and baked into every store id the file's entries use: oauth+<ns>+<encoded-url>. The namespace charset excludes + and :, so ids cannot prefix-collide and the keyring's first-colon account parse is unaffected.
Transparent adoption: a pre-namespace file is adopted on its first save — legacy un-namespaced entries are copied under the new namespace, the file is stamped (the commit point), then the legacy originals are deleted best-effort. A copy or stamp failure rolls the copies back and leaves the file legacy, so the next write retries. Nobody is logged out.
Reads never adopt (a legacy file keeps resolving through legacy ids until it writes); removeOAuthStore purges only the file's own ids, never another profile's.
The namespace is node-file-backend-only: the isomorphic OAuthPersistSnapshot type is unchanged (parseOAuthPersistBlob ignores unknown keys), and every writer re-reads it from disk under the file lock, so a snapshot round-tripped through the web API cannot strip it.
Acceptance
Two profiles against one server keep separate tokens — tested on the in-memory backend and on the keyring backend (mocked @napi-rs/keyring, same pattern as secret-store.test.ts): oauth-secrets-namespace.test.ts.
Migration tested: adoption moves legacy entries, deletes the originals, read-back joins; invalid namespaces are ignored; rollback on adoption failure leaves legacy state intact.
Docs updated: docs/secret-storage.md, docs/cli-smoke-testing.md (the isolation recipe is now correct as documented), docs/environment-variables.md.
Coverage: oauth-persist-file.ts 95.5/90.2/97.9/96.5, oauth-secrets.ts ≥98 on all four dims. npm run local:gate green.
Two OAuth state files (profiles) that authorize against the same server
previously shared one secret-store entry, so each profile's save silently
overwrote the other's tokens (#2549).
Each state file now carries a top-level secretsNamespace UUID, minted and
stamped on its first write and baked into every secret-store id the file's
entries use (oauth+<ns>+<encoded-url>). The namespace charset excludes the
'+' delimiter and ':' so ids cannot prefix-collide and the keyring's
first-colon account parse is unaffected.
A pre-namespace file is adopted transparently on its first save: its legacy
un-namespaced entries are copied under the new namespace, the file is
stamped (the commit point), and the legacy originals are deleted
best-effort. A copy or stamp failure rolls the copies back and leaves the
file legacy, so the next write retries. Reads never adopt; removeOAuthStore
purges only the file's own ids.
Closes#2549
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Review follow-up (#2556): the cleanup-failure warning called the surviving
legacy entries "harmless duplicates" while the adoption docblock notes a
pre-namespace profile can still read them as stale credentials; say that
explicitly in both. Narrow the docs' "nobody is logged out" claim to the
adopting file, with the one-time re-auth for other pre-namespace profiles.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Review round 1 (2 low-severity findings, both wording accuracy) addressed in 82343a7:
Cleanup-failure warning: no longer calls surviving legacy entries "harmless duplicates" — now states a pre-namespace profile may read them as stale credentials until it adopts or re-authorizes. Docblock reworded to match.
docs/secret-storage.md: "nobody is logged out" narrowed to the adopting file, with the one-time re-auth for other pre-namespace profiles made explicit.
The two-profile regression test covers writes and reads but not removal, even though deleting one profile's state was one of the reported cross-profile failures and the purge-ID behavior is changed in this PR. Add a case that writes both profiles into one store, removes profile A, and verifies profile B still reads its token and retains its namespaced store entry.
Attempt best-effort cleanup for every legacy credential ID
core/auth/node/oauth-persist-file.ts:404
The try wraps the entire cleanup loop, so the first failed purge prevents every later legacy id from even being attempted. Adoption is already committed and cleanup is never retried, which permanently leaves all subsequent stale credential slots behind. Catch per legacyId so the best-effort cleanup still attempts every entry.
…l test
- Make the secrets namespace participate in write convergence: each save
attempt re-reads the raw blob and, when a concurrent adopter's different
valid namespace is observed on disk (degraded unlocked locking), rolls
this call's earlier store writes back to baseline and re-keys to the
namespace on disk instead of re-stamping its own mint.
- Catch per legacy id in the adoption cleanup loop so one failed purge no
longer abandons the remaining best-effort deletions.
- Remove readDiskForMutation (its one remaining caller now reads raw).
- Tests: namespace-convergence race in oauth-write-convergence.test.ts;
removal isolation and per-id cleanup cases in
oauth-secrets-namespace.test.ts.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
High — converge namespace selection during concurrent first writes: Fixed. The namespace now participates in the convergence loop: each save attempt reads the raw blob, and when a different valid namespace is observed on disk (a concurrent adopter won under degraded locking) the attempt rolls its earlier store writes back to baseline and re-keys to the namespace on disk instead of re-stamping its own mint. New race test in oauth-write-convergence.test.ts. Details in the inline thread.
Medium — regression coverage for removing one profile (previously missed, no thread): Added removing one profile's state purges only its own entries, not the other's to oauth-secrets-namespace.test.ts — writes profiles A and B into one store, removes A, asserts B's namespaced store entry survives and B still reads its tokens back.
Medium — best-effort cleanup for every legacy credential id (previously missed, no thread): Fixed. The adoption cleanup loop now catches per legacyId, so one failed purge no longer abandons the remaining deletions. Covered by a new test: the server purge throws, and the idp session's legacy original is still purged while the failure is warned, not thrown.
Also removed readDiskForMutation — its one remaining caller now reads the raw blob itself for the namespace check.
This test targets core/auth/node/oauth-persist-file.ts, so the repository's integration-test placement rule requires mirroring that source path under src/test/integration/auth/node/. Keeping the new file under integration/storage/ associates it with core/storage and perpetuates the older misplaced layout; move it to clients/web/src/test/integration/auth/node/oauth-secrets-namespace.test.ts (the folder glob will discover it automatically).
Review round 3 (#2556): two adopters of the same legacy state file
under degraded locking could race the migration — one copies and
deletes the legacy entries while the other strict-reads null, stamps
its own namespace, and strands the first adopter's copies.
withSecretFileLock now tells its callback whether the lock is actually
held, withOAuthStateLock threads that through, and adoptSecretsNamespace
refuses a legacy migration (snapshot present, lock not held) with a
retryable SecretStoreUnavailableError before touching the file or the
store. Locked adopters serialize; mint-only adoption of a fresh file and
saves against an already-stamped file stay allowed unlocked, where the
write-convergence re-key already handles namespace divergence.
Also moves oauth-secrets-namespace.test.ts to integration/auth/node/ to
mirror the source path (review round 3, previously-missed finding).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
High — legacy adoption races under degraded locking (inline, oauth-persist-file.ts): Fixed, via the first suggested remedy. Legacy migration is now gated on actually holding the file lock: withSecretFileLock reports to its callback whether the lock is held, and adoptSecretsNamespace throws a retryable SecretStoreUnavailableError when a pre-namespace file would need migrating without it — before any move, stamp, or store write. Locked adopters serialize (the second adopts the first's stamp); degraded mode refuses loudly; mint-only adoption of a fresh file and saves against an already-stamped file remain allowed unlocked, with namespace divergence still converging through the round-2 re-key. New tests: integration/auth/node/oauth-adoption-locking.test.ts (three unlocked cases) plus locked-argument assertions in file-lock.test.ts. docs/secret-storage.md notes the lock requirement.
Low — test placement (previously missed, no thread): Fixed — oauth-secrets-namespace.test.ts moved to clients/web/src/test/integration/auth/node/, mirroring core/auth/node/. (The older oauth persist suites under integration/storage/ predate that convention; moving them is out-of-scope churn for this PR.)
…antics
Review round 4 (#2556):
- An entry-less legacy file ({servers:{},idpSessions:{}}) indexes no
store ids, so no destructive migration race exists — the degraded-lock
gate now sits after the moves are computed and an empty move list
mints like a fresh file, so lock-hostile filesystems can still save.
- Pin the deliberate legacy-removal semantics with a test and a docs
clause: removing a still-pre-namespace profile purges the shared
legacy ids, since the file being deleted is the store's only index
of them — skipping the purge would strand credentials.
- Two-profile isolation now also runs against the real FileSecretStore
(nested locking, serialized whole-file mutations).
- Cover the commit-point failure: a stamp write that rejects after the
scoped copies landed rolls the copies back and leaves the legacy file
and ids authoritative.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
High — legacy removal purges shared legacy ids (oauth-persist-file.ts): Declined as a behavior change; narrowed, documented, and covered instead. The purge is deliberate: removeOAuthStore deletes the state file, which is the store's only index of the legacy entries — skipping the purge would strand credentials in the shared store with nothing able to find or clear them. Another still-legacy profile re-authorizes once, the same cost it pays when a sibling adopts. Isolation on removal is a property of stamped files (already tested); a new test pins the legacy semantics with the rationale, and docs/secret-storage.md now states it.
Medium — entry-less legacy file refused under degraded lock:Fixed. The gate now sits after the move list is computed; an empty move list mints like a fresh file, locked or not. New degraded-lock test covers it.
Low — FileSecretStore isolation coverage:Fixed. The two-profile scenario now also runs against the real FileSecretStore.
Low — commit-point rollback coverage:Fixed. New test makes the stamp write reject after the scoped copies landed and asserts the copies are rolled back while the legacy file and ids stay authoritative.
JSON.parse returns any, so this newly added destructuring bypasses type checking before the namespace is passed into the ID builder. Cast the parsed value to the expected object shape, as the other new namespace reads in this PR do.
Review round 5 (#2556): JSON.parse returns any, so the destructured
secretsNamespace bypassed type checking before reaching the id builder.
Cast to the expected shape like the PR's other namespace reads.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Copied state files share a namespace (inline): Declined as out of scope — a copied oauth.json duplicating its credential references is deliberate user action, and the suggested path-binding remedies break on moves/renames/symlinks, where a spurious re-key strands credentials (strictly worse than what it prevents). #2549 is about independently created files colliding, which the UUID fixes. Full rationale in the thread.
Previously missed — untyped JSON.parse destructuring in adapters.test.ts:643:Fixed in d211178 — cast to the expected shape like the PR's other namespace reads.
Review round 6 (#2556): the re-key's restoreToBaseline was best-effort,
so a failed restore still cleared the baseline, switched namespaces, and
could let the save report success with earlier attempts' writes stranded
under the abandoned namespace, unindexed by any file. Unlike the failure
exits there is no original error to preserve here, so the re-key restore
is now strict: the first restore failure aborts the attempt, keeping the
baseline for the rethrow's reconciliation, and a retried save converges
cleanly. New convergence test refuses the unwind once and asserts the
save fails loudly with nothing stranded.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Bob Dickinson <bob.dickinson@gmail.com>
Re-key continues past a failed baseline restore (inline, oauth-persist-file.ts): Fixed. The namespace re-key's restore is now strict — any restore failure aborts the attempt (no cleared baseline, no namespace switch, no false success); the failure funnels into the existing reconciliation and a retried save converges. Covered by a new convergence test.
Copilot review loop closed: round 7 posted no findings (its headline recommends a final human pass over the migration/degraded-lock concurrency behavior, which is maintainer review, not a defect). Six substantive rounds total — fixes in 82343a7, cfd93f5, 49dcf99, d6da6d1, d211178 and 3e4b4ec, with each round's findings answered in its threads and summaries. Ready for maintainer review and merge.
The reason will be displayed to describe this comment to others. Learn more.
Verdict: ✅ Mergeable — no blockers
This fixes #2549 as specified, and every acceptance box holds:
Two profiles against one server keep separate tokens, tested on the in-memory, keyring and file backends.
Migration is tested, including rollback at both the copy step and the stamp step.
The smoke-testing isolation recipe is now accurate.
CI is green (build, coverage). All seven authored commits are signed off. Closes #2549 is the first line, and the card is at In Review on v2.10.0. Copilot's last round (on 3e4b4ec3) had no findings but asked for human review of the credential migration and the degraded-lock behavior. Those are what I focused on, and both are covered below.
What I verified (head 4517108c)
The tests pass locally:
unit oauth-persist-file + oauth-secrets: 41/41
integration auth/node/** + storage/**: 475/475 across 14 files
cli stored-auth: 47/47
The id grammar can't collide.
A legacy id is oauth+<enc> with exactly one +, because encodeURIComponent turns + into %2B. A namespaced id is oauth+<ns>+<enc>, and the namespace charset excludes + and :. So a namespaced id never equals a legacy id, no two (namespace, url) pairs produce the same id, and the keyring's first-colon account parse is unaffected.
The regex also bounds the namespace (128 chars), so a hand-edited value can't create an oversized keychain account.
Every OAuth secret-id call site in core was updated.oauthSecretServerId / oauthIdpSecretServerId are used only in oauth-persist-file.ts, and every use there passes the namespace (or undefined on purpose for the legacy id). The deleteAllForServer calls in core/mcp/remote/node/server.ts use mcp.json server-secret ids and aren't affected.
Adoption order holds up. It copies, then stamps (the commit point), then deletes. Copies land on ids that are vacant by construction, so rollback is just "delete what was copied". A failed stamp rolls back too, which prevents stranding the copies under a UUID nothing references. Cleanup is attempted once per id.
The other paths agree:
Reads never adopt.
The plaintext migration keeps the file's existing namespace and never mints one.
removeOAuthStore purges only the ids this file uses.
The unlocked re-key in writeOAuthSections rolls earlier attempts back strictly before switching to the namespace found on disk.
A fresh smoke-test state file never touches legacy entries. Absent file → mint only, so the new cli-smoke-testing.md paragraph is accurate.
Recommended (non-blocking — docs or follow-up issues, not changes to this PR's logic)
1. Mixed versions or a downgrade on one state file: the user is logged out, and credentials are orphaned. After adoption the legacy ids are deleted. An older Inspector (≤ 2.9.x, e.g. a pinned npx …@2.9) reading the same oauth.json resolves only legacy ids, so it sees no tokens. Its next save then removes the stamp: the old parseOAuthPersistBlob → serializeOAuthPersistBlob round trip drops unknown keys. The next new-version save re-adopts under a fresh UUID (the scenario the "re-adopts … when the stamp is stripped" test covers). That leaves the first namespace's entries in the keychain with no state file referencing them. They are real, possibly still-valid refresh tokens that nothing will ever find or clear. Using one state file across different Inspector versions is plausible, so I'd at least add a sentence to docs/secret-storage.md: downgrading after adoption logs you out, and alternating versions on one state file leaves orphaned entries. A cleanup mechanism, if wanted, belongs in its own issue.
2. On a box where the lock is unavailable, a legacy file with entries can never be saved again. On the boxes openSecretFileLock deliberately degrades for (#1848/#1905: a mount owned by another uid, a filesystem without mkdir semantics), the !locked refusal fires on every save, not just once. Every save re-checks: no namespace, entries present, unlocked → throw. So logins and token-refresh persistence fail permanently on exactly the boxes the degraded path exists to serve. I think refusing is the right call, for the race the comment describes. But the error offers only "make the lock directory writable", which those users may be unable to do. There is an escape that works: clearing stored auth (removeOAuthStore doesn't adopt), after which the fresh file mints unlocked. It belongs in the message and the doc (inline below).
Nits
file-lock.ts: the new paragraph about fn receiving locked was added to withSecretFileLock's JSDoc, but that block sits above openSecretFileLock's JSDoc rather than above withSecretFileLock. The misplacement predates this PR, but the new contract won't show on hover for the function it describes (inline below).
Each save now does one extra readStoreFile in adoptSecretsNamespace before the attempt loop reads the file again. It's negligible, and I'm only mentioning it so it's a known cost.
The branch's merge commit 4517108c (the "Update branch" merge) has no Signed-off-by. Every authored commit is signed, and no DCO check shows in this PR's check rollup, so I can't confirm whether it would be flagged. Worth checking if the DCO gate counts merge commits.
The reason will be displayed to describe this comment to others. Learn more.
Recommended (summary #2). This refusal is re-evaluated on every save, so on a box where the lock is unavailable (the #1848/#1905 class that openSecretFileLock degrades for), a legacy file with entries can never be saved again. Refusing is right, but the message's only remedy, "make the lock directory writable", may be impossible there. Suggest also naming the escape that does work: clear this file's stored OAuth state (remove doesn't adopt) and re-authorize, after which the fresh file mints its namespace unlocked. The same sentence would fit in the adoption paragraph of docs/secret-storage.md.
The reason will be displayed to describe this comment to others. Learn more.
Recommended (summary #1). This early return is the only thing preventing a second adoption, and the stamp it reads doesn't survive a save by an Inspector older than this PR: the old parse → serialize round trip drops unknown keys. After a downgrade, or when two versions alternate on one oauth.json, the old version sees no tokens (the legacy ids were deleted at adoption), and the next new-version save re-adopts under a fresh UUID. That orphans the previous namespace's entries in the keychain, where no state file will ever purge them. I'm not asking for a logic change here, but this deserves a line in docs/secret-storage.md, plus a follow-up issue if cleaning up orphans is wanted.
The reason will be displayed to describe this comment to others. Learn more.
Nit: this JSDoc block documents withSecretFileLock, but it sits directly above openSecretFileLock's own JSDoc, so neither the existing text nor the new locked contract shows on hover for withSecretFileLock (line 520). The misplacement predates this PR, but moving the block down to the function it describes is a cheap fix now that it carries a new contract.
This branch has not been deployed
No deployments
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.
cliffhall
left a comment
Closes #2549
Problem
Secret-store ids were derived from the server URL alone, so two OAuth state files (profiles via
MCP_INSPECTOR_OAUTH_STATE_PATH) that authorize against the same server shared one entry in any shared backend (OS keychain, shared secrets file) — each profile's save silently overwrote the other's tokens, andremoveOAuthStoreon one profile deleted the other's credentials.Fix
secretsNamespaceUUID, minted and stamped on its first write and baked into every store id the file's entries use:oauth+<ns>+<encoded-url>. The namespace charset excludes+and:, so ids cannot prefix-collide and the keyring's first-colon account parse is unaffected.removeOAuthStorepurges only the file's own ids, never another profile's.OAuthPersistSnapshottype is unchanged (parseOAuthPersistBlobignores unknown keys), and every writer re-reads it from disk under the file lock, so a snapshot round-tripped through the web API cannot strip it.Acceptance
@napi-rs/keyring, same pattern assecret-store.test.ts):oauth-secrets-namespace.test.ts.docs/secret-storage.md,docs/cli-smoke-testing.md(the isolation recipe is now correct as documented),docs/environment-variables.md.Coverage:
oauth-persist-file.ts95.5/90.2/97.9/96.5,oauth-secrets.ts≥98 on all four dims.npm run local:gategreen.