feat: OAuth MCP Server Connections (HTTP) #137
Conversation
- InspectorOAuthProvider: implements MCP SDK OAuthClientProvider interface - Token persistence via TokenStore - PKCE code verifier storage (memory + disk) - Dynamic client registration support (opt-in) - Authorization redirect with pending URL mechanism - Status tracking for dashboard display - Credential invalidation by scope - TokenStore: XDG-compliant persistent token storage - Per-server-URL storage at ~/.config/mcp-inspector/tokens/ - SHA-256 URL hashing for filesystem-safe filenames - Atomic writes (temp + rename) to prevent corruption - Merge semantics (saves don't overwrite unrelated fields) - Restrictive file permissions (0o600) - OAuth types: shared types for the oauth module - Re-exports SDK types (OAuthTokens, OAuthClientProvider, etc.) - PersistedTokenData, OAuthClientConfig, OAuthState, OAuthPresetConfig - 47 tests covering both TokenStore and InspectorOAuthProvider
- TestClient: accept authProvider option, pass to StreamableHTTPClientTransport - ConnectionManager: create InspectorOAuthProvider when oauthConfig provided, wire to createTestClient, expose getOAuthProvider/getOAuthState/setOAuthProvider, clear provider on disconnect - ConnectionRegistry: include OAuth state in listConnections output - connection-types: add oauthConfig to ConnectOptions, oauth to ConnectionStatusOutput - 10 integration tests covering OAuth+Connection wiring
Implement OAuth callback and API endpoints for both server types: callback-handler.ts: - GET /oauth/callback: auth code exchange via MCP SDK auth(), HTML response - POST /api/oauth/configure: create OAuth provider for a connection - GET /api/oauth/status?connectionId=X: poll auth state + pending URL - POST /api/oauth/revoke?connectionId=X: local token invalidation - Single handleOAuthRoutes() dispatcher, connectionId-aware with fallback standalone-server.ts: - Wire handleOAuthRoutes before health check route dual-server.ts: - Wire handleOAuthRoutes before /api/connect route Tests: 41 tests covering all 4 endpoints (routing, CORS, error handling, configure validation, connectionId resolution, token lifecycle, XSS)
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds end-to-end OAuth2.1 + PKCE support to the inspector: new SDK dependency, CLI preset flags, server OAuth routes and .well‑known proxy, InspectorOAuthProvider and XDG-backed TokenStore, dashboard hook/UI and wiring into connections, test-client auth propagation, extensive tests, and build/test config updates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Dashboard as Dashboard UI
participant Hook as useOAuth Hook
participant API as Inspector API (/api/oauth)
participant Provider as InspectorOAuthProvider
participant AuthServer as OAuth Authorization Server
participant Store as Token Store
User->>Dashboard: Click "Configure OAuth"
Dashboard->>Hook: configure(params)
Hook->>API: POST /api/oauth/configure (connectionId)
API->>Provider: initialize & build auth URL
Provider->>AuthServer: Redirect to authorization endpoint
AuthServer-->>User: User consents -> callback to /oauth/callback
API->>Provider: exchange code for tokens
Provider->>Store: save tokens & metadata
API-->>Hook: return configured / authorizationUrl
Hook->>Dashboard: poll GET /api/oauth/status -> show status
sequenceDiagram
actor CLI
participant Inspector as Inspector Startup
participant Preset as Preset Config Resolution
participant Provider as InspectorOAuthProvider
participant Store as Token Store
participant AuthServer as OAuth Server
CLI->>Inspector: start with preset flags + --url
Inspector->>Preset: resolvePresetConfig(flags)
Preset->>Store: checkExistingTokens(serverUrl)
alt tokens exist
Preset->>Inspector: supply provider (non-interactive)
Inspector->>CLI: startup completes (no interactive auth)
else tokens missing
Preset->>Provider: create preset provider
Provider->>AuthServer: produce authorizationUrl
CLI/User->>AuthServer: authorize / callback -> exchange -> persist tokens
Inspector->>CLI: ready
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Code Review: PR #137 - OAuth MCP Server Connections📋 OverviewThis is a substantial and well-architected OAuth integration for the MCP Inspector. The PR adds ~8,000 lines across 34 files, implementing comprehensive OAuth 2.0 support with PKCE, token management, dynamic client registration, and both dashboard and CLI modes. ✅ Strengths1. Excellent Code Organization
2. Security Best Practices
3. RFC Compliance
4. Comprehensive Test Coverage
5. Error Handling
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@packages/inspector/src/dashboard/react/components/OAuthPanel.tsx`:
- Around line 241-252: The function getStatusBadgeStyle currently returns a
value that TypeScript narrows to CSSProperties | undefined causing TS2322;
explicitly annotate its return type as React.CSSProperties in the signature
(function getStatusBadgeStyle(status: string): React.CSSProperties) so the
compiler knows it always returns a defined style, and ensure the referenced
style keys on oauthPanelStyles (statusAuthenticated, statusAuthenticating,
statusError, statusUnauthenticated) are typed as React.CSSProperties in the
oauthPanelStyles declaration or its type so all returned branches satisfy the
annotated return type.
In `@packages/inspector/src/dashboard/react/hooks/useOAuth.ts`:
- Around line 135-141: When connectionId is falsy the effect resets oauth state
but leaves previous error and loading flags intact; update the same useEffect
(the one that checks connectionId) to also clear the error and loading state by
calling setError(null) and setIsLoading(false) alongside setOauthState(null),
setIsConfigured(false), and setAuthorizationUrl(null) so stale errors or
spinners are removed when no connection is active.
In `@packages/inspector/src/oauth/callback-handler.ts`:
- Around line 536-542: The escapeHtml function currently replaces &, <, >, and "
but misses single quotes, which can enable attribute-injection risks; update the
escapeHtml function to also replace single quotes (e.g., add .replace(/'/g,
"'") or equivalent) so escapeHtml consistently encodes '&', '<', '>', '"',
and '\''; ensure the replacement order remains correct and that the function
name escapeHtml is updated in-place with this additional .replace call.
In `@packages/inspector/src/oauth/preset-config.ts`:
- Line 16: The import line in preset-config.ts imports OAuthPresetConfig but
it's not used; update the type-only import to remove OAuthPresetConfig and keep
only OAuthClientConfig (i.e., change the import from "import type {
OAuthClientConfig, OAuthPresetConfig } from './types';" to import only
OAuthClientConfig) to resolve the ESLint unused-import error and preserve the
type usage for OAuthClientConfig.
In `@packages/inspector/src/oauth/provider.ts`:
- Around line 190-196: In saveTokens, replace the non-strict null check on
tokens.expires_in (currently using != null) with a strict comparison to handle
both null and undefined per ESLint eqeqeq; for example check tokens.expires_in
!== undefined && tokens.expires_in !== null or simply tokens.expires_in != null
alternative using optional chaining like (tokens.expires_in ?? null) and then
proceed to set this._expiresAt as before; update the conditional in the
saveTokens method accordingly to satisfy ESLint while preserving the existing
semantics.
- Around line 250-259: In invalidateCredentials when scope === "tokens" the
current save({ tokens: undefined, clientInformation }) doesn't clear tokens
because TokenStore.save uses a nullish-coalescing fallback; instead explicitly
remove stored tokens then re-save only the client info: call
this.tokenStore.delete(this.serverUrl) (to remove the token entry) and then
this.tokenStore.save(this.serverUrl, { clientInformation:
existing.clientInformation }) so clientInformation is preserved but tokens are
cleared; update the block handling scope "tokens" to use tokenStore.delete +
save as described.
In `@packages/inspector/src/oauth/token-store.ts`:
- Around line 117-128: The current save logic constructs a fallback tokens
object ({ access_token: "", token_type: "bearer" }) which can persist invalid
token data; update the save path that builds the merged PersistedTokenData to
only accept real tokens (e.g., prefer data.tokens, then existing?.tokens) and if
neither contains a non-empty access_token then either omit tokens from merged or
throw/return an error instead of inserting the empty stub; ensure any downstream
load/validation (referenced by tokens and PersistedTokenData) relies on a
non-empty access_token before persisting or explicitly marks the record as
incomplete so invalid empty tokens are not saved.
In `@packages/inspector/src/oauth/wellknown-proxy.ts`:
- Around line 108-131: The fetchUpstream function can hang if fetch never
returns; add an AbortController-based timeout inside fetchUpstream (use
AbortController and pass its signal to fetch), make the timeout value
configurable (e.g., via a constant or env var like FETCH_TIMEOUT_MS) and start a
setTimeout to call controller.abort() after the timeout; ensure you clearTimeout
on success/failure, handle the abort error path (treat as a timeout and return
null) and keep existing log messages, referencing fetchUpstream, authToken, and
the AbortController/signal usage so the request is cancelled instead of blocking
indefinitely.
In `@packages/inspector/tests/oauth-token-store.test.ts`:
- Line 6: The test titled something like "verifies restrictive permissions"
currently never checks the file mode; add an assertion that the token file's
mode is restrictive (e.g. check via await stat(filePath) or
fs.statSync(filePath) and assert (st.mode & 0o177) === 0 or an equivalent mask)
to ensure group/other bits are cleared, and guard this check behind a platform
guard (if (process.platform !== 'win32') { ... }) so it skips on Windows; locate
the assertion to run after the token file is created/closed in the same test
(and apply the same addition to the other similar test around lines ~195-209)
and reference mkdtemp/readFile/rm as the surrounding helpers to find the right
spot.
🧹 Nitpick comments (17)
packages/inspector/tests/oauth-integration-acceptance.test.ts (2)
22-76: Add file-level cleanup for global spies.
Several tests spy on global fetch; restoring mocks after each test prevents cross-test contamination.🧹 Proposed test cleanup
vi.mock("@mcp-apps-kit/testing", () => { const client = { listTools: vi.fn().mockResolvedValue([]), @@ }; return { createTestClient: (...args: unknown[]) => { createTestClientSpy(...args); return Promise.resolve(client); }, }; }); + +afterEach(() => { + vi.restoreAllMocks(); +}); import { InspectorOAuthProvider } from "../src/oauth/provider";
1235-1331: Add disconnect cleanup for ConnectionManager-based tests.
Even with mocks, explicit disconnects prevent future open-handle leaks if connection logic changes.As per coding guidelines: Clean up servers in tests to avoid resource leaks and hanging processes.🧹 Proposed cleanup
it("should connect and work without any OAuth config (HTTP transport)", async () => { const manager = new ConnectionManager(); - const result = await manager.connect({ - transport: "http", - url: "http://localhost:3000/mcp", - }); - - expect(result.toolCount).toBe(0); - expect(manager.getOAuthProvider()).toBeNull(); - expect(manager.getOAuthState()).toBeUndefined(); - - // createTestClient should NOT receive authProvider - expect(createTestClientSpy).toHaveBeenCalledWith( - { transport: "http", url: "http://localhost:3000/mcp" }, - expect.not.objectContaining({ authProvider: expect.anything() }) - ); + try { + const result = await manager.connect({ + transport: "http", + url: "http://localhost:3000/mcp", + }); + + expect(result.toolCount).toBe(0); + expect(manager.getOAuthProvider()).toBeNull(); + expect(manager.getOAuthState()).toBeUndefined(); + + // createTestClient should NOT receive authProvider + expect(createTestClientSpy).toHaveBeenCalledWith( + { transport: "http", url: "http://localhost:3000/mcp" }, + expect.not.objectContaining({ authProvider: expect.anything() }) + ); + } finally { + await manager.disconnect(); + } }); @@ it("should connect via stdio without OAuth (even if oauthConfig is mistakenly passed)", async () => { const manager = new ConnectionManager(); - await manager.connect( - { transport: "stdio", command: "node", args: ["server.js"] }, - { - oauthConfig: { - clientId: "should-be-ignored", - redirectUri: "http://127.0.0.1:6274/oauth/callback", - }, - } - ); - - // OAuth should NOT be created for stdio - expect(manager.getOAuthProvider()).toBeNull(); - expect(manager.getOAuthState()).toBeUndefined(); + try { + await manager.connect( + { transport: "stdio", command: "node", args: ["server.js"] }, + { + oauthConfig: { + clientId: "should-be-ignored", + redirectUri: "http://127.0.0.1:6274/oauth/callback", + }, + } + ); + + // OAuth should NOT be created for stdio + expect(manager.getOAuthProvider()).toBeNull(); + expect(manager.getOAuthState()).toBeUndefined(); + } finally { + await manager.disconnect(); + } });packages/inspector/tests/wellknown-proxy.test.ts (2)
325-349: Consider using fake timers instead of real delays for TTL expiry test.The test relies on
setTimeoutwith a real 60ms delay (line 341), which can cause flakiness in CI environments under load. Using Vitest's fake timers would make the test deterministic and faster.♻️ Proposed refactor using fake timers
it("should re-fetch after cache TTL expires", async () => { + vi.useFakeTimers(); // Create proxy with very short TTL const shortTtlProxy = createWellKnownProxy({ cacheTtlMs: 50 }); const fetchSpy = vi .spyOn(globalThis, "fetch") .mockResolvedValue(new Response(JSON.stringify(sampleResourceMetadata), { status: 200 })); const req = mockRequest("GET", "/.well-known/oauth-protected-resource"); // First fetch const res1 = mockResponse(); await shortTtlProxy.handleWellKnownRequest(req, res1, defaultCtx); expect(fetchSpy).toHaveBeenCalledTimes(1); // Wait for TTL to expire - await new Promise((resolve) => setTimeout(resolve, 60)); + vi.advanceTimersByTime(60); // Should re-fetch const res2 = mockResponse(); await shortTtlProxy.handleWellKnownRequest(req, res2, defaultCtx); expect(fetchSpy).toHaveBeenCalledTimes(2); shortTtlProxy.clearCache(); + vi.useRealTimers(); });
419-422: Type assertion on headers may be fragile.The assertion
fetchOptions.headers as Record<string, string>assumes the headers are a plain object. If the implementation usesHeadersinstance or array format, this would fail silently. Consider adding a runtime check or using a more defensive approach.♻️ Proposed defensive check
// Verify Authorization header was sent const fetchArgs = fetchSpy.mock.calls[0]; const fetchOptions = fetchArgs[1] as RequestInit; - const headers = fetchOptions.headers as Record<string, string>; - expect(headers["Authorization"]).toBe("Bearer test-bearer-token"); + const headers = fetchOptions.headers; + if (headers instanceof Headers) { + expect(headers.get("Authorization")).toBe("Bearer test-bearer-token"); + } else if (Array.isArray(headers)) { + const authHeader = headers.find(([k]) => k === "Authorization"); + expect(authHeader?.[1]).toBe("Bearer test-bearer-token"); + } else { + expect((headers as Record<string, string>)["Authorization"]).toBe("Bearer test-bearer-token"); + }packages/inspector/src/dashboard/react/components/Toolbar.tsx (1)
81-110: Nested ternaries are readable but could be simplified with a lookup object.The triple-nested ternary expressions for
icon,label, andbadgeStyle(lines 82-98) work correctly but could be more maintainable with a status-to-config mapping.♻️ Optional refactor using a lookup object
+const oauthStatusConfig: Record<OAuthStatus, { icon: string; label: string; style: React.CSSProperties }> = { + authenticated: { icon: "🔒", label: "Auth", style: oauthBadgeStyles.authenticated }, + authenticating: { icon: "🔄", label: "Auth...", style: oauthBadgeStyles.authenticating }, + error: { icon: "🔓", label: "Auth Err", style: oauthBadgeStyles.error }, + unauthenticated: { icon: "🔓", label: "No Auth", style: oauthBadgeStyles.unauthenticated }, +}; + /** OAuth status indicator icon + label */ function OAuthBadge({ status }: { status: OAuthStatus }): React.ReactElement { - const icon = status === "authenticated" ? "🔒" : status === "authenticating" ? "🔄" : "🔓"; - const label = - status === "authenticated" - ? "Auth" - : status === "authenticating" - ? "Auth..." - : status === "error" - ? "Auth Err" - : "No Auth"; - const badgeStyle = - status === "authenticated" - ? oauthBadgeStyles.authenticated - : status === "authenticating" - ? oauthBadgeStyles.authenticating - : status === "error" - ? oauthBadgeStyles.error - : oauthBadgeStyles.unauthenticated; + const config = oauthStatusConfig[status]; return ( <div - style={{ ...oauthBadgeStyles.badge, ...badgeStyle }} + style={{ ...oauthBadgeStyles.badge, ...config.style }} title={`OAuth: ${status}`} data-testid="oauth-status-indicator" > - <span>{icon}</span> - <span>{label}</span> + <span>{config.icon}</span> + <span>{config.label}</span> </div> ); }packages/inspector/src/connection.ts (1)
236-270: Refactor non-null assertions to satisfy ESLint rules.Lines 244 and 264 use forbidden non-null assertions (
authProvider!). SinceauthProvideris defined at the start of this block and assigned before these callbacks are registered, you can safely capture it in a local constant before registering the callback.♻️ Proposed fix to eliminate non-null assertions
// Set up OAuth provider for HTTP connections. // Prefer pre-built provider (e.g., from CLI preset) over oauthConfig. let authProvider: InspectorOAuthProvider | undefined; if (params.transport === "http" && prebuiltAuthProvider) { authProvider = prebuiltAuthProvider; + const provider = authProvider; // Track OAuth status changes - authProvider.onStatusChange = () => { + provider.onStatusChange = () => { if (this.debug) { - const state = authProvider!.getOAuthState(); + const state = provider.getOAuthState(); console.log(`[inspector] OAuth status changed: ${state.status}`); } }; this.oauthProvider = authProvider; } else if (params.transport === "http" && oauthConfig) { // Port is set later via setInspectorUrl, default to 6274 const port = this.inspectorUrl ? new URL(this.inspectorUrl).port : "6274"; authProvider = new InspectorOAuthProvider({ serverUrl: params.url, config: oauthConfig, callbackPort: parseInt(port, 10), debug: this.debug, }); + const provider = authProvider; // Track OAuth status changes - authProvider.onStatusChange = () => { + provider.onStatusChange = () => { if (this.debug) { - const state = authProvider!.getOAuthState(); + const state = provider.getOAuthState(); console.log(`[inspector] OAuth status changed: ${state.status}`); } }; this.oauthProvider = authProvider; }packages/inspector/tests/oauth-preset-config.test.ts (1)
103-109: Consider clarifying the expected trimming behavior.The test at line 108 expects
"read write"(three spaces) due to.replace(/,/g, " ").trim()convertingread , writetoread write. This seems intentional based on the implementation, but the test name "should trim whitespace from scopes" may be slightly misleading since internal whitespace is preserved. The implementation trims only leading/trailing whitespace, not internal gaps.Consider either:
- Renaming the test to clarify it only trims outer whitespace
- Or normalizing multiple spaces to single spaces in the implementation
- it("should trim whitespace from scopes", () => { + it("should trim leading and trailing whitespace from scopes", () => {packages/inspector/src/dashboard/react/components/OAuthPanel.tsx (2)
258-269: Consider updatingRefObjecttypes for React 19 compatibility.In React 19,
RefObject<T>now includesnullin the type by default. The current typeRefObject<HTMLElement>may cause type issues. While the runtime code correctly handles null (Line 345), the type definition could be more explicit.♻️ Suggested type update
export interface OAuthPanelProps { /** Whether the panel is visible */ isOpen: boolean; /** Anchor element ref for positioning */ - anchorRef: React.RefObject<HTMLElement>; + anchorRef: React.RefObject<HTMLElement | null>; /** Container element ref for boundary calculations */ - containerRef: React.RefObject<HTMLElement>; + containerRef: React.RefObject<HTMLElement | null>; /** Close the panel */ onClose: () => void; /** OAuth hook result */ oauth: UseOAuthResult; }
305-335: Consider extracting scope parsing logic to reduce duplication.The scope string parsing logic (
scopes.split(/\s+/).map(s => s.trim()).filter(Boolean)) is repeated in three places:selectedScopesSet(Line 299-302),toggleScope(Line 307-311), andaddCustomScope(Line 326-330). This creates maintenance overhead.♻️ Suggested refactor
+ // Helper to parse scopes string into a Set + const parseScopesString = useCallback((scopeStr: string): Set<string> => { + return new Set( + scopeStr + .split(/\s+/) + .map((s) => s.trim()) + .filter(Boolean) + ); + }, []); + // Parse current scopes string into a Set for quick lookups - const selectedScopesSet = new Set( - scopes - .split(/\s+/) - .map((s) => s.trim()) - .filter(Boolean) - ); + const selectedScopesSet = useMemo(() => parseScopesString(scopes), [scopes, parseScopesString]); const toggleScope = useCallback( (scope: string) => { - const current = new Set( - scopes - .split(/\s+/) - .map((s) => s.trim()) - .filter(Boolean) - ); + const current = parseScopesString(scopes); if (current.has(scope)) { current.delete(scope); } else { current.add(scope); } setScopes(Array.from(current).join(" ")); }, - [scopes] + [scopes, parseScopesString] );packages/inspector/src/oauth/callback-handler.ts (2)
29-35: Consider adding type guard for chunk casting.The
chunk as Buffercast assumes all chunks are Buffers, which is true for HTTP requests but not enforced by the type system. While this works in practice, a defensive check could prevent subtle issues.♻️ Optional defensive check
async function readBody(req: http.IncomingMessage): Promise<string> { const chunks: Buffer[] = []; for await (const chunk of req) { - chunks.push(chunk as Buffer); + chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk as Uint8Array)); } return Buffer.concat(chunks).toString("utf-8"); }
238-252: Consider using Zod for request body validation.The JSON body is parsed with a type assertion and minimal validation. Per coding guidelines, Zod v4 should be used for schema validation. This would provide better error messages and type safety.
♻️ Suggested Zod schema
import { z } from "zod"; const OAuthConfigureBodySchema = z.object({ connectionId: z.string().optional(), config: z.object({ clientId: z.string().optional(), clientSecret: z.string().optional(), redirectUri: z.string().optional(), scopes: z.string().optional(), enableDynamicRegistration: z.boolean().optional(), clientName: z.string().optional(), }).refine( (c) => c.clientId || c.enableDynamicRegistration, { message: "Config must include clientId or enableDynamicRegistration" } ), }); // Then in handler: const parseResult = OAuthConfigureBodySchema.safeParse(JSON.parse(await readBody(req))); if (!parseResult.success) { jsonResponse(res, 400, { error: parseResult.error.message }); return true; } const body = parseResult.data;As per coding guidelines: "Always use Zod v4 for schema/validation".
packages/inspector/src/oauth/token-store.ts (2)
71-75: Potential race condition inensureDir.Using
existsSyncfollowed by asyncmkdircreates a TOCTOU (time-of-check to time-of-use) race condition. Another process could create the directory between the check and the mkdir call. Whilemkdirwithrecursive: truehandles existing directories gracefully, the sync/async mix is unnecessary.♻️ Suggested fix
private async ensureDir(): Promise<void> { - if (!existsSync(this.storePath)) { - await mkdir(this.storePath, { recursive: true, mode: 0o700 }); - } + await mkdir(this.storePath, { recursive: true, mode: 0o700 }); }The
recursive: trueoption already handles the case where the directory exists, making theexistsSynccheck redundant.
136-138: Moverenameimport to top-level.The dynamic import of
renameis unnecessary since it's a stable API fromnode:fs/promises. Moving it to the top-level import improves clarity and avoids repeated import overhead.♻️ Suggested fix
-import { readFile, writeFile, mkdir, unlink, readdir } from "node:fs/promises"; +import { readFile, writeFile, mkdir, unlink, readdir, rename } from "node:fs/promises";Then at line 136-138:
// Atomic rename - const { rename } = await import("node:fs/promises"); await rename(tmpPath, filePath);packages/inspector/src/oauth/preset-config.ts (2)
139-194: Consider using Zod for config file validation.The manual JSON parsing and type checking is verbose and doesn't leverage TypeScript's type system fully. Per coding guidelines, Zod v4 should be used for schema validation.
♻️ Suggested Zod schema
import { z } from "zod"; const PresetConfigFileSchema = z.object({ clientId: z.string().optional(), clientSecret: z.string().optional(), scopes: z.string().optional(), autoRegister: z.boolean().optional(), clientName: z.string().optional(), }).refine( (data) => data.clientId || data.autoRegister, { message: 'Config must include "clientId" or "autoRegister"' } ); // Usage: const parseResult = PresetConfigFileSchema.safeParse(JSON.parse(content)); if (!parseResult.success) { throw new Error(`Invalid OAuth config: ${parseResult.error.message}`); } const file = parseResult.data;As per coding guidelines: "Always use Zod v4 for schema/validation".
270-284: Monkey-patching approach works but consider a cleaner alternative.The method override pattern works but creates tight coupling. A cleaner approach might be to add a
nonInteractiveoption toInspectorOAuthProviderthat controls this behavior internally. However, this is acceptable for the current use case.packages/inspector/src/oauth/provider.ts (2)
310-317:waitForAuthorizationPromise may never resolve.If authorization is abandoned (user closes browser, network error, etc.), the Promise created by
waitForAuthorizationwill never resolve and the stored resolve function creates a closure that can't be garbage collected. Consider adding a timeout or cleanup mechanism.♻️ Optional timeout implementation
- waitForAuthorization(): Promise<void> { + waitForAuthorization(timeoutMs = 300000): Promise<void> { if (!this._pendingAuthUrl) { return Promise.resolve(); } return new Promise<void>((resolve, reject) => { this._pendingAuthResolve = resolve; + const timeoutId = setTimeout(() => { + this._pendingAuthResolve = null; + reject(new Error("Authorization timed out")); + }, timeoutMs); + // Store cleanup for onAuthorizationComplete + const originalResolve = resolve; + this._pendingAuthResolve = () => { + clearTimeout(timeoutId); + originalResolve(); + }; }); }
160-165: Consider importingOAuthClientInformationFullat the top level.The inline import path for the type assertion is verbose. Importing the type at the top of the file would improve readability.
♻️ Suggested change
At top of file:
+import type { OAuthClientInformationFull } from "@modelcontextprotocol/sdk/shared/auth.js";Then at line 161-163:
await this.tokenStore.save(this.serverUrl, { - clientInformation: - clientInformation as import("@modelcontextprotocol/sdk/shared/auth.js").OAuthClientInformationFull, + clientInformation: clientInformation as OAuthClientInformationFull, });
| function getStatusBadgeStyle(status: string): React.CSSProperties { | ||
| switch (status) { | ||
| case "authenticated": | ||
| return oauthPanelStyles.statusAuthenticated; | ||
| case "authenticating": | ||
| return oauthPanelStyles.statusAuthenticating; | ||
| case "error": | ||
| return oauthPanelStyles.statusError; | ||
| default: | ||
| return oauthPanelStyles.statusUnauthenticated; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix return type to resolve TypeScript error TS2322.
The pipeline failure indicates that getStatusBadgeStyle returns CSSProperties | undefined because TypeScript cannot verify that all code paths return a defined value from the styles object. While the switch has a default case, the function's return type should be explicitly annotated.
🛠️ Proposed fix
-function getStatusBadgeStyle(status: string): React.CSSProperties {
+function getStatusBadgeStyle(status: string): React.CSSProperties {
switch (status) {
case "authenticated":
- return oauthPanelStyles.statusAuthenticated;
+ return oauthPanelStyles.statusAuthenticated!;
case "authenticating":
- return oauthPanelStyles.statusAuthenticating;
+ return oauthPanelStyles.statusAuthenticating!;
case "error":
- return oauthPanelStyles.statusError;
+ return oauthPanelStyles.statusError!;
default:
- return oauthPanelStyles.statusUnauthenticated;
+ return oauthPanelStyles.statusUnauthenticated!;
}
}Alternatively, define the styles object with a more precise type or use type assertions at the call site.
🧰 Tools
🪛 GitHub Actions: PR Check
[error] 244-244: TypeScript error TS2322: Type 'CSSProperties | undefined' is not assignable to type 'CSSProperties'.
🤖 Prompt for AI Agents
In `@packages/inspector/src/dashboard/react/components/OAuthPanel.tsx` around
lines 241 - 252, The function getStatusBadgeStyle currently returns a value that
TypeScript narrows to CSSProperties | undefined causing TS2322; explicitly
annotate its return type as React.CSSProperties in the signature (function
getStatusBadgeStyle(status: string): React.CSSProperties) so the compiler knows
it always returns a defined style, and ensure the referenced style keys on
oauthPanelStyles (statusAuthenticated, statusAuthenticating, statusError,
statusUnauthenticated) are typed as React.CSSProperties in the oauthPanelStyles
declaration or its type so all returned branches satisfy the annotated return
type.
| */ | ||
|
|
||
| import { describe, it, expect, beforeEach, afterEach } from "vitest"; | ||
| import { mkdtemp, rm, readdir, readFile } from "node:fs/promises"; |
There was a problem hiding this comment.
Assert the restrictive file mode you’re describing.
The test name says it verifies restrictive permissions but it never checks the mode, so a chmod regression would still pass. Add a mode assertion (guarded on non-Windows if needed).
🛠️ Proposed fix
-import { mkdtemp, rm, readdir, readFile } from "node:fs/promises";
+import { mkdtemp, rm, readdir, readFile, stat } from "node:fs/promises";
@@
const files = await readdir(tempDir);
expect(files.length).toBeGreaterThan(0);
+ if (process.platform !== "win32") {
+ const { mode } = await stat(join(tempDir, files[0]!));
+ expect(mode & 0o777).toBe(0o600);
+ }
+
// Verify file content is valid JSON
const content = await readFile(join(tempDir, files[0]!), "utf-8");Also applies to: 195-209
🤖 Prompt for AI Agents
In `@packages/inspector/tests/oauth-token-store.test.ts` at line 6, The test
titled something like "verifies restrictive permissions" currently never checks
the file mode; add an assertion that the token file's mode is restrictive (e.g.
check via await stat(filePath) or fs.statSync(filePath) and assert (st.mode &
0o177) === 0 or an equivalent mask) to ensure group/other bits are cleared, and
guard this check behind a platform guard (if (process.platform !== 'win32') {
... }) so it skips on Windows; locate the assertion to run after the token file
is created/closed in the same test (and apply the same addition to the other
similar test around lines ~195-209) and reference mkdtemp/readFile/rm as the
surrounding helpers to find the right spot.
Code Review: OAuth MCP Server Connections (HTTP)SummaryThis is an excellent, production-ready implementation of OAuth 2.1 + PKCE support for the MCP Inspector. The code demonstrates strong adherence to security best practices, clean architecture, and comprehensive testing. The PR adds 7,997 lines (mostly well-structured code and tests) across 34 files. ✅ Strengths1. Outstanding Security Practices
2. Clean Architecture
3. Excellent Test Coverage
4. Production-Ready Features
🔍 Areas for ImprovementMinor Issues
Documentation
Performance
🛡️ Security ReviewNo Major Vulnerabilities Found ✅
Minor Security Considerations
📊 Code Quality Metrics
🎯 RecommendationsBefore Merging
Post-Merge Enhancements
📝 Final VerdictAPPROVED ✅ - This PR is ready to merge with minor suggestions. The implementation is exceptionally well-crafted, with strong security practices, comprehensive testing, and clean architecture. The three-mode support (Dashboard, CLI, Dual) is elegant and well-separated. The test coverage is thorough, covering edge cases and integration scenarios. The minor issues noted above are suggestions for improvement rather than blockers. Great work! Testing Checklist
Reviewed by: Claude Code (Automated Code Review) |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/inspector/src/oauth/provider.ts`:
- Around line 505-516: The revocation request in postRevocation uses fetch
without a timeout and can hang; update postRevocation to use an AbortController:
create an AbortController, start a timer (matching the project's standard
timeout used in wellknown-proxy.ts or a sensible millis constant) that calls
controller.abort(), pass controller.signal into fetch(endpointUrl, { method:
"POST", headers, body: params.toString(), signal }), clear the timer after fetch
completes, and handle AbortError by throwing a clear timeout-specific error
before continuing to the existing response.ok handling (use the existing
endpointUrl/headers/params names and the postRevocation function to locate and
change the code).
In `@packages/inspector/tests/oauth-integration-acceptance.test.ts`:
- Around line 700-703: Test assertion is brittle because it compares
secondState.expiresAt! to Date.now() + 3_500_000 with little slack; change the
assertion to allow a safety margin (e.g. compute const now = Date.now() and
assert expect(secondState.expiresAt! - now).toBeGreaterThan(3_500_000 - 60_000)
or similar), or assert against the firstExpiry (e.g.
expect(secondState.expiresAt!).toBeGreaterThan(firstExpiry + 3_400_000)) so the
check on provider.getOAuthState().expiresAt is lenient and avoids flakes.
- Around line 591-604: The permission assertion test for TokenStore in the
"should write files with 0o600 permissions (owner-only read/write)" case will
fail on Windows because Windows ignores Unix permission bits; update the test
declaration to skip on Windows using Vitest's conditional skip (e.g., replace
the current it(...) with it.skipIf(process.platform === "win32")(...)) so the
body (creating TokenStore, save(), reading directory, stat(), and checking
perms) only runs on non-Windows platforms.
🧹 Nitpick comments (4)
packages/inspector/src/oauth/wellknown-proxy.ts (1)
127-127: Unsafe type assertion on JSON response.The
as Tcast trusts the upstream response structure without validation. If the upstream returns malformed JSON or an unexpected schema, this could lead to runtime errors downstream.For a proxy layer this is typically acceptable (garbage-in/garbage-out), but consider adding a defensive check or documenting that callers must handle potentially malformed metadata.
packages/inspector/src/connection.ts (1)
251-260: Port parsing may fail silently for URLs without explicit port.Line 253 extracts the port from
inspectorUrlbut uses an empty string fallback fornew URL().portwhen no port is specified (e.g.,http://localhost/path). TheparseInt("", 10)returnsNaN, which then falls through to"6274"default.However, if
inspectorUrlis set to something likehttp://localhost(no port),new URL(inspectorUrl).portreturns"", soparseInt("", 10)yieldsNaN, and the ternary at line 253 uses"6274". This logic is actually correct but could be clearer:const port = this.inspectorUrl ? parseInt(new URL(this.inspectorUrl).port, 10) || 6274 : 6274;Current implementation works but the indirection through string
"6274"then parsing back to int at line 258 is slightly convoluted.packages/inspector/src/oauth/callback-handler.ts (2)
40-44: CORS headers use wildcard origin.
Access-Control-Allow-Origin: *(line 41) allows any origin to call these endpoints. For OAuth-related APIs, this is generally acceptable since:
- The callback endpoint is browser-redirect-based
- The API endpoints require connection context
- The inspector is a development tool, not production
However, if the inspector is ever exposed beyond localhost, consider restricting CORS to known origins.
239-247: JSON parse error handling could be more specific.The bare
catchat lines 244-246 treats all errors as "Invalid JSON body", which is correct forJSON.parsefailures. However, TypeScript's strict mode prefers typed catch variables. Consider:} catch (e: unknown) { jsonResponse(res, 400, { error: "Invalid JSON body" }); return true; }This is a minor style nit given the context.
| const response = await fetch(endpointUrl, { | ||
| method: "POST", | ||
| headers, | ||
| body: params.toString(), | ||
| }); | ||
|
|
||
| // RFC 7009: The server responds with HTTP 200 for both successful | ||
| // and invalid token revocations (the client shouldn't need to know). | ||
| if (!response.ok) { | ||
| const body = await response.text().catch(() => ""); | ||
| throw new Error(`Revocation endpoint returned ${response.status}: ${body}`); | ||
| } |
There was a problem hiding this comment.
Add timeout to revocation fetch to avoid hanging requests.
The postRevocation method uses fetch without an AbortController timeout. If the revocation endpoint is slow or unresponsive, this could block indefinitely. This is the same pattern addressed in wellknown-proxy.ts.
⏱️ Proposed timeout hardening
private async postRevocation(
endpointUrl: string,
token: string,
tokenTypeHint: string,
clientInfo?: OAuthClientInformationMixed
): Promise<void> {
const params = new URLSearchParams({
token,
token_type_hint: tokenTypeHint,
});
// ...headers setup...
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 10_000);
+
- const response = await fetch(endpointUrl, {
+ const response = await fetch(endpointUrl, {
method: "POST",
headers,
body: params.toString(),
+ signal: controller.signal,
});
+
+ clearTimeout(timeoutId);🤖 Prompt for AI Agents
In `@packages/inspector/src/oauth/provider.ts` around lines 505 - 516, The
revocation request in postRevocation uses fetch without a timeout and can hang;
update postRevocation to use an AbortController: create an AbortController,
start a timer (matching the project's standard timeout used in
wellknown-proxy.ts or a sensible millis constant) that calls controller.abort(),
pass controller.signal into fetch(endpointUrl, { method: "POST", headers, body:
params.toString(), signal }), clear the timer after fetch completes, and handle
AbortError by throwing a clear timeout-specific error before continuing to the
existing response.ok handling (use the existing endpointUrl/headers/params names
and the postRevocation function to locate and change the code).
| it("should write files with 0o600 permissions (owner-only read/write)", async () => { | ||
| const store = new TokenStore(tempDir); | ||
|
|
||
| await store.save("http://localhost:3000/mcp", { | ||
| tokens: { access_token: "secret-token", token_type: "bearer" } as OAuthTokens, | ||
| }); | ||
|
|
||
| const files = (await readdir(tempDir)).filter((f) => f.endsWith(".json")); | ||
| expect(files).toHaveLength(1); | ||
|
|
||
| const fileStat = await stat(join(tempDir, files[0]!)); | ||
| const perms = fileStat.mode & 0o777; | ||
| expect(perms).toBe(0o600); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are other platform-specific tests or skip patterns in the repo
rg -n "skipIf.*win32|platform.*win|skip\.if|\.skip\(|process\.platform" packages/inspector/tests/ -A 2Repository: AndurilCode/mcp-apps-kit
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Also search more broadly for Vitest skip patterns
rg -n "test\.skip|it\.skip|describe\.skip|\.skip\(" packages/inspector/tests/ -A 1 | head -50Repository: AndurilCode/mcp-apps-kit
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Verify the exact file and lines exist
if [ -f "packages/inspector/tests/oauth-integration-acceptance.test.ts" ]; then
echo "File exists. Checking lines 591-604:"
sed -n '591,604p' packages/inspector/tests/oauth-integration-acceptance.test.ts
else
echo "File not found"
fiRepository: AndurilCode/mcp-apps-kit
Length of output: 634
🏁 Script executed:
#!/bin/bash
# Check Vitest configuration and skip patterns used in the project
rg -n "import.*vitest|from.*vitest" packages/inspector/tests/ -A 1 | head -20Repository: AndurilCode/mcp-apps-kit
Length of output: 1703
🏁 Script executed:
#!/bin/bash
# Search for any process.platform usage in tests
rg -n "process\.platform" packages/inspector/tests/Repository: AndurilCode/mcp-apps-kit
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search across the entire repo for skipIf or similar patterns
rg -n "skipIf|skip\.if|test\.skip|it\.skip" --type ts --type tsx | head -20Repository: AndurilCode/mcp-apps-kit
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Check vitest config to see if there are any platform-specific settings
find . -name "vitest.config.*" -o -name "vite.config.*" | xargs cat 2>/dev/null | head -50Repository: AndurilCode/mcp-apps-kit
Length of output: 1680
🏁 Script executed:
#!/bin/bash
# Look at the full test file to understand its context better
wc -l packages/inspector/tests/oauth-integration-acceptance.test.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Check the setup.ts file to see if there are any custom skip helpers
cat packages/inspector/tests/setup.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Search for any custom test utilities or helpers
find packages/inspector/tests -name "*.ts" -not -name "*.test.ts" | head -10Repository: AndurilCode/mcp-apps-kit
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Check if there's a test utilities file that might define skipIf
rg -n "skipIf|export.*skip" packages/inspector/tests/ -B 2 -A 5Repository: AndurilCode/mcp-apps-kit
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Look for the actual Vitest version in package.json
rg -n "vitest" . --type json | grep -i versionRepository: AndurilCode/mcp-apps-kit
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check the test utilities file
cat packages/inspector/tests/test-utils.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 2322
🏁 Script executed:
#!/bin/bash
# Look for package.json in inspector package
cat packages/inspector/package.json | grep -A 5 -B 5 "vitest"Repository: AndurilCode/mcp-apps-kit
Length of output: 386
🏁 Script executed:
#!/bin/bash
# Check root package.json for vitest version
cat package.json | grep -A 2 -B 2 "vitest"Repository: AndurilCode/mcp-apps-kit
Length of output: 373
🏁 Script executed:
#!/bin/bash
# Look at the beginning of the oauth-integration test to understand its structure
head -30 packages/inspector/tests/oauth-integration-acceptance.test.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 1484
🏁 Script executed:
#!/bin/bash
# Search more broadly for any skip patterns in all test files
rg -n "\.skip\(|skip(" packages/inspector/tests/ --type tsRepository: AndurilCode/mcp-apps-kit
Length of output: 146
🏁 Script executed:
#!/bin/bash
# Look at context around the permission test and related tests
sed -n '570,620p' packages/inspector/tests/oauth-integration-acceptance.test.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 2031
🏁 Script executed:
#!/bin/bash
# Check if there are Windows-specific concerns in the codebase or if TokenStore handles platform differences
rg -n "win32|platform|windows|POSIX" packages/inspector/src/ | head -20Repository: AndurilCode/mcp-apps-kit
Length of output: 2150
🏁 Script executed:
#!/bin/bash
# Look at the TokenStore implementation to understand file permission handling
find packages/inspector/src -name "*token*" -o -name "*store*" | head -5Repository: AndurilCode/mcp-apps-kit
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Look at the TokenStore implementation
cat packages/inspector/src/oauth/token-store.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 6299
🏁 Script executed:
#!/bin/bash
# Check if there's any platform-specific logic in the save method or file operations
rg -n "chmod|mode|0o600|permissions" packages/inspector/src/oauth/Repository: AndurilCode/mcp-apps-kit
Length of output: 1486
Skip permission test on Windows platforms.
The permission check at lines 601-603 reads fileStat.mode & 0o777 to verify 0o600 permissions. On Windows, the mode parameter in writeFile() is ignored—Windows does not support Unix file permission bits. This test will fail on Windows with an unexpected permission value.
Use Vitest's skipIf modifier to skip on Windows:
Proposed change
it.skipIf(process.platform === "win32")("should write files with 0o600 permissions (owner-only read/write)", async () => {🤖 Prompt for AI Agents
In `@packages/inspector/tests/oauth-integration-acceptance.test.ts` around lines
591 - 604, The permission assertion test for TokenStore in the "should write
files with 0o600 permissions (owner-only read/write)" case will fail on Windows
because Windows ignores Unix permission bits; update the test declaration to
skip on Windows using Vitest's conditional skip (e.g., replace the current
it(...) with it.skipIf(process.platform === "win32")(...)) so the body (creating
TokenStore, save(), reading directory, stat(), and checking perms) only runs on
non-Windows platforms.
| const secondState = provider.getOAuthState(); | ||
| expect(secondState.expiresAt!).toBeGreaterThan(firstExpiry); | ||
| expect(secondState.expiresAt!).toBeGreaterThan(Date.now() + 3500_000); | ||
| }); |
There was a problem hiding this comment.
Assertion timing may be flaky.
Line 702 asserts secondState.expiresAt! > Date.now() + 3500_000, but expires_in: 3600 means expiry is ~3600 seconds (3,600,000 ms) from save time. The gap between saveTokens and the assertion should be negligible, but this leaves only ~100 seconds of margin.
Consider a more lenient assertion:
- expect(secondState.expiresAt!).toBeGreaterThan(Date.now() + 3500_000);
+ expect(secondState.expiresAt!).toBeGreaterThan(Date.now() + 3000_000); // ~50 min margin📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const secondState = provider.getOAuthState(); | |
| expect(secondState.expiresAt!).toBeGreaterThan(firstExpiry); | |
| expect(secondState.expiresAt!).toBeGreaterThan(Date.now() + 3500_000); | |
| }); | |
| const secondState = provider.getOAuthState(); | |
| expect(secondState.expiresAt!).toBeGreaterThan(firstExpiry); | |
| expect(secondState.expiresAt!).toBeGreaterThan(Date.now() + 3000_000); // ~50 min margin | |
| }); |
🤖 Prompt for AI Agents
In `@packages/inspector/tests/oauth-integration-acceptance.test.ts` around lines
700 - 703, Test assertion is brittle because it compares secondState.expiresAt!
to Date.now() + 3_500_000 with little slack; change the assertion to allow a
safety margin (e.g. compute const now = Date.now() and assert
expect(secondState.expiresAt! - now).toBeGreaterThan(3_500_000 - 60_000) or
similar), or assert against the firstExpiry (e.g.
expect(secondState.expiresAt!).toBeGreaterThan(firstExpiry + 3_400_000)) so the
check on provider.getOAuthState().expiresAt is lenient and avoids flakes.
Code Review: OAuth MCP Server ConnectionsSummaryThis PR adds comprehensive OAuth authentication support to the MCP Inspector. The implementation is very well-architected with strong type safety, comprehensive testing, and excellent documentation. Overall Assessment: ✅ Approve with Minor Suggestions Strengths
Issues Found (Medium Priority)1. Potential Race Condition (provider.ts:312-327)
2. Missing Input Length Validation (callback-handler.ts:536)
3. Unconstrained Cache TTL (wellknown-proxy.ts:70)
Low Priority Improvements
Security Audit ✅
Final Verdict✅ APPROVED This is exceptional work demonstrating deep OAuth 2.0 understanding, strong architectural design, and comprehensive testing. The suggested improvements are minor and non-blocking. The code is production-ready. Review Score: 9.5/10 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/inspector/src/connection.ts`:
- Around line 252-259: The code uses new URL(this.inspectorUrl).port which can
be an empty string (producing NaN when passed to parseInt) and thus yields an
invalid callbackPort for InspectorOAuthProvider; update the port derivation to
fall back to the default inspector port when empty (e.g., compute const portStr
= this.inspectorUrl ? (new URL(this.inspectorUrl).port || "6274") : "6274" and
then use parseInt(portStr, 10) for callbackPort) so InspectorOAuthProvider
receives a valid numeric callbackPort.
In `@packages/inspector/src/dashboard/react/hooks/useOAuth.ts`:
- Around line 176-185: The current construction of the OAuth configure body
treats an empty string clientId as falsy, enabling dynamic registration
incorrectly; update the body creation in useOAuth.ts (the OAuthConfigureParams
object where body is built) so clientId is normalized to undefined when empty
(e.g., use a trimmed value and set clientId to undefined if it's an empty
string) and derive enableDynamicRegistration from the presence of the normalized
clientId (e.g., enableDynamicRegistration = !normalizedClientId). Ensure you
reference the same symbols: connectionId, params.clientId, and the
body/OAuthConfigureParams object when making the change.
In `@packages/inspector/src/oauth/preset-config.ts`:
- Around line 130-193: Replace the manual runtime checks in loadPresetConfigFile
with a Zod v4 schema: define a schema (e.g., PresetConfigFileSchema) that models
PresetConfigFile (clientId optional string, clientSecret optional string, scopes
optional string, autoRegister optional boolean, clientName optional string) and
enforces the "clientId OR autoRegister" constraint via refine; after JSON.parse
call, run PresetConfigFileSchema.parse(parsed) to validate and get typed data,
then map the parsed fields to the OAuthClientConfig object (set redirectUri,
clientId, clientSecret, clientName, enableDynamicRegistration) and normalize
scopes (replace commas with spaces and trim) either via a z.preprocess on scopes
or after parsing; remove all the manual typeof/array/null checks and throw
behavior in favor of schema.parse errors.
In `@packages/inspector/src/oauth/provider.ts`:
- Around line 312-325: The timeout handler in waitForAuthorization leaves
pending state set, so update the timeout branch to clear the pending auth state
and surface an error: when the timeout fires, set this._pendingAuthResolve =
null and this._pendingAuthUrl = null, and mark an error (for example by setting
this.status = 'error' or calling whatever existing method records auth failures)
before rejecting with the timeout Error; this ensures the UI stops showing
"authenticating" and allows retries.
🧹 Nitpick comments (1)
packages/inspector/src/dashboard/react/hooks/useOAuth.ts (1)
106-108: Consider validating API responses with Zod.Type assertions (
as OAuthStatusResponse) bypass runtime validation. Malformed server responses could cause subtle downstream errors. Per coding guidelines, Zod v4 is preferred for schema validation.This pattern repeats at lines 194, 198, 245, and 249.
♻️ Example using Zod for response validation
import { z } from "zod"; const OAuthStatusResponseSchema = z.object({ configured: z.boolean(), connectionId: z.string().nullable(), status: z.string().optional(), expiresAt: z.number().optional(), grantedScopes: z.string().optional(), errorMessage: z.string().optional(), supportsDynamicRegistration: z.boolean().optional(), supportsRevocation: z.boolean().optional(), supportedScopes: z.array(z.string()).optional(), authorizationUrl: z.string().nullable().optional(), }); // Then in fetchStatus: const data = OAuthStatusResponseSchema.parse(await res.json());
| // Port is set later via setInspectorUrl, default to 6274 | ||
| const port = this.inspectorUrl ? new URL(this.inspectorUrl).port : "6274"; | ||
|
|
||
| authProvider = new InspectorOAuthProvider({ | ||
| serverUrl: params.url, | ||
| config: oauthConfig, | ||
| callbackPort: parseInt(port, 10), | ||
| debug: this.debug, |
There was a problem hiding this comment.
Guard against empty port when deriving the callback port.
If inspectorUrl omits the port, new URL(...).port is "", which parses to NaN and produces an invalid callback URL. Consider falling back to the default inspector port when the URL has no explicit port.
✅ Suggested fix
- const port = this.inspectorUrl ? new URL(this.inspectorUrl).port : "6274";
+ const portFromUrl = this.inspectorUrl ? new URL(this.inspectorUrl).port : "";
+ const port = portFromUrl || "6274";
authProvider = new InspectorOAuthProvider({
serverUrl: params.url,
config: oauthConfig,
- callbackPort: parseInt(port, 10),
+ callbackPort: Number.parseInt(port, 10),
debug: this.debug,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Port is set later via setInspectorUrl, default to 6274 | |
| const port = this.inspectorUrl ? new URL(this.inspectorUrl).port : "6274"; | |
| authProvider = new InspectorOAuthProvider({ | |
| serverUrl: params.url, | |
| config: oauthConfig, | |
| callbackPort: parseInt(port, 10), | |
| debug: this.debug, | |
| // Port is set later via setInspectorUrl, default to 6274 | |
| const portFromUrl = this.inspectorUrl ? new URL(this.inspectorUrl).port : ""; | |
| const port = portFromUrl || "6274"; | |
| authProvider = new InspectorOAuthProvider({ | |
| serverUrl: params.url, | |
| config: oauthConfig, | |
| callbackPort: Number.parseInt(port, 10), | |
| debug: this.debug, |
🤖 Prompt for AI Agents
In `@packages/inspector/src/connection.ts` around lines 252 - 259, The code uses
new URL(this.inspectorUrl).port which can be an empty string (producing NaN when
passed to parseInt) and thus yields an invalid callbackPort for
InspectorOAuthProvider; update the port derivation to fall back to the default
inspector port when empty (e.g., compute const portStr = this.inspectorUrl ?
(new URL(this.inspectorUrl).port || "6274") : "6274" and then use
parseInt(portStr, 10) for callbackPort) so InspectorOAuthProvider receives a
valid numeric callbackPort.
| try { | ||
| const body: OAuthConfigureParams = { | ||
| connectionId, | ||
| config: { | ||
| clientId: params.clientId ?? undefined, | ||
| clientSecret: params.clientSecret ?? undefined, | ||
| scopes: params.scopes ?? undefined, | ||
| enableDynamicRegistration: !params.clientId, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Empty string triggers unintended dynamic registration.
Line 183 uses !params.clientId which is truthy for empty strings. If a user clears the clientId field (resulting in ""), dynamic registration will be enabled unexpectedly.
🐛 Proposed fix
const body: OAuthConfigureParams = {
connectionId,
config: {
- clientId: params.clientId ?? undefined,
- clientSecret: params.clientSecret ?? undefined,
- scopes: params.scopes ?? undefined,
- enableDynamicRegistration: !params.clientId,
+ clientId: params.clientId || undefined,
+ clientSecret: params.clientSecret || undefined,
+ scopes: params.scopes || undefined,
+ enableDynamicRegistration: !params.clientId?.trim(),
},
};🤖 Prompt for AI Agents
In `@packages/inspector/src/dashboard/react/hooks/useOAuth.ts` around lines 176 -
185, The current construction of the OAuth configure body treats an empty string
clientId as falsy, enabling dynamic registration incorrectly; update the body
creation in useOAuth.ts (the OAuthConfigureParams object where body is built) so
clientId is normalized to undefined when empty (e.g., use a trimmed value and
set clientId to undefined if it's an empty string) and derive
enableDynamicRegistration from the presence of the normalized clientId (e.g.,
enableDynamicRegistration = !normalizedClientId). Ensure you reference the same
symbols: connectionId, params.clientId, and the body/OAuthConfigureParams object
when making the change.
| export async function loadPresetConfigFile(filePath: string): Promise<OAuthClientConfig> { | ||
| let content: string; | ||
| try { | ||
| content = await readFile(filePath, "utf-8"); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| throw new Error(`Failed to read OAuth config file: ${message}`); | ||
| } | ||
|
|
||
| let parsed: unknown; | ||
| try { | ||
| parsed = JSON.parse(content); | ||
| } catch { | ||
| throw new Error(`OAuth config file is not valid JSON: ${filePath}`); | ||
| } | ||
|
|
||
| if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { | ||
| throw new Error(`OAuth config file must contain a JSON object: ${filePath}`); | ||
| } | ||
|
|
||
| const file = parsed as PresetConfigFile; | ||
|
|
||
| // Validate: need at least clientId or autoRegister | ||
| if (!file.clientId && !file.autoRegister) { | ||
| throw new Error(`OAuth config file must include "clientId" or "autoRegister": ${filePath}`); | ||
| } | ||
|
|
||
| const config: OAuthClientConfig = { | ||
| redirectUri: "", // Filled by provider based on callbackPort | ||
| }; | ||
|
|
||
| if (file.clientId) { | ||
| if (typeof file.clientId !== "string") { | ||
| throw new Error(`OAuth config: "clientId" must be a string`); | ||
| } | ||
| config.clientId = file.clientId; | ||
| } | ||
|
|
||
| if (file.clientSecret) { | ||
| if (typeof file.clientSecret !== "string") { | ||
| throw new Error(`OAuth config: "clientSecret" must be a string`); | ||
| } | ||
| config.clientSecret = file.clientSecret; | ||
| } | ||
|
|
||
| if (file.scopes) { | ||
| if (typeof file.scopes !== "string") { | ||
| throw new Error(`OAuth config: "scopes" must be a string`); | ||
| } | ||
| config.scopes = file.scopes.replace(/,/g, " ").trim(); | ||
| } | ||
|
|
||
| if (file.autoRegister) { | ||
| config.enableDynamicRegistration = true; | ||
| } | ||
|
|
||
| if (file.clientName) { | ||
| if (typeof file.clientName !== "string") { | ||
| throw new Error(`OAuth config: "clientName" must be a string`); | ||
| } | ||
| config.clientName = file.clientName; | ||
| } | ||
|
|
||
| return config; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Check for Zod dependency in package.json files
find . -name "package.json" -type f | head -20 | xargs grep -l "zod"Repository: AndurilCode/mcp-apps-kit
Length of output: 272
🏁 Script executed:
# Check the actual version of Zod required
rg '"zod"' package.jsonRepository: AndurilCode/mcp-apps-kit
Length of output: 50
🏁 Script executed:
# Examine the current implementation
head -n 200 packages/inspector/src/oauth/preset-config.ts | tail -n 80Repository: AndurilCode/mcp-apps-kit
Length of output: 2443
🏁 Script executed:
# Search for existing Zod usage in the inspector package
rg "import.*zod" packages/inspector/src/ -iRepository: AndurilCode/mcp-apps-kit
Length of output: 2125
🏁 Script executed:
# Check Zod version in packages/inspector
cat packages/inspector/package.json | jq '.dependencies.zod, .devDependencies.zod'Repository: AndurilCode/mcp-apps-kit
Length of output: 81
🏁 Script executed:
# Check packages/core for Zod version
cat packages/core/package.json | jq '.dependencies.zod, .devDependencies.zod'Repository: AndurilCode/mcp-apps-kit
Length of output: 81
🏁 Script executed:
# Find PresetConfigFile type definition
rg "PresetConfigFile" packages/inspector/src/oauth/Repository: AndurilCode/mcp-apps-kit
Length of output: 798
🏁 Script executed:
# Check one example of how Zod is used in the inspector tools
head -n 50 packages/inspector/src/tools/list-tools.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 1598
🏁 Script executed:
# Get the PresetConfigFile interface definition
rg -A 15 "export interface PresetConfigFile" packages/inspector/src/oauth/preset-config.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 419
Use Zod v4 schemas for config validation.
This function manually validates JSON shape and types. The coding guidelines and codebase patterns require Zod v4 for all schema/validation in this package. All other tool files in packages/inspector/src/tools/ consistently use Zod schemas. Replace the manual type checks with a Zod schema.
✅ Suggested refactor (Zod v4)
+import { z } from "zod";
+
+const PresetConfigFileSchema = z
+ .object({
+ clientId: z.string().optional(),
+ clientSecret: z.string().optional(),
+ scopes: z.string().optional(),
+ autoRegister: z.boolean().optional(),
+ clientName: z.string().optional(),
+ })
+ .refine((data) => data.clientId || data.autoRegister, {
+ message: 'OAuth config must include "clientId" or "autoRegister".',
+ });
...
- const file = parsed as PresetConfigFile;
+ const file = PresetConfigFileSchema.parse(parsed);🤖 Prompt for AI Agents
In `@packages/inspector/src/oauth/preset-config.ts` around lines 130 - 193,
Replace the manual runtime checks in loadPresetConfigFile with a Zod v4 schema:
define a schema (e.g., PresetConfigFileSchema) that models PresetConfigFile
(clientId optional string, clientSecret optional string, scopes optional string,
autoRegister optional boolean, clientName optional string) and enforces the
"clientId OR autoRegister" constraint via refine; after JSON.parse call, run
PresetConfigFileSchema.parse(parsed) to validate and get typed data, then map
the parsed fields to the OAuthClientConfig object (set redirectUri, clientId,
clientSecret, clientName, enableDynamicRegistration) and normalize scopes
(replace commas with spaces and trim) either via a z.preprocess on scopes or
after parsing; remove all the manual typeof/array/null checks and throw behavior
in favor of schema.parse errors.
| waitForAuthorization(): Promise<void> { | ||
| if (!this._pendingAuthUrl) { | ||
| return Promise.resolve(); | ||
| } | ||
| return new Promise<void>((resolve, reject) => { | ||
| const timeout = setTimeout(() => { | ||
| this._pendingAuthResolve = null; | ||
| reject(new Error("Authorization timed out after 5 minutes")); | ||
| }, 300_000); | ||
|
|
||
| this._pendingAuthResolve = () => { | ||
| clearTimeout(timeout); | ||
| resolve(); | ||
| }; |
There was a problem hiding this comment.
Clear pending auth state on timeout to avoid stuck UI.
The timeout path rejects but leaves _pendingAuthUrl and status unchanged, which can keep the UI in an “authenticating” state and block retries. Consider clearing the pending URL and setting an error state when the timeout fires.
💡 Suggested fix
const timeout = setTimeout(() => {
this._pendingAuthResolve = null;
+ this._pendingAuthUrl = null;
+ this.setError("Authorization timed out after 5 minutes");
reject(new Error("Authorization timed out after 5 minutes"));
}, 300_000);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| waitForAuthorization(): Promise<void> { | |
| if (!this._pendingAuthUrl) { | |
| return Promise.resolve(); | |
| } | |
| return new Promise<void>((resolve, reject) => { | |
| const timeout = setTimeout(() => { | |
| this._pendingAuthResolve = null; | |
| reject(new Error("Authorization timed out after 5 minutes")); | |
| }, 300_000); | |
| this._pendingAuthResolve = () => { | |
| clearTimeout(timeout); | |
| resolve(); | |
| }; | |
| waitForAuthorization(): Promise<void> { | |
| if (!this._pendingAuthUrl) { | |
| return Promise.resolve(); | |
| } | |
| return new Promise<void>((resolve, reject) => { | |
| const timeout = setTimeout(() => { | |
| this._pendingAuthResolve = null; | |
| this._pendingAuthUrl = null; | |
| reject(new Error("Authorization timed out after 5 minutes")); | |
| }, 300_000); | |
| this._pendingAuthResolve = () => { | |
| clearTimeout(timeout); | |
| resolve(); | |
| }; |
🤖 Prompt for AI Agents
In `@packages/inspector/src/oauth/provider.ts` around lines 312 - 325, The timeout
handler in waitForAuthorization leaves pending state set, so update the timeout
branch to clear the pending auth state and surface an error: when the timeout
fires, set this._pendingAuthResolve = null and this._pendingAuthUrl = null, and
mark an error (for example by setting this.status = 'error' or calling whatever
existing method records auth failures) before rejecting with the timeout Error;
this ensures the UI stops showing "authenticating" and allows retries.
Code Review: OAuth MCP Server Connections (PR #137)Thank you for this comprehensive OAuth implementation! This is a well-architected feature with strong security practices and excellent test coverage (1,052 tests passing, 40 new OAuth integration tests). Below is a detailed review organized by category. 🎯 SummaryOverall Assessment: ✅ APPROVE with recommended improvements This PR delivers a production-quality OAuth 2.1 + PKCE implementation for the MCP inspector. The code demonstrates:
However, several critical improvements are needed before merge to align with repository standards (particularly Zod v4 validation requirements). 🔴 Critical Issues (Must Fix)1. Missing Zod Validation (HIGH SEVERITY)Repository Requirement: AGENTS.md:10 mandates Zod v4 for all validation Issue: Multiple files use raw Locations:
Example from callback-handler.ts:240: body = JSON.parse(await readBody(req)) as {
connectionId?: string;
config?: OAuthClientConfig;
};Security Risk: Malformed JSON or unexpected fields bypass validation, leading to runtime errors or potential vulnerabilities. Recommendation: Define Zod schemas in import { z } from 'zod';
export const OAuthClientConfigSchema = z.object({
clientId: z.string().optional(),
clientSecret: z.string().optional(),
redirectUri: z.string().url(),
scopes: z.string().optional(),
enableDynamicRegistration: z.boolean().optional(),
clientName: z.string().optional(),
});
export const PersistedTokenDataSchema = z.object({
serverUrl: z.string().url(),
tokens: z.object({
access_token: z.string(),
token_type: z.string(),
expires_in: z.number().positive().optional(),
refresh_token: z.string().optional(),
scope: z.string().optional(),
}).optional(),
codeVerifier: z.string().optional(),
clientInformation: z.any().optional(),
savedAt: z.number(),
expiresAt: z.number().optional(),
requestedScopes: z.string().optional(),
});Then use in handlers: // callback-handler.ts:240
const result = OAuthConfigureBodySchema.safeParse(JSON.parse(await readBody(req)));
if (!result.success) {
jsonResponse(res, 400, { error: "Invalid request body", details: result.error });
return true;
}
const { config, connectionId } = result.data;2. Missing URL Validation (MEDIUM SEVERITY)Location: export function hashServerUrl(serverUrl: string): string {
return createHash("sha256").update(serverUrl).digest("hex");
}Issue: No validation that Recommendation: export function hashServerUrl(serverUrl: string): string {
try {
new URL(serverUrl); // Validate URL format
} catch {
throw new Error(`Invalid server URL for token storage: ${serverUrl}`);
}
return createHash("sha256").update(serverUrl).digest("hex");
}3. Interface Inconsistency (MEDIUM SEVERITY)Location: Issue: const merged: PersistedTokenData = {
tokens: (data.tokens ?? existing?.tokens) as OAuthTokens, // Can be undefined!
// ...
};Recommendation: Make export interface PersistedTokenData {
serverUrl: string;
tokens?: OAuthTokens; // Allow undefined for partial saves
// ...
}🟡 High Priority Recommendations4. Rate Limiting Missing (MEDIUM SEVERITY)Location: Issue: OAuth endpoints ( Recommendation: Add per-connection request throttling: const requestCounts = new Map<string, { count: number; resetAt: number }>();
function checkRateLimit(connectionId: string, maxPerMinute: number): boolean {
const now = Date.now();
const entry = requestCounts.get(connectionId);
if (!entry || now > entry.resetAt) {
requestCounts.set(connectionId, { count: 1, resetAt: now + 60_000 });
return true;
}
if (entry.count >= maxPerMinute) return false;
entry.count++;
return true;
}
// In handlers:
if (!checkRateLimit(connectionId, 20)) {
jsonResponse(res, 429, { error: "Too many requests" });
return true;
}5. Timeout Cleanup Leak (LOW SEVERITY)Location: Issue: If Recommendation: private _pendingAuthTimeout: ReturnType<typeof setTimeout> | null = null;
waitForAuthorization(): Promise<void> {
if (!this._pendingAuthUrl) return Promise.resolve();
return new Promise<void>((resolve, reject) => {
this._pendingAuthTimeout = setTimeout(() => {
this._pendingAuthResolve = null;
this._pendingAuthTimeout = null;
reject(new Error("Authorization timed out after 5 minutes"));
}, 300_000);
this._pendingAuthResolve = () => {
if (this._pendingAuthTimeout) {
clearTimeout(this._pendingAuthTimeout);
this._pendingAuthTimeout = null;
}
resolve();
};
});
}6. Race Condition in invalidateCredentials (MEDIUM SEVERITY)Location: Issue: Delete → save pattern is non-atomic and could lose data in concurrent scenarios. Recommendation: Make the operation atomic by using a single save with explicit } else if (scope === "tokens") {
const existing = await this.tokenStore.load(this.serverUrl);
await this.tokenStore.save(this.serverUrl, {
clientInformation: existing?.clientInformation,
tokens: undefined,
codeVerifier: undefined,
expiresAt: undefined,
});
this.updateStatus("unauthenticated");
}(Requires updating 7. Token Expiry Bounds Check (LOW SEVERITY)Location: Issue: No bounds checking on Recommendation: const MAX_EXPIRES_IN = 365 * 24 * 60 * 60; // 1 year in seconds
if (tokens.expires_in !== undefined && tokens.expires_in !== null && tokens.expires_in > 0) {
const expiresIn = Math.min(tokens.expires_in, MAX_EXPIRES_IN);
this._expiresAt = Date.now() + expiresIn * 1000;
} else {
this._expiresAt = undefined;
}🟢 React/UI Improvements8. Missing Memoization (MEDIUM PRIORITY)Location: Issue: Callbacks and computed values recreated on every render, causing unnecessary re-renders. Recommendation: const selectedScopesSet = useMemo(() =>
new Set(scopes.split(/\s+/).map(s => s.trim()).filter(Boolean)),
[scopes]
);
const toggleScope = useCallback((scope: string) => {
setScopes(prev => {
const current = new Set(prev.split(/\s+/).map(s => s.trim()).filter(Boolean));
if (current.has(scope)) current.delete(scope);
else current.add(scope);
return Array.from(current).join(" ");
});
}, []); // No dependencies with functional update9. Accessibility Gaps (MEDIUM PRIORITY)Location: Issues:
Recommendations: <div
ref={popoverRef}
role="dialog"
aria-modal="false"
aria-labelledby="oauth-panel-title"
tabIndex={-1} // Allow focus
// ...
>
<span id="oauth-panel-title" style={styles.title}>OAuth Configuration</span>
{/* Status badge */}
<span role="status" aria-live="polite" style={styles.badge}>
{status}
</span>
{/* Scope chips */}
<div role="group" aria-label="Available scopes">
{supportedScopes.map(scope => (
<button aria-pressed={selectedScopesSet.has(scope)} ...>
{scope}
</button>
))}
</div>
</div>Add focus management: useEffect(() => {
if (isOpen && popoverRef.current) {
popoverRef.current.focus();
}
}, [isOpen]);10. Error State Not Cleared (LOW PRIORITY)Location: Issue: Errors persist across retries. Clear errors at the start of operations: const configure = useCallback(async (config: OAuthClientConfig) => {
setError(null); // Add this
setIsLoading(true);
try {
// ...
}
}, [/* ... */]);✅ Security Strengths (Excellent Work!)
🧪 Test Coverage AssessmentOverall Test Quality: A (93/100) Strengths:
Minor Gaps:
Recommendation: Add explicit permission check: it("should create files with restrictive permissions", async () => {
// ... existing code ...
if (process.platform !== 'win32') {
const fileStat = await stat(join(tempDir, files[0]!));
expect(fileStat.mode & 0o777).toBe(0o600);
}
});📊 Performance ConsiderationsStrengths:
Minor Concerns:
🎓 OAuth Best Practices ComplianceExcellent Implementation:
Minor Enhancement Opportunity:
📝 Priority Summary🔴 Must Fix Before Merge
🟡 Strongly Recommended
🟢 Nice to Have
🚀 ConclusionThis is exceptional work that demonstrates deep understanding of OAuth 2.1 security practices and TypeScript development. The implementation is well-architected with excellent separation of concerns across the three operational modes. After addressing the critical Zod validation requirements and minor security improvements, this PR will be production-ready. The comprehensive test suite (particularly the 10 acceptance criteria integration tests) provides high confidence in the implementation's correctness and robustness. Estimated effort for critical fixes: 4-6 hours Great job on this feature! 🎉 |
Code Review - PR #137Comprehensive OAuth implementation with excellent architecture. Main issues to address:
Verdict: High-quality work. REQUEST CHANGES for Zod + timeouts, then APPROVE. See detailed review in my comment above or full analysis available on request. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/inspector/src/oauth/provider.ts`:
- Around line 262-269: The client scope branch incorrectly attempts to clear
clientInformation by passing clientInformation: undefined to tokenStore.save,
but TokenStore.save uses nullish coalescing so undefined preserves the old
value; change the logic in the block handling scope === "client" (the
tokenStore.load / tokenStore.save flow) to first delete the stored entry via
tokenStore.delete(this.serverUrl) and then, if the previously-loaded
existing.tokens exist, call tokenStore.save(this.serverUrl, { tokens:
existing.tokens }) so clientInformation is truly removed; use the same pattern
as the tokens-scope fix (referencing tokenStore.load, tokenStore.delete, and
tokenStore.save).
🧹 Nitpick comments (1)
packages/inspector/src/oauth/provider.ts (1)
392-394: Include error details in scope discovery failure log.The empty catch block discards the error. Including the error message would aid debugging when scope discovery fails.
♻️ Suggested improvement
- } catch { - this.log("Scope discovery failed — auth server metadata unavailable"); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + this.log(`Scope discovery failed: ${message}`); }
| } else if (scope === "client") { | ||
| const existing = await this.tokenStore.load(this.serverUrl); | ||
| if (existing) { | ||
| await this.tokenStore.save(this.serverUrl, { | ||
| tokens: existing.tokens, | ||
| clientInformation: undefined, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/inspector/src/oauth/token-store.ts | head -100Repository: AndurilCode/mcp-apps-kit
Length of output: 3684
🏁 Script executed:
cat -n packages/inspector/src/oauth/token-store.ts | sed -n '100,200p'Repository: AndurilCode/mcp-apps-kit
Length of output: 3897
🏁 Script executed:
cat -n packages/inspector/src/oauth/provider.ts | sed -n '242,280p'Repository: AndurilCode/mcp-apps-kit
Length of output: 1864
Fix clientInformation clearing in the "client" scope.
The "client" scope at lines 262-269 uses the same broken pattern that was fixed for the "tokens" scope. Because TokenStore.save() line 119 uses nullish coalescing (clientInformation: data.clientInformation ?? existing?.clientInformation), passing clientInformation: undefined will preserve the existing value instead of clearing it.
Follow the "tokens" scope pattern: delete the stored data first, then re-save with only the tokens:
Suggested fix
} else if (scope === "client") {
const existing = await this.tokenStore.load(this.serverUrl);
await this.tokenStore.delete(this.serverUrl);
if (existing?.tokens) {
await this.tokenStore.save(this.serverUrl, {
tokens: existing.tokens,
});
}
}
🤖 Prompt for AI Agents
In `@packages/inspector/src/oauth/provider.ts` around lines 262 - 269, The client
scope branch incorrectly attempts to clear clientInformation by passing
clientInformation: undefined to tokenStore.save, but TokenStore.save uses
nullish coalescing so undefined preserves the old value; change the logic in the
block handling scope === "client" (the tokenStore.load / tokenStore.save flow)
to first delete the stored entry via tokenStore.delete(this.serverUrl) and then,
if the previously-loaded existing.tokens exist, call
tokenStore.save(this.serverUrl, { tokens: existing.tokens }) so
clientInformation is truly removed; use the same pattern as the tokens-scope fix
(referencing tokenStore.load, tokenStore.delete, and tokenStore.save).
Detailed Code Review FindingsCritical Issues (Must Fix Before Merge)1. Zod v4 Schema Validation Missing
|
📋 Action Items ChecklistBefore merging, please address these items: Required (Blocking)
Recommended (Non-blocking)
Code Quality Improvements (Optional)
Review Status: ⏸️ Requesting Changes Once the Required items are addressed, I recommend APPROVAL. The codebase quality is excellent and the architecture is sound. Feel free to push fixes in follow-up commits. I'll re-review once ready! |
1 participant
Summary
Support OAuth-authenticated MCP server connections in the inspector with three modes:
Mode A — Dashboard Flow: User connects to OAuth-protected MCP server, configures client credentials or uses Dynamic Client Registration (RFC 7591), browser opens for authorization, PKCE exchange completes, Bearer token established.
Mode B — CLI/Agent Preset:
--oauth-client-id,--oauth-client-secret,--oauth-scopes,--oauth-config,--oauth-auto-registerflags for non-interactive auth with cached token reuse.Mode C — Dual Mode Mirroring: Proxy port serves upstream
.well-known/oauth-protected-resource(RFC 9728) and.well-known/oauth-authorization-server(RFC 8414) with resource URL rewriting.Key Features
InspectorOAuthProviderimplementing MCP SDK'sOAuthClientProviderinterface~/.config/mcp-inspector/tokens/) with SHA-256 URL hashing, atomic writes, 0o600 permissionsChanges
packages/inspector/src/oauth/(provider, token-store, callback-handler, preset-config, wellknown-proxy, types)Tests
Pipeline
Vega (plan) → Rigel (build ×3) → Altair (review ×3) → Canopus (gap ×2) → Polaris (test) ✅