diff --git a/coderd/mcp.go b/coderd/mcp.go index d6346900b3..2644d49670 100644 --- a/coderd/mcp.go +++ b/coderd/mcp.go @@ -118,9 +118,81 @@ func (api *API) createMCPServerConfig(rw http.ResponseWriter, r *http.Request) { // Metadata (RFC 9728) and Authorization Server Metadata // (RFC 8414), then register a client dynamically. if req.OAuth2ClientID == "" && req.OAuth2AuthURL == "" && req.OAuth2TokenURL == "" { - callbackURL := fmt.Sprintf("%s/api/experimental/mcp/servers/{id}/oauth2/callback", api.AccessURL.String()) + // Auto-discovery flow: we need the config ID first to + // build the correct callback URL. Insert the record + // with empty OAuth2 fields, perform discovery, then + // update. + customHeadersJSON, err := marshalCustomHeaders(req.CustomHeaders) + if err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid custom headers.", + Detail: err.Error(), + }) + return + } + + inserted, err := api.Database.InsertMCPServerConfig(ctx, database.InsertMCPServerConfigParams{ + DisplayName: strings.TrimSpace(req.DisplayName), + Slug: strings.TrimSpace(req.Slug), + Description: strings.TrimSpace(req.Description), + IconURL: strings.TrimSpace(req.IconURL), + Transport: strings.TrimSpace(req.Transport), + Url: strings.TrimSpace(req.URL), + AuthType: strings.TrimSpace(req.AuthType), + OAuth2ClientID: "", + OAuth2ClientSecret: "", + OAuth2ClientSecretKeyID: sql.NullString{}, + OAuth2AuthURL: "", + OAuth2TokenURL: "", + OAuth2Scopes: "", + APIKeyHeader: strings.TrimSpace(req.APIKeyHeader), + APIKeyValue: strings.TrimSpace(req.APIKeyValue), + APIKeyValueKeyID: sql.NullString{}, + CustomHeaders: customHeadersJSON, + CustomHeadersKeyID: sql.NullString{}, + ToolAllowList: coalesceStringSlice(trimStringSlice(req.ToolAllowList)), + ToolDenyList: coalesceStringSlice(trimStringSlice(req.ToolDenyList)), + Availability: strings.TrimSpace(req.Availability), + Enabled: req.Enabled, + CreatedBy: apiKey.UserID, + UpdatedBy: apiKey.UserID, + }) + if err != nil { + switch { + case database.IsUniqueViolation(err): + httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{ + Message: "MCP server config already exists.", + Detail: err.Error(), + }) + return + case database.IsCheckViolation(err): + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid MCP server config.", + Detail: err.Error(), + }) + return + default: + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to create MCP server config.", + Detail: err.Error(), + }) + return + } + } + + // Now build the callback URL with the actual ID. + callbackURL := fmt.Sprintf("%s/api/experimental/mcp/servers/%s/oauth2/callback", api.AccessURL.String(), inserted.ID) result, err := discoverAndRegisterMCPOAuth2(ctx, strings.TrimSpace(req.URL), callbackURL) if err != nil { + // Clean up: delete the partially created config. + deleteErr := api.Database.DeleteMCPServerConfigByID(ctx, inserted.ID) + if deleteErr != nil { + api.Logger.Warn(ctx, "failed to clean up MCP server config after OAuth2 discovery failure", + slog.F("config_id", inserted.ID), + slog.Error(deleteErr), + ) + } + api.Logger.Warn(ctx, "mcp oauth2 auto-discovery failed", slog.F("url", req.URL), slog.Error(err), @@ -131,13 +203,51 @@ func (api *API) createMCPServerConfig(rw http.ResponseWriter, r *http.Request) { }) return } - req.OAuth2ClientID = result.clientID - req.OAuth2ClientSecret = result.clientSecret - req.OAuth2AuthURL = result.authURL - req.OAuth2TokenURL = result.tokenURL - if req.OAuth2Scopes == "" { - req.OAuth2Scopes = result.scopes + + // Determine scopes: use the request value if provided, + // otherwise fall back to the discovered value. + oauth2Scopes := strings.TrimSpace(req.OAuth2Scopes) + if oauth2Scopes == "" { + oauth2Scopes = result.scopes } + + // Update the record with discovered OAuth2 credentials. + updated, err := api.Database.UpdateMCPServerConfig(ctx, database.UpdateMCPServerConfigParams{ + ID: inserted.ID, + DisplayName: inserted.DisplayName, + Slug: inserted.Slug, + Description: inserted.Description, + IconURL: inserted.IconURL, + Transport: inserted.Transport, + Url: inserted.Url, + AuthType: inserted.AuthType, + OAuth2ClientID: result.clientID, + OAuth2ClientSecret: result.clientSecret, + OAuth2ClientSecretKeyID: sql.NullString{}, + OAuth2AuthURL: result.authURL, + OAuth2TokenURL: result.tokenURL, + OAuth2Scopes: oauth2Scopes, + APIKeyHeader: inserted.APIKeyHeader, + APIKeyValue: inserted.APIKeyValue, + APIKeyValueKeyID: inserted.APIKeyValueKeyID, + CustomHeaders: inserted.CustomHeaders, + CustomHeadersKeyID: inserted.CustomHeadersKeyID, + ToolAllowList: inserted.ToolAllowList, + ToolDenyList: inserted.ToolDenyList, + Availability: inserted.Availability, + Enabled: inserted.Enabled, + UpdatedBy: apiKey.UserID, + }) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to update MCP server config with OAuth2 credentials.", + Detail: err.Error(), + }) + return + } + + httpapi.Write(ctx, rw, http.StatusCreated, convertMCPServerConfig(updated)) + return } else if req.OAuth2ClientID == "" || req.OAuth2AuthURL == "" || req.OAuth2TokenURL == "" { // Partial manual config: all three fields are required together. httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ diff --git a/coderd/mcp_test.go b/coderd/mcp_test.go index 7c0b2297ed..98e59b12a0 100644 --- a/coderd/mcp_test.go +++ b/coderd/mcp_test.go @@ -509,6 +509,141 @@ func TestMCPServerConfigsOAuth2AutoDiscovery(t *testing.T) { require.Equal(t, "read write", created.OAuth2Scopes) }) + // Regression test: verify that during dynamic client registration + // the redirect_uris sent to the authorization server contain the + // real config UUID, NOT the literal string "{id}". Before the + // fix, the callback URL was built before the config row existed, + // so it contained "{id}" literally, which caused "redirect URIs + // not approved" errors when the user later tried to connect. + t.Run("RedirectURIContainsRealConfigID", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + + // Buffered channel so the handler never blocks. + registeredRedirectURI := make(chan string, 1) + + // Stand up a mock auth server that captures the redirect_uris + // from the RFC 7591 Dynamic Client Registration request. + authServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/.well-known/oauth-authorization-server": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "issuer": "` + "http://" + r.Host + `", + "authorization_endpoint": "` + "http://" + r.Host + `/authorize", + "token_endpoint": "` + "http://" + r.Host + `/token", + "registration_endpoint": "` + "http://" + r.Host + `/register", + "response_types_supported": ["code"], + "scopes_supported": ["read", "write"] + }`)) + case "/register": + if r.Method != http.MethodPost { + http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + return + } + + // Decode the registration body and capture redirect_uris. + var body map[string]interface{} + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + http.Error(w, "bad json", http.StatusBadRequest) + return + } + if uris, ok := body["redirect_uris"].([]interface{}); ok && len(uris) > 0 { + if uri, ok := uris[0].(string); ok { + registeredRedirectURI <- uri + } + } + + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{ + "client_id": "test-client-id", + "client_secret": "test-client-secret" + }`)) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(authServer.Close) + + // Stand up a mock MCP server that returns RFC 9728 Protected + // Resource Metadata pointing to the auth server. + mcpServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/oauth-protected-resource" { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "resource": "` + "http://" + r.Host + `", + "authorization_servers": ["` + authServer.URL + `"] + }`)) + return + } + http.NotFound(w, r) + })) + t.Cleanup(mcpServer.Close) + + client := newMCPClient(t) + _ = coderdtest.CreateFirstUser(t, client) + + // Create config with auth_type=oauth2 but no OAuth2 fields to + // trigger auto-discovery and dynamic client registration. + created, err := client.CreateMCPServerConfig(ctx, codersdk.CreateMCPServerConfigRequest{ + DisplayName: "Redirect URI Test", + Slug: "redirect-uri-test", + Transport: "streamable_http", + URL: mcpServer.URL + "/v1/mcp", + AuthType: "oauth2", + Availability: "default_on", + Enabled: true, + ToolAllowList: []string{}, + ToolDenyList: []string{}, + }) + require.NoError(t, err) + require.Equal(t, "test-client-id", created.OAuth2ClientID) + require.True(t, created.HasOAuth2Secret) + + // The registration request has already completed by the time + // CreateMCPServerConfig returns, so the URI is in the channel. + var redirectURI string + select { + case redirectURI = <-registeredRedirectURI: + case <-ctx.Done(): + t.Fatal("timed out waiting for registration redirect URI") + } + + // Core assertion: the redirect URI must NOT contain the + // literal placeholder "{id}". Before the fix the callback + // URL was built before the database insert, so it had + // "{id}" where the UUID should be. + require.NotContains(t, redirectURI, "{id}", + "redirect URI sent during registration must not contain the literal \"{id}\" placeholder") + + // Verify the redirect URI contains the real config UUID that + // was assigned by the database. + require.Contains(t, redirectURI, created.ID.String(), + "redirect URI should contain the actual config UUID") + + // Sanity-check the full path structure. + require.Contains(t, redirectURI, + "/api/experimental/mcp/servers/"+created.ID.String()+"/oauth2/callback", + "redirect URI should have the expected callback path") + + // Double-check that the ID segment is a valid UUID (not some + // other placeholder or malformed value). + pathParts := strings.Split(redirectURI, "/") + var foundUUID bool + for _, part := range pathParts { + if _, err := uuid.Parse(part); err == nil { + foundUUID = true + require.Equal(t, created.ID.String(), part, + "UUID in redirect URI path should match created config ID") + break + } + } + require.True(t, foundUUID, + "redirect URI path should contain a valid UUID segment") + }) + t.Run("PartialOAuth2FieldsRejected", func(t *testing.T) { t.Parallel()