[workers-utils] Stop container key validation from crashing on malformed input - #15088
Conversation
🦋 Changeset detectedLatest commit: 7b2da6b The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This PR fixes @cloudflare/workers-utils config validation for containers so malformed authorized_keys, trusted_user_ca_keys, and configuration values produce diagnostics instead of throwing raw TypeErrors from normalizeAndValidateConfig() (improving Wrangler’s error handling for invalid configs).
Changes:
- Adds a shared
validateSshPublicKeys()helper to validatecontainers.authorized_keysandcontainers.trusted_user_ca_keyswith proper gating (no dereference after a failed type check). - Treats
containers.configuration: nullas invalid and avoids running additional-properties validation unless it’s a real object. - Adds unit tests covering several previously-crashing malformed
containersconfigurations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/workers-utils/src/config/validation.ts | Refactors SSH key validation into a helper and tightens containers.configuration validation/guarding. |
| packages/workers-utils/tests/config/validation/normalize-and-validate-config.test.ts | Adds regression tests ensuring malformed containers configs produce diagnostics instead of throwing. |
| .changeset/container-key-validation-crash.md | Adds a patch changeset documenting the crash-to-diagnostics behavior fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
dario-piotrowicz
left a comment
dario-piotrowicz
left a comment
There was a problem hiding this comment.
The PR generally looks good to me but there seem to be a few things that need fixing
Could you have a look @Neal006? 🙏
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
|
@dario-piotrowicz thanks for taking the time to go through this. All three points are now covered, and two of them were already handled in a follow-up commit that landed before your pass, so the older threads are pointing at code that has since changed. Summary in one place: 1. Test placement (your comment on 2. Grammar in the unsupported key type message. Already fixed in 3. Where the PR stands. Three root causes in Verification on the current head: Could you have another look when you get a moment, and let me know if anything else needs changing? Glad to make any further adjustments you would like. |
…med 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.
…uration 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.
…r 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.
e622478 to
adb618b
Compare
|
We recently landed #14994, a large change to Miniflare's configuration internals that touched ~177 files across the repo. Leaving this PR on its old base was likely to cause conflicts, so we've rebased it onto the latest Your local copy of this branch is now out of date. Before you push again, please reset to the new version: git fetch origin
git checkout fix/container-key-validation-crash
git reset --hard origin/fix/container-key-validation-crashBecause the base moved a long way, it's also worth reinstalling before you carry on — the lockfile changed: pnpm installSorry for the interruption. If the rebase looks wrong, or CI now fails in a way that seems related to the Miniflare config change rather than your own work, comment here and we'll help get it sorted. |
|
Hi @petebacondarwin, @dario-piotrowicz, thanks for the rebase onto the new base. I have reset my local branch to the force-pushed version and re-verified the change against it:
Nothing outstanding on my side. CI shows 13 workflow suites at |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
workers-devprod
left a comment
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
Fixes #15087.
Validating a
containersentry pushed a type error intodiagnosticsand then dereferenced the value it had just rejected. Five malformed configs therefore threw a rawTypeErrorout ofnormalizeAndValidateConfig. None of them is aUserError, sohandle-errors.tsprinted a stack trace and "If you think this is a bug, please open an issue" instead of naming the offending key.Three root causes, all in
validateContainerApp:public_keydereferenced after being rejected.if (!isRequiredProperty(key, "public_key", "string"))recorded the error, then the next statement calledkey.public_key.toLowerCase()unconditionally. Missingpublic_keythrewCannot read properties of undefined (reading 'toLowerCase'); a non-string one threwkey.public_key.toLowerCase is not a function.isRequiredPropertybottoms out inhasProperty, which isproperty in target, andinthrows on a primitive.authorized_keys: ["ssh-ed25519 AAAA..."]threw before any check could report anything.configuration: null. The guard wastypeof configuration !== "object" || Array.isArray(configuration), andtypeof null === "object", sonullpassed as a valid object andObject.keys(null)threw about 240 lines later.The
authorized_keysandtrusted_user_ca_keysblocks were copies of each other differing only in whethernameis required, so both carried both key faults. They are now onevalidateSshPublicKeyshelper: it reports a non-array up front, skips entries that are not objects, and makes the ED25519 prefix check anelse ifso it only runs on apublic_keythat already passed the string check. Theconfigurationtype check gains the missingnullcase, and the laterObject.keyscall is guarded so it only runs for a real object, which also removes the bogusUnexpected fields found in containers.configuration field: "0","1","2"warning a string value produced from its character offsets.All of these now produce ordinary diagnostics such as
containers.authorized_keys[0].public_key must be a string. Net effect on the source is 28 fewer lines.Addressed from review
configuration: nullcombined withinstance_typestill crashed. The type check reported the problem and then fell through to the limits-versus-instance_typecheck, which dereferencesconfiguration.disk,.vcpuand.memory_mib, throwingTypeError: Cannot read properties of null (reading 'disk'). That is the same class of crash this PR exists to remove, and my first pass missed it. The limits check is now anelsebranch of the type check, so it only runs onceconfigurationis known to be a real object.Eight tests are added in
normalize-and-validate-config.test.ts. Six failure cases each fail onmainwith the exactTypeErrorreported in the issue or found in review, one covers the corrected unsupported-key-type message, and one asserts a validauthorized_keysandtrusted_user_ca_keyspair still validates clean, passing both before and after. These fields previously had no negative test coverage anywhere in the repo.containers.authorized_keys,containers.trusted_user_ca_keysandcontainers.configurationis unchanged, and no valid config behaves differently.