Skip to content

Commit d6d7bac

Browse files
fredbifredbi
authored
Merge pull request #291 from fredbi/fix/no-in-place-expansion-of-caller-schema
Fix/no in place expansion of caller schema
2 parents bec3b87 + bbce87e commit d6d7bac

7 files changed

Lines changed: 132 additions & 47 deletions

File tree

‎go.mod‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,27 @@
11
module github.com/go-openapi/validate
22

33
require (
4-
github.com/go-openapi/analysis v0.26.0
4+
github.com/go-openapi/analysis v0.26.1
55
github.com/go-openapi/errors v0.22.8
66
github.com/go-openapi/jsonpointer v1.0.0
77
github.com/go-openapi/loads v0.25.1
8-
github.com/go-openapi/spec v0.22.9
8+
github.com/go-openapi/spec v0.22.10
99
github.com/go-openapi/strfmt v0.27.0
10-
github.com/go-openapi/swag/conv v0.29.0
11-
github.com/go-openapi/swag/fileutils v0.29.0
12-
github.com/go-openapi/swag/jsonutils v0.29.0
13-
github.com/go-openapi/swag/loading v0.29.0
14-
github.com/go-openapi/swag/pools v0.29.0
15-
github.com/go-openapi/swag/stringutils v0.29.0
16-
github.com/go-openapi/testify/v2 v2.6.1
10+
github.com/go-openapi/swag/conv v0.29.1
11+
github.com/go-openapi/swag/fileutils v0.29.1
12+
github.com/go-openapi/swag/jsonutils v0.29.1
13+
github.com/go-openapi/swag/loading v0.29.1
14+
github.com/go-openapi/swag/pools v0.29.1
15+
github.com/go-openapi/swag/stringutils v0.29.1
16+
github.com/go-openapi/testify/v2 v2.7.0
1717
go.yaml.in/yaml/v3 v3.0.5
1818
)
1919

2020
require (
2121
github.com/go-openapi/jsonreference v1.0.0 // indirect
22-
github.com/go-openapi/swag/mangling v0.28.0 // indirect
23-
github.com/go-openapi/swag/typeutils v0.29.0 // indirect
24-
github.com/go-openapi/swag/yamlutils v0.29.0 // indirect
22+
github.com/go-openapi/swag/mangling v0.29.1 // indirect
23+
github.com/go-openapi/swag/typeutils v0.29.1 // indirect
24+
github.com/go-openapi/swag/yamlutils v0.29.1 // indirect
2525
github.com/go-viper/mapstructure/v2 v2.5.0 // indirect
2626
github.com/google/uuid v1.6.0 // indirect
2727
github.com/oklog/ulid/v2 v2.1.2 // indirect

‎go.sum‎

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
github.com/go-openapi/analysis v0.26.0 h1:1xECln1iMMmQnTjgcknC1vi1hA4KISt6IHpSwnqcuwI=
2-
github.com/go-openapi/analysis v0.26.0/go.mod h1:40gERFi/2dyXA1FaqRRLxkv1IlC6X+GPDNd1xrYAjZE=
1+
github.com/go-openapi/analysis v0.26.1 h1:BqYuDaQiFflcgPWGDQc7niUFE2pHTVq1H3iwDsRYMqM=
2+
github.com/go-openapi/analysis v0.26.1/go.mod h1:E2siwFrz00/Z1sifwhy63h+hMkg0USofwD4gYPYeQME=
33
github.com/go-openapi/errors v0.22.8 h1:oP7sW7TWc3wFFjrzzj0nI83H2qMBkNjNfSd+XRejk/I=
44
github.com/go-openapi/errors v0.22.8/go.mod h1:BuUoHcYrU6E7V9gfj1I5wLQqgtIHnup/alXZ8KdgQ0w=
55
github.com/go-openapi/jsonpointer v1.0.0 h1:kR9tHqY0CtZaOPVFm622dPVNhrvYpwr4uCxgL3h1H8s=
@@ -8,34 +8,34 @@ github.com/go-openapi/jsonreference v1.0.0 h1:jlmTr6torcd1YgDQvSfNmRtKzYDO4FGBkr
88
github.com/go-openapi/jsonreference v1.0.0/go.mod h1:jtwdyGbJk0Xhe5Y+rwtglQP6Sb1WZST4rT32LWB+sv0=
99
github.com/go-openapi/loads v0.25.1 h1:toKQdIDLxlqfKLLGUUmUsiTd5/X0Chzvde9EGYQP/Ac=
1010
github.com/go-openapi/loads v0.25.1/go.mod h1:33Hen4tsKXHL45TyYojvfD5fZUFN4O1y4r/XhsRW2zc=
11-
github.com/go-openapi/spec v0.22.9 h1:/vKIFDcGKp0ktZWGbym/tJEWbk6/XOEmAVU0kqKMH+w=
12-
github.com/go-openapi/spec v0.22.9/go.mod h1:b/mNUYIOQOyIiUzUzXEE8xzyZqf93KvM9hQGP91yfl0=
11+
github.com/go-openapi/spec v0.22.10 h1:5cp1dq++t4U/4WCg6f1wqReZowUrJ5kl8Eri4SMl51s=
12+
github.com/go-openapi/spec v0.22.10/go.mod h1:aWRr+Ntv5tHoMQo0C1slTNLFo1FOYdZXFlUqViCN7yM=
1313
github.com/go-openapi/strfmt v0.27.0 h1:kbcTeaD9TXuXD0hhMXzuYa1sdTo6+dWGvwjW93E80IM=
1414
github.com/go-openapi/strfmt v0.27.0/go.mod h1:s/qhDqfY72irigXUGJmtgid2Rm+3tnz3k8hZaRmvWYc=
15-
github.com/go-openapi/swag/conv v0.29.0 h1:4+1TogWpOIzMPzVKrvx1BfqBYlApB7D7DW3EAWpwmp4=
16-
github.com/go-openapi/swag/conv v0.29.0/go.mod h1:ch1l7V87F6zQXuLs5s0RFvrro6aFvrVcfVXn2PTZnu8=
17-
github.com/go-openapi/swag/fileutils v0.29.0 h1:meobnn3MsAkF6XmJn6qw3hPeMAOhwf7XD5BOKeFzqWU=
18-
github.com/go-openapi/swag/fileutils v0.29.0/go.mod h1:/wofKYckbtRl2p3+EwQsosie5CT1B38+dQ+PS579BzI=
19-
github.com/go-openapi/swag/jsonutils v0.29.0 h1:Xgnf9g32ycQjQUnDxkhqLraH2FhitcSE3w7ayQB3TgA=
20-
github.com/go-openapi/swag/jsonutils v0.29.0/go.mod h1:5WYmjf6hJcBve+ArzBaUsYy4M1GXsgjIQTmwJKfZHrA=
21-
github.com/go-openapi/swag/jsonutils/fixtures_test v0.29.0 h1:bpSF6LFkJJVtaRtJCzbZADVPVHQYKPwPdKthOQA2/5o=
22-
github.com/go-openapi/swag/jsonutils/fixtures_test v0.29.0/go.mod h1:julgTUKZ9/D0j6O7GKajmRs+812FWxQg/mMpGunWSjg=
23-
github.com/go-openapi/swag/loading v0.29.0 h1:r1lg2DQbT1VgBwgiPYXBM059RNswFI6r36CC0QCcRGw=
24-
github.com/go-openapi/swag/loading v0.29.0/go.mod h1:l/Z4MNbom0jSqzvWJqK2VUUWEceBknGEuVbLHLq4KN0=
25-
github.com/go-openapi/swag/mangling v0.28.0 h1:pH8eyeNO9SLYsTMWJrurnNfKmDa28XrlA+HePVD53VM=
26-
github.com/go-openapi/swag/mangling v0.28.0/go.mod h1:jtBE2+V+3pILxOR7Vgce+Cwp6A2PgZbvVqfNntbVs0w=
27-
github.com/go-openapi/swag/pools v0.29.0 h1:uMQcoJeHJ8fWkdfEXJZMMpqk6hpfW8qTL5Q/IoRFFII=
28-
github.com/go-openapi/swag/pools v0.29.0/go.mod h1:leDcaghjkRAhCuCRv9NfJU5f0mjoU3cT/XZObhMk3pc=
29-
github.com/go-openapi/swag/stringutils v0.29.0 h1:/IEOuZ7PGJi6lqgH83dVt7/A9eHsDGEH1459lm+gpEo=
30-
github.com/go-openapi/swag/stringutils v0.29.0/go.mod h1:7fSqZ+z8Qc0tOfAAK0jVa5qFGrnIlRi6n7NeGGrr1vc=
31-
github.com/go-openapi/swag/typeutils v0.29.0 h1:HrWCYZeXVVNDo/7QQPRaYk33XeIDxksbxpalID3bWR8=
32-
github.com/go-openapi/swag/typeutils v0.29.0/go.mod h1:hxpgDZJVBkBsi/d3MIUosafoFdE5exaQRmVp0zwu3YE=
33-
github.com/go-openapi/swag/yamlutils v0.29.0 h1:JOKKuhMnBx4HYTM+kPEYw8S5YKKU9PnC4Mwb+c69BBA=
34-
github.com/go-openapi/swag/yamlutils v0.29.0/go.mod h1:/+FVozjFWZzku6mRz5U/Qmq5Yk8PLFxBLLWA/jHaxYE=
15+
github.com/go-openapi/swag/conv v0.29.1 h1:AC4Eh/5c/eUDOUCzzsRC9ghmFgOSBHeRMGIngY0ZUGA=
16+
github.com/go-openapi/swag/conv v0.29.1/go.mod h1:S1X7/ZrBEZOC0Wc8AGxjbcGS92l3WEjA7aPtpl+RaqM=
17+
github.com/go-openapi/swag/fileutils v0.29.1 h1:ZcPzMceVhU1WPbK6N1G6sNQKdd1CWJlf3cA08UHuoM0=
18+
github.com/go-openapi/swag/fileutils v0.29.1/go.mod h1:/wofKYckbtRl2p3+EwQsosie5CT1B38+dQ+PS579BzI=
19+
github.com/go-openapi/swag/jsonutils v0.29.1 h1:AFCxs0eQZ24/QyfhVHM2t49rMz7Vv3XCsZQI6yrNy+c=
20+
github.com/go-openapi/swag/jsonutils v0.29.1/go.mod h1:u3+sCfJpttDpcmS5kpm0yxL6GK0eWgODsx8Yw8fcqNM=
21+
github.com/go-openapi/swag/jsonutils/fixtures_test v0.29.1 h1:BiiXE31Bx9SfpsMmOQj5KYpUhTZBpLVriVhJDuLuY2o=
22+
github.com/go-openapi/swag/jsonutils/fixtures_test v0.29.1/go.mod h1:julgTUKZ9/D0j6O7GKajmRs+812FWxQg/mMpGunWSjg=
23+
github.com/go-openapi/swag/loading v0.29.1 h1:FCv5fG8UhTdDJa2R7w+5O9Ekpcbw7tt0nFWvmDKGBjc=
24+
github.com/go-openapi/swag/loading v0.29.1/go.mod h1:N0ESuem4p2oedKal8EJhciqnJ9Q9Wmt83L1CRB3Fouw=
25+
github.com/go-openapi/swag/mangling v0.29.1 h1:lHALtvYCdxVnRl4GrHmFPwfBTZYIObqdGNSKyu/8D6I=
26+
github.com/go-openapi/swag/mangling v0.29.1/go.mod h1:SAop9pB7PUjQ/CGCNf/JmCKTRK+GDO+RqE9UHqC/N6s=
27+
github.com/go-openapi/swag/pools v0.29.1 h1:NRogYxdEW9SjRM4mkAOji9iefO4MRXq3p/ZJcoQbUKg=
28+
github.com/go-openapi/swag/pools v0.29.1/go.mod h1:leDcaghjkRAhCuCRv9NfJU5f0mjoU3cT/XZObhMk3pc=
29+
github.com/go-openapi/swag/stringutils v0.29.1 h1:1ykunK7iJQk1uOO7+oUH1ukbsK85fFCOiCFMOVSY+F0=
30+
github.com/go-openapi/swag/stringutils v0.29.1/go.mod h1:7fSqZ+z8Qc0tOfAAK0jVa5qFGrnIlRi6n7NeGGrr1vc=
31+
github.com/go-openapi/swag/typeutils v0.29.1 h1:Nzv9nhnlLCRBPQqfOX+7lB6Guju370or8StT+lIOf6M=
32+
github.com/go-openapi/swag/typeutils v0.29.1/go.mod h1:hxpgDZJVBkBsi/d3MIUosafoFdE5exaQRmVp0zwu3YE=
33+
github.com/go-openapi/swag/yamlutils v0.29.1 h1:69w3tsBajm7MR/fejLy7HD/3J68Ys1SeeZMEzZ3w2sk=
34+
github.com/go-openapi/swag/yamlutils v0.29.1/go.mod h1:rgsp3vT/QdWzKwn43CigDwjOGIenPyTZMKnxEM8jZOA=
3535
github.com/go-openapi/testify/enable/yaml/v2 v2.6.1 h1:Jm+/ze2rMtbD98yen92AhATGLGREDYXG56Xr4gMjEtE=
3636
github.com/go-openapi/testify/enable/yaml/v2 v2.6.1/go.mod h1:YDPnwCRDu38/oJBVMBVXOUDiJ9cIeBHWvfImHaXqnv4=
37-
github.com/go-openapi/testify/v2 v2.6.1 h1:6CNJhTjMzgaeaH8WhshcsZNPIvRemiOcFpU7seO/y7Q=
38-
github.com/go-openapi/testify/v2 v2.6.1/go.mod h1:SgsVHtfooshd0tublTtJ50FPKhujf47YRqauXXOUxfw=
37+
github.com/go-openapi/testify/v2 v2.7.0 h1:bycOreEj6wfBvijg3YFogZ/sFjTCDmQnwSodSzHa3X8=
38+
github.com/go-openapi/testify/v2 v2.7.0/go.mod h1:SgsVHtfooshd0tublTtJ50FPKhujf47YRqauXXOUxfw=
3939
github.com/go-viper/mapstructure/v2 v2.5.0 h1:vM5IJoUAy3d7zRSVtIwQgBj7BiWtMPfmPEgAXnvj1Ro=
4040
github.com/go-viper/mapstructure/v2 v2.5.0/go.mod h1:oJDH3BJKyqBA2TXFhDsKDGDTlndYOZ6rGS0BRZIxGhM=
4141
github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=

‎schema.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,22 @@ func NewSchemaValidator(schema *spec.Schema, rootSchema any, root string, format
6161
o(opts)
6262
}
6363

64+
// the caller still owns this schema, and validation expands what it walks: work on a copy,
65+
// once, here - every validator below this one is then free to expand in place
66+
if !opts.ownSchemata && schema != nil {
67+
cloned, err := deepCloneSchema(*schema)
68+
if err != nil {
69+
panic(invalidSchemaProvidedMsg(err).Error())
70+
}
71+
72+
if rootSchema == schema {
73+
rootSchema = &cloned
74+
}
75+
76+
schema = &cloned
77+
opts.ownSchemata = true
78+
}
79+
6480
return newSchemaValidator(schema, rootSchema, rootPathFromString(root), formats, opts)
6581
}
6682

‎schema_option.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,15 @@ type SchemaValidatorOptions struct {
1818
recycleResult bool
1919
skipSchemataResult bool
2020
pathLoaderWithOptions func(string, ...loading.Option) (json.RawMessage, error)
21+
22+
// ownSchemata tells the validators that the schemata they walk are theirs to rewrite.
23+
//
24+
// Validation expands every schema carrying a $ref or an id, and expansion rewrites what it is
25+
// given. The copy that makes this safe is taken once, at the entry point - the whole document
26+
// in [SpecValidator.Validate], the single schema in [NewSchemaValidator] - because a copy per
27+
// schema multiplies with the recursion, and because parameters, path items and responses
28+
// expand too, not only schemata.
29+
ownSchemata bool
2130
}
2231

2332
// Option sets optional rules for schema validation.

‎spec.go‎

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

66
import (
7-
"bytes"
8-
"encoding/gob"
97
"encoding/json"
108
"fmt"
119
"slices"
@@ -99,6 +97,17 @@ func (s *SpecValidator) Validate(data any) (*Result, *Result) {
9997
errs.AddErrors(invalidDocumentMsg())
10098
return errs, warnings // no point in continuing
10199
}
100+
101+
// Validation expands what it walks - schemata, but also parameters, path items and responses -
102+
// and expansion rewrites what it is given. Take one copy of the whole document here and work
103+
// on that, so the caller gets back the document it handed over. Raw() still reads the bytes as
104+
// they were authored, so the checks below that go through them are unaffected.
105+
//
106+
// Cloning here rather than per schema is what keeps the cost flat: a copy taken inside
107+
// newSchemaValidator is paid again at every level of a recursive document.
108+
raw := sd.Raw()
109+
sd = sd.Pristine()
110+
s.schemaOptions.ownSchemata = true
102111
s.spec = sd
103112
s.analyzer = analysis.New(sd.Spec())
104113
// where each $ref sits, as authored: refs are reported against the
@@ -113,7 +122,7 @@ func (s *SpecValidator) Validate(data any) (*Result, *Result) {
113122

114123
// Raw spec unmarshalling errors
115124
var obj any
116-
if err := json.Unmarshal(sd.Raw(), &obj); err != nil {
125+
if err := json.Unmarshal(raw, &obj); err != nil {
117126
// NOTE: under normal conditions, the *load.Document has been already unmarshalled
118127
// So this one is just a paranoid check on the behavior of the spec package
119128
panic(InvalidDocumentError)
@@ -951,14 +960,15 @@ func (s *SpecValidator) expandedAnalyzer() *analysis.Spec {
951960
return s.analyzer
952961
}
953962

963+
// deepCloneSchema returns a copy of src that shares nothing with it.
964+
//
965+
// The copy goes through JSON, which is the form [spec.Schema] is defined by. gob drops any field
966+
// holding its zero value and flattens a pointer to what it points at, so a *float64 pointing at
967+
// 0 - "minimum": 0, which the JSON Schema meta-schema spells for every positiveInteger - came
968+
// back nil and the bound was lost. JSON is also the faster of the two on this model.
954969
func deepCloneSchema(src spec.Schema) (spec.Schema, error) {
955-
var b bytes.Buffer
956-
if err := gob.NewEncoder(&b).Encode(src); err != nil {
957-
return spec.Schema{}, err
958-
}
959-
960970
var dst spec.Schema
961-
if err := gob.NewDecoder(&b).Decode(&dst); err != nil {
971+
if err := jsonutils.FromDynamicJSON(src, &dst); err != nil {
962972
return spec.Schema{}, err
963973
}
964974

‎spec_test.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -942,3 +942,33 @@ func Test_2866(t *testing.T) {
942942

943943
require.NoError(t, Spec(doc, strfmt.Default))
944944
}
945+
946+
// TestSpec_DoesNotMutateDocument checks that validation leaves the caller's document as it was.
947+
//
948+
// newSchemaValidator expands a schema carrying a $ref, and several call sites hand it a schema
949+
// that belongs to the document the caller still holds - additionalProperties here, reached while
950+
// the default validator walks the specification. Expanding in place replaced the $ref with its
951+
// target, so callers that went on to flatten the document worked on a different document than the
952+
// one they loaded (go-swagger works around this by reloading the spec after validating).
953+
func TestSpec_DoesNotMutateDocument(t *testing.T) {
954+
path := filepath.Join("testdata", "bugs", "no-mutation", "fixture.json")
955+
956+
doc, err := loads.Spec(path)
957+
require.NoError(t, err)
958+
959+
before, err := json.Marshal(doc.Spec())
960+
require.NoError(t, err)
961+
962+
require.NoError(t, Spec(doc, strfmt.Default))
963+
964+
after, err := json.Marshal(doc.Spec())
965+
require.NoError(t, err)
966+
967+
assert.JSONEqf(t, string(before), string(after), "validation rewrote the caller's document")
968+
969+
// the $ref that used to be inlined
970+
bag := doc.Spec().Definitions["Bag"]
971+
require.NotNil(t, bag.AdditionalProperties)
972+
require.NotNil(t, bag.AdditionalProperties.Schema)
973+
assert.EqualT(t, "#/definitions/Item", bag.AdditionalProperties.Schema.Ref.String())
974+
}

‎testdata/bugs/no-mutation/fixture.json‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
{
2+
"swagger": "2.0",
3+
"info": {"title": "validate must not rewrite the caller's document", "version": "1.0.0"},
4+
"paths": {
5+
"/things": {
6+
"get": {
7+
"parameters": [{"name": "body", "in": "body", "schema": {"$ref": "#/definitions/Bag"}}],
8+
"responses": {"200": {"description": "ok"}}
9+
}
10+
}
11+
},
12+
"definitions": {
13+
"Bag": {
14+
"type": "object",
15+
"additionalProperties": {"$ref": "#/definitions/Item"},
16+
"default": {"anything": {"name": "x"}}
17+
},
18+
"Item": {"type": "object", "properties": {"name": {"type": "string"}}}
19+
}
20+
}

0 commit comments

Comments
 (0)