Skip to content

fix(mcp): audit the columns an MCP server update wrote, not the params it got - #6598

Merged
waleedlatif1 merged 1 commit into
stagingfrom
fix/mcp-audit-updated-fields
Aug 12, 2026
Merged

fix(mcp): audit the columns an MCP server update wrote, not the params it got#6598
waleedlatif1 merged 1 commit into
stagingfrom
fix/mcp-audit-updated-fields

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • Every PATCH /api/mcp/servers/[id] audit row listed oauthClientId, oauthClientIdProvided and oauthClientSecretProvided — including renames and URL edits that never touched credentials — and omitted the connectionStatus/lastConnected/lastError resets the write actually performed
  • The route always sends oauthClientId: body.oauthClientId || null and *Provided: ... !== undefined, so null and false both survive the value !== undefined filter. All three appear on every PATCH regardless of what the client sends — no modal-side condition needed. The two *Provided flags aren't even columns; they're control params
  • Regression from improvement(external-endpoints): v2 versions with clean signatures + updated docs based on openapi spec #5273 (263e3ca67e), which split performUpdateMcpServer into a writer that owns updateData and a thin audit wrapper with no access to it. main derived the list from Object.keys(updateData); the wrapper fell back to the params it could see
  • Only the writer knows which columns a write touched, so updateMcpServer now returns updatedFields and both the internal wrapper and the v2 use case record it. Net −9 lines of derivation logic
  • Audit fidelity only — .map(([key]) => key) records names, never values. No credential value was or is exposed

Why this shape

This is the pattern the repo already uses: workflow-mcp-lifecycle.ts:96,552,939 and credentials/orchestration/index.ts:159,339 both carry updatedFields?: string[] on the result and populate it from the writer's updateData. This makes server-lifecycle.ts the third instance rather than a new convention. The v2 twin shared the flaw in a different form (Object.keys(input) with no filter at all) and now reads the same result.updatedFields.

Intentional change to audit output

Rows now include connectionStatus/lastConnected/lastError on an auth or credential change, include authType when it is implicitly promoted to oauth, and stop including oauthClientIdProvided/oauthClientSecretProvided. I grepped for downstream consumers keyed on the old strings (EE audit-log UI, v1 export, dashboards) and found none.

Type of Change

  • Bug fix

Testing

  • Two new tests in server-lifecycle.test.ts: one asserting a rename records exactly ['name'], one asserting an auth-type flip records the reset columns
  • Verified they can fail: against the old params-derived logic they go red with expected [ 'name', 'oauthClientId', …(2) ] to deeply equal [ 'name' ] and expected [ 'authType' ] to deeply equal ArrayContaining{…}
  • 875 tests pass across lib/mcp, app/api/mcp, app/api/v2/mcp, lib/credentials
  • check:openapi unchanged and check:api-validation:strict pass — confirms no updatedFields key leaks into a wire response (no caller spreads the result; the v2 projection re-wraps picked fields)

Follow-ups (not in this PR)

  • performCreateMcpServer's upsert branch rewrites an existing server (name, URL, headers, OAuth creds, auth-type flip, same connection reset) but gates its audit on if (!result.updated), so an upsert-rewrite emits no MCP_SERVER_UPDATED row at all. That's a behavior change, and it needs updateValues tightened from Record<string, unknown> to Partial<typeof mcpServers.$inferInsert> first to be column-safe
  • Object.keys(updateData).filter(k => k !== 'updatedAt') now appears in 6 places across 4 files (5 predate this PR). Worth one shared helper so the exclusion set is a single edit, but that spans lib/credentials and a v1 admin route — its own PR

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

…s it got

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.
@vercel

vercel Bot commented Aug 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 12, 2026 4:37am

Request Review

@cursor

cursor Bot commented Aug 12, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Audit metadata only — field names, never values. No change to update behavior, auth, or credential handling.

Overview
MCP server update audits now record the columns the writer actually set, instead of deriving names from request params.

updateMcpServer returns updatedFields from its updateData (excluding updatedAt). Both the legacy wrapper and the v2 use case record that list. Renames no longer spam OAuth control params, and auth/credential changes correctly include the connectionStatus/lastConnected/lastError resets.

Reviewed by Cursor Bugbot for commit 3bf7a0d. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR corrects MCP server update audits by deriving updatedFields from the database writer’s actual update object rather than request parameters.

  • Returns written column names from updateMcpServer, excluding the routinely updated timestamp.
  • Uses those names in both internal and application use-case audit paths.
  • Adds regression coverage for rename-only updates and auth-related connection resets.

Confidence Score: 5/5

The PR appears safe to merge, with the audit metadata now matching the columns written by MCP server updates.

The writer derives audit fields from the same update object passed to the database, and both audit paths preserve that result without transformation; no actionable defects remain.

Important Files Changed

Filename Overview
apps/sim/lib/mcp/orchestration/server-lifecycle.ts Returns audit field names from the exact object written to the database and forwards them through the internal audit wrapper.
apps/sim/lib/mcp/application/use-cases.ts Replaces input-derived audit metadata with the writer-provided list of updated columns.
apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts Adds regression assertions for rename-only audits and implicit connection-reset columns.

Sequence Diagram

sequenceDiagram
  participant Caller
  participant UseCase
  participant Writer as updateMcpServer
  participant DB
  participant Audit
  Caller->>UseCase: Update MCP server
  UseCase->>Writer: Validated update parameters
  Writer->>Writer: Build updateData
  Writer->>DB: SET updateData
  DB-->>Writer: Updated server
  Writer-->>UseCase: server + keys(updateData) excluding updatedAt
  UseCase->>Audit: MCP_SERVER_UPDATED with updatedFields
Loading

Reviews (1): Last reviewed commit: "fix(mcp): audit the columns an MCP serve..." | Re-trigger Greptile

@waleedlatif1
waleedlatif1 merged commit 933eea5 into staging Aug 12, 2026
30 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/mcp-audit-updated-fields branch August 12, 2026 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant