| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Bob Dickinson
|
a71e6d2012 |
Auth hardening CI gaps + SDK consolidation: EMA leg 2 / scopes (#1527) (#1725)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
Close #1527 acceptance gaps with automated tests (SEP-2468 iss, SEP-2207 offline_access, SEP-2351 discovery shapes, SEP-2352 AS-migration, EraNegotiationFailed→401 unwrap); lean EMA leg 2 and scope helpers on the v2 SDK; drop deprecated TS baseUrl. Reviewed LGTM. |
||
Cliff Hall
|
7fecebed5e |
OAuth: re-key store to (server, issuer) + SEP-837/2350/2352 auth UX (#1694)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
* OAuth: re-key store to (server, issuer) + SEP-837/2350/2352 auth UX (#1625) Re-key the persisted OAuth store to (server, issuer) and add the SEP-837/2350/2352 authorization-hardening UX. The SDK v2 auth() drives the SEP-2352 flow off the provider's ctx.issuer; the Inspector's job is to honor it and round-trip the issuer stamp. SEP-2352 — issuer-bound credentials - ServerOAuthState nests issuer-bound credentials under byIssuer[issuer] = { clientInformation, clientRegistrationKind, tokens }, with an activeIssuer pointer answering the transport's ctx-less per-request bearer read. - Fix a latent bug: OAuthStorageBase re-parsed tokens/client-info through OAuthTokensSchema/OAuthClientInformationSchema, which strip the non-RFC `issuer` field and silently defeat the SDK's discardIfIssuerMismatch. The store re-attaches the stamp from the byIssuer key. - BaseOAuthClientProvider threads ctx.issuer and implements discoveryState()/saveDiscoveryState() (activates the SDK's callback-leg AuthorizationServerMismatchError binding check) and invalidateCredentials(). - Lazy, migration-free upgrade: a legacy pre-1625 blob's top-level clientInformation/tokens are returned as an unkeyed fallback until the first issuer-stamped save promotes them into byIssuer. SEP-837 — application_type - DCR declares application_type: "native" (localhost Inspector). - ClientSettingsForm renders RegistrationRejectedError detail and reflects DCR's deprecated-in-favor-of-CIMD (SEP-991) status. SEP-2350 — step-up scope union - Per-server onInsufficientScope (reauthorize | throw) setting wired ServerSettingsForm -> StoredMCPServer.oauth -> StreamableHTTPClientTransport. - StepUpAuthModal visualizes the prior-union-challenged scope set, tagging each scope `new` vs `already granted`. mcpAuth simplification - Delete the hand-rolled authorizeWithoutRefresh (mishandled issuer stamping); forward forceReauthorization/skipIssuerMetadataValidation to native SDK auth() so both the initial and step-up paths get SEP-2352 stamping. Vocabulary + docs - OAuthStep gains cimd_fetch / issuer_comparison / scope_step_up (recorded on a step-up redirect); the Network tab labels auth requests by OAuth phase. - v2_auth_hardening.md gains an as-built status section. Deferred to the era-aware card: the SdkError(EraNegotiationFailed) -> UnauthorizedError unwrap and finishAuth(URLSearchParams) form (only fire under non-legacy negotiation). Closes #1625 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address @claude review: issuer-stamp source gating + clear/activeIssuer + phase over-match - SEP-2352: stamp the issuer only when the credential actually came from the byIssuer slot (`fromSlot`), not merely when a slot exists. A partially-populated slot alongside a surviving legacy field would otherwise stamp a legacy (possibly cross-AS) credential with the current issuer, defeating discardIfIssuerMismatch. - Issuer-scoped clears no longer promote the cleared issuer to `activeIssuer` (updateIssuerSlot gains a `setActive` flag; save paths keep the promotion). - oauthNetworkPhase matches register/token/authorize as the final path segment (trailing slash tolerated) so `/token/refresh` or `/api/register/foo` no longer over-match. - Tests for all three. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
cliffhall
|
f4fb4b50c9 |
Forward RFC 9207 iss to the v2 SDK (fixes OAuth + EMA leg 1)
SDK v2 added RFC 9207 §2.4 authorization-response issuer validation
(`validateAuthorizationResponseIssuer` / `IssuerMismatchError`) as a mix-up
attack defense. It is strict: when AS metadata advertises
`authorization_response_iss_parameter_supported: true` and no `iss` reaches
`exchangeAuthorization`/`auth()`, the exchange throws.
v1.29.0 had no `iss` parameter and no RFC 9207 validation at all, so dropping
the callback `iss` was harmless. The migration swapped in the stricter SDK
without feeding it the new input, breaking the authorization-code exchange
against any RFC 9207 authorization server (Okta/xaa.dev advertise it):
Issuer mismatch in authorization response (RFC 9207):
expected "https://idp.xaa.dev", received undefined
Found by smoke-testing EMA against live xaa.dev staging; not caught by CI
because the mock IdP never advertised the parameter, so the SDK took its
lenient path (a missing `iss` is only fatal when advertised).
`iss` was dropped in three places, with the plumbing half-built —
`oauthManager`/`inspectorClient` already accepted `iss`, but nothing supplied
or forwarded it:
- `parseOAuthCallbackParams` never extracted `iss` from the callback query
- `mcpAuth` never forwarded `options.iss` to SDK `auth()` — its own comment
("forwarded on v2 upgrade; ignored by v1 SDK auth()") predicted this exact
gap, so standard OAuth was broken identically, not just EMA
- EMA leg 1 (`completeIdpOidcAuthorization`) never passed `iss` to
`exchangeAuthorization`
Thread `iss` end to end: callback query -> CallbackParams -> resumeAfterOAuth
-> completeOAuthFlow -> {mcpAuth | EMA leg 1} -> SDK. The Node callback server
and runner carry it for CLI/TUI.
Tests: `ema-mock-servers.ts` now advertises
`authorization_response_iss_parameter_supported: true` so the mock is at least
as strict as real IdPs — with it absent the SDK silently takes the lenient path
and a dropped `iss` goes unnoticed. Flipping it made three existing tests
reproduce the failure. Adds regression coverage for forwarding and for
rejecting a mismatched `iss` before any network call.
Verified end to end against live xaa.dev staging: EMA legs 1-3 (IdP SSO ->
ID-JAG mint -> resource token) now complete and the MCP session initializes.
web 3099, cli 212, tui 274 green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VjMemXZohSR2EndBQYif2b
|
||
|
cliffhall
|
f9f79fff54 |
Address @claude review: format core/, typed output-validation error, document progress heuristic
- Run Prettier on the 60 core/ files this PR touched — the deterministic import rewrite had collapsed multi-line imports and left throws unwrapped; core/ is in no format gate so it slipped past CI. Now Prettier-clean. - Default-path output-schema violation now throws ProtocolError(ProtocolErrorCode.InvalidParams, …) instead of a bare Error, matching the prior v1 contract (SDK threw a typed McpError with a code) so downstream `instanceof`/`.code` branching keeps working. - Document the "assumes serial construction" invariant on pollTaskToolCall's progress-subscription revival (the `_progressHandlers` new-key heuristic). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
cliffhall
|
19a6df7ad3 |
SDK v2 migration (behavior-neutral, versionNegotiation: 'legacy') (#1624)
Migrate all three clients + core + test-servers from
@modelcontextprotocol/sdk@1.29 to SDK v2 (2.0.0-beta.4), keeping behavior
identical against legacy (2025-11-25) servers. Foundation for the SDK V2 +
New Spec stack.
Packages
- Split @modelcontextprotocol/sdk -> @modelcontextprotocol/client (Client,
transports, OAuth, error taxonomy, protocol types) + core (Zod schemas);
stdio -> client/stdio, Ajv -> client/validators/ajv. Test-servers ->
@modelcontextprotocol/server (+ server-legacy/sse). Deterministic import
rewrite (schemas -> core, everything else -> client); the codemod misroutes
client code to /server so it was not used for path routing.
- new Client(..., { versionNegotiation: { mode: 'legacy' } }) — nothing
2026-era goes on the wire.
Runtime (core/mcp/inspectorClient.ts)
- Error taxonomy: McpError -> ProtocolError, ErrorCode -> ProtocolErrorCode;
RequestTimeout -> SdkError/SdkErrorCode.
- Handlers take method strings; task methods (tasks/*, notifications/tasks/*)
use the 3-arg custom form (excluded from v2's spec-method set).
- Legacy tasks kept working (per project requirement): requestor tasks re-
implemented over raw client.request + explicit deprecated schemas;
experimental.tasks.callToolStream replaced by a local pollTaskToolCall
async generator (with _progressHandlers revival so task-execution progress
still reaches the caller). Receiver (server-initiated) tasks: the v2 Client
validates a sampling/elicit handler result and rejects the legacy { task }
response with -32602 — installReceiverTaskResponseBypass swaps the wrapped
entry in the private _requestHandlers map for the task-augmented branch so
{ task } reaches the wire (fixes server-initiated sampling-via-task).
- Pagination: list verbs use raw client.request({method}, List*ResultSchema)
for single-page behavior (v2's client.listX() auto-aggregate all pages).
Web / auth / apps
- Rename local JsonSchemaType -> InspectorFormSchema + add toFormSchema()
structural narrowing at the SDK-schema -> form boundary.
- ext-apps still peers on SDK v1: one compat boundary with documented casts
(remove when ext-apps#702 lands a v2 peer).
- OAuth provider saveClientInformation reconciled with v2's
OAuthClientInformationContext; InvalidClientMetadataError re-homed as a
local class (server-legacy/auth is not browser-safe).
- vitest.shared.mts dedupes @modelcontextprotocol/client+core to one copy so
vi.mock intercepts the SUT's import.
Accepted v2 behavior changes (per #1624 / spec §9.2): unknown-tool call
rejects with -32602; cancellation is stream-abort (no guaranteed
notifications/cancelled frame); output-schema strictness restored via the
Inspector's own Ajv check on the default path.
npm run ci green: validate (web 3087 / cli 212 / tui 274 / launcher 5),
coverage (>=90 per-file, all clients), smoke (launcher/cli/web), storybook
(410).
Closes #1624
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Cliff Hall
|
a029e42a19 |
cli: Wave 3 — programmatic review path (rollup of #1573–#1575) (#1655)
* feat(cli): programmatic ergonomics — initialize, tool-args-json, connect-timeout, env-var fixes Adds the #1573 programmatic-ergonomics surface to the CLI, re-implemented informed by PR #1510 and adapted to the v2/main OAuth-wired cli.ts: - `--method initialize` — connect-only probe emitting `{serverInfo, protocolVersion, capabilities, instructions}`. - `--tool-args-json '{"k":"v"}'` — tool args passed verbatim as JSON (no key=value coercion); mutually exclusive with `--tool-arg`, with clear errors for combined use, malformed JSON, and non-object values. - `--connect-timeout <ms>` — default 15000 for ad-hoc `--server-url`/target runs, file-level timeout otherwise; `0` disables. Backed by the exported `withConnectTimeout()` helper. - MCP_CATALOG_PATH is ignored when an ad-hoc target is given, so a shell that exports it can still run one-off ad-hoc invocations. - MCP_STORAGE_DIR is honored by core `getStateFilePath` (`<dir>/oauth.json`), below the per-file MCP_INSPECTOR_OAUTH_STATE_PATH override. Tests: new programmatic-ergonomics.test.ts (CLI, in-process runCli + direct withConnectTimeout unit tests) and getStateFilePath resolution cases in the web storage integration suite. Per-file coverage gate stays ≥90. README: documents the new flags and env-var semantics. Closes #1573 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * feat(cli): --app-info probe + --format json envelope Adds the #1574 app-probing and machine-readable output surface, re-implemented informed by PR #1510 and adapted to the v2/main OAuth-wired cli.ts. - `--method tools/call --tool-name <t> --app-info` — prints one JSON line describing the tool's app posture (hasApp, resourceUri, csp, permissions, domain, …) WITHOUT invoking the tool; exit 0 when it has an app, 2 (no_app) when it doesn't. A missing tool exits 5 (tool_not_found) — distinct from the no-app exit so a typo isn't mistaken for "no app". - `--method tools/list --app-info` — one NDJSON line per tool over a single connection (`| jq -c 'select(.hasApp)'`). - `--format json` — a single stdout envelope (`{result}` plus `{appInfo}` as a sibling key for App tools), no banner, so any method pipes into jq. - A tools/call returning isError:true now exits 5 (tool_is_error) after printing its payload, so && chains stop on a failed call. Implementation: `collectAppInfo()` (core `extractAppInfo` + a tolerated resource read that reports `resourceError`), an NDJSON loop in tools/list, and a single `emitResult()` that owns format + exit-code handling. `runMethod` now returns a `MethodOutcome` discriminated union so the NDJSON path can signal "already emitted". Tests: app-info.test.ts (exit-0/2/5 + NDJSON + no-invoke), format-json.test.ts (envelope shapes + bad --format), emit-result.test.ts (direct unit coverage of the isError→exit-5 and resourceError branches via exported emitResult/ collectAppInfo). Updated the tools.test "nonexistent tool" case to the new exit-5 tool_not_found contract. Per-file coverage ≥90 on all four dimensions (cli.ts 100% stmts/funcs/lines). README documents the flags, NDJSON shape, and exit semantics. Closes #1574 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * feat(cli): stored-auth suite — --use-stored-auth, --wait-for-auth, --list-stored-auth, --print-handoff Adds the #1575 web→CLI OAuth handoff surface, re-implemented informed by PR #1510 and adapted to v2/main. - `--use-stored-auth` — read the shared oauth.json and inject `Authorization: Bearer <token>` for --server-url; exit 3 (no_stored_token), listing stored keys, when none matches. - `--wait-for-auth <sec>` — poll oauth.json (500ms) until a token for --server-url appears, then proceed; timeout → exit 3 (auth_wait_timeout). - `--list-stored-auth` — print `{oauthStatePath, storedServerUrls}` and exit (short-circuit, no connect). - `--print-handoff` — emit `{serverUrl, deepLink, portForwardCmd, oauthStatePath, apiToken, note}` for --server-url and exit. Adaptations vs. the reference: - State-path resolution reuses core `getStateFilePath()` (the MCP_INSPECTOR_OAUTH_STATE_PATH → <MCP_STORAGE_DIR>/oauth.json → default precedence added in #1573) instead of a CLI-local resolver. - The state file is read through the shared isomorphic `parseOAuthPersistBlob`, so both the plain `{servers,idpSessions}` layout the web backend writes and the legacy `{state,version}` envelope are accepted. - `parseArgs` now returns a ParseResult union so the short-circuit modes return without a server connection. Blind injection: the token is passed as-is; a stale one surfaces as HTTP 401 → exit 3. Documented in the README along with all four flags, the state-path precedence, and a follow-up note that --print-handoff's deepLink shape is interim pending #1576's canonical handoff format. Tests: stored-auth.test.ts drives injection, --header coexistence, missing --server-url, no-token exit-3, URL-normalisation matching, MCP_STORAGE_DIR resolution, listing, handoff (+note), wait-for-auth polling/timeout, and normalizeServerUrl units — all against a real in-process HTTP test server. Per-file coverage ≥90 on all four dimensions (cli.ts 100% stmts/funcs/lines). Closes #1575 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * docs(core): scope MCP_STORAGE_DIR comment in getStateFilePath to the OAuth backend Address review of PR #1651: the doc comment claimed MCP_STORAGE_DIR was honoured "across the app, so setting it once points every persist backend at the same directory", but the branch only relocates oauth.json. Narrow the comment to state it is OAuth-backend-only (client.json/mcp.json still resolve from getDefaultStorageDir → HOME) and record why the resolution lives here rather than in getDefaultStorageDir (the default-path constant is evaluated at module load, so a runtime-set MCP_STORAGE_DIR — as the CLI stored-auth / --wait-for-auth flows set it — must be read at call time). Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * review(cli): per-tool-tolerant app-info listing + drop dead helper (PR #1653) Address review of PR #1653: - collectAppInfo now never throws: a malformed `_meta.ui.resourceUri` (extractAppInfo throws) folds into `{hasApp:false, resourceError}` just like a resources/read failure. This makes `tools/list --app-info` per-tool tolerant — one bad tool no longer aborts the whole NDJSON listing with a generic exit-1. Guarding inside collectAppInfo (vs. a loop-local try/catch) keeps the branch unit-testable; added an emit-result.test.ts case for the malformed-URI path. - Documented that `tools/list --app-info` always emits NDJSON regardless of `--format` (the per-tool list shape is fixed; --format json only reshapes the single-result paths) — in both the code comment and the README. - Removed the now-unused `expectJsonError` helper (its only caller moved to the exit-5 tool_not_found contract). Coverage stays ≥90 on all four dimensions (cli.ts 100% stmts/funcs/lines). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw * review(cli): stored-auth follow-ups — TODO(#1576), wait-for-auth guard test, docs (PR #1655) Address review of PR #1655: - Added an explicit TODO(#1576) on buildHandoff's deepLink: `transport:"http"` is hardcoded even for SSE, and `autoConnect=<token>` is not one of the three token sources the web app reads (per CLAUDE.md) — both to be reconciled with #1576's canonical handoff format. - Added a test for `--wait-for-auth` without `--server-url`, covering the previously-untested `--wait-for-auth` arm of the shared missing-server-url guard's message ternary. - Documented that `--list-stored-auth` / `--print-handoff` are mutually exclusive short-circuit modes (list takes precedence) and that a stored refresh_token is not yet used (noted as a follow-up in code + README). No behavior change; coverage stays ≥90 on all four dimensions (cli.ts 100% stmts/funcs/lines). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
cliffhall
|
32a3610737 |
Merge remote-tracking branch 'origin/v2/main' into 1579-wave-1
# Conflicts: # clients/cli/package-lock.json # clients/cli/package.json # clients/web/src/App.tsx # clients/web/src/lib/environmentFactory.test.ts # clients/web/src/lib/environmentFactory.ts # clients/web/src/lib/remoteOAuthStorage.test.ts # clients/web/src/lib/remoteOAuthStorage.ts # clients/web/src/test/core/auth/connection-state.test.ts # clients/web/src/test/core/auth/providers.test.ts # clients/web/src/test/core/auth/storage-browser.test.ts # clients/web/src/test/core/auth/storage-remote.test.ts # clients/web/src/test/core/mcp/oauthManager.test.ts # clients/web/src/test/integration/auth/node/storage.test.ts # clients/web/src/test/integration/storage/adapters.test.ts # clients/web/src/utils/clearServerOAuthState.test.ts # clients/web/src/utils/clearServerOAuthState.ts # core/auth/connection-state.ts # core/auth/oauth-storage.ts # core/auth/providers.ts # core/auth/storage.ts # core/auth/store.ts # core/storage/adapters/remote-storage.ts |
||
|
cliffhall
|
1e6a2825d4 |
fix(auth): keep oauth-persist browser-safe (move Node file backend out)
The refactor routed the browser's RemoteOAuthStorage through `oauth-persist.ts`, which value-imported the Node-only `store-io.js` (`atomically` -> `stubborn-fs` -> `node:process.getuid`, plus `node:fs/promises`). That dragged Node built-ins into the browser bundle and the web app crashed to a blank page at runtime — a regression from v2/main, where browser OAuth storage used a Zustand JSON adapter with no Node imports. Unit/integration tests run in node/happy-dom and `smoke:web` only checks the served HTML, so none of them execute the app in a real browser and the break slipped through. Fix, following the existing `store-id.ts` precedent: - Split the pure JSON (de)serializers into Node-free `core/storage/store-serialize.ts`; `store-io.ts` re-exports them so existing importers are unaffected. - Move the Node-only `createFileOAuthPersistBackend` into `core/auth/node/oauth-persist-file.ts`; update `storage-node.ts` and the storage adapters test to import from there. - `oauth-persist.ts` now imports only the Node-free serializers, so the browser bundle no longer pulls in `node:fs`/`atomically`. Tests: - oauth-persist-browser-safe.test.ts: source-level guard that `oauth-persist.ts` never imports store-io/node:/atomically. - Direct unit tests for the isomorphic remote + session backends (restoring oauth-persist.ts coverage after the file backend moved out). - store.test.ts: OAuthMemoryStore unit coverage incl. replace() fallbacks. - storage.test.ts: clearAllOAuthClientState null-snapshot (no file) path. Verified end-to-end in a real browser (Playwright): DCR OAuth flow completes, tokens persist via the remote store, and after a full page reload the server reconnects with no re-authentication. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
cliffhall
|
54d5b18b1e |
fix(auth): drop redundant empty-object guard in remote OAuth read()
parseOAuthPersistBlob already returns null for {} (the server's
missing-file response) and for a literal null body, so the
Object.keys(store).length === 0 guard was redundant — and would throw a
TypeError on a null body before parseOAuthPersistBlob could handle it.
Pass the parsed res.json() straight through.
Addresses Claude review item #1 on #1612.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
cliffhall
|
edc7dbb1c1 |
Merge remote-tracking branch 'origin/v2/main' into v2/1549-oauth-storage-no-zustand
# Conflicts: # core/auth/store.ts |
||
|
cliffhall
|
ba48e06ffd |
fix(auth): guard OAuth-storage async paths against unhandled rejections
Review follow-ups on the RemoteOAuthStorage web migration: - useEmaIdpLoginState.logout(): add a .catch to the now-async clearEmaIdpSession call so a storage failure no longer surfaces as an unhandled rejection; leave loginState unchanged on failure (the session was not actually cleared). Add a test asserting no unhandled rejection. - createOAuthStore: attach a no-op catch to the eager persistLoadPromise so a hydration read that fails before any load() awaits it does not become an unhandled rejection; real awaiters still observe the rejection via the shared reference. - storage-remote.test: replace `as unknown as typeof fetch` double-casts with the typed vi.fn<typeof fetch>() pattern the same PR established. - remoteOAuthStorage: correct the doc comment to describe the cache-of-one accurately instead of implying a keyed map. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Bob Dickinson
|
af174666d6 |
fix(auth): address Claude review on OAuth persist serialization
Serialize persist() in OAuthStorageBase so overlapping remote POSTs cannot reorder and clobber state. Regenerate launcher package-lock.json to drop stale zustand entries. Let parseOAuthPersistBlob accept parsed objects from res.json() without a redundant JSON round-trip. |
||
|
Bob Dickinson
|
3c65bdb4c6 | Removed Zustand storage, cleanup up auth storage IO interfaces, implementations, and usage. | ||
|
Bob Dickinson
|
512f45d357 | fix(auth): reject load() when persist read fails | ||
|
cliffhall
|
cfbdfdf2bd |
Merge remote-tracking branch 'origin/v2/main' into 1579-wave-1
# Conflicts: # AGENTS.md # clients/cli/README.md # clients/web/src/App.test.tsx # clients/web/src/App.tsx # clients/web/src/test/core/auth/utils.test.ts # core/mcp/node/transport.ts # test-servers/src/composable-test-server.ts |
||
|
Bob Dickinson
|
a24091484c | fix(auth): await OAuth storage load() to fix remote OAuth races | ||
|
cliffhall
|
f4b6095719 |
test: close ≥90% per-file coverage gaps for mid-session auth
The mid-session auth work passed `validate`/CI but failed the local-only
`npm run test:coverage` per-file gate (≥90% lines/statements/functions/
branches) on 12 files. Add targeted unit/integration tests so every changed
file clears the gate; annotate provably-dead defensive branches with justified
`/* v8 ignore */` comments rather than relaxing the gate.
Files brought to ≥90 on all four dimensions:
- core/auth: mcpAuth, oauthUx, utils, challenge, node/runner-interactive-oauth
- core/mcp: oauthManager, inspectorClient, node/authChallengeFetch,
remote/remoteClientTransport, remote/node/{remote-session,server}
- clients/web/src/utils: oauthResume, browserTabVisibility
`npm run test:coverage` now passes (3575 tests, 0 threshold failures);
`npm run validate` green across all four clients.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DZyeWManYTvcXxjcrhxSQc
|
||
|
Bob Dickinson
|
a066873e37 | Fixes for remaining issues | ||
|
Bob Dickinson
|
52023ff1c3 |
fix(auth): clear OAuth callback timer and reorder EMA scope persist
Clear the loopback callback timeout in runner-interactive-oauth finally so already_authorized and error paths do not hang the CLI/TUI. Check EMA step-up satisfaction against minted token scope before persisting the requested union, matching the standard OAuth path. Co-authored-by: Cursor <cursoragent@cursor.com> |
||
|
Bob Dickinson
|
562bdd00f1 | All of the mid-session and step-up auth work | ||
|
cliffhall
|
588fc3f8ea |
docs(auth): note getClientRegistrationKind's hydration invariant like getScope
Round-3 @claude review of #1592 (optional nit): getClientRegistrationKind is the other synchronous getter that, like getScope, is safe only because its sole caller (buildOAuthConnectionState) awaits getClientInformation/ getServerMetadata first, flushing hydration before this runs on the post-redirect callback path. Add a docstring mirroring getScope's so a future refactor doesn't read it without a preceding awaited storage read. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX |
||
|
cliffhall
|
4e149719bd |
fix(auth): route clearClientInformation through clearAfterHydration too
Round-2 @claude review of #1592 caught a seventh member of the clear family that the first pass missed: clearClientInformation still wrote to the store synchronously, carrying the same pre-hydration clobber risk the clearAfterHydration helper was written for (a clear before hydration lands persists a near-empty blob, overwriting the whole on-disk oauth.json). Wrap its mutation the same way and add a clear-before-hydration test proving a sibling token (and thus every other server's blob) survives. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX |
||
|
cliffhall
|
8c10b8eb8e |
fix(auth): make clear* hydration-safe, migrate raw-key blobs, doc keepalive/memo caveats
Address round-1 @claude review of #1592: - OAuthStorageBase.clear* no longer mutate the pre-hydration (empty) store — a clear issued before hydration is deferred until it lands, so it merges onto the real persisted state instead of (a) being resurrected by the late rehydrate merge or (b) persisting a near-empty blob that clobbers every other server's on-disk credential. Already-hydrated clears stay synchronous. - store.setServerState migrates a pre-normalization raw-key blob onto the canonical key on first partial write (was: shadowed it with a fresh canonical entry, orphaning the raw blob's other fields). clearServerState now also drops the raw-key orphan. - Document the keepalive 64KB combined-body ceiling on the remote-storage write, the getRemoteOAuthStorage memo key intentionally omitting fetchFn, and the real ordering invariant behind the sync getScope. - Tests for the clear-before-hydration and raw-key-migration paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX |
||
|
cliffhall
|
c299b6e9d8 |
feat(web): migrate OAuth store to shared /store API (RemoteOAuthStorage parity) (closes #1548)
Migrate the web client's OAuth storage from BrowserOAuthStorage (sessionStorage) to RemoteOAuthStorage, which POSTs through the backend's /api/storage/oauth route to ~/.mcp-inspector/storage/oauth.json (mode 0600). A credential obtained in the browser is now the same blob the TUI/CLI read on the same host, giving web ⇄ TUI ⇄ CLI parity (Wave 1 of #1579). Core (shared with TUI/CLI): - OAuthStorageBase now drives a single async hydration (`ready()`, `getHydrationError()`); every post-redirect read (getClientInformation, getTokens, getCodeVerifier, getServerMetadata, getIdpSession) and every save awaits it so a late hydration merge cannot clobber a fresh write. getCodeVerifier/getServerMetadata become async; the store is created with skipHydration:true so there is no auto-hydration to race the explicit one. - store.ts adds normalizeServerUrl so a token saved under https://Example.com/mcp is found when the CLI asks for https://example.com/mcp/; getServerState falls back to the raw key. - remote-storage adapter POSTs with keepalive so the write survives the OAuth authorize redirect, surfaces swallowed persist failures, and gives richer read/write error messages. - generateOAuthState throws instead of silently degrading to Math.random. - NodeOAuthStorage honors MCP_INSPECTOR_OAUTH_STATE_PATH for isolated fixtures. Provider codeVerifier()/getServerMetadata() + the EMA idpOidc and connection-state readers await the now-async storage. Web: - environmentFactory + a new shared lib/remoteOAuthStorage accessor (memoized per {baseUrl, authToken}) wire RemoteOAuthStorage into the connection path, the EMA IdP hook, and the per-server "clear OAuth" action so all three share one in-memory view of oauth.json. Tests: async-hydration + normalizeServerUrl suite, keepalive/error-path coverage, and the sync→async migration across existing storage/provider/EMA tests. Web validate + coverage gate + integration suite green; CLI/TUI/ launcher validate green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S3fTN8H3R8YV4yUGvZjYnX |
||
|
Bob Dickinson
|
13dafe457b |
Guided-auth removal, OAuth smoke testing, and TUI/CLI support (#1531)
* refactor(web): update OAuth E2E tests to use new authentication method - Replaced `runGuidedAuth` with `authenticate` in the InspectorClient OAuth end-to-end tests to support quick authentication with CIMD pre-registration. - Updated comments to reflect the change in authentication method and its support for HTTP test metadata URLs. - Ensured that the tests continue to validate the expected authorization URL after the change. Additionally, imported `ensureCimdClientRegistration` in the OAuthManager to streamline client registration during the authentication process. * Guided-auth removal, OAuth smoke testing, and TUI/CLI support * Added CLI OAuth runner flags tests * Added clarifying comment on port 0 binding * README notes on TUI oauth callback port, prettier fixes for CLI tests * CIMD validation uses inline like EMA IdP * Fixed TUI test failures from merge * test: bring 5 OAuth files over the 90 per-file coverage gate The guided-auth-removal work added/touched several files that fell below the repo's >=90 per-file coverage gate (npm run coverage). CI doesn't run the gate (it's local-only), so validate + test:integration passed while these gaps slipped through. Bring them all back over the line: - core/auth/node/runner-oauth-callback.ts: test bad-URL/non-http-scheme throws, no-port->80 default, and IPv6 hostname bracketing. Annotate the provably-dead guards (empty host, out-of-range port, String(err) fallback, pathname "/" fallback) -- all rejected by new URL() upstream -- with justified v8 ignore comments instead of lowering the gate. - core/auth/cimd.ts: test the AS-doesn't-support-CIMD early return. - core/react/useClientSettingsDraft.ts: test the functional-updater path, the stale-onChange-after-close prev===null guard, and the timer-fires-after-close value===null guard. - ClientSettingsForm.tsx: test the CIMD checkbox toggle and CIMD URL clear. - clients/tui/src/utils/oauthDisplay.ts: test static/dcr registration kinds, the expired IdP session, and the configured-scope/empty-scope branches of formatScopes. npm run coverage now passes (exit 0) across web, cli, tui, launcher. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: cliffhall <cliff@futurescale.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
c96db0402e |
test: raise per-file coverage gate to 90 across all clients (closes #1271) (#1532)
* test(web): audit branch-coverage outliers, raise per-file gate to 70 (#1271) The web per-file coverage gate had drifted red: the recently-landed EMA auth feature (core/auth/ema/*) and ClientSettings components shipped undertested, and the prior 50% branch floor let business-logic branches go untested. CI does not run the coverage gate, so this accumulated. Add tests to bring every failing file above the gate and push branch coverage well past 70 across the board, then raise the per-file branches threshold 50 -> 70 in clients/web/vite.config.ts. - EMA core (emaFlow, resourceContext, jwt, wire): 50-88% -> 86-100% branch - EMA OIDC/transport (idpOidc, tokenEndpoint, transportProvider): -> 97-100% - oauthManager / discovery: 73-83% -> 88-92% - ClientSettingsModal/Form, ConnectionInfoContent: 0-88% -> 96-100% - useClientSettingsDraft / useEmaIdpLoginState hooks: 20% -> 100% - TaskStatusBadge / SortableServerCard outliers: 50% -> 100% Coverable branches reached by mocking useComputedColorScheme (dark theme), dnd-kit useSortable (isDragging), and fetch/OIDC paths. Remaining sub-100 branches are genuinely-unreachable defensive guards or happy-dom-inherent (portal / useMediaQuery / typeof window) and documented in test comments. No source files changed. Global web branch coverage 88.73% -> 90.63%. AGENTS.md updated; cli/tui/launcher stay at 50 (out of scope). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: raise per-file coverage gate to 90/90/90/90 across all clients Follows the #1271 web branch-coverage audit (50 -> 70) by raising the whole per-file gate to >=90 on all four dimensions — lines, statements, functions, branches — for web, cli, tui, and launcher (core/ is measured under the web suite). A codebase-wide audit added real tests for every outlier and brought global coverage up across the board. Config: all four vitest/vite coverage configs set to 90/90/90/90. Two pure-type modules (core/mcp/import/types.ts, clients/web/server/ types.ts) added to the web coverage exclude list, matching the existing type-only exclusions. Tests: ~50 test files extended/created across core (auth, ema, mcp, remote, state, storage, react), web components/screens/server, and the cli/tui clients. Highlights: full EMA OAuth flow + transport coverage, inspectorClient/remote server backfill, every Mantine control's handler branches, and the cli unsupported-method / argv-fallback paths. Genuinely-unreachable branches are annotated at the source with justified `/* v8 ignore -- <reason> */` comments rather than lowering the gate: happy-dom-inherent paths (Mantine portal, useMediaQuery, typeof window SSR guards), React StrictMode effect replay, and provably-dead defensive guards (?? fallbacks the types make impossible, Select.onChange values outside the allowed list). No runtime logic changed; the only structural edits are two behavior-preserving refactors (ResourceTemplatePanel hoists the field value to a local so the ignore applies in JSX; ToolsScreen swaps a ternary for an if) and prettier line-wrapping. Also fixed a flaky real-timer debounce assertion in PromptArgumentsForm (asserted an exact sibling-typing interleaving that raced the 300ms debounce; now asserts the load-bearing fact — a non-empty fire-time sibling capture). AGENTS.md updated to describe the uniform 90 gate and the v8-ignore policy. All four client coverage gates, root validate, storybook (370), and smoke pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(web): make connect-crash SSE reader stop on the terminal event `readSseEvents` looped until the server closed the stream (`done`). The server closes the events stream immediately after writing the terminal `transport_error`, but under the heavy parallel scheduler pressure of a full `coverage` run (v8 instrumentation across all workers) that close can lag far enough behind the event to trip the 30s test timeout — a hang, not a wrong result. This made the test intermittently fail under `npm run coverage` (it passed in isolation and under CI's lighter `test:integration`). Stop reading as soon as the terminal `transport_error` event arrives (cancelling the reader) instead of waiting only for `done`. That event is always the last one the session queue emits, so nothing is lost, and the test no longer depends on stream-close timing. The `done` break is kept for any stream that closes first. Verified green across two consecutive full `npm run test:coverage` runs (3094 tests) where it previously hung, plus isolation (~0.5s). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Bob Dickinson
|
5de4e911e3 |
feat(web,core): enterprise-managed authorization (EMA) (#1509) (#1512)
Adds enterprise-managed authorization (EMA): browser-side IdP login + RFC 8693 token-exchange and JWT-bearer resource-token mint, per-server EMA opt-in in Server Settings, IdP config in Client Settings, and IdP clientSecret keychain storage. Includes the v2/main merge and review follow-ups (400 on malformed client config; normalizeIdpIssuer import consolidation). |
||
|
Cliff Hall
|
98192fd86e |
fix(auth): persist OAuth pre-redirect Network log across the redirect (#1384) (#1385)
* feat(auth): inject MCP_INSPECTOR_API_TOKEN into served index.html (#1378) Reloading the web client at the bare URL (no `?MCP_INSPECTOR_API_TOKEN=…` query string) with empty sessionStorage made every `/api/*` request 401 — the browser had no way to recover the backend's auth token. Embed the token into `index.html` on every page load so the browser no longer depends on the query string surviving navigation: - New shared helper `clients/web/server/inject-auth-token.ts` embeds `<script>window.__INSPECTOR_API_TOKEN__ = "…"</script>` (escaped against `</script>` injection; no-op when auth is dangerously omitted). - Dev: the Vite plugin injects via `transformIndexHtml`. - Prod: the Hono server injects on the `/` route. - `App.tsx` `getAuthToken()` now reads the injected global first, then the query string, then sessionStorage (both fallbacks preserved). - Shared global name lives in `INSPECTOR_API_TOKEN_GLOBAL` (`core/mcp/remote/constants.ts`). Tests: helper unit coverage + an integration test exercising the real prod server's `/` → `/api/*` flow (injected token authenticates; missing token 401s). AGENTS.md documents the token-recovery order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(auth): wire OAuth authorization-code flow into App.tsx (#1379) OAuth-protected MCP servers could not be connected to from the v2 web client: the core OAuth pipeline exists, but App.tsx never invoked it, so a connect attempt 401'd and surfaced "Remote send failed (401): … Missing Authorization header" as a toast. Wire the two missing entry points (all core primitives already in place): - Auto-trigger on 401: in onToggleConnection's catch, detect an upstream 401 (isUnauthorizedError) and call client.authenticate(), which runs discovery + DCR (backend-proxied) and redirects the page to the auth server via BrowserNavigation. The initiating server id is persisted to sessionStorage first, since the OAuth `state` carries only mode+authId and the full-page redirect wipes React state. - /oauth/callback handler: a mount effect that, once `servers` hydrate, parses the callback params, recovers the pending server, rebuilds its InspectorClient, runs completeOAuthFlow(code) (PKCE verifier + DCR client info survive in BrowserOAuthStorage), replaceState("/") so a reload can't replay the single-use code, then connect(). An `error=` callback toasts instead of retrying. connect() already attaches the OAuth provider to the transport (inspectorClient.ts), so once tokens land in BrowserOAuthStorage the outbound request carries the bearer token. Extracted the pure pieces (constants + isUnauthorizedError) to src/utils/oauthFlow.ts with unit tests. Verified end-to-end in a real browser against the MCP SDK demo OAuth server: Connect -> redirect -> auto-approve -> callback -> Connected, with the access token shown in the Connection Info modal (#1377). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): persist OAuth pre-redirect Network log across the redirect (#1384) After #1379, the Network tab showed only the post-redirect auth HTTP (discovery re-run + POST /token); the pre-redirect discovery and the DCR POST /register that run during authenticate() were lost when the page navigated to the auth server. Root causes: 1. Ordering — BrowserNavigation set `window.location.href` before the client's `saveSession` event fired (OAuthManager calls onBeforeOAuthRedirect *after* auth() already navigated), so the save raced the unload and was dropped. Fix: BrowserNavigation now runs a synchronous `beforeNavigate` hook immediately before assigning location.href; App wires it through createWebEnvironment to flush the active fetch log to RemoteInspectorClient Storage (keyed by the authId parsed from the auth URL) via a keepalive POST that outlives the unload. 2. Illegal invocation — RemoteInspectorClientStorage defaulted to `this.fetchFn = globalThis.fetch` and called `this.fetchFn(...)`, which re-binds `this` and makes native fetch throw "Illegal invocation" (swallowed by the catch). This silently broke *all* session save/load. Fix: default to a wrapper that preserves the global receiver. 3. Restore race — hydrateFetchRequests replaced the list, so a load that resolved after the resuming connect appended live entries would clobber them. Fix: merge restored (older) entries ahead of live ones, dedupe by id. saveSession also now uses keepalive: true. Verified end-to-end against the MCP SDK demo OAuth server: the connected page's Network tab shows the full handshake — pre-redirect discovery + DCR /register plus post-redirect discovery + /token as `auth`, alongside `transport`. Added unit tests for the beforeNavigate ordering and the hydrate merge/dedupe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): inject token into prod SPA fallback + prime sessionStorage + no-store (#1378) Addresses code-review feedback on the token-injection PR: - Prod `server.ts` SPA deep-link fallback (e.g. `/oauth/callback`) previously served the raw index.html off disk via serveStatic, bypassing injection — so a bookmark/reload at a non-`/` route with empty sessionStorage would 401. Route the SPA fallback through the same `serveIndexHtml` (inject) helper; real static assets (paths with a dot) still serve verbatim. Dev already injected on every HTML serve via Vite `transformIndexHtml`. - `getAuthToken()` now persists the injected `window.__INSPECTOR_API_TOKEN__` to sessionStorage (not just the URL-param branch), priming the backstop for any later navigation that loses the global. - Injected HTML responses now send `Cache-Control: no-store`, so a page carrying a token isn't cached and served stale after a restart regenerates the token. Integration tests added: SPA fallback (`/oauth/callback`) carries the token, `Cache-Control: no-store` on injected HTML, real assets served verbatim, and unknown `/api` routes 404 rather than falling through to the HTML shell. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(auth): address #1383 review — split OAuth/connect toasts, tighten 401 match (#1379) Code-review feedback on the OAuth-wiring PR: - Callback effect: split completeOAuthFlow vs connect() into separate try/catch blocks. A token-exchange failure now reads "OAuth token exchange failed … Please try connecting again." (the single-use code is spent and the URL was cleared, so a reload can't retry); a post-OAuth connect failure reads "Failed to connect" since OAuth actually succeeded and re-clicking Connect reuses the persisted tokens. - isUnauthorizedError: anchor the message fallback on the transport's `failed …(401)` wording instead of a bare `(401)`, so an unrelated `(401)` spliced into an error message can't trip the OAuth flow. Added a test. - Documented that clearing the pending id + URL before the server lookup is intentional (deleted/renamed server mid-flow → require a fresh Connect). Also merges the squash-merged #1382 base from v2/main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(auth): address #1385 review — clarify double-save + keepalive cap + test fetch default (#1384) Code-review feedback on the OAuth Network-log persistence PR: - Documented the double-save: `FetchRequestLogState`'s `saveSession` listener is the backstop; `BrowserNavigation`'s `beforeNavigate` hook is the primary flush for the redirect case. Notes the listener may lose the navigation race and is harmless when it duplicates (last-writer-wins, identical payload). - Reworded the keepalive comment in `RemoteInspectorClientStorage.saveSession`: the 64KB cap is general (the method is also reachable from the listener with the full session log), so a long session could exceed it and drop silently — acceptable since the persisted log is best-effort, not load-bearing. - Added a regression test that constructs `RemoteInspectorClientStorage` without a `fetchFn`, stubs `globalThis.fetch`, and asserts the default wrapper calls it (locks in the "Illegal invocation" fix, which callers otherwise swallow). Optional items (logger.warn on swallowed save errors; setupClientForServer dep churn) acknowledged on the PR, not changed. Also merges the squash-merged #1383 base from v2/main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
037225a9fb |
feat(servers): move oauthClientSecret and stdio env values to OS keychain (#1356) (#1360)
* feat(servers): move oauthClientSecret and stdio env values to OS keychain (#1356) mcp.json is designed to be tool-shareable (symlinked from Claude Desktop's config, pasted into bug reports, synced via dotfiles), so storing OAuth client secrets and stdio env values in plaintext there meant any of those flows could leak them. Lift both into the OS keychain via @napi-rs/keyring (active replacement for the archived keytar). The wire shape is unchanged: the GET /api/servers handler rehydrates from the keychain so browser code sees the same JSON; the on-disk file no longer contains the secret material. Includes an idempotent migration that lifts plaintext from older mcp.json files (or hand-edited ones) into the keychain on first read. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): tolerate keychain unavailability on non-set ops (#1356) CI on Linux failed because `@napi-rs/keyring` hard-errors without libsecret, and the route handlers were touching the keychain even for no-secret flows (defensive `deleteAllForServer` on POST/DELETE, sweep on PUT). The user's intent for the Linux fallback was "hard fail when a secret is actually involved" — not on every keychain touch. Make `KeyringSecretStore.get / delete / deleteAllForServer` silently degrade when the keychain is unavailable (return null / no-op); `set` remains the one operation that hard-fails with `KeychainUnavailableError`. The migration path catches the error and preserves the disk plaintext rather than losing the secret. Also inject `InMemorySecretStore` in `useServers.test.tsx` and `servers-events.test.ts` so tests never touch a developer's real keychain even when libsecret is present. Adds `KeyringSecretStore` tests (vi.mock'd native bindings) and an integration suite that simulates Linux-without-libsecret to verify the tolerance contract end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): close GET migration race; reverse keychain/disk order (#1356) Addresses code review: 1. **Concurrency**: GET /api/servers was reading + migrating outside the write lock, then writing inside it — a concurrent POST/PUT/DELETE between the read and the lock acquisition could be clobbered by the migration write. Also affected the first-launch seed write. Fix: take a fast unlocked peek; if the file is absent (seed branch) or carries plaintext to migrate (migration branch), re-read inside the write lock and decide there. Added `hasPlaintextSecrets()` as a pure predicate so the common no-migration path stays lock-free. 2. **Write ordering**: POST and PUT wrote disk first, then keychain. A keychain `set` failure mid-write left a half-configured entry on disk and trapped retries at 409. Reversed to keychain-first so a 503 leaves both stores in their pre-write state. PUT in-place now sets new fields, writes disk, then deletes obsolete fields (a failed disk write leaves orphan keychain entries — recoverable — rather than premature deletions of fields the user still expects to see). 3. **Tolerance simplification**: dropped the no-entry regex in `KeyringSecretStore.delete` — both "no entry" and "keychain unavailable" collapse to the same desired outcome (the entry isn't there anymore), so a uniform swallow is clearer. 4. **Parallelism**: `readKeychainEntriesFor` now `Promise.all`s the per-field `get` calls. macOS Keychain round-trips are 10-50ms; serializing 5+ env vars per server × N servers is a meaningful wall-clock cost on GET. 5. **Spec doc**: reflects the tolerance contract (only `set` hard-fails) and the keychain-before-disk write ordering rationale. New regression test in servers-route.test.ts covers the retry-after-503 contract directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(servers): parallel keychain writes + migration partial-state comment (#1356) Two nice-to-have observations from the re-review: - `writeKeychainEntriesFor` now `Promise.all`s its per-field sets, matching the read side. No ordering requirement among distinct (id, field) keys; on unavailability every set rejects and Promise.all surfaces the first rejection (still 503 from the route handler). - `migratePlaintextSecrets` catch block now spells out the partial-migration semantics: we return the original config so the disk plaintext stays intact, and the next successful GET retries with the keychain-wins branch absorbing already-set entries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(servers): parallel keychain deletes for symmetry (#1356) Picks up the symmetry observation from the third review: `deleteKeychainFields` now uses `Promise.all` like its read and write siblings. Distinct (id, field) deletes have no ordering requirement, and `secretStore.delete` is already a silent no-op on unavailability so Promise.all has no failure-mode surprise. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
82269e78cb |
refactor(core): apply v1.5-port cleanups deferred from #1306 (#1316)
* refactor(core): apply v1.5-port cleanups deferred from #1306 Addresses issue #1309 — five small, mechanical cleanups deferred from the v1.5 runtime port: 1. Collapse BrowserOAuthStorage / NodeOAuthStorage / RemoteOAuthStorage onto a shared OAuthStorageBase. Three ~130-line duplicates become thin adapter wrappers; behavior + public API unchanged. 2. Re-enable erasableSyntaxOnly in tsconfig.app.json by rewriting the seven (plus two the issue missed in sampling/elicitation create- message) parameter-property constructors as explicit field + assign. 3. Route the lone console.error in inspectorClient.ts through this.logger. 4. Replace `${Date.now()}-${Math.random()}` with crypto.randomUUID() at the five local-only ID sites (message tracking + sampling/elicit). 5. Pin the remote-server hono app generic at new Hono<Env>() and drop the streamSSE cast. Root cause of the cast was an unresolved hono subpath: the tsconfig "hono/*" wildcard didn't honor the exports map, so hono/streaming leaked to the root-hoisted hono@4.12.12 while hono itself resolved to clients/web/hono@4.12.18. Replaced the wildcard with an explicit hono/streaming entry in both tsconfig.app and tsconfig.test, removing the package-identity mismatch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(core): address #1316 PR review feedback - Drop the stranded no-op getServerState() read at the top of clearClientInformation in OAuthStorageBase (carried over verbatim from the three originals). - Drop the crypto.randomUUID() guard at the task-ID site so all six in-scope sites are consistent; the fallback branch was dead code on every supported runtime. - Remove the residual `c as Context` cast at the streamSSE call site in core/mcp/remote/node/server.ts — once both hono imports resolve to the same package the route-typed Context flows through to streamSSE with no cast at all. - Move the new `options` / `config` field declarations in RemoteClientTransport up with the rest of the field block, matching the layout used by the other five rewritten classes in this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Cliff Hall
|
0ead1deea6 |
feat(core): port v1.5 InspectorClient + transports + auth subsystem (#1302) (#1306)
* wip(core): bulk-copy v1.5 InspectorClient runtime files (#1302) Verbatim ports from origin/v1.5/main into v2/main core/: - logging/ (2 files) - storage/ + storage/adapters/ (4 files) - auth/ + auth/browser/ + auth/node/ + auth/remote/ (16 files) - mcp/config.ts + mcp/node/ (4 files, stdio transport) - mcp/remote/ + mcp/remote/node/ (12 files, HTTP/SSE transport) - mcp/inspectorClient.ts (~2169 LOC), messageTrackingTransport, fetchTracking, oauthManager, index, state/index Total: ~7,800 LOC of source files, copied unmodified via `git show origin/v1.5/main:<path> > <dest>`. Also: - Add deps: pino, hono, @hono/node-server, atomically, zustand, @types/node - tsconfig.app.json: extend paths to resolve new node-only deps from core/, add "node" to types, disable erasableSyntaxOnly (v1.5 code uses parameter properties — TODO to re-enable in follow-up) Compiles down to 47 type errors, all in three categories: - 24 TS2305: v2 dropped v1.5 type exports (InspectorClientOptions, CreateTransport, InspectorClientEnvironment, AppRendererClient, SamplingCreateMessage class, ElicitationCreateMessage class, jsonUtils helpers, OAuth events). Per user direction, will re-add these to v2's diverged files (types.ts, samplingCreateMessage.ts, etc.) so the ported code stays verbatim. - 15 TS7006: implicit-any params in v1.5 code (v1.5 tsconfig was looser than v2's noImplicitAny). Need explicit annotations. - 8 other TS errors: ../package.json import, Transport|null narrowing, hono Context generic mismatch, one unused parameter. Tests not yet ported (19 files, ~11,170 LOC). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * wip(core): integrate ported v1.5 InspectorClient + port tests (#1302) Source integration (typecheck + build now pass): - Re-add v1.5 types/classes dropped during v2 evolution: - types.ts: CreateTransport*, AppRendererClient, InspectorClient{Environment,Options} - samplingCreateMessage.ts / elicitationCreateMessage.ts: classes + keep v2's InspectorPending{Sampling,Elicitation} interface - inspectorClientEventTarget.ts: oauth* event keys - json/jsonUtils.ts: convertParameterValue / convertToolParameters / convertPromptArguments + ParameterSchema - inspectorClient.ts: replace `../package.json` import (v2 has no core/package.json) with a hardcoded identity constant - remote/node/server.ts: cast hono Context for streamSSE (hono's generic typing tightened since v1.5) - oauth-callback-server.ts: drop unused hostname field - tsconfig: enable @types/node, add paths for new bare-module deps (pino, zustand, hono, atomically) in app + test configs - vite.config.ts: mirror those module aliases for the unit project (which runs from repoRoot) Tests ported (19 files, ~11K LOC) under clients/web/src/test/core/: - All core/__tests__/auth/* (7 files) - inspectorClient.test.ts + 4 OAuth variants - mcp/oauthManager.test.ts, jsonUtils.test.ts - transport, remote-transport, remote-server-config, storage-adapters - helpers/oauth-client-fixtures.ts Bulk-rewrote `from "../mcp/…"` → `from "@inspector/core/mcp/…"` etc. Current state: - typecheck: 0 errors - build: passes (vite v8, 788 modules) - unit tests: 1014 / 1034 pass (937 pre-existing + 77 new core tests) - 20 failures in 13 files; failure modes: - 13 file-load failures needing the v1.5 `test-servers/` package (@modelcontextprotocol/inspector-test-server) which isn't in v2 yet or real fs/network not available in happy-dom - 7 assertion failures in discovery.test.ts + state-machine.test.ts (SDK 1.29 vs 1.25 API drift — mocks don't match new shape) Coverage gate not yet checked against new files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * wip(core): port test-servers, more types, partial test integration (#1302) - Port v1.5 test-servers/ package (12 files, ~5K LOC) under repo-root/test-servers/, wire @modelcontextprotocol/inspector-test-server alias in both vite.config.ts (regex form) and tsconfig.test.json. - Add deps: express ^5, yaml ^2, @types/express ^5. - Re-export InspectorClientOptions/Environment/Transport* from inspectorClient.ts so v1.5 tests that import them from that module path keep resolving. - Switch InspectorClientEventMap to use SamplingCreateMessage / ElicitationCreateMessage classes (matches v1.5 event payload contracts) while keeping the InspectorPending{Sampling,Elicitation} interfaces in the *.ts modules for v2 consumers that still use them. - Fix verbatim-module-syntax issue in test-servers and drop unused stdio config field. - Adjust inspectorClient.test.ts task types to TaskWithOptionalCreatedAt (SDK 1.29 tightened Task shape vs v1.5's 1.25). - Drop spurious maxMessages/maxFetchRequests/maxStderrLogEvents from remote-transport.test.ts (never on InspectorClientOptions). State: - typecheck: 0 errors - build: passes - unit tests: 1099 / 1270 pass - 171 failures in 10 test files, almost all integration tests that need real HTTP/fs/network (happy-dom can't host them). Next: add a node-env vitest project to run these properly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(core): port v1.5 InspectorClient + transports + auth subsystem (#1302) Bulk-port of the v1.5 core runtime (~7,800 LOC source + ~16K LOC tests + ~5K LOC test-servers/) into v2. Files copied verbatim from origin/v1.5/main via `git show >`; only minimal adapters added on the v2 side. Subsystems landed ----------------- - core/logging/, core/storage/, core/auth/ (browser + node + remote) - core/mcp/ runtime: inspectorClient.ts (2169 LOC), oauthManager, messageTrackingTransport, fetchTracking, config, index - core/mcp/node/ (stdio transport) - core/mcp/remote/ + remote/node/ (HTTP/SSE transport + Hono server) - test-servers/ package (composable MCP test servers, fixtures, harness) v2 adapter layer ---------------- - types.ts: append CreateTransport*, AppRendererClient, InspectorClient{Environment,Options} (v2 had pruned these — v1.5 needs them) - samplingCreateMessage / elicitationCreateMessage: keep v1.5 classes AND v2's Inspector Pending* interfaces in the same files - inspectorClientEventTarget: add oauth* event keys; switch pending sampling/elicitation payloads to the classes (matches v1.5 contracts) - json/jsonUtils: append convertParameterValue/convertToolParameters/ convertPromptArguments + ParameterSchema - inspectorClient.ts: re-export Options/Environment/Transport* so tests that import these from `inspectorClient.js` keep resolving; replace `../package.json` import with a hardcoded identity (v2 has no core/package.json) - remote/node/server.ts: cast hono Context for streamSSE (hono generic tightened since v1.5) - oauth-callback-server: drop unused hostname field - tsconfig.app.json / tsconfig.test.json: add @types/node + paths for pino/zustand/hono/@hono/node-server/atomically/express/yaml, disable erasableSyntaxOnly (parameter-property syntax from v1.5) - vite.config.ts: regex-form module aliases (so the package's exports field handles subpaths like `hono/streaming`, `zustand/middleware`), alias for @modelcontextprotocol/inspector-test-server → test-servers/src Tests ----- - 19 v1.5 test files copied under clients/web/src/test/core/, imports rewired to @inspector/core/* and @modelcontextprotocol/inspector-test-server - 1004 unit tests pass; 298 storybook tests pass; coverage gate passes Known follow-ups (out-of-scope here, tracked in TODOs) ------------------------------------------------------ - 13 integration test files are excluded from the unit project (spawn real HTTP/stdio servers; run real OAuth flows; mock SDK auth → happy-dom CORS). Needs a node-env vitest project. Files listed under `test.projects[0].test.exclude`. - Source files exercised only by those integration tests are excluded from the coverage gate pending the node-env project. Listed under `test.coverage.exclude` with comment. - Manual smoke tests against real stdio / HTTP / OAuth server per the AC on #1302 still need verification locally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs/fix: address #1306 review (AGENTS tree, #1307 links, @types/node, .substr) - AGENTS.md: expand the project-structure tree to include all v1.5-ported subsystems (core/auth/{browser,node,remote}, core/storage, core/logging, core/mcp/{node,remote/{,node}}, core/mcp/state) plus the new top-level test-servers/ directory (with a note about its @modelcontextprotocol/ inspector-test-server alias). - vite.config.ts: reference #1307 from both the test exclude block and the coverage exclude block so the next person editing either sees the marching order for shrinking the lists. - package.json: restore @types/node to ^24 (an earlier `npm install --save-dev @types/node@^22` accidentally pinned it lower than the repo was using; nothing here required ^22). - core/mcp/fetchTracking.ts: replace deprecated .substr(2, 9) with .slice(2, 11) to match the convention in core/mcp/inspectorClient.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
a71e6d2012 |
||
|
|
7fecebed5e |
||
|
|
f4fb4b50c9 | ||
|
|
f9f79fff54 | ||
|
|
19a6df7ad3 | ||
|
|
a029e42a19 |
||
|
|
32a3610737 | ||
|
|
1e6a2825d4 | ||
|
|
54d5b18b1e | ||
|
|
edc7dbb1c1 | ||
|
|
ba48e06ffd | ||
|
|
af174666d6 | ||
|
|
3c65bdb4c6 | ||
|
|
512f45d357 | ||
|
|
cfbdfdf2bd | ||
|
|
a24091484c | ||
|
|
f4b6095719 | ||
|
|
a066873e37 | ||
|
|
52023ff1c3 | ||
|
|
562bdd00f1 | ||
|
|
588fc3f8ea | ||
|
|
4e149719bd | ||
|
|
8c10b8eb8e | ||
|
|
c299b6e9d8 | ||
|
|
13dafe457b |
||
|
|
c96db0402e |
||
|
|
5de4e911e3 |
||
|
|
98192fd86e |
||
|
|
037225a9fb |
||
|
|
82269e78cb |
||
|
|
0ead1deea6 |