Skip to content

Guided-auth removal, OAuth smoke testing, and TUI/CLI support#1531

Open
BobDickinson wants to merge 6 commits into
v2/mainfrom
feat/1514-remove-guided-auth
Open

Guided-auth removal, OAuth smoke testing, and TUI/CLI support#1531
BobDickinson wants to merge 6 commits into
v2/mainfrom
feat/1514-remove-guided-auth

Conversation

@BobDickinson

Copy link
Copy Markdown
Contributor

How this PR grew

This PR started as a narrow cleanup: remove guided OAuth from v2. Guided auth (beginGuidedAuth, runGuidedAuth, proceedOAuthStep, and core/auth/state-machine.ts) was a debugging-oriented, step-through flow that duplicated what the normal OAuth path already does; product flows use authenticate() and completeOAuthFlow() only. Removing it simplified OAuthManager, InspectorClient, and the test surface.

The next step was to validate standard OAuth against real MCP servers and authorization servers—not only the in-repo TestServerHttp fixtures. That work included:

  • specification/v2_auth_smoke_testing.md — manual procedures for header/PAT bypass, static (preregistered) client, DCR, and CIMD against hosted endpoints (MCP Example, GitHub MCP, Stytch demo, xaa.dev EMA, etc.).

.gitignore now includes /configs/ so developers can keep local catalog and client-config files for smokes without checking them in. The smoke spec describes how to set those up (--client-config, --catalog, default paths under ~/.mcp-inspector/).

Running those smokes on web required restoring two v1/v1.5 capabilities that had not yet been ported to v2:

  1. Install-level CIMD — supply a Client ID Metadata Document URL from Client Settings / client.json and wire it into InspectorClient (needed for Stytch and similar ASes).
  2. Per-server OAuth clear — reset stored tokens and DCR registration between smoke phases via Clear stored OAuth state in Server Settings and Connection Info.

TUI work to run the same smokes:

  • Load client.json at startup (EMA IdP, CIMD) via loadRunnerClientConfig / buildRunnerClientAuthOptions.
  • OAuth on connect — 401 → authenticate → loopback callback on 127.0.0.1:6276 (default runner port; T9 “MCPO”).
  • Auth tab aligned with web Connection Info: protocol, registration kind (static / DCR / CIMD), IdP session (EMA), clear OAuth (S; disconnects when connected).
  • Keychain rehydration in loadServerEntries() so TUI/CLI read oauth.clientSecret from the OS keychain after web Server Settings saved it.
  • Core connect behavior — omit transport authProvider when no tokens exist (so the SDK does not navigate before the callback server listens); dropCachedTransport() so connect → OAuth → connect works on the same InspectorClient.

CLI — shared flags (--client-config, CIMD/static overrides, --callback-url), async loadServerEntries, token reuse from ~/.mcp-inspector/storage/oauth.json. Interactive OAuth (callback server, 401 retry) remains a Phase 4 follow-up; see CLI README and smoke spec known gaps.

EMA — same runner/client-config path; Phase 4 checklist and smoke cross-links updated in v2_auth_ema.md.

The diff (~90 files) spans guided-auth removal, smoke-test documentation, web parity, TUI/CLI runners, core connect fixes, and specs.


Summary

Remove guided OAuth and enable production-style OAuth smoke testing across web, TUI, and CLI: install-level CIMD, per-server OAuth clear, TUI connect-time auth with loopback callback, shared client.json and keychain-aware catalog load, and auth display including client registration kind (static / DCR / CIMD).


Core / auth

  • Remove guided OAuth API (beginGuidedAuth, runGuidedAuth, proceedOAuthStep, core/auth/state-machine.ts, oauthStepChange); single authenticate() / completeOAuthFlow() path.
  • Connect-time OAuth: omit transport authProvider when no stored tokens; attach when authorized.
  • transportHasAuthProvider + dropCachedTransport() for connect → OAuth → connect on the same instance.
  • Runner helpers: core/client/runner.ts, core/auth/node/runner-oauth-callback.ts (default callback 6276).
  • Keychain: rehydrateMcpConfigFromKeychain() in loadServerEntries() for TUI/CLI (same effective secrets as web GET /api/servers).
  • Connection state: OAuthClientRegistrationKind; CIMD registrationKind on saveClientInformation.

Web

  • Client Settings: CIMD metadata URL → InspectorClient at connect.
  • Clear OAuth: Server Settings + Connection Info (clearServerOAuthState).
  • Connection Info: registration kind, access token field.

TUI

  • client.json at startup; OAuth on HTTP/SSE servers.
  • Connect (C) → OAuth on 401; callback on 6276; Auth tab details + S clear.
  • Auth tab shown only for sse / streamable-http (isOAuthCapableServerConfig in App.tsx).

CLI

  • --client-config, --client-id, --client-secret, --client-metadata-url, --callback-url; async loadServerEntries.
  • Token reuse from ~/.mcp-inspector/storage/oauth.json; interactive OAuth deferred (README).

Specs / docs

  • specification/v2_auth_smoke_testing.md — real-server matrix, TUI/CLI procedures.
  • .gitignore/configs/ for optional local dev config (not shipped).
  • v2_auth_ema.md, v2_servers_file.md, v2_scope.md — runner/keychain updates.
  • clients/tui/README.md, clients/cli/README.md.

Removed / replaced

  • specification/v2_enterprise_managed_auth.mdv2_auth_ema.md.
  • clients/web/src/test/integration/auth/state-machine.test.ts.

Test plan

  • npm run validate (web, CLI, TUI, launcher)
  • npm run test:integration (web) — OAuth e2e including connect retry without disconnect
  • Manual web OAuth smokes (author)
  • Manual TUI smokes: DCR, static, EMA, clear + reconnect (author)

Manual procedures: specification/v2_auth_smoke_testing.md.


Follow-ups (not in this PR)

  • CLI interactive OAuth (callback server + 401 retry) — EMA Phase 4.
  • Web shared OAuth store (RemoteOAuthStorage / oauth.json parity with TUI/CLI).

BobDickinson and others added 3 commits June 25, 2026 16:03
- 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.
Resolve Code.ts theme conflict: keep both block margin reset and nowrap
ellipsis variant from v2/main.

Co-authored-by: Cursor <cursoragent@cursor.com>
@BobDickinson

Copy link
Copy Markdown
Contributor Author
Screenshot 2026-06-26 at 9 21 21 PM

CIMD support in Client settings

@BobDickinson

Copy link
Copy Markdown
Contributor Author
Screenshot 2026-06-26 at 9 48 49 PM

Clear oauth state on server settings

@BobDickinson

Copy link
Copy Markdown
Contributor Author
Screenshot 2026-06-26 at 9 19 15 PM

Successful DCR OAuth connection

@BobDickinson

Copy link
Copy Markdown
Contributor Author
Screenshot 2026-06-26 at 9 40 08 PM

Successful CIMD OAuth connection

@BobDickinson

Copy link
Copy Markdown
Contributor Author
Screenshot 2026-06-26 at 9 19 44 PM

Successful static client OAuth connection

@BobDickinson

Copy link
Copy Markdown
Contributor Author
Screenshot 2026-06-26 at 9 49 42 PM

TUI auth details page (no guided or explicit auth, shows similar details to web client)

@BobDickinson

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @BobDickinson's task in 4m 17s —— View job


PR Review — Guided-auth removal, CIMD, OAuth smoke testing

I read the diff against origin/v2/main, focusing on the core auth/connect changes, the web CIMD + clear-OAuth UI, and the TUI/CLI runners. Overall this is a clean, well-structured PR: the guided-auth removal is thorough (no dangling references to beginGuidedAuth/runGuidedAuth/proceedOAuthStep/oauthStepChange/AuthExecution/state-machine), the connect-time auth model is now unified across web/TUI, and the new core/client/runner.ts + runner-oauth-callback.ts helpers nicely deduplicate TUI/CLI wiring. Most new core modules ship with tests.

A few things worth a look before merge — mostly test-coverage gaps rather than logic bugs.

Findings

Correctness / behavior (looks right, noting for the record)

  • Connect-time authProvider omission is consistent across clients. inspectorClient.ts now omits authProvider when not authorized so connect() surfaces a plain 401, and web (App.tsx:1615), TUI (runOAuthAuthenticationfinishConnect), all explicitly call authenticate() on 401. isOAuthAuthorized() returns true whenever any tokens exist (not gated on expiry), so a stale access token with a refresh token still attaches the provider and lets the SDK refresh — no silent-refresh regression. 👍
  • dropCachedTransport() + transportHasAuthProvider correctly enables connect → OAuth → connect on the same instance. The top-of-connect() guard only drops when !transportHasAuthProvider && isOAuthAuthorized(), which is the right condition.
  • All three storage classes (Browser/Node/Remote) extend OAuthStorageBase, so the new required getClientRegistrationKind / 3-arg saveClientInformation are inherited — no missing implementations.

Test coverage gaps (CI runs validate, not coverage, so these won't fail CI but will fail the local per-file gate)

  1. clients/cli/src/cli.ts gained ~100 lines (OAuth environment.oauth setup, buildRunnerClientAuthOptions wiring, and 5 new flags), but I don't see any CLI test exercising the new flags/branches (grep for --client-config/clientMetadataUrl/callback-url in clients/cli/__tests__ is empty). Per AGENTS.md the CLI enforces the same 90%-lines per-file gate. Please confirm npm run coverage (or coverage:cli) still passes. Fix this →
  2. core/mcp/node/server-secrets.ts (rehydrateMcpConfigFromKeychain) has no dedicated test. It's exercised transitively through loadServerEntries in servers.test.ts (which now injects a secretStore), so it's probably covered — just worth confirming the file hits the line gate.
  3. Per-server clientRegistrationKind round-trip is verified for the in-memory store (connection-state.test.ts, oauthManager.test.ts) and install-level CIMD in client-store-route.test.ts, but not through RemoteOAuthStorage's server sync. Web today uses browser storage so this is fine; flagging since the field is new on ServerOAuthState and relies on the generic store middleware to persist.

Minor / nits

  1. parseRunnerOAuthCallbackUrl allows port 0, and the default changed from ephemeral 0 → fixed 6276. For TUI this is safe because runOAuthAuthentication overwrites redirectUrlProvider.redirectUrl with the live callback server's actual URL. But formatRunnerOAuthRedirectUrl({port:0}) would emit …:0/oauth/callback if used before that overwrite — only correct because of the overwrite. Worth a one-line comment noting that invariant.
  2. Fixed default port 6276 means two concurrent TUI instances doing OAuth at once would EADDRINUSE. Acceptable and documented, but a behavior change from the old ephemeral default — make sure the README/known-gaps mention it (the smoke spec does cover the port choice).
  3. clearServerOAuthState.ts reaches for getBrowserOAuthStorage() directly for the non-active-connection path. Correct for current web (browser storage), but it'll need revisiting when the RemoteOAuthStorage/oauth.json parity follow-up lands — already called out in your Follow-ups list, so just linking the two.
  4. Nice cleanup removing the inline wordBreak: break-all span style in OAuthAccessTokenField — the Code wrapping variant already applies break-all, so this correctly moves to the theme layer per the repo's no-inline-style rule.

Verdict

No blocking logic issues found. The main ask is to run npm run coverage locally and confirm the new CLI code (and server-secrets.ts) clear the per-file gate, since CI won't catch a coverage regression here. The minor items (1) are optional polish.
· feat/1514-remove-guided-auth

@BobDickinson

Copy link
Copy Markdown
Contributor Author

Review actions and feedback

Test coverage gaps

1. clients/cli/src/cli.ts — OAuth flags / per-file coverage gate

Added clients/cli/__tests__/oauth-runner.test.ts — five in-process tests via runCli: HTTP connect with --client-id / --client-secret / --client-metadata-url / --callback-url (asserts overrides reach buildRunnerClientAuthOptions); --client-config loading with CLI CIMD override; port-0 callback URL; MCP_OAUTH_CALLBACK_URL env fallback; invalid callback URL rejected before connect. npm run test:coverage in clients/cli passes (108 tests; cli.ts ~98% lines).

Shared wiring lives in core/client/runner.ts and core/auth/node/runner-oauth-callback.ts, already covered under web in runner.test.ts and runner-oauth-callback.test.ts. TUI has the same flag glue in tui.tsx but that entry point is excluded from the TUI per-file gate until #1501.

2. core/mcp/node/server-secrets.ts (rehydrateMcpConfigFromKeychain)

No dedicated unit test. Covered transitively by clients/web/src/test/core/mcp/node/servers.test.ts“rehydrates OAuth client secrets from the keychain when stripped on disk” exercises loadServerEntriesrehydrateMcpConfigFromKeychain. Intentionally left at that boundary; no separate file needed.

3. Per-server clientRegistrationKind via RemoteOAuthStorage

Acknowledged. Round-trip is tested for the in-memory store (connection-state.test.ts, oauthManager.test.ts) and install-level CIMD in client-store-route.test.ts. Remote sync path not covered here; web uses browser storage today. Will add when RemoteOAuthStorage / oauth.json parity lands (follow-up in PR description).

Minor / nits

4. Port 0 / formatRunnerOAuthRedirectUrl

Added JSDoc on formatRunnerOAuthRedirectUrl: port 0 yields …:0/…; the TUI overwrites redirectUrlProvider.redirectUrl with the bound listener URL before OAuth starts, so :0 is never sent to the AS.

5. Fixed default port 6276 / concurrent TUI OAuth

Documented in clients/tui/README.md (why EMA/CIMD/static need a pre-registered redirect URI; one OAuth flow at a time on the default port → possible EADDRINUSE; override via --callback-url / MCP_OAUTH_CALLBACK_URL or :0 for DCR). Module comment on core/auth/node/runner-oauth-callback.ts states the same. Smoke spec already covers 6276 and ephemeral :0.

6. clearServerOAuthState.tsgetBrowserOAuthStorage()

Correct for current web (browser storage). Will revisit with the RemoteOAuthStorage / oauth.json parity follow-up — same item as test coverage gap #3.

7. OAuthAccessTokenField inline style removal

Agreed — Code variant already applies break-all; no further change.

@BobDickinson BobDickinson added auth Issues and PRs related to authorization v2 Issues and PRs for v2 labels Jun 27, 2026
@BobDickinson BobDickinson requested a review from cliffhall June 27, 2026 06:28
@BobDickinson

Copy link
Copy Markdown
Contributor Author

This is the next step in the auth cleanup/evolution. When this is merged, I will finish up #1514 (which should be more focussed), then do #1526 (which will handle mid-session auth and step up, but also addresses other common issues like invalid token on initial auth, token refresh, etc). All of that combined should put our OAuth support in pretty good shape.

@BobDickinson BobDickinson marked this pull request as ready for review June 28, 2026 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auth Issues and PRs related to authorization v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant