diff --git a/apps/sim/executor/handlers/condition/condition-handler.test.ts b/apps/sim/executor/handlers/condition/condition-handler.test.ts index d12270cc45c..f7db2aa7846 100644 --- a/apps/sim/executor/handlers/condition/condition-handler.test.ts +++ b/apps/sim/executor/handlers/condition/condition-handler.test.ts @@ -202,6 +202,107 @@ describe('ConditionBlockHandler', () => { ) }) + it('mounts only the secrets the condition names', async () => { + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) + + const conditions = [ + { id: 'cond1', title: 'if', value: '"{{ROUTE_KEY}}" === "beta"' }, + { id: 'else1', title: 'else', value: '' }, + ] + + await handler.execute(mockContext, mockBlock, { conditions: JSON.stringify(conditions) }) + + const [, toolParams] = mockExecuteTool.mock.calls[0] + expect(toolParams.secretScope).toBe('selected') + expect(toolParams.mountedSecrets).toEqual(['ROUTE_KEY']) + }) + + it('denies every secret to a condition that names none', async () => { + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) + + const conditions = [ + { id: 'cond1', title: 'if', value: 'context.value > 5' }, + { id: 'else1', title: 'else', value: '' }, + ] + + await handler.execute(mockContext, mockBlock, { conditions: JSON.stringify(conditions) }) + + const [, toolParams] = mockExecuteTool.mock.calls[0] + expect(toolParams.secretScope).toBe('selected') + expect(toolParams.mountedSecrets).toEqual([]) + }) + + it('does not let resolved data decide which secrets the sandbox holds', async () => { + // The script carries the source block's output as data. Reading that data for either + // signal would let a caller pick what materializes beside it — the whole map by naming + // the global, or one secret by naming its placeholder. + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) + mockContext.blockStates.set('source-block-1', { + output: { text: 'environmentVariables.OPENAI_API_KEY {{OPENAI_API_KEY}}' }, + executed: true, + executionTime: 0, + } as BlockState) + + const conditions = [ + { id: 'cond1', title: 'if', value: `context.text === 'x'` }, + { id: 'else1', title: 'else', value: '' }, + ] + + await handler.execute(mockContext, mockBlock, { conditions: JSON.stringify(conditions) }) + + const [, toolParams] = mockExecuteTool.mock.calls[0] + expect(toolParams.code).toContain('{{OPENAI_API_KEY}}') + expect(toolParams.secretScope).toBe('selected') + expect(toolParams.mountedSecrets).toEqual([]) + }) + + it('trusts the resolver record over the word appearing in a resolved expression', async () => { + // The resolver saw the author's text before any value was inlined; the expression by now + // carries trigger data, where the same word means nothing. + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) + + const conditions = [ + { + id: 'cond1', + title: 'if', + value: `'environmentVariables.OPENAI_API_KEY' === 'x'`, + _readsEnvironmentVariables: false, + }, + { id: 'else1', title: 'else', value: '' }, + ] + + await handler.execute(mockContext, mockBlock, { conditions: JSON.stringify(conditions) }) + + const [, toolParams] = mockExecuteTool.mock.calls[0] + expect(toolParams.secretScope).toBe('selected') + expect(toolParams.mountedSecrets).toEqual([]) + }) + + it('keeps the whole environment for a condition that reads the environment directly', async () => { + // Every shape an expression can reach the map through, including the ones a member-access + // pattern would miss — narrowing one of those would route the run silently. + const reads = [ + 'environmentVariables.ROUTE_KEY === "beta"', + 'environmentVariables["ROUTE_KEY"] === "beta"', + 'environmentVariables?.ROUTE_KEY === "beta"', + 'Object.keys(environmentVariables).length > 0', + ] + + for (const value of reads) { + mockExecuteTool.mockReset() + mockExecuteTool.mockResolvedValueOnce(matchedAt(0)) + const conditions = [ + { id: 'cond1', title: 'if', value }, + { id: 'else1', title: 'else', value: '' }, + ] + + await handler.execute(mockContext, mockBlock, { conditions: JSON.stringify(conditions) }) + + const [, toolParams] = mockExecuteTool.mock.calls[0] + expect(toolParams.secretScope, `condition ${value}`).toBe('all') + } + }) + it('should never forward collected block outputs in the request body', async () => { mockCollectBlockData.mockReturnValueOnce({ blockData: { 'huge-block': { payload: 'x'.repeat(1024) } }, diff --git a/apps/sim/executor/handlers/condition/condition-handler.ts b/apps/sim/executor/handlers/condition/condition-handler.ts index d646428bd0f..1db5a685e0e 100644 --- a/apps/sim/executor/handlers/condition/condition-handler.ts +++ b/apps/sim/executor/handlers/condition/condition-handler.ts @@ -10,12 +10,14 @@ import type { BlockOutput } from '@/blocks/types' import { BlockType, DEFAULTS, EDGE } from '@/executor/constants' import type { BlockHandler, ExecutionContext } from '@/executor/types' import { collectBlockData } from '@/executor/utils/block-data' +import { createEnvVarPattern } from '@/executor/utils/reference-validation' import { buildBranchNodeId, extractBaseBlockId, extractBranchIndex, isBranchNodeId, } from '@/executor/utils/subflow-utils' +import { CONDITION_READS_ENVIRONMENT_KEY } from '@/executor/variables/resolver' import type { SerializedBlock } from '@/serializer/types' import { executeTool } from '@/tools' import type { ToolResponse } from '@/tools/types' @@ -28,6 +30,8 @@ interface ConditionEntry { id: string title: string value: string + /** Set by the resolver from the author's pre-resolution expression. */ + [CONDITION_READS_ENVIRONMENT_KEY]?: boolean } /** Verdict for a whole condition list evaluated in one function execution. */ @@ -88,6 +92,50 @@ function buildConditionScript(expressions: string[], evalContext: Record { + const recorded = condition[CONDITION_READS_ENVIRONMENT_KEY] + return recorded ?? /\benvironmentVariables\b/.test(condition.value) + }) + if (readsEnvironment) { + return { secretScope: 'all', mountedSecrets: [] } + } + + const named = new Set() + for (const condition of conditions) { + for (const match of String(condition.value ?? '').matchAll(createEnvVarPattern())) { + named.add(String(match[1]).trim()) + } + } + return { secretScope: 'selected', mountedSecrets: [...named] } +} + /** * Runs condition code through the shared function execution boundary. * @@ -100,15 +148,19 @@ function buildConditionScript(expressions: string[], evalContext: Record { const { blockNameMapping, blockOutputSchemas } = collectBlockData(ctx, currentNodeId) + const { secretScope, mountedSecrets } = scopeConditionSecrets(conditions) return executeTool( 'function_execute', { code, timeout: CONDITION_TIMEOUT_MS, + secretScope, + mountedSecrets, envVars: normalizeStringRecord(ctx.environmentVariables), workflowVariables: normalizeWorkflowVariables(ctx.workflowVariables), blockData: {}, @@ -143,13 +195,15 @@ function isTimeoutFailure(error: string | undefined): boolean { /** Evaluates the whole condition list in a single function execution. */ async function evaluateConditionList( ctx: ExecutionContext, - expressions: string[], + conditions: ConditionEntry[], evalContext: Record, currentNodeId?: string ): Promise { + const expressions = conditions.map((condition) => String(condition.value || '')) const result = await runConditionCode( ctx, buildConditionScript(expressions, evalContext), + conditions, currentNodeId ) @@ -229,12 +283,13 @@ async function evaluateConditionList( */ async function evaluateSingleCondition( ctx: ExecutionContext, - expression: string, + condition: ConditionEntry, evalContext: Record, currentNodeId?: string ): Promise { + const expression = String(condition.value || '') const code = `const context = ${JSON.stringify(evalContext)};\nreturn ${buildBooleanTest(expression)}` - const result = await runConditionCode(ctx, code, currentNodeId) + const result = await runConditionCode(ctx, code, [condition], currentNodeId) if (!result.success) { if (result.retryable === false) { @@ -418,11 +473,9 @@ export class ConditionBlockHandler implements BlockHandler { ): Promise { if (conditions.length === 0) return null - const expressions = conditions.map((condition) => String(condition.value || '')) - let evaluation: ConditionEvaluation try { - evaluation = await evaluateConditionList(ctx, expressions, evalContext, currentNodeId) + evaluation = await evaluateConditionList(ctx, conditions, evalContext, currentNodeId) } catch (error) { if (isNonRetryableExecutionError(error)) throw error evaluation = { @@ -474,7 +527,7 @@ export class ConditionBlockHandler implements BlockHandler { try { const conditionMet = await evaluateSingleCondition( ctx, - String(condition.value || ''), + condition, evalContext, currentNodeId ) diff --git a/apps/sim/executor/utils/code-formatting.ts b/apps/sim/executor/utils/code-formatting.ts index a4a73dee8ab..f7db26e28fd 100644 --- a/apps/sim/executor/utils/code-formatting.ts +++ b/apps/sim/executor/utils/code-formatting.ts @@ -46,3 +46,36 @@ export function formatLiteralForCode(value: unknown, language: 'javascript' | 'p } return JSON.stringify(value) } + +/** + * Escapes text so it cannot terminate whatever JavaScript literal it is spliced into. + * + * Condition expressions are user-authored JavaScript into which resolved references are + * inlined as source, so the author's quoting — not this module's — decides which string + * context the value lands in. Escaping only the quote the emitted literal opens leaves + * `"`, a backtick, `${`, and `/` live, and `"".includes('urgent')` then lets + * trigger data close the author's string and run as code in the condition sandbox. + * + * So every terminator of every JavaScript string context is escaped, not just the one the + * emitted literal opens. `\"`, `` \` ``, `\$`, and `\/` are identity escapes in JavaScript, + * so the value a condition compares is byte-identical to what it was before — this is a + * syntax guard, not a value transform. + * + * JavaScript only: `\$`, `` \` `` and `\/` are not identity escapes in Python, where they + * would change the value. Code blocks bind their values as runtime context variables + * instead of splicing them, which is why they need no escaping in any language. + */ +export function escapeInertStringContent(value: string): string { + return value + .replace(/\\/g, '\\\\') + .replace(/['"`$/]/g, '\\$&') + .replace(/\n/g, '\\n') + .replace(/\r/g, '\\r') + .replace(/\u2028/g, '\\u2028') + .replace(/\u2029/g, '\\u2029') +} + +/** Wraps {@link escapeInertStringContent} output as a complete JavaScript string literal. */ +export function formatInertStringLiteral(value: string, quote: '"' | "'" = "'"): string { + return `${quote}${escapeInertStringContent(value)}${quote}` +} diff --git a/apps/sim/executor/variables/resolver.test.ts b/apps/sim/executor/variables/resolver.test.ts index aa0c8c93b42..f4243461260 100644 --- a/apps/sim/executor/variables/resolver.test.ts +++ b/apps/sim/executor/variables/resolver.test.ts @@ -144,6 +144,43 @@ async function resolveConditionExpression( return (result.conditions as Array<{ value: string }>)[0].value } +/** Resolves one condition expression against a producer output an attacker supplied. */ +async function resolveConditionWithBlockOutput(value: string, result: unknown): Promise { + const { ctx, resolver, state } = createResolver() + state.setBlockOutput('producer', { result } as never) + const conditionBlock = createBlock('condition', 'Condition', BlockType.CONDITION) + const resolved = await resolver.resolveInputs( + ctx, + conditionBlock.id, + { conditions: JSON.stringify([{ id: 'condition-1', title: 'if', value }]) }, + conditionBlock + ) + return (resolved.conditions as Array<{ value: string }>)[0].value +} + +const INJECTION_CANARY = '__conditionInjectionCanary' + +/** + * Evaluates a resolved expression inside the same `Boolean(...)` wrapper the handler builds, + * reporting both the branch verdict and whether anything the resolved data carried executed. + */ +function runResolvedCondition(expression: string): { matched: boolean; injected: boolean } { + Reflect.set(globalThis, INJECTION_CANARY, 'not-executed') + try { + const matched = Boolean( + new Function(`const context = {};\nreturn Boolean(\n${expression}\n)`)() + ) + return { matched, injected: Reflect.get(globalThis, INJECTION_CANARY) !== 'not-executed' } + } catch { + return { + matched: false, + injected: Reflect.get(globalThis, INJECTION_CANARY) !== 'not-executed', + } + } finally { + Reflect.deleteProperty(globalThis, INJECTION_CANARY) + } +} + /** * Completes the round trip a condition actually takes: resolver, then the execution-boundary * compiler, then evaluation of the same `Boolean(...)` wrapper `condition-handler.ts` builds. @@ -199,16 +236,77 @@ describe('VariableResolver function block inputs', () => { ) expect(result.conditions).toEqual([ - { id: 'condition-1', title: 'if', value: '123 === 123' }, - { id: 'condition-2', title: 'else if', value: 'true === true' }, + { id: 'condition-1', title: 'if', value: '123 === 123', _readsEnvironmentVariables: false }, + { + id: 'condition-2', + title: 'else if', + value: 'true === true', + _readsEnvironmentVariables: false, + }, { id: 'condition-3', title: 'else if', value: '"Bearer {{API_KEY}}" === "Bearer token"', + _readsEnvironmentVariables: false, }, ]) }) + it('records whether the author, not the trigger data, reads the environment map', async () => { + const { ctx, resolver } = createResolver() + ctx.environmentVariables = {} + const conditionBlock = createBlock('condition', 'Condition', BlockType.CONDITION) + // The second branch only quotes a producer output that happens to contain the word. + const conditions = [ + { id: 'c1', title: 'if', value: `environmentVariables.FLAG === 'on'` }, + { id: 'c2', title: 'else if', value: `"" === 'x'` }, + ] + ;(ctx.blockStates as Map).set('producer', { + output: { result: 'environmentVariables.OPENAI_API_KEY' }, + executed: true, + executionTime: 0, + }) + + const result = await resolver.resolveInputs( + ctx, + conditionBlock.id, + { conditions: JSON.stringify(conditions) }, + conditionBlock + ) + + const resolvedConditions = result.conditions as Array> + expect(resolvedConditions[0]._readsEnvironmentVariables).toBe(true) + expect(resolvedConditions[1]._readsEnvironmentVariables).toBe(false) + expect(resolvedConditions[1].value).toContain('environmentVariables.OPENAI_API_KEY') + }) + + it('counts an environment read only where it can execute', async () => { + const { ctx, resolver } = createResolver() + const conditionBlock = createBlock('condition', 'Condition', BlockType.CONDITION) + const conditions = [ + // Reads, in the shapes a pattern would have to anticipate. + { id: 'c1', title: 'if', value: `environmentVariables?.FLAG === 'on'` }, + { id: 'c2', title: 'else if', value: 'Object.keys(environmentVariables).length > 0' }, + // Mentions: text, not code. + { id: 'c3', title: 'else if', value: `'environmentVariables.FLAG' === 'x'` }, + { id: 'c4', title: 'else if', value: '`environmentVariables` === "x"' }, + { id: 'c5', title: 'else if', value: `/environmentVariables/.test('x')` }, + ] + + const result = await resolver.resolveInputs( + ctx, + conditionBlock.id, + { conditions: JSON.stringify(conditions) }, + conditionBlock + ) + + expect( + (result.conditions as Array>).map( + (condition) => condition._readsEnvironmentVariables + ) + ).toEqual([true, true, false, false, false]) + }) + it('preserves legacy condition outcomes end to end through the boundary compiler', async () => { const environmentVariables = { API_KEY: 'token', @@ -255,6 +353,135 @@ describe('VariableResolver function block inputs', () => { ).resolves.toBe(true) }) + it('stops trigger data from breaking out of a quoted condition reference', async () => { + // Every quoting an author can put around a reference. The author picks the context; + // the resolved value must be data in all of them, not just the one it wraps itself in. + const quotings = [ + `"".includes('urgent')`, + '"" === "admin"', + '``.length > 0', + '//.test("x")', + ` === 'admin'`, + ] + const payloads = [ + `" + (globalThis.${INJECTION_CANARY} = "ran") + "`, + `\${(globalThis.${INJECTION_CANARY} = "ran")}`, + `' + (globalThis.${INJECTION_CANARY} = "ran") + '`, + `/ + (globalThis.${INJECTION_CANARY} = "ran") + /`, + ] + + for (const value of quotings) { + for (const payload of payloads) { + const expression = await resolveConditionWithBlockOutput(value, payload) + expect( + runResolvedCondition(expression).injected, + `condition ${value} executed trigger data: ${expression}` + ).toBe(false) + } + } + }) + + it('stops a trigger-supplied object from closing the string it is quoted inside', async () => { + // JSON's own structural quotes close the author's string, and the key is the attacker's. + const expression = await resolveConditionWithBlockOutput('"" === "{}"', { + [`+(globalThis.${INJECTION_CANARY}=1)+`]: 1, + }) + expect(runResolvedCondition(expression).injected).toBe(false) + }) + + it('stops a trigger-supplied object from escaping wherever the quote scanner mis-reads', async () => { + // A regex literal is not tracked by the quote scanner, and a quote inside one + // desynchronizes it for everything that follows, so the emitted object must be inert + // whichever context the scanner reports. + const payloads = [ + { [`+(globalThis.${INJECTION_CANARY}=1)+`]: 1 }, + { forged: `/ + (globalThis.${INJECTION_CANARY}=1) + /` }, + { closed: `" + (globalThis.${INJECTION_CANARY}=1) + "` }, + ] + const quotings = [ + '//.test("x")', + `/['"]/.test('a') && .count === 2`, + `/['"]/.test('a') && "" === "{}"`, + '.count === 2', + ] + + for (const value of quotings) { + for (const payload of payloads) { + const expression = await resolveConditionWithBlockOutput(value, payload) + expect( + runResolvedCondition(expression).injected, + `condition ${value} executed object data: ${expression}` + ).toBe(false) + } + } + }) + + it('keeps navigating an object reference the scanner reports as unquoted', async () => { + const expression = await resolveConditionWithBlockOutput('.count === 2', { + count: 2, + note: `a "quoted" / slashed ' value`, + }) + expect(runResolvedCondition(expression).matched).toBe(true) + }) + + it('evaluates references that follow a regex literal, quote-bearing or not', async () => { + // A regex body is the one place a lone quote is not a string delimiter. Reading it as one + // left every later reference formatted for a context it was not in — a quoted object + // reference stayed raw source, and a bare one was emitted as escaped JSON that cannot parse. + const cases: Array<{ value: string; result: unknown; expected: boolean }> = [ + // Every case reaches its reference — a short-circuit would pass on a formatter that + // emits source the sandbox cannot parse, which is the failure being pinned here. + { + value: `/['"a]/.test('a') && .count === 2`, + result: { count: 2 }, + expected: true, + }, + { value: `/['"]/.test('a') || === 'x'`, result: 'x', expected: true }, + { + value: `/it's/.test('a') || "".includes('b')`, + result: 'abc', + expected: true, + }, + { + value: `/[a-z]/.test('a') && .count === 2`, + result: { count: 2 }, + expected: true, + }, + // Division, not a regex: the scan must not swallow the rest of the expression. + { value: `.total / 2 === 5`, result: { total: 10 }, expected: true }, + { + value: `(.total / 2) === 5 && ''.length > 0`, + result: { total: 10 }, + expected: true, + }, + ] + + for (const { value, result, expected } of cases) { + const expression = await resolveConditionWithBlockOutput(value, result) + const verdict = runResolvedCondition(expression) + expect(verdict.injected, `condition ${value} executed data: ${expression}`).toBe(false) + expect(verdict.matched, `condition ${value} resolved to: ${expression}`).toBe(expected) + } + }) + + it('keeps quoted and bare condition references comparing what they compared before', async () => { + const cases: Array<{ value: string; result: unknown; expected: boolean }> = [ + { value: ` === 'urgent'`, result: 'urgent', expected: true }, + { value: ` === 'urgent'`, result: 'other', expected: false }, + { value: `"".includes('urgent')`, result: 'urgent ticket', expected: true }, + { value: `"".includes('urgent')`, result: 'calm ticket', expected: false }, + { value: '``.length > 3', result: 'hello', expected: true }, + { value: ` === 'a"b'`, result: 'a"b', expected: true }, + { value: ' === `a$b/c`', result: 'a$b/c', expected: true }, + { value: `.count === 2`, result: { count: 2 }, expected: true }, + ] + + for (const { value, result, expected } of cases) { + const expression = await resolveConditionWithBlockOutput(value, result) + expect(runResolvedCondition(expression).matched, `condition ${value}`).toBe(expected) + } + }) + it('compares a bare string placeholder instead of throwing a reference error', async () => { await expect( evaluateResolvedCondition(`{{NAME}} === 'alice'`, { NAME: 'alice' }) @@ -719,6 +946,161 @@ describe('VariableResolver function block inputs', () => { expect(result.contextVariables).toEqual({ __blockRef_0: ref }) }) + it('reads the context of a reference that follows a statement-position regex', async () => { + // `)` ends a value in `(a + b) / 2` and a control-flow head in `if (a) /re/.test(b)`, and + // the closing parenthesis alone does not say which. Guessing either way misreads one of + // them, and a quote inside the regex then decides how every later reference is spliced. + const { block, ctx, resolver } = createResolver('javascript') + + // Both cases stay on one line: a string mode ends at a newline, so only a reference sharing + // the line with the misread slash sees the wrong context. + const result = await resolver.resolveInputsForFunctionBlock( + ctx, + 'function', + { + code: [ + `if (params.a) /['"]/.test('')`, + `const divided = (params.c + 1) / 2 + Number('')`, + 'return divided', + ].join('\n'), + }, + block + ) + + const code = result.resolvedInputs.code as string + // Statement-position regex: the reference after it is inside the author's quotes. + expect(code).toContain(`.test('' + JSON.stringify(globalThis["__blockRef_0"]) + '')`) + // Division after a value: the slash must not open a regex that swallows the quotes. + expect(code).toContain(`Number('' + JSON.stringify(globalThis["__blockRef_1"]) + '')`) + }) + + it('steps over a comment rather than reading it as the preceding token', async () => { + const { block, ctx, resolver } = createResolver('javascript') + + const result = await resolver.resolveInputsForFunctionBlock( + ctx, + 'function', + { + code: [ + `/* lead */ if (params.a) /['"]/.test('')`, + `const n = params.p./* mid */catch(() => 0) / 2 + Number('')`, + // A comment body may contain another opening delimiter; the comment still ends at + // the first `*/`, which only the scan that passed through it knows. + `const m = params.q./* a /* b */catch(() => 0) / 2 + Number('')`, + ].join('\n'), + }, + block + ) + + // A comment before a control-flow keyword leaves it a head; one hiding a property dot + // still leaves the call a call. + const code = result.resolvedInputs.code as string + expect(code).toContain(`.test('' + JSON.stringify(globalThis["__blockRef_0"]) + '')`) + expect(code).toContain(`Number('' + JSON.stringify(globalThis["__blockRef_1"]) + '')`) + expect(code).toContain(`Number('' + JSON.stringify(globalThis["__blockRef_2"]) + '')`) + }) + + it('starts a new identifier at a line break rather than continuing the last one', async () => { + const { block, ctx, resolver } = createResolver('javascript') + + const result = await resolver.resolveInputsForFunctionBlock( + ctx, + 'function', + { + // `if` begins a statement here; reading the previous *significant* character would + // see the `b` of `params.b` and carry its property-access answer into this token. + code: ['const seen = params.a.b', `if (seen) /['"]/.test('')`].join('\n'), + }, + block + ) + + expect(result.resolvedInputs.code).toContain( + `.test('' + JSON.stringify(globalThis["__blockRef_0"]) + '')` + ) + }) + + it('divides after a postfix update rather than opening a regex', async () => { + const { block, ctx, resolver } = createResolver('javascript') + + const result = await resolver.resolveInputsForFunctionBlock( + ctx, + 'function', + { + code: [ + `let i = params.i; const half = i++ / 2 + Number('')`, + // The same characters as an operator still precede a regex. + `const hit = params.n + /['"]/.test('')`, + ].join('\n'), + }, + block + ) + + const code = result.resolvedInputs.code as string + expect(code).toContain(`Number('' + JSON.stringify(globalThis["__blockRef_0"]) + '')`) + expect(code).toContain(`.test('' + JSON.stringify(globalThis["__blockRef_1"]) + '')`) + }) + + it('does not read a method named after a keyword as a control-flow head', async () => { + const { block, ctx, resolver } = createResolver('javascript') + + const result = await resolver.resolveInputsForFunctionBlock( + ctx, + 'function', + { code: `const n = params.p.catch(() => 0) / 2 + Number('')` }, + block + ) + + // `.catch(…)` is a call, so the slash after it divides — it must not open a regex that + // runs over the quotes around the reference. + expect(result.resolvedInputs.code).toContain( + `Number('' + JSON.stringify(globalThis["__blockRef_0"]) + '')` + ) + }) + + it('binds a run value that names a secret instead of expanding it', async () => { + const { block, ctx, resolver } = createResolver('javascript') + ctx.workflowVariables = { + 'var-1': { id: 'var-1', name: 'authTemplate', type: 'string', value: 'Bearer {{API_KEY}}' }, + } + + const result = await resolver.resolveInputsForFunctionBlock( + ctx, + 'function', + { code: `const header = ''; const item = ` }, + block + ) + + // An author-configured variable keeps its placeholder in source, where the boundary + // compiler expands it; a run value naming a secret binds instead, so whoever supplies + // the text cannot pick what the compiler materializes next to it. + const code = result.resolvedInputs.code as string + expect(code).toContain('{{API_KEY}}') + expect(code).toContain('const item = globalThis["__blockRef_0"]') + expect(result.contextVariables).toEqual({ __blockRef_0: 'hello world' }) + }) + + it('binds a workflow variable carrying quote characters instead of splicing it into code', async () => { + // A Variables block can assign trigger data at runtime, so a variable's value is not + // necessarily the author's. Inlined as a literal it closed the string it landed in. + const { block, ctx, resolver } = createResolver('javascript') + const payload = `' + (globalThis.__functionInjection = 1) + '` + ctx.workflowVariables = { + 'var-1': { id: 'var-1', name: 'userinput', type: 'string', value: payload }, + } + + const result = await resolver.resolveInputsForFunctionBlock( + ctx, + 'function', + { code: `const x = ''; return x` }, + block + ) + + expect(result.resolvedInputs.code).toBe( + `const x = '' + JSON.stringify(globalThis["__blockRef_0"]) + ''; return x` + ) + expect(result.contextVariables).toEqual({ __blockRef_0: payload }) + }) + it('rewrites whole manifest workflow variables to lazy JavaScript array reads', async () => { const { block, ctx, resolver } = createResolver('javascript') const manifest = createTestManifest() @@ -791,8 +1173,11 @@ describe('VariableResolver function block inputs', () => { ['0', 'key'], expect.objectContaining({ allowLargeValueRefs: true }) ) - expect(result.resolvedInputs.code).toBe('return "SIM-0"') - expect(result.contextVariables).toEqual({}) + // The navigated element binds like any other resolved value; what must not appear is + // the manifest, or the array it stands for. + expect(result.resolvedInputs.code).toBe('return globalThis["__blockRef_0"]') + expect(result.displayInputs.code).toBe('return "SIM-0"') + expect(result.contextVariables).toEqual({ __blockRef_0: 'SIM-0' }) }) it('resolves named loop result bracket paths in function code', async () => { diff --git a/apps/sim/executor/variables/resolver.ts b/apps/sim/executor/variables/resolver.ts index 94c0bee918e..7184e4660db 100644 --- a/apps/sim/executor/variables/resolver.ts +++ b/apps/sim/executor/variables/resolver.ts @@ -18,6 +18,10 @@ import { isLikelyReferenceSegment } from '@/lib/workflows/sanitization/reference import { BlockType, parseReferencePath, REFERENCE } from '@/executor/constants' import type { ExecutionState, LoopScope } from '@/executor/execution/state' import type { ExecutionContext, UserFile } from '@/executor/types' +import { + escapeInertStringContent, + formatInertStringLiteral, +} from '@/executor/utils/code-formatting' import { createEnvVarPattern, createReferencePattern } from '@/executor/utils/reference-validation' import { BlockResolver } from '@/executor/variables/resolvers/block' import { EnvResolver } from '@/executor/variables/resolvers/env' @@ -32,6 +36,18 @@ import { import { WorkflowResolver } from '@/executor/variables/resolvers/workflow' import type { SerializedBlock, SerializedWorkflow } from '@/serializer/types' +/** + * Marks a Condition branch whose author-written expression reads the environment map. + * + * Carried per branch because only the pre-resolution text can answer it: the handler decides + * which secrets to mount, and by the time it runs, resolved trigger data quoted inside the + * expression would read the same as the author reaching for the map. + */ +export const CONDITION_READS_ENVIRONMENT_KEY = '_readsEnvironmentVariables' + +/** The sandbox global holding the run's secrets, in whatever shape an expression reaches it. */ +const ENVIRONMENT_MAP_IDENTIFIER = /\benvironmentVariables\b/g + /** Key used to carry pre-resolved context variables through the inputs map. */ export const FUNCTION_BLOCK_CONTEXT_VARS_KEY = '_runtimeContextVars' /** Key used to carry display-resolved code through the function execution path. */ @@ -142,7 +158,12 @@ function isStructurallyInertConditionLiteral(value: string): boolean { } type ShellQuoteContext = 'single' | 'double' | null -type CodeStringQuoteContext = ShellQuoteContext | 'triple-single' | 'triple-double' | 'template' +type CodeStringQuoteContext = + | ShellQuoteContext + | 'triple-single' + | 'triple-double' + | 'template' + | 'regex' type CodeScanMode = | { type: 'normal' } | { type: 'single' } @@ -153,6 +174,46 @@ type CodeScanMode = | { type: 'template-expression'; depth: number } | { type: 'line-comment' } | { type: 'block-comment' } + | { type: 'regex'; inCharacterClass: boolean } + +/** + * Characters after which a `/` opens a regular expression rather than dividing. + * + * The scanner has to tell the two apart, because a regex body is the one place a lone quote + * is not a string delimiter: `/['"]/` left the scan believing everything after it sat inside + * a string, and every reference past that point was then formatted for the wrong context. + * Division always follows a value — an identifier, literal, `)`, or `]` — so anything else + * ending the preceding token means a regex may start. + * + * `)` is the one that is not decidable from the character alone: it ends a value in + * `(a + b) / 2` and a control-flow head in `if (a) /re/.test(b)`. Reading it as either one + * unconditionally breaks the other, so the scan remembers which kind of parenthesis each `)` + * closed rather than guessing — see {@link CONTROL_FLOW_HEAD_KEYWORDS}. + */ +const JAVASCRIPT_REGEX_ALLOWED_AFTER = new Set('(,=:[!&|?{};+-*%^~<>/'.split('')) + +/** Keywords whose parenthesized head is followed by a statement, where a regex may start. */ +const CONTROL_FLOW_HEAD_KEYWORDS = new Set(['if', 'while', 'for', 'switch', 'catch', 'with']) + +const WHITESPACE_CHAR = /\s/ + +/** Keywords a regex may directly follow, where the preceding token is a word rather than punctuation. */ +const JAVASCRIPT_REGEX_ALLOWED_AFTER_KEYWORDS = new Set([ + 'return', + 'typeof', + 'instanceof', + 'in', + 'of', + 'new', + 'delete', + 'void', + 'case', + 'throw', + 'do', + 'else', + 'yield', + 'await', +]) export class VariableResolver { private resolvers: Resolver[] @@ -297,6 +358,11 @@ export class VariableResolver { const value = Reflect.get(condition, 'value') return { ...condition, + // Recorded before resolution: once values are inlined, an expression that reads + // the environment map is indistinguishable from one that merely quotes trigger + // data containing the word, and the handler decides what to mount from this. + [CONDITION_READS_ENVIRONMENT_KEY]: + typeof value === 'string' && this.readsEnvironmentMap(value), value: typeof value === 'string' ? await this.resolveTemplateWithoutConditionFormatting( @@ -621,34 +687,31 @@ export class VariableResolver { throw getNestedLargeValueMaterializationError() } - if ( - this.isWorkflowVariableReference(match) && - this.shouldUseContextVariable(effectiveValue) - ) { - const varName = `__blockRef_${Object.keys(contextVarAccumulator).length}` - contextVarAccumulator[varName] = effectiveValue - const replacement = this.formatContextVariableReference( - varName, - language, - template, - index, - effectiveValue - ) - displayResult += this.formatDisplayValueForCodeContext( + if (this.canInlineResolvedCodeLiteral(effectiveValue, match)) { + const replacement = this.blockResolver.formatValueForBlock( effectiveValue, - language, - template, - index + BlockType.FUNCTION, + language ) + displayResult += replacement return replacement } - const replacement = this.blockResolver.formatValueForBlock( + const varName = `__blockRef_${Object.keys(contextVarAccumulator).length}` + contextVarAccumulator[varName] = effectiveValue + const replacement = this.formatContextVariableReference( + varName, + language, + template, + index, + effectiveValue + ) + displayResult += this.formatDisplayValueForCodeContext( effectiveValue, - BlockType.FUNCTION, - language + language, + template, + index ) - displayResult += replacement return replacement } catch (error) { replacementError = error instanceof Error ? error : new Error(String(error)) @@ -898,13 +961,37 @@ export class VariableResolver { }) } - private isWorkflowVariableReference(reference: string): boolean { - const parts = parseReferencePath(reference) - return parts[0] === REFERENCE.PREFIX.VARIABLE - } - - private shouldUseContextVariable(value: unknown): boolean { - return typeof value === 'object' && value !== null + /** + * Whether a resolved value may stay a literal in generated code rather than being bound + * as a context variable. + * + * Splicing a value into user-authored code hands the author's quoting the decision of how + * that value parses, so only values that cannot terminate a literal may stay inline. + * Numbers, booleans, and null render as digits or keywords. A string qualifies only when + * it names an environment variable and carries no character that could close a string in + * any supported language: that shape has to stay in source because the placeholder, never + * the secret, is what gets inlined, and the execution-boundary compiler binds it + * downstream — that is how `` reaches its value. + * + * Everything else binds, which is what block outputs have always done. Inlining the rest + * is what let a runtime-assigned variable or loop item carrying trigger data close the + * string it landed in and run as code. + * + * The placeholder case is admitted only for a workflow variable, never for a loop item or + * any other run value. Whoever supplies the text picks which secret the compiler expands + * into the generated source, so that choice stays with the surface an author configures. + */ + private canInlineResolvedCodeLiteral(value: unknown, reference: string): boolean { + if (value === null || typeof value === 'number' || typeof value === 'boolean') { + return true + } + if (typeof value !== 'string') { + return false + } + if (parseReferencePath(reference)[0] !== REFERENCE.PREFIX.VARIABLE) { + return false + } + return createEnvVarPattern().test(value) && !/['"`$\\\n\r\u2028\u2029]/.test(value) } private formatJavaScriptAsyncExpression( @@ -1055,6 +1142,105 @@ export class VariableResolver { ) } + /** + * Whether a `/` at this point opens a regular expression rather than dividing. + * + * Division always follows a value, so the preceding token decides: an identifier that is not + * one of the keywords a regex may follow, a number, a `)`, or a `]` means division, and + * anything else means a regex may start. Guessing wrong is not silent — the scan would swallow + * text up to the next `/` — so the check reads the actual preceding token rather than assuming. + */ + private canStartJavaScriptRegex( + template: string, + previousSignificantIndex: number, + closes: { controlHeadParenCloses: ReadonlySet; regexCloseIndices: ReadonlySet } + ): boolean { + if (previousSignificantIndex < 0) { + return true + } + const previous = template[previousSignificantIndex] + if (previous === ')') { + return closes.controlHeadParenCloses.has(previousSignificantIndex) + } + if (previous === '/' && closes.regexCloseIndices.has(previousSignificantIndex)) { + return false + } + // `+` and `-` precede a regex as operators but end a value when doubled: `i++ / 2` divides. + if ( + (previous === '+' || previous === '-') && + template[previousSignificantIndex - 1] === previous + ) { + return false + } + if (JAVASCRIPT_REGEX_ALLOWED_AFTER.has(previous)) { + return true + } + if (!this.isJavaScriptIdentifierChar(previous)) { + return false + } + + let start = previousSignificantIndex + while (start > 0 && this.isJavaScriptIdentifierChar(template[start - 1])) { + start-- + } + return JAVASCRIPT_REGEX_ALLOWED_AFTER_KEYWORDS.has( + template.slice(start, previousSignificantIndex + 1) + ) + } + + /** + * Whether a Condition expression reads the run's secrets off the environment map. + * + * The name is only a read where it can execute, so each occurrence is placed with the same + * scanner that decides how references are spliced — a mention inside a string, a template, + * or a regex is text and mounts nothing. No shape of the read itself is assumed: + * `environmentVariables?.FLAG` and `Object.keys(environmentVariables)` both count, because + * narrowing an expression that does reach the map would route the run down a branch the + * author did not write, silently, while admitting one too many only costs the narrowing. + */ + private readsEnvironmentMap(expression: string): boolean { + ENVIRONMENT_MAP_IDENTIFIER.lastIndex = 0 + let match = ENVIRONMENT_MAP_IDENTIFIER.exec(expression) + while (match !== null) { + if (this.getCodeStringQuoteContext(expression, match.index, 'javascript') === null) { + return true + } + match = ENVIRONMENT_MAP_IDENTIFIER.exec(expression) + } + return false + } + + /** + * Whether a `(` opens a control-flow head rather than a value. + * + * What follows the matching `)` differs entirely between the two — a statement, where a regex + * literal may begin, versus an operator, where a `/` divides — and the closing parenthesis + * carries no trace of which it was. The keyword in front of the opening one is what tells + * them apart, so `if (a) /re/.test(b)` and `(a + b) / 2` both scan correctly. + * + * Both inputs come from the forward scan rather than a walk back through the source: the + * scan already knows which characters were code and which sat inside a comment, and reading + * backwards cannot recover that — the opener of `/* a /* b *\/` is its first delimiter, not + * its last, and only the scan that passed through knows the difference. + */ + private opensControlFlowHead( + template: string, + previousSignificantIndex: number, + precededByPropertyAccess: boolean + ): boolean { + if (precededByPropertyAccess || previousSignificantIndex < 0) { + return false + } + if (!this.isJavaScriptIdentifierChar(template[previousSignificantIndex])) { + return false + } + let start = previousSignificantIndex + while (start > 0 && this.isJavaScriptIdentifierChar(template[start - 1])) { + start-- + } + return CONTROL_FLOW_HEAD_KEYWORDS.has(template.slice(start, previousSignificantIndex + 1)) + } + private matchesKeywordAt(template: string, index: number, keyword: string): boolean { if (!template.startsWith(keyword, index)) { return false @@ -1182,6 +1368,11 @@ export class VariableResolver { ): CodeStringQuoteContext { const isPython = language === 'python' const modes: CodeScanMode[] = [{ type: 'normal' }] + let lastSignificantIndex = -1 + const openParenIsControlHead: boolean[] = [] + let identifierFollowsPropertyAccess = false + const controlHeadParenCloses = new Set() + const regexCloseIndices = new Set() for (let i = 0; i < index; i++) { const char = template[i] @@ -1203,6 +1394,34 @@ export class VariableResolver { continue } + if (mode.type === 'regex') { + if (char === '\\') { + i++ + continue + } + // A regex literal cannot span a line, so an unterminated one means the `/` was + // division after all; dropping the mode keeps the rest of the scan honest. + if (char === '\n') { + modes.pop() + continue + } + if (char === '[') { + mode.inCharacterClass = true + continue + } + if (char === ']') { + mode.inCharacterClass = false + continue + } + if (char === '/' && !mode.inCharacterClass) { + modes.pop() + // The literal that just closed is a value, so the next `/` divides it. + regexCloseIndices.add(i) + lastSignificantIndex = i + } + continue + } + if (mode.type === 'single' || mode.type === 'double') { const quote = mode.type === 'single' ? "'" : '"' if (char === '\\') { @@ -1255,6 +1474,39 @@ export class VariableResolver { i++ continue } + const previousSignificantIndex = lastSignificantIndex + if (!WHITESPACE_CHAR.test(char)) { + lastSignificantIndex = i + } + if (this.isJavaScriptIdentifierChar(char)) { + // An identifier continues only when the character right before it is part of the same + // token. Asking the previous *significant* character instead treats a name after a + // line break as a continuation and leaves it carrying the last one's answer. + if (i === 0 || !this.isJavaScriptIdentifierChar(template[i - 1])) { + identifierFollowsPropertyAccess = template[previousSignificantIndex] === '.' + } + } else if (char === '(') { + openParenIsControlHead.push( + this.opensControlFlowHead( + template, + previousSignificantIndex, + identifierFollowsPropertyAccess + ) + ) + } else if (char === ')') { + if (openParenIsControlHead.pop()) controlHeadParenCloses.add(i) + } + if ( + !isPython && + char === '/' && + this.canStartJavaScriptRegex(template, previousSignificantIndex, { + controlHeadParenCloses, + regexCloseIndices, + }) + ) { + modes.push({ type: 'regex', inCharacterClass: false }) + continue + } if (isPython && char === "'" && next === "'" && template[i + 2] === "'") { modes.push({ type: 'triple-single' }) i += 2 @@ -1304,6 +1556,39 @@ export class VariableResolver { i++ continue } + const previousSignificantIndex = lastSignificantIndex + if (!WHITESPACE_CHAR.test(char)) { + lastSignificantIndex = i + } + if (this.isJavaScriptIdentifierChar(char)) { + // An identifier continues only when the character right before it is part of the same + // token. Asking the previous *significant* character instead treats a name after a + // line break as a continuation and leaves it carrying the last one's answer. + if (i === 0 || !this.isJavaScriptIdentifierChar(template[i - 1])) { + identifierFollowsPropertyAccess = template[previousSignificantIndex] === '.' + } + } else if (char === '(') { + openParenIsControlHead.push( + this.opensControlFlowHead( + template, + previousSignificantIndex, + identifierFollowsPropertyAccess + ) + ) + } else if (char === ')') { + if (openParenIsControlHead.pop()) controlHeadParenCloses.add(i) + } + if ( + !isPython && + char === '/' && + this.canStartJavaScriptRegex(template, previousSignificantIndex, { + controlHeadParenCloses, + regexCloseIndices, + }) + ) { + modes.push({ type: 'regex', inCharacterClass: false }) + continue + } if (isPython && char === "'" && next === "'" && template[i + 2] === "'") { modes.push({ type: 'triple-single' }) i += 2 @@ -1320,6 +1605,9 @@ export class VariableResolver { } const mode = modes[modes.length - 1] + if (mode.type === 'regex') { + return 'regex' + } if ( mode.type === 'single' || mode.type === 'double' || @@ -1532,19 +1820,12 @@ export class VariableResolver { } if (typeof resolved === 'string') { - const escaped = resolved - .replace(/\\/g, '\\\\') - .replace(/'/g, "\\'") - .replace(/\n/g, '\\n') - .replace(/\r/g, '\\r') - .replace(/\u2028/g, '\\u2028') - .replace(/\u2029/g, '\\u2029') - const formatted = `'${escaped}'` + const formatted = formatInertStringLiteral(resolved) projectedReferenceResult += containsResolvedSecret ? match : formatted return formatted } if (typeof resolved === 'object' && resolved !== null) { - const formatted = JSON.stringify(resolved) + const formatted = this.formatConditionJson(resolved, template, index) projectedReferenceResult += containsResolvedSecret ? match : formatted return formatted } @@ -1576,6 +1857,27 @@ export class VariableResolver { return result } + /** + * Renders a resolved object for a Condition expression. + * + * Inside a quoted string the object is data, so its JSON is escaped to stay inside the + * string the author opened: raw, the JSON's own structural quotes close that string, and + * `"" === "{}"` with a crafted key emits `"{"+attackerCode()+":1}"`, which + * parses as concatenation and runs. + * + * Everywhere else the object is parsed at runtime rather than spliced as source. The value + * is identical to the object literal it replaces, but the payload is a fully escaped + * literal, so a crafted key can neither close a string nor forge a regex delimiter — which + * matters because the emitted form must be safe even when the quote scanner reads the + * surrounding context wrongly, and a quote inside a regex literal is enough to do that. + * Splicing raw JSON would make that heuristic load-bearing for injection. + */ + private formatConditionJson(value: object, template: string, matchIndex: number): string { + const escaped = escapeInertStringContent(JSON.stringify(value)) + const quoteContext = this.getCodeStringQuoteContext(template, matchIndex, 'javascript') + return quoteContext === null ? `JSON.parse('${escaped}')` : escaped + } + private async resolveReference(reference: string, context: ResolutionContext): Promise { for (const resolver of this.resolvers) { if (resolver.canResolve(reference)) { diff --git a/apps/sim/executor/variables/resolvers/block.test.ts b/apps/sim/executor/variables/resolvers/block.test.ts index 7fa1576742b..de4cb9da017 100644 --- a/apps/sim/executor/variables/resolvers/block.test.ts +++ b/apps/sim/executor/variables/resolvers/block.test.ts @@ -711,6 +711,17 @@ describe('BlockResolver', () => { expect(resolver.formatValueForBlock('tab\there', 'condition')).toBe('"tab\there"') }) + it.concurrent('should escape the quotes it does not open for condition block', () => { + // The author's quoting decides which literal this lands in, so escaping only the + // double quote this wrapper opens leaves the other contexts breakable. + const resolver = new BlockResolver(createTestWorkflow()) + expect(resolver.formatValueForBlock("' + evil() + '", 'condition')).toBe( + '"\\\' + evil() + \\\'"' + ) + expect(resolver.formatValueForBlock(`\${evil()}`, 'condition')).toBe(`"\\\${evil()}"`) + expect(resolver.formatValueForBlock('`evil()`', 'condition')).toBe('"\\`evil()\\`"') + }) + it.concurrent('should format object for condition block', () => { const resolver = new BlockResolver(createTestWorkflow()) const result = resolver.formatValueForBlock({ key: 'value' }, 'condition') diff --git a/apps/sim/executor/variables/resolvers/block.ts b/apps/sim/executor/variables/resolvers/block.ts index 83f20eecd27..2c3f634e08b 100644 --- a/apps/sim/executor/variables/resolvers/block.ts +++ b/apps/sim/executor/variables/resolvers/block.ts @@ -13,7 +13,7 @@ import { resolveBlockReference, resolveBlockReferenceAsync, } from '@/executor/utils/block-reference' -import { formatLiteralForCode } from '@/executor/utils/code-formatting' +import { formatInertStringLiteral, formatLiteralForCode } from '@/executor/utils/code-formatting' import { buildClonedSubflowId, extractOuterBranchIndex } from '@/executor/utils/subflow-utils' import { type AsyncPathNavigator, @@ -414,12 +414,7 @@ export class BlockResolver implements Resolver { private stringifyForCondition(value: any): string { if (typeof value === 'string') { - const sanitized = value - .replace(/\\/g, '\\\\') - .replace(/"/g, '\\"') - .replace(/\n/g, '\\n') - .replace(/\r/g, '\\r') - return `"${sanitized}"` + return formatInertStringLiteral(value, '"') } if (value === null) { return 'null'