From 3bf7a0d7ee6bee6be864b8f6c9e75c900d9b9d0e Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 11 Aug 2026 21:37:21 -0700 Subject: [PATCH] fix(mcp): audit the columns an MCP server update wrote, not the params it got MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every PATCH /api/mcp/servers/[id] audit row listed oauthClientId, oauthClientIdProvided and oauthClientSecretProvided — on edits that never touched credentials — while omitting the connectionStatus/lastConnected/ lastError resets the write actually performed. The route always sends `oauthClientId: body.oauthClientId || null` and `*Provided: ... !== undefined`, and null and false both survive a `value !== undefined` filter. The two *Provided flags are control params, not columns at all. Only the writer knows which columns a write touched, so updateMcpServer now returns updatedFields from its updateData and both the internal audit wrapper and the v2 use case record it. This matches workflow-mcp-lifecycle and credentials/orchestration, which already report written columns this way. --- apps/sim/lib/mcp/application/use-cases.ts | 4 +- .../orchestration/server-lifecycle.test.ts | 46 +++++++++++++++++++ .../lib/mcp/orchestration/server-lifecycle.ts | 24 ++++++---- 3 files changed, 61 insertions(+), 13 deletions(-) diff --git a/apps/sim/lib/mcp/application/use-cases.ts b/apps/sim/lib/mcp/application/use-cases.ts index b656ea30cf5..bbc6519e335 100644 --- a/apps/sim/lib/mcp/application/use-cases.ts +++ b/apps/sim/lib/mcp/application/use-cases.ts @@ -315,9 +315,7 @@ function updateAudit( serverName: result.server.name, transport: result.server.transport, url: result.server.url, - updatedFields: Object.keys(input).filter( - (key) => !['workspaceId', 'serverId', 'source'].includes(key) - ), + updatedFields: result.updatedFields ?? [], source: input.source, }, } diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts index 073c5a1d0fa..cdcd59d2f58 100644 --- a/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts +++ b/apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts @@ -3,6 +3,7 @@ */ import { auditMock, + auditMockFns, dbChainMock, dbChainMockFns, encryptionMock, @@ -64,6 +65,9 @@ import { } from '@/lib/mcp/orchestration/server-lifecycle' describe('MCP server lifecycle orchestration', () => { + const auditUpdatedFields = (): string[] | undefined => + auditMockFns.mockRecordAudit.mock.calls.at(-1)?.[0].metadata.updatedFields + beforeEach(() => { vi.clearAllMocks() resetDbChainMock() @@ -151,6 +155,48 @@ describe('MCP server lifecycle orchestration', () => { ) // ...and revoke the now-orphaned OAuth tokens rather than leaving them stored and valid. expect(mockRevokeOauthTokens).toHaveBeenCalledWith('server-1', 'workspace-1') + // The reset columns are the point of this audit row — an auditor needs to see + // that the connection was invalidated, not just that authType was touched. + expect(auditUpdatedFields()).toEqual( + expect.arrayContaining(['authType', 'connectionStatus', 'lastConnected', 'lastError']) + ) + }) + + it('audits only the columns an edit wrote, not the params it was handed', async () => { + dbChainMockFns.limit.mockResolvedValueOnce([ + { + url: 'https://example.com/mcp', + authType: 'headers', + oauthClientId: null, + oauthClientSecret: null, + }, + ]) + dbChainMockFns.returning.mockResolvedValueOnce([ + { + id: 'server-1', + workspaceId: 'workspace-1', + name: 'Renamed', + transport: 'streamable-http', + url: 'https://example.com/mcp', + authType: 'headers', + }, + ]) + + // A rename from the settings modal: the route always sends the OAuth params. + const result = await performUpdateMcpServer({ + workspaceId: 'workspace-1', + userId: 'user-1', + serverId: 'server-1', + name: 'Renamed', + oauthClientId: null, + oauthClientIdProvided: false, + oauthClientSecretProvided: false, + }) + + expect(result.success).toBe(true) + // `updatedAt` is excluded deliberately — it moves on every write, so it would + // be noise in every audit row. + expect(auditUpdatedFields()).toEqual(['name']) }) it('resets to disconnected when a create/upsert flips an existing OAuth server to headers', async () => { diff --git a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts index 6f378ffafd5..2899aa659b8 100644 --- a/apps/sim/lib/mcp/orchestration/server-lifecycle.ts +++ b/apps/sim/lib/mcp/orchestration/server-lifecycle.ts @@ -91,6 +91,13 @@ export interface PerformMcpServerResult { updated?: boolean authType?: McpAuthType configurationChanged?: boolean + /** + * Fields the update's SET clause wrote, minus `updatedAt`, for audit. Only + * the writer knows these: a param is not a write, and callers cannot see the + * `connectionStatus`/`lastConnected`/`lastError` reset that an auth or + * credential change forces. Record this instead of deriving names from input. + */ + updatedFields?: string[] } export type McpServerMutationAction = 'create' | 'update' | 'delete' @@ -389,7 +396,12 @@ export async function updateMcpServer( params.timeout !== undefined || params.retries !== undefined - return { success: true, server, configurationChanged: shouldClearCache } + return { + success: true, + server, + configurationChanged: shouldClearCache, + updatedFields: Object.keys(updateData).filter((key) => key !== 'updatedAt'), + } } catch (error) { logger.error('Failed to update MCP server', { error }) throw error @@ -501,15 +513,7 @@ export async function performUpdateMcpServer( serverName: result.server.name, transport: result.server.transport, url: result.server.url, - updatedFields: Object.entries(params) - .filter( - ([key, value]) => - value !== undefined && - !['workspaceId', 'userId', 'serverId', 'actorName', 'actorEmail', 'request'].includes( - key - ) - ) - .map(([key]) => key), + updatedFields: result.updatedFields ?? [], }, request: params.request, })