fix(folders): close reorder lock TOCTOU race and archived-folder PUT gap

Two more real findings, both confirmed against actual code:

- The reorder route checks lock state before calling performReorderFolders,
  but that check and the transactional write are separate round-trips -- an
  admin locking a source folder or target parent in that window wasn't
  re-checked before the write applied. Extended the shared lock engine
  (getFolderLockStatus/assertFolderMutable in
  @sim/platform-authz/resource-lock) with an optional dbClient parameter
  (mirrors assertFolderParentValid's existing DbOrTx pattern) so the
  reorder transaction can re-check every source folder and target parent
  using the same `tx` the writes run in, rather than reimplementing the
  lock-walk ad hoc.

- PUT/DELETE /api/folders/[id] loaded the target folder by id only, and
  performUpdateFolder's workflow and knowledge_base/table branches updated
  rows without requiring deletedAt to be null -- an archived folder could
  still be renamed, reparented, or relocked. The file-folder and reorder
  paths already filtered this correctly; added the same isNull(deletedAt)
  filter to the route's existingFolder lookup and both performUpdateFolder
  branches' UPDATE clauses.

Added regression tests for both.

Author: waleedlatif1

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

@@ -207,6 +207,36 @@ describe('Individual Folder API Route', () => {
       })
     })
 
+    it('returns 404 for an archived (soft-deleted) folder rather than updating it', async () => {
+      // Regression test: the existingFolder lookup previously had no deletedAt
+      // filter, so a soft-deleted folder could still be renamed/reparented/
+      // relocked via this route even though it no longer appears anywhere in
+      // the UI. Simulates the lookup finding nothing (as it now would for an
+      // archived row, since the route filters isNull(deletedAt)).
+      mockAuthenticatedUser()
+      mockDbRef.current = {
+        select: vi.fn().mockReturnValue({
+          from: vi.fn().mockReturnValue({
+            where: vi.fn().mockReturnValue({
+              then: vi.fn().mockImplementation((callback) => Promise.resolve(callback([]))),
+            }),
+          }),
+        }),
+        update: vi.fn(),
+        delete: vi.fn(),
+      }
+
+      const req = createMockRequest('PUT', { name: 'Should not apply' })
+      const params = Promise.resolve({ id: 'folder-1' })
+
+      const response = await PUT(req, { params })
+
+      expect(response.status).toBe(404)
+      const data = await response.json()
+      expect(data.error).toBe('Folder not found')
+      expect(mockPerformUpdateFolder).not.toHaveBeenCalled()
+    })
+
     it('should update parent folder successfully', async () => {
       mockAuthenticatedUser()
 

apps/sim/app/api/folders/[id]/route.ts

@@ -2,7 +2,7 @@ import { db } from '@sim/db'
 import { folder } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
 import { ResourceLockedError } from '@sim/platform-authz/resource-lock'
-import { eq } from 'drizzle-orm'
+import { and, eq, isNull } from 'drizzle-orm'
 import { type NextRequest, NextResponse } from 'next/server'
 import { updateFolderContract } from '@/lib/api/contracts'
 import { parseRequest } from '@/lib/api/server'
@@ -43,7 +43,7 @@ export const PUT = withRouteHandler(
       const existingFolder = await db
         .select()
         .from(folder)
-        .where(eq(folder.id, id))
+        .where(and(eq(folder.id, id), isNull(folder.deletedAt)))
         .then((rows) => rows[0])
 
       if (!existingFolder) {
@@ -130,7 +130,7 @@ export const DELETE = withRouteHandler(
       const existingFolder = await db
         .select()
         .from(folder)
-        .where(eq(folder.id, id))
+        .where(and(eq(folder.id, id), isNull(folder.deletedAt)))
         .then((rows) => rows[0])
 
       if (!existingFolder) {

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

@@ -3,7 +3,15 @@
  *
  * @vitest-environment node
  */
-import { authMockFns, createMockRequest, permissionsMock, permissionsMockFns } from '@sim/testing'
+
+import { ResourceLockedError } from '@sim/platform-authz/resource-lock'
+import {
+  authMockFns,
+  createMockRequest,
+  permissionsMock,
+  permissionsMockFns,
+  resourceLockMockFns,
+} from '@sim/testing'
 import { drizzleOrmMock } from '@sim/testing/mocks'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
@@ -43,6 +51,8 @@ describe('PUT /api/folders/reorder', () => {
 
   beforeEach(() => {
     vi.clearAllMocks()
+    resourceLockMockFns.mockAssertFolderMutable.mockReset()
+    resourceLockMockFns.mockAssertFolderMutable.mockResolvedValue(undefined)
 
     authMockFns.mockGetSession.mockResolvedValue({ user: { id: 'user-123' } })
     mockGetUserEntityPermissions.mockResolvedValue('admin')
@@ -257,6 +267,39 @@ describe('PUT /api/folders/reorder', () => {
     expect(data.error).toBe('One or more folders were not found')
   })
 
+  it('returns a 423 when a folder is concurrently locked before the write', async () => {
+    // Regression test: the route checks lock state before calling
+    // performReorderFolders (both of the route's own checks -- for update.id and
+    // update.parentId, since parentId is `null` not `undefined` -- succeed below,
+    // simulating the folder being unlocked at that moment), but that's a separate
+    // round-trip -- an admin could lock the folder in the window between that
+    // check and the transaction. The third assertFolderMutable call is the new
+    // in-transaction recheck this test targets.
+    mockWhere
+      .mockReturnValueOnce([
+        { id: 'folder-1', workspaceId: 'workspace-123', resourceType: 'workflow' },
+      ])
+      .mockReturnValueOnce([{ id: 'folder-1', parentId: null }])
+      .mockReturnValueOnce([{ id: 'folder-1', workspaceId: 'workspace-123' }])
+
+    resourceLockMockFns.mockAssertFolderMutable
+      .mockResolvedValueOnce(undefined)
+      .mockResolvedValueOnce(undefined)
+      .mockRejectedValueOnce(new ResourceLockedError('workflow', false, 'Folder is locked'))
+
+    const req = createMockRequest('PUT', {
+      workspaceId: 'workspace-123',
+      updates: [{ id: 'folder-1', sortOrder: 2, parentId: null }],
+    })
+
+    const response = await PUT(req)
+
+    expect(response.status).toBe(423)
+    expect(resourceLockMockFns.mockAssertFolderMutable).toHaveBeenCalledTimes(3)
+    expect(mockDb.transaction).toHaveBeenCalledTimes(1)
+    expect(mockTxUpdate).not.toHaveBeenCalled()
+  })
+
   it('rejects a batch that would form a cycle', async () => {
     mockWhere
       .mockReturnValueOnce([

apps/sim/lib/folders/orchestration.ts

@@ -729,7 +729,8 @@ export async function performUpdateFolder(
       and(
         eq(folderTable.id, params.folderId),
         eq(folderTable.workspaceId, params.workspaceId),
-        eq(folderTable.resourceType, params.resourceType)
+        eq(folderTable.resourceType, params.resourceType),
+        isNull(folderTable.deletedAt)
       )
     )
     .returning()
@@ -838,6 +839,18 @@ export async function performReorderFolders(
         }
       }
 
+      // The route checks lock state before calling this function, but that's a
+      // separate round-trip -- an admin could lock a source folder or a target
+      // parent in the window between that check and this transaction. Re-check
+      // both inside the transaction (joining `tx` so the read is part of the
+      // same atomic unit as the writes below) before applying anything.
+      for (const update of validUpdates) {
+        await assertFolderMutable(update.id, resourceType, tx)
+      }
+      for (const parentId of targetParentIds) {
+        await assertFolderMutable(parentId, resourceType, tx)
+      }
+
       for (const update of validUpdates) {
         const updateData: Record<string, unknown> = {
           sortOrder: update.sortOrder,
@@ -861,6 +874,10 @@ export async function performReorderFolders(
       }
     })
   } catch (error) {
+    // Propagate uncaught so the route's ResourceLockedError -> 423 handling applies.
+    if (error instanceof ResourceLockedError) {
+      throw error
+    }
     return { success: false, updated: 0, error: toError(error).message }
   }
 

apps/sim/lib/workflows/orchestration/folder-lifecycle.ts

@@ -193,7 +193,8 @@ export async function performUpdateFolder(
         and(
           eq(folder.id, params.folderId),
           eq(folder.workspaceId, params.workspaceId),
-          isWorkflowFolder
+          isWorkflowFolder,
+          isNull(folder.deletedAt)
         )
       )
       .returning()

packages/platform-authz/src/resource-lock.ts

@@ -7,11 +7,23 @@ import {
   workflow,
   workspaceFiles,
 } from '@sim/db'
+import type * as schema from '@sim/db/schema'
+import type { ExtractTablesWithRelations } from 'drizzle-orm'
 import { and, eq, isNull } from 'drizzle-orm'
-import type { PgColumn, PgTable } from 'drizzle-orm/pg-core'
+import type { PgColumn, PgTable, PgTransaction } from 'drizzle-orm/pg-core'
+import type { PostgresJsQueryResultHKT } from 'drizzle-orm/postgres-js'
 
 export type FolderResourceType = (typeof folderResourceTypeEnum.enumValues)[number]
 
+/** Allows a caller to join its own transaction instead of the module-level `db`. */
+type DbOrTx =
+  | typeof db
+  | PgTransaction<
+      PostgresJsQueryResultHKT,
+      typeof schema,
+      ExtractTablesWithRelations<typeof schema>
+    >
+
 export interface LockStatus {
   locked: boolean
   directLocked: boolean
@@ -94,7 +106,8 @@ const UNLOCKED_STATUS: LockStatus = {
  */
 export async function getFolderLockStatus(
   folderId: string | null,
-  resourceType: FolderResourceType
+  resourceType: FolderResourceType,
+  dbClient: DbOrTx = db
 ): Promise<LockStatus> {
   if (!folderId) return UNLOCKED_STATUS
 
@@ -104,7 +117,7 @@ export async function getFolderLockStatus(
 
   while (currentFolderId && !visited.has(currentFolderId)) {
     visited.add(currentFolderId)
-    const [folderRow] = await db
+    const [folderRow] = await dbClient
       .select({
         id: folder.id,
         parentId: folder.parentId,
@@ -174,9 +187,10 @@ export async function getResourceLockStatus(
 
 export async function assertFolderMutable(
   folderId: string | null,
-  resourceType: FolderResourceType
+  resourceType: FolderResourceType,
+  dbClient: DbOrTx = db
 ): Promise<void> {
-  const status = await getFolderLockStatus(folderId, resourceType)
+  const status = await getFolderLockStatus(folderId, resourceType, dbClient)
   if (status.locked) {
     throw new ResourceLockedError(
       resourceType,