fix(mcp): audit an upsert that rewrites or revives a server

apps/sim/lib/mcp/application/use-cases.ts

@@ -4,6 +4,7 @@ import { getPostgresErrorCode } from '@sim/utils/errors'
 import type { ListSortOrder } from '@/lib/api/list-query'
 import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application'
 import { OrchestrationError } from '@/lib/core/orchestration/types'
+import { sanitizeUrlForLog } from '@/lib/core/utils/logging'
 import { mcpServerDelegationPolicy } from '@/lib/mcp/application/authorization'
 import { mcpServerOperations } from '@/lib/mcp/application/operations'
 import {
@@ -176,25 +177,32 @@ async function saveMcpServer(args: {
  return requireSuccessfulResult(result, 'Failed to register MCP server')
 }
 
+/**
+ * A registration is an addition when it inserts a row or revives a soft-deleted
+ * one, and an update when it rewrites a live row — which `registerMcpServer`
+ * allows, repointing headers and the URL's query string. Auditing only the
+ * insert left both upsert outcomes unrecorded.
+ */
 function createAudit(
  input: SaveMcpServerInput,
  result: PerformMcpServerResult & { server: McpServerRow }
 ) {
-if (result.updated) return []
+const isRewrite = result.updated === true && !result.revived
  return [
  {
- action: AuditAction.MCP_SERVER_ADDED,
+ action: isRewrite ? AuditAction.MCP_SERVER_UPDATED : AuditAction.MCP_SERVER_ADDED,
  resourceType: AuditResourceType.MCP_SERVER,
  resourceId: result.server.id,
  resourceName: result.server.name,
- description: `Added MCP server "${result.server.name}"`,
+ description: `${isRewrite ? 'Updated' : 'Added'} MCP server "${result.server.name}"`,
  metadata: {
  serverName: result.server.name,
  transport: result.server.transport,
- url: result.server.url,
+ url: result.server.url ? sanitizeUrlForLog(result.server.url) : null,
  timeout: result.server.timeout,
  retries: result.server.retries,
  source: input.source,
+ ...(isRewrite ? { updatedFields: result.updatedFields ?? [] } : {}),
  },
  },
  ]
@@ -314,7 +322,7 @@ function updateAudit(
  metadata: {
  serverName: result.server.name,
  transport: result.server.transport,
- url: result.server.url,
+ url: result.server.url ? sanitizeUrlForLog(result.server.url) : null,
  updatedFields: result.updatedFields ?? [],
  source: input.source,
  },
@@ -382,7 +390,7 @@ export const deleteMcpServerUseCase = defineAuthorizedWorkspaceUseCase({
  metadata: {
  serverName: result.server.name,
  transport: result.server.transport,
- url: result.server.url,
+ url: result.server.url ? sanitizeUrlForLog(result.server.url) : null,
  source: input.source,
  },
  }),

apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts

@@ -58,6 +58,7 @@ vi.mock('@/lib/mcp/service', () => ({
 vi.mock('@/lib/mcp/utils', () => ({ generateMcpServerId: mockGenerateMcpServerId }))
 vi.mock('@/lib/posthog/server', () => posthogServerMock)
 
+import { AuditAction } from '@sim/audit'
 import {
  performCreateMcpServer,
  performDeleteMcpServer,
@@ -67,6 +68,10 @@ import {
 describe('MCP server lifecycle orchestration', () => {
  const auditUpdatedFields = (): string[] | undefined =>
  auditMockFns.mockRecordAudit.mock.calls.at(-1)?.[0].metadata.updatedFields
+ const auditAction = (): string | undefined =>
+ auditMockFns.mockRecordAudit.mock.calls.at(-1)?.[0].action
+ const auditMetadata = (): Record<string, unknown> | undefined =>
+ auditMockFns.mockRecordAudit.mock.calls.at(-1)?.[0].metadata
 
  beforeEach(() => {
  vi.clearAllMocks()
@@ -245,6 +250,90 @@ describe('MCP server lifecycle orchestration', () => {
  expect(mockRevokeOauthTokens).toHaveBeenCalledWith('server-1', 'workspace-1')
  })
 
+ it('audits a re-registration that rewrites a live server as an update', async () => {
+ mockGenerateMcpServerId.mockReturnValue('server-1')
+ dbChainMockFns.limit.mockResolvedValueOnce([
+ {
+ id: 'server-1',
+ deletedAt: null,
+ url: 'https://example.com/mcp?token=old',
+ authType: 'headers',
+ oauthClientId: null,
+ oauthClientSecret: null,
+ },
+ ])
+ dbChainMockFns.limit.mockResolvedValueOnce([
+ {
+ id: 'server-1',
+ workspaceId: 'workspace-1',
+ name: 'Example',
+ transport: 'streamable-http',
+ url: 'https://example.com/mcp?token=new',
+ authType: 'headers',
+ },
+ ])
+
+ // The server id hashes origin + pathname only, so a different query string
+ // lands on the same row and repoints it.
+ const result = await performCreateMcpServer({
+ workspaceId: 'workspace-1',
+ userId: 'user-1',
+ name: 'Example',
+ url: 'https://example.com/mcp?token=new',
+ headers: { authorization: 'Bearer rotated' },
+ })
+
+ expect(result.success).toBe(true)
+ expect(result.updated).toBe(true)
+ expect(result.revived).toBe(false)
+ expect(auditAction()).toBe(AuditAction.MCP_SERVER_UPDATED)
+ expect(auditUpdatedFields()).toEqual(expect.arrayContaining(['url', 'headers']))
+ // The registration omitted `description`, and Drizzle skips undefined in
+ // .set(), so the audit must not claim that column was written.
+ expect(auditUpdatedFields()).not.toContain('description')
+ // A query string routinely carries the endpoint's token, and audit rows are
+ // readable by org admins who need no workspace MCP access.
+ expect(auditMetadata()?.url).toBe('https://example.com/mcp')
+ })
+
+ it('audits a re-registration that revives a soft-deleted server as an addition', async () => {
+ mockGenerateMcpServerId.mockReturnValue('server-1')
+ dbChainMockFns.limit.mockResolvedValueOnce([
+ {
+ id: 'server-1',
+ deletedAt: new Date(),
+ url: 'https://example.com/mcp',
+ authType: 'headers',
+ oauthClientId: null,
+ oauthClientSecret: null,
+ },
+ ])
+ dbChainMockFns.limit.mockResolvedValueOnce([
+ {
+ id: 'server-1',
+ workspaceId: 'workspace-1',
+ name: 'Example',
+ transport: 'streamable-http',
+ url: 'https://example.com/mcp',
+ authType: 'headers',
+ },
+ ])
+
+ const result = await performCreateMcpServer({
+ workspaceId: 'workspace-1',
+ userId: 'user-1',
+ name: 'Example',
+ url: 'https://example.com/mcp',
+ })
+
+ expect(result.success).toBe(true)
+ expect(result.revived).toBe(true)
+ // Bringing a deleted server back is an addition, so it keeps the ADDED action
+ // and carries no updatedFields.
+ expect(auditAction()).toBe(AuditAction.MCP_SERVER_ADDED)
+ expect(auditUpdatedFields()).toBeUndefined()
+ })
+
  it('evicts the deleted server from the connection pool (row is already gone from clearCache)', async () => {
  dbChainMockFns.returning.mockResolvedValueOnce([
  { id: 'server-1', workspaceId: 'workspace-1', name: 'Example', transport: 'streamable-http' },

apps/sim/lib/mcp/orchestration/server-lifecycle.ts

@@ -6,6 +6,7 @@ import { generateId } from '@sim/utils/id'
 import { and, eq, isNull } from 'drizzle-orm'
 import type { NextRequest } from 'next/server'
 import { encryptSecret } from '@/lib/core/security/encryption'
+import { sanitizeUrlForLog } from '@/lib/core/utils/logging'
 import {
  McpDnsResolutionError,
  McpDomainNotAllowedError,
@@ -89,6 +90,12 @@ export interface PerformMcpServerResult {
  serverId?: string
  server?: typeof mcpServers.$inferSelect
  updated?: boolean
+ /**
+ * Whether an `updated` result brought a soft-deleted row back rather than
+ * rewriting a live one. The two need different audit actions: a revival is an
+ * addition, a rewrite is an update.
+ */
+ revived?: boolean
  authType?: McpAuthType
  configurationChanged?: boolean
  /**
@@ -204,11 +211,12 @@ export async function createMcpServer(
 
  if (shouldClearOauth) await revokeMcpOauthTokens(serverId, params.workspaceId)
 
+ let updatedFields: string[] = []
  await db.transaction(async (tx) => {
  if (shouldClearOauth) {
  await tx.delete(mcpServerOauth).where(eq(mcpServerOauth.mcpServerId, serverId))
  }
- const updateValues: Record<string, unknown> = {
+ const updateValues: Partial<typeof mcpServers.$inferInsert> = {
  name: params.name,
  description: params.description,
  transport,
@@ -238,6 +246,16 @@ export async function createMcpServer(
  if (params.oauthClientSecretProvided) {
  updateValues.oauthClientSecret = oauthClientSecretEncrypted
  }
+ /**
+ * Drizzle skips `undefined` in `.set()`, and this object assigns every
+ * column unconditionally — `description` is present but undefined when
+ * the registration omits it. Keys must therefore be filtered by value,
+ * or the audit claims a column the write never touched. `null` stays:
+ * clearing a value is a write.
+ */
+ updatedFields = Object.entries(updateValues)
+ .filter(([key, value]) => key !== 'updatedAt' && value !== undefined)
+ .map(([key]) => key)
  await tx.update(mcpServers).set(updateValues).where(eq(mcpServers.id, serverId))
  })
 
@@ -247,7 +265,15 @@ export async function createMcpServer(
  .where(and(eq(mcpServers.id, serverId), eq(mcpServers.workspaceId, params.workspaceId)))
  .limit(1)
  if (!server) throw new Error(`MCP server ${serverId} missing after a successful update`)
- return { success: true, serverId, server, updated: true, authType: resolvedAuthType }
+ return {
+ success: true,
+ serverId,
+ server,
+ updated: true,
+ revived: isRevival,
+ updatedFields,
+ authType: resolvedAuthType,
+ }
  }
 
  await db.insert(mcpServers).values({
@@ -447,8 +473,8 @@ export async function performCreateMcpServer(
  workspaceId: params.workspaceId,
  result,
  })
+ const source = legacySource(params.source)
  if (!result.updated) {
- const source = legacySource(params.source)
  captureServerEvent(
  params.userId,
  'mcp_server_connected',
@@ -463,27 +489,36 @@ export async function performCreateMcpServer(
  setOnce: { first_mcp_connected_at: new Date().toISOString() },
  }
  )
- recordAudit({
- workspaceId: params.workspaceId,
- actorId: params.userId,
- actorName: params.actorName ?? undefined,
- actorEmail: params.actorEmail ?? undefined,
- action: AuditAction.MCP_SERVER_ADDED,
- resourceType: AuditResourceType.MCP_SERVER,
- resourceId: result.server.id,
- resourceName: result.server.name,
- description: `Added MCP server "${result.server.name}"`,
- metadata: {
- serverName: result.server.name,
- transport: result.server.transport,
- url: result.server.url,
- timeout: result.server.timeout,
- retries: result.server.retries,
- source,
- },
- request: params.request,
- })
  }
+
+ /**
+ * Registering a URL that already exists rewrites the live row — headers, the
+ * URL's query string, transport, enabled — so it is an update, not an
+ * addition. Reviving a soft-deleted row is still an addition. Auditing only
+ * the insert left both cases with no trace at all.
+ */
+ const isRewrite = result.updated === true && !result.revived
+ recordAudit({
+ workspaceId: params.workspaceId,
+ actorId: params.userId,
+ actorName: params.actorName ?? undefined,
+ actorEmail: params.actorEmail ?? undefined,
+ action: isRewrite ? AuditAction.MCP_SERVER_UPDATED : AuditAction.MCP_SERVER_ADDED,
+ resourceType: AuditResourceType.MCP_SERVER,
+ resourceId: result.server.id,
+ resourceName: result.server.name,
+ description: `${isRewrite ? 'Updated' : 'Added'} MCP server "${result.server.name}"`,
+ metadata: {
+ serverName: result.server.name,
+ transport: result.server.transport,
+ url: result.server.url ? sanitizeUrlForLog(result.server.url) : null,
+ timeout: result.server.timeout,
+ retries: result.server.retries,
+ source,
+ ...(isRewrite ? { updatedFields: result.updatedFields ?? [] } : {}),
+ },
+ request: params.request,
+ })
  return result
  } catch (error) {
  logger.error('Failed to register MCP server', { error })
@@ -512,7 +547,7 @@ export async function performUpdateMcpServer(
  metadata: {
  serverName: result.server.name,
  transport: result.server.transport,
- url: result.server.url,
+ url: result.server.url ? sanitizeUrlForLog(result.server.url) : null,
  updatedFields: result.updatedFields ?? [],
  },
  request: params.request,
@@ -561,7 +596,7 @@ export async function performDeleteMcpServer(
  metadata: {
  serverName: result.server.name,
  transport: result.server.transport,
- url: result.server.url,
+ url: result.server.url ? sanitizeUrlForLog(result.server.url) : null,
  source,
  },
  request: params.request,