Skip to content

Commit 16f7a05

Browse files
fredbiclaude
andauthored
Recursion guard (#306)
* fix: do not redeem a pooled validator twice when Validate panics With recycled validators, SchemaValidator.Validate and the allOf, anyOf, oneOf and not loops of schemaPropsValidator cleared the slot of a child only after the child's Validate returned. A child redeems itself in a deferred call, so when its Validate panicked (e.g. a $ref under a property that cannot be expanded), the parent's deferred redeemChildren redeemed it a second time. The pool then hands the same object to two later borrowers. A server survives the panic (net/http recovers handler panics), so later requests could share validator state and return wrong results. Clear the slot before calling Validate. TestPools_NoDoubleRedeemOnPanic reproduces it with -tags poolsdebug, which reports the double redeem. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Frédéric BIDON <fredbi@yahoo.com> * fix: cap the validators built for allOf, anyOf, oneOf and not A circular $ref through a composition keyword, such as A: {allOf: [{$ref: A}]}, made schema validation overflow the goroutine stack: a fatal error that recover cannot catch. go-openapi/spec bounds each expansion, and leaves a circular $ref in place. But newSchemaPropsValidator builds the allOf, anyOf, oneOf and not validators along with their parent, and each one expands its $ref again, one level down, in a new expansion. Nothing bounded that loop. With two branches, A: {allOf: [{$ref: A}, {$ref: A}]}, it builds 2^depth validators. Thread a compositionBudget through validator construction. It caps the nesting at maxCompositionDepth (128) and the count at maxCompositionValidators (10,000) per schema validator. Going over either cap panics with "Invalid schema provided to SchemaValidator", like any other schema that cannot be expanded. Only schemas with composition keywords create a budget, and it stays on the stack: no pre-walk, no allocation. Validators for properties and items are built as Validate walks the data, and start their own budget, so recursive schemas such as trees are not affected. SpecValidator no longer panics on such a schema when it validates defaults and examples (reached with ContinueOnErrors, since the circular ancestry check stops it otherwise): validateValueAgainstSchema returns the panic as an error at the location of the value. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Frédéric BIDON <fredbi@yahoo.com> --------- Signed-off-by: Frédéric BIDON <fredbi@yahoo.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent d16ffd7 commit 16f7a05

9 files changed

Lines changed: 364 additions & 33 deletions

‎default_validator.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -230,9 +230,7 @@ func (d *defaultValidator) validateDefaultValueSchemaAgainstSchema(path pathSegm
230230
s := d.SpecValidator
231231

232232
if schema.Default != nil {
233-
res.Merge(
234-
newSchemaValidator(schema, s.spec.Spec(), path.child(jsonDefault), s.KnownFormats, d.schemaOptions).Validate(schema.Default),
235-
)
233+
res.Merge(s.validateValueAgainstSchema(schema, path.child(jsonDefault), schema.Default, d.schemaOptions))
236234
}
237235
if schema.Items != nil {
238236
if schema.Items.Schema != nil {

‎example_validator.go‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -214,10 +214,7 @@ func (ex *exampleValidator) validateExampleInResponse(
214214
exampleAt := responsePath(path, method, responseCodeAsStr).
215215
child(swaggerExamples).
216216
structuralChild(jsonMimeApplicationJSON)
217-
res.MergeAsWarnings(
218-
newSchemaValidator(response.Schema, s.spec.Spec(),
219-
exampleAt, s.KnownFormats, s.schemaOptions).Validate(example),
220-
)
217+
res.MergeAsWarnings(s.validateValueAgainstSchema(response.Schema, exampleAt, example, s.schemaOptions))
221218
} else {
222219
// Proposal for enhancement: validate other media types too
223220
res.addWarningsAt(responsePath(path, method, responseCodeAsStr).child(swaggerExamples), examplesMimeNotSupportedMsg(operationID, responseName))
@@ -239,9 +236,7 @@ func (ex *exampleValidator) validateExampleValueSchemaAgainstSchema(path pathSeg
239236
res := validatorPools.results.Borrow()
240237

241238
if schema.Example != nil {
242-
res.MergeAsWarnings(
243-
newSchemaValidator(schema, s.spec.Spec(), path.child(swaggerExample), s.KnownFormats, ex.schemaOptions).Validate(schema.Example),
244-
)
239+
res.MergeAsWarnings(s.validateValueAgainstSchema(schema, path.child(swaggerExample), schema.Example, ex.schemaOptions))
245240
}
246241
if schema.Items != nil {
247242
if schema.Items.Schema != nil {

‎pools_leak_test.go‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,3 +79,24 @@ func TestPools_NoLeakOnNilSchema(t *testing.T) {
7979
require.NoError(t, AgainstSchema(nil, map[string]any{}, strfmt.Default))
8080
require.True(t, pools.AssertNoLeaks(t))
8181
}
82+
83+
// TestPools_NoDoubleRedeemOnPanic builds a validator that panics inside Validate: the $ref of
84+
// property "x" is expanded when the object validator walks the data. The validators that
85+
// redeem themselves on the way out must not be redeemed again by their parent.
86+
//
87+
// Like the other tests here, it only bites with the "poolsdebug" tag: the pools then panic
88+
// on a double redeem, and that panic replaces the one from the schema.
89+
func TestPools_NoDoubleRedeemOnPanic(t *testing.T) {
90+
resetPools()
91+
pools.ResetTracking()
92+
t.Cleanup(resetPools)
93+
t.Cleanup(pools.ResetTracking)
94+
95+
var schema spec.Schema
96+
require.NoError(t, json.Unmarshal([]byte(`{"properties":{"x":{"$ref":"#/definitions/missing"}}}`), &schema))
97+
98+
require.PanicsWithValue(t,
99+
`Invalid schema provided to SchemaValidator: nil value has no field "missing": JSON pointer error`,
100+
func() { _ = AgainstSchema(&schema, map[string]any{"x": 1}, strfmt.Default) },
101+
)
102+
}

‎schema.go‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,15 @@ func rootPathFromString(root string) pathSegments {
9494
}
9595

9696
func newSchemaValidator(schema *spec.Schema, rootSchema any, root pathSegments, formats strfmt.Registry, opts *SchemaValidatorOptions) *SchemaValidator {
97+
return newSchemaValidatorWithBudget(schema, rootSchema, root, formats, opts, nil)
98+
}
99+
100+
// newSchemaValidatorWithBudget builds a schema validator, drawing the validators built for
101+
// allOf, anyOf, oneOf and not from budget. A nil budget starts a new one if needed.
102+
func newSchemaValidatorWithBudget(
103+
schema *spec.Schema, rootSchema any, root pathSegments, formats strfmt.Registry, opts *SchemaValidatorOptions,
104+
budget *compositionBudget,
105+
) *SchemaValidator {
97106
if schema == nil {
98107
return nil
99108
}
@@ -131,7 +140,7 @@ func newSchemaValidator(schema *spec.Schema, rootSchema any, root pathSegments,
131140

132141
s.validators = [8]valueValidator{
133142
s.typeValidator(),
134-
s.schemaPropsValidator(),
143+
s.schemaPropsValidator(budget),
135144
s.stringValidator(),
136145
s.formatValidator(),
137146
s.numberValidator(),
@@ -265,10 +274,12 @@ func (s *SchemaValidator) Validate(data any) *Result {
265274
continue
266275
}
267276

268-
result.Merge(v.Validate(d))
269277
if s.Options.recycleValidators {
270-
s.validators[idx] = nil // prevents further (unsafe) usage
278+
// prevents further (unsafe) usage. Validate redeems it, even if it panics:
279+
// redeemChildren must not redeem it again
280+
s.validators[idx] = nil
271281
}
282+
result.Merge(v.Validate(d))
272283
result.Inc()
273284
}
274285
result.Inc()
@@ -352,11 +363,11 @@ func (s *SchemaValidator) formatValidator() valueValidator {
352363
)
353364
}
354365

355-
func (s *SchemaValidator) schemaPropsValidator() valueValidator {
366+
func (s *SchemaValidator) schemaPropsValidator(budget *compositionBudget) valueValidator {
356367
sch := s.Schema
357368
return newSchemaPropsValidator(
358369
s.path, s.in, sch.AllOf, sch.OneOf, sch.AnyOf, sch.Not, sch.Dependencies, s.Root, s.KnownFormats,
359-
s.Options,
370+
s.Options, budget,
360371
)
361372
}
362373

‎schema_composition_test.go‎

Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,206 @@
1+
// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package validate
5+
6+
import (
7+
"encoding/json"
8+
"fmt"
9+
"strings"
10+
"testing"
11+
12+
"github.com/go-openapi/loads"
13+
"github.com/go-openapi/spec"
14+
"github.com/go-openapi/strfmt"
15+
"github.com/go-openapi/testify/v2/assert"
16+
"github.com/go-openapi/testify/v2/require"
17+
)
18+
19+
func TestCompositionBudget(t *testing.T) {
20+
// each of these schemas loops through allOf, anyOf, oneOf or not and their $ref:
21+
// without a budget, building the validator overflows the goroutine stack
22+
// (fatal, not recoverable) or never ends
23+
for _, tc := range []struct {
24+
name string
25+
schema string
26+
}{
27+
{
28+
name: "allOf cycle",
29+
schema: `{"definitions":{"CycleAllOf":{"allOf":[{"$ref":"#/definitions/CycleAllOf"}]}},"properties":{"x":{"$ref":"#/definitions/CycleAllOf"}}}`,
30+
},
31+
{
32+
name: "anyOf cycle",
33+
schema: `{"definitions":{"CycleAnyOf":{"anyOf":[{"type":"string"},{"$ref":"#/definitions/CycleAnyOf"}]}},"properties":{"x":{"$ref":"#/definitions/CycleAnyOf"}}}`,
34+
},
35+
{
36+
name: "oneOf cycle",
37+
schema: `{"definitions":{"CycleOneOf":{"oneOf":[{"$ref":"#/definitions/CycleOneOf"}]}},"properties":{"x":{"$ref":"#/definitions/CycleOneOf"}}}`,
38+
},
39+
{
40+
name: "not cycle",
41+
schema: `{"definitions":{"CycleNot":{"not":{"$ref":"#/definitions/CycleNot"}}},"properties":{"x":{"$ref":"#/definitions/CycleNot"}}}`,
42+
},
43+
{
44+
name: "mutual cycle",
45+
schema: `{
46+
"definitions":{
47+
"CycleMutualA":{"allOf":[{"$ref":"#/definitions/CycleMutualB"}]},
48+
"CycleMutualB":{"anyOf":[{"$ref":"#/definitions/CycleMutualA"}]}
49+
},
50+
"properties":{"x":{"$ref":"#/definitions/CycleMutualA"}}
51+
}`,
52+
},
53+
{
54+
// 2 branches per level: 2^depth validators without a budget
55+
name: "fan-out cycle",
56+
schema: `{
57+
"definitions":{"CycleFanOut":{"allOf":[{"$ref":"#/definitions/CycleFanOut"},{"$ref":"#/definitions/CycleFanOut"}]}},
58+
"properties":{"x":{"$ref":"#/definitions/CycleFanOut"}}
59+
}`,
60+
},
61+
} {
62+
t.Run(tc.name, func(t *testing.T) {
63+
var schema spec.Schema
64+
require.NoError(t, json.Unmarshal([]byte(tc.schema), &schema))
65+
66+
defer func() {
67+
r := recover()
68+
require.NotNil(t, r)
69+
msg, ok := r.(string)
70+
require.TrueT(t, ok)
71+
assert.True(t, strings.HasPrefix(msg, "Invalid schema provided to SchemaValidator: "))
72+
assert.True(t, strings.Contains(msg, fmt.Sprintf("deeper than %d levels or into more than %d validators", maxCompositionDepth, maxCompositionValidators)))
73+
}()
74+
75+
_ = AgainstSchema(&schema, map[string]any{"x": 1}, strfmt.Default)
76+
})
77+
}
78+
}
79+
80+
func TestCompositionBudgetRecursiveSchema(t *testing.T) {
81+
// a $ref back to an ancestor through properties or items is built as the data is walked:
82+
// the depth of the data bounds it, and it does not draw from the budget
83+
const tree = `{
84+
"definitions":{
85+
"BudgetNode":{
86+
"allOf":[
87+
{"type":"object","required":["name"],"properties":{"name":{"type":"string"}}},
88+
{"properties":{"children":{"type":"array","items":{"$ref":"#/definitions/BudgetNode"}}}}
89+
]
90+
}
91+
},
92+
"properties":{"root":{"$ref":"#/definitions/BudgetNode"}}
93+
}`
94+
95+
var schema spec.Schema
96+
require.NoError(t, json.Unmarshal([]byte(tree), &schema))
97+
98+
// nest the data deeper than both caps: each level only builds the validators of one Node
99+
const depth = 2 * maxCompositionDepth
100+
node := map[string]any{nameProp: "leaf"}
101+
for range depth {
102+
node = map[string]any{nameProp: "node", "children": []any{node}}
103+
}
104+
105+
require.NoError(t, AgainstSchema(&schema, map[string]any{"root": node}, strfmt.Default))
106+
107+
node["children"] = []any{map[string]any{"children": []any{}}} // missing name
108+
require.Error(t, AgainstSchema(&schema, map[string]any{"root": node}, strfmt.Default))
109+
}
110+
111+
func TestCompositionBudgetWideSchema(t *testing.T) {
112+
// many siblings within the budget still build
113+
const width = maxCompositionValidators
114+
branches := make([]spec.Schema, width)
115+
for i := range branches {
116+
branches[i] = *spec.StringProperty()
117+
}
118+
schema := &spec.Schema{SchemaProps: spec.SchemaProps{AllOf: branches}}
119+
120+
require.NoError(t, AgainstSchema(schema, "a", strfmt.Default))
121+
require.Error(t, AgainstSchema(schema, 1, strfmt.Default))
122+
123+
// one more panics
124+
schema.AllOf = append(schema.AllOf, *spec.StringProperty())
125+
require.Panics(t, func() { _ = AgainstSchema(schema, "a", strfmt.Default) })
126+
}
127+
128+
func TestCompositionBudgetAmplification(t *testing.T) {
129+
// no cycle, and within maxCompositionDepth: A0 refers twice to A1, which refers twice to A2,
130+
// and so on. Expanding it stays within the budget of go-openapi/spec, but it would build
131+
// 2^levels validators.
132+
const levels = 13
133+
definitions := make(map[string]any, levels+1)
134+
for i := range levels {
135+
ref := map[string]any{jsonRef: fmt.Sprintf("#/definitions/BudgetA%d", i+1)}
136+
definitions[fmt.Sprintf("BudgetA%d", i)] = map[string]any{"allOf": []any{ref, ref}}
137+
}
138+
definitions[fmt.Sprintf("BudgetA%d", levels)] = map[string]any{jsonType: stringType}
139+
140+
doc, err := json.Marshal(map[string]any{
141+
swaggerDefinitions: definitions,
142+
"properties": map[string]any{"x": map[string]any{jsonRef: "#/definitions/BudgetA0"}},
143+
})
144+
require.NoError(t, err)
145+
146+
var schema spec.Schema
147+
require.NoError(t, json.Unmarshal(doc, &schema))
148+
149+
require.PanicsWithValue(t,
150+
invalidSchemaProvidedMsg(tooManyCompositionValidatorsMsg("x", maxCompositionDepth, maxCompositionValidators)).Error(),
151+
func() { _ = AgainstSchema(&schema, map[string]any{"x": "a"}, strfmt.Default) },
152+
)
153+
}
154+
155+
func TestCompositionBudgetDeepSchema(t *testing.T) {
156+
// nesting allOf within the depth cap still builds
157+
schema := spec.StringProperty()
158+
for range maxCompositionDepth {
159+
schema = &spec.Schema{SchemaProps: spec.SchemaProps{AllOf: []spec.Schema{*schema}}}
160+
}
161+
162+
require.NoError(t, AgainstSchema(schema, "a", strfmt.Default))
163+
require.Error(t, AgainstSchema(schema, 1, strfmt.Default))
164+
165+
// one level more panics
166+
schema = &spec.Schema{SchemaProps: spec.SchemaProps{AllOf: []spec.Schema{*schema}}}
167+
require.Panics(t, func() { _ = AgainstSchema(schema, "a", strfmt.Default) })
168+
}
169+
170+
func TestCompositionBudgetSpec(t *testing.T) {
171+
// Spec stops on the circular ancestry of CycleSpec before it checks defaults and examples.
172+
// ContinueOnErrors goes on: building a validator for the default or the example panics,
173+
// and the spec validator returns it as an error (default) or a warning (example).
174+
const doc = `{
175+
"swagger":"2.0",
176+
"info":{"title":"t","version":"1"},
177+
"paths":{},
178+
"definitions":{
179+
"CycleSpec":{"default":1,"example":1,"allOf":[{"$ref":"#/definitions/CycleSpec"}]}
180+
}
181+
}`
182+
183+
analyzed, err := loads.Analyzed(json.RawMessage(doc), "")
184+
require.NoError(t, err)
185+
186+
validator := NewSpecValidator(analyzed.Schema(), strfmt.Default)
187+
validator.SetContinueOnErrors(true)
188+
189+
var errs, warnings *Result
190+
require.NotPanics(t, func() { errs, warnings = validator.Validate(analyzed) })
191+
192+
const tooDeep = "nests allOf, anyOf, oneOf and not deeper than"
193+
assert.True(t, hasMessageContaining(errs.Errors, "has circular ancestry"))
194+
assert.True(t, hasMessageContaining(errs.Errors, tooDeep))
195+
assert.True(t, hasMessageContaining(warnings.Errors, tooDeep))
196+
}
197+
198+
func hasMessageContaining(errs []error, substr string) bool {
199+
for _, err := range errs {
200+
if strings.Contains(err.Error(), substr) {
201+
return true
202+
}
203+
}
204+
205+
return false
206+
}

‎schema_messages.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
package validate
55

66
import (
7+
"strings"
8+
79
"github.com/go-openapi/errors"
810
)
911

@@ -34,17 +36,29 @@ const (
3436
// NOTE: punctuation in message.
3537
MustValidateAllSchemasError = "%q must validate all the schemas (allOf)%s"
3638

39+
// TooManyCompositionValidatorsError indicates that allOf, anyOf, oneOf and not, followed through
40+
// their $ref, nest too deep or need too many validators. A circular $ref through these
41+
// keywords is the usual cause.
42+
TooManyCompositionValidatorsError = "%q nests allOf, anyOf, oneOf and not deeper than %d levels or into more than %d validators: look for a circular $ref"
43+
3744
// MustNotValidateSchemaError indicates that in a Not construct, the schema constraint specified was verified.
3845
MustNotValidateSchemaError = "%q must not validate the schema (not)"
3946
)
4047

48+
// invalidSchemaProvidedPrefix starts the panic message of a schema validator that cannot be built.
49+
var invalidSchemaProvidedPrefix = strings.TrimSuffix(InvalidSchemaProvidedError, "%v")
50+
4151
// Warning messages related to schema validation and returned as results.
4252
const ()
4353

4454
func invalidSchemaProvidedMsg(err error) errors.Error {
4555
return errors.New(InternalErrorCode, InvalidSchemaProvidedError, err)
4656
}
4757

58+
func tooManyCompositionValidatorsMsg(path string, maxDepth, maxValidators int) errors.Error {
59+
return errors.New(InternalErrorCode, TooManyCompositionValidatorsError, path, maxDepth, maxValidators)
60+
}
61+
4862
func invalidTypeConversionMsg(path string, err error) errors.Error {
4963
return errors.New(errors.CompositeErrorCode, InvalidTypeConversionError, path, err)
5064
}

0 commit comments

Comments
 (0)