Skip to content

Commit 66deec3

Browse files
committed
fix(folders): comprehensive final audit — cycle-check gap, dead code, unbounded arrays
Real bugs found and fixed (independently confirmed by two separate audits): - performUpdateFolder's knowledge_base/table branch had no cycle check when reparenting a folder (only self-parent + existence checks) -- unlike workflow/file, which already guard against it. A folder could be dragged into its own descendant, corrupting the tree (detaching a subtree from the root; getFolderLockStatus's visited-Set keeps this from infinite-looping, but the tree becomes unrenderable). Extracted the workflow-only checkForCircularReference into a generic, resourceType-agnostic checkFolderCircularReference in folders/parent-validation.ts (folder ids are globally unique, so no resourceType filter is needed) and wired it into the missing branch. Added dedicated unit tests. - performReorderFolders' existing-folder lookup didn't filter deletedAt, so a soft-deleted folder id could still be reordered/reparented via a stale or direct API request (Greptile P1). - The generic folder-restore route always returned 400, even for "not found" -- threaded a proper errorCode through all three resourceType-specific restore implementations and mapped to 404 (Cursor finding). - Bulk file move/delete and folder reorder request bodies had no array bounds, letting a single request move/delete/reorder an unbounded number of items. Capped at 100 (matches this codebase's existing convention for chunkIds/documentIds) and 1000 respectively. Cleanup (all independently verified against actual usage before removing): - Deleted createFolderRecord/updateFolderRecord/deleteFolderRecord from workflows/utils.ts -- zero real callers since the copilot handler migrated to the generic performCreateFolder/performUpdateFolder/performDeleteFolder, only referenced by now-equally-dead mock scaffolding. - Removed the isWorkflowEffectivelyLocked deprecated alias and migrated its 4 call sites to the canonical isResourceEffectivelyLocked name. - Removed the WorkflowFolder deprecated type alias and migrated its 6 call sites to the canonical Folder type.
1 parent cce0d83 commit 66deec3

21 files changed

Lines changed: 252 additions & 195 deletions

File tree

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

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,10 @@ import {
1111
permissionsMock,
1212
permissionsMockFns,
1313
workflowsOrchestrationMockFns,
14-
workflowsUtilsMock,
15-
workflowsUtilsMockFns,
1614
} from '@sim/testing'
1715
import { beforeEach, describe, expect, it, vi } from 'vitest'
1816

19-
const { mockLogger, mockDbRef } = vi.hoisted(() => {
17+
const { mockLogger, mockDbRef, mockCheckFolderCircularReference } = vi.hoisted(() => {
2018
const logger = {
2119
info: vi.fn(),
2220
warn: vi.fn(),
@@ -29,6 +27,10 @@ const { mockLogger, mockDbRef } = vi.hoisted(() => {
2927
return {
3028
mockLogger: logger,
3129
mockDbRef: { current: null as any },
30+
// Stands in for the real cycle-check the mocked performUpdateFolder below
31+
// hand-simulates -- not tied to a real module export, just an internal
32+
// test-double control lever for exercising the route's error handling.
33+
mockCheckFolderCircularReference: vi.fn(),
3234
}
3335
})
3436

@@ -53,7 +55,6 @@ vi.mock('@/lib/folders/orchestration', () => ({
5355
performDeleteFolder: workflowsOrchestrationMockFns.mockPerformDeleteFolder,
5456
performUpdateFolder: workflowsOrchestrationMockFns.mockPerformUpdateFolder,
5557
}))
56-
vi.mock('@/lib/workflows/utils', () => workflowsUtilsMock)
5758

5859
import { DELETE, PUT } from '@/app/api/folders/[id]/route'
5960

@@ -163,10 +164,7 @@ describe('Individual Folder API Route', () => {
163164
}
164165
if (
165166
params.parentId &&
166-
(await workflowsUtilsMockFns.mockCheckForCircularReference(
167-
params.folderId,
168-
params.parentId
169-
))
167+
(await mockCheckFolderCircularReference(params.folderId, params.parentId))
170168
) {
171169
return {
172170
success: false,
@@ -186,7 +184,7 @@ describe('Individual Folder API Route', () => {
186184
},
187185
}
188186
})
189-
workflowsUtilsMockFns.mockCheckForCircularReference.mockResolvedValue(false)
187+
mockCheckFolderCircularReference.mockResolvedValue(false)
190188
})
191189

192190
describe('PUT /api/folders/[id]', () => {
@@ -392,7 +390,7 @@ describe('Individual Folder API Route', () => {
392390
},
393391
})
394392

395-
workflowsUtilsMockFns.mockCheckForCircularReference.mockResolvedValue(true)
393+
mockCheckFolderCircularReference.mockResolvedValue(true)
396394

397395
const req = createMockRequest('PUT', {
398396
name: 'Updated Folder 3',
@@ -406,10 +404,7 @@ describe('Individual Folder API Route', () => {
406404

407405
const data = await response.json()
408406
expect(data).toHaveProperty('error', 'Cannot create circular folder reference')
409-
expect(workflowsUtilsMockFns.mockCheckForCircularReference).toHaveBeenCalledWith(
410-
'folder-3',
411-
'folder-1'
412-
)
407+
expect(mockCheckFolderCircularReference).toHaveBeenCalledWith('folder-3', 'folder-1')
413408
})
414409
})
415410

apps/sim/app/workspace/[workspaceId]/settings/components/recently-deleted/recently-deleted.tsx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import { useRestoreTable, useTablesList } from '@/hooks/queries/tables'
2828
import { useRestoreWorkflow, useWorkflows } from '@/hooks/queries/workflows'
2929
import { useRestoreWorkspaceFile, useWorkspaceFiles } from '@/hooks/queries/workspace-files'
3030
import { useFolderStore } from '@/stores/folders/store'
31-
import type { WorkflowFolder } from '@/stores/folders/types'
31+
import type { Folder as FolderType } from '@/stores/folders/types'
3232

3333
type ResourceType =
3434
| 'all'
@@ -385,10 +385,10 @@ export function RecentlyDeleted() {
385385
function handleView(resource: DeletedResource) {
386386
if (resource.type === 'folder') {
387387
const setExpanded = useFolderStore.getState().setExpanded
388-
const byId = new Map<string, WorkflowFolder>()
388+
const byId = new Map<string, FolderType>()
389389
for (const folder of foldersQuery.data ?? []) byId.set(folder.id, folder)
390390
for (const folder of activeFoldersQuery.data ?? []) byId.set(folder.id, folder)
391-
let current: WorkflowFolder | undefined = byId.get(resource.id)
391+
let current: FolderType | undefined = byId.get(resource.id)
392392
const seen = new Set<string>()
393393
while (current && !seen.has(current.id)) {
394394
seen.add(current.id)

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/editor.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ import { PreviewWorkflow } from '@/app/workspace/[workspaceId]/w/components/prev
5252
import { getTileIconColorClass } from '@/blocks/icon-color'
5353
import { getBlock } from '@/blocks/registry'
5454
import { useFolderMap } from '@/hooks/queries/folders'
55-
import { isWorkflowEffectivelyLocked } from '@/hooks/queries/utils/folder-tree'
55+
import { isResourceEffectivelyLocked } from '@/hooks/queries/utils/folder-tree'
5656
import { useWorkflowMap, useWorkflowState } from '@/hooks/queries/workflows'
5757
import { useCollaborativeWorkflow } from '@/hooks/use-collaborative-workflow'
5858
import { usePanelEditorSearchStore, usePanelEditorStore } from '@/stores/panel'
@@ -128,7 +128,7 @@ export function Editor() {
128128
const { data: workflows = {} } = useWorkflowMap(workspaceId)
129129
const { data: folders = {} } = useFolderMap(workspaceId)
130130
const workflowMetadata = workflowId ? workflows[workflowId] : undefined
131-
const workflowLocked = isWorkflowEffectivelyLocked(workflowMetadata, folders)
131+
const workflowLocked = isResourceEffectivelyLocked(workflowMetadata, folders)
132132

133133
// Check if block is locked (or inside a locked ancestor) and compute edit permission
134134
// Locked blocks cannot be edited by anyone (admins can only lock/unlock)

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/panel.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ import {
7474
useCopilotChats,
7575
} from '@/hooks/queries/copilot-chats'
7676
import { useFolderMap } from '@/hooks/queries/folders'
77-
import { isWorkflowEffectivelyLocked } from '@/hooks/queries/utils/folder-tree'
77+
import { isResourceEffectivelyLocked } from '@/hooks/queries/utils/folder-tree'
7878
import { useDuplicateWorkflowMutation, useWorkflowMap } from '@/hooks/queries/workflows'
7979
import { useCollaborativeWorkflow } from '@/hooks/use-collaborative-workflow'
8080
import { usePermissionConfig } from '@/hooks/use-permission-config'
@@ -232,7 +232,7 @@ export const Panel = memo(function Panel() {
232232
)
233233

234234
const currentWorkflow = activeWorkflowId ? workflows[activeWorkflowId] : null
235-
const workflowLocked = isWorkflowEffectivelyLocked(currentWorkflow, folders)
235+
const workflowLocked = isResourceEffectivelyLocked(currentWorkflow, folders)
236236
const canMutateWorkflow = userPermissions.canEdit && !workflowLocked
237237
const { isSnapshotView } = useCurrentWorkflow()
238238

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/search-replace/workflow-search-replace.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ import { useMcpTools } from '@/hooks/mcp/use-mcp-tools'
4040
import { useWorkspaceCredentials } from '@/hooks/queries/credentials'
4141
import { useCustomTools } from '@/hooks/queries/custom-tools'
4242
import { useFolderMap } from '@/hooks/queries/folders'
43-
import { isWorkflowEffectivelyLocked } from '@/hooks/queries/utils/folder-tree'
43+
import { isResourceEffectivelyLocked } from '@/hooks/queries/utils/folder-tree'
4444
import { useWorkflowMap } from '@/hooks/queries/workflows'
4545
import { useCollaborativeWorkflow } from '@/hooks/use-collaborative-workflow'
4646
import { usePanelEditorSearchStore, usePanelEditorStore } from '@/stores/panel'
@@ -122,7 +122,7 @@ export function WorkflowSearchReplace() {
122122
const { data: workflows = {} } = useWorkflowMap(workspaceId)
123123
const { data: folders = {} } = useFolderMap(workspaceId)
124124
const workflowMetadata = workflowId ? workflows[workflowId] : undefined
125-
const workflowLocked = isWorkflowEffectivelyLocked(workflowMetadata, folders)
125+
const workflowLocked = isResourceEffectivelyLocked(workflowMetadata, folders)
126126
const searchReadOnly = currentWorkflow.isSnapshotView || workflowLocked
127127
const readonlyReason = currentWorkflow.isSnapshotView
128128
? 'Snapshot view is readonly'

apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workflow-list/components/workflow-item/workflow-item.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ import { useFolderMap } from '@/hooks/queries/folders'
3535
import { getFolderMap } from '@/hooks/queries/utils/folder-cache'
3636
import {
3737
isFolderOrAncestorLocked,
38-
isWorkflowEffectivelyLocked,
38+
isResourceEffectivelyLocked,
3939
} from '@/hooks/queries/utils/folder-tree'
4040
import { getWorkflows } from '@/hooks/queries/utils/workflow-cache'
4141
import { useUpdateWorkflow } from '@/hooks/queries/workflows'
@@ -79,7 +79,7 @@ export const WorkflowItem = memo(function WorkflowItem({
7979

8080
const { data: foldersById = {} } = useFolderMap(workspaceId)
8181
const inheritedFolderLocked = isFolderOrAncestorLocked(workflow.folderId, foldersById)
82-
const effectiveLocked = isWorkflowEffectivelyLocked(workflow, foldersById)
82+
const effectiveLocked = isResourceEffectivelyLocked(workflow, foldersById)
8383

8484
const { canDeleteWorkflows, canDeleteFolder } = useCanDelete({ workspaceId })
8585

apps/sim/app/workspace/[workspaceId]/w/hooks/use-export-folder.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import {
1212
import { useFolderMap } from '@/hooks/queries/folders'
1313
import { useWorkflowMap } from '@/hooks/queries/workflows'
1414
import { useFolderStore } from '@/stores/folders/store'
15-
import type { WorkflowFolder } from '@/stores/folders/types'
15+
import type { Folder } from '@/stores/folders/types'
1616
import type { WorkflowMetadata } from '@/stores/workflows/registry/types'
1717

1818
const logger = createLogger('useExportFolder')
@@ -43,7 +43,7 @@ interface CollectedWorkflow {
4343
function collectWorkflowsInFolder(
4444
folderId: string,
4545
workflows: Record<string, WorkflowMetadata>,
46-
folders: Record<string, WorkflowFolder>
46+
folders: Record<string, Folder>
4747
): CollectedWorkflow[] {
4848
const collectedWorkflows: CollectedWorkflow[] = []
4949

@@ -69,7 +69,7 @@ function collectWorkflowsInFolder(
6969
*/
7070
function collectSubfolders(
7171
rootFolderId: string,
72-
folders: Record<string, WorkflowFolder>
72+
folders: Record<string, Folder>
7373
): FolderExportData[] {
7474
const subfolders: FolderExportData[] = []
7575

apps/sim/app/workspace/[workspaceId]/w/hooks/use-export-selection.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
import { getFolderMap } from '@/hooks/queries/utils/folder-cache'
1111
import { getWorkflows } from '@/hooks/queries/utils/workflow-cache'
1212
import { useFolderStore } from '@/stores/folders/store'
13-
import type { WorkflowFolder } from '@/stores/folders/types'
13+
import type { Folder } from '@/stores/folders/types'
1414
import type { WorkflowMetadata } from '@/stores/workflows/registry/types'
1515

1616
const logger = createLogger('useExportSelection')
@@ -37,7 +37,7 @@ interface CollectedWorkflow {
3737
function collectWorkflowsInFolder(
3838
folderId: string,
3939
workflows: Record<string, WorkflowMetadata>,
40-
folders: Record<string, WorkflowFolder>
40+
folders: Record<string, Folder>
4141
): CollectedWorkflow[] {
4242
const collectedWorkflows: CollectedWorkflow[] = []
4343

@@ -62,7 +62,7 @@ function collectWorkflowsInFolder(
6262
*/
6363
function collectSubfoldersForMultipleFolders(
6464
rootFolderIds: string[],
65-
folders: Record<string, WorkflowFolder>
65+
folders: Record<string, Folder>
6666
): FolderExportData[] {
6767
const subfolders: FolderExportData[] = []
6868

apps/sim/hooks/queries/utils/folder-tree.test.ts

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,11 @@ import {
77
getFolderPath,
88
isFolderEffectivelyLocked,
99
isFolderOrAncestorLocked,
10-
isWorkflowEffectivelyLocked,
10+
isResourceEffectivelyLocked,
1111
} from '@/hooks/queries/utils/folder-tree'
12-
import type { WorkflowFolder } from '@/stores/folders/types'
12+
import type { Folder } from '@/stores/folders/types'
1313

14-
function makeFolder(overrides: Partial<WorkflowFolder> & { id: string }): WorkflowFolder {
14+
function makeFolder(overrides: Partial<Folder> & { id: string }): Folder {
1515
return {
1616
id: overrides.id,
1717
name: overrides.name ?? overrides.id,
@@ -146,31 +146,31 @@ describe('findLockedAncestorFolder', () => {
146146
})
147147
})
148148

149-
describe('isWorkflowEffectivelyLocked', () => {
149+
describe('isResourceEffectivelyLocked', () => {
150150
it('treats undefined or null workflows as unlocked', () => {
151-
expect(isWorkflowEffectivelyLocked(undefined, {})).toBe(false)
152-
expect(isWorkflowEffectivelyLocked(null, {})).toBe(false)
151+
expect(isResourceEffectivelyLocked(undefined, {})).toBe(false)
152+
expect(isResourceEffectivelyLocked(null, {})).toBe(false)
153153
})
154154

155155
it('returns true when the workflow row itself is locked', () => {
156-
expect(isWorkflowEffectivelyLocked({ locked: true, folderId: null }, {})).toBe(true)
156+
expect(isResourceEffectivelyLocked({ locked: true, folderId: null }, {})).toBe(true)
157157
})
158158

159159
it('returns true when an ancestor folder is locked', () => {
160160
const folders = {
161161
eng: makeFolder({ id: 'eng', locked: true }),
162162
be: makeFolder({ id: 'be', parentId: 'eng' }),
163163
}
164-
expect(isWorkflowEffectivelyLocked({ locked: false, folderId: 'be' }, folders)).toBe(true)
164+
expect(isResourceEffectivelyLocked({ locked: false, folderId: 'be' }, folders)).toBe(true)
165165
})
166166

167167
it('returns false when neither row nor any ancestor is locked', () => {
168168
const folders = { eng: makeFolder({ id: 'eng' }) }
169-
expect(isWorkflowEffectivelyLocked({ locked: false, folderId: 'eng' }, folders)).toBe(false)
169+
expect(isResourceEffectivelyLocked({ locked: false, folderId: 'eng' }, folders)).toBe(false)
170170
})
171171

172172
it('returns false for a workflow at workspace root with no row lock', () => {
173-
expect(isWorkflowEffectivelyLocked({ locked: false, folderId: null }, {})).toBe(false)
173+
expect(isResourceEffectivelyLocked({ locked: false, folderId: null }, {})).toBe(false)
174174
})
175175
})
176176

apps/sim/hooks/queries/utils/folder-tree.ts

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -98,12 +98,6 @@ export function isResourceEffectivelyLocked(
9898
return isFolderOrAncestorLocked(resource.folderId, folders)
9999
}
100100

101-
/**
102-
* @deprecated Use {@link isResourceEffectivelyLocked} — kept as a named alias
103-
* so existing workflow call sites read naturally.
104-
*/
105-
export const isWorkflowEffectivelyLocked = isResourceEffectivelyLocked
106-
107101
/**
108102
* Effective lock state for a folder as visible to the client. Mirrors the
109103
* server's `getFolderLockStatus(folderId)` (in `@sim/platform-authz/workflow`) but

0 commit comments

Comments
 (0)