From 3b48e9d132cee226841878e7a7b8926227e01740 Mon Sep 17 00:00:00 2001 From: Neal006 Date: Sat, 8 Aug 2026 13:41:43 +0530 Subject: [PATCH 1/3] [workers-utils] Stop container key validation from crashing on malformed input Validating a `containers` entry pushed a type error into diagnostics and then dereferenced the value it had just rejected. An `authorized_keys` or `trusted_user_ca_keys` entry whose `public_key` was missing or was not a string reached `key.public_key.toLowerCase()` and threw, and an entry that was not an object threw earlier still, from the `in` operator inside `hasProperty`. A `configuration` of `null` passed the `typeof !== "object"` check and threw later from `Object.keys()`. None of these are a `UserError`, so wrangler printed a stack trace and asked the user to report a bug instead of naming the offending key. Each check now gates the checks that depend on it. The two duplicated key blocks become one shared `validateSshPublicKeys` helper, which also rejects a non-object entry up front, and the `configuration` type check gains the missing `null` case so `Object.keys` only sees a real object. No existing error message changed. --- .changeset/container-key-validation-crash.md | 9 ++ .../workers-utils/src/config/validation.ts | 112 +++++++------ .../normalize-and-validate-config.test.ts | 153 ++++++++++++++++++ 3 files changed, 223 insertions(+), 51 deletions(-) create mode 100644 .changeset/container-key-validation-crash.md diff --git a/.changeset/container-key-validation-crash.md b/.changeset/container-key-validation-crash.md new file mode 100644 index 00000000000..3af2a17135a --- /dev/null +++ b/.changeset/container-key-validation-crash.md @@ -0,0 +1,9 @@ +--- +"@cloudflare/workers-utils": patch +--- + +Report malformed container SSH keys and a null `containers.configuration` as config errors instead of crashing + +Validation of a `containers` entry recorded a type error and then dereferenced the value it had just rejected. An `authorized_keys` or `trusted_user_ca_keys` entry whose `public_key` was missing or was not a string reached `key.public_key.toLowerCase()` and threw `TypeError: Cannot read properties of undefined (reading 'toLowerCase')`, and an entry that was not an object at all threw from the `in` operator inside `hasProperty`. Separately, `configuration: null` passed the `typeof value !== "object"` check, because `typeof null === "object"`, and later threw from `Object.keys()`. Because none of these are a `UserError`, wrangler printed a stack trace and asked the user to file a bug rather than naming the offending key. + +Each check now gates the ones that depend on it, so all of these produce an ordinary diagnostic such as `containers.authorized_keys[0].public_key must be a string`. The two duplicated key-validation blocks are now a single shared helper, and no existing error message changed. diff --git a/packages/workers-utils/src/config/validation.ts b/packages/workers-utils/src/config/validation.ts index 9d93794119d..8e6eb905000 100644 --- a/packages/workers-utils/src/config/validation.ts +++ b/packages/workers-utils/src/config/validation.ts @@ -3366,6 +3366,49 @@ const validateBindingArray = return isValid; }; +/** + * Validate a list of SSH public key entries, used by both `containers.authorized_keys` + * and `containers.trusted_user_ca_keys`. Each check gates the next one, so a malformed + * entry is reported as a configuration error rather than dereferenced. + */ +function validateSshPublicKeys( + diagnostics: Diagnostics, + field: string, + value: unknown, + nameRequired: boolean +): void { + if (!Array.isArray(value)) { + diagnostics.errors.push(`${field} must be an array`); + return; + } + + for (const [index, key] of value.entries()) { + const fieldPath = `${field}[${index}]`; + + if (typeof key !== "object" || key === null || Array.isArray(key)) { + diagnostics.errors.push(`${fieldPath} must be an object`); + continue; + } + + const hasValidName = nameRequired + ? isRequiredProperty(key, "name", "string") + : isOptionalProperty(key, "name", "string"); + if (!hasValidName) { + diagnostics.errors.push(`${fieldPath}.name must be a string`); + } + + if ( + !isRequiredProperty<{ public_key: string }>(key, "public_key", "string") + ) { + diagnostics.errors.push(`${fieldPath}.public_key must be a string`); + } else if (!key.public_key.toLowerCase().startsWith("ssh-ed25519")) { + diagnostics.errors.push( + `${fieldPath}.public_key is a unsupported key type. Please provide a ED25519 public key.` + ); + } + } +} + function validateContainerApp( envName: string, topLevelName: string | undefined, @@ -3440,6 +3483,7 @@ function validateContainerApp( ); if ( typeof containerAppOptional.configuration !== "object" || + containerAppOptional.configuration === null || Array.isArray(containerAppOptional.configuration) ) { diagnostics.errors.push( @@ -3674,7 +3718,11 @@ function validateContainerApp( "unsafe", ] ); - if ("configuration" in containerAppOptional) { + if ( + typeof containerAppOptional.configuration === "object" && + containerAppOptional.configuration !== null && + !Array.isArray(containerAppOptional.configuration) + ) { validateAdditionalProperties( diagnostics, `${field}.configuration`, @@ -3722,59 +3770,21 @@ function validateContainerApp( } if ("authorized_keys" in containerAppOptional) { - if (!Array.isArray(containerAppOptional.authorized_keys)) { - diagnostics.errors.push(`${field}.authorized_keys must be an array`); - } else { - for (const index in containerAppOptional.authorized_keys) { - const fieldPath = `${field}.authorized_keys[${index}]`; - const key = containerAppOptional.authorized_keys[index]; - - if (!isRequiredProperty(key, "name", "string")) { - diagnostics.errors.push(`${fieldPath}.name must be a string`); - } - - if (!isRequiredProperty(key, "public_key", "string")) { - diagnostics.errors.push( - `${fieldPath}.public_key must be a string` - ); - } - - if (!key.public_key.toLowerCase().startsWith("ssh-ed25519")) { - diagnostics.errors.push( - `${fieldPath}.public_key is a unsupported key type. Please provide a ED25519 public key.` - ); - } - } - } + validateSshPublicKeys( + diagnostics, + `${field}.authorized_keys`, + containerAppOptional.authorized_keys, + true + ); } if ("trusted_user_ca_keys" in containerAppOptional) { - if (!Array.isArray(containerAppOptional.trusted_user_ca_keys)) { - diagnostics.errors.push( - `${field}.trusted_user_ca_keys must be an array` - ); - } else { - for (const index in containerAppOptional.trusted_user_ca_keys) { - const fieldPath = `${field}.trusted_user_ca_keys[${index}]`; - const key = containerAppOptional.trusted_user_ca_keys[index]; - - if (!isOptionalProperty(key, "name", "string")) { - diagnostics.errors.push(`${fieldPath}.name must be a string`); - } - - if (!isRequiredProperty(key, "public_key", "string")) { - diagnostics.errors.push( - `${fieldPath}.public_key must be a string` - ); - } - - if (!key.public_key.toLowerCase().startsWith("ssh-ed25519")) { - diagnostics.errors.push( - `${fieldPath}.public_key is a unsupported key type. Please provide a ED25519 public key.` - ); - } - } - } + validateSshPublicKeys( + diagnostics, + `${field}.trusted_user_ca_keys`, + containerAppOptional.trusted_user_ca_keys, + false + ); } if ( diff --git a/packages/workers-utils/tests/config/validation/normalize-and-validate-config.test.ts b/packages/workers-utils/tests/config/validation/normalize-and-validate-config.test.ts index faabe5400e4..0203ff27de9 100644 --- a/packages/workers-utils/tests/config/validation/normalize-and-validate-config.test.ts +++ b/packages/workers-utils/tests/config/validation/normalize-and-validate-config.test.ts @@ -3956,6 +3956,159 @@ describe("normalizeAndValidateConfig()", () => { `); }); + it("should error if containers.configuration is null", ({ expect }) => { + const { diagnostics } = normalizeAndValidateConfig( + { + name: "test-worker", + containers: [ + { + class_name: "test-class", + image: "registry.cloudflare.com/test:latest", + configuration: null, + }, + ], + } as unknown as RawConfig, + undefined, + undefined, + { env: undefined } + ); + + expect(diagnostics.renderErrors()).toMatchInlineSnapshot(` + "Processing wrangler configuration: + - "containers.configuration" should be an object" + `); + }); + + it("should error if an authorized_keys entry has no public_key", ({ + expect, + }) => { + const { diagnostics } = normalizeAndValidateConfig( + { + name: "test-worker", + containers: [ + { + class_name: "test-class", + image: "registry.cloudflare.com/test:latest", + authorized_keys: [{ name: "laptop" }], + }, + ], + } as unknown as RawConfig, + undefined, + undefined, + { env: undefined } + ); + + expect(diagnostics.renderErrors()).toMatchInlineSnapshot(` + "Processing wrangler configuration: + - containers.authorized_keys[0].public_key must be a string" + `); + }); + + it("should error if an authorized_keys entry is not an object", ({ + expect, + }) => { + const { diagnostics } = normalizeAndValidateConfig( + { + name: "test-worker", + containers: [ + { + class_name: "test-class", + image: "registry.cloudflare.com/test:latest", + authorized_keys: ["ssh-ed25519 AAAAC3NzaC1lZDI1NTE5"], + }, + ], + } as unknown as RawConfig, + undefined, + undefined, + { env: undefined } + ); + + expect(diagnostics.renderErrors()).toMatchInlineSnapshot(` + "Processing wrangler configuration: + - containers.authorized_keys[0] must be an object" + `); + }); + + it("should error if an authorized_keys public_key is not a string", ({ + expect, + }) => { + const { diagnostics } = normalizeAndValidateConfig( + { + name: "test-worker", + containers: [ + { + class_name: "test-class", + image: "registry.cloudflare.com/test:latest", + authorized_keys: [{ name: "laptop", public_key: 42 }], + }, + ], + } as unknown as RawConfig, + undefined, + undefined, + { env: undefined } + ); + + expect(diagnostics.renderErrors()).toMatchInlineSnapshot(` + "Processing wrangler configuration: + - containers.authorized_keys[0].public_key must be a string" + `); + }); + + it("should error if a trusted_user_ca_keys entry has no public_key", ({ + expect, + }) => { + const { diagnostics } = normalizeAndValidateConfig( + { + name: "test-worker", + containers: [ + { + class_name: "test-class", + image: "registry.cloudflare.com/test:latest", + trusted_user_ca_keys: [{ name: "ca" }], + }, + ], + } as unknown as RawConfig, + undefined, + undefined, + { env: undefined } + ); + + expect(diagnostics.renderErrors()).toMatchInlineSnapshot(` + "Processing wrangler configuration: + - containers.trusted_user_ca_keys[0].public_key must be a string" + `); + }); + + it("should accept valid authorized_keys and trusted_user_ca_keys", ({ + expect, + }) => { + const { diagnostics } = normalizeAndValidateConfig( + { + name: "test-worker", + containers: [ + { + class_name: "test-class", + image: "registry.cloudflare.com/test:latest", + authorized_keys: [ + { + name: "laptop", + public_key: "ssh-ed25519 AAAAC3NzaC1lZDI1", + }, + ], + trusted_user_ca_keys: [ + { public_key: "ssh-ed25519 AAAAC3NzaC1lZDI1" }, + ], + }, + ], + } as unknown as RawConfig, + undefined, + undefined, + { env: undefined } + ); + + expect(diagnostics.hasErrors()).toBe(false); + }); + it.for([{ value: 25 }, { value: [20, 50, 100] }])( "should accept rollout_step_percentage set to $value", (value, { expect }) => { From cdb7dcdde0a4f08ba5ea5d91019b19f6245e68d1 Mon Sep 17 00:00:00 2001 From: Neal006 Date: Sat, 8 Aug 2026 13:58:26 +0530 Subject: [PATCH 2/3] [workers-utils] Gate the instance_type limits check on a valid configuration Review caught that the `configuration` type check reported a non-object and then fell through to the limits-versus-instance_type check, which dereferences `configuration.disk`, `.vcpu` and `.memory_mib`. A config pairing `configuration: null` with `instance_type` therefore still threw `TypeError: Cannot read properties of null (reading 'disk')`, which is the exact class of crash this branch removes. The limits check is now an `else` branch of the type check, so it only runs once `configuration` is known to be a real object. Also correct the article agreement in the unsupported key type message, which read "a unsupported" and "a ED25519". No test or snapshot covered that string, so a case for it is added alongside the regression test pairing a null configuration with an instance type. --- .changeset/container-key-validation-crash.md | 6 +-- .../workers-utils/src/config/validation.ts | 6 +-- .../normalize-and-validate-config.test.ts | 53 +++++++++++++++++++ 3 files changed, 58 insertions(+), 7 deletions(-) diff --git a/.changeset/container-key-validation-crash.md b/.changeset/container-key-validation-crash.md index 3af2a17135a..f47673ea83e 100644 --- a/.changeset/container-key-validation-crash.md +++ b/.changeset/container-key-validation-crash.md @@ -2,8 +2,8 @@ "@cloudflare/workers-utils": patch --- -Report malformed container SSH keys and a null `containers.configuration` as config errors instead of crashing +Report malformed container SSH keys and a non-object `containers.configuration` as config errors instead of crashing -Validation of a `containers` entry recorded a type error and then dereferenced the value it had just rejected. An `authorized_keys` or `trusted_user_ca_keys` entry whose `public_key` was missing or was not a string reached `key.public_key.toLowerCase()` and threw `TypeError: Cannot read properties of undefined (reading 'toLowerCase')`, and an entry that was not an object at all threw from the `in` operator inside `hasProperty`. Separately, `configuration: null` passed the `typeof value !== "object"` check, because `typeof null === "object"`, and later threw from `Object.keys()`. Because none of these are a `UserError`, wrangler printed a stack trace and asked the user to file a bug rather than naming the offending key. +Previously, a `containers` entry with a malformed `authorized_keys` or `trusted_user_ca_keys` entry (a missing or non-string `public_key`, or an entry that is not an object), or a `containers.configuration` set to `null`, made Wrangler exit with a stack trace and "If you think this is a bug, please open an issue" rather than pointing at the field. -Each check now gates the ones that depend on it, so all of these produce an ordinary diagnostic such as `containers.authorized_keys[0].public_key must be a string`. The two duplicated key-validation blocks are now a single shared helper, and no existing error message changed. +These configurations now produce an ordinary configuration error naming the offending field and array index, such as `containers.authorized_keys[0].public_key must be a string`. A `public_key` that is not an ED25519 key is also now reported with correct grammar. diff --git a/packages/workers-utils/src/config/validation.ts b/packages/workers-utils/src/config/validation.ts index 8e6eb905000..711524ac4c8 100644 --- a/packages/workers-utils/src/config/validation.ts +++ b/packages/workers-utils/src/config/validation.ts @@ -3403,7 +3403,7 @@ function validateSshPublicKeys( diagnostics.errors.push(`${fieldPath}.public_key must be a string`); } else if (!key.public_key.toLowerCase().startsWith("ssh-ed25519")) { diagnostics.errors.push( - `${fieldPath}.public_key is a unsupported key type. Please provide a ED25519 public key.` + `${fieldPath}.public_key is an unsupported key type. Please provide an ED25519 public key.` ); } } @@ -3489,9 +3489,7 @@ function validateContainerApp( diagnostics.errors.push( `"containers.configuration" should be an object` ); - } - - if ( + } else if ( containerAppOptional.instance_type && (containerAppOptional.configuration.disk !== undefined || containerAppOptional.configuration.vcpu !== undefined || diff --git a/packages/workers-utils/tests/config/validation/normalize-and-validate-config.test.ts b/packages/workers-utils/tests/config/validation/normalize-and-validate-config.test.ts index 0203ff27de9..98f93ec46a3 100644 --- a/packages/workers-utils/tests/config/validation/normalize-and-validate-config.test.ts +++ b/packages/workers-utils/tests/config/validation/normalize-and-validate-config.test.ts @@ -4079,6 +4079,59 @@ describe("normalizeAndValidateConfig()", () => { `); }); + it("should error if containers.configuration is null and instance_type is set", ({ + expect, + }) => { + const { diagnostics } = normalizeAndValidateConfig( + { + name: "test-worker", + containers: [ + { + class_name: "test-class", + image: "registry.cloudflare.com/test:latest", + configuration: null, + instance_type: "lite", + }, + ], + } as unknown as RawConfig, + undefined, + undefined, + { env: undefined } + ); + + expect(diagnostics.renderErrors()).toMatchInlineSnapshot(` + "Processing wrangler configuration: + - "containers.configuration" should be an object" + `); + }); + + it("should error if an authorized_keys public_key is not an ED25519 key", ({ + expect, + }) => { + const { diagnostics } = normalizeAndValidateConfig( + { + name: "test-worker", + containers: [ + { + class_name: "test-class", + image: "registry.cloudflare.com/test:latest", + authorized_keys: [ + { name: "laptop", public_key: "ssh-rsa AAAAB3NzaC1yc2E" }, + ], + }, + ], + } as unknown as RawConfig, + undefined, + undefined, + { env: undefined } + ); + + expect(diagnostics.renderErrors()).toMatchInlineSnapshot(` + "Processing wrangler configuration: + - containers.authorized_keys[0].public_key is an unsupported key type. Please provide an ED25519 public key." + `); + }); + it("should accept valid authorized_keys and trusted_user_ca_keys", ({ expect, }) => { From adb618bc79173b008adf8ea4a7ba9b4474741754 Mon Sep 17 00:00:00 2001 From: Neal006 Date: Wed, 12 Aug 2026 02:13:48 +0530 Subject: [PATCH 3/3] [workers-utils] Group the container configuration tests with the other containers type checks The two tests covering a null `containers.configuration`, one on its own and one paired with an `instance_type`, sat further down among the SSH key cases, which is where they happened to be written rather than where they belong. Move both up next to the checks that assert on the shape of the `containers` field, so every type error for the block reads in one place. No test bodies or assertions change. --- .../normalize-and-validate-config.test.ts | 98 +++++++++---------- 1 file changed, 49 insertions(+), 49 deletions(-) diff --git a/packages/workers-utils/tests/config/validation/normalize-and-validate-config.test.ts b/packages/workers-utils/tests/config/validation/normalize-and-validate-config.test.ts index 98f93ec46a3..7e072bba514 100644 --- a/packages/workers-utils/tests/config/validation/normalize-and-validate-config.test.ts +++ b/packages/workers-utils/tests/config/validation/normalize-and-validate-config.test.ts @@ -3664,6 +3664,55 @@ describe("normalizeAndValidateConfig()", () => { `); }); + it("should error if containers.configuration is null", ({ expect }) => { + const { diagnostics } = normalizeAndValidateConfig( + { + name: "test-worker", + containers: [ + { + class_name: "test-class", + image: "registry.cloudflare.com/test:latest", + configuration: null, + }, + ], + } as unknown as RawConfig, + undefined, + undefined, + { env: undefined } + ); + + expect(diagnostics.renderErrors()).toMatchInlineSnapshot(` + "Processing wrangler configuration: + - "containers.configuration" should be an object" + `); + }); + + it("should error if containers.configuration is null and instance_type is set", ({ + expect, + }) => { + const { diagnostics } = normalizeAndValidateConfig( + { + name: "test-worker", + containers: [ + { + class_name: "test-class", + image: "registry.cloudflare.com/test:latest", + configuration: null, + instance_type: "lite", + }, + ], + } as unknown as RawConfig, + undefined, + undefined, + { env: undefined } + ); + + expect(diagnostics.renderErrors()).toMatchInlineSnapshot(` + "Processing wrangler configuration: + - "containers.configuration" should be an object" + `); + }); + it("should error if no containers name and no worker name are provided", ({ expect, }) => { @@ -3956,29 +4005,6 @@ describe("normalizeAndValidateConfig()", () => { `); }); - it("should error if containers.configuration is null", ({ expect }) => { - const { diagnostics } = normalizeAndValidateConfig( - { - name: "test-worker", - containers: [ - { - class_name: "test-class", - image: "registry.cloudflare.com/test:latest", - configuration: null, - }, - ], - } as unknown as RawConfig, - undefined, - undefined, - { env: undefined } - ); - - expect(diagnostics.renderErrors()).toMatchInlineSnapshot(` - "Processing wrangler configuration: - - "containers.configuration" should be an object" - `); - }); - it("should error if an authorized_keys entry has no public_key", ({ expect, }) => { @@ -4079,32 +4105,6 @@ describe("normalizeAndValidateConfig()", () => { `); }); - it("should error if containers.configuration is null and instance_type is set", ({ - expect, - }) => { - const { diagnostics } = normalizeAndValidateConfig( - { - name: "test-worker", - containers: [ - { - class_name: "test-class", - image: "registry.cloudflare.com/test:latest", - configuration: null, - instance_type: "lite", - }, - ], - } as unknown as RawConfig, - undefined, - undefined, - { env: undefined } - ); - - expect(diagnostics.renderErrors()).toMatchInlineSnapshot(` - "Processing wrangler configuration: - - "containers.configuration" should be an object" - `); - }); - it("should error if an authorized_keys public_key is not an ED25519 key", ({ expect, }) => {