fix(api): align credential mutation gating, provider-outage status, and unique-violation conflicts

Author: TheodoreSpeaks

apps/docs/openapi-v2-resources.json

@@ -1601,7 +1601,7 @@
       "patch": {
         "operationId": "updateCredential",
         "summary": "Update Credential",
-        "description": "Rename a credential, change its description, or rotate its stored secret. Requires credential admin.\n\nSending a secret field rotates that secret in place: it is re-verified against the provider and re-encrypted, and the display name is preserved. Secrets are never returned in the response.",
+        "description": "Rename a credential, change its description, or rotate its stored secret. Requires credential admin \u2014 access to the workspace plus admin rights on the credential itself. Workspace `write` is not required: credentials are gated per credential, not per workspace.\n\nSending a secret field rotates that secret in place: it is re-verified against the provider and re-encrypted, and the display name is preserved. Secrets are never returned in the response. If the provider cannot be reached to verify the new secret, the request returns `503`.\n\nA credential you cannot see answers `404` rather than `403`, so its existence is never disclosed.",
         "tags": ["Credentials"],
         "x-codeSamples": [
           {
@@ -1673,13 +1673,14 @@
           "404": { "$ref": "#/components/responses/NotFound" },
           "409": { "$ref": "#/components/responses/Conflict" },
           "429": { "$ref": "#/components/responses/RateLimited" },
-          "500": { "$ref": "#/components/responses/InternalError" }
+          "500": { "$ref": "#/components/responses/InternalError" },
+          "503": { "$ref": "#/components/responses/ServiceUnavailable" }
         }
       },
       "delete": {
         "operationId": "deleteCredential",
         "summary": "Delete Credential",
-        "description": "Delete a credential. Requires credential admin. Blocks and workflows configured against it stop authenticating, and any environment variable it backed is removed.",
+        "description": "Delete a credential. Requires credential admin \u2014 access to the workspace plus admin rights on the credential itself; workspace `write` is not required. Blocks and workflows configured against it stop authenticating, and any environment variable it backed is removed.\n\nA credential you cannot see answers `404` rather than `403`.",
         "tags": ["Credentials"],
         "x-codeSamples": [
           {

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

@@ -188,6 +188,7 @@ describe('PATCH /api/v2/credentials/[id]', () => {
     mockCheckRateLimit.mockResolvedValue(RATE_LIMIT_OK)
     mockResolveWorkspaceAccess.mockResolvedValue(null)
     mockGetWorkspaceCredential.mockResolvedValue(buildRow())
+    mockGetCredentialActorContext.mockResolvedValue({ member: { role: 'admin' }, isAdmin: true })
     mockPerformUpdateCredential.mockResolvedValue({ success: true })
   })
 
@@ -246,6 +247,35 @@ describe('PATCH /api/v2/credentials/[id]', () => {
     expect((await res.json()).error.code).toBe('FORBIDDEN')
   })
 
+  it('gates on workspace read, leaving admin rights to the per-credential check', async () => {
+    await callPatch({ workspaceId: WORKSPACE_ID, displayName: 'Renamed' })
+    expect(mockResolveWorkspaceAccess).toHaveBeenCalledWith(
+      expect.anything(),
+      'user-1',
+      WORKSPACE_ID,
+      'read'
+    )
+  })
+
+  it('masks a credential the caller cannot see as 404, not 403', async () => {
+    mockGetCredentialActorContext.mockResolvedValue({ member: null, isAdmin: false })
+    const res = await callPatch({ workspaceId: WORKSPACE_ID, displayName: 'Renamed' })
+    expect(res.status).toBe(404)
+    expect(mockPerformUpdateCredential).not.toHaveBeenCalled()
+  })
+
+  it('503s when the provider is unreachable during a secret rotation', async () => {
+    mockPerformUpdateCredential.mockResolvedValue({
+      success: false,
+      error: 'provider_unavailable',
+      errorCode: 'validation',
+      providerErrorCode: 'provider_unavailable',
+    })
+    const res = await callPatch({ workspaceId: WORKSPACE_ID, apiToken: 'tok' })
+    expect(res.status).toBe(503)
+    expect((await res.json()).error.code).toBe('SERVICE_UNAVAILABLE')
+  })
+
   it('rotates a secret without echoing it back', async () => {
     const res = await callPatch({ workspaceId: WORKSPACE_ID, apiToken: 'brand-new-token' })
     const body = await res.json()
@@ -268,6 +298,7 @@ describe('DELETE /api/v2/credentials/[id]', () => {
     mockCheckRateLimit.mockResolvedValue(RATE_LIMIT_OK)
     mockResolveWorkspaceAccess.mockResolvedValue(null)
     mockGetWorkspaceCredential.mockResolvedValue(buildRow())
+    mockGetCredentialActorContext.mockResolvedValue({ member: { role: 'admin' }, isAdmin: true })
     mockPerformDeleteCredential.mockResolvedValue({ success: true })
   })
 
@@ -309,6 +340,23 @@ describe('DELETE /api/v2/credentials/[id]', () => {
     expect(mockPerformDeleteCredential).not.toHaveBeenCalled()
   })
 
+  it('gates on workspace read, leaving admin rights to the per-credential check', async () => {
+    await callDelete()
+    expect(mockResolveWorkspaceAccess).toHaveBeenCalledWith(
+      expect.anything(),
+      'user-1',
+      WORKSPACE_ID,
+      'read'
+    )
+  })
+
+  it('masks a credential the caller cannot see as 404, not 403', async () => {
+    mockGetCredentialActorContext.mockResolvedValue({ member: null, isAdmin: false })
+    const res = await callDelete()
+    expect(res.status).toBe(404)
+    expect(mockPerformDeleteCredential).not.toHaveBeenCalled()
+  })
+
   it('deletes the credential and acknowledges the id', async () => {
     const res = await callDelete()
     expect(res.status).toBe(200)

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

@@ -101,20 +101,30 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Rout
     const { id } = parsed.data.params
     const { workspaceId, ...changes } = parsed.data.body
 
-    const access = await resolveWorkspaceAccess(rateLimit, userId, workspaceId, 'write')
+    /**
+     * Credential mutations are gated per credential, not per workspace:
+     * `performUpdateCredential` requires credential admin, and the internal
+     * surface applies no workspace-level bar at all. Requiring workspace `write`
+     * here would lock out a credential admin who only holds `read`.
+     */
+    const access = await resolveWorkspaceAccess(rateLimit, userId, workspaceId, 'read')
     if (access) return v2WorkspaceAccessError(access)
 
     // Tenant-scope the id before the orchestration re-derives access from the
     // credential's own workspace.
     const existing = await getWorkspaceCredential({ workspaceId, credentialId: id })
     if (!existing) return v2Error('NOT_FOUND', 'Credential not found')
 
+    const actor = await getCredentialActorContext(id, userId)
+    if (!actor.member && !actor.isAdmin) return v2Error('NOT_FOUND', 'Credential not found')
+
     const result = await performUpdateCredential({ ...changes, credentialId: id, userId, request })
 
     if (!result.success) {
       return v2CredentialOrchestrationError(
         result.errorCode,
-        result.error ?? 'Failed to update credential'
+        result.error ?? 'Failed to update credential',
+        { providerUnavailable: result.providerErrorCode === 'provider_unavailable' }
       )
     }
 
@@ -151,12 +161,23 @@ export const DELETE = withRouteHandler(async (request: NextRequest, context: Rou
     const { id } = parsed.data.params
     const { workspaceId } = parsed.data.query
 
-    const access = await resolveWorkspaceAccess(rateLimit, userId, workspaceId, 'write')
+    // Gated per credential by `performDeleteCredential`, same as PATCH above.
+    const access = await resolveWorkspaceAccess(rateLimit, userId, workspaceId, 'read')
     if (access) return v2WorkspaceAccessError(access)
 
     const existing = await getWorkspaceCredential({ workspaceId, credentialId: id })
     if (!existing) return v2Error('NOT_FOUND', 'Credential not found')
 
+    /**
+     * A credential the caller cannot see answers 404, matching GET, so a
+     * workspace member cannot tell an inaccessible credential from a missing one
+     * and enumerate ids. A credential they *can* see but cannot administer still
+     * gets the orchestration's 403 — that distinction is not a leak, since GET
+     * already shows them the credential.
+     */
+    const actor = await getCredentialActorContext(id, userId)
+    if (!actor.member && !actor.isAdmin) return v2Error('NOT_FOUND', 'Credential not found')
+
     const result = await performDeleteCredential({ credentialId: id, userId, request })
     if (!result.success) {
       return v2CredentialOrchestrationError(

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

@@ -238,6 +238,18 @@ describe('POST /api/v2/custom-tools', () => {
     expect((await res.json()).error.code).toBe('CONFLICT')
   })
 
+  it('409s when the unique index rejects the loser of a title race', async () => {
+    const pgError = Object.assign(new Error('duplicate key value violates unique constraint'), {
+      code: '23505',
+    })
+    mockUpsertCustomTools.mockRejectedValue(pgError)
+
+    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/utils.ts

@@ -1,18 +1,26 @@
 import type { customTools } from '@sim/db/schema'
-import { getErrorMessage } from '@sim/utils/errors'
+import { getErrorMessage, getPostgresErrorCode } 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 + 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.
+ * Classifies a title collision as a conflict so it surfaces as 409 rather than a
+ * generic 500. Two distinct failures reach here and both must be covered:
+ *
+ * - `upsertCustomTools` throws its own message when its in-transaction duplicate
+ *   `SELECT` finds one.
+ * - Under a concurrent create or rename, both callers pass that `SELECT` too, and
+ *   the loser is rejected by `custom_tools_workspace_title_unique` as a raw
+ *   Postgres `23505` — whose message matches nothing, which is exactly the race
+ *   the message check alone cannot see.
  */
 export function v2CustomToolWriteError(error: unknown): NextResponse | null {
+  if (getPostgresErrorCode(error) === '23505') {
+    return v2Error('CONFLICT', 'A custom tool with that title already exists in this workspace')
+  }
   const message = getErrorMessage(error, '')
   if (/already exists in this workspace/i.test(message)) {
     return v2Error('CONFLICT', message)