Skip to content

fix: reject non-mapping entries under networks and models - #932

Merged
ndeloof merged 1 commit into
compose-spec:mainfrom
glours:fix/networks-additional-properties-panic
Sep 18, 2026
Merged

ndeloof merged 1 commit into
compose-spec:mainfrom
glours:fix/networks-additional-properties-panic

Conversation

@glours

@glours glours commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

The compose-spec schema allowed a key under the top-level networks and models blocks to bypass the patternProperties check, since additionalProperties wasn't set to false like volumes/secrets/configs already do. A malformed key such as "/" therefore skipped schema validation entirely and reached transformMaybeExternal with its raw scalar value, which panicked on an unchecked type assertion instead of surfacing a validation error.

Align the networks and models schema entries with their siblings, and make transformMaybeExternal return an error instead of panicking if a schema gap lets a non-mapping value through again.

fix https://docker.atlassian.net/browse/DDB-706

The compose-spec schema allowed a key under the top-level networks
and models blocks to bypass the patternProperties check, since
additionalProperties wasn't set to false like volumes/secrets/configs
already do. A malformed key such as "/" therefore skipped schema
validation entirely and reached transformMaybeExternal with its raw
scalar value, which panicked on an unchecked type assertion instead
of surfacing a validation error.

Align the networks and models schema entries with their siblings,
and make transformMaybeExternal return an error instead of panicking
if a schema gap lets a non-mapping value through again.

Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
@glours
glours requested a review from ndeloof as a code owner September 18, 2026 11:52
@glours glours self-assigned this Sep 18, 2026

@ndeloof ndeloof left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with local verification on the head:

The fix is correct and complete.

  • Reproduced the failure mode on the base commit with this PR's tests only: transformMaybeExternal(1, …) panics with the exact interface-conversion described (DDB-706), and the malformed /: key sails through validation. On the head both surface proper errors.
  • Schema parity verified programmatically: all six top-level resource sections (services, networks, volumes, secrets, configs, models) now carry the same patternProperties + additionalProperties: false shape.
  • No x-* regression: extension-style keys match ^[a-zA-Z0-9._-]+$ and keep validating as resource names, exactly like under volumes today.
  • The test split is right: the loader test locks the schema rejection (its /: entry carries a valid mapping body, so it exercises the key check, not the panic), while the transform unit test locks the panic→error conversion as defense in depth against future schema gaps.
  • loader, loader/tests, transform and schema suites all green locally on the head.

One follow-up needed after merge: compose-spec/compose-spec's schema/compose-spec.json does not have these two additionalProperties entries yet — this PR breaks the byte-identity between the two schema copies that compose-spec's CI validates. A companion compose-spec PR should mirror the change.

@ndeloof
ndeloof merged commit 14d7cfa into compose-spec:main Sep 18, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants