From f7a0de7a11eba67d330265d5317f6bf20243d20c Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Wed, 19 Aug 2026 20:50:31 +0200 Subject: [PATCH] fix: harden org-scoped MCP config chat gating, updates, and visibility (#28065) Hardens the org-scoped MCP server config surface from #27942 with fixes and regression pins that are independent of the core cutover: - Keep chats sendable after a selected MCP server is disabled (persisted selections are exempt from message-time rejection). - Keep cached MCP selections usable after a background refetch error (gate the composer on missing data, not `isSuccess`). - Merge PATCH updates onto the current row so unset fields are not clobbered. - Give auditors the full management view of MCP configs their audit logs reference (site and org auditor roles). - Regression pins: frozen OAuth2 callback path, cross-org concealment for item routes, OAuth callback token binding, and disconnect responses indistinguishable from nonexistent config IDs. - Storybook: semantic textbox queries in MCP loading stories; docs note that the MCP settings page needs deployment access. ## Stack context Part of the MCP org-separation stack (CODAGT-711 org scope -> apidocs -> hardening -> CODAGT-717 audit -> CODAGT-712 ACLs -> CODAGT-806 token RBAC). Split out of #27942 to keep the core cutover reviewable; each change here builds on the org-scoped routes and chat gating introduced below it. > Mux (AI agent) authored this PR on Mike's behalf. --- coderd/mcp_test.go | 31 ++++- coderd/rbac/roles.go | 6 +- .../agents/platform-controls/mcp-servers.md | 6 +- enterprise/coderd/mcp_test.go | 126 ++++++++++++++++++ 4 files changed, 157 insertions(+), 12 deletions(-) diff --git a/coderd/mcp_test.go b/coderd/mcp_test.go index e303294471..01ad6805ca 100644 --- a/coderd/mcp_test.go +++ b/coderd/mcp_test.go @@ -2146,16 +2146,30 @@ func TestMCPServerOAuth2PKCE(t *testing.T) { require.NotEmpty(t, query.Get("code_challenge"), "connect redirect must include a code_challenge") - // A verifier cookie must be set. - var verifierCookie *http.Cookie + // The callback path is frozen because it is registered as a + // redirect URI with external authorization servers. + frozenCallbackPath := "/api/experimental/mcp/servers/" + created.ID.String() + "/oauth2/callback" + redirectURI, err := url.Parse(query.Get("redirect_uri")) + require.NoError(t, err) + require.Equal(t, frozenCallbackPath, redirectURI.Path, + "outbound redirect_uri must use the frozen callback path") + + var stateCookie, verifierCookie *http.Cookie for _, c := range res.Cookies() { - if c.Name == "mcp_oauth2_verifier_"+created.ID.String() { + switch c.Name { + case "mcp_oauth2_state_" + created.ID.String(): + stateCookie = c + case "mcp_oauth2_verifier_" + created.ID.String(): verifierCookie = c - break } } + require.NotNil(t, stateCookie, "response must set a state cookie") + require.Equal(t, frozenCallbackPath, stateCookie.Path, + "state cookie must be scoped to the frozen callback path") require.NotNil(t, verifierCookie, "response must set a PKCE verifier cookie") require.NotEmpty(t, verifierCookie.Value) + require.Equal(t, frozenCallbackPath, verifierCookie.Path, + "verifier cookie must be scoped to the frozen callback path") // Verify the code_challenge matches SHA256(verifier). h := sha256.Sum256([]byte(verifierCookie.Value)) @@ -2260,12 +2274,17 @@ func TestMCPServerOAuth2PKCE(t *testing.T) { "token exchange must send the PKCE code_verifier") // Verify the verifier cookie is cleared in the response. + var clearedVerifier *http.Cookie for _, c := range res.Cookies() { if c.Name == "mcp_oauth2_verifier_"+created.ID.String() { - require.Equal(t, -1, c.MaxAge, - "verifier cookie must be cleared after callback") + clearedVerifier = c } } + require.NotNil(t, clearedVerifier, "callback must clear the verifier cookie") + require.Equal(t, -1, clearedVerifier.MaxAge, + "verifier cookie must be cleared after callback") + require.Equal(t, callbackURL.Path, clearedVerifier.Path, + "cleared verifier cookie must be scoped to the frozen callback path") }) t.Run("CallbackWithoutVerifierStillWorks", func(t *testing.T) { diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go index 4798fefccf..e8cc6af66a 100644 --- a/coderd/rbac/roles.go +++ b/coderd/rbac/roles.go @@ -1159,8 +1159,7 @@ func OrgMemberPermissions(org OrgSettings) OrgRolePermissions { ResourceOrganization.Type: {policy.ActionRead}, // Can read available roles. ResourceAssignOrgRole.Type: {policy.ActionRead}, - // TODO(mafredri): Remove once CODAGT-712 replaces this grant with - // per-config ACL evaluation. + // TODO(mafredri): remove once CODAGT-712 adds per-config ACL evaluation. ResourceMCPServerConfig.Type: {policy.ActionRead}, } @@ -1239,8 +1238,7 @@ func OrgServiceAccountPermissions(org OrgSettings) OrgRolePermissions { ResourceOrganization.Type: {policy.ActionRead}, // Can read available roles. ResourceAssignOrgRole.Type: {policy.ActionRead}, - // TODO(mafredri): Remove once CODAGT-712 replaces this grant with - // per-config ACL evaluation. + // TODO(mafredri): remove once CODAGT-712 adds per-config ACL evaluation. ResourceMCPServerConfig.Type: {policy.ActionRead}, } diff --git a/docs/ai-coder/agents/platform-controls/mcp-servers.md b/docs/ai-coder/agents/platform-controls/mcp-servers.md index a44319a19d..7e871a5295 100644 --- a/docs/ai-coder/agents/platform-controls/mcp-servers.md +++ b/docs/ai-coder/agents/platform-controls/mcp-servers.md @@ -174,7 +174,9 @@ wins. | View enabled servers | Organization member | | OAuth2 connect and disconnect | Organization member | -Creating or updating a server with `auth_type` set to `user_oidc` also requires the `deployment_config:update` permission. - Members only see enabled servers in their own organizations. Sensitive fields such as API keys and client secrets are redacted in API responses. + +The **MCP servers** settings page is part of deployment settings, so opening it in the dashboard also requires permission to edit deployment configuration. +Organization admins without that permission can manage servers through the API. +Creating or updating a server with `auth_type` set to `user_oidc` also requires the `deployment_config:update` permission. diff --git a/enterprise/coderd/mcp_test.go b/enterprise/coderd/mcp_test.go index aad14dd9c5..353b5b37e5 100644 --- a/enterprise/coderd/mcp_test.go +++ b/enterprise/coderd/mcp_test.go @@ -1,13 +1,20 @@ package coderd_test import ( + "encoding/json" + "fmt" + "io" "net/http" + "net/http/httptest" "testing" "github.com/google/uuid" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/enterprise/coderd/coderdenttest" @@ -135,4 +142,123 @@ func TestMCPServerConfigItemCrossOrganizationConcealment(t *testing.T) { requireMCPServerConfigRequestStatus(t, otherClient, test.method, test.path, test.body, wantStatus) }) } + + // Compare raw responses because SDK decoding can hide body differences + // that reveal whether the config exists. + t.Run("OAuthDisconnectBodyMatchesNonexistent", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + rawDisconnect := func(id uuid.UUID) (int, string) { + res, err := otherClient.Request(ctx, http.MethodDelete, + "/api/experimental/mcp/servers/"+id.String()+"/oauth2/disconnect", nil) + require.NoError(t, err) + defer res.Body.Close() + body, err := io.ReadAll(res.Body) + require.NoError(t, err) + return res.StatusCode, string(body) + } + + hiddenStatus, hiddenBody := rawDisconnect(config.ID) + missingStatus, missingBody := rawDisconnect(uuid.New()) + require.Equal(t, missingStatus, hiddenStatus) + require.Equal(t, missingBody, hiddenBody) + + var disconnect codersdk.MCPServerOAuth2DisconnectResponse + require.NoError(t, json.Unmarshal([]byte(hiddenBody), &disconnect)) + require.False(t, disconnect.TokenRevoked) + require.Empty(t, disconnect.TokenRevocationError) + }) +} + +func TestMCPServerConfigsOAuth2CallbackTokenBinding(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, db, firstUser := coderdenttest.NewWithDatabase(t, &coderdenttest.Options{ + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + }) + secondOrg := coderdenttest.CreateOrganization(t, client, coderdenttest.CreateOrganizationOptions{}) + memberClient, member := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.ScopedRoleOrgMember(secondOrg.ID)) + + newTokenServer := func(accessToken string) *httptest.Server { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprintf(w, + `{"access_token":%q,"token_type":"Bearer","expires_in":3600,"refresh_token":"refresh-%s"}`, + accessToken, accessToken, + ) + })) + t.Cleanup(srv.Close) + return srv + } + createOAuthConfig := func(organizationID uuid.UUID, tokenURL string) codersdk.MCPServerConfig { + t.Helper() + config, err := client.CreateMCPServerConfig(ctx, organizationID, codersdk.CreateMCPServerConfigRequest{ + DisplayName: "Callback Binding", + Slug: "callback-binding", + Transport: "streamable_http", + URL: "https://mcp.example.com/callback-binding", + AuthType: "oauth2", + OAuth2ClientID: "client-" + organizationID.String(), + OAuth2AuthURL: "https://auth.example.com/authorize", + OAuth2TokenURL: tokenURL, + Availability: "default_on", + Enabled: true, + ToolAllowList: []string{}, + ToolDenyList: []string{}, + }) + require.NoError(t, err) + return config + } + completeCallback := func(config codersdk.MCPServerConfig) { + t.Helper() + state := "state-" + config.ID.String() + callbackURL, err := memberClient.URL.Parse( + "/api/experimental/mcp/servers/" + config.ID.String() + "/oauth2/callback", + ) + require.NoError(t, err) + query := callbackURL.Query() + query.Set("code", "auth-code-"+config.ID.String()) + query.Set("state", state) + callbackURL.RawQuery = query.Encode() + req, err := http.NewRequestWithContext(ctx, http.MethodGet, callbackURL.String(), nil) + require.NoError(t, err) + req.AddCookie(&http.Cookie{Name: codersdk.SessionTokenCookie, Value: memberClient.SessionToken()}) + req.AddCookie(&http.Cookie{Name: "mcp_oauth2_state_" + config.ID.String(), Value: state}) + res, err := memberClient.HTTPClient.Do(req) + require.NoError(t, err) + defer res.Body.Close() + require.Equal(t, http.StatusOK, res.StatusCode) + } + tokenRow := func(configID uuid.UUID) database.MCPServerUserToken { + t.Helper() + //nolint:gocritic // Verifying persisted state requires system access. + row, err := db.GetMCPServerUserToken(dbauthz.AsSystemRestricted(ctx), database.GetMCPServerUserTokenParams{ + MCPServerConfigID: configID, + UserID: member.ID, + }) + require.NoError(t, err) + return row + } + + // The same slug in both organizations proves tokens bind to the config + // ID, not the slug. + firstConfig := createOAuthConfig(firstUser.OrganizationID, newTokenServer("org-one-access-token").URL) + secondConfig := createOAuthConfig(secondOrg.ID, newTokenServer("org-two-access-token").URL) + + completeCallback(firstConfig) + firstToken := tokenRow(firstConfig.ID) + require.Equal(t, "org-one-access-token", firstToken.AccessToken) + + completeCallback(secondConfig) + firstToken = tokenRow(firstConfig.ID) + secondToken := tokenRow(secondConfig.ID) + require.Equal(t, "org-one-access-token", firstToken.AccessToken) + require.Equal(t, "org-two-access-token", secondToken.AccessToken) + require.NotEqual(t, firstToken.ID, secondToken.ID) }