Skip to content

Commit 9c3feb9

Browse files
committed
fix(condition): used isolated vms for condition block RCE
1 parent 3120a78 commit 9c3feb9

File tree

3 files changed

+109
-36
lines changed

3 files changed

+109
-36
lines changed

apps/sim/executor/handlers/condition/condition-handler.test.ts

Lines changed: 49 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,51 @@
1-
import '@/executor/__test-utils__/mock-dependencies'
2-
31
import { beforeEach, describe, expect, it, vi } from 'vitest'
42
import { BlockType } from '@/executor/constants'
53
import { ConditionBlockHandler } from '@/executor/handlers/condition/condition-handler'
64
import type { BlockState, ExecutionContext } from '@/executor/types'
75
import type { SerializedBlock, SerializedWorkflow } from '@/serializer/types'
86

7+
vi.mock('@/lib/logs/console/logger', () => ({
8+
createLogger: vi.fn(() => ({
9+
info: vi.fn(),
10+
error: vi.fn(),
11+
warn: vi.fn(),
12+
debug: vi.fn(),
13+
})),
14+
}))
15+
16+
vi.mock('@/lib/core/utils/request', () => ({
17+
generateRequestId: vi.fn(() => 'test-request-id'),
18+
}))
19+
20+
vi.mock('@/lib/execution/isolated-vm', () => ({
21+
executeInIsolatedVM: vi.fn(),
22+
}))
23+
24+
import { executeInIsolatedVM } from '@/lib/execution/isolated-vm'
25+
26+
const mockExecuteInIsolatedVM = executeInIsolatedVM as ReturnType<typeof vi.fn>
27+
28+
/**
29+
* Helper to simulate isolated VM execution behavior
30+
* This evaluates the code using the contextVariables, mimicking real isolated VM behavior
31+
*/
32+
function simulateIsolatedVMExecution(
33+
code: string,
34+
contextVariables: Record<string, unknown>
35+
): { result: unknown; stdout: string; error?: { message: string; name: string } } {
36+
try {
37+
const fn = new Function(...Object.keys(contextVariables), code)
38+
const result = fn(...Object.values(contextVariables))
39+
return { result, stdout: '' }
40+
} catch (error: any) {
41+
return {
42+
result: null,
43+
stdout: '',
44+
error: { message: error.message, name: error.name || 'Error' },
45+
}
46+
}
47+
}
48+
949
describe('ConditionBlockHandler', () => {
1050
let handler: ConditionBlockHandler
1151
let mockBlock: SerializedBlock
@@ -18,7 +58,6 @@ describe('ConditionBlockHandler', () => {
1858
let mockPathTracker: any
1959

2060
beforeEach(() => {
21-
// Define blocks first
2261
mockSourceBlock = {
2362
id: 'source-block-1',
2463
metadata: { id: 'source', name: 'Source Block' },
@@ -33,7 +72,7 @@ describe('ConditionBlockHandler', () => {
3372
metadata: { id: BlockType.CONDITION, name: 'Test Condition' },
3473
position: { x: 50, y: 50 },
3574
config: { tool: BlockType.CONDITION, params: {} },
36-
inputs: { conditions: 'json' }, // Corrected based on previous step
75+
inputs: { conditions: 'json' },
3776
outputs: {},
3877
enabled: true,
3978
}
@@ -56,7 +95,6 @@ describe('ConditionBlockHandler', () => {
5695
enabled: true,
5796
}
5897

59-
// Then define workflow using the block objects
6098
mockWorkflow = {
6199
blocks: [mockSourceBlock, mockBlock, mockTargetBlock1, mockTargetBlock2],
62100
connections: [
@@ -84,7 +122,6 @@ describe('ConditionBlockHandler', () => {
84122

85123
handler = new ConditionBlockHandler(mockPathTracker, mockResolver)
86124

87-
// Define mock context *after* workflow and blocks are set up
88125
mockContext = {
89126
workflowId: 'test-workflow-id',
90127
blockStates: new Map<string, BlockState>([
@@ -99,7 +136,7 @@ describe('ConditionBlockHandler', () => {
99136
]),
100137
blockLogs: [],
101138
metadata: { duration: 0 },
102-
environmentVariables: {}, // Now set the context's env vars
139+
environmentVariables: {},
103140
decisions: { router: new Map(), condition: new Map() },
104141
loopExecutions: new Map(),
105142
executedBlocks: new Set([mockSourceBlock.id]),
@@ -108,11 +145,11 @@ describe('ConditionBlockHandler', () => {
108145
completedLoops: new Set(),
109146
}
110147

111-
// Reset mocks using vi
112148
vi.clearAllMocks()
113149

114-
// Default mock implementations - Removed as it's in the shared mock now
115-
// mockResolver.resolveBlockReferences.mockImplementation((value) => value)
150+
mockExecuteInIsolatedVM.mockImplementation(async ({ code, contextVariables }) => {
151+
return simulateIsolatedVMExecution(code, contextVariables)
152+
})
116153
})
117154

118155
it('should handle condition blocks', () => {
@@ -141,7 +178,6 @@ describe('ConditionBlockHandler', () => {
141178
selectedOption: 'cond1',
142179
}
143180

144-
// Mock the full resolution pipeline
145181
mockResolver.resolveVariableReferences.mockReturnValue('context.value > 5')
146182
mockResolver.resolveBlockReferences.mockReturnValue('context.value > 5')
147183
mockResolver.resolveEnvVariables.mockReturnValue('context.value > 5')
@@ -182,7 +218,6 @@ describe('ConditionBlockHandler', () => {
182218
selectedOption: 'else1',
183219
}
184220

185-
// Mock the full resolution pipeline
186221
mockResolver.resolveVariableReferences.mockReturnValue('context.value < 0')
187222
mockResolver.resolveBlockReferences.mockReturnValue('context.value < 0')
188223
mockResolver.resolveEnvVariables.mockReturnValue('context.value < 0')
@@ -207,7 +242,7 @@ describe('ConditionBlockHandler', () => {
207242
const inputs = { conditions: '{ "invalid json ' }
208243

209244
await expect(handler.execute(mockContext, mockBlock, inputs)).rejects.toThrow(
210-
/^Invalid conditions format: Unterminated string.*/
245+
/^Invalid conditions format:/
211246
)
212247
})
213248

@@ -218,7 +253,6 @@ describe('ConditionBlockHandler', () => {
218253
]
219254
const inputs = { conditions: JSON.stringify(conditions) }
220255

221-
// Mock the full resolution pipeline
222256
mockResolver.resolveVariableReferences.mockReturnValue('{{source-block-1.value}} > 5')
223257
mockResolver.resolveBlockReferences.mockReturnValue('10 > 5')
224258
mockResolver.resolveEnvVariables.mockReturnValue('10 > 5')
@@ -245,7 +279,6 @@ describe('ConditionBlockHandler', () => {
245279
]
246280
const inputs = { conditions: JSON.stringify(conditions) }
247281

248-
// Mock the full resolution pipeline for variable resolution
249282
mockResolver.resolveVariableReferences.mockReturnValue('"john" !== null')
250283
mockResolver.resolveBlockReferences.mockReturnValue('"john" !== null')
251284
mockResolver.resolveEnvVariables.mockReturnValue('"john" !== null')
@@ -272,7 +305,6 @@ describe('ConditionBlockHandler', () => {
272305
]
273306
const inputs = { conditions: JSON.stringify(conditions) }
274307

275-
// Mock the full resolution pipeline for env variable resolution
276308
mockResolver.resolveVariableReferences.mockReturnValue('{{POOP}} === "hi"')
277309
mockResolver.resolveBlockReferences.mockReturnValue('{{POOP}} === "hi"')
278310
mockResolver.resolveEnvVariables.mockReturnValue('"hi" === "hi"')
@@ -300,7 +332,6 @@ describe('ConditionBlockHandler', () => {
300332
const inputs = { conditions: JSON.stringify(conditions) }
301333

302334
const resolutionError = new Error('Could not resolve reference: invalid-ref')
303-
// Mock the pipeline to throw at the variable resolution stage
304335
mockResolver.resolveVariableReferences.mockImplementation(() => {
305336
throw resolutionError
306337
})
@@ -317,23 +348,21 @@ describe('ConditionBlockHandler', () => {
317348
]
318349
const inputs = { conditions: JSON.stringify(conditions) }
319350

320-
// Mock the full resolution pipeline
321351
mockResolver.resolveVariableReferences.mockReturnValue(
322352
'context.nonExistentProperty.doSomething()'
323353
)
324354
mockResolver.resolveBlockReferences.mockReturnValue('context.nonExistentProperty.doSomething()')
325355
mockResolver.resolveEnvVariables.mockReturnValue('context.nonExistentProperty.doSomething()')
326356

327357
await expect(handler.execute(mockContext, mockBlock, inputs)).rejects.toThrow(
328-
/^Evaluation error in condition "if": Evaluation error in condition: Cannot read properties of undefined \(reading 'doSomething'\)\. \(Resolved: context\.nonExistentProperty\.doSomething\(\)\)$/
358+
/Evaluation error in condition "if".*doSomething/
329359
)
330360
})
331361

332362
it('should handle missing source block output gracefully', async () => {
333363
const conditions = [{ id: 'cond1', title: 'if', value: 'true' }]
334364
const inputs = { conditions: JSON.stringify(conditions) }
335365

336-
// Create a new context with empty blockStates instead of trying to delete from readonly map
337366
const contextWithoutSource = {
338367
...mockContext,
339368
blockStates: new Map<string, BlockState>(),
@@ -355,7 +384,6 @@ describe('ConditionBlockHandler', () => {
355384

356385
mockContext.workflow!.blocks = [mockSourceBlock, mockBlock, mockTargetBlock2]
357386

358-
// Mock the full resolution pipeline
359387
mockResolver.resolveVariableReferences.mockReturnValue('true')
360388
mockResolver.resolveBlockReferences.mockReturnValue('true')
361389
mockResolver.resolveEnvVariables.mockReturnValue('true')
@@ -381,7 +409,6 @@ describe('ConditionBlockHandler', () => {
381409
},
382410
]
383411

384-
// Mock the full resolution pipeline
385412
mockResolver.resolveVariableReferences
386413
.mockReturnValueOnce('false')
387414
.mockReturnValueOnce('context.value === 99')
@@ -394,12 +421,10 @@ describe('ConditionBlockHandler', () => {
394421

395422
const result = await handler.execute(mockContext, mockBlock, inputs)
396423

397-
// Should return success with no path selected (branch ends gracefully)
398424
expect((result as any).conditionResult).toBe(false)
399425
expect((result as any).selectedPath).toBeNull()
400426
expect((result as any).selectedConditionId).toBeNull()
401427
expect((result as any).selectedOption).toBeNull()
402-
// Decision should not be set when no condition matches
403428
expect(mockContext.decisions.condition.has(mockBlock.id)).toBe(false)
404429
})
405430

@@ -410,7 +435,6 @@ describe('ConditionBlockHandler', () => {
410435
]
411436
const inputs = { conditions: JSON.stringify(conditions) }
412437

413-
// Mock the full resolution pipeline
414438
mockResolver.resolveVariableReferences.mockReturnValue('context.item === "apple"')
415439
mockResolver.resolveBlockReferences.mockReturnValue('context.item === "apple"')
416440
mockResolver.resolveEnvVariables.mockReturnValue('context.item === "apple"')

apps/sim/executor/handlers/condition/condition-handler.ts

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { generateRequestId } from '@/lib/core/utils/request'
2+
import { executeInIsolatedVM } from '@/lib/execution/isolated-vm'
13
import { createLogger } from '@/lib/logs/console/logger'
24
import type { BlockOutput } from '@/blocks/types'
35
import { BlockType, CONDITION, DEFAULTS, EDGE } from '@/executor/constants'
@@ -6,6 +8,8 @@ import type { SerializedBlock } from '@/serializer/types'
68

79
const logger = createLogger('ConditionBlockHandler')
810

11+
const CONDITION_TIMEOUT_MS = 5000
12+
913
/**
1014
* Evaluates a single condition expression with variable/block reference resolution
1115
* Returns true if condition is met, false otherwise
@@ -35,11 +39,32 @@ export async function evaluateConditionExpression(
3539
}
3640

3741
try {
38-
const conditionMet = new Function(
39-
'context',
40-
`with(context) { return ${resolvedConditionValue} }`
41-
)(evalContext)
42-
return Boolean(conditionMet)
42+
const requestId = generateRequestId()
43+
44+
const code = `return Boolean(${resolvedConditionValue})`
45+
46+
const result = await executeInIsolatedVM({
47+
code,
48+
params: {},
49+
envVars: {},
50+
contextVariables: { ...evalContext, context: evalContext },
51+
timeoutMs: CONDITION_TIMEOUT_MS,
52+
requestId,
53+
})
54+
55+
if (result.error) {
56+
logger.error(`Failed to evaluate condition: ${result.error.message}`, {
57+
originalCondition: conditionExpression,
58+
resolvedCondition: resolvedConditionValue,
59+
evalContext,
60+
error: result.error,
61+
})
62+
throw new Error(
63+
`Evaluation error in condition: ${result.error.message}. (Resolved: ${resolvedConditionValue})`
64+
)
65+
}
66+
67+
return Boolean(result.result)
4368
} catch (evalError: any) {
4469
logger.error(`Failed to evaluate condition: ${evalError.message}`, {
4570
originalCondition: conditionExpression,
@@ -87,7 +112,6 @@ export class ConditionBlockHandler implements BlockHandler {
87112
block
88113
)
89114

90-
// Handle case where no condition matched and no else exists - branch ends gracefully
91115
if (!selectedConnection || !selectedCondition) {
92116
return {
93117
...((sourceOutput as any) || {}),
@@ -206,14 +230,12 @@ export class ConditionBlockHandler implements BlockHandler {
206230
if (elseConnection) {
207231
return { selectedConnection: elseConnection, selectedCondition: elseCondition }
208232
}
209-
// Else exists but has no connection - treat as no match, branch ends
210233
logger.info(`No condition matched and else has no connection - branch ending`, {
211234
blockId: block.id,
212235
})
213236
return { selectedConnection: null, selectedCondition: null }
214237
}
215238

216-
// No condition matched and no else exists - branch ends gracefully
217239
logger.info(`No condition matched and no else block - branch ending`, { blockId: block.id })
218240
return { selectedConnection: null, selectedCondition: null }
219241
}

apps/sim/lib/execution/isolated-vm.ts

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -204,12 +204,17 @@ async function ensureWorker(): Promise<void> {
204204

205205
import('node:child_process').then(({ spawn }) => {
206206
worker = spawn('node', [workerPath], {
207-
stdio: ['ignore', 'pipe', 'inherit', 'ipc'],
207+
stdio: ['ignore', 'pipe', 'pipe', 'ipc'],
208208
serialization: 'json',
209209
})
210210

211211
worker.on('message', handleWorkerMessage)
212212

213+
let stderrData = ''
214+
worker.stderr?.on('data', (data: Buffer) => {
215+
stderrData += data.toString()
216+
})
217+
213218
const startTimeout = setTimeout(() => {
214219
worker?.kill()
215220
worker = null
@@ -232,20 +237,42 @@ async function ensureWorker(): Promise<void> {
232237
}
233238
worker.on('message', readyHandler)
234239

235-
worker.on('exit', () => {
240+
worker.on('exit', (code) => {
236241
if (workerIdleTimeout) {
237242
clearTimeout(workerIdleTimeout)
238243
workerIdleTimeout = null
239244
}
245+
246+
const wasStartupFailure = !workerReady && workerReadyPromise
247+
240248
worker = null
241249
workerReady = false
242250
workerReadyPromise = null
251+
252+
let errorMessage = 'Worker process exited unexpectedly'
253+
if (stderrData.includes('isolated_vm') || stderrData.includes('MODULE_NOT_FOUND')) {
254+
errorMessage =
255+
'Code execution requires the isolated-vm native module which failed to load. ' +
256+
'This usually means the module needs to be rebuilt for your Node.js version. ' +
257+
'Please run: cd node_modules/isolated-vm && npm rebuild'
258+
logger.error('isolated-vm module failed to load', { stderr: stderrData })
259+
} else if (stderrData) {
260+
errorMessage = `Worker process failed: ${stderrData.slice(0, 500)}`
261+
logger.error('Worker process failed', { stderr: stderrData })
262+
}
263+
264+
if (wasStartupFailure) {
265+
clearTimeout(startTimeout)
266+
reject(new Error(errorMessage))
267+
return
268+
}
269+
243270
for (const [id, pending] of pendingExecutions) {
244271
clearTimeout(pending.timeout)
245272
pending.resolve({
246273
result: null,
247274
stdout: '',
248-
error: { message: 'Worker process exited unexpectedly', name: 'WorkerError' },
275+
error: { message: errorMessage, name: 'WorkerError' },
249276
})
250277
pendingExecutions.delete(id)
251278
}

0 commit comments

Comments
 (0)