Since round 1 the doc claimed the `name` fallback covers "a structured clone or
a JSON hop". Verified, and it is false for half of that:
structuredClone(new InsecureTokenEndpointError(url))
-> name: "Error", tokenEndpoint: undefined, constructor: Error
structuredClone normalizes a custom Error subclass back to Error, so neither
arm has anything left to match. A caller routing this error through
structuredClone or postMessage would silently get the generic retryable
handling back — the exact defect this PR removes.
The test could not have caught it: it hand-built a look-alike plain object, so
it asserted what I believed the boundary does rather than what it does. It now
performs a real JSON round trip (asserting the prototype is gone), and a second
case pins that structuredClone is NOT recognized — turning a false claim into a
tested limitation.
The doc comment now scopes the contract to JSON and states the structuredClone
limit with the remedy: serialize the fields explicitly across such a boundary.
Also removes an orphaned JSDoc block the round-4 refactor stranded above the
report helper, and gives showInsecureTokenEndpointNotice its own doc back.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju
Signed-off-by: cliffhall <cliff@futurescale.com>
- The recovery text told users to move the endpoint to `::1`. A bare IPv6
literal is not a legal URL host — `new URL("http://::1/token")` throws — so
anyone copying that into the Token URL override got a parse error from advice
meant to unblock them. Now `[::1]`, in both the notice and the manual
reproduction guide.
The exemption list still reads `::1`, because that is the host the SDK
compares; only the remedy is bracketed, because that is what a user types.
The comment says so, so neither gets "corrected" into the other.
- Dropped SEP-2207 from all nine files it had reached. I took the label from
the SDK's own source comments, which attribute this check to it — but this
repo already uses SEP-2207 for OIDC refresh / offline_access
(specification/v2_auth_hardening.md, plus an e2e test), so grepping it here
would have turned up two unrelated things. Replaced with plain description
rather than a different SEP number: having guessed wrong once, guessing again
is not the fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju
Signed-off-by: cliffhall <cliff@futurescale.com>
Both are accuracy defects in the one message whose entire job is to be
actionable, so both matter more than their size.
- "before any credentials were sent" is FALSE on the mid-session refresh and
re-authentication paths this same notice serves, where credentials were
legitimately sent earlier in the session. A user connected for an hour would
read it as describing some other failure. Now scoped to the request actually
refused: "without sending this request".
- The recovery guidance named "Server Settings → Authorization". The UI renders
that accordion as "OAuth Settings" and the field as "Token URL override"
(ServerSettingsForm.tsx), so the message directed people to a section that
does not exist, precisely when it was asking them to go reconfigure
something.
Both pinned by tests, including negative assertions on the old wording so it
cannot drift back.
The round's third finding was against test-server-http.ts's strictPort
precondition, which no longer exists — that option was dropped in 6c1b532,
after the review ran.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju
Signed-off-by: cliffhall <cliff@futurescale.com>
Closes#2280
InsecureTokenEndpointError appeared nowhere in our source, so it fell through
to the generic auth-failure path and rendered a "Re-authentication required"
banner with a Re-authenticate button. That button can never work: the SDK's
assertSecureTokenEndpoint runs inside executeTokenRequest, the error does not
extend OAuthError, and auth() special-cases it to rethrow rather than start a
fresh /authorize redirect. Clicking re-ran the same flow to the same refusal,
under the raw SDK message, which names the three exempt host literals and
nothing actionable. A terminal configuration error was being presented as a
retryable auth error.
Not specific to *.localhost: it fires for any endpoint outside the SDK's
exemption — host.docker.internal (#1911), a LAN hostname, a reverse-proxy
name, a mistyped scheme.
It is now recognized (core/auth/insecureTokenEndpoint.ts, mirroring
issuerBinding.ts's brand-plus-name classifier and walking cause / data.cause,
since era negotiation and the transport wrappers bury the rejection) and
surfaced as the configuration error it is, naming the endpoint and both ways
out, with no action affordance.
Six paths reach the refusal and all six classify it — the connect handshake,
authenticate() during connect, the satisfied-challenge connect retry, the
post-redirect callback, the command/background path, the deferred tab-visible
resume (which re-armed on every focus, an unbounded loop on a terminal error),
and the banner action. They share one wrapper so a seventh cannot omit the
banner clear, and every clear is scoped to its own serverId: these paths are
asynchronous, so a late continuation for one server must not erase a banner
another raised in the meantime.
Adds oauth-insecure-token-endpoint-http.json to reproduce it, whose issuer is
http://localhost.:8091 — the root-anchored spelling every resolver sends to
loopback but which is none of the SDK's exempt literals. transport.strictPort
keeps that fixture honest, since its port is hard-coded inside the issuer.
Making such an endpoint work has to land in the SDK (typescript-sdk#2591);
this changes the reporting, not the outcome.
Split out of #2282, where it was tangled with the *.localhost origin work for
#1944 — a separate question the reporter is still clarifying.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018MvNbmAtQuWhDvzi4yXDju
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review (#2287): `EmaTransportOAuthProvider` forwarded
`clientInformation` / `saveClientInformation` without the SDK's `ctx`, and
implemented neither `discoveryState` nor `saveDiscoveryState`. Since the wrapper
does expose `clientMetadataUrl`, an EMA connection can still take SDK `auth()`'s
URL-based-client-ID branch — and the inner provider then saw
`issuer === undefined` with no discovery state to read back, so the CIMD write
was recorded as DCR.
Forward `ctx` on both, and delegate the two discovery-state methods to the inner
provider. Both were pre-existing SEP-2352 gaps in their own right: dropping the
issuer put every EMA read and write on the unkeyed slot, and the missing
discovery state meant the SDK re-discovered on every call and warned that it
could not run its callback-leg authorization-server binding check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review (#2287), two findings — and taken together they said the marker
was the wrong mechanism, so this removes it rather than patching it again:
- A transient failure in our CIMD preflight recorded no marker, so a *successful*
SDK fallback discovery that advertised CIMD was still stored as `dcr`.
- `existing` and `existingKind` could describe different registrations, because
`clientInformation()` answers preregistered-first while `clientRegistrationKind()`
prefers the issuer's dynamic slot.
SDK v2 `auth()` persists the authorization-server metadata via
`saveDiscoveryState` *before* it reads or writes client information, and it takes
its URL-based-client-ID branch — rather than `registerClient` — exactly when that
metadata advertises `client_id_metadata_document_supported` and a
`clientMetadataUrl` is configured. So the branch is not inferred at all now: it is
read back from the state the SDK itself just wrote.
`resolveSdkRegistrationKind` becomes two cases. A save for an issuer that already
has a registration under this `client_id` is a back-stamp, answered by that
registration's recorded kind — so an existing DCR on the metadata URL stays
`dcr`. A save for an issuer with none is a new registration, answered by the
discovery state, guarded on the metadata describing that same issuer.
This drops `cimdClientMetadataUrl` and its storage accessors entirely, and
restores `ensureCimdClientRegistration` to a plain issuer-bound pre-registration.
Invalid-client recovery, second-issuer CIMD, second-issuer DCR minting the
metadata URL, and preflight failure are all consequences of the two cases rather
than separate mechanisms.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review (#2287): the marker was written as soon as the AS advertised
`client_id_metadata_document_supported`, before checking what registration
already existed. RFC 7591 §3.2 leaves a dynamically issued `client_id` opaque,
so an existing DCR may carry the configured metadata URL — and marking the
issuer then relabels that real dynamic registration as CIMD on the SDK's next
issuer stamp.
The marker now records both facts it is read for: that this AS accepts the URL
as a `client_id`, *and* that the registration standing for it got there through
CIMD. It is written where this helper establishes the registration itself,
reaffirmed for one already recorded as `cimd` under that exact URL, and withdrawn
otherwise — which also covers a static client and an AS that has stopped
advertising CIMD.
Covered at both levels: a mocked case asserting the marker is withdrawn for an
existing DCR on the same URL, and an end-to-end real-storage case asserting such
a registration is still `dcr` after the SDK issuer-stamps it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review (#2287, suppressed comment): the comment still said internal
callers supply no issuer, which stopped being true when
ensureCimdClientRegistration started binding its save to the discovered issuer.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review (#2287), three findings:
1. **invalid-client recovery lost the provenance.** SDK v2 `auth()` answers
`invalid_client` / `unauthorized_client` with `invalidateCredentials("client")`
and an immediate retry; our clear drops the registration *and* its kind, so
the retry's URL-based client-ID save landed with nothing recorded and was
stored as `dcr`. Provenance now lives in `cimdClientMetadataUrl`, an
issuer-keyed marker on the issuer slot that records a property of the AS
rather than a credential — `clearClientInformation` deliberately leaves it
alone. `ensureCimdClientRegistration` writes it, and withdraws it when the AS
stops advertising CIMD, on every connect.
2. **Discovery became a hard network dependency.** Moving the existing-client
check after discovery meant a well-known outage could fail a reconnect the SDK
would have served from its persisted discovery state. Reuse
`provider.discoveryState()` first, and treat a discovery failure as "skip
pre-registration" rather than an error — this helper is an optimization over
what `auth()` does for itself.
3. **Dropped an unjustified double cast** in the test fetch helper; the async
signature is directly assignable to `typeof fetch`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review (#2287): the active-issuer fallback added last round could copy
issuer A's CIMD provenance onto a real dynamic registration for issuer B, since
RFC 7591 §3.2 permits B to mint the same opaque URL as its `client_id`.
Fix it at the source rather than in the resolver. `ensureCimdClientRegistration`
now runs discovery first and records the registration against the issuer it just
discovered, having confirmed *that* AS advertises
`client_id_metadata_document_supported`. Its "already registered?" check moves
after discovery and is keyed by that issuer — read ctx-less it resolved through
the active issuer and early-returned for every later one, which is what forced
the cross-issuer fallback in the first place.
`resolveSdkRegistrationKind` is therefore issuer-scoped again, with no fallback.
A second AS behind one resource now gets its own determination: CIMD when it
advertises CIMD, `dcr` otherwise — including when it mints the metadata URL as
its own `client_id`.
The cost is a discovery round trip per connect attempt rather than only the
first; noted at the call site.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review (#2287): scoping the stored-CIMD check to the incoming issuer
alone mislabels the SDK's own CIMD write for a second authorization server.
SEP-2352 keys registrations per AS, so the first binding promotes the unkeyed
CIMD entry into issuer A's slot and clears the fallback;
`ensureCimdClientRegistration` then early-returns on its ctx-less read, and the
save under issuer B finds nothing recorded for B.
Check the issuer slot and then the server's active registration, so a second
issuer stays CIMD while a `client_id` the AS minted itself still falls through
to `dcr`. Covered by two tests driving a real `OAuthStorageBase` through the
A → B sequence, since the behaviour under test is how storage promotes and
clears slots rather than anything a mock would express.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review (#2287): RFC 7591 §3.2 leaves a dynamically issued `client_id`
opaque, so `client_id === clientMetadataUrl` is not on its own proof that CIMD
ran — an authorization server could in principle mint that value from
`POST /register`.
Narrow the claim to a conjunction, whose decisive term is a registration we
recorded as CIMD ourselves rather than an inference about what the AS returned:
CIMD must be configured for this connection, the incoming `client_id` must be
exactly that metadata-document URL, and the registration already stored under
that id must be recorded as `cimd`. Everything else falls through to `dcr`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
`BaseOAuthClientProvider.saveClientInformation` treated every save without an
explicit `registrationKind` as DCR. The SDK reaches it from three places in
`auth()` and only one is a dynamic registration, so the issuer-binding write
overwrote the `cimd` provenance our own pre-registration had just stored — and
Connection Info reported `Dynamic (DCR)` for a connection that never issued a
`POST /oauth/register`.
Recover the kind instead: a `client_id` equal to the configured
`clientMetadataUrl` is CIMD (which also covers the SDK's own CIMD write), and
otherwise a stored registration with the same `client_id` carries its recorded
kind forward. Matching on `client_id` keeps stale CIMD provenance from leaking
onto a later DCR registration for the same server.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lmj6X2epWpPE7ScdSKQmtA
Signed-off-by: cliffhall <cliff@futurescale.com>
The shared teardown deadline was decided on `remainingMs > 0` against
`Date.now()`. Both halves of that are noise-sensitive: the deadline is
enforced by a `setTimeout`, and a wall clock at millisecond resolution
can still read a hair short of it when the loop comes round, so the next
grant inherits a fractional budget and spends it on a request that cannot
possibly complete. The same run then issues one request or two depending
on scheduling — which is what makes the "shares one deadline across
grants" test intermittent on CI (#2252).
Measure the deadline with `performance.now()` (monotonic, so an NTP step
or a suspend cannot expire or extend the budget, and sub-millisecond, so
none is lost to rounding), and skip any grant reaching the loop with less
than MIN_REVOCATION_REQUEST_BUDGET_MS left. The skipped grant is already
reported as `failed`, so nothing is silently dropped — the needless call
to the authorization server is.
The CLI's outer per-plan budget in `sendPlans` shares both the shape and
the defect, so it gets the same treatment.
One consequence worth naming: a monotonic clock hands `revokeToken` a
fractional budget, and Node's `AbortSignal.timeout` throws
`ERR_OUT_OF_RANGE` on a non-integer delay — before the fetch, so the
request would never be sent and the revocation would report that as its
failure. `revokeToken` now rounds its own budget to whole milliseconds,
which also covers any other caller passing a fractional one.
Tests: the epsilon skip is pinned by a grant that lands inside the floor
but above zero (removing the floor fails it, verified); the fractional
budget by a `revokeToken` call at 19.996ms.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017wEXtbs8UEHUMxDEAxbs99
Signed-off-by: cliffhall <cliff@futurescale.com>
#2222 reported that `core/auth/revocation.ts` breaks a client secret
containing `+`, on the basis that `encodeURIComponent` leaves `+` bare.
It does not — it escapes it as `%2B`. The characters it leaves bare are
exactly `!'()*-._~` and alphanumerics, every one of which a form-
urldecoder passes through unchanged, so its output decodes identically
under both algorithms; verified over every code point up to U+2FFF.
Switching to `URLSearchParams` would be the literal algorithm §2.3.1
names and a small regression: it encodes a space as `+`, which a lenient
server decoding with `decodeURIComponent` alone reads as a literal `+`,
while `%20` is understood by both. The encoder is therefore unchanged
and the reasoning is recorded on it.
The report's second half stands and is the real defect: the fixture
decoded with `decodeURIComponent`, the encoder's own inverse, so the
round trip succeeded for every input by construction and no test could
have failed on an encoding mistake. `/oauth/revoke` now runs a genuine
RFC 6749 Appendix B decode, with unit cases for `+`, a space, `:` and
`%` (asserted on the wire and through an independent decoder), an
end-to-end client whose secret is base64 with a `+` in it, and a guard
that an unencoded credential is refused — the case that separates the
two decoders.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BhVaufrEZAfFAXcg6456hK
Signed-off-by: cliffhall <cliff@futurescale.com>
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>
