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" });