fix(folders): close TOCTOU race in reorder writes and reject empty batches

Two more real findings, both confirmed against actual code:

- performReorderFolders validated every folder/parent at read time but the
  transactional write only filtered by id, with no re-check that the row
  (or a target parent) hadn't been concurrently soft-deleted in the window
  between validation and the write. A folder deleted mid-flight by a
  concurrent request could still get its sortOrder/parentId mutated, and a
  parent deleted mid-flight could leave an active folder pointing at a
  deleted one. Added a write-time deletedAt recheck on each row update
  (rolling back the whole transaction via a thrown error if any row no
  longer matches) and a fresh active-parent recheck inside the same
  transaction before any writes happen.

- The reorder contract had no lower bound on `updates`, so an empty array
  passed validation and crashed the route (validUpdates[0] was undefined)
  instead of failing cleanly. Added .min(1) to the contract schema, matching
  the .max(1000) bound already added and the .min(1) precedent already used
  elsewhere in this codebase for bulk-item arrays.

Added regression tests for both.

Author: waleedlatif1

apps/sim/app/api/folders/reorder/route.test.ts

@@ -39,6 +39,7 @@ describe('PUT /api/folders/reorder', () => {
   const mockWhere = vi.fn()
   const mockLimit = vi.fn()
   const mockTxUpdate = vi.fn()
+  const mockTxSelect = vi.fn()
 
   beforeEach(() => {
     vi.clearAllMocks()
@@ -50,10 +51,17 @@ describe('PUT /api/folders/reorder', () => {
     mockFrom.mockReturnValue({ where: mockWhere })
 
     mockTxUpdate.mockReturnValue({
-      set: vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue(undefined) }),
+      set: vi.fn().mockReturnValue({
+        where: vi.fn().mockReturnValue({
+          returning: vi.fn().mockResolvedValue([{ id: 'folder-1' }]),
+        }),
+      }),
+    })
+    mockTxSelect.mockReturnValue({
+      from: vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue([]) }),
     })
     mockDb.transaction.mockImplementation(async (cb: (tx: unknown) => Promise<unknown>) =>
-      cb({ update: mockTxUpdate })
+      cb({ update: mockTxUpdate, select: mockTxSelect })
     )
   })
 
@@ -205,6 +213,50 @@ describe('PUT /api/folders/reorder', () => {
     expect(mockDb.transaction).not.toHaveBeenCalled()
   })
 
+  it('rejects an empty updates array', async () => {
+    // Regression test: an empty `updates` array previously passed contract
+    // validation and crashed the route (validUpdates[0] was undefined) instead
+    // of failing cleanly.
+    const req = createMockRequest('PUT', { workspaceId: 'workspace-123', updates: [] })
+
+    const response = await PUT(req)
+
+    expect(response.status).toBe(400)
+    expect(mockDb.transaction).not.toHaveBeenCalled()
+  })
+
+  it('rolls back the whole batch when a folder is concurrently deleted before the write', async () => {
+    // Regression test: the transactional write previously only filtered by id,
+    // so a folder soft-deleted between validation and the transaction could
+    // still have its sortOrder/parentId mutated. The write-time recheck should
+    // find no matching row and roll back rather than silently applying it.
+    mockWhere
+      .mockReturnValueOnce([
+        { id: 'folder-1', workspaceId: 'workspace-123', resourceType: 'workflow' },
+      ])
+      .mockReturnValueOnce([{ id: 'folder-1', parentId: null }])
+      .mockReturnValueOnce([{ id: 'folder-1', workspaceId: 'workspace-123' }])
+
+    mockTxUpdate.mockReturnValue({
+      set: vi.fn().mockReturnValue({
+        where: vi.fn().mockReturnValue({
+          returning: vi.fn().mockResolvedValue([]), // row no longer matches (deletedAt set)
+        }),
+      }),
+    })
+
+    const req = createMockRequest('PUT', {
+      workspaceId: 'workspace-123',
+      updates: [{ id: 'folder-1', sortOrder: 2, parentId: null }],
+    })
+
+    const response = await PUT(req)
+
+    expect(response.status).toBe(400)
+    const data = await response.json()
+    expect(data.error).toBe('One or more folders were not found')
+  })
+
   it('rejects a batch that would form a cycle', async () => {
     mockWhere
       .mockReturnValueOnce([

apps/sim/lib/api/contracts/folders.ts

@@ -80,6 +80,7 @@ export const reorderFoldersBodySchema = z.object({
         parentId: z.string().nullable().optional(),
       })
     )
+    .min(1, 'At least one folder must be provided')
     .max(1000, 'At most 1000 folders can be reordered at once'),
 })
 

apps/sim/lib/folders/orchestration.ts

@@ -822,16 +822,47 @@ export async function performReorderFolders(
     return { success: false, updated: 0, error: firstParentError.error }
   }
 
-  await db.transaction(async (tx) => {
-    for (const update of validUpdates) {
-      const updateData: Record<string, unknown> = {
-        sortOrder: update.sortOrder,
-        updatedAt: new Date(),
+  try {
+    await db.transaction(async (tx) => {
+      // Re-check each target parent is still active inside the transaction --
+      // assertFolderParentValid above only reads at validation time, so a parent
+      // concurrently soft-deleted before this transaction opens could otherwise
+      // leave an active folder pointing at a deleted one.
+      if (targetParentIds.length > 0) {
+        const activeParents = await tx
+          .select({ id: folderTable.id })
+          .from(folderTable)
+          .where(and(inArray(folderTable.id, targetParentIds), isNull(folderTable.deletedAt)))
+        if (activeParents.length !== targetParentIds.length) {
+          throw new Error('Parent folder not found')
+        }
       }
-      if (update.parentId !== undefined) updateData.parentId = update.parentId || null
-      await tx.update(folderTable).set(updateData).where(eq(folderTable.id, update.id))
-    }
-  })
+
+      for (const update of validUpdates) {
+        const updateData: Record<string, unknown> = {
+          sortOrder: update.sortOrder,
+          updatedAt: new Date(),
+        }
+        if (update.parentId !== undefined) updateData.parentId = update.parentId || null
+
+        // Re-check deletedAt at write time (not just the validation read above) --
+        // without this, a folder concurrently soft-deleted between validation and
+        // this transaction could still have its sortOrder/parentId mutated. Throwing
+        // rolls back the whole transaction rather than silently applying a partial
+        // batch, matching this function's fail-whole-batch behavior on other errors.
+        const [updated] = await tx
+          .update(folderTable)
+          .set(updateData)
+          .where(and(eq(folderTable.id, update.id), isNull(folderTable.deletedAt)))
+          .returning({ id: folderTable.id })
+        if (!updated) {
+          throw new Error('One or more folders were not found')
+        }
+      }
+    })
+  } catch (error) {
+    return { success: false, updated: 0, error: toError(error).message }
+  }
 
   return { success: true, updated: validUpdates.length }
 }