feat(mcp-oauth): add full OAuth 2.1 authentication for MCP servers (#1169)

* feat(mcp-oauth): add oauth field to ClaudeCodeMcpServer schema Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> * feat(mcp-oauth): add RFC 7591 Dynamic Client Registration * feat(mcp-oauth): add RFC 9728 PRM + RFC 8414 AS discovery * feat(mcp-oauth): add secure token storage with {host}/{resource} key format * feat(mcp-oauth): add dynamic port OAuth callback server * feat(mcp-oauth): add RFC 8707 Resource Indicators * feat(mcp-oauth): implement full-spec McpOAuthProvider * feat(mcp-oauth): add step-up authorization handler * feat(mcp-oauth): integrate authProvider into SkillMcpManager * feat(doctor): add MCP OAuth token status check * feat(cli): add mcp oauth subcommand structure * feat(cli): implement mcp oauth login command * fix(mcp-oauth): address cubic review — security, correctness, and test issues - Remove @ts-nocheck from provider.ts, storage.ts, provider.test.ts - Fix server resource leak on missing code/state (close + reject) - Fix command injection in openBrowser (spawn array args, cross-platform) - Mock McpOAuthProvider in login.test.ts for deterministic CI - Recreate auth provider with merged scopes in step-up flow - Add listAllTokens() for global status listing - Fix logout to accept --server-url for correct token deletion - Support both quoted and unquoted WWW-Authenticate params (RFC 2617) - Save/restore OPENCODE_CONFIG_DIR in storage.test.ts - Fix index.test.ts: vitest → bun:test * fix(mcp-oauth): use explorer instead of cmd /c start on Windows to prevent shell injection * fix(mcp-oauth): address remaining cubic review issues - Add 5-minute timeout to provider callback server to prevent indefinite hangs - Persist client registration from token storage across process restarts - Require --server-url for logout to match token storage key format - Use listTokensByHost for server-specific status lookups - Fix callback-server test to handle promise rejection ordering - Fix provider test port expectations (8912 → 19877) - Fix cli-guide.md duplicate Section 7 numbering - Fix manager test for login-on-missing-tokens behavior * fix(mcp-oauth): address final review issues - P1: Redact token values in status.ts output to prevent credential leakage - P2: Read OAuth error response body before throwing in token exchange - Test: Fix mcp-oauth doctor test to use epoch seconds (not milliseconds) --------- Co-authored-by: justsisyphus <justsisyphus@users.noreply.github.com> Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
2026-01-29 19:48:36 +09:00
parent a94fbadd57
commit dcda8769cc
37 changed files with 3221 additions and 27 deletions
@@ -3,8 +3,6 @@ import { SkillMcpManager } from "./manager"
 import type { SkillMcpClientInfo, SkillMcpServerContext } from "./types"
 import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"

-
-
 // Mock the MCP SDK transports to avoid network calls
 const mockHttpConnect = mock(() => Promise.reject(new Error("Mocked HTTP connection failure")))
 const mockHttpClose = mock(() => Promise.resolve())
@@ -24,6 +22,21 @@ mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({
  },
 }))

+const mockTokens = mock(() => null as { accessToken: string; refreshToken?: string; expiresAt?: number } | null)
+const mockLogin = mock(() => Promise.resolve({ accessToken: "new-token" }))
+
+mock.module("../mcp-oauth/provider", () => ({
+  McpOAuthProvider: class MockMcpOAuthProvider {
+    constructor(public options: { serverUrl: string; clientId?: string; scopes?: string[] }) {}
+    tokens() {
+      return mockTokens()
+    }
+    async login() {
+      return mockLogin()
+    }
+  },
+}))
+



@@ -518,7 +531,6 @@ describe("SkillMcpManager", () => {
        skillName: "retry-skill",
      }

-      // Mock client that fails first time with "Not connected", then succeeds
      let callCount = 0
      const mockClient = {
        callTool: mock(async () => {
@@ -531,7 +543,6 @@ describe("SkillMcpManager", () => {
        close: mock(() => Promise.resolve()),
      }

-      // Spy on getOrCreateClientWithRetry to inject mock client
      const getOrCreateSpy = spyOn(manager as any, "getOrCreateClientWithRetry")
      getOrCreateSpy.mockResolvedValue(mockClient)

@@ -539,9 +550,9 @@ describe("SkillMcpManager", () => {
      const result = await manager.callTool(info, context, "test-tool", {})

      // #then
-      expect(callCount).toBe(2) // First call fails, second succeeds
+      expect(callCount).toBe(2)
      expect(result).toEqual([{ type: "text", text: "success" }])
-      expect(getOrCreateSpy).toHaveBeenCalledTimes(2) // Called twice due to retry
+      expect(getOrCreateSpy).toHaveBeenCalledTimes(2)
    })

    it("should fail after 3 retry attempts", async () => {
@@ -558,7 +569,6 @@ describe("SkillMcpManager", () => {
        skillName: "fail-skill",
      }

-      // Mock client that always fails with "Not connected"
      const mockClient = {
        callTool: mock(async () => {
          throw new Error("Not connected")
@@ -573,7 +583,7 @@ describe("SkillMcpManager", () => {
      await expect(manager.callTool(info, context, "test-tool", {})).rejects.toThrow(
        /Failed after 3 reconnection attempts/
      )
-      expect(getOrCreateSpy).toHaveBeenCalledTimes(3) // Initial + 2 retries
+      expect(getOrCreateSpy).toHaveBeenCalledTimes(3)
    })

    it("should not retry on non-connection errors", async () => {
@@ -590,7 +600,6 @@ describe("SkillMcpManager", () => {
        skillName: "error-skill",
      }

-      // Mock client that fails with non-connection error
      const mockClient = {
        callTool: mock(async () => {
          throw new Error("Tool not found")
@@ -605,7 +614,194 @@ describe("SkillMcpManager", () => {
      await expect(manager.callTool(info, context, "test-tool", {})).rejects.toThrow(
        "Tool not found"
      )
-      expect(getOrCreateSpy).toHaveBeenCalledTimes(1) // No retry
+      expect(getOrCreateSpy).toHaveBeenCalledTimes(1)
+    })
+  })
+
+  describe("OAuth integration", () => {
+    beforeEach(() => {
+      mockTokens.mockClear()
+      mockLogin.mockClear()
+    })
+
+    it("injects Authorization header when oauth config has stored tokens", async () => {
+      // #given
+      const info: SkillMcpClientInfo = {
+        serverName: "oauth-server",
+        skillName: "oauth-skill",
+        sessionID: "session-oauth-1",
+      }
+      const config: ClaudeCodeMcpServer = {
+        url: "https://mcp.example.com/mcp",
+        oauth: {
+          clientId: "my-client",
+          scopes: ["read", "write"],
+        },
+      }
+      mockTokens.mockReturnValue({ accessToken: "stored-access-token" })
+
+      // #when
+      try {
+        await manager.getOrCreateClient(info, config)
+      } catch { /* connection fails in test */ }
+
+      // #then
+      const headers = lastTransportInstance.options?.requestInit?.headers as Record<string, string> | undefined
+      expect(headers?.Authorization).toBe("Bearer stored-access-token")
+    })
+
+    it("does not inject Authorization header when no stored tokens exist and login fails", async () => {
+      // #given
+      const info: SkillMcpClientInfo = {
+        serverName: "oauth-no-token",
+        skillName: "oauth-skill",
+        sessionID: "session-oauth-2",
+      }
+      const config: ClaudeCodeMcpServer = {
+        url: "https://mcp.example.com/mcp",
+        oauth: {
+          clientId: "my-client",
+        },
+      }
+      mockTokens.mockReturnValue(null)
+      mockLogin.mockRejectedValue(new Error("Login failed"))
+
+      // #when
+      try {
+        await manager.getOrCreateClient(info, config)
+      } catch { /* connection fails in test */ }
+
+      // #then
+      const headers = lastTransportInstance.options?.requestInit?.headers as Record<string, string> | undefined
+      expect(headers?.Authorization).toBeUndefined()
+    })
+
+    it("preserves existing static headers alongside OAuth token", async () => {
+      // #given
+      const info: SkillMcpClientInfo = {
+        serverName: "oauth-with-headers",
+        skillName: "oauth-skill",
+        sessionID: "session-oauth-3",
+      }
+      const config: ClaudeCodeMcpServer = {
+        url: "https://mcp.example.com/mcp",
+        headers: {
+          "X-Custom": "custom-value",
+        },
+        oauth: {
+          clientId: "my-client",
+        },
+      }
+      mockTokens.mockReturnValue({ accessToken: "oauth-token" })
+
+      // #when
+      try {
+        await manager.getOrCreateClient(info, config)
+      } catch { /* connection fails in test */ }
+
+      // #then
+      const headers = lastTransportInstance.options?.requestInit?.headers as Record<string, string> | undefined
+      expect(headers?.["X-Custom"]).toBe("custom-value")
+      expect(headers?.Authorization).toBe("Bearer oauth-token")
+    })
+
+    it("does not create auth provider when oauth config is absent", async () => {
+      // #given
+      const info: SkillMcpClientInfo = {
+        serverName: "no-oauth-server",
+        skillName: "test-skill",
+        sessionID: "session-no-oauth",
+      }
+      const config: ClaudeCodeMcpServer = {
+        url: "https://mcp.example.com/mcp",
+        headers: {
+          Authorization: "Bearer static-token",
+        },
+      }
+
+      // #when
+      try {
+        await manager.getOrCreateClient(info, config)
+      } catch { /* connection fails in test */ }
+
+      // #then
+      const headers = lastTransportInstance.options?.requestInit?.headers as Record<string, string> | undefined
+      expect(headers?.Authorization).toBe("Bearer static-token")
+      expect(mockTokens).not.toHaveBeenCalled()
+    })
+
+    it("handles step-up auth by triggering re-login on 403 with scope", async () => {
+      // #given
+      const info: SkillMcpClientInfo = {
+        serverName: "stepup-server",
+        skillName: "stepup-skill",
+        sessionID: "session-stepup-1",
+      }
+      const config: ClaudeCodeMcpServer = {
+        url: "https://mcp.example.com/mcp",
+        oauth: {
+          clientId: "my-client",
+          scopes: ["read"],
+        },
+      }
+      const context: SkillMcpServerContext = {
+        config,
+        skillName: "stepup-skill",
+      }
+
+      mockTokens.mockReturnValue({ accessToken: "initial-token" })
+      mockLogin.mockResolvedValue({ accessToken: "upgraded-token" })
+
+      let callCount = 0
+      const mockClient = {
+        callTool: mock(async () => {
+          callCount++
+          if (callCount === 1) {
+            throw new Error('403 WWW-Authenticate: Bearer scope="admin write"')
+          }
+          return { content: [{ type: "text", text: "success" }] }
+        }),
+        close: mock(() => Promise.resolve()),
+      }
+
+      const getOrCreateSpy = spyOn(manager as any, "getOrCreateClientWithRetry")
+      getOrCreateSpy.mockResolvedValue(mockClient)
+
+      // #when
+      const result = await manager.callTool(info, context, "test-tool", {})
+
+      // #then
+      expect(result).toEqual([{ type: "text", text: "success" }])
+      expect(mockLogin).toHaveBeenCalled()
+    })
+
+    it("does not attempt step-up when oauth config is absent", async () => {
+      // #given
+      const info: SkillMcpClientInfo = {
+        serverName: "no-stepup-server",
+        skillName: "no-stepup-skill",
+        sessionID: "session-no-stepup",
+      }
+      const context: SkillMcpServerContext = {
+        config: {
+          url: "https://mcp.example.com/mcp",
+        },
+        skillName: "no-stepup-skill",
+      }
+
+      const mockClient = {
+        callTool: mock(async () => {
+          throw new Error('403 WWW-Authenticate: Bearer scope="admin"')
+        }),
+        close: mock(() => Promise.resolve()),
+      }
+
+      const getOrCreateSpy = spyOn(manager as any, "getOrCreateClientWithRetry")
+      getOrCreateSpy.mockResolvedValue(mockClient)
+
+      // #when / #then
+      await expect(manager.callTool(info, context, "test-tool", {})).rejects.toThrow(/403/)
+      expect(mockLogin).not.toHaveBeenCalled()
    })
  })
 })
@@ -4,6 +4,8 @@ import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/
 import type { Tool, Resource, Prompt } from "@modelcontextprotocol/sdk/types.js"
 import type { ClaudeCodeMcpServer } from "../claude-code-mcp-loader/types"
 import { expandEnvVarsInObject } from "../claude-code-mcp-loader/env-expander"
+import { McpOAuthProvider } from "../mcp-oauth/provider"
+import { isStepUpRequired, mergeScopes } from "../mcp-oauth/step-up"
 import { createCleanMcpEnvironment } from "./env-cleaner"
 import type { SkillMcpClientInfo, SkillMcpServerContext } from "./types"

@@ -60,6 +62,7 @@ function getConnectionType(config: ClaudeCodeMcpServer): ConnectionType | null {
 export class SkillMcpManager {
  private clients: Map<string, ManagedClient> = new Map()
  private pendingConnections: Map<string, Promise<Client>> = new Map()
+  private authProviders: Map<string, McpOAuthProvider> = new Map()
  private cleanupRegistered = false
  private cleanupInterval: ReturnType<typeof setInterval> | null = null
  private readonly IDLE_TIMEOUT = 5 * 60 * 1000
@@ -68,6 +71,28 @@ export class SkillMcpManager {
    return `${info.sessionID}:${info.skillName}:${info.serverName}`
  }

+  /**
+   * Get or create an McpOAuthProvider for a given server URL + oauth config.
+   * Providers are cached by server URL to reuse tokens across reconnections.
+   */
+  private getOrCreateAuthProvider(
+    serverUrl: string,
+    oauth: NonNullable<ClaudeCodeMcpServer["oauth"]>
+  ): McpOAuthProvider {
+    const existing = this.authProviders.get(serverUrl)
+    if (existing) {
+      return existing
+    }
+
+    const provider = new McpOAuthProvider({
+      serverUrl,
+      clientId: oauth.clientId,
+      scopes: oauth.scopes,
+    })
+    this.authProviders.set(serverUrl, provider)
+    return provider
+  }
+
  private registerProcessCleanup(): void {
    if (this.cleanupRegistered) return
    this.cleanupRegistered = true
@@ -204,7 +229,30 @@ export class SkillMcpManager {
    // Build request init with headers if provided
    const requestInit: RequestInit = {}
    if (config.headers && Object.keys(config.headers).length > 0) {
-      requestInit.headers = config.headers
+      requestInit.headers = { ...config.headers }
+    }
+
+    let authProvider: McpOAuthProvider | undefined
+    if (config.oauth) {
+      authProvider = this.getOrCreateAuthProvider(config.url, config.oauth)
+      let tokenData = authProvider.tokens()
+
+      const isExpired = tokenData?.expiresAt != null && tokenData.expiresAt < Math.floor(Date.now() / 1000)
+      if (!tokenData || isExpired) {
+        try {
+          tokenData = await authProvider.login()
+        } catch {
+          // Login failed — proceed without auth header
+        }
+      }
+
+      if (tokenData) {
+        const existingHeaders = (requestInit.headers ?? {}) as Record<string, string>
+        requestInit.headers = {
+          ...existingHeaders,
+          Authorization: `Bearer ${tokenData.accessToken}`,
+        }
+      }
    }

    const transport = new StreamableHTTPClientTransport(url, {
@@ -460,6 +508,12 @@ export class SkillMcpManager {
        lastError = error instanceof Error ? error : new Error(String(error))
        const errorMessage = lastError.message.toLowerCase()

+        const stepUpHandled = await this.handleStepUpIfNeeded(lastError, config)
+        if (stepUpHandled) {
+          await this.forceReconnect(info)
+          continue
+        }
+
        if (!errorMessage.includes("not connected")) {
          throw lastError
        }
@@ -470,23 +524,66 @@ export class SkillMcpManager {
          )
        }

-        const key = this.getClientKey(info)
-        const existing = this.clients.get(key)
-        if (existing) {
-          this.clients.delete(key)
-          try {
-            await existing.client.close()
-          } catch { /* process may already be terminated */ }
-          try {
-            await existing.transport.close()
-          } catch { /* transport may already be terminated */ }
-        }
+        await this.forceReconnect(info)
      }
    }

    throw lastError || new Error("Operation failed with unknown error")
  }

+  private async handleStepUpIfNeeded(
+    error: Error,
+    config: ClaudeCodeMcpServer
+  ): Promise<boolean> {
+    if (!config.oauth || !config.url) {
+      return false
+    }
+
+    const statusMatch = /\b403\b/.exec(error.message)
+    if (!statusMatch) {
+      return false
+    }
+
+    const headers: Record<string, string> = {}
+    const wwwAuthMatch = /WWW-Authenticate:\s*(.+)/i.exec(error.message)
+    if (wwwAuthMatch?.[1]) {
+      headers["www-authenticate"] = wwwAuthMatch[1]
+    }
+
+    const stepUp = isStepUpRequired(403, headers)
+    if (!stepUp) {
+      return false
+    }
+
+    const currentScopes = config.oauth.scopes ?? []
+    const merged = mergeScopes(currentScopes, stepUp.requiredScopes)
+    config.oauth.scopes = merged
+
+    this.authProviders.delete(config.url)
+    const provider = this.getOrCreateAuthProvider(config.url, config.oauth)
+
+    try {
+      await provider.login()
+      return true
+    } catch {
+      return false
+    }
+  }
+
+  private async forceReconnect(info: SkillMcpClientInfo): Promise<void> {
+    const key = this.getClientKey(info)
+    const existing = this.clients.get(key)
+    if (existing) {
+      this.clients.delete(key)
+      try {
+        await existing.client.close()
+      } catch { /* process may already be terminated */ }
+      try {
+        await existing.transport.close()
+      } catch { /* transport may already be terminated */ }
+    }
+  }
+
  private async getOrCreateClientWithRetry(
    info: SkillMcpClientInfo,
    config: ClaudeCodeMcpServer