feat(auth): revoke OAuth tokens at the authorization server on clear (#2144)
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>
This commit is contained in:
parent
baff393675
commit
65fd2b9e35
5 changed files with 2159 additions and 46 deletions
|
|
@ -238,6 +238,7 @@ import {
|
|||
type HandleAuthChallengeOptions,
|
||||
} from "../auth/challenge.js";
|
||||
import { withOAuthEndpointOverrides } from "../auth/endpointOverrides.js";
|
||||
import type { TokenRevocationOutcome } from "../auth/revocation.js";
|
||||
import type { OAuthTokens } from "@modelcontextprotocol/client";
|
||||
import { silentLogger, type InspectorLogger } from "../logging/logger.js";
|
||||
import { createFetchTracker } from "./fetchTracking.js";
|
||||
|
|
@ -813,6 +814,7 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
return Promise.resolve();
|
||||
},
|
||||
initialConfig: oauthConfig,
|
||||
logger: this.logger,
|
||||
enterpriseManagedAuth: options.enterpriseManagedAuth,
|
||||
installEnterpriseManagedAuth: options.installEnterpriseManagedAuth,
|
||||
dispatchOAuthComplete: (detail) =>
|
||||
|
|
@ -6722,8 +6724,25 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
/**
|
||||
* Clears OAuth tokens and client information
|
||||
*/
|
||||
async clearOAuthTokens(): Promise<void> {
|
||||
await this.oauthManager?.clearOAuthTokens();
|
||||
/**
|
||||
* Drop this server's stored OAuth state, revoking the grant at the
|
||||
* authorization server first (RFC 7009, #2144).
|
||||
*
|
||||
* The revocation is best-effort — an authorization server that advertises no
|
||||
* `revocation_endpoint` is left behaving exactly as before, and a network
|
||||
* error, a non-2xx or a timeout is reported in the returned outcome rather
|
||||
* than thrown — so the local clear always completes. Pass
|
||||
* `{ revoke: false }` to skip it.
|
||||
*/
|
||||
async clearOAuthTokens(options?: {
|
||||
revoke?: boolean;
|
||||
}): Promise<TokenRevocationOutcome> {
|
||||
return (
|
||||
(await this.oauthManager?.clearOAuthTokens(options)) ?? {
|
||||
status: "skipped",
|
||||
reason: "no_tokens",
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -13,6 +13,11 @@ import type { OAuthClientInformation } from "@modelcontextprotocol/client";
|
|||
import { mcpAuth } from "../auth/mcpAuth.js";
|
||||
import type { OAuthStorage } from "../auth/storage.js";
|
||||
import { parseOAuthState } from "../auth/utils.js";
|
||||
import {
|
||||
revokeStoredOAuthTokens,
|
||||
type TokenRevocationOutcome,
|
||||
} from "../auth/revocation.js";
|
||||
import type { InspectorLogger } from "../logging/index.js";
|
||||
import type { EnterpriseManagedAuthIdpConfig } from "../client/types.js";
|
||||
import type { ClientConfig } from "../client/types.js";
|
||||
import { EmaClientNotConfiguredError } from "../auth/ema/clientConfigError.js";
|
||||
|
|
@ -71,6 +76,8 @@ export interface OAuthManagerParams {
|
|||
dispatchOAuthComplete: (detail: { tokens: OAuthTokens }) => void;
|
||||
dispatchOAuthAuthorizationRequired: (detail: { url: URL }) => void;
|
||||
dispatchOAuthError: (detail: { error: Error }) => void;
|
||||
/** Used for the best-effort RFC 7009 revocation warning (#2144). */
|
||||
logger?: InspectorLogger;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -488,16 +495,42 @@ export class OAuthManager {
|
|||
}
|
||||
}
|
||||
|
||||
async clearOAuthTokens(): Promise<void> {
|
||||
/**
|
||||
* Revoke the grant at the authorization server (RFC 7009), then drop the
|
||||
* local OAuth state.
|
||||
*
|
||||
* Revocation runs **first** and reads what it needs out of the same store the
|
||||
* next line wipes — the token, the client credentials and the discovered
|
||||
* `revocation_endpoint` all live there, so after the clear there is nothing
|
||||
* left to revoke with. It is best-effort by construction: every failure is
|
||||
* reported through the returned outcome and the clear proceeds regardless,
|
||||
* because forgetting the tokens is what the caller actually asked for (#2144).
|
||||
*
|
||||
* `options.revoke === false` skips the request. That is not only an escape
|
||||
* hatch for an authorization server that mishandles it — disconnecting while
|
||||
* still holding live tokens is a case a user may want to reproduce
|
||||
* deliberately, to watch how a server under test copes with it.
|
||||
*/
|
||||
async clearOAuthTokens(options?: {
|
||||
revoke?: boolean;
|
||||
}): Promise<TokenRevocationOutcome> {
|
||||
if (!this.oauthConfig?.storage) {
|
||||
return;
|
||||
return { status: "skipped", reason: "no_tokens" };
|
||||
}
|
||||
|
||||
const serverUrl = this.getServerUrl();
|
||||
const outcome = await revokeStoredOAuthTokens({
|
||||
serverUrl,
|
||||
storage: this.oauthConfig.storage,
|
||||
fetchFn: this.params.effectiveAuthFetch,
|
||||
enabled: options?.revoke,
|
||||
logger: this.params.logger,
|
||||
});
|
||||
await this.oauthConfig.storage.clear(serverUrl);
|
||||
|
||||
this.oauthFlowState = null;
|
||||
this.pendingAuthorizationScope = undefined;
|
||||
return outcome;
|
||||
}
|
||||
|
||||
async isOAuthAuthorized(): Promise<boolean> {
|
||||
|
|
|
|||
|
|
@ -1392,6 +1392,7 @@ export function createRemoteApp(
|
|||
tokenUrl?: string;
|
||||
enterpriseManaged?: boolean;
|
||||
requestRefreshToken?: boolean;
|
||||
revokeOnClear?: boolean;
|
||||
} => {
|
||||
if (v === null || typeof v !== "object" || Array.isArray(v)) return false;
|
||||
const o = v as Record<string, unknown>;
|
||||
|
|
@ -1427,6 +1428,11 @@ export function createRemoteApp(
|
|||
) {
|
||||
return false;
|
||||
}
|
||||
// #2144 — shape only, same as the flag above: the read side keeps just an
|
||||
// explicit `false`, so a stray `true` reads back as the default anyway.
|
||||
if (o.revokeOnClear !== undefined && typeof o.revokeOnClear !== "boolean") {
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
};
|
||||
// A roots array: each entry must have a string `uri` and, when present, a
|
||||
|
|
@ -1540,7 +1546,7 @@ export function createRemoteApp(
|
|||
if ("oauth" in valObj && !isOauthObject(valObj.oauth)) {
|
||||
logWarn(
|
||||
{ route: "/api/servers", id, droppedKey: "oauth" },
|
||||
"Dropping malformed `oauth` field — expected `{ clientId?, clientSecret?, scopes?, authorizationParams?, authorizationUrl?, tokenUrl?, enterpriseManaged?, onInsufficientScope?, requestRefreshToken? }`.",
|
||||
"Dropping malformed `oauth` field — expected `{ clientId?, clientSecret?, scopes?, authorizationParams?, authorizationUrl?, tokenUrl?, enterpriseManaged?, onInsufficientScope?, requestRefreshToken?, revokeOnClear? }`.",
|
||||
);
|
||||
delete valObj.oauth;
|
||||
}
|
||||
|
|
@ -1852,6 +1858,15 @@ export function createRemoteApp(
|
|||
error: "settings.oauthRequestRefreshToken must be a boolean",
|
||||
};
|
||||
}
|
||||
if (
|
||||
obj.oauthRevokeOnClear !== undefined &&
|
||||
typeof obj.oauthRevokeOnClear !== "boolean"
|
||||
) {
|
||||
return {
|
||||
ok: false,
|
||||
error: "settings.oauthRevokeOnClear must be a boolean",
|
||||
};
|
||||
}
|
||||
if (
|
||||
obj.oauthOnInsufficientScope !== undefined &&
|
||||
obj.oauthOnInsufficientScope !== "reauthorize" &&
|
||||
|
|
@ -1984,6 +1999,10 @@ export function createRemoteApp(
|
|||
if (obj.oauthRequestRefreshToken === false) {
|
||||
value.oauthRequestRefreshToken = false;
|
||||
}
|
||||
// #2144: same shape — the default is on, so only the opt-out travels.
|
||||
if (obj.oauthRevokeOnClear === false) {
|
||||
value.oauthRevokeOnClear = false;
|
||||
}
|
||||
if (
|
||||
obj.oauthOnInsufficientScope === "reauthorize" ||
|
||||
obj.oauthOnInsufficientScope === "throw"
|
||||
|
|
|
|||
|
|
@ -617,6 +617,11 @@ export function storedFieldsToInspectorSettings(
|
|||
if (stored.oauth?.requestRefreshToken === false) {
|
||||
settings.oauthRequestRefreshToken = false;
|
||||
}
|
||||
// Same inverted default as the flag above: revocation is on unless the entry
|
||||
// explicitly opted out. (#2144)
|
||||
if (stored.oauth?.revokeOnClear === false) {
|
||||
settings.oauthRevokeOnClear = false;
|
||||
}
|
||||
// Mirror the stdio working directory for the form. Like the OAuth fields, an
|
||||
// empty string coerces to absent so the form's "(inherit)" placeholder shows.
|
||||
if (stored.cwd) settings.cwd = stored.cwd;
|
||||
|
|
@ -750,6 +755,10 @@ export function inspectorSettingsToStoredFields(
|
|||
if (settings.oauthRequestRefreshToken === false) {
|
||||
oauthFields.requestRefreshToken = false;
|
||||
}
|
||||
// Same omit-the-default rule as the flag above. (#2144)
|
||||
if (settings.oauthRevokeOnClear === false) {
|
||||
oauthFields.revokeOnClear = false;
|
||||
}
|
||||
if (Object.keys(oauthFields).length > 0) {
|
||||
out.oauth = oauthFields;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -193,6 +193,14 @@ export type StoredMCPServer = MCPServerConfig & {
|
|||
* Inspector-specific. (#2068)
|
||||
*/
|
||||
requestRefreshToken?: boolean;
|
||||
/**
|
||||
* Whether clearing this server's OAuth state also revokes the grant at the
|
||||
* authorization server (RFC 7009). Defaults to `true`; only `false` is
|
||||
* written to disk, so an entry that never touched the setting keeps a
|
||||
* minimal diff. See {@link InspectorServerSettings.oauthRevokeOnClear}.
|
||||
* Inspector-specific. (#2144)
|
||||
*/
|
||||
revokeOnClear?: boolean;
|
||||
/**
|
||||
* Custom query parameters appended to the OAuth **authorization request**
|
||||
* (never the token request) — e.g. Keycloak's `kc_idp_hint`, OIDC's
|
||||
|
|
@ -613,6 +621,12 @@ export interface OAuthSettings {
|
|||
* reason as `authorizationParams`; `undefined` means the default, on.
|
||||
*/
|
||||
requestRefreshToken?: boolean;
|
||||
/**
|
||||
* Whether clearing the stored OAuth state revokes the grant at the
|
||||
* authorization server (#2144). Optional for the same reason as
|
||||
* `authorizationParams`; `undefined` means the default, on.
|
||||
*/
|
||||
revokeOnClear?: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -818,6 +832,24 @@ export interface InspectorServerSettings {
|
|||
* the stored OAuth state is cleared.
|
||||
*/
|
||||
oauthRequestRefreshToken?: boolean;
|
||||
/**
|
||||
* Whether clearing this server's stored OAuth state first revokes the grant
|
||||
* at the authorization server, per RFC 7009 (#2144). `undefined` (the
|
||||
* default) means on; persisted as `oauth.revokeOnClear` only when explicitly
|
||||
* off.
|
||||
*
|
||||
* On is the right default because the alternative is silent: the Inspector
|
||||
* deletes its local copy and the access token — and the refresh token, which
|
||||
* is long-lived by design — stay valid at the authorization server until they
|
||||
* expire on their own, leaving it holding grants for sessions that ended
|
||||
* hours ago.
|
||||
*
|
||||
* Turning it off is a testing affordance rather than only an escape hatch: a
|
||||
* client that walks away still holding live tokens is a case a server author
|
||||
* may want to reproduce on purpose. It is read at clear time, not at connect
|
||||
* time, so toggling it takes effect on the next clear without reconnecting.
|
||||
*/
|
||||
oauthRevokeOnClear?: boolean;
|
||||
/**
|
||||
* When true, connect via the configured enterprise IdP (EMA) instead of
|
||||
* interactive OAuth to the MCP authorization server. Per-server OAuth
|
||||
|
|
|
|||
Loading…
Reference in a new issue