fix(auth): address Copilot review round 1 on the RFC 7009 revocation (#2144)
- 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>
This commit is contained in:
cliffhall
parent
15f381c6ad
commit
7f27d7e9c7
10 changed files with 617 additions and 87 deletions
|
|
@ -128,11 +128,10 @@ describe("clearStoredAuthForRelogin", () => {
|
|||
expect(blob.servers["https://example.com/mcp"]).toBeUndefined();
|
||||
});
|
||||
|
||||
// Both key spellings are cleared, but they are two spellings of one server:
|
||||
// a second request would name a grant the first one already ended. The raw
|
||||
// key here holds nothing, so this also proves the walk does not stop at the
|
||||
// first empty one.
|
||||
it("sends one request even though both key spellings are cleared", async () => {
|
||||
// Both spellings resolve to the same stored entry, so this is one grant and
|
||||
// must produce one request. It also proves the walk does not stop at the
|
||||
// first empty key: the raw spelling holds nothing.
|
||||
it("sends one request when both key spellings name the same grant", async () => {
|
||||
seed();
|
||||
const fetchSpy = vi
|
||||
.spyOn(globalThis, "fetch")
|
||||
|
|
@ -148,6 +147,75 @@ describe("clearStoredAuthForRelogin", () => {
|
|||
}
|
||||
});
|
||||
|
||||
// Both keys are deleted, so both must be revoked: a stale entry under the
|
||||
// other spelling is still a live grant at the authorization server, and
|
||||
// dropping it locally without revoking is the exact leak this closes.
|
||||
// The *reported* outcome is the normalised key's, matching the precedence
|
||||
// `findStoredServerState` reads with — that is the grant a connect uses.
|
||||
it("revokes a stale entry under the other spelling too", async () => {
|
||||
dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-both-"));
|
||||
const file = path.join(dir, "oauth.json");
|
||||
const metadata = {
|
||||
issuer: "https://as.example.com",
|
||||
authorization_endpoint: "https://as.example.com/authorize",
|
||||
token_endpoint: "https://as.example.com/token",
|
||||
revocation_endpoint: "https://as.example.com/revoke",
|
||||
response_types_supported: ["code"],
|
||||
};
|
||||
fs.writeFileSync(
|
||||
file,
|
||||
JSON.stringify({
|
||||
servers: {
|
||||
// The raw spelling the transport keyed by, holding a STALE grant.
|
||||
"https://example.com": {
|
||||
tokens: {
|
||||
access_token: "stale-a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "stale-r",
|
||||
},
|
||||
serverMetadata: metadata,
|
||||
},
|
||||
// The normalised spelling, holding the CURRENT grant.
|
||||
"https://example.com/": {
|
||||
tokens: {
|
||||
access_token: "live-a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "live-r",
|
||||
},
|
||||
serverMetadata: metadata,
|
||||
},
|
||||
},
|
||||
idpSessions: {},
|
||||
}),
|
||||
"utf8",
|
||||
);
|
||||
prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH;
|
||||
process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file;
|
||||
resetNodeOAuthStorageCache();
|
||||
|
||||
const fetchSpy = vi
|
||||
.spyOn(globalThis, "fetch")
|
||||
.mockResolvedValue(new Response(null, { status: 200 }));
|
||||
try {
|
||||
await expect(
|
||||
clearStoredAuthForRelogin("https://example.com"),
|
||||
).resolves.toMatchObject({ status: "revoked" });
|
||||
const tokens = fetchSpy.mock.calls.map((call) =>
|
||||
new URLSearchParams(String(call[1]?.body)).get("token"),
|
||||
);
|
||||
// Normalised first, then the stale raw one — neither is left behind.
|
||||
expect(tokens).toEqual(["live-r", "stale-r"]);
|
||||
} finally {
|
||||
fetchSpy.mockRestore();
|
||||
}
|
||||
|
||||
const blob = JSON.parse(fs.readFileSync(file, "utf8")) as {
|
||||
servers: Record<string, unknown>;
|
||||
};
|
||||
expect(blob.servers["https://example.com"]).toBeUndefined();
|
||||
expect(blob.servers["https://example.com/"]).toBeUndefined();
|
||||
});
|
||||
|
||||
it("deletes the local entry even when the request fails", async () => {
|
||||
const file = seed();
|
||||
const fetchSpy = vi
|
||||
|
|
|
|||
205
clients/cli/__tests__/relogin-revocation.test.ts
Normal file
205
clients/cli/__tests__/relogin-revocation.test.ts
Normal file
|
|
@ -0,0 +1,205 @@
|
|||
/**
|
||||
* `--relogin` token revocation, driven through `runCli` (#2144).
|
||||
*
|
||||
* The helper's own tests cover `clearStoredAuthForRelogin({ revoke })`. What
|
||||
* they cannot cover is the wiring above it: that Commander maps `--no-revoke`,
|
||||
* that the per-server `oauth.revokeOnClear` reaches the call, and that a failed
|
||||
* revocation is reported without changing what `--relogin` does. A regression
|
||||
* in any of those turns revocation silently back on or off, which is precisely
|
||||
* the class of bug the opt-out exists to make controllable.
|
||||
*
|
||||
* Every case here fails to connect on purpose — the fetch double refuses
|
||||
* everything but the revocation endpoint. That is fine: revocation runs before
|
||||
* the connect, so the assertions are about the requests made and the warnings
|
||||
* printed, not about the exit code.
|
||||
*/
|
||||
import { describe, it, expect, afterEach, vi } from "vitest";
|
||||
import * as fs from "node:fs";
|
||||
import * as os from "node:os";
|
||||
import * as path from "node:path";
|
||||
import { resetNodeOAuthStorageCache } from "@inspector/core/auth/node/storage-node.js";
|
||||
import { runCli } from "./helpers/cli-runner.js";
|
||||
|
||||
const SERVER_URL = "https://example.com/mcp";
|
||||
const REVOKE_URL = "https://as.example.com/revoke";
|
||||
|
||||
let dir: string | undefined;
|
||||
let prevPath: string | undefined;
|
||||
|
||||
afterEach(() => {
|
||||
if (prevPath === undefined) delete process.env.MCP_INSPECTOR_OAUTH_STATE_PATH;
|
||||
else process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = prevPath;
|
||||
prevPath = undefined;
|
||||
resetNodeOAuthStorageCache();
|
||||
vi.restoreAllMocks();
|
||||
if (dir) {
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
dir = undefined;
|
||||
}
|
||||
});
|
||||
|
||||
/** Seed an OAuth store holding a revocable grant for {@link SERVER_URL}. */
|
||||
function seedStore(): void {
|
||||
dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-revoke-e2e-"));
|
||||
const file = path.join(dir, "oauth.json");
|
||||
fs.writeFileSync(
|
||||
file,
|
||||
JSON.stringify({
|
||||
servers: {
|
||||
[SERVER_URL]: {
|
||||
tokens: {
|
||||
access_token: "a",
|
||||
token_type: "Bearer",
|
||||
refresh_token: "r",
|
||||
},
|
||||
serverMetadata: {
|
||||
issuer: "https://as.example.com",
|
||||
authorization_endpoint: "https://as.example.com/authorize",
|
||||
token_endpoint: "https://as.example.com/token",
|
||||
revocation_endpoint: REVOKE_URL,
|
||||
response_types_supported: ["code"],
|
||||
},
|
||||
},
|
||||
},
|
||||
idpSessions: {},
|
||||
}),
|
||||
"utf8",
|
||||
);
|
||||
prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH;
|
||||
process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file;
|
||||
resetNodeOAuthStorageCache();
|
||||
}
|
||||
|
||||
/**
|
||||
* Answer the revocation endpoint with `status` and refuse everything else, so
|
||||
* the run stops at the connect instead of reaching the network.
|
||||
*/
|
||||
function stubFetch(status = 200): ReturnType<typeof vi.spyOn> {
|
||||
return vi.spyOn(globalThis, "fetch").mockImplementation((input) => {
|
||||
if (String(input instanceof Request ? input.url : input) === REVOKE_URL) {
|
||||
return Promise.resolve(new Response(null, { status }));
|
||||
}
|
||||
return Promise.reject(new Error("network blocked in test"));
|
||||
}) as ReturnType<typeof vi.spyOn>;
|
||||
}
|
||||
|
||||
function revocationCalls(spy: { mock: { calls: unknown[][] } }): unknown[][] {
|
||||
return spy.mock.calls.filter(
|
||||
(call) => String(call[0] as string) === REVOKE_URL,
|
||||
);
|
||||
}
|
||||
|
||||
/** A catalog holding one HTTP server, optionally opting out of revocation. */
|
||||
function writeCatalog(revokeOnClear?: boolean): string {
|
||||
const catalogDir = fs.mkdtempSync(
|
||||
path.join(os.tmpdir(), "cli-relogin-catalog-"),
|
||||
);
|
||||
const catalogPath = path.join(catalogDir, "mcp.json");
|
||||
fs.writeFileSync(
|
||||
catalogPath,
|
||||
JSON.stringify({
|
||||
mcpServers: {
|
||||
remote: {
|
||||
type: "streamable-http",
|
||||
url: SERVER_URL,
|
||||
...(revokeOnClear !== undefined && { oauth: { revokeOnClear } }),
|
||||
},
|
||||
},
|
||||
}),
|
||||
"utf8",
|
||||
);
|
||||
return catalogPath;
|
||||
}
|
||||
|
||||
describe("--relogin token revocation", () => {
|
||||
it("revokes the stored grant by default", async () => {
|
||||
seedStore();
|
||||
const fetchSpy = stubFetch();
|
||||
|
||||
await runCli([
|
||||
"--relogin",
|
||||
"--server-url",
|
||||
SERVER_URL,
|
||||
"--method",
|
||||
"tools/list",
|
||||
]);
|
||||
|
||||
expect(revocationCalls(fetchSpy)).toHaveLength(1);
|
||||
});
|
||||
|
||||
// Commander turns `--no-revoke` into `options.revoke === false`; nothing else
|
||||
// proves that mapping survives a refactor of the flag.
|
||||
it("--no-revoke skips the request", async () => {
|
||||
seedStore();
|
||||
const fetchSpy = stubFetch();
|
||||
|
||||
await runCli([
|
||||
"--relogin",
|
||||
"--no-revoke",
|
||||
"--server-url",
|
||||
SERVER_URL,
|
||||
"--method",
|
||||
"tools/list",
|
||||
]);
|
||||
|
||||
expect(revocationCalls(fetchSpy)).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("honors the per-server oauth.revokeOnClear opt-out", async () => {
|
||||
seedStore();
|
||||
const catalogPath = writeCatalog(false);
|
||||
const fetchSpy = stubFetch();
|
||||
try {
|
||||
await runCli([
|
||||
"--catalog",
|
||||
catalogPath,
|
||||
"--server",
|
||||
"remote",
|
||||
"--relogin",
|
||||
"--method",
|
||||
"tools/list",
|
||||
]);
|
||||
expect(revocationCalls(fetchSpy)).toHaveLength(0);
|
||||
} finally {
|
||||
fs.rmSync(path.dirname(catalogPath), { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("revokes for a catalog server that did not opt out", async () => {
|
||||
seedStore();
|
||||
const catalogPath = writeCatalog();
|
||||
const fetchSpy = stubFetch();
|
||||
try {
|
||||
await runCli([
|
||||
"--catalog",
|
||||
catalogPath,
|
||||
"--server",
|
||||
"remote",
|
||||
"--relogin",
|
||||
"--method",
|
||||
"tools/list",
|
||||
]);
|
||||
expect(revocationCalls(fetchSpy)).toHaveLength(1);
|
||||
} finally {
|
||||
fs.rmSync(path.dirname(catalogPath), { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
// A failed revocation must be visible — the grant is still live at the
|
||||
// authorization server — without turning `--relogin` into a failure, which
|
||||
// is a local delete the user still gets.
|
||||
it("warns on stderr when the authorization server refuses", async () => {
|
||||
seedStore();
|
||||
stubFetch(500);
|
||||
|
||||
const result = await runCli([
|
||||
"--relogin",
|
||||
"--server-url",
|
||||
SERVER_URL,
|
||||
"--method",
|
||||
"tools/list",
|
||||
]);
|
||||
|
||||
expect(result.stderr).toMatch(/could not revoke the OAuth grant/i);
|
||||
});
|
||||
});
|
||||
|
|
@ -4,6 +4,7 @@ import {
|
|||
} from "@inspector/core/auth/node/storage-node.js";
|
||||
import {
|
||||
revokeStoredOAuthTokens,
|
||||
selectRevocableToken,
|
||||
type TokenRevocationOutcome,
|
||||
} from "@inspector/core/auth/revocation.js";
|
||||
import { createProxyFetch } from "@inspector/core/mcp/node/proxyFetch.js";
|
||||
|
|
@ -40,13 +41,17 @@ export async function clearStoredAuthForRelogin(
|
|||
// this is about to empty. Best-effort — the outcome is returned for the
|
||||
// caller to report, never thrown, so `--relogin` succeeds regardless.
|
||||
//
|
||||
// Only the key that actually holds the state is revoked from: the two keys
|
||||
// below are two spellings of one server, so revoking from both would send a
|
||||
// second request for a grant the first one already ended.
|
||||
// Both spellings are cleared below, so both are revoked from — a stale entry
|
||||
// under the other key is a live grant at the authorization server, and
|
||||
// deleting it locally without revoking is exactly the leak this closes. The
|
||||
// normalised key goes first because that is the precedence `findStoredServerState`
|
||||
// reads with, so the grant actually in use is the one whose outcome is
|
||||
// reported; a second key holding the *same* token is skipped rather than
|
||||
// revoked twice.
|
||||
const revocation =
|
||||
options?.revoke === false
|
||||
? undefined
|
||||
: await revokeFirstStoredKey(storage, [raw, normalized]);
|
||||
: await revokeStoredKeys(storage, [normalized, raw]);
|
||||
await storage.clear(raw);
|
||||
if (normalized !== raw) {
|
||||
await storage.clear(normalized);
|
||||
|
|
@ -58,27 +63,43 @@ export async function clearStoredAuthForRelogin(
|
|||
}
|
||||
|
||||
/**
|
||||
* Revoke against the first of `keys` that actually has a revocable token,
|
||||
* returning that attempt's outcome. Reports the last "nothing to do" answer
|
||||
* when no key holds one, so the caller can still distinguish "no tokens" from
|
||||
* "this authorization server advertises no revocation endpoint".
|
||||
* Revoke every distinct grant held under `keys`, in order, and report the first
|
||||
* key that had something to revoke.
|
||||
*
|
||||
* Both keys are about to be deleted, so both are revoked from: a stale entry
|
||||
* under the other spelling is still a live grant at the authorization server,
|
||||
* and deleting it locally without revoking is the leak this whole change
|
||||
* closes. Two keys holding the *same* token are one grant, so the duplicate is
|
||||
* skipped rather than producing a second request for something already ended.
|
||||
*
|
||||
* The reported outcome is the first key's that was not "nothing to do", which
|
||||
* with `keys` in `findStoredServerState` precedence means the grant the CLI
|
||||
* would actually have connected with. When no key holds a token, the last
|
||||
* "nothing to do" answer is returned so the caller can still tell "no tokens"
|
||||
* from "this authorization server advertises no revocation endpoint".
|
||||
*/
|
||||
async function revokeFirstStoredKey(
|
||||
async function revokeStoredKeys(
|
||||
storage: NodeOAuthStorage,
|
||||
keys: string[],
|
||||
): Promise<TokenRevocationOutcome | undefined> {
|
||||
const fetchFn = createProxyFetch() ?? fetch;
|
||||
let last: TokenRevocationOutcome | undefined;
|
||||
const revokedTokens = new Set<string>();
|
||||
let reported: TokenRevocationOutcome | undefined;
|
||||
let lastSkip: TokenRevocationOutcome | undefined;
|
||||
for (const key of new Set(keys)) {
|
||||
const token = selectRevocableToken(await storage.getTokens(key))?.token;
|
||||
if (token !== undefined && revokedTokens.has(token)) continue;
|
||||
const outcome = await revokeStoredOAuthTokens({
|
||||
serverUrl: key,
|
||||
storage,
|
||||
fetchFn,
|
||||
});
|
||||
if (!(outcome.status === "skipped" && outcome.reason === "no_tokens")) {
|
||||
return outcome;
|
||||
if (outcome.status === "skipped" && outcome.reason === "no_tokens") {
|
||||
lastSkip = outcome;
|
||||
continue;
|
||||
}
|
||||
last = outcome;
|
||||
if (token !== undefined) revokedTokens.add(token);
|
||||
reported ??= outcome;
|
||||
}
|
||||
return last;
|
||||
return reported ?? lastSkip;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -360,6 +360,26 @@ function oneHttp(): Record<string, TuiServer> {
|
|||
};
|
||||
}
|
||||
|
||||
/** An HTTP server that opted out of RFC 7009 revocation on clear (#2144). */
|
||||
function oneHttpNoRevoke(): Record<string, TuiServer> {
|
||||
return {
|
||||
web: {
|
||||
config: { type: "streamable-http", url: "http://x" },
|
||||
settings: {
|
||||
requestTimeout: 0,
|
||||
metadata: {},
|
||||
headers: [],
|
||||
env: [],
|
||||
roots: [],
|
||||
maxFetchRequests: 1000,
|
||||
taskTtl: 0,
|
||||
connectionTimeout: 0,
|
||||
oauthRevokeOnClear: false,
|
||||
},
|
||||
} as never,
|
||||
};
|
||||
}
|
||||
|
||||
function oneEmaHttp(): Record<string, TuiServer> {
|
||||
return {
|
||||
ema: {
|
||||
|
|
@ -666,6 +686,12 @@ beforeEach(() => {
|
|||
new URL("https://auth.example/start"),
|
||||
);
|
||||
h.clientSpies.clearOAuthTokens.mockReset();
|
||||
// #2144: the clear now reads the returned revocation outcome, so the reset
|
||||
// default has to be an outcome rather than `undefined`.
|
||||
h.clientSpies.clearOAuthTokens.mockResolvedValue({
|
||||
status: "skipped",
|
||||
reason: "no_endpoint",
|
||||
});
|
||||
h.clientSpies.completeOAuthFlow.mockReset();
|
||||
h.clientSpies.completeOAuthFlow.mockResolvedValue(undefined);
|
||||
h.clientSpies.getOAuthState.mockReset();
|
||||
|
|
@ -1351,6 +1377,39 @@ describe("App (mid-session auth lifecycle events)", () => {
|
|||
expect(h.disconnect).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// #2144 — revocation is on unless the entry opted out, and the opt-out has to
|
||||
// reach the client, since that is where the RFC 7009 request is made.
|
||||
it("asks for revocation by default when clearing", async () => {
|
||||
const r = await mount(oneHttp());
|
||||
await press(r, ["a", "s"]);
|
||||
await waitUntil(() => h.clientSpies.clearOAuthTokens.mock.calls.length > 0);
|
||||
expect(h.clientSpies.clearOAuthTokens).toHaveBeenCalledWith({
|
||||
revoke: true,
|
||||
});
|
||||
});
|
||||
|
||||
it("forwards the per-server revocation opt-out", async () => {
|
||||
const r = await mount(oneHttpNoRevoke());
|
||||
await press(r, ["a", "s"]);
|
||||
await waitUntil(() => h.clientSpies.clearOAuthTokens.mock.calls.length > 0);
|
||||
expect(h.clientSpies.clearOAuthTokens).toHaveBeenCalledWith({
|
||||
revoke: false,
|
||||
});
|
||||
});
|
||||
|
||||
// A failed revocation leaves the grant live at the authorization server, so
|
||||
// it has to be visible rather than swallowed — the local clear succeeded and
|
||||
// would otherwise look like the whole operation did.
|
||||
it("reports a failed revocation without failing the clear", async () => {
|
||||
h.clientSpies.clearOAuthTokens.mockResolvedValue({
|
||||
status: "failed",
|
||||
detail: "unreachable",
|
||||
});
|
||||
const r = await mount(oneHttp());
|
||||
await press(r, ["a", "s"]);
|
||||
await expectFrame(r, "unreachable");
|
||||
});
|
||||
|
||||
const stepUpChallenge = {
|
||||
reason: "insufficient_scope" as const,
|
||||
requiredScopes: ["env:read"],
|
||||
|
|
|
|||
|
|
@ -136,7 +136,10 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
|
|||
}
|
||||
return Promise.resolve(true);
|
||||
});
|
||||
clearOAuthTokens = vi.fn().mockResolvedValue(undefined);
|
||||
// #2144: the clear path reads the returned RFC 7009 outcome.
|
||||
clearOAuthTokens = vi
|
||||
.fn()
|
||||
.mockResolvedValue({ status: "skipped", reason: "no_endpoint" });
|
||||
}
|
||||
const instances: FakeInspectorClient[] = [];
|
||||
return {
|
||||
|
|
|
|||
|
|
@ -1326,8 +1326,18 @@ function App() {
|
|||
|
||||
const handleClearStoredOAuthFromSettings = useCallback(() => {
|
||||
if (!settingsModalTarget) return;
|
||||
void clearServerOAuthAndDisconnect(settingsModalTarget);
|
||||
}, [settingsModalTarget, clearServerOAuthAndDisconnect]);
|
||||
// Clear from *inside* the settings modal, so the draft is what the user is
|
||||
// looking at: `settingsModalTarget` comes from the persisted `servers`
|
||||
// list, and edits only reach it after the save debounce. Reading it would
|
||||
// mean toggling "Revoke tokens on clear" and immediately clearing used the
|
||||
// previous value — revoking despite an opt-out, or skipping despite an
|
||||
// opt-in (#2144). The draft is the live answer; `settingsModalValue` falls
|
||||
// back to the persisted entry whenever there is no draft.
|
||||
void clearServerOAuthAndDisconnect({
|
||||
...settingsModalTarget,
|
||||
settings: settingsDraft ?? settingsModalTarget.settings,
|
||||
});
|
||||
}, [settingsModalTarget, settingsDraft, clearServerOAuthAndDisconnect]);
|
||||
|
||||
const onSettingsModalClose = useCallback(() => {
|
||||
flushSettingsDraft();
|
||||
|
|
|
|||
|
|
@ -9,6 +9,22 @@ import {
|
|||
revokeToken,
|
||||
selectRevocableToken,
|
||||
} from "@inspector/core/auth/revocation.js";
|
||||
import type { InspectorLogger } from "@inspector/core/logging/index.js";
|
||||
|
||||
/** A fully-typed `InspectorLogger` double, so the mock's shape is checked. */
|
||||
function fakeLogger(): InspectorLogger {
|
||||
return {
|
||||
level: "info",
|
||||
fatal: vi.fn(),
|
||||
error: vi.fn(),
|
||||
warn: vi.fn(),
|
||||
info: vi.fn(),
|
||||
debug: vi.fn(),
|
||||
trace: vi.fn(),
|
||||
silent: vi.fn(),
|
||||
child: vi.fn(() => fakeLogger()),
|
||||
};
|
||||
}
|
||||
|
||||
const SERVER_URL = "https://mcp.example.com/mcp";
|
||||
const REVOKE_URL = "https://as.example.com/revoke";
|
||||
|
|
@ -72,19 +88,47 @@ describe("revocationAuthMethods", () => {
|
|||
).toEqual(["client_secret_post"]);
|
||||
});
|
||||
|
||||
// RFC 8414 gives the revocation endpoint the token endpoint's default when it
|
||||
// advertises no list of its own.
|
||||
it("falls back to the token endpoint's list", () => {
|
||||
// RFC 8414 §2 defaults an omitted revocation list to `client_secret_basic` —
|
||||
// it does NOT inherit the token endpoint's. Inheriting would send POST
|
||||
// credentials to an endpoint that never advertised that method.
|
||||
it("ignores the token endpoint's list", () => {
|
||||
expect(
|
||||
revocationAuthMethods(
|
||||
metadata({ token_endpoint_auth_methods_supported: ["none"] }),
|
||||
metadata({
|
||||
token_endpoint_auth_methods_supported: ["client_secret_post"],
|
||||
}),
|
||||
),
|
||||
).toEqual(["none"]);
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it("yields nothing when neither is advertised", () => {
|
||||
it("yields nothing when the revocation list is absent", () => {
|
||||
expect(revocationAuthMethods(metadata())).toEqual([]);
|
||||
});
|
||||
|
||||
// The empty list is not "no authentication": it is what makes the SDK apply
|
||||
// RFC 8414's actual default.
|
||||
it("an empty list resolves to the RFC 8414 default", () => {
|
||||
const { init } = buildRevocationRequest({
|
||||
endpoint: REVOKE_URL,
|
||||
token: "r",
|
||||
tokenTypeHint: "refresh_token",
|
||||
clientInformation: { client_id: "cid", client_secret: "sec" },
|
||||
supportedAuthMethods: revocationAuthMethods(metadata()),
|
||||
});
|
||||
expect(headerOf(init, "Authorization")).toBe(`Basic ${btoa("cid:sec")}`);
|
||||
});
|
||||
|
||||
it("an empty list leaves a public client unauthenticated", () => {
|
||||
const { init } = buildRevocationRequest({
|
||||
endpoint: REVOKE_URL,
|
||||
token: "r",
|
||||
tokenTypeHint: "refresh_token",
|
||||
clientInformation: { client_id: "cid" },
|
||||
supportedAuthMethods: revocationAuthMethods(metadata()),
|
||||
});
|
||||
expect(headerOf(init, "Authorization")).toBeUndefined();
|
||||
expect(body(init).get("client_id")).toBe("cid");
|
||||
});
|
||||
});
|
||||
|
||||
describe("buildRevocationRequest", () => {
|
||||
|
|
@ -203,6 +247,26 @@ describe("revokeToken", () => {
|
|||
});
|
||||
});
|
||||
|
||||
// The web fetch is `createRemoteFetch`, which re-issues the call as a POST to
|
||||
// `/api/fetch` and drops `init.signal`; the backend's outbound fetch gets no
|
||||
// signal either. A signal-only bound is therefore inert on exactly the path
|
||||
// the timeout exists for, so the deadline has to hold against a fetch that
|
||||
// ignores the signal entirely.
|
||||
it("gives up on a fetch that never settles and ignores the signal", async () => {
|
||||
const outcome = await revokeToken({
|
||||
endpoint: REVOKE_URL,
|
||||
token: "r",
|
||||
tokenTypeHint: "refresh_token",
|
||||
supportedAuthMethods: [],
|
||||
fetchFn: () => new Promise<Response>(() => {}),
|
||||
timeoutMs: 20,
|
||||
});
|
||||
expect(outcome).toMatchObject({ status: "failed", endpoint: REVOKE_URL });
|
||||
expect(outcome.status === "failed" ? outcome.detail : "").toContain(
|
||||
"timed out",
|
||||
);
|
||||
});
|
||||
|
||||
// The teardown is already committed by the time this runs, so a wedged
|
||||
// authorization server must not be able to hold it open.
|
||||
it("bounds the request with an abort signal", async () => {
|
||||
|
|
@ -334,18 +398,7 @@ describe("revokeStoredOAuthTokens", () => {
|
|||
|
||||
it("warns and reports rather than throwing when the request fails", async () => {
|
||||
await seed();
|
||||
const warn = vi.fn();
|
||||
const logger = {
|
||||
level: "info",
|
||||
fatal: vi.fn(),
|
||||
error: vi.fn(),
|
||||
warn,
|
||||
info: vi.fn(),
|
||||
debug: vi.fn(),
|
||||
trace: vi.fn(),
|
||||
silent: vi.fn(),
|
||||
child: vi.fn(),
|
||||
};
|
||||
const logger = fakeLogger();
|
||||
const fetchFn = vi.fn<typeof fetch>(
|
||||
async () => new Response(null, { status: 500, statusText: "Boom" }),
|
||||
);
|
||||
|
|
@ -354,52 +407,38 @@ describe("revokeStoredOAuthTokens", () => {
|
|||
serverUrl: SERVER_URL,
|
||||
storage,
|
||||
fetchFn,
|
||||
logger: logger as unknown as Parameters<
|
||||
typeof revokeStoredOAuthTokens
|
||||
>[0]["logger"],
|
||||
logger,
|
||||
});
|
||||
|
||||
expect(outcome.status).toBe("failed");
|
||||
expect(warn).toHaveBeenCalledTimes(1);
|
||||
expect(logger.warn).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("logs the no-endpoint case at debug", async () => {
|
||||
await seed({ revocation_endpoint: undefined });
|
||||
const debug = vi.fn();
|
||||
const logger = {
|
||||
level: "info",
|
||||
fatal: vi.fn(),
|
||||
error: vi.fn(),
|
||||
warn: vi.fn(),
|
||||
info: vi.fn(),
|
||||
debug,
|
||||
trace: vi.fn(),
|
||||
silent: vi.fn(),
|
||||
child: vi.fn(),
|
||||
};
|
||||
const logger = fakeLogger();
|
||||
await revokeStoredOAuthTokens({
|
||||
serverUrl: SERVER_URL,
|
||||
storage,
|
||||
fetchFn: vi.fn<typeof fetch>(),
|
||||
logger: logger as unknown as Parameters<
|
||||
typeof revokeStoredOAuthTokens
|
||||
>[0]["logger"],
|
||||
logger,
|
||||
});
|
||||
expect(debug).toHaveBeenCalledTimes(1);
|
||||
expect(logger.debug).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
// A store that cannot be read is not a reason to abandon the clear the user
|
||||
// asked for, so the read is inside the try.
|
||||
// asked for, so the read is inside the try. Spying on the real instance keeps
|
||||
// the `OAuthStorage` contract intact — a spread-and-cast stand-in would type
|
||||
// as storage while being a plain object with none of its methods.
|
||||
it("reports a store read failure as failed", async () => {
|
||||
const broken = {
|
||||
...storage,
|
||||
getTokens: () => Promise.reject(new Error("store unreadable")),
|
||||
} as unknown as BrowserOAuthStorage;
|
||||
vi.spyOn(storage, "getTokens").mockRejectedValue(
|
||||
new Error("store unreadable"),
|
||||
);
|
||||
|
||||
await expect(
|
||||
revokeStoredOAuthTokens({
|
||||
serverUrl: SERVER_URL,
|
||||
storage: broken,
|
||||
storage,
|
||||
fetchFn: vi.fn<typeof fetch>(),
|
||||
}),
|
||||
).resolves.toEqual({ status: "failed", detail: "store unreadable" });
|
||||
|
|
|
|||
|
|
@ -138,6 +138,31 @@ describe("OAuth token revocation (RFC 7009)", () => {
|
|||
return storage;
|
||||
}
|
||||
|
||||
// The fixture enforces RFC 7009 §2.1 client authentication, so this is what
|
||||
// makes the passing cases below mean something: a request with the wrong
|
||||
// credentials is refused, and the endpoint is not simply answering 200 to
|
||||
// anything.
|
||||
it("refuses a revocation request with the wrong client secret", async () => {
|
||||
const response = await fetch(`${serverUrl}/oauth/revoke`, {
|
||||
method: "POST",
|
||||
headers: {
|
||||
"Content-Type": "application/x-www-form-urlencoded",
|
||||
Authorization: `Basic ${Buffer.from(`${CLIENT_ID}:wrong`).toString("base64")}`,
|
||||
},
|
||||
body: new URLSearchParams({ token: "anything" }),
|
||||
});
|
||||
expect(response.status).toBe(401);
|
||||
});
|
||||
|
||||
it("refuses a revocation request naming no client at all", async () => {
|
||||
const response = await fetch(`${serverUrl}/oauth/revoke`, {
|
||||
method: "POST",
|
||||
headers: { "Content-Type": "application/x-www-form-urlencoded" },
|
||||
body: new URLSearchParams({ token: "anything" }),
|
||||
});
|
||||
expect(response.status).toBe(401);
|
||||
});
|
||||
|
||||
it("advertises a revocation endpoint", () => {
|
||||
expect(metadata.revocation_endpoint).toBe(`${serverUrl}/oauth/revoke`);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -87,20 +87,23 @@ export function selectRevocableToken(
|
|||
}
|
||||
|
||||
/**
|
||||
* The client-authentication methods to consider for the revocation endpoint.
|
||||
* The client-authentication methods the revocation endpoint advertises.
|
||||
*
|
||||
* RFC 8414 gives `revocation_endpoint_auth_methods_supported` the same default
|
||||
* as the token endpoint's when it is omitted, so fall through to
|
||||
* `token_endpoint_auth_methods_supported` before giving up. An empty result
|
||||
* lets {@link selectClientAuthMethod} apply the SDK's own default rather than
|
||||
* this module inventing a second one.
|
||||
* RFC 8414 §2 gives `revocation_endpoint_auth_methods_supported` a default of
|
||||
* **`client_secret_basic`** when it is omitted — it does *not* inherit
|
||||
* `token_endpoint_auth_methods_supported`. Falling through to the token
|
||||
* endpoint's list would make metadata advertising only `client_secret_post`
|
||||
* there send POST credentials to a revocation endpoint that never advertised
|
||||
* that method, which a strict server rejects.
|
||||
*
|
||||
* An empty result is returned rather than a literal `["client_secret_basic"]`
|
||||
* because that is what makes {@link selectClientAuthMethod} apply exactly the
|
||||
* RFC's default — `client_secret_basic` when the client holds a secret, `none`
|
||||
* when it does not — while still honoring a `token_endpoint_auth_method` the
|
||||
* client's own registration declares.
|
||||
*/
|
||||
export function revocationAuthMethods(metadata: OAuthMetadata): string[] {
|
||||
return (
|
||||
metadata.revocation_endpoint_auth_methods_supported ??
|
||||
metadata.token_endpoint_auth_methods_supported ??
|
||||
[]
|
||||
);
|
||||
return metadata.revocation_endpoint_auth_methods_supported ?? [];
|
||||
}
|
||||
|
||||
export interface RevocationRequestParams {
|
||||
|
|
@ -195,10 +198,18 @@ export async function revokeToken(
|
|||
const { url, init } = buildRevocationRequest(params);
|
||||
const timeoutMs = params.timeoutMs ?? DEFAULT_REVOCATION_TIMEOUT_MS;
|
||||
try {
|
||||
const response = await params.fetchFn(url, {
|
||||
...init,
|
||||
signal: AbortSignal.timeout(timeoutMs),
|
||||
});
|
||||
// The signal alone is not enough to bound this. In the browser the fetch is
|
||||
// `createRemoteFetch`, which re-issues the call as a POST to `/api/fetch`
|
||||
// and does not forward `init.signal`; the backend's outbound fetch gets no
|
||||
// signal either. So a wedged authorization server would hold the teardown
|
||||
// open indefinitely on exactly the path the timeout exists for. The race is
|
||||
// what actually enforces the deadline; the signal is kept because it does
|
||||
// cancel the direct-fetch paths (CLI, TUI, backend) rather than merely
|
||||
// abandoning them.
|
||||
const response = await withDeadline(
|
||||
params.fetchFn(url, { ...init, signal: AbortSignal.timeout(timeoutMs) }),
|
||||
timeoutMs,
|
||||
);
|
||||
if (!response.ok) {
|
||||
return {
|
||||
status: "failed",
|
||||
|
|
@ -221,6 +232,34 @@ export async function revokeToken(
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Reject with a timeout error if `promise` has not settled within `timeoutMs`.
|
||||
*
|
||||
* The underlying request is abandoned rather than cancelled — nothing here can
|
||||
* cancel a fetch the proxy already stripped the signal from. That is the right
|
||||
* trade for this caller: the point is that the *teardown* proceeds, and the
|
||||
* response, if it ever arrives, is a revocation we no longer need to wait for.
|
||||
* The timer is cleared on the settled path so a caller is never held awake by it.
|
||||
*/
|
||||
async function withDeadline<T>(
|
||||
promise: Promise<T>,
|
||||
timeoutMs: number,
|
||||
): Promise<T> {
|
||||
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||
const deadline = new Promise<never>((_resolve, reject) => {
|
||||
timer = setTimeout(
|
||||
() =>
|
||||
reject(new Error(`revocation request timed out after ${timeoutMs}ms`)),
|
||||
timeoutMs,
|
||||
);
|
||||
});
|
||||
try {
|
||||
return await Promise.race([promise, deadline]);
|
||||
} finally {
|
||||
if (timer !== undefined) clearTimeout(timer);
|
||||
}
|
||||
}
|
||||
|
||||
export interface RevokeStoredOAuthTokensParams {
|
||||
serverUrl: string;
|
||||
storage: OAuthStorage;
|
||||
|
|
|
|||
|
|
@ -117,7 +117,7 @@ export function setupOAuthRoutes(
|
|||
setupAuthorizationEndpoint(app, config);
|
||||
setupTokenEndpoint(app, config);
|
||||
if (config.supportRevocation !== false) {
|
||||
setupRevocationEndpoint(app);
|
||||
setupRevocationEndpoint(app, config);
|
||||
}
|
||||
if (config.supportDCR) {
|
||||
setupDCREndpoint(app);
|
||||
|
|
@ -711,21 +711,36 @@ function setupTokenEndpoint(
|
|||
* That is why the Inspector sends one request naming the refresh token, and a
|
||||
* fixture that ignored the linkage would let a regression through silently.
|
||||
*
|
||||
* Client authentication is accepted but not enforced: the point of the fixture
|
||||
* is the revocation semantics, and every client this repo drives it with is
|
||||
* either public or has already authenticated at the token endpoint.
|
||||
* Client authentication is **enforced**, not merely accepted. RFC 7009 §2.1
|
||||
* requires it of a confidential client, and a fixture that skipped the check
|
||||
* would answer 200 to a request carrying no `Authorization` header at all — at
|
||||
* which point the end-to-end test claiming to prove the Inspector authenticates
|
||||
* correctly proves nothing. Both RFC 6749 §2.3.1 forms are accepted (Basic and
|
||||
* the request body), as is a public client identifying itself by `client_id`.
|
||||
*/
|
||||
function setupRevocationEndpoint(app: express.Application): void {
|
||||
function setupRevocationEndpoint(
|
||||
app: express.Application,
|
||||
config: OAuthConfig,
|
||||
): void {
|
||||
app.post(
|
||||
"/oauth/revoke",
|
||||
express.urlencoded({ extended: true }),
|
||||
(req: Request, res: Response) => {
|
||||
async (req: Request, res: Response) => {
|
||||
const token: unknown = req.body?.token;
|
||||
if (typeof token !== "string" || token === "") {
|
||||
res.status(400).json({ error: "invalid_request" });
|
||||
return;
|
||||
}
|
||||
|
||||
const authenticated = await authenticateRevocationClient(req, config);
|
||||
if (!authenticated) {
|
||||
res
|
||||
.status(401)
|
||||
.set("WWW-Authenticate", 'Basic realm="revoke"')
|
||||
.json({ error: "invalid_client" });
|
||||
return;
|
||||
}
|
||||
|
||||
const refreshTokenData = refreshTokens.get(token);
|
||||
if (refreshTokenData) {
|
||||
for (const accessToken of refreshTokenData.accessTokens) {
|
||||
|
|
@ -744,6 +759,52 @@ function setupRevocationEndpoint(app: express.Application): void {
|
|||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Authenticate the caller of `/oauth/revoke` (RFC 7009 §2.1).
|
||||
*
|
||||
* Credentials may arrive either way RFC 6749 §2.3.1 allows — an `Authorization:
|
||||
* Basic` header or `client_id`/`client_secret` in the form body — because the
|
||||
* Inspector picks between them from the metadata, and a fixture that only read
|
||||
* one would silently pass a request whose credentials went to the other place.
|
||||
*
|
||||
* A request naming no client at all is rejected: the Inspector always sends at
|
||||
* least `client_id` once it holds any client information, so an unidentified
|
||||
* request means it lost track of its credentials.
|
||||
*/
|
||||
async function authenticateRevocationClient(
|
||||
req: Request,
|
||||
config: OAuthConfig,
|
||||
): Promise<boolean> {
|
||||
let clientId: string | undefined;
|
||||
let clientSecret: string | undefined;
|
||||
|
||||
const authorization = req.get("authorization");
|
||||
if (authorization?.startsWith("Basic ")) {
|
||||
const decoded = Buffer.from(
|
||||
authorization.slice("Basic ".length),
|
||||
"base64",
|
||||
).toString("utf8");
|
||||
const separator = decoded.indexOf(":");
|
||||
if (separator === -1) return false;
|
||||
clientId = decoded.slice(0, separator);
|
||||
clientSecret = decoded.slice(separator + 1);
|
||||
} else {
|
||||
const bodyId: unknown = req.body?.client_id;
|
||||
const bodySecret: unknown = req.body?.client_secret;
|
||||
if (typeof bodyId === "string") clientId = bodyId;
|
||||
if (typeof bodySecret === "string") clientSecret = bodySecret;
|
||||
}
|
||||
|
||||
if (!clientId) return false;
|
||||
const client = await findClient(clientId, config);
|
||||
if (!client) return false;
|
||||
// A client registered with a secret must present it; a public one must not be
|
||||
// asked for one it never had.
|
||||
return client.clientSecret === undefined
|
||||
? true
|
||||
: clientSecret === client.clientSecret;
|
||||
}
|
||||
|
||||
/**
|
||||
* Set up Dynamic Client Registration endpoint
|
||||
*/
|
||||
|
|
|
|||
Loading…
Reference in a new issue