fix(api): correct credential role, skill permission bar, MCP url identity, and custom-tool conflict mapping

Author: TheodoreSpeaks

apps/docs/openapi-v2-resources.json

@@ -275,7 +275,7 @@
       "patch": {
         "operationId": "updateMcpServer",
         "summary": "Update MCP Server",
-        "description": "Update an MCP server's configuration. Only the fields you send are changed. Requires `write` permission on the workspace.\n\nChanging `url`, the auth type, or the OAuth client credentials invalidates any existing OAuth grant for the server and resets its connection state.",
+        "description": "Update an MCP server's configuration. Only the fields you send are changed. Requires `write` permission on the workspace.\n\n`url` is immutable: a server's id is derived from its URL, so re-pointing it would leave the id hashing an address the server no longer uses and allow two servers on one URL. Sending a different `url` returns `400` — delete the server and create one at the new address. Sending the URL it already has is accepted, so a full-object PATCH still works.\n\nChanging the auth type or the OAuth client credentials invalidates any existing OAuth grant for the server and resets its connection state.",
         "tags": ["MCP Servers"],
         "x-codeSamples": [
           {
@@ -2173,7 +2173,7 @@
             "type": "string",
             "minLength": 1,
             "maxLength": 2048,
-            "description": "Absolute http or https endpoint URL. May not contain `{{ENV_VAR}}` references."
+            "description": "Immutable. Must equal the server's current URL — a different value returns `400`, because the server's id is derived from its URL."
           },
           "authType": { "type": "string", "enum": ["none", "headers", "oauth"] },
           "headers": {

apps/sim/app/api/v2/credentials/route.test.ts

@@ -13,12 +13,14 @@ const {
   mockCheckWorkspaceAccess,
   mockListVisibleWorkspaceCredentials,
   mockPerformCreateCredential,
+  mockGetCredentialActorContext,
 } = vi.hoisted(() => ({
   mockCheckRateLimit: vi.fn(),
   mockResolveWorkspaceAccess: vi.fn(),
   mockCheckWorkspaceAccess: vi.fn(),
   mockListVisibleWorkspaceCredentials: vi.fn(),
   mockPerformCreateCredential: vi.fn(),
+  mockGetCredentialActorContext: vi.fn(),
 }))
 
 vi.mock('@/app/api/v1/middleware', () => ({
@@ -38,6 +40,10 @@ vi.mock('@/lib/credentials/orchestration', () => ({
   performCreateCredential: mockPerformCreateCredential,
 }))
 
+vi.mock('@/lib/credentials/access', () => ({
+  getCredentialActorContext: mockGetCredentialActorContext,
+}))
+
 vi.mock('@/app/api/v2/lib/gate', () => ({
   v2ApiGateError: vi.fn().mockResolvedValue(null),
 }))
@@ -210,6 +216,7 @@ describe('POST /api/v2/credentials', () => {
       credential: buildRow(),
       created: true,
     })
+    mockGetCredentialActorContext.mockResolvedValue({ member: null, isAdmin: false })
   })
 
   it('returns 404 when the v2 API surface flag is off', async () => {
@@ -279,6 +286,28 @@ describe('POST /api/v2/credentials', () => {
     expect((await res.json()).error.code).toBe('SERVICE_UNAVAILABLE')
   })
 
+  it('reports the real role when an idempotent create matches a credential the caller only belongs to', async () => {
+    mockPerformCreateCredential.mockResolvedValue({
+      success: true,
+      credential: buildRow(),
+      created: false,
+    })
+    mockGetCredentialActorContext.mockResolvedValue({ member: { role: 'member' }, isAdmin: false })
+
+    const res = await callCreate(VALID_BODY)
+    const body = await res.json()
+
+    expect(res.status).toBe(201)
+    expect(body.data.credential.role).toBe('member')
+  })
+
+  it('reports admin for a fresh insert without a second access lookup', async () => {
+    const res = await callCreate(VALID_BODY)
+
+    expect((await res.json()).data.credential.role).toBe('admin')
+    expect(mockGetCredentialActorContext).not.toHaveBeenCalled()
+  })
+
   it('creates the credential and never echoes the submitted secret', async () => {
     const res = await callCreate({
       workspaceId: WORKSPACE_ID,

apps/sim/app/api/v2/credentials/route.ts

@@ -8,6 +8,7 @@ import {
 import { parseRequest } from '@/lib/api/server'
 import { generateRequestId } from '@/lib/core/utils/request'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
+import { getCredentialActorContext } from '@/lib/credentials/access'
 import { performCreateCredential } from '@/lib/credentials/orchestration'
 import { listVisibleWorkspaceCredentials } from '@/lib/credentials/queries'
 import { checkWorkspaceAccess } from '@/lib/workspaces/permissions/utils'
@@ -119,10 +120,16 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
     }
 
     /**
-     * The creator is always an admin of the credential they just made, whether
-     * the row was inserted now or matched an existing source.
+     * A fresh insert makes the creator an admin, but an idempotent match against
+     * an existing source does not — the orchestration admits a caller who is
+     * only a *member* of that credential. Resolve the real role rather than
+     * assuming the create case, or the response would advertise administrative
+     * actions the caller cannot perform.
      */
-    const credential = toV2CredentialRow(result.credential, 'admin')
+    const actor = result.created
+      ? { isAdmin: true }
+      : await getCredentialActorContext(result.credential.id, userId)
+    const credential = toV2CredentialRow(result.credential, actor.isAdmin ? 'admin' : 'member')
 
     /**
      * Always 201, including when an existing credential already occupied this

apps/sim/app/api/v2/custom-tools/[id]/route.ts

@@ -17,7 +17,7 @@ import {
   upsertCustomTools,
 } from '@/lib/workflows/custom-tools/operations'
 import { checkRateLimit, resolveWorkspaceAccess } from '@/app/api/v1/middleware'
-import { toV2CustomTool } from '@/app/api/v2/custom-tools/utils'
+import { toV2CustomTool, v2CustomToolWriteError } from '@/app/api/v2/custom-tools/utils'
 import { v2ApiGateError } from '@/app/api/v2/lib/gate'
 import {
   v2Data,
@@ -144,6 +144,9 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Rout
 
     return v2Data({ customTool: toV2CustomTool(updated) }, { rateLimit })
   } catch (error) {
+    const writeError = v2CustomToolWriteError(error)
+    if (writeError) return writeError
+
     logger.error(`[${requestId}] Error updating custom tool`, {
       error: getErrorMessage(error, 'Unknown error'),
     })

apps/sim/app/api/v2/custom-tools/route.test.ts

@@ -227,6 +227,17 @@ describe('POST /api/v2/custom-tools', () => {
     expect(mockUpsertCustomTools).not.toHaveBeenCalled()
   })
 
+  it('409s when a concurrent create loses the title race inside the lib', async () => {
+    mockUpsertCustomTools.mockRejectedValue(
+      new Error('A tool with the title "v2_smoke_tool" already exists in this workspace')
+    )
+
+    const res = await callCreate(VALID_BODY)
+
+    expect(res.status).toBe(409)
+    expect((await res.json()).error.code).toBe('CONFLICT')
+  })
+
   it('creates the tool and returns 201 with the single tool', async () => {
     const res = await callCreate(VALID_BODY)
     const body = await res.json()

apps/sim/app/api/v2/custom-tools/route.ts

@@ -15,7 +15,7 @@ import {
   upsertCustomTools,
 } from '@/lib/workflows/custom-tools/operations'
 import { checkRateLimit, resolveWorkspaceAccess } from '@/app/api/v1/middleware'
-import { toV2CustomTool } from '@/app/api/v2/custom-tools/utils'
+import { toV2CustomTool, v2CustomToolWriteError } from '@/app/api/v2/custom-tools/utils'
 import { v2ApiGateError } from '@/app/api/v2/lib/gate'
 import {
   v2CursorList,
@@ -125,6 +125,9 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
 
     return v2Data({ customTool: toV2CustomTool(created) }, { rateLimit, status: 201 })
   } catch (error) {
+    const writeError = v2CustomToolWriteError(error)
+    if (writeError) return writeError
+
     logger.error(`[${requestId}] Error creating custom tool`, {
       error: getErrorMessage(error, 'Unknown error'),
     })

apps/sim/app/api/v2/custom-tools/utils.ts

@@ -1,7 +1,24 @@
 import type { customTools } from '@sim/db/schema'
+import { getErrorMessage } from '@sim/utils/errors'
+import type { NextResponse } from 'next/server'
 import type { V2CustomTool } from '@/lib/api/contracts/v2/custom-tools'
+import { v2Error } from '@/app/api/v2/lib/response'
 
-/** Shared serialization for the v2 custom tool surface. */
+/** Shared serialization + error mapping for the v2 custom tool surface. */
+
+/**
+ * `upsertCustomTools` reports a title collision as a thrown Error, and the unique
+ * index behind it fires on the race the pre-check cannot cover (two concurrent
+ * creates of the same title both pass the check, then one insert loses). Classify
+ * it as a conflict so that race surfaces as 409 rather than a generic 500.
+ */
+export function v2CustomToolWriteError(error: unknown): NextResponse | null {
+  const message = getErrorMessage(error, '')
+  if (/already exists in this workspace/i.test(message)) {
+    return v2Error('CONFLICT', message)
+  }
+  return null
+}
 
 type CustomToolRow = typeof customTools.$inferSelect
 

apps/sim/app/api/v2/mcp-servers/[id]/route.test.ts

@@ -240,6 +240,32 @@ describe('PATCH /api/v2/mcp-servers/[id]', () => {
     expect((await res.json()).error.code).toBe('NOT_FOUND')
   })
 
+  it('400s when the url is changed, since the id is derived from it', async () => {
+    mockGetWorkspaceMcpServer.mockResolvedValue(buildRow())
+
+    const res = await callPatch({
+      workspaceId: 'workspace-1',
+      url: 'https://different.example.com/sse',
+    })
+
+    expect(res.status).toBe(400)
+    expect((await res.json()).error.message).toContain('url cannot be changed')
+    expect(mockPerformUpdateMcpServer).not.toHaveBeenCalled()
+  })
+
+  it('allows a url that matches the stored one, so a full-object PATCH still works', async () => {
+    mockGetWorkspaceMcpServer.mockResolvedValue(buildRow())
+
+    const res = await callPatch({
+      workspaceId: 'workspace-1',
+      url: 'https://mcp.example.com/sse',
+      enabled: false,
+    })
+
+    expect(res.status).toBe(200)
+    expect(mockPerformUpdateMcpServer).toHaveBeenCalled()
+  })
+
   it('updates the server and returns the public shape', async () => {
     const res = await callPatch({ workspaceId: 'workspace-1', name: 'Renamed', enabled: false })
     const body = await res.json()

apps/sim/app/api/v2/mcp-servers/[id]/route.ts

@@ -91,6 +91,24 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Rout
     const access = await resolveWorkspaceAccess(rateLimit, userId, workspaceId, 'write')
     if (access) return v2WorkspaceAccessError(access)
 
+    /**
+     * A server's id is the hash of its workspace + URL, and this surface promises
+     * that identity. The lib will happily move `url` while the id keeps hashing
+     * the old one, which both breaks that promise and defeats the duplicate
+     * check on create (id-keyed, so it would not see the moved URL) — leaving two
+     * rows on one URL. Re-pointing a server at a different URL is a new server.
+     */
+    if (body.url !== undefined) {
+      const current = await getWorkspaceMcpServer({ workspaceId, serverId: id })
+      if (!current) return v2Error('NOT_FOUND', 'MCP server not found')
+      if (current.url !== body.url) {
+        return v2Error(
+          'BAD_REQUEST',
+          'url cannot be changed: an MCP server’s id is derived from its URL. Delete this server and create one at the new URL.'
+        )
+      }
+    }
+
     const result = await performUpdateMcpServer({
       workspaceId,
       userId,

apps/sim/app/api/v2/skills/[id]/route.test.ts

@@ -227,6 +227,16 @@ describe('PATCH /api/v2/skills/[id]', () => {
     expect((await res.json()).error.message).toContain('Built-in')
   })
 
+  it('gates on workspace read, leaving edit rights to the per-skill editor check', async () => {
+    await callPatch({ workspaceId: 'workspace-1', description: 'Updated' })
+    expect(mockResolveWorkspaceAccess).toHaveBeenCalledWith(
+      expect.anything(),
+      'user-1',
+      'workspace-1',
+      'read'
+    )
+  })
+
   it('updates the skill and returns the single skill', async () => {
     const res = await callPatch({ workspaceId: 'workspace-1', description: 'Updated' })
     const body = await res.json()
@@ -295,6 +305,16 @@ describe('DELETE /api/v2/skills/[id]', () => {
     expect(res.status).toBe(400)
   })
 
+  it('gates on workspace read, leaving delete rights to the per-skill editor check', async () => {
+    await callDelete()
+    expect(mockResolveWorkspaceAccess).toHaveBeenCalledWith(
+      expect.anything(),
+      'user-1',
+      'workspace-1',
+      'read'
+    )
+  })
+
   it('deletes the skill and acknowledges the id', async () => {
     const res = await callDelete()
     expect(res.status).toBe(200)