Add SDK MCP OAuth host token handlers#1669
Conversation
github-actions
Bot
added
the
dependencies
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1669 · sonnet46 2.1M
| .field( | ||
| "mcp_auth_handler", | ||
| &self.mcp_auth_handler.as_ref().map(|_| "<set>"), | ||
| ) |
There was a problem hiding this comment.
Duplicate Debug field — Rust SessionConfig
The mcp_auth_handler field is emitted twice in the Debug implementation (lines 1462–1465 and 1466–1469 are identical). The second block should be removed; otherwise debug/trace output will contain duplicate keys and DebugStruct::finish() may behave unexpectedly with some formatters.
// Remove this duplicate block (lines 1466-1469):
.field(
"mcp_auth_handler",
&self.mcp_auth_handler.as_ref().map(|_| "<set>"),
)
| catch (Exception) | ||
| { | ||
| try | ||
| { | ||
| await Rpc.Mcp.Oauth.HandlePendingRequestAsync(requestId, new McpOauthPendingRequestResponseCancelled()); | ||
| } | ||
| catch (Exception rpcEx) when (rpcEx is IOException or ObjectDisposedException) | ||
| { | ||
| // Connection lost or RPC error — nothing we can do. | ||
| } | ||
| } |
| export interface McpAuthStaticClientConfig { | ||
| /** OAuth client ID for the server. */ | ||
| clientId: string; | ||
| /** Optional non-default OAuth grant type. */ | ||
| grantType?: "client_credentials"; | ||
| /** Whether this is a public OAuth client. */ | ||
| publicClient?: boolean; |
There was a problem hiding this comment.
what about the clientSecret? A lot of AS's use client secrets even for public clients. (VS Code allows specifying a client secret, stored in secret storage)
| export interface McpAuthToken { | ||
| /** Access token acquired by the SDK host. */ | ||
| accessToken: string; | ||
| /** OAuth token type. Defaults to Bearer when omitted. */ | ||
| tokenType?: string; | ||
| /** Refresh token supplied by the host, if available. */ | ||
| refreshToken?: string; | ||
| /** Token lifetime in seconds, if known. */ | ||
| expiresIn?: number; | ||
| } |
There was a problem hiding this comment.
what about an idToken? idTokens are useful as they contain claims that are good to display like usernames/email/etc.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
roji
force-pushed
the
roji/roji-mcp-auth-sdk
branch
from
59694fd to
6d3d655
Compare
| session.setOpenCanvases(response.openCanvases()); | ||
|
|
||
| return updateSessionOptionsForMode(session, config.getSkipCustomInstructions().orElse(null), | ||
| return interest.thenCompose(ignored -> updateSessionOptionsForMode(session, |
| return connection.rpc.invoke("session.resume", request, ResumeSessionResponse.class) | ||
| return interest | ||
| .thenCompose( | ||
| ignored -> connection.rpc.invoke("session.resume", request, ResumeSessionResponse.class)) |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1669 · sonnet46 1.8M
| &self.mcp_auth_handler.as_ref().map(|_| "<set>"), | ||
| ) | ||
| .field( | ||
| "mcp_auth_handler", |
There was a problem hiding this comment.
🐛 Duplicate Debug field — mcp_auth_handler is emitted twice here. The second .field("mcp_auth_handler", ...) block (lines 1800–1803) is an exact copy of the one immediately above it (lines 1796–1799). When {:?} is printed, the field will appear twice in the output. One of these blocks should be removed.
Add MCP OAuth host-token E2E coverage for Python, Go, .NET, Java, and Rust using the shared OAuth-protected MCP test server. Align create-session MCP OAuth interest registration with the packaged runtime by registering interest after session.create succeeds while preserving pre-registration of local session handlers for create-time callbacks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| private record OAuthMcpServer(Process process, String url) implements AutoCloseable { | ||
| static OAuthMcpServer start(Path repoRoot) throws Exception { | ||
| var script = repoRoot.resolve("test").resolve("harness").resolve("test-mcp-oauth-server.mjs"); | ||
| var processBuilder = new ProcessBuilder("node", script.toString()); |
Cross-SDK Consistency ReviewOverall this PR does an excellent job of implementing MCP OAuth host-token callbacks uniformly across all six SDKs (Node.js, Python, Go, .NET, Java, Rust). The handler registration pattern, One inconsistency found
All other SDKs expose the
This means a .NET handler cannot identify or correlate the specific OAuth request it's responding to (e.g. for logging or debugging). Notes on RustRust's Everything else looks consistent ✅
|
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1669 · sonnet46 3.1M
| public string? ResourceMetadata { get; set; } | ||
|
|
||
| /// <summary>Static OAuth client configuration, if the server specifies one.</summary> | ||
| public McpOauthRequiredStaticClientConfig? StaticClientConfig { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency suggestion: McpAuthContext is missing a RequestId property.
Every other SDK exposes the request ID to the MCP auth handler:
- Node.js:
McpAuthRequest.requestId - Python:
McpAuthRequest["requestId"](TypedDict key) - Go:
MCPAuthRequest.RequestID - Java:
McpAuthRequest.requestId()(record component) - Rust:
request_id: RequestIdas a separatehandle()parameter
In .NET, a handler receiving an MCP OAuth request has no way to identify or log which specific request it's processing. Consider adding:
/// <summary>Unique request identifier for this MCP OAuth request.</summary>
public string RequestId { get; set; } = string.Empty;(The existing .NET PermissionInvocation also omits request ID, so there is a consistent internal precedent — but for the new McpAuthContext type this cross-SDK gap is worth revisiting.)
| public static async Task<OAuthMcpServer> StartAsync(string expectedToken) | ||
| { | ||
| var repoRoot = FindRepoRoot(); | ||
| var script = Path.Combine(repoRoot, "test", "harness", "test-mcp-oauth-server.mjs"); |
| var dir = new DirectoryInfo(AppContext.BaseDirectory); | ||
| while (dir != null) | ||
| { | ||
| var candidate = Path.Combine(dir.FullName, "test", "harness", "test-mcp-oauth-server.mjs"); |
3 participants
Summary
session.mcp.oauth.handlePendingRequestRPC path and cancel on denied/empty/throwing handlers according to existing callback conventions.mcp.oauth_required.data.resourceMetadatathrough public MCP auth request handler models across SDK languages.wwwAuthenticateParamswhen runtimes omit them.mcp.oauth_requiredinterest only when the high-level MCP auth callback is configured.Notes
mainbrought in@github/copilot1.0.64-3 and generated protocol surfaces forsession.mcp.oauth.handlePendingRequestplusmcp.oauth_required.data.resourceMetadata.main; the temporary sibling-runtime override has been removed from the net PR diff.session.eventLog.registerInterestbeforesession.createfor client-generated session IDs, so SDKs now keep local session handler pre-registration for create-time callbacks but register MCP OAuth interest immediately aftersession.createsucceeds.Validation
cd nodejs && npm cicd test/harness && npm cicd nodejs && npm run typecheck && npx vitest run test/e2e/mcp_oauth.e2e.test.tscd go && go test ./...cd python && uv run --extra dev pytest ../python/test_client.py -k mcp_auth && uv run --extra dev pytest ../python/e2e/test_mcp_oauth_e2e.pycd dotnet && DOTNET_ROOT="$HOME/.dotnet" PATH="$HOME/.dotnet:$PATH" dotnet test test/GitHub.Copilot.SDK.Test.csproj --filter "FullyQualifiedName~McpOAuthE2ETests|FullyQualifiedName~ClientSessionLifetimeTests"cd java && mvn verifycd java && mvn test -Dtest=McpOAuthE2ETest,McpAuthInterestRegistrationTest && mvn spotless:checkcd rust && PATH="$HOME/.cargo/bin:/opt/homebrew/opt/rust/bin:$PATH" cargo test --features test-support --test session_test mcpcd rust && PATH="$HOME/.cargo/bin:/opt/homebrew/opt/rust/bin:$PATH" cargo test --features test-support --test e2e should_satisfy_mcp_oauth_using_host_provided_tokencd rust && PATH="$HOME/.cargo/bin:/opt/homebrew/opt/rust/bin:$PATH" cargo +nightly-2026-04-14 fmt --checkcd rust && PATH="$HOME/.cargo/bin:/opt/homebrew/opt/rust/bin:$PATH" cargo clippy --all-features --all-targets -- -D warnings