fix(credentials): prevent stale secrets and drafts

Author: TheodoreSpeaks

apps/sim/app/api/auth/oauth2/authorize/route.test.ts

@@ -111,6 +111,18 @@ describe('OAuth2 authorize route', () => {
     )
   })
 
+  it('requires a configured OAuth client before creating a legacy draft', async () => {
+    mocks.requireClient.mockImplementationOnce(() => {
+      throw new Error('OAuth client is not configured')
+    })
+
+    const response = await GET(request({ providerId: 'google-email', workspaceId: WORKSPACE_ID }))
+
+    expect(response.headers.get('location')).toBe(`${BASE_URL}/workspace?error=oauth_link_failed`)
+    expect(mocks.requireClient).toHaveBeenCalledWith('google-email')
+    expect(mocks.createConnection).not.toHaveBeenCalled()
+  })
+
   it('launches an exact draft without creating another one', async () => {
     const response = await GET(request({ draftId: 'draft-1' }))
 

apps/sim/app/api/auth/oauth2/authorize/route.ts

@@ -65,6 +65,8 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
       throw new Error('Validated OAuth authorization request is missing its target')
     }
 
+    requireConfiguredOAuthClient(providerId)
+
     const connectionCompleteUrl = new URL('/oauth/credential-connected', baseUrl)
     connectionCompleteUrl.searchParams.set('result', 'connected')
     const callbackURL = fromConnectionDraft
@@ -109,8 +111,6 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
       }
     }
 
-    requireConfiguredOAuthClient(providerId)
-
     if (!connectionDraftId) {
       throw new Error('OAuth authorization is missing its credential draft id')
     }

apps/sim/lib/credentials/orchestration/credential-create.ts

@@ -2,12 +2,14 @@ import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
 import { db } from '@sim/db'
 import { account, credential, credentialMember } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
+import { safeCompare } from '@sim/security/compare'
 import { getPostgresErrorCode } from '@sim/utils/errors'
 import { generateId } from '@sim/utils/id'
 import { and, eq } from 'drizzle-orm'
 import { normalizeCredentialEnvKey } from '@/lib/api/contracts/credentials'
 import { acquireOrganizationUserMutationLocks } from '@/lib/billing/organizations/membership'
 import type { OrchestrationRequestContext } from '@/lib/core/orchestration/types'
+import { decryptSecret } from '@/lib/core/security/encryption'
 import { getCredentialActorContext } from '@/lib/credentials/access'
 import { AtlassianValidationError } from '@/lib/credentials/atlassian-service-account'
 import { getCredentialCreationWorkspaceContext } from '@/lib/credentials/environment'
@@ -185,6 +187,18 @@ async function findExistingCredentialBySourceWith(
   return null
 }
 
+async function serviceAccountSecretsMatch(
+  existingEncryptedSecret: string | null,
+  submittedEncryptedSecret: string | null
+): Promise<boolean> {
+  if (!existingEncryptedSecret || !submittedEncryptedSecret) return false
+  const [existing, submitted] = await Promise.all([
+    decryptSecret(existingEncryptedSecret),
+    decryptSecret(submittedEncryptedSecret),
+  ])
+  return safeCompare(existing.decrypted, submitted.decrypted)
+}
+
 function failure(
   error: string,
   errorCode: CredentialOrchestrationErrorCode,
@@ -325,12 +339,11 @@ export async function createCredentialRecord(
         )
       }
 
-      /**
-       * Token service-account creates always carry a fresh token that must be
-       * stored — falling through to the existing-credential path would return
-       * the old credential as success and silently drop the submitted token.
-       */
-      if (resolvedProviderId && isTokenServiceAccountProviderId(resolvedProviderId)) {
+      if (
+        type === 'service_account' &&
+        resolvedProviderId &&
+        isTokenServiceAccountProviderId(resolvedProviderId)
+      ) {
         return failure(
           `A credential named "${resolvedDisplayName}" already exists in this workspace. Give this one a different name.`,
           'conflict',
@@ -346,6 +359,27 @@ export async function createCredentialRecord(
         return failure('A credential with this source already exists in this workspace', 'conflict')
       }
 
+      /**
+       * Non-token service accounts may replay only the exact stored secret. A
+       * source match with rotated secret material must not report success while
+       * silently retaining the old ciphertext. Compare only after credential
+       * access is established so the encrypted value stays behind its resource
+       * authorization boundary.
+       */
+      if (
+        type === 'service_account' &&
+        !(await serviceAccountSecretsMatch(
+          existingCredential.encryptedServiceAccountKey,
+          resolvedEncryptedServiceAccountKey
+        ))
+      ) {
+        return failure(
+          `A credential named "${resolvedDisplayName}" already exists in this workspace. Give this one a different name.`,
+          'conflict',
+          { providerErrorCode: 'duplicate_display_name' }
+        )
+      }
+
       const shouldUpdateDisplayName =
         type === 'oauth' &&
         resolvedDisplayName &&

apps/sim/lib/credentials/orchestration/index.test.ts

@@ -64,7 +64,11 @@ vi.mock('@/lib/credentials/token-service-accounts/errors', () => ({
 }))
 vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: vi.fn() }))
 
-import { deleteCredentialRecord, performUpdateCredential } from '@/lib/credentials/orchestration'
+import {
+  createServiceAccountCredential,
+  deleteCredentialRecord,
+  performUpdateCredential,
+} from '@/lib/credentials/orchestration'
 
 const OLD_EMAIL = 'old-sa@old-project.iam.gserviceaccount.com'
 const NEW_EMAIL = 'new-sa@new-project.iam.gserviceaccount.com'
@@ -434,6 +438,93 @@ describe('performUpdateCredential — service-account secret rotation', () => {
   })
 })
 
+describe('createServiceAccountCredential', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    resetDbChainMock()
+  })
+
+  it('rejects an existing service-account source instead of discarding the submitted secret', async () => {
+    mockVerifyAndBuildServiceAccountSecret.mockResolvedValue({
+      providerId: 'zoom-service-account',
+      encryptedServiceAccountKey: 'new-cipher',
+      displayName: 'Production Zoom',
+      auditMetadata: {},
+      principal: { kind: 'tenant', id: 'account-1' },
+    })
+    queueTableRows(schemaMock.credential, [
+      {
+        id: 'credential-1',
+        workspaceId: 'workspace-1',
+        type: 'service_account',
+        providerId: 'zoom-service-account',
+        displayName: 'Production Zoom',
+        encryptedServiceAccountKey: 'old-cipher',
+      },
+    ])
+    mockDecryptSecret
+      .mockResolvedValueOnce({ decrypted: 'stored-secret' })
+      .mockResolvedValueOnce({ decrypted: 'rotated-secret' })
+    mockGetCredentialActorContext.mockResolvedValue({ member: { role: 'admin' }, isAdmin: true })
+
+    const result = await createServiceAccountCredential({
+      workspaceId: 'workspace-1',
+      userId: 'user-1',
+      providerId: 'zoom-service-account',
+      displayName: 'Production Zoom',
+      clientId: 'client-id',
+      clientSecret: 'rotated-client-secret',
+      orgId: 'account-1',
+    })
+
+    expect(result).toMatchObject({
+      success: false,
+      errorCode: 'conflict',
+      providerErrorCode: 'duplicate_display_name',
+    })
+    expect(dbChainMockFns.insert).not.toHaveBeenCalled()
+    expect(mockGetCredentialActorContext).toHaveBeenCalledWith('credential-1', 'user-1', {})
+  })
+
+  it('returns an accessible credential for an exact non-token secret replay', async () => {
+    const existingCredential = {
+      id: 'credential-1',
+      workspaceId: 'workspace-1',
+      type: 'service_account',
+      providerId: 'zoom-service-account',
+      displayName: 'Production Zoom',
+      encryptedServiceAccountKey: 'stored-cipher',
+    }
+    mockVerifyAndBuildServiceAccountSecret.mockResolvedValue({
+      providerId: 'zoom-service-account',
+      encryptedServiceAccountKey: 'replay-cipher',
+      displayName: 'Production Zoom',
+      auditMetadata: {},
+      principal: { kind: 'tenant', id: 'account-1' },
+    })
+    queueTableRows(schemaMock.credential, [existingCredential])
+    mockDecryptSecret.mockResolvedValue({ decrypted: 'same-secret' })
+    mockGetCredentialActorContext.mockResolvedValue({ member: { role: 'admin' }, isAdmin: true })
+
+    const result = await createServiceAccountCredential({
+      workspaceId: 'workspace-1',
+      userId: 'user-1',
+      providerId: 'zoom-service-account',
+      displayName: 'Production Zoom',
+      clientId: 'client-id',
+      clientSecret: 'client-secret',
+      orgId: 'account-1',
+    })
+
+    expect(result).toMatchObject({
+      success: true,
+      credential: existingCredential,
+      created: false,
+    })
+    expect(dbChainMockFns.insert).not.toHaveBeenCalled()
+  })
+})
+
 describe('deleteCredentialRecord', () => {
   beforeEach(() => {
     vi.clearAllMocks()