diff --git a/AGENTS.md b/AGENTS.md index 28a290e2..5dfdb218 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -101,6 +101,43 @@ v2/main/ │ │ # needs to know WHICH candidate │ │ # answered, since that is the base its │ │ # token request is made against — #2110; +│ │ # revocation.ts RFC 7009 token revocation — +│ │ # the request the three clear paths send. +│ │ # TWO halves — clearAndPlanRevocation then +│ │ # executeOAuthRevocation — and the split is +│ │ # the contract. The first TAKES the stored +│ │ # state and CLEARS it in one atomic storage +│ │ # step (OAuthStorage.takeRevocationSnapshot); +│ │ # the second sends from what was taken. +│ │ # There is no separate `storage.clear()` in +│ │ # any caller, deliberately — the clear cannot +│ │ # be forgotten or reordered. Both halves of +│ │ # that matter: the requests need state the +│ │ # clear destroys, and the clear must not wait +│ │ # on the network, or a fresh authorization +│ │ # completing during a 5s request is deleted +│ │ # by a clear reasoning about the grant it +│ │ # replaced. NOT cross-process atomic — no +│ │ # OAuth-store mutation takes a lock, and +│ │ # `clear` alone always had that property. +│ │ # Names the +│ │ # REFRESH token when there is one (§2.1 asks +│ │ # the AS to invalidate the access tokens +│ │ # under the same grant, so one request +│ │ # covers both). BEST-EFFORT by construction: +│ │ # every path returns a +│ │ # TokenRevocationOutcome rather than +│ │ # throwing — no advertised endpoint means +│ │ # nothing is sent and the AS behaves exactly +│ │ # as before, and a network error / non-2xx / +│ │ # timeout is reported so the local clear +│ │ # always finishes. Opt out per server with +│ │ # `oauth.revokeOnClear`, read at CLEAR time +│ │ # rather than connect time. `lost_authorization_state` +│ │ # recovery passes `revoke: false` — it clears a +│ │ # half-finished flow to retry it, and an +│ │ # authorization that never completed has no +│ │ # grant to revoke — #2144; │ │ # scopes.ts SEP-2350 scope union, oauthUx.ts │ │ # shared copy, mcpAuth.ts force-reauthorization, │ │ # issuerBinding.ts SEP-2352 callback-leg failure diff --git a/README.md b/README.md index baab1076..b9efa7bf 100644 --- a/README.md +++ b/README.md @@ -31,6 +31,7 @@ inspector/ │ └── launcher/ # Shared launcher — provides the `mcp-inspector` bin, dispatches to web/cli/tui ├── core/ # Shared code consumed via the `@inspector/core` alias (no package.json) │ ├── auth/ # OAuth: providers, discovery, storage, endpoint overrides, mid-session recovery (browser/node/remote backends); +│ │ # RFC 7009 token revocation on clear (revocation.ts); │ │ # plus per-server secret storage — the keychain/file/memory SecretStore │ │ # implementations, the selection policy, and the descriptor the banner and UI report │ ├── client/ # Install-level client config (`client.json`): browser-safe parse/validate + Node load/save, remote backend, secrets @@ -159,11 +160,57 @@ Each config below is a ready-made server for exercising one feature by hand. Loa | `rfc6570-templates-http.json` | Resources tab: RFC 6570 resource-template expansion | [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) | | `advertised-extensions-http.json` | Tool registration gated on advertised extensions | [#1739](https://github.com/modelcontextprotocol/inspector/issues/1739) | | `oauth-custom-resource-metadata-http.json` **(legacy era)** | OAuth discovery driven by the challenge's `resource_metadata` | [#2071](https://github.com/modelcontextprotocol/inspector/issues/2071) | +| `oauth-revocation-http.json` / `oauth-no-revocation-http.json` **(legacy era)** | RFC 7009 token revocation on clear, with and without a `revocation_endpoint` | [#2144](https://github.com/modelcontextprotocol/inspector/issues/2144) | | `oauth-rfc8414-at-oidc-path-http.json` **(legacy era)** | Plain OAuth 2.0 AS metadata served at the OIDC well-known path | [#2172](https://github.com/modelcontextprotocol/inspector/issues/2172) | | `logging-{legacy,modern}-http.json` | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) | | `subscriptions-{legacy,modern}-http.json` | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) | | `subscriptions-never-acknowledged-http.json` | A `subscriptions/listen` answered with a bare result | [#2097](https://github.com/modelcontextprotocol/inspector/issues/2097) | | `tasks-{legacy,modern}-http.json` | Tasks, both eras | [#1631](https://github.com/modelcontextprotocol/inspector/issues/1631) | +| `cancellation-modern-http.json` | Cancelling a call by closing its response stream | [#2140](https://github.com/modelcontextprotocol/inspector/issues/2140) | + +#### Cancelling a call + +`cancellation-modern-http.json` serves `slow_task`, which reports progress once +a second for up to 60 seconds and stops early if it is cancelled, printing how +far it got to **the server's terminal**. Connect with **Protocol Era = Modern**. + +Watch the terminal you started the server in, run `slow_task` from the Tools +tab, and click **Cancel** after a few seconds. The progress must stop +immediately, the Inspector must report the call cancelled, and the server must +print `[slow_task] cancelled after Ns`. On the broken build the progress kept +arriving until the tool completed all 60 seconds and the server printed +`completed all 60s without being cancelled`, because the Inspector was sending +the wrong cancellation signal +([#2140](https://github.com/modelcontextprotocol/inspector/issues/2140)). + +The server's terminal is the place to watch, not the Inspector's result panel: +cancellation closes the very stream the tool's result would travel on, so on a +successful cancel the tool's return value is undeliverable by construction and +the Inspector shows a cancelled call rather than a result. Which is the point — +what has to stop is the *work*, and only the server can report that. + +The 2026-07-28 spec makes this +[transport-specific](https://modelcontextprotocol.io/specification/2026-07-28/basic/patterns/cancellation#transport-specific-cancellation): +for Streamable HTTP, **closing the request's SSE response stream is the +cancellation signal**, and a `notifications/cancelled` is "neither required nor +expected"; stdio, which has no per-request stream to close, keeps the +notification. A spec-compliant server therefore answers the notification `202 +Accepted` and drops it — which is precisely what the reporter observed, with the +task running on to completion while the Inspector reported it cancelled. + +The SDK already implements that fork, off `transport.hasPerRequestStream`. Every +Inspector connection is wrapped in `MessageTrackingTransport` (it feeds the +Protocol and Network tabs), which did not forward the flag — so the SDK saw +`undefined` and took the stdio branch on **every** client, CLI and TUI included. +The web client needed two more links in the chain: its browser-side transport +answers for the real upstream one that lives on the Node backend, and the abort +has to survive the `POST /api/mcp/send` hop to reach it. + +Watch it on the wire in the **Protocol** tab: cancelling now emits no +`notifications/cancelled` frame at all, and the `tools/call` entry ends as an +aborted request rather than a completed one. Switch the same server to +**Legacy** and the notification comes back — that era has no per-stream +mechanism, so it is still the correct signal there. #### MCP Apps @@ -439,6 +486,21 @@ The same server is worth running against `--cli` / `--tui`, which reach it by a The value now rides the normalized `AuthChallenge` as a string — it has to be serializable, because the web client's challenge crosses the remote-backend boundary as JSON — and is converted to a `URL` at the OAuth boundary, where it is handed to `auth()` as `resourceMetadataUrl` and to the CIMD pre-registration probe, which runs *before* `auth()` and would otherwise do its own default-location discovery. A malformed value is ignored rather than surfaced, matching the SDK's own `WWW-Authenticate` parser: discovery falls back to the default locations instead of failing the whole authorization on a bad header. The callback leg needs nothing extra — SDK `auth()` persists the URL in its discovery state, so it survives both the web full-page redirect and the CLI/TUI loopback callback. +#### Revoking tokens on clear (RFC 7009) + +`oauth-revocation-http.json` and `oauth-no-revocation-http.json` are the same OAuth-protected server (combined AS + resource, DCR, refresh tokens) differing in one thing: the first advertises a `revocation_endpoint`, the second advertises none. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. + +Add either server, connect and complete authorization, then use **Clear OAuth state and disconnect** (Server Settings → Authorization) and watch the Network tab. + +- On `oauth-revocation-http.json` a `POST /oauth/revoke` goes out naming the **refresh token**. RFC 7009 §2.1 asks the authorization server to invalidate the access tokens issued under the same grant, so one request ends both halves — the fixture implements that linkage, so re-sending the old bearer token to `/mcp` afterwards gets a 401. The request is built from the stored state *before* the local clear and sent *after* it, so the clear never waits on the network; in the Network tab the POST therefore follows the local teardown rather than preceding it. +- On `oauth-no-revocation-http.json` nothing is sent at all, and the clear behaves exactly as it did before the feature existed. That no-op path is what makes this safe against every authorization server with no RFC 7009 support ([#2144](https://github.com/modelcontextprotocol/inspector/issues/2144)). + +On the broken build both servers behaved like the second: the Inspector deleted its local copy and the grant stayed valid at the authorization server until it expired on its own — which for a refresh token is a long time, by design. + +Uncheck **Revoke tokens on clear** in the same panel (persisted as `oauth.revokeOnClear: false`) and the first server behaves like the second. That is not only an escape hatch: a client that disconnects still holding live tokens is a case worth reproducing when the server is the thing under test. + +The same behavior is reachable from the other clients — the TUI's **Clear OAuth State**, and the CLI's `--relogin` (with `--no-revoke` as the per-run opt-out). + #### Plain OAuth 2.0 metadata at the OIDC well-known path `oauth-rfc8414-at-oidc-path-http.json` is an OAuth-protected server (combined AS + resource, DCR enabled) whose RFC 8414 authorization-server metadata is served **only** from `/.well-known/openid-configuration`. It is a plain OAuth 2.0 authorization server — no ID tokens, no `jwks_uri`, no `sub` claims — and RFC 8414 §5 explicitly permits that filename for general OAuth metadata. `/.well-known/oauth-authorization-server` is deliberately left unserved. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. diff --git a/clients/cli/README.md b/clients/cli/README.md index de89799d..3c3ddc07 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -123,8 +123,26 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en | `--strict` | With `--method tools/list`: report tool-schema portability problems in full (path, issue, suggested fix) on stderr, and exit `6` if any is error-severity. Without it, a one-line count is printed instead. See [Schema portability](#schema-portability---strict). | | `--format ` | Output format. `text` (default) pretty-prints the result. `json` emits a single JSON object on stdout (`{ "result": … }`, plus `{ "appInfo": … }` as a sibling key for App tools) with no banners, so the whole output pipes cleanly into `jq`. | | `--relogin` | Delete stored OAuth for this server URL from the shared store before connect; interactive login still only runs if the server requires auth. Requires an HTTP/SSE URL (rejected for stdio). Conflicts with `--stored-auth-only` / `--use-stored-auth` / `--wait-for-auth` / catalog short-circuits. | +| `--no-revoke` | With `--relogin`, skip the [RFC 7009](https://datatracker.ietf.org/doc/html/rfc7009) revocation request that would otherwise end the grant at the authorization server when the local state is deleted. The per-server `oauth.revokeOnClear` setting is the persistent form of the same opt-out; either one is enough to skip it. See [Revoking on `--relogin`](#revoking-on---relogin). | | `--stored-auth-only` | **CI / non-interactive safe:** never start interactive OAuth / step-up (and never auto-open a browser); use the shared store if present, otherwise fail immediately with `auth_required`. Prefer this over a bare pipe/CI run that would otherwise attempt interactive login. | +#### Revoking on `--relogin` + +`--relogin` deletes this server's stored OAuth state so the next connect cannot silently reuse it. By default it now also **revokes the grant at the authorization server**, per [RFC 7009](https://datatracker.ietf.org/doc/html/rfc7009) ([#2144](https://github.com/modelcontextprotocol/inspector/issues/2144)) — otherwise the delete is invisible to the AS, and the access token, plus the refresh token when one was issued, stay valid there until they expire on their own. + +The request is built from the stored state before the delete and sent after it, so the local delete never waits on the network. That matters because the OAuth store is shared: holding this process's view of it across a five-second request would let another CLI or TUI write a fresh grant that this one then erased. + +The request names the refresh token when there is one: RFC 7009 §2.1 asks the authorization server to invalidate the access tokens issued under the same grant, so a single request covers both. + +It is best-effort and never changes the exit code. An authorization server that advertises no `revocation_endpoint` gets no request at all; a network error, a non-2xx, or the short timeout prints a one-line warning on stderr and the local delete proceeds either way. + +Turn it off per run with `--no-revoke`, or per server with `oauth.revokeOnClear: false` in the catalog — either is enough, and neither can turn it on for the other. Disconnecting while still holding live tokens is a case worth reproducing when the server under test is the thing being debugged. + +```bash +mcp-inspector --cli --server-url https://example.com/mcp --relogin --method tools/list +mcp-inspector --cli --server-url https://example.com/mcp --relogin --no-revoke --method tools/list +``` + `servers/show` redacts secret-bearing fields (`env` values, sensitive headers, sensitive `settings.metadata` keys whose whole value is replaced whether or not it is structured, `requestInit` / `eventSourceInit` headers, `oauthClientSecret`). It does **not** scrub credentials embedded in a server `url` (userinfo or query tokens) or in stdio `args` — treat `detail` / raw URL fields as potentially sensitive before pasting into issues. #### App probing (`--app-info`) and machine-readable output (`--format json`) diff --git a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts index 1da26d7f..9b5e587d 100644 --- a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts +++ b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, afterEach } from "vitest"; +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"; @@ -8,6 +8,8 @@ import { } from "@inspector/core/auth/node/storage-node.js"; import { clearStoredAuthForRelogin } from "../src/clear-stored-auth-for-relogin.js"; +const AS_ISSUER = "https://as.example.com"; + describe("clearStoredAuthForRelogin", () => { let dir: string | undefined; let prevPath: string | undefined; @@ -66,6 +68,386 @@ describe("clearStoredAuthForRelogin", () => { expect(blob.servers["not a url"]).toBeUndefined(); }); + // #2144 — RFC 7009. The request is *built* before the delete (from the token, + // the client id and the cached metadata it removes) and *sent* after it, so + // the local delete never waits on the network. + describe("token revocation", () => { + function seed(over: Record = {}): string { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-revoke-")); + const file = path.join(dir, "oauth.json"); + fs.writeFileSync( + file, + JSON.stringify({ + servers: { + // Issuer-bound (SEP-2352), matching the cached metadata below: an + // unkeyed grant records no authorization server and is refused, so + // seeding one would make these cases assert that instead. + "https://example.com/mcp": { + activeIssuer: AS_ISSUER, + byIssuer: { + [AS_ISSUER]: { + tokens: { + access_token: "a", + token_type: "Bearer", + refresh_token: "r", + }, + }, + }, + serverMetadata: { + issuer: AS_ISSUER, + authorization_endpoint: `${AS_ISSUER}/authorize`, + token_endpoint: `${AS_ISSUER}/token`, + revocation_endpoint: `${AS_ISSUER}/revoke`, + response_types_supported: ["code"], + }, + ...over, + }, + }, + idpSessions: {}, + }), + "utf8", + ); + prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file; + resetNodeOAuthStorageCache(); + return file; + } + + /** Seed both key spellings of one server, each with its own refresh token. */ + function seedBothSpellings( + normalizedToken: string, + rawToken: string, + ): void { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-two-")); + const file = path.join(dir, "oauth.json"); + const metadata = { + issuer: AS_ISSUER, + authorization_endpoint: `${AS_ISSUER}/authorize`, + token_endpoint: `${AS_ISSUER}/token`, + revocation_endpoint: `${AS_ISSUER}/revoke`, + response_types_supported: ["code"], + }; + const entry = (refresh: string) => ({ + activeIssuer: AS_ISSUER, + byIssuer: { + [AS_ISSUER]: { + tokens: { + access_token: `a-${refresh}`, + token_type: "Bearer", + refresh_token: refresh, + }, + }, + }, + serverMetadata: metadata, + }); + fs.writeFileSync( + file, + JSON.stringify({ + servers: { + "https://example.com": entry(rawToken), + "https://example.com/": entry(normalizedToken), + }, + idpSessions: {}, + }), + "utf8", + ); + prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file; + resetNodeOAuthStorageCache(); + } + + it("revokes the stored grant, having snapshotted it before the delete", async () => { + const file = seed(); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response(null, { status: 200 })); + try { + await expect( + clearStoredAuthForRelogin("https://example.com/mcp"), + ).resolves.toMatchObject({ + status: "revoked", + tokenTypeHint: "refresh_token", + }); + expect(fetchSpy).toHaveBeenCalledTimes(1); + const [url, init] = fetchSpy.mock.calls[0]!; + expect(String(url)).toBe("https://as.example.com/revoke"); + expect(new URLSearchParams(String(init?.body)).get("token")).toBe("r"); + } finally { + fetchSpy.mockRestore(); + } + const blob = JSON.parse(fs.readFileSync(file, "utf8")) as { + servers: Record; + }; + expect(blob.servers["https://example.com/mcp"]).toBeUndefined(); + }); + + // 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") + .mockResolvedValue(new Response(null, { status: 200 })); + try { + // Normalises to the stored `https://example.com/mcp`. + await expect( + clearStoredAuthForRelogin("https://Example.com/mcp"), + ).resolves.toMatchObject({ status: "revoked" }); + expect(fetchSpy).toHaveBeenCalledTimes(1); + } finally { + fetchSpy.mockRestore(); + } + }); + + // 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: AS_ISSUER, + authorization_endpoint: `${AS_ISSUER}/authorize`, + token_endpoint: `${AS_ISSUER}/token`, + revocation_endpoint: `${AS_ISSUER}/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": { + activeIssuer: AS_ISSUER, + byIssuer: { + [AS_ISSUER]: { + tokens: { + access_token: "stale-a", + token_type: "Bearer", + refresh_token: "stale-r", + }, + }, + }, + serverMetadata: metadata, + }, + // The normalised spelling, holding the CURRENT grant. + "https://example.com/": { + activeIssuer: AS_ISSUER, + byIssuer: { + [AS_ISSUER]: { + 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; + }; + 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 + .spyOn(globalThis, "fetch") + .mockRejectedValue(new Error("unreachable")); + try { + await expect( + clearStoredAuthForRelogin("https://example.com/mcp"), + ).resolves.toMatchObject({ status: "failed" }); + } finally { + fetchSpy.mockRestore(); + } + const blob = JSON.parse(fs.readFileSync(file, "utf8")) as { + servers: Record; + }; + expect(blob.servers["https://example.com/mcp"]).toBeUndefined(); + }); + + it("skips the request when revocation is turned off", async () => { + seed(); + const fetchSpy = vi.spyOn(globalThis, "fetch"); + try { + await expect( + clearStoredAuthForRelogin("https://example.com/mcp", { + revoke: false, + }), + ).resolves.toBeUndefined(); + expect(fetchSpy).not.toHaveBeenCalled(); + } finally { + fetchSpy.mockRestore(); + } + }); + + // Keys are revoked from independently. Deduplicating them could only be + // done by pre-reading one token, which would skip the second key's OTHER + // issuer-bound grants along with it; a duplicate RFC 7009 request is + // harmless (§2.2 makes an unknown token a success), a missed one is not. + it("revokes from both keys even when they hold the same token", async () => { + seedBothSpellings("same-r", "same-r"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response(null, { status: 500 })); + try { + await expect( + clearStoredAuthForRelogin("https://example.com"), + ).resolves.toMatchObject({ status: "failed" }); + expect(fetchSpy).toHaveBeenCalledTimes(2); + } finally { + fetchSpy.mockRestore(); + } + }); + + // A grant still live at the authorization server is what the user needs to + // hear about; reporting the earlier success would print no warning at all. + // One budget across both keys, so a first request that outlives it fails + // rather than each key getting a fresh five seconds. + it("bounds both keys with one shared budget", async () => { + seedBothSpellings("live-r", "stale-r"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockImplementation(async () => { + // Outlives the injected budget, so the second key is never attempted. + await new Promise((r) => setTimeout(r, 40)); + return new Response(null, { status: 200 }); + }); + try { + const outcome = await clearStoredAuthForRelogin("https://example.com", { + budgetMs: 20, + }); + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome?.status === "failed" ? outcome.detail : "").toContain( + "timed out after 20ms", + ); + } finally { + fetchSpy.mockRestore(); + } + }); + + // A plan reached with no budget left is reported rather than firing a + // request it would immediately abandon — and that report must outrank an + // earlier success, since the unattempted key's grant may still be live. + it("reports exhaustion for a key it never attempted", async () => { + seedBothSpellings("live-r", "stale-r"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response(null, { status: 200 })); + try { + const outcome = await clearStoredAuthForRelogin("https://example.com", { + budgetMs: 0, + }); + expect(fetchSpy).not.toHaveBeenCalled(); + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome?.status === "failed" ? outcome.detail : "").toContain( + "budget was exhausted", + ); + } finally { + fetchSpy.mockRestore(); + } + }); + + it("reports a later failure over an earlier success", async () => { + seedBothSpellings("live-r", "stale-r"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValueOnce(new Response(null, { status: 200 })) + .mockResolvedValue(new Response(null, { status: 500 })); + try { + await expect( + clearStoredAuthForRelogin("https://example.com"), + ).resolves.toMatchObject({ status: "failed" }); + } finally { + fetchSpy.mockRestore(); + } + }); + + // `getTokens` parses through `OAuthTokensSchema`, so a persisted token that + // no longer validates rejects. That must not abandon the local delete + // `--relogin` promises — over a grant that could not have been revoked. + it("still clears when the stored token cannot be parsed", async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-bad-token-")); + const file = path.join(dir, "oauth.json"); + fs.writeFileSync( + file, + JSON.stringify({ + servers: { + "https://example.com/mcp": { + activeIssuer: AS_ISSUER, + byIssuer: { + // No `token_type` — fails OAuthTokensSchema. + [AS_ISSUER]: { tokens: { access_token: 42 } }, + }, + }, + }, + idpSessions: {}, + }), + "utf8", + ); + prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file; + resetNodeOAuthStorageCache(); + + await expect( + clearStoredAuthForRelogin("https://example.com/mcp"), + ).resolves.toMatchObject({ status: "failed" }); + + const blob = JSON.parse(fs.readFileSync(file, "utf8")) as { + servers: Record; + }; + expect(blob.servers["https://example.com/mcp"]).toBeUndefined(); + }); + + it("reports no_tokens when the store holds nothing for the server", async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-empty-")); + const file = path.join(dir, "oauth.json"); + fs.writeFileSync( + file, + JSON.stringify({ servers: {}, idpSessions: {} }), + "utf8", + ); + prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file; + resetNodeOAuthStorageCache(); + + await expect( + clearStoredAuthForRelogin("https://example.com/mcp"), + ).resolves.toEqual({ status: "skipped", reason: "no_tokens" }); + }); + }); + it("clears both raw and URL-normalised keys (bare origin / mixed-case host)", async () => { dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-norm-")); const file = path.join(dir, "oauth.json"); diff --git a/clients/cli/__tests__/relogin-revocation.test.ts b/clients/cli/__tests__/relogin-revocation.test.ts new file mode 100644 index 00000000..eb1698ea --- /dev/null +++ b/clients/cli/__tests__/relogin-revocation.test.ts @@ -0,0 +1,227 @@ +/** + * `--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 AS_ISSUER = "https://as.example.com"; +const REVOKE_URL = `${AS_ISSUER}/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: { + // Issuer-bound (SEP-2352) and matching the cached metadata: an unkeyed + // grant records no authorization server and is deliberately refused. + [SERVER_URL]: { + activeIssuer: AS_ISSUER, + byIssuer: { + [AS_ISSUER]: { + tokens: { + access_token: "a", + token_type: "Bearer", + refresh_token: "r", + }, + }, + }, + serverMetadata: { + issuer: AS_ISSUER, + authorization_endpoint: `${AS_ISSUER}/authorize`, + token_endpoint: `${AS_ISSUER}/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 { + 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; +} + +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 }); + } + }); + + // Inert-but-accepted flags are rejected in this parser (see the `--strict` + // rationale in `cli.ts`). On its own `--no-revoke` reads as "this run will + // not revoke anything", which is true only because nothing was being cleared. + it("rejects --no-revoke without --relogin", async () => { + const result = await runCli([ + "--no-revoke", + "--server-url", + SERVER_URL, + "--method", + "tools/list", + ]); + expect(result.stderr).toMatch(/--no-revoke requires --relogin/); + }); + + // 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); + }); +}); diff --git a/clients/cli/src/clear-stored-auth-for-relogin.ts b/clients/cli/src/clear-stored-auth-for-relogin.ts index bcbdc880..6ec64e6e 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -2,6 +2,14 @@ import { NodeOAuthStorage, resetNodeOAuthStorageCache, } from "@inspector/core/auth/node/storage-node.js"; +import { + DEFAULT_REVOCATION_TIMEOUT_MS, + clearAndPlanRevocation, + executeOAuthRevocation, + type OAuthRevocationPlan, + type TokenRevocationOutcome, +} from "@inspector/core/auth/revocation.js"; +import { createProxyFetch } from "@inspector/core/mcp/node/proxyFetch.js"; /** Same canonicalisation as CLI `normalizeServerUrl` (avoid cycles). */ function normalizeServerUrl(serverUrl: string): string { @@ -24,16 +32,121 @@ function normalizeServerUrl(serverUrl: string): string { */ export async function clearStoredAuthForRelogin( serverUrl: string | undefined, -): Promise { - if (!serverUrl?.trim()) return; + options?: { + revoke?: boolean; + /** + * Total wall-clock budget for the revocation requests across both key + * spellings. Defaults to {@link DEFAULT_REVOCATION_TIMEOUT_MS}; injectable + * so the exhaustion path can be exercised without a five-second test. + */ + budgetMs?: number; + }, +): Promise { + if (!serverUrl?.trim()) return undefined; const raw = serverUrl.trim(); const normalized = normalizeServerUrl(raw); const storage = new NodeOAuthStorage(); - await storage.clear(raw); - if (normalized !== raw) { - await storage.clear(normalized); + // RFC 7009 (#2144), ordered snapshot -> clear -> revoke. Everything the + // requests need is read first, because the clear empties the store; but the + // clear then runs immediately rather than behind the network. Waiting would + // hold this process's in-memory view of a *shared*, file-backed store for up + // to five seconds, and another CLI/TUI writing a fresh grant in that window + // would be erased by the clear that followed. + // + // Both spellings are cleared, so both are planned 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. + // They are deliberately not deduplicated; see `sendPlans`. + const keys = normalized === raw ? [raw] : [normalized, raw]; + // Each key's state is taken and deleted in ONE atomic storage step, so the + // clear is never a separate check-then-act over an already-read snapshot. + const plans: OAuthRevocationPlan[] = []; + for (const key of keys) { + plans.push( + await clearAndPlanRevocation({ + serverUrl: key, + storage, + enabled: options?.revoke !== false, + }), + ); } // Drop the in-process singleton so the next connect cannot reuse a cleared // entry from the NodeOAuthStorage cache. resetNodeOAuthStorageCache(); + + return options?.revoke === false + ? undefined + : sendPlans(plans, options?.budgetMs ?? DEFAULT_REVOCATION_TIMEOUT_MS); +} + +/** + * Send each plan's requests, in order, and report the outcome that matters most. + * + * Both key spellings are cleared, so both are planned 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. + * + * There is deliberately **no** cross-key deduplication. It looked cheap — the + * two keys are usually two spellings of one server — but it can only be done by + * pre-reading a single token, and a plan enumerates every issuer slot under a + * key. So a shared *active* token would have skipped the second key entirely, + * taking any additional issuer-bound grant under it with the local delete. A + * duplicate RFC 7009 request is harmless (§2.2 makes an unknown token a + * success), which is a much better trade than a missed one. + * + * One budget across both plans, for the same reason a plan shares one across + * its grants: two keys would otherwise double the wait. + * + * Reporting prefers a **failure** over any success — a grant still live at the + * authorization server is what the user needs to hear about, and a success + * would otherwise silence the warning. Failing that it is the first plan's + * outcome that was not "nothing to do", which with the keys in + * `findStoredServerState` precedence means the grant the CLI would actually + * have connected with. When no key held 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 sendPlans( + plans: OAuthRevocationPlan[], + budgetMs: number, +): Promise { + const fetchFn = createProxyFetch() ?? fetch; + const deadlineAt = Date.now() + budgetMs; + let reported: TokenRevocationOutcome | undefined; + let lastSkip: TokenRevocationOutcome | undefined; + for (const plan of plans) { + const remainingMs = deadlineAt - Date.now(); + // A plan that already knows its answer needs no network, so the budget is + // irrelevant to it. Synthesising exhaustion here would warn that a grant + // may still be live when the key held no grant at all — a false alarm, and + // one that outranks the real outcome under the failure-first rule below. + const needsNetwork = plan.outcome === undefined; + if (needsNetwork && remainingMs <= 0) { + // Overrides an earlier success rather than deferring to it: this key's + // grant may still be live at the authorization server, and that is the + // thing the user needs to hear about. Same failure-first rule as below. + const exhausted: TokenRevocationOutcome = { + status: "failed", + detail: `the ${budgetMs}ms revocation budget was exhausted before "${plan.serverUrl}" was attempted`, + }; + if (reported?.status !== "failed") reported = exhausted; + continue; + } + const outcome = await executeOAuthRevocation(plan, { + fetchFn, + timeoutMs: needsNetwork ? remainingMs : undefined, + }); + if (outcome.status === "skipped" && outcome.reason === "no_tokens") { + lastSkip = outcome; + continue; + } + if (outcome.status === "failed") { + if (reported?.status !== "failed") reported = outcome; + } else { + reported ??= outcome; + } + } + return reported ?? lastSkip; } diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 14f51ab9..17aa5ba0 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -108,6 +108,7 @@ async function callMethod( callbackUrlConfig: RunnerOAuthCallbackConfig, storedAuthOnly: boolean, relogin: boolean, + revoke: boolean, ): Promise { // Clear after parse-time validation so a bad flag combo never deletes store // entries. Deletes the shared URL-keyed OAuth entry (not "ignore for this run"). @@ -117,7 +118,18 @@ async function callMethod( "--relogin requires an HTTP/SSE server URL (no OAuth store entry for stdio)", ); } - await clearStoredAuthForRelogin(serverConfig.url); + // RFC 7009 (#2144). The flag and the per-server setting are both opt-outs, + // so either one turns the revocation off; neither can turn it on for the + // other. Reported rather than thrown — `--relogin` is a local delete and + // must not start failing because an authorization server is unreachable. + const revocation = await clearStoredAuthForRelogin(serverConfig.url, { + revoke: revoke && serverSettings?.oauthRevokeOnClear !== false, + }); + if (revocation?.status === "failed") { + process.stderr.write( + `Warning: could not revoke the OAuth grant at the authorization server (${revocation.detail}); it may still be valid there.\n`, + ); + } } // Version comes from the single source of truth — the root package.json — @@ -592,6 +604,7 @@ type ParseResult = callbackUrl?: string; storedAuthOnly?: boolean; relogin?: boolean; + revoke?: boolean; } // Short-circuit modes (`--list-stored-auth`, `--print-handoff`) do their own // output and need no server connection; runCli returns immediately. @@ -787,6 +800,10 @@ async function parseArgs(argv?: string[]): Promise { "--relogin", "Delete stored OAuth for this server URL from the shared store before connect (HTTP/SSE URL keys only); interactive login runs only if the server requires auth. Rejected for stdio (no URL-keyed store entry)", ) + .option( + "--no-revoke", + "Requires --relogin. Skips the RFC 7009 revocation request that would otherwise end the grant at the authorization server when the local state is deleted. Also skipped when the server entry sets oauth.revokeOnClear to false.", + ) .option( "--wait-for-auth ", "Poll the OAuth state file until a token for --server-url appears (or the timeout elapses), then proceed as if --use-stored-auth were set. Use after handing off to a human to complete OAuth in a browser.", @@ -842,6 +859,7 @@ async function parseArgs(argv?: string[]): Promise { useStoredAuth?: boolean; storedAuthOnly?: boolean; relogin?: boolean; + revoke?: boolean; waitForAuth?: number; listStoredAuth?: boolean; printHandoff?: boolean; @@ -871,6 +889,15 @@ async function parseArgs(argv?: string[]): Promise { } } + // `--no-revoke` only means anything alongside `--relogin` — it suppresses the + // RFC 7009 request that clear makes. Accepted on its own it is inert, and + // worse than inert: it reads as "this run will not revoke anything", which is + // true only because nothing was going to be cleared. Rejected here, ahead of + // the short-circuit returns, for the same reason `--strict` is (#2144). + if (options.revoke === false && !options.relogin) { + throw new Error("--no-revoke requires --relogin (it has no other effect)."); + } + // `--strict` is checked HERE, ahead of every short-circuit return below // (`--list-stored-auth`, `--print-handoff`, `servers/list`, `servers/show`), // rather than beside the other method-shaped validations further down. Those @@ -1134,6 +1161,9 @@ async function parseArgs(argv?: string[]): Promise { callbackUrl: options.callbackUrl, storedAuthOnly: options.storedAuthOnly === true, relogin: options.relogin === true, + // Commander's `--no-revoke` defaults this to true; only an explicit + // `--no-revoke` makes it false. + revoke: options.revoke !== false, }; } @@ -1152,6 +1182,7 @@ export async function runCli(argv?: string[]): Promise { callbackUrl, storedAuthOnly, relogin, + revoke, } = parsed; const clientConfig = await loadRunnerClientConfig({ clientConfigPath }); // A bad --callback-url / MCP_OAUTH_CALLBACK_URL is a *usage* error, but its @@ -1181,5 +1212,6 @@ export async function runCli(argv?: string[]): Promise { callbackUrlConfig, storedAuthOnly === true, relogin === true, + revoke !== false, ); } diff --git a/clients/tui/README.md b/clients/tui/README.md index 93930003..bab524ef 100644 --- a/clients/tui/README.md +++ b/clients/tui/README.md @@ -65,7 +65,7 @@ OAuth redirect URIs must match **exactly** what you register on the authorizatio 1. Select an HTTP/SSE server and press **C** to connect. 2. If authorization is required, the TUI starts OAuth automatically (browser opens for sign-in). 3. After the callback completes, connect finishes without a second **C**. -4. Use the **Auth** tab to inspect OAuth state (same fields as web Connection Info) or **Clear OAuth state** (disconnects when connected). +4. Use the **Auth** tab to inspect OAuth state (same fields as web Connection Info) or **Clear OAuth state** (disconnects when connected). Clearing also revokes the grant at the authorization server when it advertises an RFC 7009 `revocation_endpoint` — best-effort, with any failure reported in the status line and the local clear finishing regardless. Set `oauth.revokeOnClear: false` on the server entry to skip it ([#2144](https://github.com/modelcontextprotocol/inspector/issues/2144)). See also [EMA / enterprise-managed auth](../../specification/v2_auth_ema.md) and [OAuth smoke testing](../../specification/v2_auth_smoke_testing.md) for staging servers and verification steps. diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index e58d8c77..3139174f 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -59,9 +59,10 @@ const h = vi.hoisted(() => { authenticate: vi.fn( async () => new URL("https://auth.example/start"), ), - clearOAuthTokens: vi.fn( - async () => {}, - ), + clearOAuthTokens: vi.fn(async () => ({ + status: "skipped" as const, + reason: "no_endpoint" as const, + })), completeOAuthFlow: vi.fn( async () => {}, ), @@ -359,6 +360,26 @@ function oneHttp(): Record { }; } +/** An HTTP server that opted out of RFC 7009 revocation on clear (#2144). */ +function oneHttpNoRevoke(): Record { + 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 { return { ema: { @@ -665,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(); @@ -1350,6 +1377,116 @@ 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"]); + // The detail is what identifies the failure; the tone the message renders + // in is asserted against `AuthTab` directly, where the branch lives. + await expectFrame(r, "unreachable"); + }); + + // The wiring is the contract here, not just `AuthTab`'s own behavior: a + // `void`-ing arrow between them resolves instantly, which makes the pending + // state, the repeat lock and the rejection path all inert while revocation + // is still running. Driven through the real App so the arrow cannot come + // back (#2144). + it("holds the pending state until the clear actually settles", async () => { + let settle: () => void = () => {}; + h.clientSpies.clearOAuthTokens.mockImplementation( + () => + new Promise((resolve) => { + settle = () => + resolve({ status: "skipped", reason: "no_endpoint" as const }); + }), + ); + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await waitUntil(() => h.clientSpies.clearOAuthTokens.mock.calls.length > 0); + await tick(); + + // Still running: the pending line is up and the confirmation is not. + expect(r.lastFrame() ?? "").toContain("Clearing OAuth state"); + expect(r.lastFrame() ?? "").not.toContain("OAuth state cleared"); + + // And the repeat lock is live, so a second press does not start another. + await press(r, ["s"]); + expect(h.clientSpies.clearOAuthTokens).toHaveBeenCalledTimes(1); + + settle(); + await waitUntil(() => + (r.lastFrame() ?? "").includes("OAuth state cleared"), + ); + }); + + // The clear has already succeeded when the disconnect runs, so its failure + // must not propagate: `AuthTab` would report "Could not clear OAuth state", + // which is false. It goes to the disconnect error line, where the same + // failure from the `d` key already lands. + it("reports a post-clear disconnect failure as a disconnect failure", async () => { + h.ctrl.status = "connected"; + h.disconnect.mockRejectedValue(new Error("disconnect-blew-up")); + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + + await expectFrame(r, "disconnect-blew-up"); + expect(r.lastFrame() ?? "").not.toContain("Could not clear OAuth state"); + }); + + // A transport can reject with something that is not an `Error`; the message + // line must still be readable rather than "[object Object]". + it("reports a non-Error disconnect failure from a clear", async () => { + h.ctrl.status = "connected"; + h.disconnect.mockRejectedValue("plain-disconnect-string"); + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await expectFrame(r, "plain-disconnect-string"); + }); + + // A clear while still CONNECTING tears the attempt down too — the session is + // half-built, and leaving it up with its OAuth state gone is worse than not + // having it. + it("disconnects a connecting session when clearing", async () => { + h.ctrl.status = "connecting"; + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await waitUntil(() => h.disconnect.mock.calls.length > 0); + expect(h.disconnect).toHaveBeenCalled(); + }); + + it("says nothing when there was nothing to revoke", async () => { + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await waitUntil(() => h.clientSpies.clearOAuthTokens.mock.calls.length > 0); + expect(r.lastFrame() ?? "").not.toContain("authorization server failed"); + }); + const stepUpChallenge = { reason: "insufficient_scope" as const, requiredScopes: ["env:read"], diff --git a/clients/tui/__tests__/AuthTab.test.tsx b/clients/tui/__tests__/AuthTab.test.tsx index 86d6b18e..213416c5 100644 --- a/clients/tui/__tests__/AuthTab.test.tsx +++ b/clients/tui/__tests__/AuthTab.test.tsx @@ -100,6 +100,235 @@ describe("AuthTab", () => { ); }); + // #2144 — the clear is now a bounded network request, so announcing "cleared" + // on the keypress would say it while the work was still in flight. + it("shows a pending state until the clear settles, and ignores repeats", async () => { + let settle: () => void = () => {}; + const onClearOAuth = vi.fn( + () => + new Promise((resolve) => { + settle = resolve; + }), + ); + const { lastFrame, stdin } = render( + , + ); + await tick(); + + stdin.write("s"); + await tick(); + expect(lastFrame() ?? "").toContain("Clearing OAuth state"); + expect(lastFrame() ?? "").not.toContain("OAuth state cleared"); + + // A second press while one is in flight would race the first over the same + // store entry, and nothing on screen tells the user the first is running. + stdin.write("s"); + await tick(); + expect(onClearOAuth).toHaveBeenCalledTimes(1); + + settle(); + await tick(); + expect(lastFrame() ?? "").toContain("OAuth state cleared"); + }); + + // A state-based guard is not effective until React re-renders, so two `s` + // events in the SAME input turn both read the last-rendered "idle" and start + // concurrent clears against one store entry. No tick between the writes. + it("ignores a repeat delivered in the same input turn", async () => { + const onClearOAuth = vi.fn(() => new Promise(() => {})); + const { stdin } = render( + , + ); + await tick(); + + stdin.write("s"); + stdin.write("s"); + await tick(); + + expect(onClearOAuth).toHaveBeenCalledTimes(1); + }); + + // A rejection releases the lock, so the user can retry. + it("allows a retry after a rejected clear", async () => { + const onClearOAuth = vi + .fn<() => Promise>() + .mockRejectedValueOnce(new Error("nope")) + .mockResolvedValue(undefined); + const { stdin } = render( + , + ); + await tick(); + stdin.write("s"); + await tick(); + stdin.write("s"); + await tick(); + + expect(onClearOAuth).toHaveBeenCalledTimes(2); + }); + + // A stale completion owns nothing: server A settling after the user moved to + // B and started a clear there must not drop B's lock, or a second B clear + // could run concurrently against the same store entry. + it("a stale completion does not release the current server's lock", async () => { + const settlers: Array<() => void> = []; + const onClearOAuth = vi.fn( + () => + new Promise((resolve) => { + settlers.push(resolve); + }), + ); + const props = (serverName: string) => ({ + ...baseProps, + serverName, + onClearOAuth, + inspectorClient: null, + oauthStatus: "idle" as const, + oauthMessage: null, + focused: true, + }); + const { stdin, rerender } = render(); + await tick(); + + stdin.write("s"); // A's clear starts + await tick(); + rerender(); + await tick(); + stdin.write("s"); // B's clear starts + await tick(); + expect(onClearOAuth).toHaveBeenCalledTimes(2); + + // A settles late. B's clear is still running, so its lock must hold. + settlers[0]!(); + await tick(); + stdin.write("s"); + await tick(); + + expect(onClearOAuth).toHaveBeenCalledTimes(2); + }); + + // A rejection is NOT a revocation failure — those come back as outcomes and + // are reported through the message line. This is the local clear or the + // disconnect itself failing, so reporting success would be a plain lie. + it("reports a rejected clear as a failure, not as success", async () => { + const onClearOAuth = vi.fn(() => + Promise.reject(new Error("keychain locked")), + ); + const { lastFrame, stdin } = render( + , + ); + await tick(); + stdin.write("s"); + await tick(); + const frame = lastFrame() ?? ""; + expect(frame).toContain("Could not clear OAuth state"); + expect(frame).toContain("keychain locked"); + expect(frame).not.toContain("OAuth state cleared."); + expect(frame).not.toContain("Clearing OAuth state"); + }); + + // A clear started on server A must not confirm under server B: it is a + // bounded network request now, so it can settle after the user has moved on. + it("does not confirm a clear that settles after the server changed", async () => { + let settle: () => void = () => {}; + const onClearOAuth = vi.fn( + () => + new Promise((resolve) => { + settle = resolve; + }), + ); + const { lastFrame, stdin, rerender } = render( + , + ); + await tick(); + stdin.write("s"); + await tick(); + expect(lastFrame() ?? "").toContain("Clearing OAuth state"); + + rerender( + , + ); + await tick(); + settle(); + await tick(); + + const frame = lastFrame() ?? ""; + expect(frame).not.toContain("OAuth state cleared"); + expect(frame).not.toContain("Clearing OAuth state"); + }); + + // #2144 — a revocation failure is a *partial* success: the local state really + // was cleared, so it is not an `error` status, but the grant may still be + // live at the authorization server and the informational tone understates + // that. Both tones are exercised so neither branch can rot. + it("renders an idle message in the warning tone when asked", () => { + const { lastFrame } = render( + , + ); + expect(lastFrame() ?? "").toContain("Cleared locally"); + }); + + it("renders an idle message in the default informational tone", () => { + const { lastFrame } = render( + , + ); + expect(lastFrame() ?? "").toContain("Stored OAuth state cleared."); + }); + it("renders OAuth details from getOAuthState", async () => { const { client } = makeClient(sampleOAuthState); const { lastFrame } = render( diff --git a/clients/tui/__tests__/oauthMessageTone.test.ts b/clients/tui/__tests__/oauthMessageTone.test.ts new file mode 100644 index 00000000..7d00c059 --- /dev/null +++ b/clients/tui/__tests__/oauthMessageTone.test.ts @@ -0,0 +1,28 @@ +import { describe, it, expect } from "vitest"; +import { oauthMessageToneFor } from "../src/oauthMessageTone.js"; + +const WARNING = + "Cleared locally, but revoking the grant at the authorization server failed."; + +describe("oauthMessageToneFor", () => { + it("warns while the message it was raised for is showing", () => { + expect(oauthMessageToneFor(WARNING, WARNING)).toBe("warning"); + }); + + // The bug this replaced (#2144): a stored tone survived the message that + // earned it, so every later ordinary note rendered as a warning. + it("returns to informational as soon as any other message replaces it", () => { + expect(oauthMessageToneFor("Authorization updated.", WARNING)).toBe("info"); + }); + + it("is informational when nothing has ever warned", () => { + expect(oauthMessageToneFor("Authorization updated.", null)).toBe("info"); + }); + + // `null === null` must not read as a warning — with no message on screen + // there is nothing to colour, and the previous warning is long gone. + it("is informational when there is no message", () => { + expect(oauthMessageToneFor(null, null)).toBe("info"); + expect(oauthMessageToneFor(null, WARNING)).toBe("info"); + }); +}); diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index d8a8aee0..5b7d35f1 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -65,6 +65,7 @@ import { NodeOAuthStorage, runRunnerInteractiveOAuth, } from "@inspector/core/auth/node/index.js"; +import { oauthMessageToneFor } from "./oauthMessageTone.js"; import { getTuiLogger } from "./logger.js"; import { openUrl } from "./utils/openUrl.js"; import { @@ -161,6 +162,13 @@ function App({ "idle" | "authenticating" | "error" >("idle"); const [oauthMessage, setOauthMessage] = useState(null); + // The tone is derived from the message rather than stored beside it, so it + // cannot go stale — see `oauthMessageToneFor` for why that matters here. + // Both stay plain `useState` setters on purpose: wrapping `setOauthMessage` + // in a `useCallback` would make it a value `react-hooks/exhaustive-deps` + // demands in seven dependency arrays, for an identity that never changes. + const [oauthWarningText, setOauthWarningText] = useState(null); + const oauthMessageTone = oauthMessageToneFor(oauthMessage, oauthWarningText); const [oauthRevision, setOauthRevision] = useState(0); const [pendingStepUp, setPendingStepUp] = useState<{ serverName: string; @@ -175,6 +183,8 @@ function App({ const [connectError, setConnectError] = useState(null); // Monotonic token for in-flight disconnects — see handleDisconnect. const disconnectAttemptRef = useRef(0); + /** Retires a superseded "clear OAuth state" the way disconnects are (#2144). */ + const clearOAuthAttemptRef = useRef(0); // A failed disconnect is deliberately NOT folded into `connectError`. That // one is only rendered by InfoTab when the status is "error", which a // rejected disconnect leaves untouched (the status stays "connected"), so @@ -454,6 +464,11 @@ function App({ // server name matching again, so the stale rejection would land on the // re-selected A. Retiring the token on every switch is what closes that. disconnectAttemptRef.current++; + // Same reasoning for the clear (#2144): the A → B → A round trip would + // otherwise leave both its attempt token and the captured server name + // matching again, so a clear started on the first A could publish into the + // re-selected one. + clearOAuthAttemptRef.current++; setDisconnectError(null); const stepUp = pendingStepUpRef.current; if (stepUp && selectedServer && stepUp.serverName !== selectedServer) { @@ -781,6 +796,11 @@ function App({ // A connect attempt supersedes whatever the last disconnect reported — // including one still in flight, which is what the counter bump retires. disconnectAttemptRef.current++; + // And whatever a pending clear was going to do (#2144). The server-name + // check alone 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 + // would tear down the session this connect just established. + clearOAuthAttemptRef.current++; setDisconnectError(null); const finishConnect = async () => { @@ -941,15 +961,63 @@ function App({ const handleClearOAuth = useCallback(async () => { if (!selectedInspectorClient) return; - await selectedInspectorClient.clearOAuthTokens(); + // Claim this attempt before awaiting, exactly as `handleDisconnect` does. + // The RFC 7009 leg is a bounded network request, so this callback can now + // stay suspended for seconds — long enough for the user to switch servers, + // at which point publishing this result would put server A's outcome on + // server B (and would race a second clear over the same state). (#2144) + const attempt = ++clearOAuthAttemptRef.current; + const attemptServer = selectedServer; + // RFC 7009. Best-effort: the outcome is reported in the status line, never + // thrown, so clearing always completes. The per-server `oauthRevokeOnClear` + // opt-out is honored here the same way the web client honors it. + const revocation = await selectedInspectorClient.clearOAuthTokens({ + revoke: selectedServerEntry?.settings?.oauthRevokeOnClear !== false, + }); + // The local clear has happened either way — only the *reporting* is + // retired, so a superseded attempt leaves no trace on the new selection. + if ( + clearOAuthAttemptRef.current !== attempt || + selectedServerRef.current !== attemptServer + ) { + return; + } setOauthStatus("idle"); - setOauthMessage(null); + if (revocation.status === "failed") { + const warning = `Cleared locally, but revoking the grant at the authorization server failed: ${revocation.detail}. It may still be valid there.`; + setOauthWarningText(warning); + setOauthMessage(warning); + } else { + setOauthMessage(null); + } setConnectError(null); if (inspectorStatus === "connected" || inspectorStatus === "connecting") { - await disconnectInspector(); + // The clear has already succeeded by here, so a disconnect failure must + // not propagate as one: `AuthTab` would report "Could not clear OAuth + // state", which is false. It goes to the disconnect error line instead, + // where the same failure from the `d` key already lands. + try { + await disconnectInspector(); + } catch (err) { + setDisconnectError(err instanceof Error ? err.message : String(err)); + } + // Revalidate: the disconnect is a second await, and a switch during it + // would make the revision bump below land on the new selection. + if ( + clearOAuthAttemptRef.current !== attempt || + selectedServerRef.current !== attemptServer + ) { + return; + } } setOauthRevision((n) => n + 1); - }, [selectedInspectorClient, inspectorStatus, disconnectInspector]); + }, [ + selectedInspectorClient, + selectedServer, + selectedServerEntry, + inspectorStatus, + disconnectInspector, + ]); // Build current server state from InspectorClient data (tools from ManagedToolsState) const currentServerState = useMemo(() => { @@ -1741,6 +1809,7 @@ function App({ inspectorClient={selectedInspectorClient} oauthStatus={oauthStatus} oauthMessage={oauthMessage} + oauthMessageTone={oauthMessageTone} oauthRevision={oauthRevision} pendingStepUp={ pendingStepUp?.serverName === selectedServer @@ -1843,9 +1912,12 @@ function App({ focused={ focus === "tabContentList" || focus === "tabContentDetails" } - onClearOAuth={() => { - void handleClearOAuth(); - }} + // Passed directly, NOT wrapped in a `void`-ing arrow: AuthTab + // awaits this to hold its pending state, keep its repeat lock, + // and route a rejection to the failure line. Dropping the + // promise here would resolve it instantly and make all three + // inert while revocation was still running (#2144). + onClearOAuth={handleClearOAuth} connectionStatus={inspectorStatus} /> ) : null} diff --git a/clients/tui/src/components/AuthTab.tsx b/clients/tui/src/components/AuthTab.tsx index c495a4e5..d2160135 100644 --- a/clients/tui/src/components/AuthTab.tsx +++ b/clients/tui/src/components/AuthTab.tsx @@ -28,6 +28,14 @@ interface AuthTabProps { inspectorClient: InspectorClient | null; oauthStatus: "idle" | "authenticating" | "error"; oauthMessage: string | null; + /** + * How to colour {@link oauthMessage} on the `idle` status. Defaults to + * `"info"` (cyan). `"warning"` exists for the one message that reports a + * *partial* success: the OAuth state really was cleared, so this is not an + * error, but the grant may still be live at the authorization server and + * cyan would understate that (#2144). + */ + oauthMessageTone?: "info" | "warning"; oauthRevision: number; pendingStepUp?: { challenge: AuthChallenge; @@ -39,7 +47,15 @@ interface AuthTabProps { width: number; height: number; focused?: boolean; - onClearOAuth: () => void; + /** + * Clears (and, unless opted out, revokes) this server's OAuth state. + * + * Returns a promise so the confirmation below can wait for it. The RFC 7009 + * revocation leg is a network request with a five-second bound (#2144), so + * this is no longer instantaneous, and announcing "cleared" on the keypress + * would say it while the work was still in flight. + */ + onClearOAuth: () => void | Promise; connectionStatus: ConnectionStatus; } @@ -57,6 +73,7 @@ export function AuthTab({ inspectorClient, oauthStatus, oauthMessage, + oauthMessageTone = "info", oauthRevision, pendingStepUp, onAuthorizeStepUp, @@ -73,7 +90,47 @@ export function AuthTab({ const [oauthState, setOauthState] = useState< OAuthConnectionState | undefined >(undefined); - const [clearedConfirmation, setClearedConfirmation] = useState(false); + const [clearState, setClearState] = useState< + "idle" | "clearing" | "cleared" | "failed" + >("idle"); + const [clearFailure, setClearFailure] = useState(null); + /** + * The server a clear was started for, and a sequence number. The clear is a + * bounded network request now, so it can settle after the user has moved on + * — at which point server A's confirmation must not appear under server B. + */ + const clearAttemptRef = useRef(0); + /** + * In-flight lock. A ref, not the `clearState` above: two `s` keypresses + * delivered in the same input turn both read the state React last rendered, + * so a state-based guard lets both start and race each other over the same + * store entry. A ref is set before the call and seen by the second read. + */ + const clearInFlightRef = useRef(false); + /** + * Set synchronously when a clear starts, and consumed by the reset effect + * below. The clear itself bumps `oauthRevision` on its way out, so without + * this the reset would race the confirmation it is meant to outlive — and + * which of the two lands first is not something the ordering guarantees. + */ + const ownClearRef = useRef(false); + const serverNameRef = useRef(serverName); + useEffect(() => { + serverNameRef.current = serverName; + }, [serverName]); + // A new selection retires any in-flight clear and drops the previous one's + // banner: neither belongs to the server now on screen. + useEffect(() => { + clearAttemptRef.current++; + clearInFlightRef.current = false; + // The retired clear returns before its `oauthRevision` bump, so the marker + // has no bump to skip. Left set, it would swallow the first unrelated + // revision on the server just selected and strand that server's banner. + ownClearRef.current = false; + setClearState("idle"); + setClearFailure(null); + setLastClearDisconnected(false); + }, [serverName]); const [lastClearDisconnected, setLastClearDisconnected] = useState(false); const [stepUpChoiceIndex, setStepUpChoiceIndex] = useState(0); @@ -91,7 +148,13 @@ export function AuthTab({ }, [refreshOAuthState, oauthRevision, connectionStatus]); useEffect(() => { - setClearedConfirmation(false); + // Skip exactly the revision bump our own clear caused; reset on the next. + if (ownClearRef.current) { + ownClearRef.current = false; + return; + } + setClearState("idle"); + setClearFailure(null); setLastClearDisconnected(false); }, [oauthRevision]); @@ -154,9 +217,49 @@ export function AuthTab({ const h = scrollViewRef.current.getViewportHeight() || 1; scrollViewRef.current.scrollBy(h); } else if (input.toLowerCase() === "s") { + // Ignore repeats while one is in flight: the second would race the + // first over the same store entry, and the user cannot see that the + // first is still running except by the pending line below. + if (clearInFlightRef.current) return; + clearInFlightRef.current = true; + const attempt = ++clearAttemptRef.current; + const attemptServer = serverName; setLastClearDisconnected(isLiveConnection); - onClearOAuth(); - setClearedConfirmation(true); + setClearFailure(null); + ownClearRef.current = true; + setClearState("clearing"); + // A completion is only ours if nothing has superseded it and the + // selection has not moved on. + const current = () => + clearAttemptRef.current === attempt && + serverNameRef.current === attemptServer; + void Promise.resolve(onClearOAuth()).then( + () => { + // `current()` FIRST, before touching either shared ref. A stale + // completion — server A settling after the user moved to B and + // started a clear there — would otherwise drop B's lock and let a + // second B clear run concurrently. A stale clear owns nothing. + if (!current()) return; + clearInFlightRef.current = false; + setClearState("cleared"); + }, + (err: unknown) => { + if (!current()) return; + clearInFlightRef.current = false; + // A rejection is NOT a revocation failure — those come back as + // outcomes and are reported through the message line. This is the + // local clear or the disconnect itself failing, so announcing + // "OAuth state cleared" here would be a plain lie. + // + // It also means `handleClearOAuth` never reached its + // `oauthRevision` bump, so the marker below has no bump to skip. + // Left set, it would swallow the next *unrelated* revision change + // and strand this banner after the OAuth state moved on. + ownClearRef.current = false; + setClearFailure(err instanceof Error ? err.message : String(err)); + setClearState("failed"); + }, + ); } }, { isActive: focused }, @@ -190,7 +293,9 @@ export function AuthTab({ {oauthMessage} )} {oauthStatus === "idle" && oauthMessage && ( - {oauthMessage} + + {oauthMessage} + )} {pendingStepUp ? ( @@ -320,13 +425,22 @@ export function AuthTab({ Clear OAuth State {isLiveConnection && " and disconnect"} - {clearedConfirmation && ( + {clearState === "clearing" && ( + Clearing OAuth state… + )} + {clearState === "cleared" && ( {lastClearDisconnected ? "OAuth state cleared. Disconnected." : "OAuth state cleared."} )} + {clearState === "failed" && ( + + Could not clear OAuth state + {clearFailure ? `: ${clearFailure}` : "."} + + )} diff --git a/clients/tui/src/oauthMessageTone.ts b/clients/tui/src/oauthMessageTone.ts new file mode 100644 index 00000000..447cbf3e --- /dev/null +++ b/clients/tui/src/oauthMessageTone.ts @@ -0,0 +1,29 @@ +/** + * How the Auth tab should colour the OAuth note it is currently showing. + * + * The tone is **derived** from the message rather than stored beside it + * (#2144). `App.tsx` sets an ordinary OAuth note in around thirty places, none + * of which would think to reset a tone, so a second piece of state went stale + * the moment one message raised it: a single revocation failure left every + * later "Authorization updated" rendering as a warning, including after + * switching servers. + * + * Comparing the shown message against the text that was raised as a warning + * makes the tone a function of what is on screen, so it cannot outlive it — + * the next `setOauthMessage` of anything else is informational by + * construction, with nothing to remember to clear. + * + * `warning` is for a *partial* success: the OAuth state really was cleared, so + * it is not an error status, but the grant may still be live at the + * authorization server and the informational tone would understate that. + */ +export type OAuthMessageTone = "info" | "warning"; + +export function oauthMessageToneFor( + message: string | null, + warningText: string | null, +): OAuthMessageTone { + // `null === null` must not read as a warning: with no message there is + // nothing to colour. + return message !== null && message === warningText ? "warning" : "info"; +} diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 9e626f9f..82937f28 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -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 { @@ -450,122 +453,67 @@ vi.mock("./lib/publishAppDocument", () => ({ // that invoke the App's connect / call-tool / get-prompt / read-resource / // set-log-level handlers. vi.mock("./components/views/InspectorView/InspectorView", () => ({ - InspectorView: (props: { - toolCallState?: { status?: string }; - toolsUi?: { - selectedToolKey?: string; - formValues: Record; - search: string; - }; - promptsUi?: { - selectedPromptName?: string; - argumentValues: Record; - submittedFor?: string; - search: string; - }; - logsUi?: { filterText: string; visibleLevels: Record }; - getPromptState?: { status?: string }; - readResourceState?: { status?: string }; - currentLogLevel?: string; - activeTab?: string; - activeServer?: string; - erroredServerId?: string; - initializeResult?: { serverInfo: { name: string; version: string } }; - onActiveTabChange: (tab: string) => void; - onConnectionInfo: () => void; - onToggleConnection: (id: string) => void; - onToolsUiChange: (next: { - selectedToolKey?: string; - formValues: Record; - search: string; - }) => void; - onPromptsUiChange: (next: { - selectedPromptName?: string; - argumentValues: Record; - submittedFor?: string; - search: string; - }) => void; - onLogsUiChange: (next: { - filterText: string; - visibleLevels: Record; - }) => void; - progressByTaskId?: Record; - onCallTool: ( - name: string, - args: Record, - runAsTask?: boolean, - ) => void; - onGetPrompt: (name: string, args: Record) => void; - onReadResource: (uri: string) => void; - onSetLogLevel: (level: string) => void; - onCancelTask: (taskId: string) => void; - onCancelToolCall: () => void; - onClearCompletedTasks: () => void; - onRefreshTasks: () => void; - onServerSettings: (id: string) => void; - onServerEdit: (id: string) => void; - onServerAdd: () => void; - highlightedServerIds?: string[]; - onClearProtocol: () => void; - onReplayProtocol: (id: string) => void; - onTogglePinProtocol: (id: string) => void; - pinnedProtocolIds?: Set; - onRefreshTools: () => void; - toolsPagination: { - paginated: boolean; - canLoadMore: boolean; - loadedPages: number; - onPaginatedChange: (v: boolean) => void; - onLoadMore: () => void; - }; - }) => ( + InspectorView: (props: InspectorViewProps) => (
- {props.toolCallState?.status ?? "none"} + {props.tools.toolCallState?.status ?? "none"} - {Object.keys(props.progressByTaskId ?? {}).join(",") || "none"} + {Object.keys(props.tasks.progressByTaskId ?? {}).join(",") || "none"} - {props.toolsUi?.selectedToolKey ?? "none"} + {props.tools.toolsUi?.selectedToolKey ?? "none"} + + + {props.tools.toolsUi?.search || "none"} - {props.toolsUi?.search || "none"} - {props.promptsUi?.selectedPromptName ?? "none"} + {props.prompts.promptsUi?.selectedPromptName ?? "none"} + + + {props.logs.logsUi?.filterText || "none"} - {props.logsUi?.filterText || "none"} - {props.getPromptState?.status ?? "none"} + {props.prompts.getPromptState?.status ?? "none"} - {props.readResourceState?.status ?? "none"} + {props.resources.readResourceState?.status ?? "none"} - {props.currentLogLevel} - {props.activeTab ?? "none"} + {props.logs.currentLogLevel} + {props.shell.activeTab ?? "none"} - {props.initializeResult - ? `name:${props.initializeResult.serverInfo.name || "(empty)"}` + {props.connection.initializeResult + ? `name:${props.connection.initializeResult.serverInfo.name || "(empty)"}` : "none"} - {props.erroredServerId ?? "none"} + {props.connection.erroredServerId ?? "none"} - {props.activeServer ?? "none"} - - + {/* A second target, so a test can drive an A -> B -> A switch (#2095). */} - - + {/* The header's explicit Disconnect, which routes to the standalone + `onDisconnect` rather than through the toggle. */} + + - - + - - - + + - - - + + - - + + {/* The real server grid (and its Add / Edit controls) lives inside this mocked view, so the config modal is only reachable through these callbacks — and the highlight batch only observable through this prop. */} - - + + + {/* The real drag-and-drop reorder lives inside this mocked view, so the + callback is only reachable through a control like this one. */} + - {(props.highlightedServerIds ?? []).join(",") || "none"} + {(props.servers.highlightedServerIds ?? []).join(",") || "none"} - {Array.from(props.pinnedProtocolIds ?? []).join(",")} + {Array.from(props.protocol.pinnedProtocolIds ?? []).join(",")} - - - + - {String(props.toolsPagination.paginated)} + {String(props.tools.toolsPagination.paginated)} - {props.toolsPagination.loadedPages} + {props.tools.toolsPagination.loadedPages} - - - - +
), })); import App from "./App"; +import type { InspectorViewProps } from "./components/views/InspectorView/InspectorView"; import { SERVER_INFO_NOT_REPORTED_LABEL } from "./components/groups/ConnectionInfoContent/ConnectionInfoContent"; import { OAUTH_CALLBACK_PATH } from "./utils/oauthFlow.js"; import { INSPECTOR_SERVERS_TAB } from "./utils/inspectorTabs.js"; @@ -3842,6 +3814,30 @@ describe("App background command rejections (#2049)", () => { return user; }; + // A complete `useServers` return with one injected mutator, so the mock stays + // type-checked against the hook's contract rather than spread from a call. + const serversWithReorder = ( + reorderServers: ReturnType["reorderServers"], + ): ReturnType => ({ + servers: [ + { + id: "A", + name: "PlotRocket", + config: { type: "streamable-http", url: "https://api.example.com/mcp" }, + connection: { status: "disconnected" }, + }, + ], + loading: false, + error: undefined, + refresh: vi.fn().mockResolvedValue(undefined), + addServer: vi.fn().mockResolvedValue(undefined), + updateServer: vi.fn().mockResolvedValue(undefined), + updateServerSettings: vi.fn().mockResolvedValue(undefined), + removeServer: vi.fn().mockResolvedValue(undefined), + reorderServers, + importSource: vi.fn().mockResolvedValue({ servers: {} }), + }); + it("does not leak an unhandled rejection when an all-pages Refresh fails", async () => { vi.mocked(useManagedTools).mockReturnValue({ tools: [], @@ -3919,6 +3915,156 @@ describe("App background command rejections (#2049)", () => { rejections.stop(); } }); + + // `onToggleConnection` and `onDisconnect` both close a live session with + // `try { await disconnect() } finally { finalizeExplicitDisconnect() }` — a + // `finally`, not a `catch` — so a transport that fails to close rejects out + // of the handler. App discards neither: both are terminated with a `.catch` + // that toasts, because a bare `void` would turn an ordinary click into a + // global unhandled rejection with nothing shown to the user (#2130 review). + const CLOSE_FAILURE = new Error("close boom"); + + it("toasts a failed transport close from the connection toggle instead of leaking it", async () => { + const rejections = captureUnhandledRejections(); + try { + const user = await connect(); + const client = clientInstances[0] as EventTarget & { + disconnect: ReturnType; + }; + client.disconnect.mockRejectedValueOnce(CLOSE_FAILURE); + + // Second click on the same, now-connected server takes the disconnect + // branch of the toggle. + await user.click(screen.getByText("connect")); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Failed to change the connection", + message: "close boom", + color: "red", + }), + ), + ); + await settleRejections(); + expect(rejections.seen).toEqual([]); + } finally { + rejections.stop(); + } + }); + + // The four command handlers await `handleCommandScopedAuthRecovery` from + // *inside* their catch blocks, and a rejection thrown from a catch is not + // caught by that same catch — so it escapes the handler entirely. That helper + // awaits `checkAuthChallengeSatisfied`, which reaches the backend and can + // reject. Before #2130's review this escaped a bare `void` as a global + // unhandled rejection with nothing shown to the user. + it("toasts an auth-recovery failure that escapes a tool call instead of leaking it", async () => { + const rejections = captureUnhandledRejections(); + try { + const user = await connect(); + const client = clientInstances[0] as EventTarget & { + callTool: ReturnType; + }; + client.callTool.mockRejectedValueOnce( + new AuthRecoveryRequiredError( + new URL("https://as.example.com/authorize"), + { reason: "unauthorized" }, + ), + ); + // The recovery helper's first await is the one that breaks. + rejectNextChallengeCheck(new Error("challenge check failed")); + + await user.click(screen.getByText("call")); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Tool call failed", + message: "challenge check failed", + color: "red", + }), + ), + ); + await settleRejections(); + expect(rejections.seen).toEqual([]); + } finally { + rejections.stop(); + } + }); + + // The reorder handler was lifted out of the JSX into a named callback with a + // `.catch` that toasts (#2130). `reorderServers` reverts its own optimistic + // ordering and re-throws — on a 409 from a racing external edit, or a network + // error — so without the toast the drag just silently bounces back. + it("forwards a reorder to the server list", async () => { + const reorderServers = vi.fn().mockResolvedValue(undefined); + vi.mocked(useServers).mockReturnValue(serversWithReorder(reorderServers)); + const user = userEvent.setup(); + renderWithMantine(); + + await user.click(screen.getByText("reorder-servers")); + + await waitFor(() => + expect(reorderServers).toHaveBeenCalledWith(["B", "A"]), + ); + expect(notificationsMock.show).not.toHaveBeenCalled(); + }); + + it("toasts a failed reorder instead of leaking it", async () => { + const reorderServers = vi + .fn() + .mockRejectedValue(new Error("stale server list")); + vi.mocked(useServers).mockReturnValue(serversWithReorder(reorderServers)); + const rejections = captureUnhandledRejections(); + try { + const user = userEvent.setup(); + renderWithMantine(); + + await user.click(screen.getByText("reorder-servers")); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Failed to reorder servers", + message: "stale server list", + color: "red", + }), + ), + ); + await settleRejections(); + expect(rejections.seen).toEqual([]); + } finally { + rejections.stop(); + } + }); + + it("toasts a failed transport close from the explicit Disconnect instead of leaking it", async () => { + const rejections = captureUnhandledRejections(); + try { + const user = await connect(); + const client = clientInstances[0] as EventTarget & { + disconnect: ReturnType; + }; + client.disconnect.mockRejectedValueOnce(CLOSE_FAILURE); + + await user.click(screen.getByText("disconnect")); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Failed to disconnect", + message: "close boom", + color: "red", + }), + ), + ); + await settleRejections(); + expect(rejections.seen).toEqual([]); + } finally { + rejections.stop(); + } + }); }); /** A complete `useManagedResources` return, so the mock stays type-checked against the hook's contract. */ diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 91bf2c29..16f38cd4 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -64,6 +64,20 @@ import { useInitialConfig } from "@inspector/core/react/useInitialConfig.js"; import { refreshingPersist } from "./lib/refreshingPersist"; import { usePendingClientRequests } from "@inspector/core/react/usePendingClientRequests.js"; import { InspectorView } from "./components/views/InspectorView/InspectorView"; +import type { + AppsPanelProps, + ConnectionProps, + ConsolePanelProps, + LogsPanelProps, + NetworkPanelProps, + PromptsPanelProps, + ProtocolPanelProps, + ResourcesPanelProps, + ServerListProps, + ShellProps, + TasksPanelProps, + ToolsPanelProps, +} from "./components/views/InspectorView/types"; import type { ReadResourceState } from "./components/screens/ResourcesScreen/ResourcesScreen"; import { AppElicitationHost } from "./components/elements/AppElicitation/AppElicitationHost"; import type { LogEntryData } from "./components/elements/LogEntry/LogEntry"; @@ -92,6 +106,7 @@ import { type PendingClientRequestContent, } from "./components/groups/PendingClientRequestModal/PendingClientRequestModal"; import { downloadJsonFile } from "./lib/downloadFile"; +import { serverWithDraftSettings } from "./utils/serverWithDraftSettings"; import { enrichProtocolEntries } from "./utils/correlateTransportErrors"; import { visibleMalformedListItems } from "./utils/malformedListReport"; import { parseDeepLink, deepLinkParseStatus } from "./utils/deepLink"; @@ -108,6 +123,21 @@ import { FetchBodyDroppedToastMessage } from "./components/elements/Toasts/Fetch import { OutputValidationToastMessage } from "./components/elements/Toasts/OutputValidationToastMessage"; import { ReAuthBannerBar } from "./components/groups/ReAuthBanner/ReAuthBannerBar"; +/** + * Terminates a dispatched handler's promise by reporting the failure, for the + * `InspectorView` action wrappers below. Module scope because it closes over + * nothing — the title is the only thing that varies per action. + */ +function reportDispatchFailure(title: string) { + return (err: unknown) => { + notifications.show({ + title, + message: err instanceof Error ? err.message : String(err), + color: "red", + }); + }; +} + function App() { const { onToggleTheme } = useThemeToggle(); @@ -1319,15 +1349,63 @@ function App() { // target isn't resolvable. const settingsModalIsStdio = settingsModalServerType === "stdio"; + /** + * Servers whose clear is in flight (#2144). Keyed by id, not a single flag: + * the callback explicitly supports clearing a server other than the active + * one, so a global lock would silently drop B's click while A's revocation + * was still out. See `runClear`. + */ + const clearOAuthInFlightRef = useRef>(new Set()); + + /** + * Run a clear from a key/click handler, which cannot await. + * + * `clearServerOAuthAndDisconnect` does **not** own its failures — the store + * write or the disconnect can reject — so a bare `void` would produce an + * unhandled rejection and leave the user with a control that silently did + * nothing. (An RFC 7009 failure is not this: those come back as an outcome + * and are already reported in the success toast.) + */ + const runClear = useCallback( + (server: Parameters[0]) => { + // Shared by BOTH clear controls (Connection Info and Server Settings), + // because for one server they drive the same client and the same store + // entry. A ref rather than state: a double click delivers both events + // before React re-renders, so a state-based guard would let both through + // — and with revocation taking up to five seconds, that means concurrent + // RFC 7009 requests, concurrent store writes, and two contradictory + // toasts. Keyed by server so a *different* server's clear is unaffected. + if (clearOAuthInFlightRef.current.has(server.id)) return; + clearOAuthInFlightRef.current.add(server.id); + clearServerOAuthAndDisconnect(server) + .finally(() => { + clearOAuthInFlightRef.current.delete(server.id); + }) + .catch((err: unknown) => { + notifications.show({ + title: "Could not clear the stored OAuth state", + message: err instanceof Error ? err.message : String(err), + color: "red", + // The tokens may still be on disk and the session may still be up, + // so this is not a notice to let time out. + autoClose: false, + }); + }); + }, + [clearServerOAuthAndDisconnect], + ); + const handleClearConnectionOAuth = useCallback(() => { if (!activeServer) return; - void clearServerOAuthAndDisconnect(activeServer); - }, [activeServer, clearServerOAuthAndDisconnect]); + runClear(activeServer); + }, [activeServer, runClear]); 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 rather than what the debounced save has persisted (#2144). + runClear(serverWithDraftSettings(settingsModalTarget, settingsDraft)); + }, [settingsModalTarget, settingsDraft, runClear]); const onSettingsModalClose = useCallback(() => { flushSettingsDraft(); @@ -1469,6 +1547,304 @@ function App() { [pendingElicitations, inspectorClient], ); + // --- InspectorView prop bundles (#2130) ----------------------------------- + // + // The view takes one prop per domain, so the JSX below reads as a component + // tree rather than a wall of ~130 props. Everything the bundles need is + // assembled here: the handlers that used to be multi-line closures declared + // inline in the JSX are named callbacks, and the `void`-discarding wrappers + // carry their justification once rather than at a call site. + + const onServerAdd = useCallback(() => { + setHighlightedServerIds([]); + setConfigModal({ mode: "add" }); + }, []); + + const onServerImportConfig = useCallback(() => { + setHighlightedServerIds([]); + setImportConfigOpen(true); + }, []); + + const onServerImportJson = useCallback(() => { + setHighlightedServerIds([]); + setImportJsonOpen(true); + }, []); + + const onServerEdit = useCallback((id: string) => { + setConfigModal({ mode: "edit", targetId: id }); + }, []); + + const onServerClone = useCallback((id: string) => { + setHighlightedServerIds([]); + setConfigModal({ mode: "clone", targetId: id }); + }, []); + + const onServerRemove = useCallback( + (id: string) => { + const target = servers.find((s) => s.id === id); + if (target) setRemoveTarget(target); + }, + [servers], + ); + + const onServerReorderFromList = useCallback( + (orderedIds: string[]) => { + // reorderServers reverts the optimistic order via an internal refresh() + // and re-throws on failure (409 from a racing external edit, or a network + // error). Surface that to the user so the drag doesn't silently bounce + // back — matching the toast pattern every other mutation here uses. + reorderServers(orderedIds).catch((err: unknown) => { + notifications.show({ + title: "Failed to reorder servers", + message: err instanceof Error ? err.message : String(err), + color: "red", + }); + }); + }, + [reorderServers], + ); + + const openClientSettings = useCallback(() => setClientSettingsOpen(true), []); + const openConnectionInfo = useCallback( + () => setConnectionInfoModalOpen(true), + [], + ); + const openServerSettings = useCallback( + (id: string) => setSettingsModalTargetId(id), + [], + ); + + // Every `InspectorView` action prop is synchronous while the handler behind + // it is async, so each wrapper below has to terminate its promise somehow. + // They all do it the same way: a reporting `.catch`, never a bare `void`. + // + // The uniformity is the point. Deciding per handler whether it "owns its + // failures" is a judgement this PR got wrong twice (#2130 review) — the + // handlers really do catch their ordinary errors, but a rejection can still + // escape by a route the reading misses: + // + // - `onToggleConnection` awaits `initialConfigSettledRef` and constructs + // the client via `setupClientForServer` outside its own try/catch, and + // its already-connected branch is a `try/finally` that lets a failed + // transport close propagate. `onDisconnect` is that `try/finally` alone. + // - `onCallTool` / `onGetPrompt` / `onReadResource` / `onOpenApp` await + // `handleCommandScopedAuthRecovery` from *inside* their catch blocks, and + // a rejection thrown from a catch is not caught by that same catch. That + // helper awaits `checkAuthChallengeSatisfied` and `pushRemoteAuthState`, + // both of which reach the backend and can reject. + // + // A handler that does surface its own failure resolves, so it never reaches + // this `.catch` and there is no double-toast; what lands here is only the + // escape, which would otherwise be a global unhandled rejection with nothing + // shown to the user. + const dispatchToggleConnection = useCallback( + (id: string) => { + onToggleConnection(id).catch( + reportDispatchFailure("Failed to change the connection"), + ); + }, + [onToggleConnection], + ); + const dispatchDisconnect = useCallback(() => { + onDisconnect().catch(reportDispatchFailure("Failed to disconnect")); + }, [onDisconnect]); + const dispatchCallTool = useCallback( + (name: string, args: Record, runAsTask?: boolean) => { + onCallTool(name, args, runAsTask).catch( + reportDispatchFailure("Tool call failed"), + ); + }, + [onCallTool], + ); + const dispatchGetPrompt = useCallback( + (name: string, args: Record) => { + onGetPrompt(name, args).catch( + reportDispatchFailure("Failed to get prompt"), + ); + }, + [onGetPrompt], + ); + const dispatchReadResource = useCallback( + (uri: string) => { + onReadResource(uri).catch( + reportDispatchFailure("Failed to read resource"), + ); + }, + [onReadResource], + ); + const dispatchCancelTask = useCallback( + (taskId: string) => { + onCancelTask(taskId).catch( + reportDispatchFailure("Failed to cancel task"), + ); + }, + [onCancelTask], + ); + const dispatchOpenApp = useCallback( + (name: string, args: Record) => { + onOpenApp(name, args).catch(reportDispatchFailure("Failed to open app")); + }, + [onOpenApp], + ); + + const shellProps: ShellProps = { + deepLink, + deepLinkStatus, + version: inspectorVersion, + malformedListItems: shownMalformedListItems, + activeTab, + onActiveTabChange: setActiveTab, + onToggleTheme, + onOpenClientSettings: openClientSettings, + }; + + const connectionProps: ConnectionProps = { + activeServer: activeServerId, + erroredServerId: failedServerId, + connectedServerId, + connectionStatus, + connectErrorMessage, + initializeResult, + latencyMs, + protocolEra, + onCompleteArgument, + completionsSupported: capabilities?.completions !== undefined, + onToggleConnection: dispatchToggleConnection, + onDisconnect: dispatchDisconnect, + }; + + const serverListProps: ServerListProps = { + servers, + serverListWritable, + highlightedServerIds, + onClearHighlight: clearHighlight, + onServerAdd, + onServerImportConfig, + onServerImportJson, + onServerExport, + onConnectionInfo: openConnectionInfo, + onServerSettings: openServerSettings, + onServerEdit, + onServerClone, + onServerRemove, + onServerReorder: onServerReorderFromList, + }; + + const toolsPanelProps: ToolsPanelProps = { + tools, + excludedTools, + toolsListChanged, + toolsLoadError: toolsPagination.error, + toolsUi: ui.toolsUi, + toolCallState, + toolsPagination: toolsPaginationControls, + serverSupportsTaskToolCalls: + !!capabilities?.tasks?.requests?.tools?.call || + (inspectorClient?.isTasksExtensionNegotiated() ?? false), + onToolsUiChange, + onCallTool: dispatchCallTool, + onCancelToolCall, + onClearToolResult, + onRefreshTools, + onReadResourceContents, + }; + + const promptsPanelProps: PromptsPanelProps = { + prompts, + promptsListChanged, + promptsLoadError: promptsPagination.error, + promptsUi: ui.promptsUi, + getPromptState, + promptsPagination: promptsPaginationControls, + onPromptsUiChange: setUi.setPromptsUi, + onGetPrompt: dispatchGetPrompt, + onRefreshPrompts, + }; + + const resourcesPanelProps: ResourcesPanelProps = { + resources, + resourceTemplates, + subscriptions, + subscriptionStreamState, + subscriptionsSupported: capabilities?.resources?.subscribe === true, + resourcesListChanged, + resourcesLoadError: resourcesPagination.error ?? resourceTemplatesLoadError, + resourcesUi: ui.resourcesUi, + readResourceState: effectiveReadResourceState, + resourcesPagination: resourcesPaginationControls, + onResourcesUiChange: setUi.setResourcesUi, + onReadResource: dispatchReadResource, + onSubscribeResource, + onUnsubscribeResource, + onRefreshResources, + }; + + const appsPanelProps: AppsPanelProps = { + appsUi: ui.appsUi, + sandboxPath: sandboxUrl, + bridgeFactory: sandboxBridgeFactory, + appRendererRef, + onAppsUiChange: setUi.setAppsUi, + onSelectApp, + onOpenApp: dispatchOpenApp, + onCloseApp, + onAppError, + // The Apps tab is a filtered view of the tools list, so refreshing it is + // refreshing tools. + onRefreshApps: onRefreshTools, + }; + + const tasksPanelProps: TasksPanelProps = { + tasks, + progressByTaskId, + tasksUi: ui.tasksUi, + onTasksUiChange: setUi.setTasksUi, + onCancelTask: dispatchCancelTask, + onClearCompletedTasks, + onRefreshTasks, + }; + + const logsPanelProps: LogsPanelProps = { + logs, + logsUi: ui.logsUi, + currentLogLevel, + modernLogLevel, + onSetLogLevel, + onSetModernLogLevel, + onLogsUiChange: setUi.setLogsUi, + onClearLogs, + onExportLogs, + }; + + const protocolPanelProps: ProtocolPanelProps = { + protocol: protocolEntries, + protocolUi: ui.protocolUi, + pinnedProtocolIds, + onProtocolUiChange: setUi.setProtocolUi, + onClearProtocol, + onExportProtocol, + onClearProtocolSection, + onExportProtocolSection, + onReplayProtocol, + onTogglePinProtocol: togglePinProtocol, + }; + + const networkPanelProps: NetworkPanelProps = { + network: fetchRequests, + networkUi: ui.networkUi, + onNetworkUiChange: setUi.setNetworkUi, + onClearNetwork, + onExportNetwork, + }; + + const consolePanelProps: ConsolePanelProps = { + stderrLogs, + consoleUi: ui.consoleUi, + onConsoleUiChange: setUi.setConsoleUi, + onClearConsole, + onExportConsole, + }; + return ( <> @@ -1484,171 +1860,18 @@ function App() { ) : null} setClientSettingsOpen(true)} - onToggleConnection={(id) => { - void onToggleConnection(id); - }} - onDisconnect={() => { - void onDisconnect(); - }} - onServerAdd={() => { - setHighlightedServerIds([]); - setConfigModal({ mode: "add" }); - }} - onServerImportConfig={() => { - setHighlightedServerIds([]); - setImportConfigOpen(true); - }} - onServerImportJson={() => { - setHighlightedServerIds([]); - setImportJsonOpen(true); - }} - onServerExport={onServerExport} - onConnectionInfo={() => setConnectionInfoModalOpen(true)} - onServerSettings={(id) => setSettingsModalTargetId(id)} - onServerEdit={(id) => setConfigModal({ mode: "edit", targetId: id })} - onServerClone={(id) => { - setHighlightedServerIds([]); - setConfigModal({ mode: "clone", targetId: id }); - }} - onServerRemove={(id) => { - const target = servers.find((s) => s.id === id); - if (target) setRemoveTarget(target); - }} - onServerReorder={(orderedIds) => { - // reorderServers reverts the optimistic order via an internal - // refresh() and re-throws on failure (409 from a racing external - // edit, or a network error). Surface that to the user so the drag - // doesn't silently bounce back — matching the toast pattern every - // other mutation here uses. - reorderServers(orderedIds).catch((err: unknown) => { - notifications.show({ - title: "Failed to reorder servers", - message: err instanceof Error ? err.message : String(err), - color: "red", - }); - }); - }} - highlightedServerIds={highlightedServerIds} - onClearHighlight={clearHighlight} - serverSupportsTaskToolCalls={ - !!capabilities?.tasks?.requests?.tools?.call || - (inspectorClient?.isTasksExtensionNegotiated() ?? false) - } - onToolsUiChange={onToolsUiChange} - onCallTool={(name, args, runAsTask) => { - void onCallTool(name, args, runAsTask); - }} - onCancelToolCall={onCancelToolCall} - onClearToolResult={onClearToolResult} - onReadResourceContents={onReadResourceContents} - onRefreshTools={onRefreshTools} - toolsPagination={toolsPaginationControls} - promptsPagination={promptsPaginationControls} - resourcesPagination={resourcesPaginationControls} - onPromptsUiChange={setUi.setPromptsUi} - onGetPrompt={(name, args) => { - void onGetPrompt(name, args); - }} - onRefreshPrompts={onRefreshPrompts} - onResourcesUiChange={setUi.setResourcesUi} - onReadResource={(uri) => { - void onReadResource(uri); - }} - onSubscribeResource={onSubscribeResource} - onUnsubscribeResource={onUnsubscribeResource} - onRefreshResources={onRefreshResources} - onCompleteArgument={onCompleteArgument} - completionsSupported={capabilities?.completions !== undefined} - subscriptionsSupported={capabilities?.resources?.subscribe === true} - onTasksUiChange={setUi.setTasksUi} - onCancelTask={(taskId) => { - void onCancelTask(taskId); - }} - onClearCompletedTasks={onClearCompletedTasks} - onRefreshTasks={onRefreshTasks} - onSetLogLevel={onSetLogLevel} - modernLogLevel={modernLogLevel} - onSetModernLogLevel={onSetModernLogLevel} - onLogsUiChange={setUi.setLogsUi} - onClearLogs={onClearLogs} - onExportLogs={onExportLogs} - onProtocolUiChange={setUi.setProtocolUi} - onClearProtocol={onClearProtocol} - onExportProtocol={onExportProtocol} - onClearProtocolSection={onClearProtocolSection} - onExportProtocolSection={onExportProtocolSection} - onReplayProtocol={onReplayProtocol} - onTogglePinProtocol={togglePinProtocol} - pinnedProtocolIds={pinnedProtocolIds} - onNetworkUiChange={setUi.setNetworkUi} - onClearNetwork={onClearNetwork} - onExportNetwork={onExportNetwork} - onConsoleUiChange={setUi.setConsoleUi} - onClearConsole={onClearConsole} - onExportConsole={onExportConsole} - onAppsUiChange={setUi.setAppsUi} - onSelectApp={onSelectApp} - onOpenApp={(name, args) => { - void onOpenApp(name, args); - }} - onCloseApp={onCloseApp} - onAppError={onAppError} - onRefreshApps={onRefreshTools} + shell={shellProps} + connection={connectionProps} + servers={serverListProps} + tools={toolsPanelProps} + prompts={promptsPanelProps} + resources={resourcesPanelProps} + apps={appsPanelProps} + tasks={tasksPanelProps} + logs={logsPanelProps} + protocol={protocolPanelProps} + network={networkPanelProps} + console={consolePanelProps} /> { tokenUrl: "", enterpriseManaged: false, requestRefreshToken: true, + revokeOnClear: true, }); }); @@ -751,6 +752,47 @@ describe("ServerSettingsForm", () => { ); }); + // #2144 — the RFC 7009 opt-out. On by default: leaving it off silently is + // what leaves the authorization server holding a live grant. + it("renders Revoke tokens on clear checked by default", () => { + renderWithMantine( + , + ); + expect(screen.getByLabelText("Revoke tokens on clear")).toBeChecked(); + }); + + it("renders Revoke tokens on clear unchecked when the server opted out", () => { + renderWithMantine( + , + ); + expect(screen.getByLabelText("Revoke tokens on clear")).not.toBeChecked(); + }); + + it("toggles Revoke tokens on clear through onOAuthChange", async () => { + const user = userEvent.setup(); + const onOAuthChange = vi.fn(); + renderWithMantine( + , + ); + await user.click(screen.getByLabelText("Revoke tokens on clear")); + expect(onOAuthChange).toHaveBeenLastCalledWith( + expect.objectContaining({ revokeOnClear: false }), + ); + }); + it("warns when opting out while offline_access is still in Scopes", () => { renderWithMantine( { tokenUrl: "", enterpriseManaged: true, requestRefreshToken: true, + revokeOnClear: true, }); }); @@ -1363,6 +1406,7 @@ describe("ServerSettingsForm", () => { tokenUrl: "", enterpriseManaged: false, requestRefreshToken: true, + revokeOnClear: true, }); }); diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx index fec3f25a..0ac753f9 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx @@ -521,6 +521,8 @@ export function ServerSettingsForm({ onInsufficientScope: settings.oauthOnInsufficientScope, // Unset means the default, on — only an explicit opt-out is stored. requestRefreshToken: settings.oauthRequestRefreshToken ?? true, + // Unset means the default, on — only an explicit opt-out is stored. + revokeOnClear: settings.oauthRevokeOnClear ?? true, }; } @@ -570,6 +572,15 @@ export function ServerSettingsForm({ }); } + function handleRevokeOnClearChange( + event: ChangeEvent, + ): void { + onOAuthChange({ + ...currentOAuth(), + revokeOnClear: event.currentTarget.checked, + }); + } + function handleAddAuthorizationParam(): void { changeAuthorizationParams([...authorizationParams, { key: "", value: "" }]); } @@ -917,6 +928,12 @@ export function ServerSettingsForm({ checked={settings.oauthRequestRefreshToken ?? true} onChange={handleRequestRefreshTokenChange} /> + {refreshTokenOptedOut && scopesIncludeOfflineAccess ? ( The Scopes field above lists offline_access, and diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx index d9933631..0d704e42 100644 --- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx @@ -156,6 +156,42 @@ describe("ServerSettingsModal", () => { ); }); + // #2144 — same omit-the-default shape as the refresh-token pair above. + it("maps the revoke-on-clear opt-out into settings, and back to unset", async () => { + const user = userEvent.setup(); + const onSettingsChange = vi.fn(); + const { rerender } = renderWithMantine( + , + ); + await user.click(screen.getByRole("button", { name: /OAuth Settings/i })); + await user.click(screen.getByLabelText("Revoke tokens on clear")); + expect(onSettingsChange).toHaveBeenLastCalledWith( + expect.objectContaining({ oauthRevokeOnClear: false }), + ); + + rerender( + , + ); + await user.click(screen.getByLabelText("Revoke tokens on clear")); + expect(onSettingsChange).toHaveBeenLastCalledWith( + expect.objectContaining({ oauthRevokeOnClear: undefined }), + ); + }); + it("maps the selected protocol era into settings (#1626)", async () => { const user = userEvent.setup(); const onSettingsChange = vi.fn(); diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx index ef6394ab..b8a422c0 100644 --- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx +++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx @@ -191,6 +191,8 @@ export function ServerSettingsModal({ // server that never touched the switch writes no field at all. oauthRequestRefreshToken: oauth.requestRefreshToken === false ? false : undefined, + // #2144: same shape — `undefined` means on, so only the opt-out persists. + oauthRevokeOnClear: oauth.revokeOnClear === false ? false : undefined, }); } diff --git a/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx b/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx index fd7f25ac..24054fda 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx @@ -18,6 +18,20 @@ import type { Meta, StoryObj } from "@storybook/react-vite"; import { useState } from "react"; import { expect, fn } from "storybook/test"; import { InspectorView } from "./InspectorView"; +import type { + AppsPanelProps, + ConnectionProps, + ConsolePanelProps, + LogsPanelProps, + NetworkPanelProps, + PromptsPanelProps, + ProtocolPanelProps, + ResourcesPanelProps, + ServerListProps, + ShellProps, + TasksPanelProps, + ToolsPanelProps, +} from "./types"; import { noopPagination } from "../../../test/fixtures/pagination"; import { EMPTY_TOOLS_UI, @@ -324,116 +338,164 @@ const demoInitializeResult: InitializeResult = { serverInfo: { name: "Local Dev Server", version: "1.2.0" }, }; +// One constant per domain bundle, so a story can override a single field +// without replacing its whole bundle — Storybook merges args only at the top +// level, and each of these IS one top-level arg now (#2130). +const shellArgs: ShellProps = { + activeTab: "Servers", + onActiveTabChange: fn(), + onToggleTheme: fn(), + onOpenClientSettings: fn(), +}; + +// Stories default to "disconnected"; per-story overrides drive the connected / +// error narratives. +const connectionArgs: ConnectionProps = { + activeServer: undefined, + connectionStatus: "disconnected", + initializeResult: undefined, + latencyMs: undefined, + onToggleConnection: fn(), + onDisconnect: fn(), +}; + +// Callbacks are wired to storybook spies so play functions can assert on +// dispatch. Real wiring routes these to InspectorClient methods (the app shell +// at clients/web/src/App.tsx). +const serversArgs: ServerListProps = { + servers: demoServers, + onServerAdd: fn(), + onServerImportConfig: fn(), + onServerImportJson: fn(), + onServerExport: fn(), + onConnectionInfo: fn(), + onServerSettings: fn(), + onServerEdit: fn(), + onServerClone: fn(), + onServerRemove: fn(), + onServerReorder: fn(), +}; + +const toolsArgs: ToolsPanelProps = { + tools: demoTools, + toolsListChanged: false, + toolsUi: EMPTY_TOOLS_UI, + toolsPagination: noopPagination, + serverSupportsTaskToolCalls: false, + onToolsUiChange: fn(), + onCallTool: fn(), + onRefreshTools: fn(), +}; + +const promptsArgs: PromptsPanelProps = { + prompts: demoPrompts, + promptsListChanged: false, + promptsUi: EMPTY_PROMPTS_UI, + promptsPagination: noopPagination, + onPromptsUiChange: fn(), + onGetPrompt: fn(), + onRefreshPrompts: fn(), +}; + +const resourcesArgs: ResourcesPanelProps = { + resources: demoResources, + resourceTemplates: demoResourceTemplates, + subscriptions: demoSubscriptions, + resourcesListChanged: false, + resourcesUi: EMPTY_RESOURCES_UI, + resourcesPagination: noopPagination, + onResourcesUiChange: fn(), + onReadResource: fn(), + onSubscribeResource: fn(), + onUnsubscribeResource: fn(), + onRefreshResources: fn(), +}; + +const appsArgs: AppsPanelProps = { + appsUi: EMPTY_APPS_UI, + sandboxPath: "about:blank", + bridgeFactory: noopBridgeFactory, + appRendererRef: { current: null }, + onAppsUiChange: fn(), + onSelectApp: fn(), + onOpenApp: fn(), + onCloseApp: fn(), + onAppError: fn(), + onRefreshApps: fn(), +}; + +const tasksArgs: TasksPanelProps = { + tasks: demoTasks, + progressByTaskId: demoProgressByTaskId, + tasksUi: EMPTY_TASKS_UI, + onTasksUiChange: fn(), + onCancelTask: fn(), + onClearCompletedTasks: fn(), + onRefreshTasks: fn(), +}; + +const logsArgs: LogsPanelProps = { + logs: demoLogs, + logsUi: EMPTY_LOGS_UI, + currentLogLevel: "info", + onSetLogLevel: fn(), + onLogsUiChange: fn(), + onClearLogs: fn(), + onExportLogs: fn(), +}; + +const protocolArgs: ProtocolPanelProps = { + protocol: demoHistory, + protocolUi: EMPTY_PROTOCOL_UI, + onProtocolUiChange: fn(), + onClearProtocol: fn(), + onExportProtocol: fn(), + onClearProtocolSection: fn(), + onExportProtocolSection: fn(), + onReplayProtocol: fn(), + onTogglePinProtocol: fn(), +}; + +const networkArgs: NetworkPanelProps = { + network: demoNetwork, + networkUi: EMPTY_NETWORK_UI, + onNetworkUiChange: fn(), + onClearNetwork: fn(), + onExportNetwork: fn(), +}; + +const consoleArgs: ConsolePanelProps = { + stderrLogs: demoStderr, + consoleUi: EMPTY_CONSOLE_UI, + onConsoleUiChange: fn(), + onClearConsole: fn(), + onExportConsole: fn(), +}; + const meta: Meta = { title: "Views/InspectorView", component: InspectorView, parameters: { layout: "fullscreen" }, args: { - // Data - servers: demoServers, - tools: demoTools, - prompts: demoPrompts, - resources: demoResources, - resourceTemplates: demoResourceTemplates, - subscriptions: demoSubscriptions, - logs: demoLogs, - tasks: demoTasks, - progressByTaskId: demoProgressByTaskId, - protocol: demoHistory, - network: demoNetwork, - stderrLogs: demoStderr, - - // Connection state — stories default to "disconnected"; per-story - // overrides drive the connected / error narratives. - activeServer: undefined, - connectionStatus: "disconnected", - initializeResult: undefined, - latencyMs: undefined, - - // Misc state - currentLogLevel: "info", - sandboxPath: "about:blank", - bridgeFactory: noopBridgeFactory, - appRendererRef: { current: null }, - - // Per-screen UI state (search / filter / selection), one object per screen. - toolsUi: EMPTY_TOOLS_UI, - promptsUi: EMPTY_PROMPTS_UI, - resourcesUi: EMPTY_RESOURCES_UI, - appsUi: EMPTY_APPS_UI, - tasksUi: EMPTY_TASKS_UI, - logsUi: EMPTY_LOGS_UI, - protocolUi: EMPTY_PROTOCOL_UI, - networkUi: EMPTY_NETWORK_UI, - consoleUi: EMPTY_CONSOLE_UI, - - // Callbacks — all wired to storybook spies so play functions can assert - // on dispatch. Real wiring routes these to InspectorClient methods (the - // app shell at clients/web/src/App.tsx). - onToggleTheme: fn(), - onOpenClientSettings: fn(), - onToggleConnection: fn(), - onDisconnect: fn(), - onServerAdd: fn(), - onServerImportConfig: fn(), - onServerImportJson: fn(), - onServerExport: fn(), - onConnectionInfo: fn(), - onServerSettings: fn(), - onServerEdit: fn(), - onServerClone: fn(), - onServerRemove: fn(), - onServerReorder: fn(), - serverSupportsTaskToolCalls: false, - onToolsUiChange: fn(), - onCallTool: fn(), - onRefreshTools: fn(), - toolsPagination: noopPagination, - promptsPagination: noopPagination, - resourcesPagination: noopPagination, - onPromptsUiChange: fn(), - onGetPrompt: fn(), - onRefreshPrompts: fn(), - onResourcesUiChange: fn(), - onReadResource: fn(), - onSubscribeResource: fn(), - onUnsubscribeResource: fn(), - onRefreshResources: fn(), - onTasksUiChange: fn(), - onCancelTask: fn(), - onClearCompletedTasks: fn(), - onRefreshTasks: fn(), - onSetLogLevel: fn(), - onLogsUiChange: fn(), - onClearLogs: fn(), - onExportLogs: fn(), - onProtocolUiChange: fn(), - onClearProtocol: fn(), - onExportProtocol: fn(), - onReplayProtocol: fn(), - onTogglePinProtocol: fn(), - onNetworkUiChange: fn(), - onClearNetwork: fn(), - onExportNetwork: fn(), - onConsoleUiChange: fn(), - onClearConsole: fn(), - onExportConsole: fn(), - onAppsUiChange: fn(), - onSelectApp: fn(), - onOpenApp: fn(), - onCloseApp: fn(), - onAppError: fn(), - onRefreshApps: fn(), - activeTab: "Servers", - onActiveTabChange: fn(), + shell: shellArgs, + connection: connectionArgs, + servers: serversArgs, + tools: toolsArgs, + prompts: promptsArgs, + resources: resourcesArgs, + apps: appsArgs, + tasks: tasksArgs, + logs: logsArgs, + protocol: protocolArgs, + network: networkArgs, + console: consoleArgs, }, render: (args) => { - const [activeTab, setActiveTab] = useState(args.activeTab ?? "Servers"); + const [activeTab, setActiveTab] = useState(args.shell.activeTab); return ( ); }, @@ -463,7 +525,7 @@ export const Default: Story = { export const NoServers: Story = { args: { - servers: [], + servers: { ...serversArgs, servers: [] }, }, }; @@ -480,7 +542,7 @@ const manyServers: ServerEntry[] = Array.from({ length: 24 }, (_, i) => ({ export const ManyServers: Story = { args: { - servers: manyServers, + servers: { ...serversArgs, servers: manyServers }, }, play: async ({ canvasElement }) => { // Even under enough content to overflow, the shell stays viewport-clamped @@ -509,10 +571,13 @@ export const ManyServers: Story = { // regression / storybook play function coverage. export const Connected: Story = { args: { - activeServer: demoServers[0]!.id, - connectionStatus: "connected", - initializeResult: demoInitializeResult, - latencyMs: 142, + connection: { + ...connectionArgs, + activeServer: demoServers[0]!.id, + connectionStatus: "connected", + initializeResult: demoInitializeResult, + latencyMs: 142, + }, }, }; @@ -524,10 +589,13 @@ export const ConnectionError: Story = { // connect-attempt-failure signal that gates the failure column; `network: []` // (and empty Protocol) keep it a pure stdio failure so only Console is offered. args: { - activeServer: demoServers[0]!.id, - erroredServerId: demoServers[0]!.id, - connectionStatus: "error", - network: [], - stderrLogs: demoStderr, + connection: { + ...connectionArgs, + activeServer: demoServers[0]!.id, + erroredServerId: demoServers[0]!.id, + connectionStatus: "error", + }, + network: { ...networkArgs, network: [] }, + console: { ...consoleArgs, stderrLogs: demoStderr }, }, }; diff --git a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx index fc34c4bb..20fd24db 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx @@ -59,117 +59,175 @@ const noopBridgeFactory: BridgeFactory = () => close: async () => {}, }) as unknown as AppBridge; +/** + * Per-bundle overrides. Each key takes a `Partial` of that bundle, so a test + * names only the field it cares about — `makeProps({ tools: { tools: [t] } })` + * — and inherits the rest of the bundle's defaults. + */ +type PropOverrides = { + [K in keyof InspectorViewProps]?: Partial; +}; + +// Merges one bundle across every supplied override layer, later layers +// winning. Kept generic (rather than a spread over `Object.assign`) so each +// bundle stays typed to its own shape. +function mergeBundle( + key: K, + layers: PropOverrides[], +): Partial { + let merged: Partial = {}; + for (const layer of layers) { + const part = layer[key]; + if (part) merged = { ...merged, ...part }; + } + return merged; +} + // Returns a fresh fixture each call so per-test spies can be asserted on // in isolation. The view is purely prop-driven; every callback is // dispatched up to the parent — these spies stand in for App.tsx's // hook-routed handlers in the real wiring. -function makeProps( - overrides: Partial = {}, -): InspectorViewProps { +// +// Takes any number of override layers so a scenario helper can supply its own +// base (see `connectedHttp` below) and still let the caller override on top. +function makeProps(...overrides: PropOverrides[]): InspectorViewProps { return { - servers: [], - activeServer: undefined, - connectionStatus: "disconnected", - initializeResult: undefined, - latencyMs: undefined, - tools: [], - prompts: [], - resources: [], - resourceTemplates: [], - toolsListChanged: false, - promptsListChanged: false, - resourcesListChanged: false, - subscriptions: [], - logs: [], - tasks: [], - protocol: [], - network: [], - stderrLogs: [], - currentLogLevel: "info", - sandboxPath: "about:blank", - bridgeFactory: noopBridgeFactory, - appRendererRef: { current: null }, - toolsUi: EMPTY_TOOLS_UI, - promptsUi: EMPTY_PROMPTS_UI, - resourcesUi: EMPTY_RESOURCES_UI, - appsUi: EMPTY_APPS_UI, - tasksUi: EMPTY_TASKS_UI, - logsUi: EMPTY_LOGS_UI, - protocolUi: EMPTY_PROTOCOL_UI, - networkUi: EMPTY_NETWORK_UI, - consoleUi: EMPTY_CONSOLE_UI, - onToggleTheme: vi.fn(), - onOpenClientSettings: vi.fn(), - onToggleConnection: vi.fn(), - onDisconnect: vi.fn(), - onServerAdd: vi.fn(), - onServerImportConfig: vi.fn(), - onServerImportJson: vi.fn(), - onServerExport: vi.fn(), - onConnectionInfo: vi.fn(), - onServerSettings: vi.fn(), - onServerEdit: vi.fn(), - onServerClone: vi.fn(), - onServerRemove: vi.fn(), - onServerReorder: vi.fn(), - serverSupportsTaskToolCalls: false, - onToolsUiChange: vi.fn(), - onCallTool: vi.fn(), - onRefreshTools: vi.fn(), - toolsPagination: noopPagination, - promptsPagination: noopPagination, - resourcesPagination: noopPagination, - onPromptsUiChange: vi.fn(), - onGetPrompt: vi.fn(), - onRefreshPrompts: vi.fn(), - onResourcesUiChange: vi.fn(), - onReadResource: vi.fn(), - onSubscribeResource: vi.fn(), - onUnsubscribeResource: vi.fn(), - onRefreshResources: vi.fn(), - onTasksUiChange: vi.fn(), - onCancelTask: vi.fn(), - onClearCompletedTasks: vi.fn(), - onRefreshTasks: vi.fn(), - onSetLogLevel: vi.fn(), - onLogsUiChange: vi.fn(), - onClearLogs: vi.fn(), - onExportLogs: vi.fn(), - onProtocolUiChange: vi.fn(), - onClearProtocol: vi.fn(), - onExportProtocol: vi.fn(), - onClearProtocolSection: vi.fn(), - onExportProtocolSection: vi.fn(), - onReplayProtocol: vi.fn(), - onTogglePinProtocol: vi.fn(), - onNetworkUiChange: vi.fn(), - onClearNetwork: vi.fn(), - onExportNetwork: vi.fn(), - onConsoleUiChange: vi.fn(), - onClearConsole: vi.fn(), - onExportConsole: vi.fn(), - onAppsUiChange: vi.fn(), - onSelectApp: vi.fn(), - onOpenApp: vi.fn(), - onCloseApp: vi.fn(), - onAppError: vi.fn(), - onRefreshApps: vi.fn(), - activeTab: "Servers", - onActiveTabChange: vi.fn(), - ...overrides, + shell: { + activeTab: "Servers", + onActiveTabChange: vi.fn(), + onToggleTheme: vi.fn(), + onOpenClientSettings: vi.fn(), + ...mergeBundle("shell", overrides), + }, + connection: { + activeServer: undefined, + connectionStatus: "disconnected", + initializeResult: undefined, + latencyMs: undefined, + onToggleConnection: vi.fn(), + onDisconnect: vi.fn(), + ...mergeBundle("connection", overrides), + }, + servers: { + servers: [], + onServerAdd: vi.fn(), + onServerImportConfig: vi.fn(), + onServerImportJson: vi.fn(), + onServerExport: vi.fn(), + onConnectionInfo: vi.fn(), + onServerSettings: vi.fn(), + onServerEdit: vi.fn(), + onServerClone: vi.fn(), + onServerRemove: vi.fn(), + onServerReorder: vi.fn(), + ...mergeBundle("servers", overrides), + }, + tools: { + tools: [], + toolsListChanged: false, + toolsUi: EMPTY_TOOLS_UI, + toolsPagination: noopPagination, + serverSupportsTaskToolCalls: false, + onToolsUiChange: vi.fn(), + onCallTool: vi.fn(), + onRefreshTools: vi.fn(), + ...mergeBundle("tools", overrides), + }, + prompts: { + prompts: [], + promptsListChanged: false, + promptsUi: EMPTY_PROMPTS_UI, + promptsPagination: noopPagination, + onPromptsUiChange: vi.fn(), + onGetPrompt: vi.fn(), + onRefreshPrompts: vi.fn(), + ...mergeBundle("prompts", overrides), + }, + resources: { + resources: [], + resourceTemplates: [], + subscriptions: [], + resourcesListChanged: false, + resourcesUi: EMPTY_RESOURCES_UI, + resourcesPagination: noopPagination, + onResourcesUiChange: vi.fn(), + onReadResource: vi.fn(), + onSubscribeResource: vi.fn(), + onUnsubscribeResource: vi.fn(), + onRefreshResources: vi.fn(), + ...mergeBundle("resources", overrides), + }, + apps: { + appsUi: EMPTY_APPS_UI, + sandboxPath: "about:blank", + bridgeFactory: noopBridgeFactory, + appRendererRef: { current: null }, + onAppsUiChange: vi.fn(), + onSelectApp: vi.fn(), + onOpenApp: vi.fn(), + onCloseApp: vi.fn(), + onAppError: vi.fn(), + onRefreshApps: vi.fn(), + ...mergeBundle("apps", overrides), + }, + tasks: { + tasks: [], + tasksUi: EMPTY_TASKS_UI, + onTasksUiChange: vi.fn(), + onCancelTask: vi.fn(), + onClearCompletedTasks: vi.fn(), + onRefreshTasks: vi.fn(), + ...mergeBundle("tasks", overrides), + }, + logs: { + logs: [], + logsUi: EMPTY_LOGS_UI, + currentLogLevel: "info", + onSetLogLevel: vi.fn(), + onLogsUiChange: vi.fn(), + onClearLogs: vi.fn(), + onExportLogs: vi.fn(), + ...mergeBundle("logs", overrides), + }, + protocol: { + protocol: [], + protocolUi: EMPTY_PROTOCOL_UI, + onProtocolUiChange: vi.fn(), + onClearProtocol: vi.fn(), + onExportProtocol: vi.fn(), + onClearProtocolSection: vi.fn(), + onExportProtocolSection: vi.fn(), + onReplayProtocol: vi.fn(), + onTogglePinProtocol: vi.fn(), + ...mergeBundle("protocol", overrides), + }, + network: { + network: [], + networkUi: EMPTY_NETWORK_UI, + onNetworkUiChange: vi.fn(), + onClearNetwork: vi.fn(), + onExportNetwork: vi.fn(), + ...mergeBundle("network", overrides), + }, + console: { + stderrLogs: [], + consoleUi: EMPTY_CONSOLE_UI, + onConsoleUiChange: vi.fn(), + onClearConsole: vi.fn(), + onExportConsole: vi.fn(), + ...mergeBundle("console", overrides), + }, }; } function StatefulInspectorViewHost(props: InspectorViewProps) { - const [activeTab, setActiveTab] = useState(props.activeTab ?? "Servers"); - const [appsUi, setAppsUi] = useState(props.appsUi ?? EMPTY_APPS_UI); + const [activeTab, setActiveTab] = useState(props.shell.activeTab); + const [appsUi, setAppsUi] = useState(props.apps.appsUi); return ( ); } @@ -244,14 +302,26 @@ describe("InspectorView", () => { it("renders the server card from the input list", () => { renderWithMantine( - , + , ); expect(screen.getByText("Alpha")).toBeInTheDocument(); }); it("renders the footer row with the version and copyright (#1682)", () => { renderWithMantine( - , + , ); expect(screen.getByText("v9.9.9")).toBeInTheDocument(); expect( @@ -264,7 +334,14 @@ describe("InspectorView", () => { const user = userEvent.setup({ delay: null }); renderWithMantine( , ); await user.click(screen.getByRole("switch")); @@ -275,10 +352,16 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -295,9 +378,15 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -311,11 +400,15 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -336,12 +429,16 @@ describe("InspectorView", () => { renderWithMantine( , @@ -358,12 +455,16 @@ describe("InspectorView", () => { renderWithMantine( , @@ -381,12 +482,16 @@ describe("InspectorView", () => { renderWithMantine( , @@ -405,12 +510,16 @@ describe("InspectorView", () => { renderWithMantine( , @@ -427,10 +536,14 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -443,10 +556,14 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -462,10 +579,14 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -482,11 +603,15 @@ describe("InspectorView", () => { const { rerender } = renderWithMantine( , ); @@ -501,9 +626,13 @@ describe("InspectorView", () => { rerender( , ); @@ -530,10 +659,14 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -560,10 +693,14 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -576,13 +713,19 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -599,11 +742,17 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -615,10 +764,14 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -632,10 +785,14 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -648,11 +805,15 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -668,13 +829,19 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -701,12 +868,18 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -727,12 +900,18 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -746,11 +925,17 @@ describe("InspectorView", () => { renderWithMantine( , ); @@ -768,11 +953,17 @@ describe("InspectorView", () => { const { rerender } = renderWithMantine( , ); @@ -784,11 +975,17 @@ describe("InspectorView", () => { rerender( , ); @@ -803,21 +1000,31 @@ describe("InspectorView", () => { renderWithMantine( , @@ -843,21 +1050,29 @@ describe("InspectorView", () => { renderWithMantine( , @@ -900,22 +1115,30 @@ describe("InspectorView", () => { renderWithMantine( , @@ -935,20 +1158,28 @@ describe("InspectorView", () => { renderWithMantine( , @@ -965,11 +1196,17 @@ describe("InspectorView", () => { const { rerender } = renderWithMantine( , ); @@ -985,11 +1222,17 @@ describe("InspectorView", () => { rerender( , ); @@ -1003,14 +1246,20 @@ describe("InspectorView", () => { renderWithMantine(