fix(mcp): audit an upsert that rewrites or revives a server by waleedlatif1 · Pull Request #6602 · simstudioai/sim

waleedlatif1 · 2026-08-12T04:55:45Z

Summary

Registering a URL that already exists takes the upsert branch and rewrites the live row, but recorded no audit row at all — the ADDED audit was gated on !result.updated
main recorded MCP_SERVER_ADDED for these unconditionally: wrong action, but a row existed. improvement(external-endpoints): v2 versions with clean signatures + updated docs based on openapi spec #5273 added the gate, so staging records nothing. This is an audit-coverage regression, the third traced to that PR
A rewrite is now MCP_SERVER_UPDATED carrying updatedFields; reviving a soft-deleted row stays MCP_SERVER_ADDED
Follow-up to fix(mcp): audit the columns an MCP server update wrote, not the params it got #6598, which established updatedFields on the result

What the upsert silently rewrites

name, description, transport, headers, timeout, retries, enabled, authType, the connection reset — and the URL's query string. generateMcpServerId hashes origin + pathname only (utils.ts:238 strips query and fragment), so re-registering …/mcp?token=NEW over …/mcp?token=OLD lands on the same row and repoints it. MCP endpoints commonly carry tokens there, and headers carry auth.

Reachability

Entry point	`existingServerBehavior`	Reaches upsert?
`POST /api/mcp/servers` (settings "add server")	not passed	yes
Copilot `manage_mcp_tool` op `add` → `registerMcpServerUseCase`	not passed	yes
`POST /api/v2/mcp-servers` → `createMcpServerUseCase`	`'reject'`	only on revival

The Copilot path is what motivated fixing this now: an agent can rewrite a server's headers and URL query with no audit trail.

Two distinct missing rows

createMcpServerUseCase throws unless idState.deleted, so v2's only updated case is a revival — it was dropping a legitimate ADDED row on every revival. Live rewrites and revivals need different actions, so the orchestration now surfaces the isRevival it already computed as revived on the result.

Type of Change

Bug fix

Testing

Two new tests: a query-string re-registration asserts MCP_SERVER_UPDATED with updatedFields containing url and headers; a soft-deleted revival asserts MCP_SERVER_ADDED with no updatedFields
Verified they can fail: restoring the old gate turns both red with expected undefined to be 'mcp_server.updated' / 'mcp_server.added' — i.e. no row at all
514 tests pass across lib/mcp, app/api/mcp, app/api/v2/mcp
updateValues retyped from Record<string, unknown> to Partial<typeof mcpServers.$inferInsert>; typecheck passing is the proof every key is a real column, which is what makes Object.keys safe here
lint:check, check:api-validation:strict, check:openapi all pass

Deliberately unchanged

Analytics gating. mcp_server_connected still fires only for a genuine insert — a revival arguably counts as a new connection, but changing it would skew first_mcp_connected_at and is a separate decision.

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)

vercel · 2026-08-12T04:55:49Z

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

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
docs	Skipped		Aug 12, 2026 5:07am

cursor · 2026-08-12T04:55:57Z

PR Summary

Medium Risk
Touches audit recording for MCP server config changes that can rewrite auth headers and URL query tokens. Risk is moderate because it restores missing audit coverage and strips secrets from audit metadata rather than changing auth itself.

Overview
Fixes a regression where MCP server registration upserts left no audit trail. Re-registering an existing URL rewrites the live row (headers, query string, etc.) but auditing was gated on !result.updated, so both rewrites and soft-delete revivals were silent.

Upsert results now expose revived. A live-row rewrite audits as MCP_SERVER_UPDATED with updatedFields; reviving a soft-deleted server stays MCP_SERVER_ADDED. Audit metadata also runs URLs through sanitizeUrlForLog so query tokens are not stored for org admins to read.

^{Reviewed by Cursor Bugbot for commit 47daecf. Configure here.}

greptile-apps · 2026-08-12T04:59:35Z

Greptile Summary

This follow-up completes audit coverage for MCP server upserts while redacting sensitive URL components from MCP audit metadata.

Records live-row rewrites as MCP_SERVER_UPDATED with the written fields.
Records soft-deleted-row revivals as MCP_SERVER_ADDED.
Sanitizes URLs across registration, update, and deletion audit paths.
Adds regression coverage for rewrite and revival audit behavior.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
apps/sim/lib/mcp/application/use-cases.ts	Classifies registration rewrites and revivals for semantic auditing and sanitizes URL metadata across use-case audit paths.
apps/sim/lib/mcp/orchestration/server-lifecycle.ts	Surfaces revival and written-field metadata from upserts, records the corresponding audit action, and sanitizes legacy lifecycle audit URLs.
apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts	Adds regression tests for rewrite and revival audit actions, updated fields, and query-token redaction.

Sequence Diagram

sequenceDiagram
participant Caller
participant UseCase as MCP use case
participant Lifecycle as Server lifecycle
participant DB
participant Audit
Caller->>UseCase: Register MCP server URL
UseCase->>Lifecycle: createMcpServer(...)
Lifecycle->>DB: Find row by generated server ID
alt Existing live row
Lifecycle->>DB: Rewrite server configuration
Lifecycle-->>UseCase: "updated=true, revived=false, updatedFields"
UseCase->>Audit: MCP_SERVER_UPDATED with sanitized URL
else Existing soft-deleted row
Lifecycle->>DB: Revive server row
Lifecycle-->>UseCase: "updated=true, revived=true"
UseCase->>Audit: MCP_SERVER_ADDED with sanitized URL
else New row
Lifecycle->>DB: Insert server
Lifecycle-->>UseCase: "updated=false"
UseCase->>Audit: MCP_SERVER_ADDED with sanitized URL
end

_{Reviews (2): Last reviewed commit: "fix(mcp): redact audit URLs and drop unw..." | Re-trigger Greptile}

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.

…elds 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.

waleedlatif1 · 2026-08-12T05:08:39Z

@greptile

waleedlatif1 · 2026-08-12T05:08:40Z

@cursor review

cursor

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 47daecf. Configure here.}

cursor · 2026-08-12T05:15:28Z

+ */
+ updatedFields = Object.entries(updateValues)
+ .filter(([key, value]) => key !== 'updatedAt' && value !== undefined)
+ .map(([key]) => key)


Rewrite audit lists soft-delete field
Low Severity
Live-rewrite updatedFields always includes deletedAt because the upsert SET writes deletedAt: null and the new filter keeps nulls. Revival is audited as MCP_SERVER_ADDED without updatedFields, so the soft-delete column only appears when it was already null and nothing undeleted.
Additional Locations (1)
apps/sim/lib/mcp/orchestration/server-lifecycle.ts#L229-L230
^{Reviewed by Cursor Bugbot for commit 47daecf. Configure here.}

vercelBottemporarily deployed to Preview August 12, 2026 04:55 Inactive

greptile-appsBot reviewed Aug 12, 2026
View reviewed changes

Comment threadapps/sim/lib/mcp/application/use-cases.ts

cursorBot reviewed Aug 12, 2026
View reviewed changes

Comment threadapps/sim/lib/mcp/orchestration/server-lifecycle.ts Outdated

waleedlatif1 mentioned this pull request Aug 12, 2026
refactor(audit): derive updatedFields through one shared helper #6604
Merged
6 tasks

waleedlatif1 added 2 commits August 11, 2026 22:07

waleedlatif1 force-pushed the fix/mcp-upsert-audit branch from 55827c3 to 47daecfCompare August 12, 2026 05:07

vercelBottemporarily deployed to Preview August 12, 2026 05:07 Inactive

waleedlatif1 merged commit b5d9e93 into stagingAug 12, 2026
30 checks passed

waleedlatif1 deleted the fix/mcp-upsert-audit branch August 12, 2026 05:13

waleedlatif1 mentioned this pull request Aug 12, 2026
v0.8.0: desktop app, workflow UI, v2 endpoints #6606
Merged

cursorBot reviewed Aug 12, 2026
View reviewed changes

Navigation Menu

Uh oh!

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

fix(mcp): audit an upsert that rewrites or revives a server#6602
waleedlatif1 merged 2 commits into
stagingfrom
fix/mcp-upsert-audit

waleedlatif1 commented Aug 12, 2026

Uh oh!

vercelBot commented Aug 12, 2026•
edited
Loading

Uh oh!

cursorBot commented Aug 12, 2026•
edited
Loading

Uh oh!

greptile-appsBot commented Aug 12, 2026•
edited
Loading

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

Uh oh!

waleedlatif1 commented Aug 12, 2026

Uh oh!

waleedlatif1 commented Aug 12, 2026

Uh oh!

Uh oh!

cursorBot left a comment

Uh oh!

cursorBotAug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

waleedlatif1 commented Aug 12, 2026

Summary

What the upsert silently rewrites

Reachability

Two distinct missing rows

Type of Change

Testing

Deliberately unchanged

Checklist

Uh oh!

vercelBot commented Aug 12, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cursorBot commented Aug 12, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

greptile-appsBot commented Aug 12, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

Uh oh!

waleedlatif1 commented Aug 12, 2026

Uh oh!

waleedlatif1 commented Aug 12, 2026

Uh oh!

Uh oh!

cursorBot left a comment

Choose a reason for hiding this comment

Uh oh!

cursorBotAug 12, 2026

Choose a reason for hiding this comment

Rewrite audit lists soft-delete field

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vercelBot commented Aug 12, 2026•
edited
Loading

cursorBot commented Aug 12, 2026•
edited
Loading

greptile-appsBot commented Aug 12, 2026•
edited
Loading