From 829f4df48d39c6f7b39587318d58fc43c585d5f4 Mon Sep 17 00:00:00 2001 From: Jiho Lee Date: Sun, 26 Jul 2026 08:27:18 +0900 Subject: [PATCH] fix(workflows): validate YAML on import so misplaced keys aren't silently dropped (#6274) --- .../__tests__/parseWorkflowYamlToJSON.test.ts | 64 ++++++++++++++ .../workflow-import-validation.test.ts | 86 +++++++++++++++++++ .../workflow-builder-widget.tsx | 13 ++- 3 files changed, 161 insertions(+), 2 deletions(-) create mode 100644 keep-ui/widgets/workflow-builder/__tests__/workflow-import-validation.test.ts diff --git a/keep-ui/entities/workflows/lib/__tests__/parseWorkflowYamlToJSON.test.ts b/keep-ui/entities/workflows/lib/__tests__/parseWorkflowYamlToJSON.test.ts index e52cdd7e51..fd9e24dd43 100644 --- a/keep-ui/entities/workflows/lib/__tests__/parseWorkflowYamlToJSON.test.ts +++ b/keep-ui/entities/workflows/lib/__tests__/parseWorkflowYamlToJSON.test.ts @@ -420,4 +420,68 @@ describe("parseWorkflowYamlToJSON", () => { expect(result.error).toBeUndefined(); expect(result.success).toBe(true); }); + + // Regression for #6274: `with:` written as a sibling of `provider:` instead of + // nested inside it used to be accepted silently by the import path, producing a + // workflow that saved and fired with zero parameters. The schema is `.strict()`, + // so validating on import surfaces it instead. + it("should reject `with:` placed as a sibling of `provider:` instead of nested", () => { + const misplacedWithYaml = `workflow: + id: test-with-nesting + name: Test + description: Regression fixture for issue 6274 + triggers: + - type: alert + filters: + - key: source + value: prometheus + actions: + - name: test-ntfy + provider: + type: ntfy + config: default + with: + message: "test"`; + + const result = parseWorkflowYamlToJSON( + misplacedWithYaml, + workflowSchemaWithProviders + ); + + expect(result.success).toBe(false); + expect(result.error?.issues).toEqual( + expect.arrayContaining([ + expect.objectContaining({ code: "unrecognized_keys" }), + ]) + ); + }); + + it("should accept the same workflow once `with:` is nested under `provider:`", () => { + // Guard rail: the check above must reject only the misplacement, not the + // workflow shape itself. + const correctYaml = `workflow: + id: test-with-nesting + name: Test + description: Regression fixture for issue 6274 + triggers: + - type: alert + filters: + - key: source + value: prometheus + actions: + - name: test-ntfy + provider: + type: ntfy + config: default + with: + message: "test"`; + + const result = parseWorkflowYamlToJSON( + correctYaml, + workflowSchemaWithProviders + ); + + expect(result.success).toBe(true); + expect(result.error).toBeUndefined(); + }); }); diff --git a/keep-ui/widgets/workflow-builder/__tests__/workflow-import-validation.test.ts b/keep-ui/widgets/workflow-builder/__tests__/workflow-import-validation.test.ts new file mode 100644 index 0000000000..c4b497b60e --- /dev/null +++ b/keep-ui/widgets/workflow-builder/__tests__/workflow-import-validation.test.ts @@ -0,0 +1,86 @@ +import { parseWorkflowYamlToJSON } from "@/entities/workflows/lib/yaml-utils"; +import { fromZodError } from "zod-validation-error"; + +/** + * Regression for #6274. + * + * The YAML import path in `workflow-builder-widget.tsx` used to call + * `parseWorkflowYamlStringToJSON`, a bare `yaml.parse()` with no schema + * validation. A `with:` written as a sibling of `provider:` (rather than nested + * inside it) was therefore accepted, but every consumer reads + * `provider.with` — so the parameters were silently dropped and the workflow + * saved and fired with none of them. + * + * These tests pin the validation the import handler now performs. + */ + +// Mirrors the import handler in workflow-builder-widget.tsx +const validateImportedWorkflow = (contents: string): string | null => { + const result = parseWorkflowYamlToJSON(contents); + if (!result.success) { + return fromZodError(result.error).toString(); + } + return null; +}; + +const MISPLACED_WITH = `workflow: + id: test-with-nesting + name: Test + description: Regression fixture for issue 6274 + triggers: + - type: alert + filters: + - key: source + value: prometheus + actions: + - name: test-ntfy + provider: + type: ntfy + config: default + with: + message: "test"`; + +const NESTED_WITH = `workflow: + id: test-with-nesting + name: Test + description: Regression fixture for issue 6274 + triggers: + - type: alert + filters: + - key: source + value: prometheus + actions: + - name: test-ntfy + provider: + type: ntfy + config: default + with: + message: "test"`; + +describe("workflow YAML import validation", () => { + it("rejects a workflow whose `with:` is a sibling of `provider:`", () => { + const error = validateImportedWorkflow(MISPLACED_WITH); + + expect(error).not.toBeNull(); + // The message must name the offending key so the user can act on it. + expect(error).toContain("with"); + }); + + it("accepts the same workflow once `with:` is nested under `provider:`", () => { + // Guard rail: validation must reject only the misplacement, not the shape. + expect(validateImportedWorkflow(NESTED_WITH)).toBeNull(); + }); + + it("the misplaced form is exactly what every consumer would read as empty", () => { + // Shows why silently accepting it is harmful: `provider.with` — the path the + // backend parser and the UI both read — is undefined, while the parameters + // sit unreachable at step level. + const parsed: any = parseWorkflowYamlToJSON(MISPLACED_WITH); + expect(parsed.success).toBe(false); + + const raw: any = require("yaml").parse(MISPLACED_WITH); + const action = raw.workflow.actions[0]; + expect(action.provider.with).toBeUndefined(); + expect(action.with).toEqual({ message: "test" }); + }); +}); diff --git a/keep-ui/widgets/workflow-builder/workflow-builder-widget.tsx b/keep-ui/widgets/workflow-builder/workflow-builder-widget.tsx index b6e28a3293..03c55e9f2e 100644 --- a/keep-ui/widgets/workflow-builder/workflow-builder-widget.tsx +++ b/keep-ui/widgets/workflow-builder/workflow-builder-widget.tsx @@ -9,7 +9,8 @@ import { useWorkflowStore } from "@/entities/workflows"; import { WorkflowMetadataModal } from "@/features/workflows/edit-metadata"; import { WorkflowEnabledSwitch } from "@/features/workflows/enable-disable"; import { WorkflowSyncStatus } from "@/app/(keep)/workflows/[workflow_id]/workflow-sync-status"; -import { parseWorkflowYamlStringToJSON } from "@/entities/workflows/lib/yaml-utils"; +import { parseWorkflowYamlToJSON } from "@/entities/workflows/lib/yaml-utils"; +import { fromZodError } from "zod-validation-error"; import clsx from "clsx"; import { WorkflowTestRunButton } from "@/features/workflows/test-run/ui/workflow-test-run-button"; import { useUIBuilderUnsavedChanges } from "@/entities/workflows/model/workflow-store"; @@ -59,7 +60,15 @@ export function WorkflowBuilderWidget({ setFileName(fName); const contents = event.target!.result as string; try { - const _ = parseWorkflowYamlStringToJSON(contents); + // Validate against the workflow schema rather than doing a bare YAML + // parse. The schema is `.strict()`, so a misplaced key — most commonly + // `with:` written as a sibling of `provider:` instead of nested inside + // it — is reported here instead of being silently dropped and saved as + // a workflow that runs with no parameters. + const result = parseWorkflowYamlToJSON(contents); + if (!result.success) { + throw new Error(fromZodError(result.error).toString()); + } setFileContents(contents); } catch (error) { showErrorToast(error, "Failed to parse workflow");