Skip to content

Commit fd21368

Browse files
committed
fix(api): close import defects found in audit and share one write pipeline
Security: - Escape block names before interpolating them into a RegExp in updateValueReferences. Names reach it straight from imported workflow JSON and normalizeWorkflowBlockName preserves regex metacharacters, so a name like `a*a*a*a*b` compiled to a catastrophically backtracking pattern. A sub-kilobyte body blocked the event loop for 50s and grew exponentially. Also skip rename-to-itself, which is the entire map on the import path, so the scan no longer runs at all there. - Validate folder ownership before folder lock state, so a locked folder in another workspace can no longer be distinguished from a missing one. Correctness: - Gate the imported graph on workflowStateSchema, the same schema the canonical PUT /api/workflows/[id]/state path enforces. Without it a valid 201 could persist a block field of the wrong type, which then threw on every subsequent read and left a workflow nothing could open. - Guard the compensating delete so a failed rollback logs the orphaned id instead of vanishing into a generic 500. - Validate variable `type` against the enum and build the record on a null-prototype object, so a `__proto__` key no longer silently drops the variable. - Bound payload-derived names and descriptions to the same limits the contract declares for the explicit overrides. - Return the description as stored rather than coercing '' to null, matching GET /api/v1/workflows/[id]. Shared code, so the two write paths cannot drift: - Extract prepareWorkflowStateForPersistence and use it from both PUT /api/workflows/[id]/state and the v1 import route: agent-tool sanitization, block backfill, dangling-edge removal, and loop/parallel recomputation now have one implementation. - Persist inline custom tools on import, which the canonical path already did. - Move variable normalization into lib/workflows/variables and repoint the admin importer at it, removing the last duplicate. Docs: - OpenAPI: oneOf -> anyOf on the import body. WorkflowExport matches any object, so every valid object payload matched two branches and failed validation under any spec-driven validator. Document 423 and the loss of workspace-scoped bindings on export. Tests: prepare-state unit tests and a real export -> import round trip with no mocks of the sanitizer or parser, covering loop/parallel children and the regex-metacharacter payload.
1 parent 5decc01 commit fd21368

12 files changed

Lines changed: 645 additions & 158 deletions

File tree

apps/docs/openapi.json

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1085,13 +1085,14 @@
10851085
},
10861086
"workflow": {
10871087
"description": "The workflow to import. Accepts the export envelope returned by GET /api/v1/workflows/{id}/export, a bare workflow state object (`{ blocks, edges, ... }`), or a JSON string of either.",
1088-
"oneOf": [
1088+
"anyOf": [
10891089
{
10901090
"$ref": "#/components/schemas/WorkflowExport"
10911091
},
10921092
{
10931093
"type": "object",
10941094
"additionalProperties": true,
1095+
"minProperties": 1,
10951096
"description": "A bare workflow state object."
10961097
},
10971098
{
@@ -1166,6 +1167,22 @@
11661167
}
11671168
}
11681169
},
1170+
"423": {
1171+
"description": "The target folder is locked and cannot accept new workflows.",
1172+
"content": {
1173+
"application/json": {
1174+
"schema": {
1175+
"type": "object",
1176+
"properties": {
1177+
"error": {
1178+
"type": "string",
1179+
"example": "Folder is locked"
1180+
}
1181+
}
1182+
}
1183+
}
1184+
}
1185+
},
11691186
"429": {
11701187
"$ref": "#/components/responses/RateLimited"
11711188
}
@@ -1245,7 +1262,7 @@
12451262
"get": {
12461263
"operationId": "exportWorkflow",
12471264
"summary": "Export Workflow",
1248-
"description": "Export a workflow as a portable JSON envelope that POST /api/v1/workflows/import accepts verbatim. Credential and password field values stored on blocks are redacted from the payload, while `{{ENV_VAR}}` references and block positions are preserved. Workflow variables are returned as stored \u2014 they are plaintext configuration readable by anyone with workspace read, and secrets belong in environment variables, which travel as unresolved references. Requires read permission on the workflow's workspace. Returns 404 when the workflow does not exist or you do not have access to it.",
1265+
"description": "Export a workflow as a portable JSON envelope that POST /api/v1/workflows/import accepts verbatim. Credential and password field values stored on blocks are redacted from the payload, while `{{ENV_VAR}}` references and block positions are preserved. Workflow variables are returned as stored \u2014 they are plaintext configuration readable by anyone with workspace read, and secrets belong in environment variables, which travel as unresolved references. Requires read permission on the workflow's workspace. Returns 404 when the workflow does not exist or you do not have access to it. Workspace-scoped bindings (knowledge bases, workspace files, channels, projects, folders, MCP servers) are cleared, since those ids do not resolve in another workspace \u2014 an imported copy needs them re-selected.",
12491266
"tags": ["Workflows"],
12501267
"x-codeSamples": [
12511268
{

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

Lines changed: 4 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -26,18 +26,14 @@ import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
2626
import { parseWorkflowJson } from '@/lib/workflows/operations/import-export'
2727
import { saveWorkflowToNormalizedTables } from '@/lib/workflows/persistence/utils'
2828
import { deduplicateWorkflowName } from '@/lib/workflows/utils'
29+
import { normalizeImportedVariables } from '@/lib/workflows/variables/parse'
2930
import { withAdminAuth } from '@/app/api/v1/admin/middleware'
3031
import {
3132
badRequestResponse,
3233
internalErrorResponse,
3334
notFoundResponse,
3435
} from '@/app/api/v1/admin/responses'
35-
import {
36-
extractWorkflowMetadata,
37-
type VariableType,
38-
type WorkflowImportRequest,
39-
type WorkflowVariable,
40-
} from '@/app/api/v1/admin/types'
36+
import { extractWorkflowMetadata, type WorkflowImportRequest } from '@/app/api/v1/admin/types'
4137

4238
const logger = createLogger('AdminWorkflowImportAPI')
4339

@@ -117,42 +113,8 @@ export const POST = withRouteHandler(
117113
return internalErrorResponse(`Failed to save workflow state: ${saveResult.error}`)
118114
}
119115

120-
if (
121-
workflowData.variables &&
122-
typeof workflowData.variables === 'object' &&
123-
!Array.isArray(workflowData.variables)
124-
) {
125-
const variablesRecord: Record<string, WorkflowVariable> = {}
126-
const vars = workflowData.variables as Record<
127-
string,
128-
{ id?: string; name: string; type?: VariableType; value: unknown }
129-
>
130-
Object.entries(vars).forEach(([key, v]) => {
131-
const varId = v.id || key
132-
variablesRecord[varId] = {
133-
id: varId,
134-
name: v.name,
135-
type: v.type ?? 'string',
136-
value: v.value,
137-
}
138-
})
139-
140-
await db
141-
.update(workflow)
142-
.set({ variables: variablesRecord, updatedAt: new Date() })
143-
.where(eq(workflow.id, workflowId))
144-
} else if (workflowData.variables && Array.isArray(workflowData.variables)) {
145-
const variablesRecord: Record<string, WorkflowVariable> = {}
146-
workflowData.variables.forEach((v) => {
147-
const varId = v.id || generateId()
148-
variablesRecord[varId] = {
149-
id: varId,
150-
name: v.name,
151-
type: (v.type as VariableType) ?? 'string',
152-
value: v.value,
153-
}
154-
})
155-
116+
const variablesRecord = normalizeImportedVariables(workflowData.variables)
117+
if (Object.keys(variablesRecord).length > 0) {
156118
await db
157119
.update(workflow)
158120
.set({ variables: variablesRecord, updatedAt: new Date() })

apps/sim/app/api/v1/workflows/[id]/export/route.ts

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,22 @@ function toExportedEdge(edge: Edge): ExportedEdge {
6767
*
6868
* Unlike the admin export (`/api/v1/admin/workflows/[id]/export`), which emits
6969
* the raw state for backup/restore, this surface runs the payload through
70-
* `sanitizeForExport`: block sub-block values declared `password: true` or
71-
* `oauth-input` are nulled, while `{{ENV_VAR}}` references and block positions
72-
* are preserved.
70+
* `sanitizeForExport`, which nulls three classes of sub-block value:
71+
* - `password: true` fields, unless the value is a whole `{{ENV_VAR}}`
72+
* reference, which is preserved so the import resolves it in the target
73+
* workspace;
74+
* - `oauth-input` credentials;
75+
* - **workspace-scoped bindings** — `knowledge-base-selector`, `file-selector`,
76+
* `channel-selector`, `project-selector`, `folder-selector`,
77+
* `mcp-server-selector` and friends, plus fields keyed `knowledgeBaseId`,
78+
* `fileId`, `channelId`, `projectId`, `documentId`, `tagFilters`. These point
79+
* at rows that do not exist in another workspace, so they are cleared rather
80+
* than carried across as dangling ids.
81+
*
82+
* The last class means an export is **not** a byte-for-byte clone even when
83+
* re-imported into the same workspace: those bindings come back empty and must
84+
* be re-selected. This matches the in-app export and is documented on the
85+
* public endpoint so callers do not expect otherwise.
7386
*
7487
* Workflow **variables** are emitted as stored, matching `GET
7588
* /api/v1/workflows/[id]` and the in-app export. Variables are plaintext

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

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ const {
1818
mockSaveWorkflowToNormalizedTables,
1919
mockParseWorkflowJson,
2020
mockAssertFolderMutable,
21+
mockAssertFolderInWorkspace,
22+
mockExtractAndPersistCustomTools,
23+
mockPrepareWorkflowState,
2124
mockDbDelete,
2225
mockDbUpdate,
2326
mockWorkspaceRows,
@@ -28,6 +31,9 @@ const {
2831
mockSaveWorkflowToNormalizedTables: vi.fn(),
2932
mockParseWorkflowJson: vi.fn(),
3033
mockAssertFolderMutable: vi.fn(),
34+
mockAssertFolderInWorkspace: vi.fn(),
35+
mockExtractAndPersistCustomTools: vi.fn(),
36+
mockPrepareWorkflowState: vi.fn(),
3137
mockDbDelete: vi.fn(),
3238
mockDbUpdate: vi.fn(),
3339
mockWorkspaceRows: { value: [{ id: 'ws-1' }] as Array<{ id: string }> },
@@ -59,10 +65,27 @@ vi.mock('@/app/api/v1/logs/meta', () => ({
5965
}))
6066

6167
vi.mock('@sim/platform-authz/workflow', () => ({
68+
assertFolderInWorkspace: mockAssertFolderInWorkspace,
6269
assertFolderMutable: mockAssertFolderMutable,
6370
FolderLockedError: class FolderLockedError extends Error {
6471
status = 423
6572
},
73+
FolderNotFoundError: class FolderNotFoundError extends Error {
74+
status = 400
75+
},
76+
}))
77+
78+
vi.mock('@/lib/workflows/persistence/custom-tools-persistence', () => ({
79+
extractAndPersistCustomTools: mockExtractAndPersistCustomTools,
80+
}))
81+
82+
/**
83+
* Mocked to keep the block registry (and its icon/CSS graph) out of this
84+
* suite. The real normalization is covered by `prepare-state.test.ts` and by
85+
* the end-to-end round trip in `import-export-roundtrip.test.ts`.
86+
*/
87+
vi.mock('@/lib/workflows/persistence/prepare-state', () => ({
88+
prepareWorkflowStateForPersistence: mockPrepareWorkflowState,
6689
}))
6790

6891
vi.mock('@sim/db', () => ({
@@ -84,8 +107,19 @@ import { POST } from '@/app/api/v1/workflows/import/route'
84107
const WORKSPACE_ID = 'ws-1'
85108
const CREATED_AT = new Date('2026-07-01T00:00:00Z')
86109

110+
/** A schema-valid block: the route now gates on `workflowStateSchema`. */
111+
const VALID_BLOCK = {
112+
id: 'block-1',
113+
type: 'starter',
114+
name: 'Start',
115+
position: { x: 0, y: 0 },
116+
subBlocks: {},
117+
outputs: {},
118+
enabled: true,
119+
}
120+
87121
const PARSED_STATE = {
88-
blocks: { 'block-1': { id: 'block-1', type: 'starter', position: { x: 0, y: 0 } } },
122+
blocks: { 'block-1': VALID_BLOCK },
89123
edges: [],
90124
loops: {},
91125
parallels: {},
@@ -120,6 +154,12 @@ describe('POST /api/v1/workflows/import', () => {
120154
mockCheckRateLimit.mockResolvedValue({ allowed: true, userId: 'user-1' })
121155
mockValidateWorkspaceAccess.mockResolvedValue(null)
122156
mockAssertFolderMutable.mockResolvedValue(undefined)
157+
mockAssertFolderInWorkspace.mockResolvedValue(undefined)
158+
mockExtractAndPersistCustomTools.mockResolvedValue({ saved: 0, errors: [] })
159+
mockPrepareWorkflowState.mockImplementation((state: { blocks: unknown; edges: unknown }) => ({
160+
state: { blocks: state.blocks, edges: state.edges, loops: {}, parallels: {} },
161+
warnings: [],
162+
}))
123163
mockParseWorkflowJson.mockReturnValue({ data: { ...PARSED_STATE }, errors: [] })
124164
mockSaveWorkflowToNormalizedTables.mockResolvedValue({ success: true })
125165
mockPerformCreateWorkflow.mockResolvedValue({

0 commit comments

Comments
 (0)