fix(folders): fix source-lock bypass in unlock skip, add in-tx recheck to updates

Two more findings this round:

1. Greptile P1 security: the "combined unlock + other field change" fix from an
   earlier round skipped the mutable pre-check entirely whenever the request set
   locked: false, but that check also covers a lock INHERITED from an ancestor
   folder -- clearing this object's own locked flag has no effect on that. An
   admin could unlock a folder while also moving it out from under a still-locked
   ancestor, since the ancestor-chain check was skipped along with the direct one.

   Replaced the "skip if unlocking" pattern with two new shared primitives,
   assertFolderMutableUnlessUnlocking / assertResourceMutableUnlessUnlocking
   (packages/platform-authz/src/resource-lock.ts): they still run the full
   mutable check, and only swallow a DIRECT (non-inherited) lock error when the
   request is unlocking in the same write. An inherited lock still blocks.
   Applied at all 5 call sites that had the old broad skip: the generic folders
   PUT route, renameTable, updateKnowledgeBase, performRenameWorkspaceFile, and
   the new in-transaction recheck added below.

2. Cursor: generic folder updates for knowledge_base/table (performUpdateFolder's
   plain branch) and file-folder updates (updateWorkspaceFileFolder) applied
   their UPDATE from a single statement after only the route's pre-transaction
   assertMutable call -- unlike reorder/restore, there was no in-transaction
   lock recheck, so a folder or ancestor locked in the window between that
   pre-check and the write could still be mutated. Wrapped the kb/table plain
   update path in a transaction with an in-tx recheck (joining `tx` so it's
   part of the same atomic unit as the write); added the equivalent recheck
   to updateWorkspaceFileFolder's existing transaction, plus a target-parent
   lock check that was previously entirely missing there (existence was
   checked, lock state was not).

Updated the 4 existing "combined unlock" tests to reflect the corrected
behavior (the check still runs; a direct lock is satisfied by unlocking, an
inherited lock is not) and added a companion "still rejects when inherited"
test at each of those 4 sites. Updated the shared @sim/testing resource-lock
mock and knowledge/service.test.ts's local mock to implement the new
wrappers' real catch logic rather than a bare passthrough, so tests that
configure a rejection see the same behavior as production.

Full local verification passed (typecheck, full vitest suite 11436 tests,
check:api-validation, lint all clean).

Author: waleedlatif1

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

@@ -3,6 +3,7 @@
  *
  * @vitest-environment node
  */
+import { ResourceLockedError } from '@sim/platform-authz/resource-lock'
 import {
   auditMock,
   authMockFns,
@@ -238,29 +239,55 @@ describe('Individual Folder API Route', () => {
       expect(mockPerformUpdateFolder).not.toHaveBeenCalled()
     })
 
-    it('skips the mutable check when unlocking a locked folder combined with a move in the same request', async () => {
+    it('allows unlocking a directly-locked folder combined with a move in the same request', async () => {
       // Regression test: hasNonLockUpdate is true whenever parentId also changes, so
       // a combined "unlock + move" request previously still ran
       // policy.assertMutable(id) against the folder's current (still-locked) state
       // and was incorrectly rejected, even though the request unlocks it as part of
-      // this same atomic write.
+      // this same atomic write. The fixed behavior still runs the check (so an
+      // inherited lock is caught below), but treats a DIRECT lock as satisfied
+      // since this request clears it.
       mockAuthenticatedUser()
       resourceLockMockFns.mockAssertFolderMutable.mockReset()
       resourceLockMockFns.mockAssertFolderMutable.mockResolvedValue(undefined)
+      resourceLockMockFns.mockAssertFolderMutable.mockRejectedValueOnce(
+        new ResourceLockedError('workflow', false, 'Folder is locked')
+      )
 
       const req = createMockRequest('PUT', { locked: false, parentId: 'folder-2' })
       const params = Promise.resolve({ id: 'folder-1' })
 
       const response = await PUT(req, { params })
 
       expect(response.status).toBe(200)
-      expect(resourceLockMockFns.mockAssertFolderMutable).toHaveBeenCalledTimes(1)
+      expect(resourceLockMockFns.mockAssertFolderMutable).toHaveBeenCalledTimes(2)
+      expect(resourceLockMockFns.mockAssertFolderMutable).toHaveBeenCalledWith(
+        'folder-1',
+        'workflow'
+      )
       expect(resourceLockMockFns.mockAssertFolderMutable).toHaveBeenCalledWith(
         'folder-2',
         'workflow'
       )
     })
 
+    it('still rejects unlocking a folder combined with a move when the lock is inherited from an ancestor', async () => {
+      // Clearing the folder's own `locked` flag doesn't affect a lock inherited from
+      // an ancestor -- that must still block the combined request.
+      mockAuthenticatedUser()
+      resourceLockMockFns.mockAssertFolderMutable.mockReset()
+      resourceLockMockFns.mockAssertFolderMutable.mockRejectedValueOnce(
+        new ResourceLockedError('workflow', true, 'Folder is locked by an ancestor folder')
+      )
+
+      const req = createMockRequest('PUT', { locked: false, parentId: 'folder-2' })
+      const params = Promise.resolve({ id: 'folder-1' })
+
+      const response = await PUT(req, { params })
+
+      expect(response.status).toBe(423)
+    })
+
     it('should update parent folder successfully', async () => {
       mockAuthenticatedUser()
 

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

@@ -1,7 +1,10 @@
 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 {
+  assertFolderMutableUnlessUnlocking,
+  ResourceLockedError,
+} from '@sim/platform-authz/resource-lock'
 import { and, eq, isNull } from 'drizzle-orm'
 import { type NextRequest, NextResponse } from 'next/server'
 import { updateFolderContract } from '@/lib/api/contracts'
@@ -81,11 +84,12 @@ export const PUT = withRouteHandler(
 
       // An admin combining `locked: false` with other field changes (e.g. a move) in
       // one request is unlocking the folder as part of this same atomic write -- the
-      // mutable-check must not block that. Skip only when this request isn't also
-      // unlocking.
+      // mutable-check must not treat that request's own current (about-to-be-cleared)
+      // lock as blocking. It must still enforce a lock INHERITED from an ancestor,
+      // since clearing this folder's own `locked` flag doesn't affect that.
       const hasNonLockUpdate = Object.keys(parsed.data.body).some((key) => key !== 'locked')
-      if (hasNonLockUpdate && locked !== false) {
-        await policy.assertMutable(id)
+      if (hasNonLockUpdate) {
+        await assertFolderMutableUnlessUnlocking(id, existingFolder.resourceType, locked === false)
       }
       if (parentId !== undefined) {
         await policy.assertMutable(parentId)

apps/sim/lib/folders/orchestration.ts

@@ -2,7 +2,11 @@ import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
 import { db } from '@sim/db'
 import { folder as folderTable, knowledgeBase, userTableDefinitions } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
-import { assertFolderMutable, ResourceLockedError } from '@sim/platform-authz/resource-lock'
+import {
+  assertFolderMutable,
+  assertFolderMutableUnlessUnlocking,
+  ResourceLockedError,
+} from '@sim/platform-authz/resource-lock'
 import { getPostgresErrorCode, toError } from '@sim/utils/errors'
 import { generateId } from '@sim/utils/id'
 import { and, eq, inArray, isNull } from 'drizzle-orm'
@@ -753,18 +757,47 @@ export async function performUpdateFolder(
   if (params.sortOrder !== undefined) updates.sortOrder = params.sortOrder
   if (params.locked !== undefined) updates.locked = params.locked
 
-  const [updated] = await db
-    .update(folderTable)
-    .set(updates)
-    .where(
-      and(
-        eq(folderTable.id, params.folderId),
-        eq(folderTable.workspaceId, params.workspaceId),
-        eq(folderTable.resourceType, params.resourceType),
-        isNull(folderTable.deletedAt)
+  const isLockOnlyUpdate =
+    params.name === undefined && params.parentId === undefined && params.sortOrder === undefined
+
+  const updated = await db.transaction(async (tx) => {
+    // The route checks lock state before calling this function, but that's a
+    // separate round-trip -- an admin could lock this folder (or its target
+    // parent) in the window between that check and this transaction. Re-check
+    // inside the transaction (joining `tx` so the read is part of the same
+    // atomic unit as the write below) before applying anything.
+    //
+    // `assertFolderMutableUnlessUnlocking` only treats this folder's own
+    // (about-to-be-cleared) lock as satisfied when this same request unlocks it --
+    // a lock inherited from an ancestor still blocks. Skipped entirely for a
+    // lock-only update, matching the file/kb/table lock-toggle precedent.
+    if (!isLockOnlyUpdate) {
+      await assertFolderMutableUnlessUnlocking(
+        params.folderId,
+        params.resourceType,
+        params.locked === false,
+        tx
       )
-    )
-    .returning()
+    }
+    if (params.parentId) {
+      // A folder in an unlocked location could otherwise be moved into a locked one.
+      await assertFolderMutable(params.parentId, params.resourceType, tx)
+    }
+
+    const [row] = await tx
+      .update(folderTable)
+      .set(updates)
+      .where(
+        and(
+          eq(folderTable.id, params.folderId),
+          eq(folderTable.workspaceId, params.workspaceId),
+          eq(folderTable.resourceType, params.resourceType),
+          isNull(folderTable.deletedAt)
+        )
+      )
+      .returning()
+    return row
+  })
 
   if (!updated) return { success: false, error: 'Folder not found', errorCode: 'not_found' }
   return { success: true, folder: updated }

apps/sim/lib/knowledge/service.test.ts

@@ -12,32 +12,52 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 /** Minimal stand-in for `@sim/platform-authz/resource-lock`'s `ResourceLockedError`
  *  (423, carries `resourceType`/`inherited`) — avoids `vi.importActual`. */
-const { mockAssertResourceMutable, mockAssertFolderMutable, MockResourceLockedError } = vi.hoisted(
-  () => {
-    class ResourceLockedErrorStub extends Error {
-      readonly status = 423
-      readonly resourceType: string
-      readonly inherited: boolean
-      constructor(resourceType: string, inherited: boolean, message?: string) {
-        super(message ?? `${resourceType} is locked`)
-        this.name = 'ResourceLockedError'
-        this.resourceType = resourceType
-        this.inherited = inherited
-      }
+const {
+  mockAssertResourceMutable,
+  mockAssertFolderMutable,
+  mockAssertResourceMutableUnlessUnlocking,
+  MockResourceLockedError,
+} = vi.hoisted(() => {
+  class ResourceLockedErrorStub extends Error {
+    readonly status = 423
+    readonly resourceType: string
+    readonly inherited: boolean
+    constructor(resourceType: string, inherited: boolean, message?: string) {
+      super(message ?? `${resourceType} is locked`)
+      this.name = 'ResourceLockedError'
+      this.resourceType = resourceType
+      this.inherited = inherited
     }
-    return {
-      mockAssertResourceMutable: vi.fn(),
-      mockAssertFolderMutable: vi.fn(),
-      MockResourceLockedError: ResourceLockedErrorStub,
+  }
+  const assertResourceMutable = vi.fn()
+  // Real wrapper logic (not a bare passthrough) so tests that configure
+  // assertResourceMutable to reject with a direct vs. inherited error see the
+  // same "unless unlocking" behavior the production wrapper implements.
+  const assertResourceMutableUnlessUnlocking = vi.fn(
+    async (resourceType: string, resourceId: string, unlocking: boolean, tx?: unknown) => {
+      try {
+        const args = [resourceType, resourceId, tx].filter((a) => a !== undefined)
+        await assertResourceMutable(...args)
+      } catch (error) {
+        if (unlocking && error instanceof ResourceLockedErrorStub && !error.inherited) return
+        throw error
+      }
     }
+  )
+  return {
+    mockAssertResourceMutable: assertResourceMutable,
+    mockAssertFolderMutable: vi.fn(),
+    mockAssertResourceMutableUnlessUnlocking: assertResourceMutableUnlessUnlocking,
+    MockResourceLockedError: ResourceLockedErrorStub,
   }
-)
+})
 
 vi.mock('@sim/db', () => dbChainMock)
 vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock)
 vi.mock('@sim/platform-authz/resource-lock', () => ({
   assertResourceMutable: mockAssertResourceMutable,
   assertFolderMutable: mockAssertFolderMutable,
+  assertResourceMutableUnlessUnlocking: mockAssertResourceMutableUnlessUnlocking,
   ResourceLockedError: MockResourceLockedError,
 }))
 
@@ -305,29 +325,50 @@ describe('updateKnowledgeBase — resource-lock enforcement', () => {
     expect(mockAssertResourceMutable).not.toHaveBeenCalled()
   })
 
-  it('skips the lock check when unlocking a locked knowledge base combined with a move in the same request', async () => {
+  it('allows unlocking a directly-locked knowledge base combined with a move in the same request', async () => {
     // Regression test: hasNonLockUpdate is true whenever folderId also changes, so a
     // combined "unlock + move" request previously still ran assertResourceMutable
     // against the KB's current (still-locked) state and was incorrectly rejected,
-    // even though the request unlocks it as part of this same atomic write.
+    // even though the request unlocks it as part of this same atomic write. The
+    // fixed behavior still runs the check (so an inherited lock is caught below),
+    // but treats a DIRECT lock as satisfied since this request clears it.
     dbChainMockFns.limit
       .mockResolvedValueOnce([{ workspaceId: 'ws-current', userId: 'u-1' }]) // currentKb (FOR UPDATE)
       .mockResolvedValueOnce([
         { workspaceId: 'ws-current', resourceType: 'knowledge_base', deletedAt: null },
       ]) // assertFolderParentValid's parent lookup
+    mockAssertResourceMutable.mockRejectedValueOnce(
+      new MockResourceLockedError('knowledge_base', false, 'Knowledge base is locked')
+    )
 
     await updateKnowledgeBase('kb-1', { folderId: 'folder-1', locked: false }, 'req-1').catch(
       () => undefined
     )
 
-    expect(mockAssertResourceMutable).not.toHaveBeenCalled()
+    expect(mockAssertResourceMutable).toHaveBeenCalledWith('knowledge_base', 'kb-1')
     expect(mockAssertFolderMutable).toHaveBeenCalledWith(
       'folder-1',
       'knowledge_base',
       expect.anything()
     )
   })
 
+  it('still rejects unlocking a knowledge base combined with a move when the lock is inherited from its folder', async () => {
+    // Clearing the KB's own `locked` flag doesn't affect a lock inherited from its
+    // containing folder -- that must still block the combined request.
+    mockAssertResourceMutable.mockRejectedValueOnce(
+      new MockResourceLockedError(
+        'knowledge_base',
+        true,
+        'Knowledge base is locked by its containing folder'
+      )
+    )
+
+    await expect(
+      updateKnowledgeBase('kb-1', { folderId: 'folder-1', locked: false }, 'req-1')
+    ).rejects.toMatchObject({ status: 423, inherited: true })
+  })
+
   it('rejects moving the knowledge base into a locked destination folder with a 423', async () => {
     // Regression test: assertResourceMutable only checks the KB's *current* folder
     // chain -- without a separate assertFolderMutable(updates.folderId, ...) check,

apps/sim/lib/knowledge/service.ts

@@ -8,7 +8,11 @@ import {
   workspaceFiles,
 } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
-import { assertFolderMutable, assertResourceMutable } from '@sim/platform-authz/resource-lock'
+import {
+  assertFolderMutable,
+  assertResourceMutable,
+  assertResourceMutableUnlessUnlocking,
+} from '@sim/platform-authz/resource-lock'
 import { getPostgresErrorCode } from '@sim/utils/errors'
 import { generateId } from '@sim/utils/id'
 import { and, count, eq, exists, inArray, isNotNull, isNull, ne, or, sql } from 'drizzle-orm'
@@ -315,10 +319,16 @@ export async function updateKnowledgeBase(
     updates.chunkingConfig !== undefined
   // An admin combining `locked: false` with other field changes in one request is
   // unlocking the knowledge base as part of this same atomic write -- the
-  // mutable-check must not block that. Skip only when this request isn't also
-  // unlocking.
-  if (hasNonLockUpdate && updates.locked !== false) {
-    await assertResourceMutable('knowledge_base', knowledgeBaseId)
+  // mutable-check must not treat that request's own current (about-to-be-cleared)
+  // lock as blocking. It must still enforce a lock inherited from the KB's
+  // containing folder, since clearing the KB's own `locked` flag doesn't affect
+  // that.
+  if (hasNonLockUpdate) {
+    await assertResourceMutableUnlessUnlocking(
+      'knowledge_base',
+      knowledgeBaseId,
+      updates.locked === false
+    )
   }
 
   if (updates.workspaceId !== undefined && !options?.actorUserId) {

apps/sim/lib/table/__tests__/service.test.ts

@@ -93,11 +93,13 @@ describe('table service — resource-lock enforcement', () => {
       expect(resourceLockMockFns.mockAssertResourceMutable).not.toHaveBeenCalled()
     })
 
-    it('skips the lock check when unlocking a locked table combined with a move in the same request', async () => {
+    it('allows unlocking a directly-locked table combined with a move in the same request', async () => {
       // Regression test: isLockOnlyUpdate is false whenever folderId also changes, so a
       // combined "unlock + move" request previously still ran assertResourceMutable
       // against the table's current (still-locked) state and was incorrectly rejected,
-      // even though the request unlocks it as part of this same atomic write.
+      // even though the request unlocks it as part of this same atomic write. The
+      // fixed behavior still runs the check (so an inherited lock is caught below),
+      // but treats a DIRECT lock as satisfied since this request clears it.
       dbChainMockFns.limit
         .mockResolvedValueOnce([{ workspaceId: 'ws-1' }]) // tableRow lookup
         .mockResolvedValueOnce([{ workspaceId: 'ws-1', resourceType: 'table', deletedAt: null }]) // assertFolderParentValid's parent lookup
@@ -110,14 +112,31 @@ describe('table service — resource-lock enforcement', () => {
           locked: false,
         },
       ])
+      resourceLockMockFns.mockAssertResourceMutable.mockRejectedValueOnce(
+        new ResourceLockedError('table', false, 'Table is locked')
+      )
 
       await renameTable('tbl-1', 'new_name', 'req-1', undefined, 'folder-1', false, false)
 
-      expect(resourceLockMockFns.mockAssertResourceMutable).not.toHaveBeenCalled()
+      expect(resourceLockMockFns.mockAssertResourceMutable).toHaveBeenCalledWith('table', 'tbl-1')
       expect(resourceLockMockFns.mockAssertFolderMutable).toHaveBeenCalledWith('folder-1', 'table')
       expect(dbChainMockFns.update).toHaveBeenCalledTimes(1)
     })
 
+    it('still rejects unlocking a table combined with a move when the lock is inherited from its folder', async () => {
+      // Clearing the table's own `locked` flag doesn't affect a lock inherited from
+      // its containing folder -- that must still block the combined request.
+      resourceLockMockFns.mockAssertResourceMutable.mockRejectedValueOnce(
+        new ResourceLockedError('table', true, 'Table is locked by its containing folder')
+      )
+
+      await expect(
+        renameTable('tbl-1', 'new_name', 'req-1', undefined, 'folder-1', false, false)
+      ).rejects.toMatchObject({ status: 423, inherited: true })
+
+      expect(dbChainMockFns.update).not.toHaveBeenCalled()
+    })
+
     it('rejects moving the table into a locked destination folder with a 423', async () => {
       // Regression test: assertResourceMutable only checks the table's *current*
       // folder chain -- without a separate assertFolderMutable(folderId, ...) check,