mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
fix(mcp): audit the columns an MCP server update wrote, not the params it got (#6598)
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.
This commit is contained in:
@@ -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,
|
||||
},
|
||||
}
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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,
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user