Skip to content

Commit 3ee8395

Browse files
committed
fix(workflows): protect references that nest an env-var placeholder
Review round 1. `findWorkflowReferenceTokens` is contractually non-overlapping, so for `<a.pick({{B}},c)>` it reports only the inner `{{B}}` and discards the outer candidate. That is correct for a tokenizer and wrong for a splitter, which needs the union of protected regions rather than a disjoint set, so the comma was unprotected and `c)>` was validated as a literal id. Adds a candidate pass built from the tokenizer's own exported predicates, leaving the shared package's non-overlapping contract untouched. It runs only when an environment token is present, since overlap with one is the only reason a workflow candidate is dropped. Also caps the value length before tokenizing. Reference detection parses the whole string and the tokenizer is superlinear in candidate count (795ms for a 240KB value of repeated `<a.b>,`), which this change newly puts on a write path that admits megabytes. Past the cap the field is skipped rather than parsed; the lint is advisory, so declining to check is the safe direction. Adds `as const` to the test context object per the repo's TypeScript conventions.
1 parent 36c2ea5 commit 3ee8395

5 files changed

Lines changed: 97 additions & 12 deletions

File tree

apps/sim/lib/workflows/editing/selector-reference-guard.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ vi.mock('@/lib/workflows/editing/selector-validator', () => ({
2323
import { collectUnresolvedReferences } from '@/lib/workflows/editing/validation'
2424
import { getBlock } from '@/blocks/registry'
2525

26-
const CTX = { userId: 'user-1', workspaceId: 'workspace-1' }
26+
const CTX = { userId: 'user-1', workspaceId: 'workspace-1' } as const
2727

2828
/** The real basic member of the knowledge block's `knowledgeBaseId` canonical pair. */
2929
const KB_SELECTOR_ID = 'knowledgeBaseSelector'

apps/sim/lib/workflows/editing/validation.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1304,6 +1304,26 @@ describe('collectUnresolvedReferences', () => {
13041304
expect(refs).toHaveLength(0)
13051305
})
13061306

1307+
it('skips an oversized selector value instead of tokenizing it', async () => {
1308+
// Reference detection tokenizes the whole string and the tokenizer is superlinear in
1309+
// candidate count, so an implausible value is skipped rather than parsed.
1310+
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: ['x'] })
1311+
const state = {
1312+
blocks: {
1313+
kb1: {
1314+
type: 'knowledge',
1315+
name: 'KB',
1316+
subBlocks: { knowledgeBaseId: { value: `kb_${'a'.repeat(10_000)}` } },
1317+
},
1318+
},
1319+
}
1320+
const startedAt = performance.now()
1321+
const refs = await collectUnresolvedReferences(state, CTX)
1322+
expect(performance.now() - startedAt).toBeLessThan(1000)
1323+
expect(mockValidateSelectorIds).not.toHaveBeenCalled()
1324+
expect(refs).toHaveLength(0)
1325+
})
1326+
13071327
it('still validates a plain literal id (the guard must not over-skip)', async () => {
13081328
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: ['kb_missing'] })
13091329
const state = {

apps/sim/lib/workflows/editing/validation.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1082,6 +1082,15 @@ interface SelectorFieldToValidate {
10821082
value: string | string[]
10831083
}
10841084

1085+
/**
1086+
* A selector value is an id, or a short comma-separated list of them. Reference detection
1087+
* tokenizes the whole string and `findWorkflowReferenceTokens` is superlinear in candidate count,
1088+
* so an oversized value is both implausible and expensive on a write path that admits megabytes.
1089+
* Past this the field is skipped rather than tokenized: the lint is advisory, so declining to
1090+
* check is the safe direction.
1091+
*/
1092+
const MAX_SELECTOR_VALUE_LENGTH = 10_000
1093+
10851094
/**
10861095
* Walk a workflow state and collect selector/credential fields to validate.
10871096
* For canonical pairs only the ACTIVE member is collected (an intentionally-empty
@@ -1135,6 +1144,9 @@ function collectSelectorFields(
11351144

11361145
const subBlockValue = blockData.subBlocks?.[subBlockConfig.id]?.value
11371146
if (!subBlockValue) continue
1147+
if (typeof subBlockValue === 'string' && subBlockValue.length > MAX_SELECTOR_VALUE_LENGTH) {
1148+
continue
1149+
}
11381150

11391151
// Handle comma-separated values for multi-select
11401152
let values: string | string[] = subBlockValue

apps/sim/lib/workflows/sanitization/references.test.ts

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -132,15 +132,22 @@ describe('splitOutsideReferences', () => {
132132
expect(elapsedMs).toBeLessThan(1000)
133133
})
134134

135-
it('does not yet protect a comma inside a reference that nests an env-var placeholder', () => {
136-
// Known limitation, unchanged from the plain `.split(',')` this replaced: the tokenizer
137-
// reports the inner `{{A}}` and suppresses the outer `<...>` span, so the comma reads as a
138-
// separator. Characterized rather than fixed - the suppression lives in the shared
139-
// `@sim/utils/workflow-references` tokenizer.
135+
it('protects a comma inside a reference that nests an env-var placeholder', () => {
136+
// `findWorkflowReferenceTokens` is non-overlapping, so it reports only the inner `{{A}}` and
137+
// drops the outer candidate. The candidate pass is what keeps the outer region protected.
140138
expect(splitOutsideReferences('<start.body.pick({{A}},b)>,kb_literal')).toEqual([
141-
'<start.body.pick({{A}}',
142-
'b)>',
139+
'<start.body.pick({{A}},b)>',
143140
'kb_literal',
144141
])
142+
expect(splitOutsideReferences('<a.{{B}}x,y>,kb_literal')).toEqual([
143+
'<a.{{B}}x,y>',
144+
'kb_literal',
145+
])
146+
})
147+
148+
it('still splits a near-miss that does not read as a reference', () => {
149+
// `<a.b+c,d>` fails `isLikelyReferenceSegment` (the `+`), so it is not a protected region.
150+
// Loud rather than silent: the fragments are reported as ids that do not resolve.
151+
expect(splitOutsideReferences('<a.b+c,d>')).toEqual(['<a.b+c', 'd>'])
145152
})
146153
})

apps/sim/lib/workflows/sanitization/references.ts

Lines changed: 50 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,32 +23,78 @@ export function containsReference(value: unknown): boolean {
2323
return findWorkflowReferenceTokens(value).length > 0
2424
}
2525

26+
/**
27+
* Mark every `<...>` region that reads as a workflow reference, including one the tokenizer
28+
* dropped for overlapping an `{{ENV_VAR}}` token.
29+
*
30+
* `findWorkflowReferenceTokens` is contractually NON-overlapping, so for `<a.pick({{B}},c)>` it
31+
* reports only the inner `{{B}}` and discards the outer candidate. That is right for a tokenizer
32+
* and wrong for a splitter, which needs the UNION of protected regions rather than a disjoint set.
33+
*/
34+
function markCandidateReferenceRegions(value: string, insideReference: Uint8Array): void {
35+
let candidateStart = -1
36+
for (let index = 0; index < value.length; index += 1) {
37+
const character = value[index]
38+
if (character === REFERENCE.START && candidateStart === -1) {
39+
candidateStart = index
40+
continue
41+
}
42+
if (character === '\r' || character === '\n') {
43+
candidateStart = -1
44+
continue
45+
}
46+
if (character !== REFERENCE.END || candidateStart === -1) continue
47+
48+
const candidate = value.slice(candidateStart, index + REFERENCE.END.length)
49+
const split = splitReferenceSegment(candidate)
50+
if (split && isLikelyReferenceSegment(candidate)) {
51+
const start = candidateStart + split.leading.length
52+
const end = Math.min(start + split.reference.length, value.length)
53+
for (let position = start; position < end; position += 1) {
54+
insideReference[position] = 1
55+
}
56+
}
57+
candidateStart = -1
58+
}
59+
}
60+
2661
/**
2762
* Split a comma-separated multi-select value without tearing a reference apart.
2863
*
2964
* A `<block.path>` or `{{ENV_VAR}}` token may legitimately contain a comma (`<start.pick(a,b)>`),
3065
* and its fragments read as plain literals once split, so a naive `.split(',')` turns one dynamic
31-
* value into several bogus ones. Only commas outside every reference token are separators.
66+
* value into several bogus ones. Only commas outside every reference region are separators.
3267
*/
3368
export function splitOutsideReferences(value: string): string[] {
34-
const tokens = findWorkflowReferenceTokens(value)
35-
if (tokens.length === 0) {
36-
return value
69+
const plainSplit = () =>
70+
value
3771
.split(',')
3872
.map((part) => part.trim())
3973
.filter(Boolean)
74+
75+
if (!value.includes(REFERENCE.START) && !value.includes(REFERENCE.ENV_VAR_START)) {
76+
return plainSplit()
4077
}
4178

4279
// Marked once up front rather than scanned per comma: a per-comma `tokens.some()` is
4380
// O(commas x tokens), which reached ~2.5s on a 240KB value of repeated `{{A}},`.
81+
const tokens = findWorkflowReferenceTokens(value)
4482
const insideReference = new Uint8Array(value.length)
83+
let hasEnvironmentToken = false
4584
for (const token of tokens) {
85+
if (token.kind === 'environment') hasEnvironmentToken = true
4686
const end = Math.min(token.end, value.length)
4787
for (let index = Math.max(token.start, 0); index < end; index += 1) {
4888
insideReference[index] = 1
4989
}
5090
}
5191

92+
// Overlap with an environment token is the only reason the tokenizer drops a workflow
93+
// candidate, so without one it already reported every region and re-scanning is duplicate work.
94+
if (hasEnvironmentToken) {
95+
markCandidateReferenceRegions(value, insideReference)
96+
}
97+
5298
const parts: string[] = []
5399
let partStart = 0
54100
for (let index = 0; index < value.length; index += 1) {

0 commit comments

Comments
 (0)