Add Workflow name and instance id validations (#10785)

Author: pombosilva

.changeset/stupid-eggs-enjoy.md

@@ -0,0 +1,6 @@
+---
+"@cloudflare/workflows-shared": patch
+"wrangler": patch
+---
+
+Workflows names and instance IDs are now properly validated with production limits.

packages/create-cloudflare/templates/hello-world-workflows/js/wrangler.jsonc

@@ -9,7 +9,7 @@
 	},
 	"workflows": [
 		{
-			"name": "<TBD>",
+			"name": "workflows-hello-world",
 			"binding": "MY_WORKFLOW",
 			"class_name": "MyWorkflow",
 		},

packages/miniflare/test/plugins/workflows/index.spec.ts

@@ -15,7 +15,7 @@ export class MyWorkflow extends WorkflowEntrypoint {
   }
   export default {
 	async fetch(request, env, ctx) {
-		const workflow = await env.MY_WORKFLOW.create({id: "i'm an id"})
+		const workflow = await env.MY_WORKFLOW.create({id: "an-id"})
 
 		return new Response(JSON.stringify(await workflow.status()))
 	},

packages/workflows-shared/src/binding.ts

@@ -1,5 +1,7 @@
 import { RpcTarget, WorkerEntrypoint } from "cloudflare:workers";
 import { InstanceEvent, instanceStatusName } from "./instance";
+import { WorkflowError } from "./lib/errors";
+import { isValidWorkflowInstanceId } from "./lib/validators";
 import type {
 	DatabaseInstance,
 	DatabaseVersion,
@@ -19,6 +21,10 @@ export class WorkflowBinding extends WorkerEntrypoint<Env> implements Workflow {
 		id = crypto.randomUUID(),
 		params = {},
 	}: WorkflowInstanceCreateOptions = {}): Promise<WorkflowInstance> {
+		if (!isValidWorkflowInstanceId(id)) {
+			throw new WorkflowError("Workflow instance has invalid id");
+		}
+
 		const stubId = this.env.ENGINE.idFromName(id);
 		const stub = this.env.ENGINE.get(stubId);
 

packages/workflows-shared/src/context.ts

@@ -8,7 +8,7 @@ import {
 	WorkflowTimeoutError,
 } from "./lib/errors";
 import { calcRetryDuration } from "./lib/retries";
-import { MAX_STEP_NAME_LENGTH, validateStepName } from "./lib/validators";
+import { isValidStepName, MAX_STEP_NAME_LENGTH } from "./lib/validators";
 import type { Engine } from "./engine";
 import type { InstanceMetadata } from "./instance";
 import type {
@@ -85,7 +85,7 @@ export class Context extends RpcTarget {
 			stepConfig = {};
 		}
 
-		if (!validateStepName(name)) {
+		if (!isValidStepName(name)) {
 			// NOTE(lduarte): marking errors as user error allows the observability layer to avoid leaking
 			// user errors to sentry while making everything more observable. `isUserError` is not serialized
 			// into userland code due to how workerd serialzises errors over RPC - we also set it as undefined

packages/workflows-shared/src/lib/errors.ts

@@ -16,3 +16,7 @@ export class WorkflowFatalError extends Error {
 		};
 	}
 }
+
+export class WorkflowError extends Error {
+	name = "WorkflowError";
+}

packages/workflows-shared/src/lib/validators.ts

@@ -1,12 +1,45 @@
+export const MAX_WORKFLOW_NAME_LENGTH = 64;
+
+export const MAX_WORKFLOW_INSTANCE_ID_LENGTH = 100;
+
 export const MAX_STEP_NAME_LENGTH = 256;
+
+export const ALLOWED_STRING_ID_PATTERN = "^[a-zA-Z0-9_][a-zA-Z0-9-_]*$";
+const ALLOWED_WORKFLOW_INSTANCE_ID_REGEX = new RegExp(
+	ALLOWED_STRING_ID_PATTERN
+);
+const ALLOWED_WORKFLOW_NAME_REGEX = ALLOWED_WORKFLOW_INSTANCE_ID_REGEX;
+
 // eslint-disable-next-line no-control-regex
 const CONTROL_CHAR_REGEX = new RegExp("[\x00-\x1F]");
 
-export function validateStepName(string: string): boolean {
-	if (string.length > MAX_STEP_NAME_LENGTH) {
+export function isValidWorkflowName(name: string): boolean {
+	if (typeof name !== "string") {
+		return false;
+	}
+	if (name.length > MAX_WORKFLOW_NAME_LENGTH) {
+		return false;
+	}
+
+	return ALLOWED_WORKFLOW_NAME_REGEX.test(name);
+}
+
+export function isValidWorkflowInstanceId(id: string): boolean {
+	if (typeof id !== "string") {
+		return false;
+	}
+
+	if (id.length > MAX_WORKFLOW_INSTANCE_ID_LENGTH) {
+		return false;
+	}
+
+	return ALLOWED_WORKFLOW_INSTANCE_ID_REGEX.test(id);
+}
+
+export function isValidStepName(name: string): boolean {
+	if (name.length > MAX_STEP_NAME_LENGTH) {
 		return false;
 	}
 
-	//check for control chars
-	return !CONTROL_CHAR_REGEX.test(string);
+	return !CONTROL_CHAR_REGEX.test(name);
 }

packages/workflows-shared/tests/validators.test.ts

@@ -0,0 +1,75 @@
+import { describe, expect, it } from "vitest";
+import {
+	isValidStepName,
+	isValidWorkflowInstanceId,
+	isValidWorkflowName,
+	MAX_STEP_NAME_LENGTH,
+	MAX_WORKFLOW_INSTANCE_ID_LENGTH,
+	MAX_WORKFLOW_NAME_LENGTH,
+} from "../src/lib/validators";
+
+describe("Workflow name validation", () => {
+	it.each([
+		"",
+		NaN,
+		undefined,
+		"   ",
+		"\n\nhello",
+		"w".repeat(MAX_WORKFLOW_NAME_LENGTH + 1),
+		"#1231231!!!!",
+		"-badName",
+	])("should reject invalid names", function (value) {
+		expect(isValidWorkflowName(value as string)).toBe(false);
+	});
+
+	it.each([
+		"abc",
+		"NAME_123-hello",
+		"a-valid-string",
+		"w".repeat(MAX_WORKFLOW_NAME_LENGTH),
+	])("should accept valid names", function (value) {
+		expect(isValidWorkflowName(value as string)).toBe(true);
+	});
+});
+
+describe("Workflow instance ID validation", () => {
+	it.each([
+		"",
+		NaN,
+		undefined,
+		"   ",
+		"\n\nhello",
+		"w".repeat(MAX_WORKFLOW_INSTANCE_ID_LENGTH + 1),
+		"#1231231!!!!",
+	])("should reject invalid IDs", function (value) {
+		expect(isValidWorkflowInstanceId(value as string)).toBe(false);
+	});
+
+	it.each([
+		"abc",
+		"NAME_123-hello",
+		"a-valid-string",
+		"w".repeat(MAX_WORKFLOW_INSTANCE_ID_LENGTH),
+	])("should accept valid IDs", function (value) {
+		expect(isValidWorkflowInstanceId(value as string)).toBe(true);
+	});
+});
+
+describe("Workflow instance step name validation", () => {
+	it.each(["\x00", "w".repeat(MAX_STEP_NAME_LENGTH + 1)])(
+		"should reject invalid names",
+		function (value) {
+			expect(isValidStepName(value as string)).toBe(false);
+		}
+	);
+
+	it.each([
+		"abc",
+		"NAME_123-hello",
+		"a-valid-string",
+		"w".repeat(MAX_STEP_NAME_LENGTH),
+		"valid step name",
+	])("should accept valid names", function (value) {
+		expect(isValidStepName(value as string)).toBe(true);
+	});
+});

packages/wrangler/package.json

@@ -85,6 +85,7 @@
 		"@cloudflare/workers-shared": "workspace:*",
 		"@cloudflare/workers-tsconfig": "workspace:*",
 		"@cloudflare/workers-types": "catalog:default",
+		"@cloudflare/workflows-shared": "workspace:*",
 		"@cspotcode/source-map-support": "0.8.1",
 		"@iarna/toml": "^3.0.0",
 		"@sentry/node": "^7.86.0",

packages/wrangler/src/__tests__/workflows.test.ts

@@ -701,10 +701,35 @@ describe("wrangler workflows", () => {
 			});
 
 			await expect(runWrangler("deploy --dry-run")).rejects.toThrow();
-			expect(std.err).toContain("must be 64 characters or less");
-			expect(std.err).toContain("but got 65 characters");
+			expect(std.err).toMatchInlineSnapshot(`
+				"X [ERROR] Processing wrangler.toml configuration:
+
+				    - \\"workflows[0]\\" binding \\"name\\" field is invalid. Workflow names must be 1-64 characters long,
+				  start with a letter, number, or underscore, and may only contain letters, numbers, underscores, or
+				  hyphens.
+
+				"
+			`);
 		});
 
+		it.each(["", "   ", "\n\nhello", "#1231231!!!!", "-badName"])(
+			"should reject workflow binding with name with invalid characters",
+			async function (invalidName) {
+				writeWranglerConfig({
+					workflows: [
+						{
+							binding: "MY_WORKFLOW",
+							name: invalidName,
+							class_name: "MyWorkflow",
+						},
+					],
+				});
+
+				await expect(runWrangler("deploy --dry-run")).rejects.toThrow();
+				expect(std.err).toContain('binding "name" field is invalid');
+			}
+		);
+
 		it("should accept workflow binding with name exactly 64 characters", async () => {
 			const maxLengthName = "a".repeat(64); // exactly 64 characters
 			writeWorkerSource({ format: "ts" });