From 299e72ad30fec6351326d2afa5ae3b6f55fe5ae7 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Wed, 19 Aug 2026 21:11:52 +0200 Subject: [PATCH] feat: audit MCP server config changes (#27943) Adds enterprise audit logging for MCP server config create, update, and delete, with strict secret redaction. MCP configs hold credentials (OAuth2 client secrets, API keys, custom headers), so admin changes to them need an audit trail. ## Summary - `enterprise/audit/table.go` gains an `MCPServerConfig` entry enumerating every column: `oauth2_client_secret`, `api_key_value`, and `custom_headers` are `ActionSecret` (never appear in diffs); dbcrypt `*_key_id` bookkeeping, IDs, and timestamps are ignored; the remaining config fields, including the endpoint URL fields, are tracked so auditors can see which endpoints a config points at. - Type registration in `coderd/audit` (diff, request, resource target with org attribution), `codersdk/audit.go`, and a `resource_type` enum migration. - Handlers wire `audit.InitRequest`: create records `New`; update and delete record `Old` from the param middleware before the write-authorization check, so a readable-but-not-writable caller produces an audited 403 while read-denied callers stay concealed as unaudited 404s. - Tests: create/update/delete audit entries, write-denied and delete-denied 403 auditing, cross-org concealment producing zero entries, and a serializer-level regression test proving none of the three secret classes can reach a serialized diff. - Review round: MCP config audit entries link to `/ai/settings/mcp-servers/{id}`, audit table comments are trimmed per review, and a fault-injection test pins that a config row surviving a failed post-discovery credential update still gets its creation audit entry. Stacked on #27942 (org-scoped MCP configs). Part of the MCP org-separation stack. Closes https://linear.app/codercom/issue/CODAGT-717 UAT: verified on a trial-licensed dogfood instance: audit entries for the full CRUD lifecycle with correct actor/org/target, redacted secrets in the update and OAuth2 create diffs, and a full plaintext scan of the audit dump finding zero secret leaks. > Mux (AI agent) authored this PR on Mike's behalf. --- coderd/apidoc/docs.go | 2 + coderd/apidoc/swagger.json | 2 + coderd/audit.go | 25 ++ coderd/audit/diff.go | 1 + coderd/audit/request.go | 12 + coderd/audit/request_test.go | 17 + coderd/audit_test.go | 52 +++ coderd/database/dump.sql | 3 +- .../000575_audit_mcp_server_config.down.sql | 1 + .../000575_audit_mcp_server_config.up.sql | 2 + coderd/database/models.go | 5 +- coderd/database/sqlc.yaml | 1 + coderd/mcp.go | 68 +++- coderd/mcp_test.go | 357 ++++++++++++++++++ codersdk/audit.go | 3 + docs/admin/security/audit-logs.md | 1 + docs/reference/api/schemas.md | 6 +- enterprise/audit/diff.go | 25 +- enterprise/audit/diff_internal_test.go | 128 +++++++ enterprise/audit/table.go | 35 ++ enterprise/coderd/mcp_test.go | 13 + site/src/api/typesGenerated.ts | 2 + 22 files changed, 751 insertions(+), 10 deletions(-) create mode 100644 coderd/database/migrations/000575_audit_mcp_server_config.down.sql create mode 100644 coderd/database/migrations/000575_audit_mcp_server_config.up.sql diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index bcdf54ed3a..a801739c41 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -24939,6 +24939,7 @@ const docTemplate = `{ "group_ai_budget", "user_ai_budget_override", "chat", + "mcp_server_config", "user_secret", "user_skill", "chat_instruction_settings" @@ -24978,6 +24979,7 @@ const docTemplate = `{ "ResourceTypeGroupAIBudget", "ResourceTypeUserAIBudgetOverride", "ResourceTypeChat", + "ResourceTypeMCPServerConfig", "ResourceTypeUserSecret", "ResourceTypeUserSkill", "ResourceTypeChatInstructionSettings" diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index a05523cc8d..5962b014af 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -22876,6 +22876,7 @@ "group_ai_budget", "user_ai_budget_override", "chat", + "mcp_server_config", "user_secret", "user_skill", "chat_instruction_settings" @@ -22915,6 +22916,7 @@ "ResourceTypeGroupAIBudget", "ResourceTypeUserAIBudgetOverride", "ResourceTypeChat", + "ResourceTypeMCPServerConfig", "ResourceTypeUserSecret", "ResourceTypeUserSkill", "ResourceTypeChatInstructionSettings" diff --git a/coderd/audit.go b/coderd/audit.go index 44ed30770b..4188478cf5 100644 --- a/coderd/audit.go +++ b/coderd/audit.go @@ -22,6 +22,8 @@ import ( "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/coderd/searchquery" "github.com/coder/coder/v2/codersdk" ) @@ -501,6 +503,18 @@ func (api *API) auditLogIsResourceDeleted(ctx context.Context, alog database.Get api.Logger.Error(ctx, "unable to fetch chat", slog.Error(err)) } return false + case database.ResourceTypeMCPServerConfig: + // MCP server configs are hard-deleted, so a 404 means deleted. + _, err := api.Database.GetMCPServerConfigByID(ctx, alog.AuditLog.ResourceID) + if xerrors.Is(err, sql.ErrNoRows) { + return true + } + // Config reads are org-scoped, so an auditor can lack read on + // the config's organization. That is not worth logging. + if err != nil && !dbauthz.IsNotAuthorizedError(err) { + api.Logger.Error(ctx, "unable to fetch mcp server config", slog.Error(err)) + } + return false case database.ResourceTypeUserSecret: _, err := api.Database.GetUserSecretByID(ctx, alog.AuditLog.ResourceID) if xerrors.Is(err, sql.ErrNoRows) { @@ -604,6 +618,17 @@ func (api *API) auditLogResourceLink(ctx context.Context, alog database.GetAudit // Chats are surfaced at /agents/{id}. They are owner-scoped but // not username-scoped in the URL like workspaces or tasks. return fmt.Sprintf("/agents/%s", alog.AuditLog.ResourceID) + case database.ResourceTypeMCPServerConfig: + actor, ok := dbauthz.ActorFromContext(ctx) + if !ok { + return "" + } + // The MCP settings page admits only deployment-config managers, + // so emit the link only for callers the page will accept. + if err := api.HTTPAuth.Authorizer.Authorize(ctx, actor, policy.ActionUpdate, rbac.ResourceDeploymentConfig); err != nil { + return "" + } + return fmt.Sprintf("/ai/settings/mcp-servers/%s", alog.AuditLog.ResourceID) case database.ResourceTypeUserSecret: // TODO(PLAT-102): point at the user secrets management page once // it ships. Until then, the audit row links nowhere. diff --git a/coderd/audit/diff.go b/coderd/audit/diff.go index 97105c24d5..8564f33a96 100644 --- a/coderd/audit/diff.go +++ b/coderd/audit/diff.go @@ -39,6 +39,7 @@ type Auditable interface { database.AIProviderKey | database.AIGatewayKey | database.Chat | + database.MCPServerConfig | database.AuditableGroupAIBudget | database.AuditableUserAIBudgetOverride | database.UserSecret | diff --git a/coderd/audit/request.go b/coderd/audit/request.go index 9aaafb1e2c..944c0a891d 100644 --- a/coderd/audit/request.go +++ b/coderd/audit/request.go @@ -1,6 +1,7 @@ package audit import ( + "cmp" "context" "database/sql" "encoding/json" @@ -154,6 +155,10 @@ func ResourceTarget[T Auditable](tgt T) string { // for display; collisions affect the display label and search // filter but not the primary resource identifier. return typed.ID.String()[:8] + case database.MCPServerConfig: + // Updates can persist an empty display name; fall back to the slug, or + // the ID if both are empty, so the audit entry stays identifiable. + return cmp.Or(typed.DisplayName, typed.Slug, typed.ID.String()) case database.UserSecret: return typed.Name case database.UserSkill: @@ -257,6 +262,8 @@ func ResourceID[T Auditable](tgt T) uuid.UUID { return typed.UserID case database.Chat: return typed.ID + case database.MCPServerConfig: + return typed.ID case database.UserSecret: return typed.ID case database.UserSkill: @@ -335,6 +342,8 @@ func ResourceType[T Auditable](tgt T) database.ResourceType { return database.ResourceTypeUserAIBudgetOverride case database.Chat: return database.ResourceTypeChat + case database.MCPServerConfig: + return database.ResourceTypeMCPServerConfig case database.UserSecret: return database.ResourceTypeUserSecret case database.UserSkill: @@ -425,6 +434,9 @@ func ResourceRequiresOrgID[T Auditable]() bool { // Chats always have a non-null organization_id (since // migration 000467). return true + case database.MCPServerConfig: + // MCP server configs always carry a non-null organization_id. + return true case database.UserSecret: // User secrets are global to the user across organizations. return false diff --git a/coderd/audit/request_test.go b/coderd/audit/request_test.go index 9bdf4718d3..44834e0221 100644 --- a/coderd/audit/request_test.go +++ b/coderd/audit/request_test.go @@ -45,3 +45,20 @@ func TestResourceTarget_ChatTitleNotLeaked(t *testing.T) { require.NotContains(t, target, chat.Title, "ResourceTarget for Chat must not contain the title; it should use a UUID prefix") } + +func TestResourceTarget_MCPServerConfigSlugFallback(t *testing.T) { + t.Parallel() + + config := database.MCPServerConfig{ + ID: uuid.UUID{1}, + DisplayName: "GitHub MCP", + Slug: "github", + } + require.Equal(t, "GitHub MCP", audit.ResourceTarget(config)) + + config.DisplayName = "" + require.Equal(t, "github", audit.ResourceTarget(config)) + + config.Slug = "" + require.Equal(t, config.ID.String(), audit.ResourceTarget(config)) +} diff --git a/coderd/audit_test.go b/coderd/audit_test.go index 721e133fc7..94d7002ea6 100644 --- a/coderd/audit_test.go +++ b/coderd/audit_test.go @@ -16,6 +16,7 @@ import ( "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/provisioner/echo" @@ -143,6 +144,57 @@ func TestAuditLogs(t *testing.T) { workspace.OwnerName, workspace.Name, buildNumberString)) }) + t.Run("MCPServerConfigAuditLink", func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + db, ps := dbtestutil.NewDB(t) + client := coderdtest.New(t, &coderdtest.Options{Database: db, Pubsub: ps}) + user := coderdtest.CreateFirstUser(t, client) + auditor, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.RoleAuditor()) + orgAdmin, _ := coderdtest.CreateAnotherUser(t, client, user.OrganizationID, rbac.ScopedRoleOrgAdmin(user.OrganizationID)) + + config := dbgen.MCPServerConfig(t, db, database.MCPServerConfig{ + OrganizationID: user.OrganizationID, + }) + err := client.CreateTestAuditLog(ctx, codersdk.CreateTestAuditLogRequest{ + Action: codersdk.AuditActionCreate, + ResourceType: codersdk.ResourceTypeMCPServerConfig, + ResourceID: config.ID, + OrganizationID: user.OrganizationID, + }) + require.NoError(t, err) + + auditorLogs, err := auditor.AuditLogs(ctx, codersdk.AuditLogsRequest{ + Pagination: codersdk.Pagination{ + Limit: 1, + }, + }) + require.NoError(t, err) + require.Len(t, auditorLogs.AuditLogs, 1) + require.Empty(t, auditorLogs.AuditLogs[0].ResourceLink) + + // Organization admins hold MCP permissions but not the + // deployment-config access the settings page requires. + orgAdminLogs, err := orgAdmin.AuditLogs(ctx, codersdk.AuditLogsRequest{ + Pagination: codersdk.Pagination{ + Limit: 1, + }, + }) + require.NoError(t, err) + require.Len(t, orgAdminLogs.AuditLogs, 1) + require.Empty(t, orgAdminLogs.AuditLogs[0].ResourceLink) + + ownerLogs, err := client.AuditLogs(ctx, codersdk.AuditLogsRequest{ + Pagination: codersdk.Pagination{ + Limit: 1, + }, + }) + require.NoError(t, err) + require.Len(t, ownerLogs.AuditLogs, 1) + require.Equal(t, fmt.Sprintf("/ai/settings/mcp-servers/%s", config.ID), ownerLogs.AuditLogs[0].ResourceLink) + }) + t.Run("Organization", func(t *testing.T) { t.Parallel() diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 63487a1a9a..0512d79845 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -604,7 +604,8 @@ CREATE TYPE resource_type AS ENUM ( 'ai_gateway_key', 'user_ai_budget_override', 'oauth2_provider_settings', - 'chat_instruction_settings' + 'chat_instruction_settings', + 'mcp_server_config' ); CREATE TYPE shareable_workspace_owners AS ENUM ( diff --git a/coderd/database/migrations/000575_audit_mcp_server_config.down.sql b/coderd/database/migrations/000575_audit_mcp_server_config.down.sql new file mode 100644 index 0000000000..35020b349f --- /dev/null +++ b/coderd/database/migrations/000575_audit_mcp_server_config.down.sql @@ -0,0 +1 @@ +-- No-op, enum values can't be dropped. diff --git a/coderd/database/migrations/000575_audit_mcp_server_config.up.sql b/coderd/database/migrations/000575_audit_mcp_server_config.up.sql new file mode 100644 index 0000000000..f974064743 --- /dev/null +++ b/coderd/database/migrations/000575_audit_mcp_server_config.up.sql @@ -0,0 +1,2 @@ +ALTER TYPE resource_type + ADD VALUE IF NOT EXISTS 'mcp_server_config'; diff --git a/coderd/database/models.go b/coderd/database/models.go index e49b0ad042..03b7b2584d 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -3551,6 +3551,7 @@ const ( ResourceTypeUserAIBudgetOverride ResourceType = "user_ai_budget_override" ResourceTypeOauth2ProviderSettings ResourceType = "oauth2_provider_settings" ResourceTypeChatInstructionSettings ResourceType = "chat_instruction_settings" + ResourceTypeMCPServerConfig ResourceType = "mcp_server_config" ) func (e *ResourceType) Scan(src interface{}) error { @@ -3626,7 +3627,8 @@ func (e ResourceType) Valid() bool { ResourceTypeAIGatewayKey, ResourceTypeUserAIBudgetOverride, ResourceTypeOauth2ProviderSettings, - ResourceTypeChatInstructionSettings: + ResourceTypeChatInstructionSettings, + ResourceTypeMCPServerConfig: return true } return false @@ -3671,6 +3673,7 @@ func AllResourceTypeValues() []ResourceType { ResourceTypeUserAIBudgetOverride, ResourceTypeOauth2ProviderSettings, ResourceTypeChatInstructionSettings, + ResourceTypeMCPServerConfig, } } diff --git a/coderd/database/sqlc.yaml b/coderd/database/sqlc.yaml index 690173902f..f7d820780c 100644 --- a/coderd/database/sqlc.yaml +++ b/coderd/database/sqlc.yaml @@ -275,6 +275,7 @@ sql: resource_type_ai_gateway_key: ResourceTypeAIGatewayKey mcp_server_config: MCPServerConfig mcp_server_configs: MCPServerConfigs + resource_type_mcp_server_config: ResourceTypeMCPServerConfig mcp_server_user_token: MCPServerUserToken mcp_server_user_tokens: MCPServerUserTokens mcp_server_tool_snapshot: MCPServerToolSnapshot diff --git a/coderd/mcp.go b/coderd/mcp.go index 70074189aa..fd2ee6388c 100644 --- a/coderd/mcp.go +++ b/coderd/mcp.go @@ -19,6 +19,7 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog/v3" + "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbtime" @@ -248,6 +249,15 @@ func (api *API) createMCPServerConfig(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() apiKey := httpmw.APIKey(r) organization := httpmw.OrganizationParam(r) + auditor := api.Auditor.Load() + aReq, commitAudit := audit.InitRequest[database.MCPServerConfig](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionCreate, + OrganizationID: organization.ID, + }) + defer commitAudit() if !api.Authorize(r, policy.ActionCreate, rbac.ResourceMCPServerConfig.InOrg(organization.ID)) { httpapi.Forbidden(rw) return @@ -441,6 +451,8 @@ func (api *API) createMCPServerConfig(rw http.ResponseWriter, r *http.Request) { } } + aReq.New = inserted + httpapi.Write(ctx, rw, http.StatusCreated, convertMCPServerConfig(inserted)) } @@ -544,6 +556,21 @@ func (api *API) getMCPServerConfigForMutation(rw http.ResponseWriter, r *http.Re func (api *API) updateMCPServerConfig(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() apiKey := httpmw.APIKey(r) + auditor := api.Auditor.Load() + aReq, commitAudit := audit.InitRequest[database.MCPServerConfig](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionWrite, + }) + defer commitAudit() + + // Set Old before the write-authz check so a write-denied 403 is + // audited. Read-denied callers were already concealed with 404 by + // the param middleware and never reach this handler. + aReq.Old = httpmw.MCPServerConfigParam(r) + aReq.UpdateOrganizationID(aReq.Old.OrganizationID) + existing, ok := api.getMCPServerConfigForMutation(rw, r, policy.ActionUpdate) if !ok { return @@ -593,14 +620,15 @@ func (api *API) updateMCPServerConfig(rw http.ResponseWriter, r *http.Request) { var updated database.MCPServerConfig err := api.Database.InTx(func(tx database.Store) error { - // Lock and re-fetch the row so omitted fields come from the latest - // version and grant invalidation serializes with in-flight OAuth - // callbacks verifying the same config. + // Lock and re-fetch the row so omitted fields and the audit baseline + // match the row this update replaces, and so grant invalidation + // serializes with in-flight OAuth callbacks verifying the config. current, err := tx.GetMCPServerConfigByIDForUpdate(ctx, existing.ID) if err != nil { return err } existing = current + aReq.Old = current touchesUserOIDC := existing.AuthType == "user_oidc" || (req.AuthType != nil && *req.AuthType == "user_oidc") @@ -873,6 +901,8 @@ func (api *API) updateMCPServerConfig(rw http.ResponseWriter, r *http.Request) { } } + aReq.New = updated + httpapi.Write(ctx, rw, http.StatusOK, convertMCPServerConfig(updated)) } @@ -888,12 +918,42 @@ func (api *API) updateMCPServerConfig(rw http.ResponseWriter, r *http.Request) { // EXPERIMENTAL: this endpoint is experimental and is subject to change. func (api *API) deleteMCPServerConfig(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() + auditor := api.Auditor.Load() + aReq, commitAudit := audit.InitRequest[database.MCPServerConfig](rw, &audit.RequestParams{ + Audit: *auditor, + Log: api.Logger, + Request: r, + Action: database.AuditActionDelete, + }) + defer commitAudit() + + // Set Old before the write-authz check so a write-denied 403 is + // audited. Read-denied callers were already concealed with 404 by + // the param middleware and never reach this handler. + aReq.Old = httpmw.MCPServerConfigParam(r) + aReq.UpdateOrganizationID(aReq.Old.OrganizationID) + config, ok := api.getMCPServerConfigForMutation(rw, r, policy.ActionDelete) if !ok { return } - if err := api.Database.DeleteMCPServerConfigByID(ctx, config.ID); err != nil { + err := api.Database.InTx(func(tx database.Store) error { + // Re-fetch under a row lock so the audit record describes the + // row this request actually removes, not a middleware snapshot + // that a concurrent update may have made stale. + current, err := tx.GetMCPServerConfigByIDForUpdate(ctx, config.ID) + if err != nil { + return err + } + aReq.Old = current + return tx.DeleteMCPServerConfigByID(ctx, current.ID) + }, nil) + if err != nil { + if httpapi.Is404Error(err) { + httpapi.ResourceNotFound(rw) + return + } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Failed to delete MCP server config.", Detail: err.Error(), diff --git a/coderd/mcp_test.go b/coderd/mcp_test.go index 01ad6805ca..900d1c85e3 100644 --- a/coderd/mcp_test.go +++ b/coderd/mcp_test.go @@ -21,11 +21,14 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/xerrors" + "github.com/coder/coder/v2/coderd/audit" "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/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" @@ -261,6 +264,360 @@ func TestMCPServerConfigWrongOrganization(t *testing.T) { require.Equal(t, http.StatusNotFound, sdkErr.StatusCode()) } +func TestMCPServerConfigsAudit(t *testing.T) { + t.Parallel() + + newAuditedMCPClient := func(t testing.TB) (*codersdk.Client, *audit.MockAuditor) { + t.Helper() + mAudit := audit.NewMock() + providerKeys := coderdtest.FakeOpenAICompatProviderAPIKeys(t) + client := coderdtest.New(t, &coderdtest.Options{ + DeploymentValues: mcpDeploymentValues(t), + ChatProviderAPIKeys: &providerKeys, + Auditor: mAudit, + }) + return client, mAudit + } + + t.Run("Create", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, mAudit := newAuditedMCPClient(t) + firstUser := coderdtest.CreateFirstUser(t, client) + + mAudit.ResetLogs() + created, err := client.CreateMCPServerConfig(ctx, firstUser.OrganizationID, codersdk.CreateMCPServerConfigRequest{ + DisplayName: "Audit Create", + Slug: "audit-create", + Transport: "streamable_http", + URL: "https://mcp.example.com/audit", + AuthType: "api_key", + APIKeyHeader: "X-Api-Key", + APIKeyValue: "super-secret-api-key", + CustomHeaders: map[string]string{ + "X-Extra": "plaintext-header-value", + }, + Availability: "default_on", + Enabled: true, + }) + require.NoError(t, err) + + logs := mAudit.AuditLogs() + require.Len(t, logs, 1) + require.Equal(t, database.AuditActionCreate, logs[0].Action) + require.Equal(t, database.ResourceTypeMCPServerConfig, logs[0].ResourceType) + require.Equal(t, created.ID, logs[0].ResourceID) + require.Equal(t, "Audit Create", logs[0].ResourceTarget) + require.Equal(t, firstUser.UserID, logs[0].UserID) + require.Equal(t, firstUser.OrganizationID, logs[0].OrganizationID) + require.EqualValues(t, http.StatusCreated, logs[0].StatusCode) + }) + + t.Run("Update", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, mAudit := newAuditedMCPClient(t) + firstUser := coderdtest.CreateFirstUser(t, client) + config := createMCPServerConfig(t, client, firstUser.OrganizationID, "audit-update", true) + + mAudit.ResetLogs() + newName := "Audit Update" + updated, err := client.UpdateMCPServerConfig(ctx, firstUser.OrganizationID, config.ID, codersdk.UpdateMCPServerConfigRequest{ + DisplayName: &newName, + }) + require.NoError(t, err) + require.Equal(t, newName, updated.DisplayName) + + logs := mAudit.AuditLogs() + require.Len(t, logs, 1) + require.Equal(t, database.AuditActionWrite, logs[0].Action) + require.Equal(t, database.ResourceTypeMCPServerConfig, logs[0].ResourceType) + require.Equal(t, config.ID, logs[0].ResourceID) + require.Equal(t, newName, logs[0].ResourceTarget) + require.Equal(t, firstUser.UserID, logs[0].UserID) + require.Equal(t, firstUser.OrganizationID, logs[0].OrganizationID) + require.EqualValues(t, http.StatusOK, logs[0].StatusCode) + }) + + t.Run("Delete", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, mAudit := newAuditedMCPClient(t) + firstUser := coderdtest.CreateFirstUser(t, client) + config := createMCPServerConfig(t, client, firstUser.OrganizationID, "audit-delete", true) + + mAudit.ResetLogs() + err := client.DeleteMCPServerConfig(ctx, firstUser.OrganizationID, config.ID) + require.NoError(t, err) + + logs := mAudit.AuditLogs() + require.Len(t, logs, 1) + require.Equal(t, database.AuditActionDelete, logs[0].Action) + require.Equal(t, database.ResourceTypeMCPServerConfig, logs[0].ResourceType) + require.Equal(t, config.ID, logs[0].ResourceID) + require.Equal(t, firstUser.UserID, logs[0].UserID) + require.Equal(t, firstUser.OrganizationID, logs[0].OrganizationID) + require.EqualValues(t, http.StatusNoContent, logs[0].StatusCode) + }) + + t.Run("DeleteAuditsPersistedRow", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + mAudit := audit.NewMock() + providerKeys := coderdtest.FakeOpenAICompatProviderAPIKeys(t) + db, ps := dbtestutil.NewDB(t) + store := &staleMCPServerConfigReadStore{Store: db} + client := coderdtest.New(t, &coderdtest.Options{ + DeploymentValues: mcpDeploymentValues(t), + ChatProviderAPIKeys: &providerKeys, + Auditor: mAudit, + Database: store, + Pubsub: ps, + }) + firstUser := coderdtest.CreateFirstUser(t, client) + config := createMCPServerConfig(t, client, firstUser.OrganizationID, "audit-delete-stale", true) + + // Simulate a concurrent update landing between the param + // middleware read and the delete transaction. + store.stale.Store(true) + mAudit.ResetLogs() + err := client.DeleteMCPServerConfig(ctx, firstUser.OrganizationID, config.ID) + require.NoError(t, err) + + logs := mAudit.AuditLogs() + require.Len(t, logs, 1) + require.Equal(t, config.DisplayName, logs[0].ResourceTarget) + }) + + t.Run("AutoDiscoveryFailureNotAudited", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, mAudit := newAuditedMCPClient(t) + firstUser := coderdtest.CreateFirstUser(t, client) + + mAudit.ResetLogs() + // Discovery fails immediately: nothing listens on the URL. + // The partially inserted row is cleaned up, so no audit + // entry may reference it. + _, err := client.CreateMCPServerConfig(ctx, firstUser.OrganizationID, codersdk.CreateMCPServerConfigRequest{ + DisplayName: "Audit Discovery Failure", + Slug: "audit-discovery-failure", + Transport: "streamable_http", + URL: "http://127.0.0.1:1", + AuthType: "oauth2", + Availability: "default_on", + Enabled: true, + }) + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) + require.Empty(t, mAudit.AuditLogs()) + }) + + t.Run("CreateNotAuditedWhenInsertFails", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + mAudit := audit.NewMock() + providerKeys := coderdtest.FakeOpenAICompatProviderAPIKeys(t) + db, ps := dbtestutil.NewDB(t) + store := &failingMCPServerConfigInsertStore{Store: db} + client := coderdtest.New(t, &coderdtest.Options{ + DeploymentValues: mcpDeploymentValues(t), + ChatProviderAPIKeys: &providerKeys, + Auditor: mAudit, + Database: store, + Pubsub: ps, + }) + firstUser := coderdtest.CreateFirstUser(t, client) + + 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": "` + r.Host + `", + "authorization_endpoint": "` + "http://" + r.Host + `/authorize", + "token_endpoint": "` + "http://" + r.Host + `/token", + "registration_endpoint": "` + "http://" + r.Host + `/register", + "response_types_supported": ["code"] + }`)) + case "/register": + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{ + "client_id": "update-failure-client-id", + "client_secret": "update-failure-client-secret" + }`)) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(authServer.Close) + + mcpServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/.well-known/oauth-protected-resource/v1/mcp": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "resource": "` + "http://" + r.Host + `", + "authorization_servers": ["` + authServer.URL + `"] + }`)) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(mcpServer.Close) + + store.fail.Store(true) + mAudit.ResetLogs() + _, err := client.CreateMCPServerConfig(ctx, firstUser.OrganizationID, codersdk.CreateMCPServerConfigRequest{ + DisplayName: "Audit Update Failure", + Slug: "audit-update-failure", + Transport: "streamable_http", + URL: mcpServer.URL + "/v1/mcp", + AuthType: "oauth2", + Availability: "default_on", + Enabled: true, + }) + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + require.Equal(t, http.StatusInternalServerError, sdkErr.StatusCode()) + + configs, err := client.MCPServerConfigs(ctx, firstUser.OrganizationID) + require.NoError(t, err) + require.Empty(t, configs) + require.Empty(t, mAudit.AuditLogs()) + }) + + t.Run("DeletedResourceMarked", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, _ := newAuditedMCPClient(t) + firstUser := coderdtest.CreateFirstUser(t, client) + config := createMCPServerConfig(t, client, firstUser.OrganizationID, "audit-is-deleted", true) + deletedID := uuid.New() + + err := client.CreateTestAuditLog(ctx, codersdk.CreateTestAuditLogRequest{ + OrganizationID: firstUser.OrganizationID, + Action: codersdk.AuditActionWrite, + ResourceType: codersdk.ResourceTypeMCPServerConfig, + ResourceID: config.ID, + }) + require.NoError(t, err) + err = client.CreateTestAuditLog(ctx, codersdk.CreateTestAuditLogRequest{ + OrganizationID: firstUser.OrganizationID, + Action: codersdk.AuditActionDelete, + ResourceType: codersdk.ResourceTypeMCPServerConfig, + ResourceID: deletedID, + }) + require.NoError(t, err) + + logs, err := client.AuditLogs(ctx, codersdk.AuditLogsRequest{ + Pagination: codersdk.Pagination{Limit: 25}, + }) + require.NoError(t, err) + byResourceID := make(map[uuid.UUID]codersdk.AuditLog, len(logs.AuditLogs)) + for _, alog := range logs.AuditLogs { + byResourceID[alog.ResourceID] = alog + } + require.Contains(t, byResourceID, config.ID) + require.False(t, byResourceID[config.ID].IsDeleted) + require.Contains(t, byResourceID, deletedID) + require.True(t, byResourceID[deletedID].IsDeleted) + }) + + t.Run("WriteDeniedAudited", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, mAudit := newAuditedMCPClient(t) + firstUser := coderdtest.CreateFirstUser(t, client) + memberClient, member := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID) + config := createMCPServerConfig(t, client, firstUser.OrganizationID, "audit-denied", true) + + mAudit.ResetLogs() + newName := "denied" + _, err := memberClient.UpdateMCPServerConfig(ctx, firstUser.OrganizationID, config.ID, codersdk.UpdateMCPServerConfigRequest{ + DisplayName: &newName, + }) + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + require.Equal(t, http.StatusForbidden, sdkErr.StatusCode()) + + logs := mAudit.AuditLogs() + require.Len(t, logs, 1) + require.Equal(t, database.AuditActionWrite, logs[0].Action) + require.Equal(t, database.ResourceTypeMCPServerConfig, logs[0].ResourceType) + require.Equal(t, config.ID, logs[0].ResourceID) + require.Equal(t, member.ID, logs[0].UserID) + require.Equal(t, firstUser.OrganizationID, logs[0].OrganizationID) + require.EqualValues(t, http.StatusForbidden, logs[0].StatusCode) + }) + + t.Run("DeleteDeniedAudited", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + client, mAudit := newAuditedMCPClient(t) + firstUser := coderdtest.CreateFirstUser(t, client) + memberClient, member := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID) + config := createMCPServerConfig(t, client, firstUser.OrganizationID, "audit-delete-denied", true) + + mAudit.ResetLogs() + err := memberClient.DeleteMCPServerConfig(ctx, firstUser.OrganizationID, config.ID) + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + require.Equal(t, http.StatusForbidden, sdkErr.StatusCode()) + + logs := mAudit.AuditLogs() + require.Len(t, logs, 1) + require.Equal(t, database.AuditActionDelete, logs[0].Action) + require.Equal(t, database.ResourceTypeMCPServerConfig, logs[0].ResourceType) + require.Equal(t, config.ID, logs[0].ResourceID) + require.Equal(t, member.ID, logs[0].UserID) + require.Equal(t, firstUser.OrganizationID, logs[0].OrganizationID) + require.EqualValues(t, http.StatusForbidden, logs[0].StatusCode) + }) +} + +// failingMCPServerConfigInsertStore fails config inserts once armed. +type failingMCPServerConfigInsertStore struct { + database.Store + + fail atomic.Bool +} + +func (s *failingMCPServerConfigInsertStore) InsertMCPServerConfig(ctx context.Context, arg database.InsertMCPServerConfigParams) (database.MCPServerConfig, error) { + if s.fail.Load() { + return database.MCPServerConfig{}, xerrors.New("injected insert failure") + } + return s.Store.InsertMCPServerConfig(ctx, arg) +} + +// staleMCPServerConfigReadStore corrupts plain config reads once armed, +// simulating a concurrent update that outdates the param middleware's +// snapshot. Locked ForUpdate reads stay untouched. +type staleMCPServerConfigReadStore struct { + database.Store + + stale atomic.Bool +} + +func (s *staleMCPServerConfigReadStore) GetMCPServerConfigByID(ctx context.Context, id uuid.UUID) (database.MCPServerConfig, error) { + config, err := s.Store.GetMCPServerConfigByID(ctx, id) + if err == nil && s.stale.Load() { + config.DisplayName = "stale middleware snapshot" + } + return config, err +} + func TestMCPServerConfigsNonAdmin(t *testing.T) { t.Parallel() diff --git a/codersdk/audit.go b/codersdk/audit.go index 7ee41ce515..96b718a86a 100644 --- a/codersdk/audit.go +++ b/codersdk/audit.go @@ -53,6 +53,7 @@ const ( ResourceTypeGroupAIBudget ResourceType = "group_ai_budget" ResourceTypeUserAIBudgetOverride ResourceType = "user_ai_budget_override" ResourceTypeChat ResourceType = "chat" + ResourceTypeMCPServerConfig ResourceType = "mcp_server_config" ResourceTypeUserSecret ResourceType = "user_secret" ResourceTypeUserSkill ResourceType = "user_skill" ResourceTypeChatInstructionSettings ResourceType = "chat_instruction_settings" @@ -130,6 +131,8 @@ func (r ResourceType) FriendlyString() string { return "user ai budget override" case ResourceTypeChat: return "chat" + case ResourceTypeMCPServerConfig: + return "mcp server config" case ResourceTypeUserSecret: return "user secret" case ResourceTypeUserSkill: diff --git a/docs/admin/security/audit-logs.md b/docs/admin/security/audit-logs.md index 27082192dc..73dce51db7 100644 --- a/docs/admin/security/audit-logs.md +++ b/docs/admin/security/audit-logs.md @@ -32,6 +32,7 @@ We track the following resources: | GroupSyncSettings
| |
FieldTracked
auto_create_missing_groupstrue
fieldtrue
legacy_group_name_mappingfalse
mappingtrue
regex_filtertrue
| | HealthSettings
| |
FieldTracked
dismissed_healthcheckstrue
idfalse
| | License
create, delete | |
FieldTracked
exptrue
idfalse
jwtfalse
uploaded_attrue
uuidtrue
| +| MCPServerConfig
create, write, delete | |
FieldTracked
allow_in_plan_modetrue
api_key_headertrue
api_key_valuetrue
api_key_value_key_idfalse
auth_typetrue
availabilitytrue
created_atfalse
created_bytrue
custom_headerstrue
custom_headers_key_idfalse
descriptiontrue
display_nametrue
enabledtrue
forward_coder_headerstrue
icon_urltrue
idfalse
model_intenttrue
oauth2_auth_urltrue
oauth2_client_idtrue
oauth2_client_secrettrue
oauth2_client_secret_key_idfalse
oauth2_revocation_urltrue
oauth2_scopestrue
oauth2_token_urltrue
organization_idfalse
slugtrue
tool_allow_listtrue
tool_deny_listtrue
transporttrue
updated_atfalse
updated_bytrue
urltrue
| | NotificationTemplate
| |
FieldTracked
actionstrue
body_templatetrue
enabled_by_defaulttrue
grouptrue
idfalse
kindtrue
methodtrue
nametrue
title_templatetrue
| | NotificationsSettings
| |
FieldTracked
idfalse
notifier_pausedtrue
| | OAuth2ProviderApp
| |
FieldTracked
callback_urltrue
client_id_issued_atfalse
client_secret_expires_attrue
client_typetrue
client_uritrue
contactstrue
created_atfalse
dynamically_registeredtrue
grant_typestrue
icontrue
idfalse
jwkstrue
jwks_uritrue
logo_uritrue
nametrue
policy_uritrue
redirect_uristrue
registration_access_tokentrue
registration_client_uritrue
response_typestrue
scopetrue
software_idtrue
software_versiontrue
token_endpoint_auth_methodtrue
tos_uritrue
updated_atfalse
| diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 840463805f..2ca7cad6d5 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -11704,9 +11704,9 @@ Git clone makes use of this by parsing the URL from: 'Username for "https://gith #### Enumerated Values -| Value(s) | -|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `ai_gateway_key`, `ai_provider`, `ai_provider_key`, `ai_seat`, `api_key`, `chat`, `chat_instruction_settings`, `convert_login`, `custom_role`, `git_ssh_key`, `group`, `group_ai_budget`, `health_settings`, `idp_sync_settings_group`, `idp_sync_settings_organization`, `idp_sync_settings_role`, `license`, `notification_template`, `notifications_settings`, `oauth2_provider_app`, `oauth2_provider_app_secret`, `oauth2_provider_settings`, `organization`, `organization_member`, `prebuilds_settings`, `task`, `template`, `template_version`, `user`, `user_ai_budget_override`, `user_secret`, `user_skill`, `workspace`, `workspace_agent`, `workspace_app`, `workspace_build`, `workspace_proxy` | +| Value(s) | +|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `ai_gateway_key`, `ai_provider`, `ai_provider_key`, `ai_seat`, `api_key`, `chat`, `chat_instruction_settings`, `convert_login`, `custom_role`, `git_ssh_key`, `group`, `group_ai_budget`, `health_settings`, `idp_sync_settings_group`, `idp_sync_settings_organization`, `idp_sync_settings_role`, `license`, `mcp_server_config`, `notification_template`, `notifications_settings`, `oauth2_provider_app`, `oauth2_provider_app_secret`, `oauth2_provider_settings`, `organization`, `organization_member`, `prebuilds_settings`, `task`, `template`, `template_version`, `user`, `user_ai_budget_override`, `user_secret`, `user_skill`, `workspace`, `workspace_agent`, `workspace_app`, `workspace_build`, `workspace_proxy` | ## codersdk.Response diff --git a/enterprise/audit/diff.go b/enterprise/audit/diff.go index 8196238ecc..970767184a 100644 --- a/enterprise/audit/diff.go +++ b/enterprise/audit/diff.go @@ -73,7 +73,7 @@ func diffValues(left, right any, table Table) audit.Map { leftI, rightI = leftF.Interface(), rightF.Interface() } - if !reflect.DeepEqual(leftI, rightI) { + if !auditValuesEqual(rightT, diffName, leftI, rightI) { switch atype { case ActionTrack: baseDiff[diffName] = audit.OldNew{Old: leftI, New: rightI} @@ -90,6 +90,29 @@ func diffValues(left, right any, table Table) audit.Map { return baseDiff } +func auditValuesEqual(resourceType reflect.Type, fieldName string, left, right any) bool { + if reflect.DeepEqual(left, right) { + return true + } + if resourceType != reflect.TypeFor[database.MCPServerConfig]() { + return false + } + + switch fieldName { + case "custom_headers": + leftString, leftOK := left.(string) + rightString, rightOK := right.(string) + return leftOK && rightOK && (leftString == "" && rightString == "{}" || + leftString == "{}" && rightString == "") + case "tool_allow_list", "tool_deny_list": + leftStrings, leftOK := left.([]string) + rightStrings, rightOK := right.([]string) + return leftOK && rightOK && len(leftStrings) == 0 && len(rightStrings) == 0 + default: + return false + } +} + // convertDiffType converts external struct types to primitive types. // //nolint:forcetypeassert diff --git a/enterprise/audit/diff_internal_test.go b/enterprise/audit/diff_internal_test.go index 3a4eea8480..ae5dd2ea63 100644 --- a/enterprise/audit/diff_internal_test.go +++ b/enterprise/audit/diff_internal_test.go @@ -2,6 +2,7 @@ package audit import ( "database/sql" + "encoding/json" "reflect" "testing" "time" @@ -559,6 +560,133 @@ func Test_diff(t *testing.T) { exp: audit.Map{}, }, }) + + runDiffTests(t, []diffTest{ + { + name: "CreateEmptyStoredValues", + left: audit.Empty[database.MCPServerConfig](), + right: database.MCPServerConfig{ + CustomHeaders: "{}", + ToolAllowList: []string{}, + ToolDenyList: []string{}, + }, + exp: audit.Map{}, + }, + { + name: "Create", + left: audit.Empty[database.MCPServerConfig](), + right: database.MCPServerConfig{ + ID: uuid.UUID{1}, + DisplayName: "GitHub MCP", + Slug: "github", + Url: "https://mcp.example.com/v1", + AuthType: "api_key", + APIKeyHeader: "X-Api-Key", + APIKeyValue: "plaintext-api-key", + CustomHeaders: `{"Authorization":"Bearer plaintext-header"}`, + ToolAllowList: []string{"issues"}, + ToolDenyList: []string{"delete_repository"}, + OAuth2ClientSecret: "plaintext-oauth-secret", + Enabled: true, + CreatedBy: uuid.NullUUID{UUID: uuid.UUID{2}, Valid: true}, + UpdatedBy: uuid.NullUUID{UUID: uuid.UUID{2}, Valid: true}, + OrganizationID: uuid.UUID{4}, + }, + exp: audit.Map{ + "display_name": audit.OldNew{Old: "", New: "GitHub MCP"}, + "slug": audit.OldNew{Old: "", New: "github"}, + "url": audit.OldNew{Old: "", New: "https://mcp.example.com/v1"}, + "auth_type": audit.OldNew{Old: "", New: "api_key"}, + "api_key_header": audit.OldNew{Old: "", New: "X-Api-Key"}, + "api_key_value": audit.OldNew{Old: "", New: "", Secret: true}, + "custom_headers": audit.OldNew{Old: "", New: "", Secret: true}, + "tool_allow_list": audit.OldNew{Old: []string(nil), New: []string{"issues"}}, + "tool_deny_list": audit.OldNew{Old: []string(nil), New: []string{"delete_repository"}}, + "oauth2_client_secret": audit.OldNew{Old: "", New: "", Secret: true}, + "enabled": audit.OldNew{Old: false, New: true}, + "created_by": audit.OldNew{Old: "null", New: uuid.UUID{2}.String()}, + "updated_by": audit.OldNew{Old: "null", New: uuid.UUID{2}.String()}, + }, + }, + { + name: "CustomHeadersAdded", + left: database.MCPServerConfig{ + CustomHeaders: "{}", + }, + right: database.MCPServerConfig{ + CustomHeaders: `{"Authorization":"Bearer plaintext-header"}`, + }, + exp: audit.Map{ + "custom_headers": audit.OldNew{Old: "", New: "", Secret: true}, + }, + }, + { + name: "CustomHeadersRemoved", + left: database.MCPServerConfig{ + CustomHeaders: `{"Authorization":"Bearer plaintext-header"}`, + }, + right: database.MCPServerConfig{ + CustomHeaders: "{}", + }, + exp: audit.Map{ + "custom_headers": audit.OldNew{Old: "", New: "", Secret: true}, + }, + }, + { + name: "SecretRotationRedacted", + left: database.MCPServerConfig{ + ID: uuid.UUID{1}, + DisplayName: "GitHub MCP", + AuthType: "api_key", + APIKeyValue: "old-plaintext-api-key", + APIKeyValueKeyID: sql.NullString{String: "key-1", Valid: true}, + CustomHeaders: `{"Authorization":"Bearer old-plaintext"}`, + CustomHeadersKeyID: sql.NullString{String: "key-1", Valid: true}, + OrganizationID: uuid.UUID{4}, + }, + right: database.MCPServerConfig{ + ID: uuid.UUID{1}, + DisplayName: "Renamed MCP", + AuthType: "api_key", + APIKeyValue: "new-plaintext-api-key", + CustomHeaders: `{"Authorization":"Bearer new-plaintext"}`, + OrganizationID: uuid.UUID{4}, + }, + exp: audit.Map{ + "display_name": audit.OldNew{Old: "GitHub MCP", New: "Renamed MCP"}, + "api_key_value": audit.OldNew{Old: "", New: "", Secret: true}, + "custom_headers": audit.OldNew{Old: "", New: "", Secret: true}, + }, + }, + }) +} + +func Test_mcpServerConfigSecretsNeverSerialized(t *testing.T) { + t.Parallel() + + secrets := []string{ + "plaintext-oauth-secret", + "plaintext-api-key", + "Bearer plaintext-header", + } + left := audit.Empty[database.MCPServerConfig]() + right := database.MCPServerConfig{ + ID: uuid.UUID{1}, + DisplayName: "GitHub MCP", + AuthType: "oauth2", + OAuth2ClientID: "client-id", + OAuth2ClientSecret: secrets[0], + APIKeyValue: secrets[1], + CustomHeaders: `{"Authorization":"` + secrets[2] + `"}`, + OrganizationID: uuid.UUID{4}, + } + + raw, err := json.Marshal(diffValues(left, right, AuditableResources)) + require.NoError(t, err) + for _, secret := range secrets { + require.NotContains(t, string(raw), secret) + } + require.Contains(t, string(raw), "client-id") } func runDiffTests(t *testing.T, tests []diffTest) { diff --git a/enterprise/audit/table.go b/enterprise/audit/table.go index 4e1082a67b..2eb419decf 100644 --- a/enterprise/audit/table.go +++ b/enterprise/audit/table.go @@ -35,6 +35,7 @@ var AuditActionMap = map[string][]codersdk.AuditAction{ "AuditableGroupAIBudget": {codersdk.AuditActionWrite, codersdk.AuditActionDelete}, "AuditableUserAIBudgetOverride": {codersdk.AuditActionWrite, codersdk.AuditActionDelete}, "Chat": {codersdk.AuditActionCreate, codersdk.AuditActionWrite}, // chats get 'archived' by users, not deleted. + "MCPServerConfig": {codersdk.AuditActionCreate, codersdk.AuditActionWrite, codersdk.AuditActionDelete}, "UserSecret": {codersdk.AuditActionCreate, codersdk.AuditActionWrite, codersdk.AuditActionDelete}, "UserSkill": {codersdk.AuditActionCreate, codersdk.AuditActionWrite, codersdk.AuditActionDelete}, "ChatInstructionSettings": {codersdk.AuditActionWrite}, @@ -501,6 +502,40 @@ var auditableResourcesTypes = map[any]map[string]Action{ "requires_action_deadline_at": ActionIgnore, // Internal pending-action deadline. "compaction_requested_at": ActionIgnore, // Internal one-shot manual compaction signal. }, + &database.MCPServerConfig{}: { + "id": ActionIgnore, // Conveyed by resource_id, not useful in a diff. + "display_name": ActionTrack, + "slug": ActionTrack, + "description": ActionTrack, + "icon_url": ActionTrack, + "transport": ActionTrack, + "url": ActionTrack, + "auth_type": ActionTrack, + "oauth2_client_id": ActionTrack, + "oauth2_client_secret": ActionSecret, + "oauth2_client_secret_key_id": ActionIgnore, // dbcrypt bookkeeping. + "oauth2_auth_url": ActionTrack, + "oauth2_token_url": ActionTrack, + "oauth2_scopes": ActionTrack, + "api_key_header": ActionTrack, + "api_key_value": ActionSecret, + "api_key_value_key_id": ActionIgnore, // dbcrypt bookkeeping. + "custom_headers": ActionSecret, // May contain credentials + "custom_headers_key_id": ActionIgnore, // dbcrypt bookkeeping. + "tool_allow_list": ActionTrack, + "tool_deny_list": ActionTrack, + "availability": ActionTrack, + "enabled": ActionTrack, + "created_by": ActionTrack, + "updated_by": ActionTrack, + "created_at": ActionIgnore, + "updated_at": ActionIgnore, + "model_intent": ActionTrack, + "allow_in_plan_mode": ActionTrack, + "forward_coder_headers": ActionTrack, + "oauth2_revocation_url": ActionTrack, + "organization_id": ActionIgnore, + }, &database.UserSkill{}: { "id": ActionTrack, "user_id": ActionTrack, diff --git a/enterprise/coderd/mcp_test.go b/enterprise/coderd/mcp_test.go index 353b5b37e5..02801b5e64 100644 --- a/enterprise/coderd/mcp_test.go +++ b/enterprise/coderd/mcp_test.go @@ -11,6 +11,7 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/require" + "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" @@ -103,7 +104,11 @@ func TestMCPServerConfigCollectionOrganizationIsolation(t *testing.T) { func TestMCPServerConfigItemCrossOrganizationConcealment(t *testing.T) { t.Parallel() + mAudit := audit.NewMock() client, firstUser := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + Auditor: mAudit, + }, LicenseOptions: &coderdenttest.LicenseOptions{ Features: license.Features{ codersdk.FeatureMultipleOrganizations: 1, @@ -115,6 +120,7 @@ func TestMCPServerConfigItemCrossOrganizationConcealment(t *testing.T) { config := createMCPServerConfigForOrganization(t, client, firstUser.OrganizationID, "private-org-one-mcp") organizationPath := "/api/experimental/organizations/" + secondOrg.ID.String() + "/mcp-servers/" + config.ID.String() frozenPath := "/api/experimental/mcp/servers/" + config.ID.String() + mAudit.ResetLogs() for _, test := range []struct { name string @@ -140,6 +146,13 @@ func TestMCPServerConfigItemCrossOrganizationConcealment(t *testing.T) { wantStatus = http.StatusNotFound } requireMCPServerConfigRequestStatus(t, otherClient, test.method, test.path, test.body, wantStatus) + + // Read-gated routes 404 in the param middleware before any + // handler runs, and disconnect does not audit, so nothing + // is audited. + for _, log := range mAudit.AuditLogs() { + require.NotEqual(t, database.ResourceTypeMCPServerConfig, log.ResourceType) + } }) } diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 8c93fd9e50..058ae1d859 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -7968,6 +7968,7 @@ export type ResourceType = | "idp_sync_settings_organization" | "idp_sync_settings_role" | "license" + | "mcp_server_config" | "notification_template" | "notifications_settings" | "oauth2_provider_app" @@ -8007,6 +8008,7 @@ export const ResourceTypes: ResourceType[] = [ "idp_sync_settings_organization", "idp_sync_settings_role", "license", + "mcp_server_config", "notification_template", "notifications_settings", "oauth2_provider_app",