From 7b01cd84a15d1a468dda1f084efb60bc5c92e870 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Wed, 16 Sep 2026 11:22:24 -0700 Subject: [PATCH 1/2] fix(credentials): clean up personal secrets in archived workspaces --- apps/sim/app/api/environment/route.ts | 9 +- apps/sim/lib/credentials/environment.test.ts | 87 +++++++++-- apps/sim/lib/credentials/environment.ts | 142 +++++++++--------- .../lib/credentials/orchestration/index.ts | 7 +- apps/sim/lib/environment/utils.ts | 5 +- 5 files changed, 151 insertions(+), 99 deletions(-) diff --git a/apps/sim/app/api/environment/route.ts b/apps/sim/app/api/environment/route.ts index 96aa61c9b87..10034a9e495 100644 --- a/apps/sim/app/api/environment/route.ts +++ b/apps/sim/app/api/environment/route.ts @@ -60,12 +60,8 @@ export const POST = withRouteHandler(async (req: NextRequest) => { * persists a map derived from the pre-replace state, discarding this one * entirely. * - * The reconcile below stays outside because it opens its own transaction. - * That leaves a known gap: it prunes mirrors against this request's key - * list, so a secret added after the commit loses its mirror while its - * value survives. Closing it means having the reconcile read the map - * itself rather than trust a caller's list, across all four of its - * callers. + * The reconcile below opens its own transaction and re-reads the map + * under this same lock so a later save cannot be undone by stale keys. */ await db.transaction(async (tx) => { await lockPersonalEnvMap(tx, session.user.id) @@ -89,7 +85,6 @@ export const POST = withRouteHandler(async (req: NextRequest) => { await syncPersonalEnvCredentialsForUser({ userId: session.user.id, - envKeys: Object.keys(variables), }) recordAudit({ diff --git a/apps/sim/lib/credentials/environment.test.ts b/apps/sim/lib/credentials/environment.test.ts index 06852cef485..2d67adc0aa9 100644 --- a/apps/sim/lib/credentials/environment.test.ts +++ b/apps/sim/lib/credentials/environment.test.ts @@ -1,20 +1,31 @@ /** * @vitest-environment node */ -import { credential, permissions, workspace } from '@sim/db/schema' -import { dbChainMock, dbChainMockFns, queueTableRows, resetDbChainMock } from '@sim/testing' +import { credential, environment, permissions, workspace } from '@sim/db/schema' +import { + dbChainMock, + dbChainMockFns, + flattenMockConditions, + queueTableRows, + resetDbChainMock, +} from '@sim/testing' import { eq } from 'drizzle-orm' import { beforeEach, describe, expect, it, vi } from 'vitest' import type { DbOrTx } from '@/lib/db/types' -const { mockAcquireUserBillingIdentityLock } = vi.hoisted(() => ({ +const { mockAcquireUserBillingIdentityLock, mockLockPersonalEnvMap } = vi.hoisted(() => ({ mockAcquireUserBillingIdentityLock: vi.fn(), + mockLockPersonalEnvMap: vi.fn(), })) vi.mock('@/lib/billing/organizations/billing-identity-lock', () => ({ acquireUserBillingIdentityLock: mockAcquireUserBillingIdentityLock, })) +vi.mock('@/lib/credentials/env-locks', () => ({ + lockPersonalEnvMap: mockLockPersonalEnvMap, +})) + import { createWorkspaceEnvCredentials, getEnrolledManagedOAuthCredentials, @@ -196,9 +207,10 @@ describe('syncPersonalEnvCredentialsForUser', () => { vi.clearAllMocks() resetDbChainMock() mockAcquireUserBillingIdentityLock.mockResolvedValue(undefined) + mockLockPersonalEnvMap.mockResolvedValue(undefined) }) - it('uses one transaction and acquires the transfer fence before discovering workspaces', async () => { + it('locks the map before the transfer fence and reads current keys before reconciling', async () => { const base = dbChainMock.db const tx = { select: vi.fn(base.select), @@ -206,20 +218,24 @@ describe('syncPersonalEnvCredentialsForUser', () => { delete: vi.fn(base.delete), } as unknown as DbOrTx dbChainMockFns.transaction.mockImplementationOnce(async (callback) => callback(tx)) + queueTableRows(environment, [{ variables: { API_KEY: 'encrypted' } }]) queueTableRows(permissions, [{ workspaceId: 'ws-1' }]) queueTableRows(workspace, []) queueTableRows(credential, [{ id: 'credential-1' }]) await syncPersonalEnvCredentialsForUser({ userId: 'user-1', - envKeys: ['API_KEY'], }) + expect(mockLockPersonalEnvMap).toHaveBeenCalledWith(tx, 'user-1') + expect(mockLockPersonalEnvMap.mock.invocationCallOrder[0]).toBeLessThan( + mockAcquireUserBillingIdentityLock.mock.invocationCallOrder[0] + ) expect(mockAcquireUserBillingIdentityLock).toHaveBeenCalledWith(tx, 'user-1') expect(mockAcquireUserBillingIdentityLock.mock.invocationCallOrder[0]).toBeLessThan( (tx.select as ReturnType).mock.invocationCallOrder[0] ) - expect(tx.select).toHaveBeenCalledTimes(3) + expect(tx.select).toHaveBeenCalledTimes(4) expect(tx.insert).toHaveBeenCalledTimes(2) expect(tx.delete).toHaveBeenCalledTimes(1) }) @@ -232,19 +248,19 @@ describe('syncPersonalEnvCredentialsForUser', () => { delete: vi.fn(base.delete), } as unknown as DbOrTx dbChainMockFns.transaction.mockImplementationOnce(async (callback) => callback(tx)) + queueTableRows(environment, [{ variables: { API_KEY: 'encrypted' } }]) queueTableRows(permissions, []) queueTableRows(workspace, []) await syncPersonalEnvCredentialsForUser({ userId: 'user-1', - envKeys: ['API_KEY'], }) expect(mockAcquireUserBillingIdentityLock.mock.invocationCallOrder[0]).toBeLessThan( (tx.select as ReturnType).mock.invocationCallOrder[0] ) expect(tx.insert).not.toHaveBeenCalled() - expect(tx.delete).not.toHaveBeenCalled() + expect(tx.delete).toHaveBeenCalledTimes(1) }) it('syncs every workspace with one credential insert, lookup, membership insert, and cleanup', async () => { @@ -255,16 +271,16 @@ describe('syncPersonalEnvCredentialsForUser', () => { delete: vi.fn(base.delete), } as unknown as DbOrTx dbChainMockFns.transaction.mockImplementationOnce(async (callback) => callback(tx)) + queueTableRows(environment, [{ variables: { API_KEY: 'encrypted' } }]) queueTableRows(permissions, [{ workspaceId: 'ws-2' }, { workspaceId: 'ws-1' }]) queueTableRows(workspace, []) queueTableRows(credential, [{ id: 'credential-1' }, { id: 'credential-2' }]) await syncPersonalEnvCredentialsForUser({ userId: 'user-1', - envKeys: ['API_KEY'], }) - expect(tx.select).toHaveBeenCalledTimes(3) + expect(tx.select).toHaveBeenCalledTimes(4) expect(tx.insert).toHaveBeenCalledTimes(2) expect(tx.delete).toHaveBeenCalledTimes(1) expect(dbChainMockFns.values).toHaveBeenNthCalledWith(1, [ @@ -272,6 +288,57 @@ describe('syncPersonalEnvCredentialsForUser', () => { expect.objectContaining({ workspaceId: 'ws-2', envKey: 'API_KEY' }), ]) }) + + it.each([ + { label: 'missing', rows: [] }, + { label: 'empty', rows: [{ variables: {} }] }, + ])( + 'cleans archived-workspace mirrors with a $label map and no active workspaces', + async ({ rows }) => { + queueTableRows(environment, rows) + const deleteWhere = vi.fn().mockResolvedValue([]) + dbChainMock.db.delete.mockReturnValue({ where: deleteWhere }) + + await syncPersonalEnvCredentialsForUser({ userId: 'user-1' }) + + expect(dbChainMock.db.delete).toHaveBeenCalledWith(credential) + expect(flattenMockConditions(deleteWhere.mock.calls[0][0])).toEqual([ + { type: 'eq', left: credential.type, right: 'env_personal' }, + { type: 'eq', left: credential.envOwnerUserId, right: 'user-1' }, + ]) + expect(dbChainMockFns.insert).not.toHaveBeenCalled() + } + ) + + it.each([ + { label: 'no active workspaces', activeWorkspaces: [] }, + { label: 'an active workspace', activeWorkspaces: [{ workspaceId: 'active-workspace' }] }, + ])( + 'prunes deleted keys across archived workspaces with $label, preserving current keys and owners', + async ({ activeWorkspaces }) => { + queueTableRows(environment, [{ variables: { KEEP: 'encrypted', NEW: 'encrypted-new' } }]) + queueTableRows(permissions, activeWorkspaces) + queueTableRows(workspace, []) + const deleteWhere = vi.fn().mockResolvedValue([]) + dbChainMock.db.delete.mockReturnValue({ where: deleteWhere }) + + await syncPersonalEnvCredentialsForUser({ userId: 'user-1' }) + + expect(flattenMockConditions(deleteWhere.mock.calls[0][0])).toEqual([ + { type: 'eq', left: credential.type, right: 'env_personal' }, + { type: 'eq', left: credential.envOwnerUserId, right: 'user-1' }, + { type: 'notInArray', column: credential.envKey, values: ['KEEP', 'NEW'] }, + ]) + if (activeWorkspaces.length === 0) { + expect(dbChainMockFns.insert).not.toHaveBeenCalled() + } else { + expect(dbChainMockFns.values).toHaveBeenCalledWith([ + expect.objectContaining({ workspaceId: 'active-workspace', envKey: 'KEEP' }), + expect.objectContaining({ workspaceId: 'active-workspace', envKey: 'NEW' }), + ]) + } + } + ) }) describe('createWorkspaceEnvCredentials', () => { diff --git a/apps/sim/lib/credentials/environment.ts b/apps/sim/lib/credentials/environment.ts index 4d77e3dcb01..1840847871c 100644 --- a/apps/sim/lib/credentials/environment.ts +++ b/apps/sim/lib/credentials/environment.ts @@ -4,6 +4,7 @@ import { credentialGroup, credentialGroupEnrollment, credentialMember, + environment, permissions, user, workspace, @@ -15,6 +16,7 @@ import { generateId } from '@sim/utils/id' import { and, asc, eq, inArray, isNotNull, isNull, notInArray, or, sql } from 'drizzle-orm' import { acquireUserBillingIdentityLock } from '@/lib/billing/organizations/billing-identity-lock' import { isManagedCredentialGroupBindingLive } from '@/lib/credential-groups/credentials' +import { lockPersonalEnvMap } from '@/lib/credentials/env-locks' import type { DbOrTx } from '@/lib/db/types' import { getEffectiveWorkspacePermission, @@ -677,15 +679,13 @@ export async function deletePersonalEnvCredentialForUser(params: { await db.transaction(remove) } -export async function syncPersonalEnvCredentialsForUser(params: { - userId: string - envKeys: string[] -}): Promise { - const { userId, envKeys } = params - const normalizedKeys = Array.from(new Set(envKeys.filter(Boolean))) +/** Reconciles user-global secret deletions and active-workspace mirrors against the locked map. */ +export async function syncPersonalEnvCredentialsForUser(params: { userId: string }): Promise { + const { userId } = params const now = new Date() await db.transaction(async (tx) => { + await lockPersonalEnvMap(tx, userId) /** * Cross-organization transfer takes this same user-identity fence before * checking source-owned credentials. If this sync wins, transfer observes @@ -693,85 +693,81 @@ export async function syncPersonalEnvCredentialsForUser(params: { * workspace re-read cannot recreate credentials in the departed org. */ await acquireUserBillingIdentityLock(tx, userId) - const workspaceIds = (await getUserWorkspaceIds(userId, tx)).sort() - - if (workspaceIds.length === 0) return - - if (normalizedKeys.length > 0) { - const credentialValues = workspaceIds.flatMap((workspaceId) => - normalizedKeys.map((envKey) => ({ - id: generateId(), - workspaceId, - type: 'env_personal' as const, - displayName: envKey, - envKey, - envOwnerUserId: userId, - createdBy: userId, - createdAt: now, - updatedAt: now, - })) + const [personalEnvironment] = await tx + .select({ variables: environment.variables }) + .from(environment) + .where(eq(environment.userId, userId)) + .limit(1) + const envKeys = Object.keys(personalEnvironment?.variables ?? {}).filter(Boolean) + + /** Deleted keys must lose mirrors even in archived or no-longer-accessible workspaces. */ + await tx + .delete(credential) + .where( + and( + eq(credential.type, 'env_personal'), + eq(credential.envOwnerUserId, userId), + envKeys.length > 0 ? notInArray(credential.envKey, envKeys) : undefined + ) ) - for (const values of chunkArray(credentialValues, ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) { - await tx.insert(credential).values(values).onConflictDoNothing() - } - const currentCredentials = await tx - .select({ id: credential.id }) - .from(credential) - .where( - and( - inArray(credential.workspaceId, workspaceIds), - eq(credential.type, 'env_personal'), - eq(credential.envOwnerUserId, userId), - inArray(credential.envKey, normalizedKeys) - ) - ) + if (envKeys.length === 0) return - if (currentCredentials.length > 0) { - const membershipValues = currentCredentials.map(({ id: credentialId }) => ({ - id: generateId(), - credentialId, - userId, - role: 'admin' as const, - status: 'active' as const, - joinedAt: now, - invitedBy: userId, - createdAt: now, - updatedAt: now, - })) - for (const values of chunkArray(membershipValues, ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) { - await tx - .insert(credentialMember) - .values(values) - .onConflictDoUpdate({ - target: [credentialMember.credentialId, credentialMember.userId], - set: { role: 'admin', status: 'active', updatedAt: now }, - }) - } - } + const workspaceIds = (await getUserWorkspaceIds(userId, tx)).sort() - await tx - .delete(credential) - .where( - and( - inArray(credential.workspaceId, workspaceIds), - eq(credential.type, 'env_personal'), - eq(credential.envOwnerUserId, userId), - notInArray(credential.envKey, normalizedKeys) - ) - ) - return + if (workspaceIds.length === 0) return + + const credentialValues = workspaceIds.flatMap((workspaceId) => + envKeys.map((envKey) => ({ + id: generateId(), + workspaceId, + type: 'env_personal' as const, + displayName: envKey, + envKey, + envOwnerUserId: userId, + createdBy: userId, + createdAt: now, + updatedAt: now, + })) + ) + for (const values of chunkArray(credentialValues, ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) { + await tx.insert(credential).values(values).onConflictDoNothing() } - await tx - .delete(credential) + const currentCredentials = await tx + .select({ id: credential.id }) + .from(credential) .where( and( inArray(credential.workspaceId, workspaceIds), eq(credential.type, 'env_personal'), - eq(credential.envOwnerUserId, userId) + eq(credential.envOwnerUserId, userId), + inArray(credential.envKey, envKeys) ) ) + + if (currentCredentials.length > 0) { + const membershipValues = currentCredentials.map(({ id: credentialId }) => ({ + id: generateId(), + credentialId, + userId, + role: 'admin' as const, + status: 'active' as const, + joinedAt: now, + invitedBy: userId, + createdAt: now, + updatedAt: now, + })) + for (const values of chunkArray(membershipValues, ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) { + await tx + .insert(credentialMember) + .values(values) + .onConflictDoUpdate({ + target: [credentialMember.credentialId, credentialMember.userId], + set: { role: 'admin', status: 'active', updatedAt: now }, + }) + } + } }) } diff --git a/apps/sim/lib/credentials/orchestration/index.ts b/apps/sim/lib/credentials/orchestration/index.ts index babe6aa6e1c..356a0084680 100644 --- a/apps/sim/lib/credentials/orchestration/index.ts +++ b/apps/sim/lib/credentials/orchestration/index.ts @@ -602,11 +602,8 @@ export async function deleteCredentialRecord( * Same read-modify-write on the personal map, under the same lock its * other writers take, with the mirrors removed in the same transaction. * - * Targeted rather than a reconcile: the reconcile prunes every mirror - * absent from a caller-supplied key list, so a secret added between the - * read and the prune lost its mirror while its value survived. Deleting - * this one key's mirrors cannot strand another secret, and the lock order - * — map, then user identity — is the one `setPersonalSecret` already takes. + * Delete only this key's mirrors across every workspace. The lock order + * — map, then user identity — matches `setPersonalSecret` and bulk sync. */ await db.transaction(async (tx) => { await lockPersonalEnvMap(tx, envOwnerUserId) diff --git a/apps/sim/lib/environment/utils.ts b/apps/sim/lib/environment/utils.ts index 0c72b260fb2..3e43c5f48ac 100644 --- a/apps/sim/lib/environment/utils.ts +++ b/apps/sim/lib/environment/utils.ts @@ -656,7 +656,7 @@ export async function upsertPersonalEnvVars( * plaintext. `added`/`updated` describe the earlier read and are reporting * only — the keys actually written are exactly the re-encrypted ones. */ - const finalEncrypted = await db.transaction(async (tx) => { + await db.transaction(async (tx) => { await lockPersonalEnvMap(tx, userId) const [currentRow] = await tx @@ -679,14 +679,11 @@ export async function upsertPersonalEnvVars( target: [environment.userId], set: { variables: merged, updatedAt: new Date() }, }) - - return merged }) invalidateEffectiveDecryptedEnvCache({ userId }) await syncPersonalEnvCredentialsForUser({ userId, - envKeys: Object.keys(finalEncrypted), }) return { added, updated } From e681bce4899632a246345da2bb47b5c66b8bdc12 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Wed, 16 Sep 2026 11:37:32 -0700 Subject: [PATCH 2/2] fix(mcp): use a deterministic clock for discovery timeout test --- apps/sim/lib/mcp/client.test.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/apps/sim/lib/mcp/client.test.ts b/apps/sim/lib/mcp/client.test.ts index bb7784bdbed..74025097399 100644 --- a/apps/sim/lib/mcp/client.test.ts +++ b/apps/sim/lib/mcp/client.test.ts @@ -2,7 +2,7 @@ * @vitest-environment node */ import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const { mockLogger, mockSdkConnect, mockSdkListTools, mockPinnedClose } = vi.hoisted(() => ({ mockLogger: { @@ -100,6 +100,10 @@ describe('McpClient notification handler', () => { vi.mocked(getMaxExecutionTimeout).mockReturnValue(30_000) }) + afterEach(() => { + vi.useRealTimers() + }) + it('preserves authorization-required errors raised by a locked credential reload', async () => { const error = new McpOauthAuthorizationRequiredError('server-1', 'Test Server') mockSdkConnect.mockRejectedValueOnce(error) @@ -199,6 +203,7 @@ describe('McpClient notification handler', () => { }) it('clamps a configured tools/list timeout to the absolute discovery ceiling', async () => { + vi.useFakeTimers() vi.mocked(getMaxExecutionTimeout).mockReturnValue(120_000) const client = new McpClient({ config: { ...createConfig(), timeout: 300_000 }, @@ -210,7 +215,7 @@ describe('McpClient notification handler', () => { expect(mockSdkListTools).toHaveBeenCalledWith( undefined, - expect.objectContaining({ timeout: 60_000, maxTotalTimeout: expect.any(Number) }) + expect.objectContaining({ timeout: 60_000, maxTotalTimeout: 60_000 }) ) })