mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
fix(locks): enforce workflow/folder locks on the agent + close manual-UI create gaps (#5107)
* fix(locks): enforce workflow/folder locks on the agent + close manual-UI create gaps The copilot/agent workflow & folder mutation tools and the edit_workflow tool bypassed lock enforcement, so the agent could edit a locked workflow and move/create workflows into a locked folder. Add assertWorkflowMutable/ assertFolderMutable guards (from @sim/workflow-authz) to every agent mutation path, mirroring the REST API. Also close two parity gaps on the manual-UI REST side: creating a workflow into a locked folder and creating a subfolder under a locked parent were previously unguarded. The realtime collaborative canvas already enforced workflow-level locks server-side. * fix(locks): normalize optional folderId to null for assertFolderMutable * refactor(locks): hoist constant folder-lock check out of move loop; scope test mocks with Once * refactor(locks): drop redundant ensureWorkflowAccess fetch in rename
This commit is contained in:
@@ -9,6 +9,7 @@ import {
|
||||
createMockRequest,
|
||||
permissionsMock,
|
||||
permissionsMockFns,
|
||||
workflowAuthzMockFns,
|
||||
} from '@sim/testing'
|
||||
import { drizzleOrmMock } from '@sim/testing/mocks'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
@@ -390,6 +391,26 @@ describe('Folders API Route', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('should reject creating a subfolder inside a locked parent folder', async () => {
|
||||
mockAuthenticatedUser()
|
||||
|
||||
const { FolderLockedError } = await import('@sim/workflow-authz')
|
||||
workflowAuthzMockFns.mockAssertFolderMutable.mockRejectedValueOnce(
|
||||
new FolderLockedError('Folder is locked')
|
||||
)
|
||||
|
||||
const req = createMockRequest('POST', {
|
||||
name: 'Subfolder',
|
||||
workspaceId: 'workspace-123',
|
||||
parentId: 'locked-folder',
|
||||
})
|
||||
|
||||
const response = await POST(req)
|
||||
|
||||
expect(response.status).toBe(423)
|
||||
expect(mockTransaction).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('should reject a parentId that does not resolve to a folder in the workspace', async () => {
|
||||
mockAuthenticatedUser()
|
||||
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import { db } from '@sim/db'
|
||||
import { workflowFolder } from '@sim/db/schema'
|
||||
import { createLogger } from '@sim/logger'
|
||||
import { assertFolderMutable, FolderLockedError } from '@sim/workflow-authz'
|
||||
import { and, asc, eq, isNotNull, isNull } from 'drizzle-orm'
|
||||
import { type NextRequest, NextResponse } from 'next/server'
|
||||
import { createFolderContract, listFoldersContract } from '@/lib/api/contracts'
|
||||
@@ -93,6 +94,8 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
|
||||
)
|
||||
}
|
||||
|
||||
await assertFolderMutable(parentId ?? null)
|
||||
|
||||
const result = await performCreateFolder({
|
||||
id: clientId,
|
||||
userId: session.user.id,
|
||||
@@ -123,6 +126,9 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
|
||||
|
||||
return NextResponse.json({ folder: newFolder })
|
||||
} catch (error) {
|
||||
if (error instanceof FolderLockedError) {
|
||||
return NextResponse.json({ error: error.message }, { status: error.status })
|
||||
}
|
||||
logger.error('Error creating folder:', { error })
|
||||
return NextResponse.json({ error: 'Internal server error' }, { status: 500 })
|
||||
}
|
||||
|
||||
@@ -7,6 +7,7 @@ import {
|
||||
hybridAuthMockFns,
|
||||
permissionsMock,
|
||||
permissionsMockFns,
|
||||
workflowAuthzMockFns,
|
||||
workflowsApiUtilsMock,
|
||||
workflowsPersistenceUtilsMock,
|
||||
workflowsPersistenceUtilsMockFns,
|
||||
@@ -80,11 +81,30 @@ describe('Workflows API Route - POST ordering', () => {
|
||||
userEmail: 'test@example.com',
|
||||
})
|
||||
mockGetUserEntityPermissions.mockResolvedValue('write')
|
||||
workflowAuthzMockFns.mockAssertFolderMutable.mockResolvedValue(undefined)
|
||||
workflowsPersistenceUtilsMockFns.mockSaveWorkflowToNormalizedTables.mockResolvedValue({
|
||||
success: true,
|
||||
})
|
||||
})
|
||||
|
||||
it('rejects creating a workflow inside a locked folder', async () => {
|
||||
const { FolderLockedError } = await import('@sim/workflow-authz')
|
||||
workflowAuthzMockFns.mockAssertFolderMutable.mockRejectedValueOnce(
|
||||
new FolderLockedError('Folder is locked')
|
||||
)
|
||||
|
||||
const req = createMockRequest('POST', {
|
||||
name: 'New Workflow',
|
||||
description: 'desc',
|
||||
workspaceId: 'workspace-123',
|
||||
folderId: 'locked-folder',
|
||||
})
|
||||
|
||||
const response = await POST(req)
|
||||
expect(response.status).toBe(423)
|
||||
expect(mockDbInsert).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('uses top insertion against mixed siblings (folders + workflows)', async () => {
|
||||
const minResultsQueue: Array<Array<{ minOrder: number }>> = [
|
||||
[],
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import { createLogger } from '@sim/logger'
|
||||
import { assertFolderMutable, FolderLockedError } from '@sim/workflow-authz'
|
||||
import { type NextRequest, NextResponse } from 'next/server'
|
||||
import { createWorkflowContract, workflowListQuerySchema } from '@/lib/api/contracts/workflows'
|
||||
import { parseRequest } from '@/lib/api/server'
|
||||
@@ -116,6 +117,8 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
|
||||
)
|
||||
}
|
||||
|
||||
await assertFolderMutable(folderId ?? null)
|
||||
|
||||
const result = await performCreateWorkflow({
|
||||
id: clientId,
|
||||
name: requestedName,
|
||||
@@ -180,6 +183,9 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
|
||||
subBlockValues: createdWorkflow.subBlockValues,
|
||||
})
|
||||
} catch (error) {
|
||||
if (error instanceof FolderLockedError) {
|
||||
return NextResponse.json({ error: error.message }, { status: error.status })
|
||||
}
|
||||
logger.error(`[${requestId}] Error creating workflow`, error)
|
||||
return NextResponse.json({ error: 'Failed to create workflow' }, { status: 500 })
|
||||
}
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
/**
|
||||
* @vitest-environment node
|
||||
*/
|
||||
import { createEnvMock } from '@sim/testing'
|
||||
import { createEnvMock, workflowAuthzMockFns } from '@sim/testing'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
const {
|
||||
@@ -88,7 +88,16 @@ vi.mock('../access', () => ({
|
||||
getDefaultWorkspaceId: vi.fn(),
|
||||
}))
|
||||
|
||||
import { executeRunFromBlock, executeSetGlobalWorkflowVariables } from './mutations'
|
||||
import { performUpdateWorkflow } from '@/lib/workflows/orchestration'
|
||||
import { verifyFolderWorkspace } from '@/lib/workflows/utils'
|
||||
import {
|
||||
executeMoveWorkflow,
|
||||
executeRunFromBlock,
|
||||
executeSetGlobalWorkflowVariables,
|
||||
} from './mutations'
|
||||
|
||||
const performUpdateWorkflowMock = vi.mocked(performUpdateWorkflow)
|
||||
const verifyFolderWorkspaceMock = vi.mocked(verifyFolderWorkspace)
|
||||
|
||||
describe('executeSetGlobalWorkflowVariables', () => {
|
||||
beforeEach(() => {
|
||||
@@ -134,6 +143,56 @@ describe('executeSetGlobalWorkflowVariables', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('lock enforcement', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
global.fetch = vi.fn().mockResolvedValue(new Response(null, { status: 200 })) as typeof fetch
|
||||
workflowAuthzMockFns.mockAssertWorkflowMutable.mockResolvedValue(undefined)
|
||||
workflowAuthzMockFns.mockAssertFolderMutable.mockResolvedValue(undefined)
|
||||
})
|
||||
|
||||
it('does not persist variable changes when the workflow is locked', async () => {
|
||||
ensureWorkflowAccessMock.mockResolvedValue({
|
||||
workflow: { id: 'workflow-1', variables: {} },
|
||||
})
|
||||
workflowAuthzMockFns.mockAssertWorkflowMutable.mockRejectedValueOnce(
|
||||
new Error('Workflow is locked')
|
||||
)
|
||||
|
||||
const result = await executeSetGlobalWorkflowVariables(
|
||||
{
|
||||
workflowId: 'workflow-1',
|
||||
operations: [{ operation: 'add', name: 'threshold', type: 'number', value: '5' }],
|
||||
},
|
||||
{ userId: 'user-1' } as any
|
||||
)
|
||||
|
||||
expect(result.success).toBe(false)
|
||||
expect(result.error).toBe('Workflow is locked')
|
||||
expect(setWorkflowVariablesMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does not move a workflow into a locked target folder', async () => {
|
||||
ensureWorkflowAccessMock.mockResolvedValue({
|
||||
workspaceId: 'workspace-1',
|
||||
workflow: { id: 'workflow-1', name: 'WF', folderId: null },
|
||||
})
|
||||
verifyFolderWorkspaceMock.mockResolvedValue(true)
|
||||
workflowAuthzMockFns.mockAssertFolderMutable.mockRejectedValueOnce(
|
||||
new Error('Folder is locked')
|
||||
)
|
||||
|
||||
const result = await executeMoveWorkflow(
|
||||
{ workflowIds: ['workflow-1'], folderId: 'locked-folder' },
|
||||
{ userId: 'user-1' } as any
|
||||
)
|
||||
|
||||
expect(result.success).toBe(false)
|
||||
expect(result.error).toBe('Folder is locked')
|
||||
expect(performUpdateWorkflowMock).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
describe('executeRunFromBlock', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
|
||||
@@ -3,6 +3,7 @@ import { db, workflow as workflowTable } from '@sim/db'
|
||||
import { createLogger } from '@sim/logger'
|
||||
import { toError } from '@sim/utils/errors'
|
||||
import { generateId } from '@sim/utils/id'
|
||||
import { assertFolderMutable, assertWorkflowMutable } from '@sim/workflow-authz'
|
||||
import { mergeSubblockStateWithValues } from '@sim/workflow-persistence/subblocks'
|
||||
import { eq } from 'drizzle-orm'
|
||||
import { performCreateWorkspaceApiKey } from '@/lib/api-key/orchestration'
|
||||
@@ -353,6 +354,7 @@ export async function executeCreateWorkflow(
|
||||
const folderId = params?.folderId || null
|
||||
|
||||
await ensureWorkspaceAccess(workspaceId, context.userId, 'write')
|
||||
await assertFolderMutable(folderId)
|
||||
assertWorkflowMutationNotAborted(context)
|
||||
|
||||
const result = await performCreateWorkflow({
|
||||
@@ -422,6 +424,7 @@ export async function executeCreateFolder(
|
||||
const parentId = params?.parentId || null
|
||||
|
||||
await ensureWorkspaceAccess(workspaceId, context.userId, 'write')
|
||||
await assertFolderMutable(parentId)
|
||||
assertWorkflowMutationNotAborted(context)
|
||||
|
||||
const result = await performCreateFolder({
|
||||
@@ -512,6 +515,7 @@ export async function executeSetGlobalWorkflowVariables(
|
||||
context.userId,
|
||||
'write'
|
||||
)
|
||||
await assertWorkflowMutable(workflowId)
|
||||
|
||||
interface WorkflowVariable {
|
||||
id: string
|
||||
@@ -633,9 +637,9 @@ export async function executeRenameWorkflow(
|
||||
return { success: false, error: 'Workflow name must be 200 characters or less' }
|
||||
}
|
||||
|
||||
await ensureWorkflowAccess(workflowId, context.userId, 'write')
|
||||
assertWorkflowMutationNotAborted(context)
|
||||
const current = await ensureWorkflowAccess(workflowId, context.userId, 'write')
|
||||
await assertWorkflowMutable(workflowId)
|
||||
assertWorkflowMutationNotAborted(context)
|
||||
if (!current.workspaceId) {
|
||||
return { success: false, error: 'Workflow workspace is required' }
|
||||
}
|
||||
@@ -671,6 +675,8 @@ export async function executeMoveWorkflow(
|
||||
const moved: string[] = []
|
||||
const failed: string[] = []
|
||||
|
||||
await assertFolderMutable(folderId)
|
||||
|
||||
for (const workflowId of workflowIds) {
|
||||
try {
|
||||
const { workspaceId, workflow } = await ensureWorkflowAccess(
|
||||
@@ -688,6 +694,7 @@ export async function executeMoveWorkflow(
|
||||
continue
|
||||
}
|
||||
}
|
||||
await assertWorkflowMutable(workflowId)
|
||||
assertWorkflowMutationNotAborted(context)
|
||||
const result = await performUpdateWorkflow({
|
||||
workflowId,
|
||||
@@ -735,6 +742,8 @@ export async function executeMoveFolder(
|
||||
return { success: false, error: 'Parent folder not found' }
|
||||
}
|
||||
|
||||
await assertFolderMutable(folderId)
|
||||
await assertFolderMutable(parentId)
|
||||
assertWorkflowMutationNotAborted(context)
|
||||
const result = await performUpdateFolder({
|
||||
folderId,
|
||||
@@ -941,6 +950,7 @@ async function executeUpdateWorkflow(
|
||||
if (!current.workspaceId) {
|
||||
return { success: false, error: 'Workflow workspace is required' }
|
||||
}
|
||||
await assertWorkflowMutable(workflowId)
|
||||
assertWorkflowMutationNotAborted(context)
|
||||
const result = await performUpdateWorkflow({
|
||||
workflowId,
|
||||
@@ -984,6 +994,7 @@ export async function executeSetBlockEnabled(
|
||||
context.userId,
|
||||
'write'
|
||||
)
|
||||
await assertWorkflowMutable(workflowId)
|
||||
assertWorkflowMutationNotAborted(context)
|
||||
|
||||
const normalized = await loadWorkflowFromNormalizedTables(workflowId)
|
||||
@@ -1114,6 +1125,7 @@ export async function executeDeleteWorkflow(
|
||||
context.userId,
|
||||
'write'
|
||||
)
|
||||
await assertWorkflowMutable(workflowId)
|
||||
assertWorkflowMutationNotAborted(context)
|
||||
|
||||
const result = await performDeleteWorkflow({ workflowId, userId: context.userId })
|
||||
@@ -1162,16 +1174,22 @@ export async function executeDeleteFolder(
|
||||
|
||||
assertWorkflowMutationNotAborted(context)
|
||||
|
||||
const result = await performDeleteFolder({
|
||||
folderId,
|
||||
workspaceId,
|
||||
userId: context.userId,
|
||||
folderName: folder.folderName,
|
||||
})
|
||||
try {
|
||||
await assertFolderMutable(folderId)
|
||||
|
||||
if (result.success) {
|
||||
deleted.push(folderId)
|
||||
} else {
|
||||
const result = await performDeleteFolder({
|
||||
folderId,
|
||||
workspaceId,
|
||||
userId: context.userId,
|
||||
folderName: folder.folderName,
|
||||
})
|
||||
|
||||
if (result.success) {
|
||||
deleted.push(folderId)
|
||||
} else {
|
||||
failed.push(folderId)
|
||||
}
|
||||
} catch {
|
||||
failed.push(folderId)
|
||||
}
|
||||
}
|
||||
@@ -1206,6 +1224,7 @@ async function executeRenameFolder(
|
||||
return { success: false, error: 'Folder not found' }
|
||||
}
|
||||
|
||||
await assertFolderMutable(folderId)
|
||||
assertWorkflowMutationNotAborted(context)
|
||||
const result = await performUpdateFolder({
|
||||
folderId,
|
||||
|
||||
@@ -2,7 +2,7 @@ import { db } from '@sim/db'
|
||||
import { workflow as workflowTable } from '@sim/db/schema'
|
||||
import { createLogger } from '@sim/logger'
|
||||
import { toError } from '@sim/utils/errors'
|
||||
import { authorizeWorkflowByWorkspacePermission } from '@sim/workflow-authz'
|
||||
import { assertWorkflowMutable, authorizeWorkflowByWorkspacePermission } from '@sim/workflow-authz'
|
||||
import { eq } from 'drizzle-orm'
|
||||
import { EditWorkflow } from '@/lib/copilot/generated/tool-catalog-v1'
|
||||
import {
|
||||
@@ -106,6 +106,8 @@ export const editWorkflowServerTool: BaseServerTool<EditWorkflowParams, unknown>
|
||||
throw new Error(authorization.message || 'Unauthorized workflow access')
|
||||
}
|
||||
|
||||
await assertWorkflowMutable(workflowId)
|
||||
|
||||
const workspaceId = authorization.workflow?.workspaceId ?? undefined
|
||||
const workflowName = authorization.workflow?.name ?? undefined
|
||||
|
||||
|
||||
Reference in New Issue
Block a user