Skip to content

Commit 65850e1

Browse files
myasnikovdaniilIvanHunters
myasnikovdaniil
authored and
IvanHunters
committed
fix(api): recursively sanitize boolean additionalProperties in published spec
Addresses review feedback on #2863. The previous fix only converted the open spec wrapper cozystack-api injects, so a user-supplied ApplicationDefinition openAPISchema carrying a boolean additionalProperties — declared explicitly at the top level, or nested under properties/items/allOf/anyOf/oneOf — was republished verbatim and re-triggered the kube-controller-manager VAP type-checker crash. openAPISchema is untrusted input (external-apps and operator-authored definitions flow through unvalidated), so close the whole class rather than only the schemas cozystack ships. Recursively rewrite every boolean-form additionalProperties (nil inner schema): additionalProperties:true becomes x-kubernetes-preserve-unknown-fields:true, additionalProperties:false is dropped. Both JSON forms carry a nil inner schema and crash the type-checker identically; object-form maps are recursed into and left intact. Also publish the schemaless spec from a fresh schema so no inherited $ref or properties leak into the open object. Regression tests cover top-level true/false and nested (properties, items, allOf) boolean forms, asserting the published schema carries no boolean additionalProperties node and does not panic SchemaDeclType. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]> Signed-off-by: Ivan Okhotnikov <[email protected]>
1 parent ebde1c9 commit 65850e1

2 files changed

Lines changed: 176 additions & 16 deletions

File tree

‎pkg/cmd/server/openapi.go‎

Lines changed: 78 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -67,14 +67,14 @@ func findSpecContainer(s *spec.Schema) *spec.Schema {
6767
// properties, using the x-kubernetes-preserve-unknown-fields extension rather
6868
// than the boolean additionalProperties:true form.
6969
//
70-
// This distinction matters: the boolean additionalProperties:true construct
71-
// makes the Kubernetes ValidatingAdmissionPolicy status type-checker (run by
72-
// kube-controller-manager) nil-dereference when it recurses into the node,
73-
// crash-looping KCM cluster-wide as soon as a VAP matches one of these
74-
// aggregated resources (e.g. cozystack-tenant-host-policy on tenants). The
75-
// extension is semantically equivalent ("arbitrary fields allowed") and the
76-
// type-checker handles it safely. See
77-
// https://github.com/cozystack/cozystack/issues/2863.
70+
// This distinction matters: a boolean additionalProperties node (the JSON
71+
// `true` or `false` form) carries a nil inner schema. The Kubernetes
72+
// ValidatingAdmissionPolicy status type-checker (run by kube-controller-manager)
73+
// dereferences that nil inner schema, crash-looping KCM cluster-wide as soon as
74+
// a VAP matches one of these aggregated resources (e.g.
75+
// cozystack-tenant-host-policy on tenants). The extension is semantically
76+
// equivalent ("arbitrary fields allowed") and the type-checker handles it
77+
// safely. See https://github.com/cozystack/cozystack/issues/2863.
7878
func markOpenObject(s *spec.Schema) {
7979
if len(s.Type) == 0 {
8080
s.Type = spec.StringOrArray{"object"}
@@ -86,13 +86,66 @@ func markOpenObject(s *spec.Schema) {
8686
s.Extensions["x-kubernetes-preserve-unknown-fields"] = true
8787
}
8888

89+
// sanitizeBooleanAdditionalProperties recursively rewrites every boolean-form
90+
// additionalProperties (one whose inner Schema is nil — both the JSON `true`
91+
// and `false` forms) anywhere in s, because both crash the VAP status
92+
// type-checker (#2863): additionalProperties:true becomes the equivalent
93+
// x-kubernetes-preserve-unknown-fields:true, and additionalProperties:false is
94+
// dropped (the published schema is consumed only for documentation and
95+
// type-checking, not for enforcement, so "declared properties only" is
96+
// preserved without the crash-prone node). Object-form additionalProperties (a
97+
// real map value schema) is safe; it is recursed into, never rewritten.
98+
//
99+
// ApplicationDefinition.openAPISchema is untrusted input — external-apps and
100+
// operator-authored definitions flow through here without validation — so this
101+
// closes the whole class of crash rather than only the schemas cozystack ships.
102+
func sanitizeBooleanAdditionalProperties(s *spec.Schema) {
103+
if s == nil {
104+
return
105+
}
106+
if ap := s.AdditionalProperties; ap != nil && ap.Schema == nil {
107+
if ap.Allows {
108+
markOpenObject(s) // additionalProperties:true -> preserve-unknown-fields
109+
} else {
110+
s.AdditionalProperties = nil // additionalProperties:false -> drop
111+
}
112+
} else if ap != nil && ap.Schema != nil {
113+
sanitizeBooleanAdditionalProperties(ap.Schema)
114+
}
115+
for k := range s.Properties {
116+
prop := s.Properties[k]
117+
sanitizeBooleanAdditionalProperties(&prop)
118+
s.Properties[k] = prop
119+
}
120+
if s.Items != nil {
121+
sanitizeBooleanAdditionalProperties(s.Items.Schema)
122+
for i := range s.Items.Schemas {
123+
sanitizeBooleanAdditionalProperties(&s.Items.Schemas[i])
124+
}
125+
}
126+
for i := range s.AllOf {
127+
sanitizeBooleanAdditionalProperties(&s.AllOf[i])
128+
}
129+
for i := range s.AnyOf {
130+
sanitizeBooleanAdditionalProperties(&s.AnyOf[i])
131+
}
132+
for i := range s.OneOf {
133+
sanitizeBooleanAdditionalProperties(&s.OneOf[i])
134+
}
135+
sanitizeBooleanAdditionalProperties(s.Not)
136+
}
137+
89138
// patchSpec injects/overrides ".spec" with user JSON (or schemaless object).
90139
func patchSpec(target *spec.Schema, raw string) error {
140+
if target.Properties == nil {
141+
target.Properties = map[string]spec.Schema{}
142+
}
143+
144+
// Schemaless: publish a fresh, fully open object. Starting from a fresh
145+
// schema (rather than mutating the cloned base) discards any inherited
146+
// $ref or properties so the published spec is unambiguously open.
91147
if strings.TrimSpace(raw) == "" {
92-
if target.Properties == nil {
93-
target.Properties = map[string]spec.Schema{}
94-
}
95-
prop := target.Properties["spec"]
148+
prop := spec.Schema{}
96149
markOpenObject(&prop)
97150
target.Properties["spec"] = prop
98151
return nil
@@ -102,12 +155,21 @@ func patchSpec(target *spec.Schema, raw string) error {
102155
if err := json.Unmarshal([]byte(raw), &custom); err != nil {
103156
return err
104157
}
105-
if custom.AdditionalProperties == nil {
158+
159+
// Neutralize any crash-prone boolean additionalProperties at any depth in
160+
// the user-supplied schema before publishing it.
161+
topLevelAPAbsent := custom.AdditionalProperties == nil
162+
sanitizeBooleanAdditionalProperties(&custom)
163+
164+
// Preserve the historical "spec is a superset of the documented values"
165+
// behavior: when the documented schema does not itself declare
166+
// additionalProperties, mark the top level open so undocumented Helm
167+
// values are accepted rather than rejected. A schema that explicitly
168+
// declared additionalProperties (now sanitized) keeps its own intent.
169+
if topLevelAPAbsent {
106170
markOpenObject(&custom)
107171
}
108-
if target.Properties == nil {
109-
target.Properties = map[string]spec.Schema{}
110-
}
172+
111173
target.Properties["spec"] = custom
112174
return nil
113175
}

‎pkg/cmd/server/openapi_test.go‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,3 +166,101 @@ func TestPatchedSpecDoesNotPanicVAPTypeChecker(t *testing.T) {
166166
})
167167
}
168168
}
169+
170+
// hasBooleanAdditionalProperties walks the schema and reports whether any node
171+
// carries a boolean-form additionalProperties (inner Schema == nil) — the
172+
// construct that crashes the VAP type-checker, in either its true or false JSON
173+
// form.
174+
func hasBooleanAdditionalProperties(s *spec.Schema) bool {
175+
if s == nil {
176+
return false
177+
}
178+
if ap := s.AdditionalProperties; ap != nil {
179+
if ap.Schema == nil {
180+
return true
181+
}
182+
if hasBooleanAdditionalProperties(ap.Schema) {
183+
return true
184+
}
185+
}
186+
for k := range s.Properties {
187+
p := s.Properties[k]
188+
if hasBooleanAdditionalProperties(&p) {
189+
return true
190+
}
191+
}
192+
if s.Items != nil {
193+
if hasBooleanAdditionalProperties(s.Items.Schema) {
194+
return true
195+
}
196+
for i := range s.Items.Schemas {
197+
if hasBooleanAdditionalProperties(&s.Items.Schemas[i]) {
198+
return true
199+
}
200+
}
201+
}
202+
for _, branch := range [][]spec.Schema{s.AllOf, s.AnyOf, s.OneOf} {
203+
for i := range branch {
204+
if hasBooleanAdditionalProperties(&branch[i]) {
205+
return true
206+
}
207+
}
208+
}
209+
return hasBooleanAdditionalProperties(s.Not)
210+
}
211+
212+
// TestPatchSpecSanitizesUserSuppliedBooleanAdditionalProperties is the
213+
// regression test for the review finding on #2867: ApplicationDefinition
214+
// schemas are untrusted input, and a user-supplied boolean additionalProperties
215+
// — declared explicitly at the top level, or nested anywhere under
216+
// properties/items/allOf — reintroduces the exact #2863 KCM crash. patchSpec
217+
// must neutralize every such node (both the true and false forms, since both
218+
// carry a nil inner schema), at any depth, so the published schema can never
219+
// crash the VAP type-checker. Each case below panics SchemaDeclType before this
220+
// sanitizer and passes after it.
221+
func TestPatchSpecSanitizesUserSuppliedBooleanAdditionalProperties(t *testing.T) {
222+
cases := map[string]string{
223+
"top-level-boolean-true": `{"type":"object","additionalProperties":true}`,
224+
"top-level-boolean-false": `{"type":"object","additionalProperties":false}`,
225+
"nested-under-properties": `{"type":"object","properties":{"foo":{"type":"object","additionalProperties":true}}}`,
226+
"nested-under-items": `{"type":"object","properties":{"list":{"type":"array","items":{"type":"object","additionalProperties":true}}}}`,
227+
"nested-under-allof": `{"type":"object","allOf":[{"type":"object","additionalProperties":false}]}`,
228+
}
229+
230+
for name, raw := range cases {
231+
t.Run(name, func(t *testing.T) {
232+
obj := &spec.Schema{SchemaProps: spec.SchemaProps{
233+
Type: spec.StringOrArray{"object"},
234+
Properties: map[string]spec.Schema{
235+
"apiVersion": {SchemaProps: spec.SchemaProps{Type: spec.StringOrArray{"string"}}},
236+
"kind": {SchemaProps: spec.SchemaProps{Type: spec.StringOrArray{"string"}}},
237+
"spec": {},
238+
},
239+
}}
240+
if err := patchSpec(obj, raw); err != nil {
241+
t.Fatalf("patchSpec: %v", err)
242+
}
243+
244+
// Round-trip through JSON: this is what is actually served and what
245+
// KCM type-checks.
246+
rawJSON, err := json.Marshal(obj)
247+
if err != nil {
248+
t.Fatalf("marshal: %v", err)
249+
}
250+
var published spec.Schema
251+
if err := json.Unmarshal(rawJSON, &published); err != nil {
252+
t.Fatalf("unmarshal: %v", err)
253+
}
254+
255+
// (1) no boolean-form additionalProperties survives anywhere
256+
if hasBooleanAdditionalProperties(&published) {
257+
t.Errorf("published schema still contains a boolean additionalProperties node (#2863): %s", rawJSON)
258+
}
259+
// (2) the published schema does not crash the VAP type-checker
260+
declType := celopenapi.SchemaDeclType(&published, true)
261+
if declType == nil {
262+
t.Fatalf("SchemaDeclType returned nil for %s", name)
263+
}
264+
})
265+
}
266+
}

0 commit comments

Comments
 (0)