🔑 fix: Honor User-Provided MCP API Key Instead of Forcing OAuth (#13954)

* 🔑 fix: Honor User-Provided MCP API Key Instead of Forcing OAuth

OAuth auto-detection probes the server without credentials and treats a
`WWW-Authenticate: Bearer` 401 as an OAuth requirement. A static bearer
API-key server answers an unauthenticated probe with the same challenge,
so servers configured with "API Key / each user provides their own / Bearer"
were misclassified as `requiresOAuth: true` and connected via the OAuth path,
ignoring the user's saved key (status stuck yellow, tool calls demand OAuth).

The API-key exemption in detection was scoped to `source === 'admin'` only.
Broaden it to any `apiKey` config in both detection sites (inspector startup
detection and runtime placeholder-URL detection), since API-key and OAuth auth
are mutually exclusive in the schema.

* 🔒 fix: Skip inspection probe for user API keys; honor explicit OAuth

Addresses two Codex findings on the API-key OAuth-detection fix:

- Skip the capability probe during inspection when apiKey.source is 'user'.
  The user's key is supplied per-user at connect time, so an unauthenticated
  probe at create/update would 401 against a bearer server and fail the save
  (servers are inspected on the raw, pre-transform config with no auth header).
  Same treatment already applied to customUserVars/obo/OAuth servers.
- Only short-circuit detection to non-OAuth when no explicit 'oauth' block is
  configured, so an explicit OAuth config takes precedence if both are set.
  Applied to both detection sites for consistency.
This commit is contained in:
Danny Avila 2026-06-25 14:10:04 -04:00 committed by GitHub
parent 03ecac8ac1
commit e26ce4713a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 44 additions and 5 deletions

View file

@ -643,7 +643,7 @@ export abstract class UserConnectionManager {
}): Promise<t.ParsedServerConfig> {
if (
config.requiresOAuth != null ||
config.apiKey?.source === 'admin' ||
(config.apiKey != null && config.oauth == null) ||
!hasRuntimeUrlPlaceholders(config)
) {
return config;

View file

@ -76,6 +76,9 @@ export class MCPServerInspector {
this.config.startup !== false &&
!this.config.requiresOAuth &&
!hasCustomUserVars(this.config) &&
// user-provided API key is supplied per-user at connect time; an unauthenticated
// probe here would 401 against a bearer server and fail inspection
this.config.apiKey?.source !== 'user' &&
!hasRuntimeContextPlaceholders(this.config) &&
!this.config.obo
) {
@ -126,8 +129,11 @@ export class MCPServerInspector {
return;
}
// Admin-provided API key means no OAuth flow is needed
if (this.config.apiKey?.source === 'admin') {
// API key auth (admin- or user-provided) is API-key, not OAuth. A credential-less
// probe of a bearer server returns the same 401 challenge as an OAuth server, so
// detection would misclassify it; trust the configured auth method. An explicit
// `oauth` block still wins if both are somehow set.
if (this.config.apiKey != null && this.config.oauth == null) {
this.config.requiresOAuth = false;
return;
}

View file

@ -341,7 +341,7 @@ describe('MCPServerInspector', () => {
expect(result.apiKey?.source).toBe('admin');
});
it('should still detect OAuth when apiKey.source is user', async () => {
it('should set requiresOAuth to false and skip probing when apiKey.source is user', async () => {
const rawConfig: t.MCPOptions = {
type: 'sse',
url: 'https://api.example.com/sse',
@ -351,6 +351,39 @@ describe('MCPServerInspector', () => {
},
};
// A credential-less probe of a bearer server returns the same 401 challenge as
// an OAuth server. Detection must be skipped so the user's API key is honored
// instead of forcing an OAuth flow.
mockDetectOAuthRequirement.mockResolvedValue({
requiresOAuth: true, // This would be returned if called, but it shouldn't be
method: 'protected-resource-metadata',
});
// No connection provided: the user's key is supplied per-user at connect time, so
// inspection must NOT open an unauthenticated connection (it would 401 and fail save).
const result = await MCPServerInspector.inspect('test_server', rawConfig);
expect(mockDetectOAuthRequirement).not.toHaveBeenCalled();
expect(MCPConnectionFactory.create).not.toHaveBeenCalled();
expect(result.requiresOAuth).toBe(false);
expect(result.apiKey?.source).toBe('user');
});
it('should honor an explicit oauth block even when a user apiKey is present', async () => {
const rawConfig: t.MCPOptions = {
type: 'sse',
url: 'https://api.example.com/sse',
apiKey: {
source: 'user',
authorization_type: 'bearer',
},
oauth: {
authorization_url: 'https://api.example.com/oauth/authorize',
token_url: 'https://api.example.com/oauth/token',
scope: 'read',
},
};
mockDetectOAuthRequirement.mockResolvedValue({
requiresOAuth: true,
method: 'protected-resource-metadata',
@ -358,7 +391,7 @@ describe('MCPServerInspector', () => {
const result = await MCPServerInspector.inspect('test_server', rawConfig, mockConnection);
// Should call OAuth detection for user-provided API key
// An explicit oauth config must take precedence over the apiKey short-circuit.
expect(mockDetectOAuthRequirement).toHaveBeenCalled();
expect(result.requiresOAuth).toBe(true);
});