Copilot round 3, four items:
- the snapshot "attempt token" was the stored serialization, which is not
unique: the snapshot records neither the authorization URL nor its
`state`, so two concurrent redirects for one server with unchanged UI
serialize identically. `OAuthResumeSnapshot` now carries an optional
`attemptId`, stamped per write and matched on clear. Optional because
an older build's snapshot, mid-redirect across an upgrade, has none —
and treating that as a match would strand a live callback.
- the resume-failure toast promised a retry even when the session had
ended and nothing was restored. Copy is now chosen by whether anything
is left pending; `authRecoveryAbandonedMessage` covers the other case.
- a connect-scoped redirect that failed left the connection at
"connecting" with the active-server lock held, since the redirect was
what would have ended the attempt. It now disconnects first, as the
sibling arm in `useConnectionLifecycle` does — scoped to that context,
because an ambient or step-up failure does not mean the session is dead.
- both terminal EMA step-up failure arms now drop the stored retry. A
later step-up does not overwrite the ref, so a retained operation could
be re-run by an unrelated authorization. The `interactive` arm keeps
it: that one is still on its way to a redirect.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY
Signed-off-by: cliffhall <cliff@futurescale.com>
Each of the five sites in `useOAuthRecovery` discarded a promise with
`void`, satisfying `@typescript-eslint/no-floating-promises` but not the
narrower rule AGENTS.md states — `void` only where the callee owns its
failures or the caller cannot await, with a comment saying which.
Each now picks a user-visible outcome:
- `prepareOAuthRedirect` clears the resume snapshot (so the next load
cannot read a redirect that never started as an *abandoned* one),
flags the server and raises the re-auth banner.
- the `authChallengeInteractive` handler flags and banners, where before
a rejection drew no UI response at all.
- `resumePendingReauth` restores the pending slot and toasts, so the
next trigger retries the deferred recovery instead of losing it.
- the `/oauth/callback` effect flags and banners what falls between its
existing arms — the client rebuild and the post-resume scope check.
- `handleStepUpAuthorize`'s EMA branch reports exactly as its `failed`
outcome arm does; the stored retry gets its own catch, since by then
the permissions leg succeeded and the failure is the command's.
The two surviving `void`s carry the justification AGENTS.md asks for.
Closes#2165
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY
Signed-off-by: cliffhall <cliff@futurescale.com>
The CI coverage job failed with `ReferenceError: sessionStorage is not defined`.
`revocation-e2e.test.ts` used `BrowserOAuthStorage`, but the `integration`
vitest project runs in the node environment, not happy-dom. Node 22 gates Web
Storage behind a flag while newer Node exposes it by default, so it passed
locally and failed in CI. It uses `NodeOAuthStorage` with a temp dir now,
matching the other OAuth integration tests, with a comment naming the trap.
Also from Copilot review round 21: `listIssuers` and `getIssuerTokens` became
dead production API when `takeRevocationSnapshot` took over — a grep finds them
only in their own implementations and tests. Keeping them would widen the
exported `OAuthStorage` contract and force every implementation and mock to
carry methods nothing calls. Removed, along with their tests and the mock
entries in four suites.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot reported "no new comments" again, with four real findings in its
suppressed block.
- The web in-flight lock was global, so a pending clear for server A silently
swallowed a click on server B's — even though the callback explicitly
supports clearing a non-active server. Keyed by server id now.
- `else if (!isActive)` skipped the resume-snapshot cleanup when the target IS
active but has no client yet, which is reachable while a session is being
built or torn down. The clear then emptied storage and left stale OAuth
recovery state pointing at credentials that no longer existed.
- `revocationAuthMethods` returned `[]` for an omitted
`revocation_endpoint_auth_methods_supported`, which does not enforce RFC
8414's default: an empty list means "the metadata said nothing" to
`selectClientAuthMethod`, which then honors the registration's own
`token_endpoint_auth_method`. The default is named literally now.
- The `v8 ignore` on the CLI budget branch was wrong, and the challenge to it
was right: an injected `budgetMs` of zero reaches it directly, through a seam
I had added myself the round before. Removed, and covered by a test that
asserts no request is made and the unattempted key is reported.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 19.
- Parsing the preconfigured client registration sat outside the per-slot
salvage, so one malformed fallback aborted the whole plan and grants carrying
their own valid credentials were never revoked. It is a *fallback*; a bad one
is now recorded and skipped, matching every other slot.
- `sendPlans` preserved an earlier success when a later key was never
attempted, contradicting the failure-first rule two lines below — the
unattempted key's grant may still be live. That branch turns out to be a
bound rather than a path (`withDeadline` fails any plan that reaches the
deadline, so a preceding plan cannot both succeed and leave zero budget), so
it carries a justified `v8 ignore` saying exactly that.
- `clearStoredAuthForRelogin` takes an optional `budgetMs`, which is what lets
the shared-budget behavior be tested without a five-second test.
- Three stale references to `planOAuthRevocation`, which no longer exists —
two JSDoc links and the `AGENTS.md` entry, which additionally still told
callers to make a separate `storage.clear()` call. Following that text would
have reintroduced the second clear the atomic step exists to remove.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 18. Three of its findings were the same check-then-act:
`planOAuthRevocation` is asynchronous, so the gap between its reads and the
following `storage.clear()` is still a window — shorter than the network one
closed in round 13, but the same shape. An OAuth completion landing at any of
those awaits saved a grant the clear then destroyed.
`OAuthStorage.takeRevocationSnapshot(serverUrl)` reads what revocation needs
and clears the server in ONE synchronous pass over the in-memory state, with
the persist after the mutation. `clearAndPlanRevocation` replaces
`planOAuthRevocation`, and there is no separate `clear` call left in any
caller — the clear cannot be forgotten or reordered.
The snapshot returns values unparsed, deliberately: schema validation is pure
and belongs after the mutation, since running it inside would reintroduce the
`await` this exists to remove.
Two consequences worth naming. A failure of the take-and-clear now REJECTS
rather than returning an outcome, because the state was not cleared and the
caller's contract is that it was; both clients already have a rejection path.
And cross-process atomicity is explicitly not claimed — nothing in this store
takes a lock, every mutation is a read-modify-write over a loaded snapshot, and
`clear` alone always had that property.
Also from round 18:
- The CLI's budget check ran before considering `plan.outcome`, so a key that
needed no network was reported as budget-exhausted — warning that a grant may
be live when that key held none.
- A disconnect failure after a successful clear was reported as "could not
clear OAuth state" in both clients. It now goes to the disconnect channel,
and the success is still reported.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 17.
`resolveClientInformation` mirrored `BaseOAuthClientProvider`, preferring the
preregistered entry. That order answers "who should I authenticate as now";
revocation asks "who minted this token", and the two diverge — the store lets a
preregistered client and an issuer-bound dynamic registration coexist, so after
a server is switched from DCR to a configured `oauth.clientId` an older DCR
grant was revoked with the configured client's credentials. RFC 7009 §2.2
answers 200 for a token the server does not recognise as the caller's, so that
reported `revoked` while the grant stayed live and the local record was already
gone — the worst available outcome.
The registration bound to the grant's issuer now wins, with the preregistered
entry as the fallback, which covers both directions: a token minted with the
configured client leaves the issuer slot empty, since the SDK writes it only
after DCR.
Stated in the comment rather than implied: this is best available evidence, not
proof. The store records client information per issuer, not per token, so
binding identity at save time remains the real fix and is a storage-shape
change beyond this PR.
Also fixed a CLI test comment still describing the pre-round-13 ordering.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 15.
The issuer check still failed open for the legacy unkeyed grant. "Unkeyed"
means its authorization server is UNKNOWN, not that the cached endpoint is
proven to own it: server metadata is a single slot a later discovery
overwrites, while a pre-SEP-2352 token survives until the first issuer-stamped
save. So a clear could post an old bearer credential to an authorization server
that never minted it.
Such a grant is now cleared without being revoked, and reported as such with a
message saying re-authorizing makes future clears revocable. The cost is that
a pre-issuer-binding entry gets no revocation — which is exactly the behavior
it had before this feature existed, so nothing regresses — and it earns
revocation back on the next authorization. Not sending is the only answer that
cannot be wrong.
Every seed in the suite is now issuer-bound, which is what a real flow writes;
the two tests that were about the legacy grant now assert the refusal, since
that is the behavior worth pinning.
Also renamed the CLI test that still described the pre-round-13 ordering.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 14. Two code fixes and a documentation sweep.
- `buildRevocationRequest` ran outside `revokeToken`'s best-effort try.
`encodeURIComponent` throws on a lone UTF-16 surrogate — valid JSON, so it
can reach there from a persisted client id or secret — and every caller has
already cleared its local state by then, so a rejection instead of a `failed`
outcome would break the documented guarantee. Construction is inside the try
now, and the catch reports `params.endpoint` rather than a `url` that may not
exist on that path.
- The fixture's Basic decode turned a malformed percent escape into an Express
500, reporting a bad credential as a server fault rather than as the
`invalid_client` 401 the endpoint means. Decoding failure is failed
authentication.
- Ten places still said revocation happens *before* the clear, which the
reorder in the previous commit made false: the settings-form description, the
`--no-revoke` help text, both READMEs, `docs/mcp-server-configuration.md`,
three JSDoc blocks, and the PR body. Swept, and the ordering is stated where
it is load-bearing rather than asserted in passing. Also merged the two
adjacent JSDoc blocks on `InspectorClient.clearOAuthTokens`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 13. Three of its four findings were one root cause, and
it is a design flaw in the premise this PR started from.
Revoking *before* the clear meant the clear waited up to five seconds on the
network — and anything that wrote OAuth state in that window was then deleted
by a clear reasoning about the grant it replaced. A same-server reauthorization
in the TUI (whose Disconnect/Connect accelerators stay live), a stored-only web
clear for a server that becomes active mid-flight, and another CLI/TUI process
writing to the shared file-backed store all hit it.
`revokeStoredOAuthTokens` is therefore split into `planOAuthRevocation` and
`executeOAuthRevocation`, and every caller runs plan → `storage.clear()` →
execute. The snapshot still precedes the clear, because the store is where the
token, the credentials and the endpoint live; but the clear no longer waits on
anything. There is deliberately no single-call wrapper left, so no caller can
reintroduce the racy order by accident.
Fourth finding, same file: `handleConnect` now retires `clearOAuthAttemptRef`
alongside `disconnectAttemptRef`. The server-name check does not cover
disconnect/reconnect to the SAME server — the name still matches on the other
side of it — so a clear still in flight could tear down the session the connect
had just established.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review rounds 8 and 9.
- A cached metadata document with no `issuer` was letting every issuer-bound
token go to its single revocation endpoint. Absence establishes nothing, so
that is now a mismatch: an issuer-bound grant must match, and only a legacy
unkeyed grant — which nothing binds to a different authorization server —
proceeds without the comparison. This was a credential-disclosure risk, not
just a correctness one.
- The ctx-less grant read was skipped whenever it carried an issuer stamp, on
the assumption that slot had already been enumerated. If `listIssuers` fails
there is no enumerated slot behind the stamp, so the one readable grant went
unrevoked while `clear` deleted it. Both reads now share one issuer+token
key, and the stamped issuer is retained rather than discarded.
- `AuthTab`'s repeat guard read `clearState`, which is not updated until React
re-renders, so two `s` events in one input turn both started a clear against
the same store entry. It is a ref now, set before the call.
- A rejected clear left `ownClearRef` set — `handleClearOAuth` never reached
its `oauthRevision` bump — so the next unrelated revision was swallowed and
the failure banner stranded. Cleared on that path.
- The web `clearServerOAuthAndDisconnect` had the staleness the TUI path was
already fixed for: `isActive`/`inspectorClient` are captured before a
five-second await, so a switch during it disconnected the session the user
had just moved to and ran the session-wide cleanup against it. Revalidated
against `sessionRef` after each await.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 7.
- The five-second timeout was per request while grants are revoked
sequentially, so a server with N issuer slots could block the disconnect for
N x 5s — the "short timeout" bounded a request rather than anything the user
feels. It is now one budget for the whole call: each grant gets the remaining
time, and a grant reached after it is exhausted is reported as never
attempted rather than starting a fresh five seconds. `--relogin` shares one
budget across its two key spellings for the same reason.
- `AuthTab` called the same settle handler on rejection, so a failed local
clear or disconnect announced "OAuth state cleared". A rejection is not a
revocation failure — those come back as outcomes — so it now has its own
path and reports the real error. The completion is also scoped: a clear
carries the server it started on and an attempt number, and a switch retires
it, so server A's confirmation cannot appear under B.
- `clearOAuthAttemptRef` is now retired by the server-switch effect alongside
`disconnectAttemptRef`; without that an A -> B -> A round trip left both the
token and the captured name matching again. `handleClearOAuth` also
revalidates after `await disconnectInspector()`, since a switch during that
second await would have put the revision bump on the new selection.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 6.
- The Basic credential is now form-urlencoded per RFC 6749 §2.3.1. Round 1
matched the SDK's raw `id:secret` on the theory that a credential should be
presented the same way here as at the token endpoint; that does not hold — a
client authenticated with `client_secret_post` never exercised the SDK's
Basic path, so there is no precedent to match, and the raw form is ambiguous
for an id containing `:` and makes `btoa` throw on a non-Latin-1 secret. The
fixture decodes each half.
- The ctx-less grant read is suppressed on the store's issuer *stamp* rather
than on the token's value. The active slot can hold client information
without a token, in which case the read falls back to the legacy grant — and
a value collision with any other issuer would have dropped that grant
unrevoked and unreported.
- The TUI announced "OAuth state cleared" on the keypress, so with a bounded
revocation request it said so while the work was still in flight and then
removed it when the work finished. `onClearOAuth` returns a promise;
`AuthTab` shows "Clearing OAuth state…" until it settles, ignores repeats
while one is in flight, and skips exactly the revision bump its own clear
caused so the confirmation is not immediately reset by it.
- `handleClearOAuth` now claims an attempt and retires a superseded one, the
way `handleDisconnect` does: the await can keep it suspended for seconds,
long enough for the user to switch servers and have server A's outcome —
including its disconnect — land on server B.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 5.
- Deduplicating grants by token string alone could conflate two grants: a token
is only meaningful to the authorization server that minted it, so two issuers
minting the same opaque value are two grants. The second was dropped before
the issuer-mismatch check could report it, and `clear` then deleted it. The
key is now issuer+token; the ctx-less read stays suppressed only when its
token already came from a slot, which is exactly the active-issuer duplicate
and never the legacy one.
- A read failure in one issuer slot aborted `collectGrants`, so one corrupt
slot left every other revocable grant untouched while the clear deleted them
all. Each slot is now read independently and its failure carried as an
outcome alongside the grants that could still be revoked; the no-metadata and
no-endpoint returns carry them too, so a failure is never swallowed by a skip.
- The CLI no longer deduplicates its two key spellings. That could only be done
by pre-reading a single token, and `revokeStoredOAuthTokens` now enumerates
every issuer slot under a key — so a shared active token skipped the second
key entirely, taking any other issuer-bound grant under it with the local
delete. A duplicate RFC 7009 request is harmless (§2.2 makes an unknown token
a success); a missed one is the leak.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4.
- `getTokens(url, issuer)` falls back to the legacy unkeyed slot when that
issuer holds none — right for a connect, wrong for `collectGrants`, which
labelled the fallback with the issuer it happened to be iterating and paired
it with that issuer's credentials. During a partially migrated flow the clear
could have sent an old, unbound token to a newly discovered authorization
server. `OAuthStorage.getIssuerTokens` is the exact, no-fallback read; only
the ctx-less read still falls back, which is the one place it belongs.
- The TUI's warning tone was sticky. It lived in its own state while ~30 call
sites set ordinary OAuth notes without resetting it, so one revocation
failure left every later "Authorization updated" yellow, across server
switches. It is now DERIVED — `oauthMessageToneFor(message, warningText)` —
so it cannot outlive the message that earned it, with nothing to remember to
clear. Both setters stay plain `useState` setters: a `useCallback` wrapper
would be a value `react-hooks/exhaustive-deps` demands in seven dependency
arrays for an identity that never changes.
- Hook-level tests for the web wiring (previously missed): the per-server
opt-out and the backend-proxied fetch are now asserted to reach
`clearServerOAuthState`, so handing it the page-origin fetch — which a real
authorization server's missing CORS headers would reject — fails a test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3.
`clear(serverUrl)` deletes EVERY `byIssuer` slot, but revocation read only the
context-free (active-issuer) token. A server that had authorized against
issuers A and B therefore revoked B, then destroyed the local record of A while
A's grant stayed live at its authorization server — the same leak this feature
exists to close, one level down.
- `OAuthStorage.listIssuers(serverUrl)` is the new seam (one implementation:
every backend extends `OAuthStorageBase`). `collectGrants` walks it plus the
ctx-less read, deduping by token, so every grant the clear will delete is
covered.
- Metadata is cached once per server, not per issuer, so a grant bound to an
issuer the cached document does not describe cannot be revoked: sending its
token to that endpoint would hand a credential to a server that never minted
it. That grant is reported as failed — dropped unrevoked — rather than
silently discarded or misdirected.
- `aggregateOutcomes` gives a failure precedence over another grant's success,
so one live grant cannot hide behind another's 200. Exported for its own
test, since `computeOutcome` returns early on the empty case.
Also from that round:
- The fixture's /oauth/revoke now verifies token ownership (RFC 7009 §2.1).
Access tokens record their owning `client_id`; a token belonging to another
client is left alone and still answered 200 (§2.2 — the response must not
tell one client whether another's token exists). New e2e case proves a
second registered client cannot revoke the first's grant.
- The TUI rendered the revocation-failure note in cyan, the informational
tone. It is a partial success — the local clear really happened — so it is
not an `error` status, but the grant may still be live and cyan understated
that. `AuthTab` takes an `oauthMessageTone`; the message says so explicitly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Three review findings.
Probe responses the loop skips — and the original failed response the
substitution replaces — were dropped without releasing their bodies. On
Node/undici that holds the connection, and this loop can run on every OAuth
attempt, so repeated discovery against a 4xx candidate could exhaust the
origin's pool. Both paths now cancel, the same discipline
`core/mcp/node/authChallengeFetch.ts` uses for a discarded 401.
`fetch` follows redirects, so the candidate URL is where the probe was aimed,
not necessarily where the document came from. The source header and the console
warning now report `probe.url`, falling back to the candidate for a synthesized
response that carries none.
The CLI's stored-token refresh built its fetch from the global rather than from
`createProxyFetch()`, so a server reachable only through `HTTPS_PROXY` was
probed directly. It now builds one proxy-aware fetch and hands it to both the
discovery probe and `refreshAuthorization`, so neither leg bypasses the proxy —
this function runs outside `InspectorClient`, so nothing else puts a proxy
under it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
The probe loop advanced to the next OIDC candidate on any failure, which meant
it could promote a later candidate over an earlier one the SDK would have
stopped at — turning a failure into a success, a worse defect than the one
being worked around. Four distinct cases: a 500 (an outage the SDK surfaces), a
2xx body that will not parse (terminal for the SDK), a network error (which the
SDK propagates outside the browser's CORS case), and a non-JSON media type.
It now continues only where the SDK's own loop does — a 4xx or 502 — and
otherwise returns the original response, leaving the SDK to make the same
request and reach its own verdict. The `content-type` gate is gone entirely,
because the SDK parses a 2xx discovery body whatever media type it carries, so
gating could skip a genuine OpenID provider document and substitute an
RFC-8414-only one from a lower-priority candidate in its place.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Two more review findings, both real.
The shim wrapped `effectiveAuthFetch` only, but the transport is constructed
with the base `this.fetchFn`, and the SDK runs its own discovery from inside
the transport on the 401/refresh leg — so a reconnect with an existing auth
provider could still hit the upstream failure. It now wraps the base fetch,
the same seam `withOAuthEndpointOverrides` uses and the only one that reaches
both paths.
That trades away the previous claim that a captured Network entry shows the
real 404: below the trackers, what they record is the substitution. Rather
than leave that silently misleading, a substituted response now carries
`x-inspector-oauth-metadata-source` naming the URL its body was actually
fetched from — the transport builds its tracker internally, so there is no
seam above it to install into instead. An integration test captures the fetch
the transport is handed and drives it against a real server, since nothing
else proves that wiring.
The wrapper also keyed on URL shape alone, but nothing stops an authorization
or token endpoint from living under `/.well-known/oauth-authorization-server/`;
a POST there returning an ordinary OAuth 4xx would have had its error replaced
by a metadata document. Metadata discovery is a GET, so anything else is now
left alone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
- RFC 8414 §2 defaults an omitted `revocation_endpoint_auth_methods_supported`
to `client_secret_basic`; it does not inherit the token endpoint's list.
Inheriting made metadata advertising only `client_secret_post` there send
POST credentials to an endpoint that never advertised the method.
- The 5s timeout was inert on the web paths: `createRemoteFetch` re-issues the
call as a POST to `/api/fetch` and drops `init.signal`, and the backend's
outbound fetch gets none either, so a wedged AS could hold the teardown open
indefinitely. A wall-clock race now enforces the deadline; the signal is kept
because it genuinely cancels the direct-fetch paths.
- The settings-modal Clear read `settingsModalTarget`, which comes from the
persisted `servers` list, while edits live in `settingsDraft` until the save
debounce. Toggling the checkbox and clearing immediately used the previous
value. It now merges the draft.
- `--relogin` clears both key spellings, so it now revokes from both rather
than stopping at the first populated one — a stale entry under the other
spelling is a live grant. The normalised key goes first, matching
`findStoredServerState`, and a second key holding the same token is skipped.
- The fixture's /oauth/revoke now ENFORCES RFC 7009 §2.1 client
authentication (both RFC 6749 §2.3.1 forms), so the e2e's claim that the
Inspector authenticates correctly is actually tested; two negative cases
assert a wrong secret and an unidentified client are refused.
- Dropped the `as unknown as` casts from the tests: a typed `InspectorLogger`
double, and a spy on the real storage instead of a spread-and-cast.
- Added CLI-level tests through `runCli` for `--no-revoke`, the per-server
`oauth.revokeOnClear`, and the stderr warning, and TUI tests for the
forwarded opt-out and the rendered failure.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
Two review findings.
The stored-token refresh path (`refreshStoredAuthToken`) calls SDK discovery
directly rather than through `InspectorClient.effectiveAuthFetch`, so a state
file holding a refresh token and client information but no `serverMetadata`
still hit the upstream OIDC-schema failure and could not refresh against the
very servers this change is for. Its default `discover` now wraps the global
fetch with the compat shim; a caller-supplied `fetchFn` is left alone. Covered
by a test that injects no `discover` — injecting one would bypass the default
under test — against a real server that 404s the RFC 8414 path and serves plain
RFC 8414 metadata at the appended OIDC path.
`oidcDiscoveryCandidates` matched the well-known path by bare prefix, so it
also claimed `/.well-known/oauth-authorization-server-backup` and would have
replaced that path's failed response with a document fetched from a derived
URL. It now requires the exact path or a `/` boundary, which is precisely the
set `buildDiscoveryUrls` can emit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Clearing the Inspector's OAuth state deleted the local copy of the tokens
and stopped there. The access token — and the refresh token, which is
long-lived by design — stayed valid at the authorization server until they
expired on their own, so a day of connect/disconnect iteration left it
holding grants for sessions that ended hours ago. RFC 7009 §1 describes
exactly that case, and nothing in the repo constructed a revocation
request, so a server author implementing `revocation_endpoint` had no way
to exercise it with the tool that exists to exercise this.
core/auth/revocation.ts builds and sends the request; the three clear paths
call it, so the behavior is written once — web's "Clear OAuth state and
disconnect" (both the active and stored-only paths), the TUI's Auth tab,
and the CLI's --relogin. It runs BEFORE the local clear, because the token,
the client credentials and the discovered `revocation_endpoint` all live in
the store the clear is about to empty. The request names the refresh token
when there is one: §2.1 asks the AS to also invalidate the access tokens
issued under the same grant, so one request covers both halves.
Best-effort by construction. Every path returns a TokenRevocationOutcome
rather than throwing: no advertised endpoint means nothing is sent and that
authorization server behaves exactly as before, and a network error, a
non-2xx, a 5s timeout or an unreadable store is reported and nothing more.
Forgetting the tokens is what the user asked for. `lost_authorization_state`
recovery deliberately skips it — it clears a half-finished flow to retry it,
and an authorization that never completed has no grant to revoke.
Client authentication resolves the preregistered slot first, mirroring
BaseOAuthClientProvider.clientInformation; reading only the dynamic slot
would send no authentication for a server configured with `oauth.clientId`.
The Basic credential is byte-identical to the SDK's applyBasicAuth so this
request and the token request cannot present the same secret differently.
Opt out per server with `oauth.revokeOnClear` (default on, only `false`
written to disk) or per CLI run with --no-revoke. Read at clear time rather
than connect time, so toggling needs no reconnect. Turning it off is a
testing affordance: a client that walks away still holding live tokens is a
case a server author may want to reproduce.
test-servers now advertises `revocation_endpoint` and serves POST
/oauth/revoke, faithful on §2.2 (an unknown token is a success) and §2.1
(revoking a refresh token invalidates the access tokens under the same
grant), with oauth-revocation-http.json / oauth-no-revocation-http.json as
showcase configs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>
The SDK's `discoverAuthorizationServerMetadata` picks its validation schema
from the well-known filename that resolved rather than from the document that
came back, so anything found at `/.well-known/openid-configuration` is
validated as an OpenID provider document — requiring `jwks_uri`,
`subject_types_supported` and `id_token_signing_alg_values_supported`, three
fields RFC 8414 does not define. RFC 8414 §5 permits that filename for general
OAuth metadata, so a conforming plain OAuth 2.0 authorization server is
rejected; and because the parse throws instead of continuing the candidate
loop, discovery aborts and the connection fails outright.
Filed upstream as modelcontextprotocol/typescript-sdk#2733.
`core/auth/oidcDiscoveryCompat.ts` works around it without fabricating a
field: on a failed RFC 8414 candidate it probes the OIDC candidates the SDK
would try next and, when one returns a document that satisfies
`OAuthMetadataSchema` but fails the OIDC schema, serves that body as the RFC
8414 response so the SDK picks the schema that describes it. A genuine OpenID
provider document is left to the SDK's own OIDC leg. Issuer validation runs
unchanged, since the substituted document is the one the server published.
It wraps `effectiveAuthFetch` — above the fetch tracker, the opposite of
`withOAuthEndpointOverrides` — so the Network tab still records the real 404
and the real probe rather than the substitution.
The candidate derivation mirrors the SDK's `buildDiscoveryUrls`, which a fetch
wrapper cannot call, and a test pins it against that export so the two cannot
drift.
Adds `oauth.asMetadataPath` to the composable test server (mirroring
`oauth.resourceMetadataPath`) and the `oauth-rfc8414-at-oidc-path-http.json`
showcase config that reproduces the failure.
Closes#2172
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 1.
`refreshStoredAuthToken` consumes the same fallback and made a single SDK
discovery call, so preserving the path there — without the walk the other two
consumers got — would have regressed exactly the case the walk exists to keep:
a path-hosted MCP server whose authorization metadata is published at the
domain root.
It injects its own discovery function as a test seam, so the walk is now also
exposed as `discoverAuthorizationServerMetadataFromCandidates(candidates,
discover)`, which takes that function and reports *which* candidate answered —
the CLI needs the winning URL, since it is the base the token request is made
against, not just the metadata.
Also drops the `as unknown as typeof fetch` from the new test in favour of
`vi.fn<typeof fetch>()`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
When RFC 9728 protected-resource metadata names no authorization server, the
MCP server URL stands in as one. `getAuthorizationServerUrl` built that with
`new URL("/", serverUrl)`, which discards the path — exactly the part discovery
needs when the server is not hosted at the domain root. The SDK's
`buildDiscoveryUrls` derives the path-scoped well-known locations from that
pathname, so an origin-only value could only ever probe the domain root, which
404s for a server like `https://example.com/mod/minilesson/mcp.php`.
Keep the path (and strip query/fragment, which an RFC 8414 issuer never
carries). A straight swap would regress the case the origin-only value did
serve — a server that merely lives under a path while publishing its metadata
at the root — so the URL becomes the first of two candidates:
`getAuthorizationServerUrlCandidates` yields the path-scoped form then the bare
origin, and `discoverAuthorizationServerMetadataForServer` walks them, treating
a candidate's failure as non-fatal while another remains and rethrowing the
first error only when all of them fail.
`discoverScopes` and the CIMD pre-registration probe both go through the walk.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
#2114 landed on v2/main, which is the PR this one follows up. Two
conflicts, one of them silent.
The marked conflict was cosmetic: keep v2/main's comment on the
`provider.scope` read, which now notes the getter filters (#2068).
The real one git merged cleanly and would have left dead: #2114's
if (this.oauthConfig.requestRefreshToken === false && requestedScope) {
this.pendingAuthorizationScope = requestedScope;
}
sat directly above this branch's unconditional
`recordAuthorizationRequestScope`, which overwrites the same field a few
lines later. Dropping that gate is what #2117 is for, so it is removed
rather than kept as a no-op.
The two changes compose: the SDK adds `offline_access` only when
`clientMetadata.grant_types` includes `refresh_token`, which the provider
drops while the opt-out is on -- so reading the authorize URL yields the
filtered scope there, exactly what #2114's gated branch persisted.
#2114's third case pinned the gate and inverts here, as its issue
predicted. It now asserts the request is persisted even with the grant
on; that records no new claim, since the request equals what storage
already held.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
`trySilentEmaAuth` and `refreshEmaResourceTokens` saved tokens without
ever saving a scope. Both are reached from real paths -- initial EMA
authentication with a cached IdP session, refreshEnterpriseManagedTokens,
and EmaTransportOAuthProvider.tokens -- and each can be the moment a
newly configured or metadata-derived scope first takes effect, so a
silent token response left the previous stored scope standing.
Rather than a third copy of the rule, every EMA mint now ends in one
place: mintEmaResourceTokens reports the scope it requested, and
saveMintedTokens writes tokens and the RFC 6749 5.1 scope together.
The manager's EMA branch drops its own saveScope, which would now be a
redundant second write of the same value; the step-up branch keeps its
own, since it falls back to the challenge's authorizationScopes union
and runs last.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Two more paths had the same defect, both found in review.
`handleStandardAuthChallenge` returns an interactive redirect for
`unauthorized` / `token_expired` / `invalid_token` as well, and its
callback lands in the same completeOAuthFlow -- but the recording was
gated on `insufficient_scope`. Those challenges compute their own
`scopeForAuth`, which can differ from what storage holds, so a silent
token response left the stale scope standing. The read is now one
private helper both entry points call, applied to every redirect.
And `scopeForMint` is not always what the EMA mint asks for: with
neither config nor storage supplying a scope, discoverEmaResourceContext
resolves it from the protected resource metadata's `scopes_supported`,
which is what both exchanges send. `completeEmaIdpAuthorizationAndMint`
now resolves that context itself -- passing it into the mint, so no
extra discovery -- and returns it as `requestedScope` for the manager to
use as the RFC 6749 5.1 fallback.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 8 on #2114.
The provider's `scope` getter is not the only request path. Mid-session
`insufficient_scope` recovery builds its union from *raw* storage in
`enrichChallengeWithScopes` and hands it to `mcpAuth` directly, so it
never reads that getter. An `offline_access` inherited from an earlier
grant therefore reappeared during step-up and the SDK re-added
`prompt=consent` with the setting off — the failure this option exists
to prevent, on the one path that bypassed it. The comment claiming the
getter was "the one seam both readers go through" was wrong, and is
corrected to name both.
`handleAuthChallenge` now applies the same filter, gated on the setting
so the default path is untouched. The forced-retry reuses that value,
so filtering once at the computation site covers it.
`scopeForDeclinedRefreshGrant` gains the scopes a challenge requires,
honored exactly like a configured one: stripping a scope the server is
asking for would loop — re-authorize, earn the same challenge, strip it
again.
Three regressions; the inherited-scope one fails when the filter is
reverted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 6 on #2114.
The early return handed back the persisted scope unchanged, so an
`offline_access` the user had just typed into Scopes never reached the
request when storage predated the change — and storage routinely does,
since `createOAuthProvider` preserves a stored scope rather than
reseeding from current settings. The docs promised configured scopes
are honored and the form warned that `offline_access` "is still
requested"; both were false in exactly that state.
Honoring it means adding it, not merely declining to strip it.
Only the token itself is added, not the rest of the configured scope:
unioning unrelated configured tokens into the request is outside what
this setting governs and could push an otherwise-satisfied connection
into a step-up authorization.
Three tests, all verified to fail against the previous behavior.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 5 on #2114 — a hole in last round's fix.
When the persisted scope was `offline_access` alone,
`scopeForDeclinedRefreshGrant` returned undefined.
`OAuthManager.createOAuthProvider` reads that as "nothing stored" and
seeds storage with the configured scope, so the request-only filter
silently overwrote the value it promised to leave alone — and
re-enabling the grant could no longer restore it.
It now falls back to the configured scope, which is both the right
thing to request and inert to that branch. The previous "storage is
never rewritten" test could not see this: it ran at the provider level,
while the seeding branch lives in the manager. The new regression runs
there, and fails when the fallback is removed.
Also corrects the EMA rationale, which was wrong even though its
conclusion held. `createOAuthProviderForTransport` does build a
`BaseOAuthClientProvider` and wrap it, and `EmaTransportOAuthProvider`
forwards `clientMetadata` — so the setting is not bypassed outright.
What it cannot change is the IdP authorization leg's fixed
`openid offline_access`, which is the leg the user signs in through and
therefore the reason the control cannot affect EMA's consent behavior.
Fixed in the docs, the code comment, the UI string and the test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4 on #2114 — a real gap, not a wording one.
Declining the grant stops the SDK *adding* `offline_access`, but the
Inspector's own storage puts it back. A successful default-on
authorization persists the scope the AS granted
(`resolvePersistedScopeAfterGrant`), `offline_access` included;
`prepareForAuth` reloads it and `authenticate` hands it to the SDK as
`scope`, which sees the token in scope and appends `prompt=consent`
again. So any server that had authorized once could uncheck the box and
still hit AADSTS90094 — the users most likely to reach for the setting
were the ones it did not help.
`scopeForDeclinedRefreshGrant` filters it out of the *request* only,
applied in the provider's `scope` getter — the one seam both
`clientMetadata.scope` and `authenticate`'s `scope:` argument read
through. An `offline_access` the user configured is honored; storage is
never rewritten, so re-checking the box restores the old behavior
instead of finding the value destroyed, and scope-satisfaction checks
still see the real grant. Verified by reverting the filter: the new e2e
regression fails.
Also from that round, both previously suppressed:
The checkbox looked effective under enterprise-managed authorization,
which never builds this provider at all — it authorizes against the
enterprise IdP with a fixed `openid offline_access`
(`ema/constants.ts:16`). It now annotates itself the way the endpoint
overrides do. The existing count assertion covered two such notices and
was named for the overrides alone; it is renamed and expects three, with
a dedicated case pinning the notice to this control so the count cannot
be met by three copies elsewhere.
And "obtains no new refresh token" was another overstatement — the AS
decides issuance, and a preconfigured client is reused with whatever its
registration already permits. Replaced with what the setting actually
controls.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3 on #2114.
The unit tests asserted our own `grant_types` array and stopped there.
What actually unblocks an Entra tenant is what the SDK puts in the
authorize URL, and nothing covered that — so an SDK change that stopped
keying its augmentation off `grant_types` would leave AADSTS90094
intact with every test green.
Adds the complementary case to the existing e2e suite, against a real
AS advertising `offline_access`: with the grant declined, the authorize
URL carries neither `offline_access` nor `prompt=consent`, while the
configured scope still goes out. Verified non-vacuous by reverting the
provider conditional — it fails. The positive test alongside it now
also asserts `prompt=consent` is present, which was unasserted despite
being the literal failure in the issue.
Drops an unjustified `as unknown as ServerEntry` I added in the App
test — AGENTS.md forbids it, and it hid a real mismatch (a partial
settings object against a complete one). Typed `ServerEntry` /
`InspectorServerSettings` fixtures instead, so a field added to that
interface later is a compile error rather than a missing value at
runtime. The `constructedOAuth` helper reads through the constructor's
own parameter type and needs no cast at all now.
Scopes the two doc comments that still claimed the flag removes every
`offline_access` — `StoredMCPServer.oauth` and `OAuthProviderConfig`,
both contradicting the caveat already stated elsewhere.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
Two findings from review:
- `ensureCimdClientRegistration` ran protected-resource discovery on the
global fetch while its AS-metadata leg used the configured one. On web
that is `createRemoteFetch`, which proxies through the backend to sidestep
CORS — so the discovery this PR routes to a challenge-advertised URL would
fail in the browser, be swallowed by the catch, and leave CIMD probing the
wrong authorization server. Forward `params.fetchFn`, and assert in the
test that nothing reaches the global.
- `oauth.resourceMetadataPath` accepted `//other-host/doc`. Resolved against
the request base that re-points the origin in the advertised
`resource_metadata` while Express still registers the route locally, so the
server would advertise a document it does not serve. `/\\host` does the
same, since the URL parser folds a backslash into a slash for special
schemes — a leading-slash check misses both. Validate origin-relativity
with a shared predicate, used by the JSON-config validator and again at
server setup for a programmatically built ServerConfig.
Signed-off-by: cliffhall <cliff@futurescale.com>
The Inspector always declared `refresh_token` in its OAuth client
metadata. The SDK's `determineScope()` then appends `offline_access`
whenever the authorization server advertises it and the client declares
that grant, and `startAuthorization()` appends `prompt=consent` whenever
`offline_access` is in scope. Against Microsoft Entra ID that forced
consent prompt routes a non-admin user into the admin-consent workflow
and fails with AADSTS90094 — even after a tenant admin has consented.
Adds a per-server "Request refresh token" setting (default on) that
drops the grant from the client metadata, breaking that chain at its
source. Persisted as `oauth.requestRefreshToken` and written only when
off, so entries that never touch it keep a byte-stable round-trip. Web
edits it in Server Settings → Authorization; the CLI and TUI pick it up
from the same per-server settings via `buildRunnerClientAuthOptions`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>
An RFC 9728 `WWW-Authenticate: Bearer resource_metadata="…"` challenge was
parsed and then dropped by the normalized `AuthChallenge`, so every
challenge-driven `mcpAuth()` call ran discovery without it and probed
locations derived from the MCP server URL instead. OAuth failed outright
against any server whose metadata document lives off the well-known path.
The SDK handles this correctly inside its own transports, but the Inspector
never reaches that path — it intercepts the 401 and drives `auth()` itself
to capture the authorization URL and run the step-up scope union.
Carry the advertised URL on `AuthChallenge` as a string (it crosses the web
client's remote-backend boundary as JSON), convert it at the OAuth boundary,
and pass it to the standard challenge leg, the forced step-up leg, and CIMD
pre-registration — which runs before `auth()` and did its own discovery. A
malformed value is ignored rather than surfaced, matching the SDK's parser.
The callback leg needs nothing: SDK `auth()` persists the URL in its
discovery state.
Adds an `oauth.resourceMetadataPath` test-server mode that moves the
document off the well-known path entirely, so the integration test proves
discovery genuinely fails without the fix.
Closes#2071
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review on #2105: jwtExpiresAtMs is lenient — it parses the second of
any two-or-more dot-separated segments — so a structured opaque token such as
`opaque.<base64url({"exp":...})>` returned a parseable exp and could still
short-circuit re-authorization, recreating the replay loop.
isAccessTokenProvablyUnexpired now checks isJwtFormat first. Tests cover a
two-segment and a four-segment token whose payload decodes to a future exp.
Signed-off-by: cliffhall <cliff@futurescale.com>
`checkAuthChallengeSatisfied` answered a `token_expired` challenge with
`isAccessTokenUsable`, which treats "we cannot tell" as "still valid":
`isJwtExpired` returns false whenever the token is not a JWT or carries no
`exp`, and storage never records an absolute expiry (`expires_in` is relative
to an issuance instant that is never persisted).
So for an opaque access token — Doorkeeper, Keycloak in opaque mode,
GitHub-style `gho_…` — the local guess outvoted the resource server's
explicit verdict. Callers short-circuited re-authorization and reconnected
with the same dead credential, looping with no redirect and no ReAuthBanner.
A bare 401 also maps to `token_expired`, so this was not limited to servers
sending `error="invalid_token"`.
Adds `isAccessTokenProvablyUnexpired` beside `isAccessTokenUsable` in
`core/auth/connection-state.ts`: true only for a JWT whose `exp` is still in
the future (same 60s skew). `checkAuthChallengeSatisfied` uses it for the
`token_expired` branch, so absence of local evidence no longer beats the
server, while a provably live JWT keeps its cheap short-circuit.
`isAccessTokenUsable` stays optimistic — it is the right default for
displaying authorization state, and flipping it would show every
opaque-token server as unauthorized.
Closes#2051
Signed-off-by: cliffhall <cliff@futurescale.com>
The one genuinely new finding from Copilot round 9; the other four were
re-reports of rounds 7 and 8 against an earlier state of the files, verified
already fixed in e956d498 / edddc6c9.
This message has now been wrong in both directions. Round 5 replaced "it
expires on its own after 10s" — false whenever the same `rmdir` that blocked
us also blocks stale takeover, which reclaims through that identical call —
with "remove it by hand". That is false the other way: `proper-lockfile`
forwards whatever the filesystem returned, so a transient failure can clear,
another Inspector can legitimately acquire the path afterwards, and an
operator following the advice then deletes a *live* holder's lock. Same class
of hazard as the `ERELEASED` case in round 7: guidance that destroys the
exclusion of a process that did nothing wrong.
Nothing here can tell a permanent refusal from a passing one, so it no longer
claims to. It reports what happened, says the lock may or may not clear and
why, and states the two conditions the operator can check for themselves —
saves still failing, and no other Inspector running — rather than asserting
an outcome this code does not know.
Behavioural only in what it tells a human; the locking path is unchanged.
The test asserts both conditions are present and that the expiry promise is
absent, so neither blanket version can come back.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 8 — two findings, both upheld. The first is a gap in
round 7's own fix.
**The snapshot was unprotected between two awaits.** Returning from
`withSecretFileLock(filePath, ...)` releases the main lock *before*
`handOffStagedSecrets` acquired the snapshot's, so a second startup could
take the main lock in that window, see the staged file unlocked, and adopt
and re-claim it — leaving this process reading a path that no longer exists.
Narrower than the race round 7 closed, but the same one.
`openSecretFileLock` is now split out of `withSecretFileLock`: it takes the
lock and returns its release, so a caller can hold one *across* another's
release. The claim acquires the snapshot's lock before returning — inside the
main lock — and the hand-off then runs outside the main lock, still holding
it. `withSecretFileLock` is a thin wrapper over the same function, so which
failures refuse and which degrade is decided in one place and the two entry
points cannot drift.
Locking the snapshot degrades rather than refusing (`null` → hand off
anyway): the exposure is one uniquely-named snapshot, which is a better trade
than refusing to migrate at all on a box that cannot lock.
**Both migration scans matched the lock directories themselves.**
`secrets.json.migrating-<pid>-<uuid>.lock` passes the plain prefix test, and
treating it as a snapshot is self-sustaining damage: the liveness probe asks
about a nonexistent `<name>.lock.lock` and so answers "not held", recovery
tries to hard-link a *directory* onto the secrets path, fails, and prints the
orphan warning. Because a liveness *check* never clears a stale lock
directory — only a would-be acquirer does — that false migration repeats on
every startup forever, including long after the real snapshot was recovered.
`isSnapshotName` now excludes `.lock` for both `anythingToMigrate` and
`recoverOrphanedSnapshots`.
New test verified by mutation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 7 — two findings, both upheld.
**An in-progress migration snapshot could be adopted by another startup.**
The fast path counts every `*.migrating-*` as work and
`recoverOrphanedSnapshots` adopts them — including one whose owner is still
reading it. It link/unlinks the snapshot back to the live path and re-claims
it under a new name, the owner's hand-off then fails `ENOENT`, and if the
adopting process exits before copying, that healthy session starts with none
of those secrets, recoverable only on some later run.
Fixed the second way the reviewer suggested, not the first. Holding the main
lock across the hand-off also closes it and was tried — it broke "claims the
file atomically, so a later write is not deleted", because it blocks every
ordinary writer for the whole migration and fails their save past the retry
budget. That is a worse regression than the race, since #1950 guarantees a
write completing after the claim survives. The existing test caught it.
So the hand-off takes a lock on the **snapshot** instead, and
`recoverOrphanedSnapshots` skips an orphan whose lock is held. Liveness comes
from the lock rather than from the pid already in the filename: a pid both
outlives its process and recurs — pid 1 on every container start, which
#1950 documents as the reason the name carries a nonce — whereas a lock
expires by itself if its owner dies, so a genuinely abandoned snapshot
becomes adoptable with nothing to clean up. `isFileLockHeld` answers `false`
when it cannot tell, since the alternative to a wrong `false` is never
recovering an abandoned file.
**A compromised lock arrives as `ERELEASED`, and we told the operator to
delete the lock.** `setLockAsCompromised` marks the lock released and drops
its registry entry *before* calling `onCompromised`, so `release()` answers
`ERELEASED` without touching the filesystem — and the directory at that path
is by then the **winner's live lock**. "Remove it by hand" would destroy the
exclusion of a process that did nothing wrong. Handled separately and
silently; `onCompromised` has already reported what happened. A conditional
rather than an early return, since a `return` inside `finally` discards
whatever the body was returning or throwing.
Both new tests verified by mutation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 6 — five findings (two inline, three suppressed), all
valid.
**A stale lock that cannot be cleared caused unlocked writes.** `acquireLock`
does not only *create* directories: on finding a stale one it removes it and
retries, and that removal can fail — `ENOTEMPTY` for a lock with anything
inside it, `EACCES`/`EROFS` for one we may not touch. Those surface as
ordinary non-`ELOCKED` errors, which the probe read as "locks do not work
here" and degraded on. So every Inspector on the box quietly bypassed the
*same* stuck lock and raced its writes — while the release-failure message
was telling the operator saves would keep failing until they cleared it.
`isStuckOrHeld` now discriminates on **whether the lock directory exists**
rather than on an errno taxonomy: if it is there, something holds it and we
refuse; if it is not, we genuinely could not create one and degrading is the
documented trade (#1848, #1905). Reading the state the decision is about
avoids enumerating error codes per platform — and an enumeration is exactly
what let `ENOTEMPTY` through.
**Identity was captured after the acquire promise settled.** That span
includes the library's own `utimes`/`stat` probe, so a waiter replacing our
directory inside it would have us record *the winner's* identity as our own,
after which the removal guard would accept and delete their lock. It is now
recorded in the `mkdir` callback — the moment the directory becomes ours, and
synchronous with respect to this process, so there is no window.
**The expiry advice was still wrong for `EACCES`/`EPERM`/`EROFS`.** Since the
guard `stat`s before removing, everything reaching the release catch is an
`rmdir` that was refused — and stale takeover reclaims through that same
`rmdir`. The `ENOENT` arm was unreachable for the same reason, so the branch
was dead code: removed, and the advice is now unconditional.
**`proper-lockfile` was missing from `NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE`.** A
fourth externalization surface — the tsup lists configure the production
bundles, not `vite dev`, whose dep scanner would otherwise walk its
CJS/`graceful-fs`/`signal-exit` graph. Added, and `vite-base-config.test.ts`
now asserts it along with `chokidar` and `@napi-rs/keyring`, which were also
absent from that assertion.
**A test was named for something it did not exercise.** "does not treat an
unlistable directory as an empty one" was driving `ENOTDIR`, which is
deliberately treated *as* the fresh-install case, so the actual new behaviour
was untested. Split in two: one for `ENOTDIR`, and a real `EACCES` one using
a `--x` directory — `readdir` denied, a known name still reachable, which is
the asymmetry that makes "unlistable" different from "empty". Driven with a
real mode rather than a stub, since `vi.spyOn` cannot redefine an ESM
namespace export; skipped as root and on Windows, where the mode would not
bite and the test would assert nothing.
Both new tests verified by mutation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 5. Both comments upheld; the first is a real hole in
round 4's own fix.
**The ownership guard was in the wrong place, and overclaimed.**
Round 4 checked ownership and then called `release()`. Two problems:
- Check-then-act. The `stat` and the `rmdir` were separate async steps, so a
waiter could replace the directory in between and we deleted the winner's
fresh lock anyway — the very thing the check was added to prevent.
- Skipping `release()` on a mismatch left proper-lockfile's record registered
in its `locks` map, and its `signal-exit` handler `rmdirSync`s every
registered lock with **no ownership check of its own**. An exit during that
window deleted the winner too.
The guard now lives in `options.fs`, which is the single seam *both* removal
paths route through — `removeLock` on release, and the exit handler. It
refuses to remove a directory whose inode and birth time are not the ones
recorded at acquire, and reports the refusal as success so the library's
bookkeeping forgets the lock either way (leaving it registered is what hands
the exit handler a record pointing at the winner's directory).
The check is a `statSync` immediately followed by an `rmdirSync`, with no
`await` between them, so nothing in this process can interleave. **It still
does not close the race** — it is check-then-act across processes, which needs
the compare-and-swap Node does not expose — and saying it "closes the
destructive half outright" was wrong. file-lock.ts, README.md,
specification/v2_servers_file.md and AGENTS.md now all say it narrows the
window rather than closing it. AGENTS.md carries the negative instruction
alongside round 4's, since this is the third round spent on an overclaim.
**"It expires on its own after 10s" is false for the ENOTEMPTY case** the new
test exercises. Stale takeover reclaims through the same `rmdir`, which also
cannot remove a non-empty directory — so nothing clears it, and every later
save fails ELOCKED against it. `releaseAdvice` now branches: by-hand removal
for ENOTEMPTY, expiry for everything else.
Tests: a real child process demonstrates the exit handler removing a
replacement lock (with an **empty** directory — a non-empty one makes
`rmdirSync` fail ENOTEMPTY, which is how the first draft of this test passed
for the wrong reason). The release-failure test now asserts the message says
"by hand" and does not promise expiry. The shim's `rmdirSync` arm carries a
justified `v8 ignore`: it is reachable only from a real process exit, and its
logic is `removeIfMine`, covered through the async arm.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4 — four comments, all the same finding against the four
files carrying the claim, and it was more than a wording problem.
The comment said a holder whose lock was replaced is told. Checked against
proper-lockfile@4.1.2, that is false in the case that actually happens:
- `updateLock` compares mtime only on its refresh tick — `stale / 2`, so 5s
here — while an ordinary mutation is a read, an scrypt derivation and an
atomic write, comfortably under a second. The tick never runs, so nothing
fires.
- `release` → `unlock` → `removeLock` is an unconditional `rmdir` with **no
ownership check**. So the compromised holder goes on to delete the
*winner's* lock on the way out, ending the winner's exclusion too: one
compromised writer silently becomes two unprotected ones.
The second half is a correctness bug, not a documentation defect, and it is
fixable here even though the takeover race itself is not.
`withSecretFileLock` now records the lock directory's inode and birth time at
acquire and re-checks them before releasing. On a mismatch it **declines to
release** — leaving the new holder's lock alone costs nothing, ours is
already gone — and warns.
Inode and birth time rather than mtime, deliberately: both survive the
library's own `utimes` refresh, so a lock held past one tick is not accused of
being compromised, and both change on a delete-and-recreate, which is exactly
the event to detect. Where a filesystem reports neither, the two reads agree
and the check concludes "ours", degrading to the library's unaided behaviour
rather than to a false alarm. An unreadable baseline at acquire likewise
answers "ours" — with nothing to compare against, accusing a healthy lock
would be worse than releasing it.
Detection is therefore **best-effort**, and file-lock.ts, README.md,
specification/v2_servers_file.md and AGENTS.md now all say so rather than
promising the loser is told. AGENTS.md carries the negative instruction too,
since this is the second round spent on the same overclaim.
Tests: "does not delete the winner's lock after being taken over" replaces the
lock inside the body and asserts the replacement survives with the *same*
inode and birth time — it was left alone, not deleted and recreated. It waits
for nothing, being the fast case the library's tick misses. Verified by
mutation: forcing `stillOurs` to always answer true fails it. A second test
covers a release that fails while the lock genuinely *is* ours (a stray file
makes `rmdir` fail ENOTEMPTY), so the ownership check cannot swallow a real
release failure.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3.
- **`absorbFileSecretsIntoKeyring` must never throw, and round 2 broke that.**
Making `withSecretFileLock` reject on a held lock was right for `set` — a
user is waiting on that value — and wrong here: this is awaited directly by
both `resolveSecretStore` branches, so a stuck writer elsewhere on the box
would have failed store resolution and with it the whole session. The lock
failure is now caught, warned, and the file left for the next run. Covered
by a test that holds the lock from another handle and asserts the hand-off
resolves, the file survives, and the lock-specific warning is emitted (it
asserts that message rather than the shared "left in place" tail, which the
pre-existing claim-failure path also prints and would have let it pass
without the lock ever being reached).
- **The refusal message overstated how long it waited.** `retries *
maxTimeout` reads 20s for a schedule that sums to 15.26s — the early
attempts are the exponential ramp, not the cap. Replaced with
`RETRY_BUDGET_MS`, computed from the schedule rather than written down, so
it cannot drift from the thing it describes. `retry` applies no jitter by
default, so it is exact rather than an estimate.
- **The PR description still described the pre-round-2 design** — the
single-winner claim and the ~3s backoff. Rewritten to match the code.
Coverage: the round-3 changes cost `secret-store-selection.ts` two branches
and it fell to 89.62%. Recovered honestly rather than by annotation — the
`ENOTDIR` arm of `anythingToMigrate` now has a real test (a path whose parent
is a file, the other shape of "nothing there"), and the new catch types its
parameter `Error` instead of re-narrowing, since `withSecretFileLock` rejects
only with the `SecretStoreUnavailableError` it constructs one call away.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 2, both comments upheld.
**ELOCKED must not degrade.** Exhausting the retries on a held lock means the
lock is *working* and something else demonstrably holds it — so running the
mutation anyway entered the exact interleaving the lock exists to prevent,
and entered it knowing a concurrent writer was there. `set` now fails
instead. Two supporting changes:
- The retry budget went from ~3.3s to ~15s so it outlasts the 10s stale
window. Refusing is only defensible because a *crashed* holder resolves by
takeover first; with the shorter budget, one Inspector killed mid-save
would have failed every later save on the box until someone deleted the
lock by hand. Pinned by a new test that stages a dead holder's lock
directly (mkdir + backdated mtime) and asserts the save succeeds.
- Acquire is now probe-then-wait. `proper-lockfile` drives its whole acquire
through `retry`, which re-attempts on *any* error — so with a 20-retry
budget a read-only `$HOME` would have spent the full 15s re-issuing an
identical failing `mkdir` on every save before degrading. One retry-less
probe separates "held" (worth waiting out) from "unavailable" (not).
**The comment overclaimed what proper-lockfile provides, and the reviewer was
right about the mechanism.** Verified against lib/lockfile.js@4.1.2: on
EEXIST it stats, and if stale it rmdirs and re-mkdirs *without* checking the
directory it removed is the one it found stale — so a slow waiter can still
delete a fast waiter's fresh lock and both proceed. That is the same race the
hand-rolled version could not close, and it is not closable without
renameat2(RENAME_EXCHANGE).
What the library actually adds is that the loser is *detected*: its refresh
tick compares the lock's mtime against the value recorded at acquire, so a
compromised holder is told rather than proceeding silently. And what is
genuinely exclusive is the case that matters — mkdir is atomic and a live
holder refreshes its mtime, so its lock never goes stale and two running
Inspectors are serialized. The residual window opens only after a holder dies
without releasing, and the optimistic verify still covers it.
file-lock.ts, README.md, specification/v2_servers_file.md and AGENTS.md all
carried the overclaim; all four now state the narrower, true version.
Also fixes a test that was quietly asserting the opposite of its name: its
holder used stale: 60_000 while the waiter used 10_000, and staleness is
judged by the *waiter's* threshold — so the holder was declared stale after
10s and the save succeeded by takeover.
Not done: a two-process stale-takeover race test. It would be testing the
library rather than this code, and is inherently nondeterministic; the honest
response to that finding was to stop claiming the race is closed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
- Create the storage directory before locking. `writeStoreFile` creates it,
but from inside the locked section — so on a fresh install, where
`~/.mcp-inspector` does not exist, proper-lockfile failed ENOENT and every
*first* save degraded to an unlocked write with a warning. That is the save
most likely to be racing another (two Inspectors started together both
reach it), so the lock was absent from precisely the interleaving it exists
to close.
- `anythingToMigrate` no longer reads every `readdir` failure as "nothing to
migrate". Only ENOENT/ENOTDIR proves the fresh-install case; a directory
that denies listing can still permit access to the known `secrets.json`,
and returning false there selected the keychain and left those secrets
invisible with nothing said. Other errors fall through to the under-lock
checks, which is what happened before the fast path existed.
- Pin `proper-lockfile` to the repo-root install in `vitest.shared.mts`,
beside express and yaml: it is reached only through root-owned `core/`.
Resolution already finds the root copy since no client declares it; the pin
is what stops that depending on it never arriving as some client's
transitive dependency, which would give a test two copies of a module whose
whole job is one registry of held locks.
- Make the cross-process test prove `mutate` takes the lock. The previous one
asserted only that both secrets survived, which the optimistic verify
delivers with the lock removed entirely. It now asserts the parent's `set`
has *not* settled while a child holds the lock — an observation only a real
lock produces. Verified by removing `withSecretFileLock` from `mutate`:
three tests fail, where none did before.
The degrade test's "lock cannot be created" case is now a path whose parent
is a *file* rather than a missing directory, since a missing directory is no
longer that case. It also fails identically for root, so it cannot pass
locally and flake in a container.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>
