fix(workflow): reject if-else branches with a dangling filterGroupId (#23758)

## 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.  <a href="https://cubic.dev/pr/twentyhq/twenty/pull/23758?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a>  --------- Co-authored-by: Thomas Trompette <thomas.trompette@sfr.fr>
2026-08-04 21:34:37 +05:30
parent 29aa6e85d6
commit 0f35b5895a
4 changed files with 224 additions and 0 deletions
@@ -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);
+  });
+});
@@ -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;
@@ -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'] },
@@ -106,6 +106,14 @@ const validateBranchingStep = (
      });
    }

+    const stepFilterGroups = (input as Partial<IfElseStepInput> | 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,
+        });
+      }
    }
  }