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

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

Registering a URL that already exists takes the upsert branch and rewrites the
live row — name, transport, headers, timeout, enabled, auth type, the
connection reset, and the URL's query string, since the server id hashes only
origin and pathname. That branch recorded no audit row at all: the ADDED audit
was gated on `!result.updated`. main recorded ADDED for these (wrong action,
but a row existed), so this restores coverage and fixes the action.

Reachable from the settings POST /api/mcp/servers and from Copilot's
manage_mcp_tool `add`, neither of which passes existingServerBehavior. The v2
POST passes 'reject' so it only reaches the upsert on a revival.

A rewrite is now MCP_SERVER_UPDATED carrying updatedFields; a revival of a
soft-deleted row stays MCP_SERVER_ADDED. updateValues is typed
Partial<$inferInsert> so Object.keys is column-safe. Analytics gating is
unchanged: mcp_server_connected still fires only for a genuine insert.

* fix(mcp): redact audit URLs and drop unwritten columns from updatedFields

Two review findings on the new upsert audit.

The upsert assigns every column unconditionally, so `description` is present
on updateValues but undefined when the registration omits it. Drizzle skips
undefined in .set(), so deriving keys without checking values made the audit
claim a column the write never touched. Filter by value; null stays, since
clearing a value is a write.

MCP URLs carry tokens in their query string — that is why a silent rewrite of
one matters — and audit rows are readable by org admins who need no workspace
MCP access. Newly auditing rewrites would persist those tokens verbatim, so
every MCP audit row now records the URL through sanitizeUrlForLog, which
strips query and fragment. Applied to the add, update and delete rows alike:
redacting only the new path would leave the same credential in the row a
first registration already writes. A null url stays null rather than becoming
an empty string.

Author: waleedlatif1

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,