From 0f35b5895a4b67a84ea92b36185220ac96e6b66e Mon Sep 17 00:00:00 2001 From: blockgroot <170620375+blockgroot@users.noreply.github.com> Date: Tue, 4 Aug 2026 21:34:37 +0530 Subject: [PATCH] fix(workflow): reject if-else branches with a dangling filterGroupId (#23758) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary An If/Else workflow step branch whose `filterGroupId` doesn't resolve to any entry in `stepFilterGroups` was matching unconditionally — before any of the step's real conditions were evaluated — instead of being rejected. This let a single stale or mistyped `filterGroupId` silently hijack the routing of an entire If/Else step. Fixes #23754 ## Problem Reproduced with a standalone unit test against `findMatchingBranch`: ```ts const branches = [ { id: 'branch-A', filterGroupId: 'group-id-that-does-not-exist', nextStepIds: ['wrong-step'] }, { id: 'branch-B', filterGroupId: 'real-group', nextStepIds: ['correct-step'] }, ]; const stepFilterGroups = [{ id: 'real-group', logicalOperator: 'AND' }]; const resolvedFilters = [{ /* branch-B's real filter, evaluates to false */ }]; findMatchingBranch({ branches, stepFilterGroups, resolvedFilters }).id; // => 'branch-A' (its condition was never evaluated at all) ``` `branch-A` wins even though its `filterGroupId` doesn't exist and `branch-B`'s actual (non-matching) filter was correctly evaluated to `false`. ## Root cause `find-matching-branch.util.ts` builds `branchFilterGroups` via `collectAllDescendantGroups(branch.filterGroupId, stepFilterGroups)`, which silently returns an empty `Set` when the root id isn't found. The resulting empty `branchFilterGroups`/`branchFilters` are passed to `evaluateFilterConditions`, which treats "both empty" as vacuously `true` — a rule that's correct for the real trailing else-branch (no `filterGroupId` at all, by design) but indistinguishable, at this call site, from "the referenced group doesn't exist." Since `Array.prototype.find` returns the first match, this branch wins over any later branch whose condition was actually evaluated. There was also no validation path that would catch this before execution: `validateBranchingStep` (`validate-workflow-graph.util.ts`) already checks If/Else branch count and `nextStepIds` connectivity, but had no check for `filterGroupId` referential integrity. ## Fix 1. `find-matching-branch.util.ts` — throw `WorkflowStepExecutorException` (`INVALID_STEP_INPUT`) when a branch's `filterGroupId` doesn't resolve to any group, instead of silently falling through to `evaluateFilterConditions({filterGroups: [], filters: []})`. This mirrors the sibling guard clauses already in this action for other malformed-input cases. 2. `validate-workflow-graph.util.ts` — extended the existing `IF_ELSE` branch checks in `validateBranchingStep` with the same check, surfaced as a new `IF_ELSE_BRANCH_FILTER_GROUP_NOT_FOUND` issue code, so `validate_workflow` catches this before a workflow ever runs. **Alternative considered:** fixing only at validation time. Rejected — validation can be skipped (e.g. the AI workflow-editing tool's `validate: false` option) or bypassed entirely by a direct API write, so the execution-time guard is the actual fix; the validation check is defense in depth, not a substitute. **Alternative considered:** silently skipping the malformed branch instead of throwing. Rejected — throwing immediately gives a specific, actionable error pointing at the exact misconfiguration, matching this file's existing error granularity (distinct messages for "not an if-else step", "no branches", "missing filter groups/filters", "no matching branch"). ## Tests - `find-matching-branch.util.spec.ts` (new) — real-condition match, else-branch fallback match, throws on a dangling `filterGroupId` (fails on `main`, passes here), throws when no branch matches and there's no else branch. - `validate-workflow-graph.util.test.ts` (+2) — flags `IF_ELSE_BRANCH_FILTER_GROUP_NOT_FOUND` for a dangling reference; does not false-positive on a correctly-configured branch. - Full module suites: `npx nx test twenty-server` scoped to `src/modules/workflow` → 68 suites / 626 tests passed. `npx jest packages/twenty-shared/src/workflow` → 30 suites / 248 tests passed. - `npx nx lint twenty-server twenty-shared` and `npx nx typecheck twenty-server twenty-shared` → clean. ## Compatibility / risk Internal-only change to workflow execution and validation logic — no GraphQL schema change, no public API signature change, no migration. A workflow that today relies (accidentally) on the silent "dangling group = always match" behavior would start throwing at execution time, but that was never intentional or documented behavior. ## Out of scope - Branch **ordering** invariants (e.g. asserting the group-less else branch is always last) — not needed for this fix; the defect reproduces purely from a dangling `filterGroupId`, independent of order. Review in cubic --------- Co-authored-by: Thomas Trompette --- .../find-matching-branch.util.spec.ts | 120 ++++++++++++++++++ .../utils/find-matching-branch.util.ts | 12 ++ .../validate-workflow-graph.util.test.ts | 72 +++++++++++ .../utils/validate-workflow-graph.util.ts | 20 +++ 4 files changed, 224 insertions(+) create mode 100644 packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/__tests__/find-matching-branch.util.spec.ts diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/__tests__/find-matching-branch.util.spec.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/__tests__/find-matching-branch.util.spec.ts new file mode 100644 index 0000000000..b8d7c1c766 --- /dev/null +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/__tests__/find-matching-branch.util.spec.ts @@ -0,0 +1,120 @@ +import { StepLogicalOperator, type StepFilterGroup } from 'twenty-shared/types'; +import { type StepIfElseBranch } from 'twenty-shared/workflow'; + +import { + WorkflowStepExecutorException, + WorkflowStepExecutorExceptionCode, +} from 'src/modules/workflow/workflow-executor/exceptions/workflow-step-executor.exception'; +import { + findMatchingBranch, + type ResolvedFilter, +} from 'src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/find-matching-branch.util'; + +describe('findMatchingBranch', () => { + const realGroup: StepFilterGroup = { + id: 'real-group', + logicalOperator: StepLogicalOperator.AND, + }; + + const matchingFilter = ( + stepFilterGroupId: string, + matches: boolean, + ): ResolvedFilter => + ({ + id: 'filter-1', + type: 'TEXT', + operand: 'IS', + stepFilterGroupId, + rightOperand: 'expected-value', + leftOperand: matches ? 'expected-value' : 'something-else', + }) as ResolvedFilter; + + it('should return the branch whose filter condition evaluates to true', () => { + const branches: StepIfElseBranch[] = [ + { id: 'branch-a', filterGroupId: 'real-group', nextStepIds: [] }, + ]; + + const matched = findMatchingBranch({ + branches, + stepFilterGroups: [realGroup], + resolvedFilters: [matchingFilter('real-group', true)], + }); + + expect(matched.id).toBe('branch-a'); + }); + + it('should return the trailing else branch when no conditional branch matches', () => { + const branches: StepIfElseBranch[] = [ + { id: 'branch-a', filterGroupId: 'real-group', nextStepIds: [] }, + { id: 'branch-else', nextStepIds: [] }, + ]; + + const matched = findMatchingBranch({ + branches, + stepFilterGroups: [realGroup], + resolvedFilters: [matchingFilter('real-group', false)], + }); + + expect(matched.id).toBe('branch-else'); + }); + + it('should throw INVALID_STEP_INPUT instead of silently matching a branch whose filterGroupId does not resolve to any stepFilterGroup', () => { + const branches: StepIfElseBranch[] = [ + { + id: 'branch-dangling', + filterGroupId: 'group-id-not-in-stepFilterGroups', + nextStepIds: [], + }, + { id: 'branch-real', filterGroupId: 'real-group', nextStepIds: [] }, + ]; + + expect(() => + findMatchingBranch({ + branches, + stepFilterGroups: [realGroup], + resolvedFilters: [matchingFilter('real-group', false)], + }), + ).toThrow( + expect.objectContaining({ + code: WorkflowStepExecutorExceptionCode.INVALID_STEP_INPUT, + }), + ); + }); + + it('should throw for a dangling branch even when an earlier branch already matches', () => { + const branches: StepIfElseBranch[] = [ + { id: 'branch-match', filterGroupId: 'real-group', nextStepIds: [] }, + { + id: 'branch-dangling', + filterGroupId: 'group-id-not-in-stepFilterGroups', + nextStepIds: [], + }, + ]; + + expect(() => + findMatchingBranch({ + branches, + stepFilterGroups: [realGroup], + resolvedFilters: [matchingFilter('real-group', true)], + }), + ).toThrow( + expect.objectContaining({ + code: WorkflowStepExecutorExceptionCode.INVALID_STEP_INPUT, + }), + ); + }); + + it('should throw when no branch matches and there is no else branch', () => { + const branches: StepIfElseBranch[] = [ + { id: 'branch-a', filterGroupId: 'real-group', nextStepIds: [] }, + ]; + + expect(() => + findMatchingBranch({ + branches, + stepFilterGroups: [realGroup], + resolvedFilters: [matchingFilter('real-group', false)], + }), + ).toThrow(WorkflowStepExecutorException); + }); +}); diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/find-matching-branch.util.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/find-matching-branch.util.ts index 87f58616b9..fe0243a880 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/find-matching-branch.util.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/find-matching-branch.util.ts @@ -46,6 +46,18 @@ export const findMatchingBranch = ({ stepFilterGroups: StepFilterGroup[]; resolvedFilters: ResolvedFilter[]; }): StepIfElseBranch => { + for (const branch of branches) { + if ( + isDefined(branch.filterGroupId) && + !stepFilterGroups.some((group) => group.id === branch.filterGroupId) + ) { + throw new WorkflowStepExecutorException( + `Branch "${branch.id}" references filter group "${branch.filterGroupId}", which does not exist`, + WorkflowStepExecutorExceptionCode.INVALID_STEP_INPUT, + ); + } + } + const matchingBranch = branches.find((branch) => { if (!isDefined(branch.filterGroupId)) { return true; diff --git a/packages/twenty-shared/src/workflow/validation/utils/__tests__/validate-workflow-graph.util.test.ts b/packages/twenty-shared/src/workflow/validation/utils/__tests__/validate-workflow-graph.util.test.ts index b43a170c02..92a60135b4 100644 --- a/packages/twenty-shared/src/workflow/validation/utils/__tests__/validate-workflow-graph.util.test.ts +++ b/packages/twenty-shared/src/workflow/validation/utils/__tests__/validate-workflow-graph.util.test.ts @@ -102,6 +102,78 @@ describe('validateWorkflowGraph', () => { expect(getCodes(workflow)).toContain('IF_ELSE_BRANCH_HAS_NO_NEXT_STEP'); }); + it('should flag an if-else branch whose filterGroupId does not exist', () => { + const workflow: ValidatableWorkflow = { + trigger: { type: 'MANUAL', nextStepIds: ['if'] }, + steps: [ + { + id: 'if', + type: WorkflowActionType.IF_ELSE, + settings: { + input: { + stepFilterGroups: [{ id: 'real-group' }], + branches: [ + { nextStepIds: ['end'], filterGroupId: 'ghost-group' }, + { nextStepIds: ['end'] }, + ], + }, + }, + }, + { id: 'end', type: 'CODE' }, + ], + }; + + expect(getCodes(workflow)).toContain('INVALID_STEP_PARAMS'); + }); + + it('should not flag an if-else branch whose filterGroupId exists', () => { + const workflow: ValidatableWorkflow = { + trigger: { type: 'MANUAL', nextStepIds: ['if'] }, + steps: [ + { + id: 'if', + type: WorkflowActionType.IF_ELSE, + settings: { + input: { + stepFilterGroups: [{ id: 'real-group' }], + branches: [ + { nextStepIds: ['end'], filterGroupId: 'real-group' }, + { nextStepIds: ['end'] }, + ], + }, + }, + }, + { id: 'end', type: 'CODE' }, + ], + }; + + expect(getCodes(workflow)).not.toContain('INVALID_STEP_PARAMS'); + }); + + it('should not throw when an if-else step has a non-array stepFilterGroups', () => { + const workflow: ValidatableWorkflow = { + trigger: { type: 'MANUAL', nextStepIds: ['if'] }, + steps: [ + { + id: 'if', + type: WorkflowActionType.IF_ELSE, + settings: { + input: { + stepFilterGroups: 'not-an-array', + branches: [ + { nextStepIds: ['end'], filterGroupId: 'ghost-group' }, + { nextStepIds: ['end'] }, + ], + }, + }, + }, + { id: 'end', type: 'CODE' }, + ], + }; + + expect(() => getCodes(workflow)).not.toThrow(); + }); + it('should flag an iterator with items but no loop body', () => { const workflow: ValidatableWorkflow = { trigger: { type: 'MANUAL', nextStepIds: ['iterator'] }, diff --git a/packages/twenty-shared/src/workflow/validation/utils/validate-workflow-graph.util.ts b/packages/twenty-shared/src/workflow/validation/utils/validate-workflow-graph.util.ts index 29b411e326..6647fa9db7 100644 --- a/packages/twenty-shared/src/workflow/validation/utils/validate-workflow-graph.util.ts +++ b/packages/twenty-shared/src/workflow/validation/utils/validate-workflow-graph.util.ts @@ -106,6 +106,14 @@ const validateBranchingStep = ( }); } + const stepFilterGroups = (input as Partial | undefined) + ?.stepFilterGroups; + const stepFilterGroupIds = new Set( + (Array.isArray(stepFilterGroups) ? stepFilterGroups : []) + .filter(isDefined) + .map((filterGroup) => filterGroup.id), + ); + for (const branch of branches) { const branchNextStepIds = branch?.nextStepIds; @@ -117,6 +125,18 @@ const validateBranchingStep = ( stepId: step.id, }); } + + if ( + isDefined(branch?.filterGroupId) && + !stepFilterGroupIds.has(branch.filterGroupId) + ) { + issues.push({ + severity: 'error', + code: 'INVALID_STEP_PARAMS', + message: `A branch of If/Else step "${step.name ?? step.id}" references filter group "${branch.filterGroupId}", which does not exist.`, + stepId: step.id, + }); + } } }