Skip to content

Commit 3aca8f0

Browse files
committed
fix(folders): close the folder-engine audit findings
Data loss - Migration 0274 catches up file folders created AFTER 0272. That backfill is guarded by `WHERE NOT EXISTS (… resource_type = 'file')`, so it fires once; every file folder created between it and this cutover exists only in `workspace_file_folders`. Without the catch-up they vanish from Files on deploy and their contents become unreachable Authorization - Archived folders were mutable, which bypassed the folder lock: the folder routes and `updateFolder` filtered id + resourceType but never `deletedAt`, while `getFolderLockStatus` does — so an archived-but-locked folder reported unlocked. Lock a subfolder, delete its parent, and any write-level member could rename and reparent it. PUT and the engine now require an active row; DELETE deliberately still does not, because it reuses an archived folder's own `deletedAt` to make a partial cascade retryable - `v1/admin/workflows/import` wrote `folderId` with no validation at all. Migration 0272 dropped the FK, so it accepted a knowledge-base folder, another workspace's folder, or garbage — and the workflow then rendered under no sidebar node while still executing, billing, and escaping the folder delete cascade. It now runs the same `assertFolderInWorkspace` + `assertFolderMutable` pair as `v1/workflows/import` Restore no longer loses state - A folder restore ran a bare `archivedAt = null` over its workflows, while the delete had routed through `archiveWorkflow` — which also disables schedules, webhooks, chats, and MCP tools. Restoring a folder reported success while every schedule stayed disabled forever and every chat 404'd. The workflow tree now restores through `restoreWorkflow`, like the knowledge-base and table trees already did. Deployment state is still deliberately not revived, matching the single-workflow restore - Those canonical restores re-root a resource whose folder is archived — correct on its own, catastrophic inside a cascade, which restores children BEFORE the folder rows and would therefore dump every child at the workspace root. `resolveRestoredFolderId` takes the subtree being restored and exempts it. `restoreTable` gained the re-root check it lacked Correctness - `getFolderPath` in `lib/folders/tree.ts` walked `parentId` with no `visited` set, so a cycle in the client cache — reachable through `useReorderFolders`' unchecked optimistic write — hung the tab. Its twin already had one. Dead `buildFolderMap` deleted - The sidebar's "New folder" inside a folder hardcoded the name, so the second one 409'd, was swallowed with only a log line, and the user got no folder and no message. It now names through the existing `generateSubfolderName` and surfaces failures - `chunkedBatchDelete` deleted by id with no re-check, so a resource restored between the SELECT and the DELETE was hard-deleted anyway. Callers now re-assert their soft-delete predicate on the DELETE - Recently Deleted and the `restore_resource` tool cover every folder tree, not just workflow folders, so deleted knowledge-base and table folders are recoverable Cleanup - The folder duplicate route stops steering control flow through `throw new Error('literal')` matched by string, reuses the engine's `nextFolderSortOrder` instead of recomputing it, and names its workflow scope once - `servedFolderResourceTypeSchema` documents that its default applies only to an omitted value; a present-but-unrecognized one is rejected, never coerced to `workflow` - The `workspace_file_folders` comment described the table as unread and unwritten and invited a DROP. It was live until this cutover, and it is now the rollback copy — the comment says so, and names what must be true before anyone drops it - Pinned rows carry a small non-interactive pin glyph again; they sort to the top and had nothing explaining why since the inline pin button was removed Tests - New `lib/folders/lifecycle.test.ts` covers create/update/delete/restore, the re-root, and the 23505 mappings; `cleanup-soft-deletes.test.ts` now pins the folder target's resourceType predicate, which is what keeps the sweep off a not-yet-cut-over type
1 parent 31da1eb commit 3aca8f0

28 files changed

Lines changed: 1179 additions & 117 deletions

File tree

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

Lines changed: 66 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -4,21 +4,46 @@ import { folder as folderTable, workflow } from '@sim/db/schema'
44
import { createLogger } from '@sim/logger'
55
import { FolderLockedError } from '@sim/platform-authz/workflow'
66
import { generateId } from '@sim/utils/id'
7-
import { and, eq, isNull, min } from 'drizzle-orm'
7+
import { and, eq, isNull } from 'drizzle-orm'
88
import { type NextRequest, NextResponse } from 'next/server'
99
import { duplicateFolderContract } from '@/lib/api/contracts'
1010
import { parseRequest } from '@/lib/api/server'
1111
import { getSession } from '@/lib/auth'
1212
import { generateRequestId } from '@/lib/core/utils/request'
1313
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1414
import type { DbOrTx } from '@/lib/db/types'
15+
import { nextFolderSortOrder } from '@/lib/folders/lifecycle'
1516
import { deduplicateFolderName } from '@/lib/folders/naming'
1617
import { toFolderApi } from '@/lib/folders/queries'
1718
import { duplicateWorkflow } from '@/lib/workflows/persistence/duplicate'
1819
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
1920

2021
const logger = createLogger('FolderDuplicateAPI')
2122

23+
/**
24+
* Duplication only ever copies workflow folders. Named once so the scope is stated rather
25+
* than restated as a literal at every query — the engine reads its resourceType from config
26+
* for exactly this reason.
27+
*/
28+
const FOLDER_RESOURCE_TYPE = 'workflow' as const
29+
30+
/**
31+
* Carries the HTTP status with the failure, so the handler maps errors by type instead of by
32+
* comparing `error.message` to a literal — a coupling that breaks silently the moment
33+
* someone rewords a message.
34+
*/
35+
class FolderDuplicationError extends Error {
36+
constructor(
37+
message: string,
38+
readonly status: number,
39+
/** Message returned to the caller when it must differ from the logged one. */
40+
readonly publicMessage: string = message
41+
) {
42+
super(message)
43+
this.name = 'FolderDuplicationError'
44+
}
45+
}
46+
2247
// POST /api/folders/[id]/duplicate - Duplicate a folder with all its child folders and workflows
2348
export const POST = withRouteHandler(
2449
async (req: NextRequest, context: { params: Promise<{ id: string }> }) => {
@@ -46,13 +71,13 @@ export const POST = withRouteHandler(
4671
and(
4772
eq(folderTable.id, sourceFolderId),
4873
isNull(folderTable.deletedAt),
49-
eq(folderTable.resourceType, 'workflow')
74+
eq(folderTable.resourceType, FOLDER_RESOURCE_TYPE)
5075
)
5176
)
5277
.then((rows) => rows[0])
5378

5479
if (!sourceFolder) {
55-
throw new Error('Source folder not found')
80+
throw new FolderDuplicationError('Source folder not found', 404)
5681
}
5782

5883
const userPermission = await getUserEntityPermissions(
@@ -62,12 +87,16 @@ export const POST = withRouteHandler(
6287
)
6388

6489
if (!userPermission || userPermission === 'read') {
65-
throw new Error('Source folder not found or access denied')
90+
throw new FolderDuplicationError(
91+
'Source folder not found or access denied',
92+
403,
93+
'Access denied'
94+
)
6695
}
6796

6897
const targetWorkspaceId = workspaceId || sourceFolder.workspaceId
6998
if (targetWorkspaceId !== sourceFolder.workspaceId) {
70-
throw new Error('Cross-workspace folder duplication is not supported')
99+
throw new FolderDuplicationError('Cross-workspace folder duplication is not supported', 400)
71100
}
72101

73102
const { newFolderId, folderMapping, workflowStats } = await db.transaction(async (tx) => {
@@ -76,49 +105,26 @@ export const POST = withRouteHandler(
76105
const targetParentId = parentId ?? sourceFolder.parentId
77106
await assertTargetParentFolderMutable(tx, targetParentId, targetWorkspaceId, sourceFolderId)
78107

79-
const folderParentCondition = targetParentId
80-
? eq(folderTable.parentId, targetParentId)
81-
: isNull(folderTable.parentId)
82-
const workflowParentCondition = targetParentId
83-
? eq(workflow.folderId, targetParentId)
84-
: isNull(workflow.folderId)
85-
86-
const [[folderResult], [workflowResult]] = await Promise.all([
87-
tx
88-
.select({ minSortOrder: min(folderTable.sortOrder) })
89-
.from(folderTable)
90-
.where(
91-
and(
92-
eq(folderTable.workspaceId, targetWorkspaceId),
93-
eq(folderTable.resourceType, 'workflow'),
94-
folderParentCondition
95-
)
96-
),
108+
// Placement is the engine's rule (folders and workflows share one ordering space),
109+
// so it is read from there rather than recomputed here.
110+
const sortOrder = await nextFolderSortOrder(
111+
FOLDER_RESOURCE_TYPE,
112+
targetWorkspaceId,
113+
targetParentId,
97114
tx
98-
.select({ minSortOrder: min(workflow.sortOrder) })
99-
.from(workflow)
100-
.where(and(eq(workflow.workspaceId, targetWorkspaceId), workflowParentCondition)),
101-
])
102-
103-
const minSortOrder = [folderResult?.minSortOrder, workflowResult?.minSortOrder].reduce<
104-
number | null
105-
>((currentMin, candidate) => {
106-
if (candidate == null) return currentMin
107-
if (currentMin == null) return candidate
108-
return Math.min(currentMin, candidate)
109-
}, null)
110-
const sortOrder = minSortOrder != null ? minSortOrder - 1 : 0
115+
)
116+
111117
const deduplicatedName = await deduplicateFolderName(
112118
tx,
113119
targetWorkspaceId,
114120
targetParentId,
115121
name,
116-
'workflow'
122+
FOLDER_RESOURCE_TYPE
117123
)
118124

119125
await tx.insert(folderTable).values({
120126
id: newFolderId,
121-
resourceType: 'workflow',
127+
resourceType: FOLDER_RESOURCE_TYPE,
122128
userId: session.user.id,
123129
workspaceId: targetWorkspaceId,
124130
name: deduplicatedName,
@@ -183,41 +189,23 @@ export const POST = withRouteHandler(
183189
const duplicatedFolder = await db
184190
.select()
185191
.from(folderTable)
186-
.where(and(eq(folderTable.id, newFolderId), eq(folderTable.resourceType, 'workflow')))
192+
.where(
193+
and(eq(folderTable.id, newFolderId), eq(folderTable.resourceType, FOLDER_RESOURCE_TYPE))
194+
)
187195
.then((rows) => rows[0])
188196

189197
return NextResponse.json({ folder: toFolderApi(duplicatedFolder) }, { status: 201 })
190198
} catch (error) {
191-
if (error instanceof Error) {
192-
if (error instanceof FolderLockedError) {
193-
return NextResponse.json({ error: error.message }, { status: error.status })
194-
}
195-
196-
if (error.message === 'Source folder not found') {
197-
logger.warn(`[${requestId}] Source folder ${sourceFolderId} not found`)
198-
return NextResponse.json({ error: 'Source folder not found' }, { status: 404 })
199-
}
200-
201-
if (error.message === 'Source folder not found or access denied') {
202-
logger.warn(
203-
`[${requestId}] User ${session.user.id} denied access to source folder ${sourceFolderId}`
204-
)
205-
return NextResponse.json({ error: 'Access denied' }, { status: 403 })
206-
}
207-
208-
if (error.message === 'Cross-workspace folder duplication is not supported') {
209-
logger.warn(
210-
`[${requestId}] User ${session.user.id} attempted cross-workspace folder duplication for ${sourceFolderId}`
211-
)
212-
return NextResponse.json({ error: error.message }, { status: 400 })
213-
}
199+
if (error instanceof FolderLockedError) {
200+
return NextResponse.json({ error: error.message }, { status: error.status })
201+
}
214202

215-
if (
216-
error.message === 'Target parent folder not found' ||
217-
error.message === 'Cannot duplicate folder into itself or one of its descendants'
218-
) {
219-
return NextResponse.json({ error: error.message }, { status: 400 })
220-
}
203+
if (error instanceof FolderDuplicationError) {
204+
logger.warn(`[${requestId}] Folder duplication rejected: ${error.message}`, {
205+
sourceFolderId,
206+
userId: session.user.id,
207+
})
208+
return NextResponse.json({ error: error.publicMessage }, { status: error.status })
221209
}
222210

223211
const elapsed = Date.now() - startTime
@@ -250,14 +238,19 @@ async function assertTargetParentFolderMutable(
250238
archivedAt: folderTable.deletedAt,
251239
})
252240
.from(folderTable)
253-
.where(and(eq(folderTable.id, currentFolderId), eq(folderTable.resourceType, 'workflow')))
241+
.where(
242+
and(eq(folderTable.id, currentFolderId), eq(folderTable.resourceType, FOLDER_RESOURCE_TYPE))
243+
)
254244
.limit(1)
255245

256246
if (!folder || folder.workspaceId !== targetWorkspaceId || folder.archivedAt) {
257-
throw new Error('Target parent folder not found')
247+
throw new FolderDuplicationError('Target parent folder not found', 400)
258248
}
259249
if (folder.id === sourceFolderId) {
260-
throw new Error('Cannot duplicate folder into itself or one of its descendants')
250+
throw new FolderDuplicationError(
251+
'Cannot duplicate folder into itself or one of its descendants',
252+
400
253+
)
261254
}
262255
if (folder.locked) {
263256
throw new FolderLockedError()
@@ -284,7 +277,7 @@ async function duplicateFolderStructure(
284277
and(
285278
eq(folderTable.parentId, sourceFolderId),
286279
eq(folderTable.workspaceId, sourceWorkspaceId),
287-
eq(folderTable.resourceType, 'workflow'),
280+
eq(folderTable.resourceType, FOLDER_RESOURCE_TYPE),
288281
isNull(folderTable.deletedAt)
289282
)
290283
)
@@ -295,7 +288,7 @@ async function duplicateFolderStructure(
295288

296289
await tx.insert(folderTable).values({
297290
id: newChildFolderId,
298-
resourceType: 'workflow',
291+
resourceType: FOLDER_RESOURCE_TYPE,
299292
userId,
300293
workspaceId: targetWorkspaceId,
301294
name: childFolder.name,

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

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { db } from '@sim/db'
22
import { folder as folderTable } from '@sim/db/schema'
33
import { createLogger } from '@sim/logger'
44
import { assertFolderMutable, FolderLockedError } from '@sim/platform-authz/workflow'
5-
import { and, eq } from 'drizzle-orm'
5+
import { and, eq, isNull } from 'drizzle-orm'
66
import { type NextRequest, NextResponse } from 'next/server'
77
import { deleteFolderContract, updateFolderContract } from '@/lib/api/contracts'
88
import { parseRequest } from '@/lib/api/server'
@@ -45,10 +45,22 @@ export const PUT = withRouteHandler(
4545
const { resourceType } = parsed.data.query
4646
const { name, locked, parentId, sortOrder } = parsed.data.body
4747

48+
/**
49+
* `isNull(deletedAt)` is load-bearing, not tidiness: `getFolderLockStatus` skips
50+
* archived rows, so an archived-but-locked folder reports unlocked. Without this
51+
* filter, deleting a folder makes every locked subfolder under it freely renameable
52+
* and reparentable by any write-level member.
53+
*/
4854
const existingFolder = await db
4955
.select()
5056
.from(folderTable)
51-
.where(and(eq(folderTable.id, id), eq(folderTable.resourceType, resourceType)))
57+
.where(
58+
and(
59+
eq(folderTable.id, id),
60+
eq(folderTable.resourceType, resourceType),
61+
isNull(folderTable.deletedAt)
62+
)
63+
)
5264
.then((rows) => rows[0])
5365

5466
if (!existingFolder) {
@@ -143,6 +155,12 @@ export const DELETE = withRouteHandler(
143155
const { id } = parsed.data.params
144156
const { resourceType } = parsed.data.query
145157

158+
/**
159+
* Deliberately NOT filtered on `deletedAt`, unlike PUT above: `deleteFolder` reuses an
160+
* already-archived folder's own `deletedAt` so a cascade that failed partway can be
161+
* retried onto the same snapshot. 404ing here would strand those stragglers. Delete is
162+
* also idempotent, so re-reaching an archived folder grants nothing new.
163+
*/
146164
const existingFolder = await db
147165
.select()
148166
.from(folderTable)

apps/sim/app/api/v1/admin/workflows/import/route.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,12 @@
1717
import { db } from '@sim/db'
1818
import { workflow, workspace } from '@sim/db/schema'
1919
import { createLogger } from '@sim/logger'
20+
import {
21+
assertFolderInWorkspace,
22+
assertFolderMutable,
23+
FolderLockedError,
24+
FolderNotFoundError,
25+
} from '@sim/platform-authz/workflow'
2026
import { generateId } from '@sim/utils/id'
2127
import { and, eq, isNull } from 'drizzle-orm'
2228
import { NextResponse } from 'next/server'
@@ -63,6 +69,23 @@ export const POST = withRouteHandler(
6369
return notFoundResponse('Workspace')
6470
}
6571

72+
/**
73+
* Migration 0272 dropped the FK on `workflow.folder_id`, so nothing but this check
74+
* stands between the caller and a folder in another workspace — or one from another
75+
* resource's tree. A workflow filed under an unreachable folder still executes and
76+
* bills, escapes the folder delete cascade, and never counts toward
77+
* `guardLastWorkflows`.
78+
*
79+
* Ownership before lock state, mirroring `v1/workflows/import`: `assertFolderMutable`
80+
* walks the ancestor chain without filtering on workspace, so checking it first would
81+
* let a caller distinguish a locked folder in someone else's workspace (423) from a
82+
* nonexistent one (404).
83+
*/
84+
if (folderId) {
85+
await assertFolderInWorkspace(folderId, workspaceId)
86+
}
87+
await assertFolderMutable(folderId ?? null)
88+
6689
const workflowContent =
6790
typeof body.workflow === 'string' ? body.workflow : JSON.stringify(body.workflow)
6891

@@ -148,6 +171,12 @@ export const POST = withRouteHandler(
148171

149172
return NextResponse.json(response)
150173
} catch (error) {
174+
if (error instanceof FolderNotFoundError) {
175+
return badRequestResponse(error.message)
176+
}
177+
if (error instanceof FolderLockedError) {
178+
return NextResponse.json({ error: error.message }, { status: error.status })
179+
}
151180
logger.error('Admin API: Failed to import workflow', { error })
152181
return internalErrorResponse('Failed to import workflow')
153182
}

apps/sim/app/workspace/[workspaceId]/components/folders/folder-row.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,13 @@ import type { WorkflowFolder } from '@/stores/folders/types'
99
const FOLDER_ICON = <Folder className='size-[14px]' />
1010

1111
export interface FolderRowOptions {
12+
/**
13+
* Whether this folder is pinned. Folders pin under `resourceType: 'folder'`, a different
14+
* pin namespace from the resource they contain — a page listing both resolves two
15+
* `usePinnedIds` sets and passes the right one here. Drives the glyph only; the pin action
16+
* lives on the row's context menu.
17+
*/
18+
pinned?: boolean
1219
/**
1320
* Cells for the page's non-name columns, keyed by `ResourceColumn.id`. Folders have no
1421
* value for most resource-specific columns, so a page passes either a derived roll-up or
@@ -30,7 +37,7 @@ export interface FolderRowOptions {
3037
* field rebuilds one cell instead of every row's cells.
3138
*/
3239
export function folderRow(folder: WorkflowFolder, options: FolderRowOptions = {}): ResourceRow {
33-
const { cells, nameColumnId = 'name' } = options
40+
const { pinned, cells, nameColumnId = 'name' } = options
3441

3542
return {
3643
id: folderRowId(folder.id),
@@ -39,6 +46,7 @@ export function folderRow(folder: WorkflowFolder, options: FolderRowOptions = {}
3946
[nameColumnId]: {
4047
icon: FOLDER_ICON,
4148
label: folder.name,
49+
pinned,
4250
},
4351
},
4452
}

0 commit comments

Comments
 (0)