diff --git a/.golangci.yml b/.golangci.yml index b8875d7..c05f38e 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -10,6 +10,7 @@ linters: - gomodguard - gomodguard_v2 - exhaustruct + - exhaustruct_v5 - ireturn - nlreturn - nestif diff --git a/collection_format.go b/collection_format.go new file mode 100644 index 0000000..a77ba56 --- /dev/null +++ b/collection_format.go @@ -0,0 +1,131 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "fmt" + "strconv" + + "github.com/go-openapi/spec" +) + +// validateCollectionFormats warns about a collectionFormat that has no effect. +// +// collectionFormat says how to join the members of an array into one value on the wire — csv, ssv, +// tsv, pipes, or multi for a parameter repeated once per member: +// +// parameters: +// - name: tags +// in: query +// type: array +// items: { type: string } +// collectionFormat: pipes +// +// There is nothing to join when the type is not array, so a collectionFormat written on a string or +// an integer does nothing. Swagger 2.0 says the member "determines the format of the array if type +// array is used" and stops there — it never forbids writing it elsewhere, so this is a warning and +// the specification stays valid. +// +// The meta-schema already covers the rest of the collectionFormat rules, and covers them at every +// location: the value must be one of csv, ssv, tsv or pipes, widened with multi for a query or +// formData parameter, where repeating the parameter is possible. A body parameter cannot carry the +// member at all. None of that needs a rule here. +// +// Only a parameter, a header and an items carry a collectionFormat — [spec.SimpleSchema] holds it, +// and [spec.Schema] has no such member. So this walks what the operations of the document declare, +// the way [SpecValidator.validateItems] does, and never looks at a schema. A JSON schema validated +// on its own is untouched by this rule. +func (s *SpecValidator) validateCollectionFormats() *Result { + res := validatorPools.results.Borrow() + + operations := s.analyzer.Operations() + for _, method := range sortedKeys(operations) { + byPath := operations[method] + for _, path := range sortedKeys(byPath) { + op := byPath[path] + + for _, param := range paramHelp.safeExpandedParamsFor(path, method, op.ID, res, s) { + if param.In == swaggerBody { + // a body parameter describes itself with a schema, and the meta-schema + // forbids it a collectionFormat outright + continue + } + + at := s.parameterPath(path, method, param.In, param.Name) + in := fmt.Sprintf("parameter %q", param.Name) + checkCollectionFormat(at, param.CollectionFormat, param.Type, in, res) + checkItemsCollectionFormats(at, param.Items, in, res) + } + + for _, response := range responsesOf(op) { + at := responsePath(path, method, response.code) + for _, name := range sortedKeys(response.resp.Headers) { + header := response.resp.Headers[name] + headerAt := at.children(swaggerHeaders, name) + in := fmt.Sprintf("header %q", name) + checkCollectionFormat(headerAt, header.CollectionFormat, header.Type, in, res) + checkItemsCollectionFormats(headerAt, header.Items, in, res) + } + } + } + } + + return res +} + +// codedResponse is a response together with the code the operation files it under, "default" +// included. +type codedResponse struct { + code string + resp spec.Response +} + +// responsesOf lists the responses an operation declares, in a settled order: the default response +// first, then the status codes in ascending order. +func responsesOf(op *spec.Operation) []codedResponse { + if op == nil || op.Responses == nil { + return nil + } + + var responses []codedResponse + if op.Responses.Default != nil { + responses = append(responses, codedResponse{code: jsonDefault, resp: *op.Responses.Default}) + } + + for _, code := range sortedKeys(op.Responses.StatusCodeResponses) { + responses = append(responses, codedResponse{ + code: strconv.Itoa(code), + resp: op.Responses.StatusCodeResponses[code], + }) + } + + return responses +} + +// checkItemsCollectionFormats checks the items of a parameter or header, then the items of those +// items, as deep as the document nests them. +// +// Every level is named the same way in a message: an array of arrays that writes a pointless +// collectionFormat twice is one thing to fix, and the deeper location is reported only when the +// shallower one is sound. +func checkItemsCollectionFormats(at pathSegments, items *spec.Items, in string, res *Result) { + for items != nil { + at = at.child(jsonItems) + checkCollectionFormat(at, items.CollectionFormat, items.Type, "items of "+in, res) + items = items.Items + } +} + +// checkCollectionFormat warns when a collectionFormat is written on something that is not an array. +// +// A missing collectionFormat has nothing to answer for, and neither has a missing type: a document +// that leaves the type out is already reported by the meta-schema, and guessing what it meant here +// would only add noise. +func checkCollectionFormat(at pathSegments, collectionFormat, typ, in string, res *Result) { + if collectionFormat == "" || typ == "" || typ == arrayType { + return + } + + res.addWarningsAt(at.child(swaggerCollectionFormat), collectionFormatIgnoredMsg(collectionFormat, in, typ)) +} diff --git a/collection_format_test.go b/collection_format_test.go new file mode 100644 index 0000000..8e1d1e9 --- /dev/null +++ b/collection_format_test.go @@ -0,0 +1,242 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/go-openapi/analysis" + "github.com/go-openapi/loads" + "github.com/go-openapi/spec" + "github.com/go-openapi/strfmt" + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +// collectionFormatSpec builds a specification whose only operation declares the given parameters +// and the given headers on its 200 response. +func collectionFormatSpec(parameters, headers string) string { + return fmt.Sprintf(`{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": { + "/x": { + "get": { + "parameters": [%s], + "responses": {"200": {"description": "ok", "headers": {%s}}} + } + } + } + }`, parameters, headers) +} + +// collectionFormatValidatorFromJSON builds a SpecValidator wired the way Validate does, up to the +// point validateCollectionFormats needs. +func collectionFormatValidatorFromJSON(t *testing.T, doc string) *SpecValidator { + t.Helper() + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + s := NewSpecValidator(d.Schema(), strfmt.Default) + s.spec = d + s.analyzer = analysis.New(d.Spec()) + s.paramLocations = newParamLocations(d.Spec()) + + return s +} + +func TestValidateCollectionFormats(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + parameters string + headers string + wantWarnings []string + }{ + { + name: "collectionFormat on an array", + parameters: `{"name":"q","in":"query","type":"array","items":{"type":"string"},"collectionFormat":"pipes"}`, + }, + { + name: "array without a collectionFormat", + parameters: `{"name":"q","in":"query","type":"array","items":{"type":"string"}}`, + }, + { + name: "no collectionFormat anywhere", + parameters: `{"name":"q","in":"query","type":"string"}`, + }, + { + name: "collectionFormat on a string parameter", + parameters: `{"name":"q","in":"query","type":"string","collectionFormat":"csv"}`, + wantWarnings: []string{ + `collectionFormat "csv" is ignored in parameter "q": it joins the members of an array, and the type is "string"`, + }, + }, + { + name: "collectionFormat on an integer parameter", + parameters: `{"name":"q","in":"query","type":"integer","collectionFormat":"pipes"}`, + wantWarnings: []string{ + `collectionFormat "pipes" is ignored in parameter "q": it joins the members of an array, and the type is "integer"`, + }, + }, + { + name: "collectionFormat on a file parameter", + parameters: `{"name":"f","in":"formData","type":"file","collectionFormat":"csv"}`, + wantWarnings: []string{ + `collectionFormat "csv" is ignored in parameter "f": it joins the members of an array, and the type is "file"`, + }, + }, + { + name: "collectionFormat on the items of an array", + parameters: `{"name":"q","in":"query","type":"array","items":{"type":"string","collectionFormat":"ssv"}}`, + wantWarnings: []string{ + `collectionFormat "ssv" is ignored in items of parameter "q": it joins the members of an array, and the type is "string"`, + }, + }, + { + name: "collectionFormat on the items of an array of arrays", + parameters: `{"name":"q","in":"query","type":"array","items":{"type":"array","items":{"type":"string"},"collectionFormat":"tsv"}}`, + }, + { + name: "collectionFormat on a string response header", + headers: `"X":{"type":"string","collectionFormat":"csv"}`, + wantWarnings: []string{ + `collectionFormat "csv" is ignored in header "X": it joins the members of an array, and the type is "string"`, + }, + }, + { + name: "collectionFormat on an array response header", + headers: `"X":{"type":"array","items":{"type":"string"},"collectionFormat":"csv"}`, + }, + { + name: "collectionFormat on the items of a response header", + headers: `"X":{"type":"array","items":{"type":"integer","collectionFormat":"pipes"}}`, + wantWarnings: []string{ + `collectionFormat "pipes" is ignored in items of header "X": it joins the members of an array, and the type is "integer"`, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + doc := collectionFormatSpec(tc.parameters, tc.headers) + res := collectionFormatValidatorFromJSON(t, doc).validateCollectionFormats() + + assert.Equal(t, tc.wantWarnings, nonEmpty(warningMessages(res))) + assert.Empty(t, errorMessages(res), "the rule reports warnings only") + }) + } +} + +// TestCollectionFormatKeepsSpecValid holds the rule to a warning: Swagger 2.0 says where +// collectionFormat applies without forbidding it elsewhere, so a specification that writes one on a +// string stays valid. +func TestCollectionFormatKeepsSpecValid(t *testing.T) { + t.Parallel() + + doc := collectionFormatSpec(`{"name":"q","in":"query","type":"string","collectionFormat":"csv"}`, "") + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + require.NoError(t, Spec(d, strfmt.Default)) + + _, warns := NewSpecValidator(d.Schema(), strfmt.Default).Validate(d) + assert.SliceContainsT(t, errorMessages(warns), + `collectionFormat "csv" is ignored in parameter "q": it joins the members of an array, and the type is "string"`) +} + +// TestCollectionFormatLocations pins down where the warning says it happened. +func TestCollectionFormatLocations(t *testing.T) { + t.Parallel() + + doc := collectionFormatSpec( + `{"name":"q","in":"query","type":"array","items":{"type":"string","collectionFormat":"ssv"}}`, + `"X":{"type":"string","collectionFormat":"csv"}`, + ) + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + _, warns := NewSpecValidator(d.Schema(), strfmt.Default).Validate(d) + + pointers := pointersOf(warns.LocatedErrors()) + assert.SliceContainsT(t, pointers, "/paths/~1x/get/parameters/0/items/collectionFormat") + assert.SliceContainsT(t, pointers, "/paths/~1x/get/responses/200/headers/X/collectionFormat") +} + +// TestCollectionFormatThroughRef covers a parameter reached through a $ref: the walk sees it +// expanded, so the warning is reported just as it is for one written in place. +func TestCollectionFormatThroughRef(t *testing.T) { + t.Parallel() + + doc := `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "parameters": { + "Q": {"name": "q", "in": "query", "type": "string", "collectionFormat": "csv"} + }, + "paths": { + "/x": { + "get": { + "parameters": [{"$ref": "#/parameters/Q"}], + "responses": {"200": {"description": "ok"}} + } + } + } + }` + + res := collectionFormatValidatorFromJSON(t, doc).validateCollectionFormats() + assert.Equal(t, []string{ + `collectionFormat "csv" is ignored in parameter "q": it joins the members of an array, and the type is "string"`, + }, warningMessages(res)) +} + +// TestCollectionFormatSkipsBodyParameters guards the one location the meta-schema handles on its +// own: a body parameter may not carry a collectionFormat at all, and that is an error rather than +// this warning. +func TestCollectionFormatSkipsBodyParameters(t *testing.T) { + t.Parallel() + + doc := collectionFormatSpec(`{"name":"b","in":"body","schema":{"type":"string"}}`, "") + + res := collectionFormatValidatorFromJSON(t, doc).validateCollectionFormats() + assert.Empty(t, warningMessages(res)) +} + +// TestCollectionFormatIsNotASchemaKeyword holds the rule to what it walks. collectionFormat is a +// member of [spec.SimpleSchema], which a parameter, a header and an items carry; [spec.Schema] has +// none, so validating an ordinary JSON schema can never reach this rule. +// +// The guard is on the type, not on the walk: a compile-time field access is what would break if a +// later version of go-openapi/spec moved the member onto Schema. +func TestCollectionFormatIsNotASchemaKeyword(t *testing.T) { + t.Parallel() + + var ( + _ = spec.Items{}.CollectionFormat + _ = spec.Header{}.CollectionFormat + _ = spec.Parameter{}.CollectionFormat + ) + + raw, err := json.Marshal(spec.Schema{ + SchemaProps: spec.SchemaProps{Type: spec.StringOrArray{stringType}}, + }) + require.NoError(t, err) + + var members map[string]any + require.NoError(t, json.Unmarshal(raw, &members)) + assert.MapNotContainsT(t, members, swaggerCollectionFormat) + + // and validating data against a schema reports nothing of the sort + schema := new(spec.Schema) + require.NoError(t, json.Unmarshal([]byte(`{"type": "string"}`), schema)) + require.NoError(t, AgainstSchema(schema, "a,b,c", strfmt.Default)) +} diff --git a/discriminator.go b/discriminator.go new file mode 100644 index 0000000..7a4ab41 --- /dev/null +++ b/discriminator.go @@ -0,0 +1,156 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "slices" + + "github.com/go-openapi/spec" +) + +// validateDiscriminators checks the discriminator of every definition, and of every schema a +// definition holds inline. +// +// A discriminator names the property that tells subtypes apart: +// +// Pet: +// discriminator: petType +// required: [ petType ] +// properties: +// petType: { type: string } +// +// Swagger 2.0 asks two things of that property: the schema must define it, and must list it as +// required. Both matter for the same reason — an instance carries its subtype in that property, +// so a subtype cannot be resolved from an instance that has nowhere to put the value, or that is +// free to leave it out. +// +// The JSON meta-schema types discriminator as a plain string and never compares it against +// properties or required, so it can express neither check. +// +// A property contributed by an allOf member counts as defined, and one that member requires counts +// as required: [SpecValidator.declaresProperty] already reads a composed definition that way for +// the required rule, and a discriminator resolves against the instance the whole composition +// describes. +// +// The third clause of the rule — the value must name this schema or one that inherits it — +// constrains the data, not the document, so it belongs to schema validation rather than here. +func (s *SpecValidator) validateDiscriminators() *Result { + res := validatorPools.results.Borrow() + definitions := s.spec.Spec().Definitions + + for _, name := range sortedKeys(definitions) { + schema := definitions[name] + s.walkDiscriminators(newPathSegments(swaggerDefinitions, name), &schema, res) + } + + return res +} + +// walkDiscriminators checks the discriminator of a schema, then of every schema it holds inline. +// +// A schema written as a $ref is left alone: it is checked where it is defined, and following it +// here would report the same fault twice and, for a recursive definition, would not terminate. +// This mirrors [SpecValidator.walkRequired]. +func (s *SpecValidator) walkDiscriminators(at pathSegments, v *spec.Schema, res *Result) { + if v == nil || v.Ref.String() != "" { + return + } + + s.checkDiscriminator(at, v, res) + + for _, name := range sortedKeys(v.Properties) { + held := v.Properties[name] + s.walkDiscriminators(at.structuralChild(jsonProperties).child(name), &held, res) + } + + for _, pattern := range sortedKeys(v.PatternProperties) { + held := v.PatternProperties[pattern] + s.walkDiscriminators(at.structuralChild(jsonPatternProperties).child(pattern), &held, res) + } + + if v.Items != nil { + if v.Items.Schema != nil { + s.walkDiscriminators(at.child(jsonItems), v.Items.Schema, res) + } + for i := range v.Items.Schemas { + s.walkDiscriminators(at.child(jsonItems).item(i), &v.Items.Schemas[i], res) + } + } + + if v.AdditionalProperties != nil && v.AdditionalProperties.Schema != nil { + s.walkDiscriminators(at.child(jsonAdditionalProperties), v.AdditionalProperties.Schema, res) + } + + for _, composition := range []struct { + keyword string + members []spec.Schema + }{ + {jsonAllOf, v.AllOf}, + {jsonAnyOf, v.AnyOf}, + {jsonOneOf, v.OneOf}, + } { + for i := range composition.members { + s.walkDiscriminators(at.child(composition.keyword).item(i), &composition.members[i], res) + } + } + + if v.Not != nil { + s.walkDiscriminators(at.child(jsonNot), v.Not, res) + } +} + +// checkDiscriminator checks the discriminator a single schema declares. A schema without one has +// nothing to answer for. +// +// Both findings are reported against the discriminator itself, which is the entry a reader has to +// go and amend, and both are reported when they apply: a discriminator naming a property that is +// neither defined nor required is two separate slips to fix. +func (s *SpecValidator) checkDiscriminator(at pathSegments, v *spec.Schema, res *Result) { + if v.Discriminator == "" { + return + } + + of := identify(at).name + discriminatorAt := at.child(jsonDiscriminator) + + if _, declared := s.declaresProperty(v, v.Discriminator, maxCompositionHops); !declared { + res.addErrorsAt(discriminatorAt, discriminatorNotDefinedMsg(v.Discriminator, of)) + } + + if !s.requiresProperty(v, v.Discriminator, maxCompositionHops) { + res.addErrorsAt(discriminatorAt, discriminatorNotRequiredMsg(v.Discriminator, of)) + } +} + +// requiresProperty reports whether a schema, or any schema composed into it by allOf, lists the +// named property as required. +// +// It is the required-list counterpart of [SpecValidator.declaresProperty], and follows allOf the +// same way, including the local $ref an allOf member may be written as. +func (s *SpecValidator) requiresProperty(v *spec.Schema, name string, hops int) bool { + if v == nil || hops <= 0 { + return false + } + + if slices.Contains(v.Required, name) { + return true + } + + for i := range v.AllOf { + member := &v.AllOf[i] + if member.Ref.String() != "" { + resolved, err := s.resolveRef(&member.Ref) + if err != nil { + continue + } + member = resolved + } + + if s.requiresProperty(member, name, hops-1) { + return true + } + } + + return false +} diff --git a/discriminator_test.go b/discriminator_test.go new file mode 100644 index 0000000..ef2b420 --- /dev/null +++ b/discriminator_test.go @@ -0,0 +1,275 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/go-openapi/analysis" + "github.com/go-openapi/loads" + "github.com/go-openapi/strfmt" + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +// discriminatorSpec builds a specification holding the given definitions and nothing else of note. +func discriminatorSpec(definitions string) string { + return fmt.Sprintf(`{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": {}, + "definitions": %s + }`, definitions) +} + +// discriminatorValidatorFromJSON builds a SpecValidator wired the way Validate does, up to the +// point validateDiscriminators needs. +func discriminatorValidatorFromJSON(t *testing.T, doc string) *SpecValidator { + t.Helper() + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + s := NewSpecValidator(d.Schema(), strfmt.Default) + s.spec = d + s.analyzer = analysis.New(d.Spec()) + + return s +} + +func TestValidateDiscriminators(t *testing.T) { + t.Parallel() + + const ( + petTypeNotDefined = `discriminator "petType" of "Pet" is not defined as a property of that schema` + petTypeNotRequired = `discriminator "petType" of "Pet" is not in the required property list` + ) + + tests := []struct { + name string + defs string + wantErrors []string + }{ + { + name: "discriminator defined and required", + defs: `{ + "Pet": { + "type": "object", + "discriminator": "petType", + "required": ["petType"], + "properties": {"petType": {"type": "string"}} + } + }`, + }, + { + name: "no discriminator at all", + defs: `{"Pet": {"type": "object", "properties": {"name": {"type": "string"}}}}`, + }, + { + name: "discriminator names an undeclared property", + defs: `{ + "Pet": { + "type": "object", + "discriminator": "petType", + "required": ["petType"], + "properties": {"name": {"type": "string"}} + } + }`, + wantErrors: []string{petTypeNotDefined}, + }, + { + name: "discriminator property is optional", + defs: `{ + "Pet": { + "type": "object", + "discriminator": "petType", + "properties": {"petType": {"type": "string"}} + } + }`, + wantErrors: []string{petTypeNotRequired}, + }, + { + name: "discriminator neither defined nor required reports both", + defs: `{ + "Pet": { + "type": "object", + "discriminator": "petType", + "properties": {"name": {"type": "string"}} + } + }`, + wantErrors: []string{ + petTypeNotDefined, + petTypeNotRequired, + }, + }, + { + name: "property and required contributed by an allOf $ref", + defs: `{ + "Base": { + "type": "object", + "required": ["petType"], + "properties": {"petType": {"type": "string"}} + }, + "Pet": { + "discriminator": "petType", + "allOf": [ + {"$ref": "#/definitions/Base"}, + {"type": "object", "properties": {"name": {"type": "string"}}} + ] + } + }`, + }, + { + name: "property contributed by an inline allOf member", + defs: `{ + "Pet": { + "discriminator": "petType", + "allOf": [ + {"type": "object", "required": ["petType"], "properties": {"petType": {"type": "string"}}} + ] + } + }`, + }, + { + name: "allOf contributes the property but nothing requires it", + defs: `{ + "Base": {"type": "object", "properties": {"petType": {"type": "string"}}}, + "Pet": { + "discriminator": "petType", + "allOf": [{"$ref": "#/definitions/Base"}] + } + }`, + wantErrors: []string{petTypeNotRequired}, + }, + { + name: "several definitions are all checked, in name order", + defs: `{ + "Alpha": {"type": "object", "discriminator": "kind", "required": ["kind"]}, + "Beta": {"type": "object", "discriminator": "sort", "required": ["sort"]} + }`, + wantErrors: []string{ + `discriminator "kind" of "Alpha" is not defined as a property of that schema`, + `discriminator "sort" of "Beta" is not defined as a property of that schema`, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + res := discriminatorValidatorFromJSON(t, discriminatorSpec(tc.defs)).validateDiscriminators() + assert.Equal(t, tc.wantErrors, nonEmpty(errorMessages(res))) + assert.Empty(t, warningMessages(res), "the rule reports errors only") + }) + } +} + +// TestDiscriminatorInNestedSchema covers a discriminator that sits below a definition rather than +// on it: the same slip, wherever it is written. +func TestDiscriminatorInNestedSchema(t *testing.T) { + t.Parallel() + + doc := discriminatorSpec(`{ + "Zoo": { + "type": "object", + "properties": { + "resident": { + "type": "object", + "discriminator": "petType", + "properties": {"name": {"type": "string"}} + } + } + } + }`) + + res := discriminatorValidatorFromJSON(t, doc).validateDiscriminators() + assert.Equal(t, []string{ + `discriminator "petType" of "Zoo.resident" is not defined as a property of that schema`, + `discriminator "petType" of "Zoo.resident" is not in the required property list`, + }, errorMessages(res)) +} + +// TestDiscriminatorBehindRefCheckedOnce guards the choice not to follow a $ref: the fault is +// reported where the definition is written, not again at every schema pointing at it. +func TestDiscriminatorBehindRefCheckedOnce(t *testing.T) { + t.Parallel() + + doc := discriminatorSpec(`{ + "Pet": {"type": "object", "discriminator": "petType"}, + "First": {"$ref": "#/definitions/Pet"}, + "Second": {"type": "object", "properties": {"pet": {"$ref": "#/definitions/Pet"}}} + }`) + + res := discriminatorValidatorFromJSON(t, doc).validateDiscriminators() + assert.Equal(t, []string{ + `discriminator "petType" of "Pet" is not defined as a property of that schema`, + `discriminator "petType" of "Pet" is not in the required property list`, + }, errorMessages(res)) +} + +// TestDiscriminatorRecursiveDefinitionTerminates guards the same choice against the reason it was +// made: a definition holding itself would not terminate if the walk followed $ref. +func TestDiscriminatorRecursiveDefinitionTerminates(t *testing.T) { + t.Parallel() + + doc := discriminatorSpec(`{ + "Node": { + "type": "object", + "discriminator": "kind", + "required": ["kind"], + "properties": { + "kind": {"type": "string"}, + "child": {"$ref": "#/definitions/Node"} + } + } + }`) + + res := discriminatorValidatorFromJSON(t, doc).validateDiscriminators() + assert.Empty(t, errorMessages(res)) +} + +// TestDiscriminatorLocations pins down where a discriminator finding says it happened. The +// pointers come from a full Validate, which trims a location to a node the document holds. +func TestDiscriminatorLocations(t *testing.T) { + t.Parallel() + + doc := discriminatorSpec(`{ + "Pet": {"type": "object", "discriminator": "petType", "required": ["petType"]}, + "Zoo": { + "type": "object", + "properties": { + "resident": {"type": "object", "discriminator": "kind", "required": ["kind"]} + } + } + }`) + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + validator := NewSpecValidator(d.Schema(), strfmt.Default) + validator.SetContinueOnErrors(true) + errs, _ := validator.Validate(d) + + pointers := pointersOf(errs.LocatedErrors()) + assert.SliceContainsT(t, pointers, "/definitions/Pet/discriminator") + assert.SliceContainsT(t, pointers, "/definitions/Zoo/properties/resident/discriminator") +} + +// TestDiscriminatorMakesSpecInvalid checks that the rule reaches Spec, the package's front door, +// rather than only the validator method the other tests call. +func TestDiscriminatorMakesSpecInvalid(t *testing.T) { + t.Parallel() + + doc := discriminatorSpec(`{"Pet": {"type": "object", "discriminator": "petType"}}`) + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + err = Spec(d, strfmt.Default) + require.Error(t, err) + assert.StringContainsT(t, err.Error(), `discriminator "petType" of "Pet"`) +} diff --git a/doc.go b/doc.go index 5218ec8..f2d02d1 100644 --- a/doc.go +++ b/doc.go @@ -12,6 +12,11 @@ // Validates a spec document (from JSON or YAML) against the JSON schema for swagger, // then checks a number of extra rules that can't be expressed in JSON schema. // +// The lists below hold the extra rules only. The meta-schema already settles a great deal on its +// own, and where it does, no rule is repeated here: collectionFormat, say, must be one of csv, ssv, +// tsv or pipes, widened with multi for a query or formData parameter, and a body parameter may not +// carry one at all — all of that comes out of the meta-schema, at every location it applies to. +// // Entry points: // // - Spec() @@ -25,6 +30,9 @@ // [x] path uniqueness: each api path should be non-verbatim (account for path param names) unique per method. Validation can be laxed by disabling StrictPathParamUniqueness. // [x] each security reference should contain only unique scopes // [x] each security scope in a security definition should be unique +// [x] a discriminator must name a property the schema defines and lists as required +// [x] each security requirement must name a scheme declared in securityDefinitions +// [x] only an oauth2 security requirement may list scopes: every other scheme type must list none // [x] parameters in path must be unique // [x] each path parameter must correspond to a parameter placeholder and vice versa // [x] each referenceable definition must have references @@ -47,6 +55,8 @@ // [x] unsupported validation of examples on non-JSON media types // [x] examples in response without schema // [x] readOnly properties should not be required +// [x] an oauth2 security requirement names a scope its security scheme does not declare +// [x] collectionFormat is written on a parameter, header or items whose type is not array // // # Validating a schema // @@ -69,8 +79,11 @@ // [ ] errors and warnings are not reported with key/line number in spec // [ ] default values and examples on responses only support application/json producer type // [ ] invalid numeric constraints (such as Minimum, etc..) are not checked except for default and example values -// [ ] rules for collectionFormat are not implemented -// [ ] no validation rule for polymorphism support (discriminator) [not done here] // [ ] valid js ECMA regexp not supported by Go regexp engine are considered invalid // [ ] arbitrary large numbers are not supported: max is math.MaxFloat64 +// +// Left out by design, rather than pending: a discriminator value is not checked against the schema +// names it may take. That clause of the swagger rule constrains an instance rather than the +// document, so there is nothing in a specification for [SpecValidator] to read, and checking it +// would mean resolving polymorphic payloads, which this package does not do. package validate diff --git a/helpers.go b/helpers.go index 62deb97..ea71436 100644 --- a/helpers.go +++ b/helpers.go @@ -42,6 +42,7 @@ const ( jsonRequired = "required" jsonRef = "$ref" jsonDefault = "default" + jsonDiscriminator = "discriminator" jsonAllOf = "allOf" jsonAnyOf = "anyOf" @@ -50,12 +51,17 @@ const ( jsonAdditionalItems = "additionalItems" jsonAdditionalProperties = "additionalProperties" - swaggerPaths = "paths" - swaggerDefinitions = "definitions" - swaggerResponses = "responses" - swaggerParameters = "parameters" - swaggerHeaders = "headers" - swaggerOperationID = "operationId" + swaggerPaths = "paths" + swaggerDefinitions = "definitions" + swaggerResponses = "responses" + swaggerParameters = "parameters" + swaggerHeaders = "headers" + swaggerOperationID = "operationId" + swaggerSecurity = "security" + swaggerCollectionFormat = "collectionFormat" + + // securitySchemeOAuth2 is the only security scheme type whose requirements carry scopes. + securitySchemeOAuth2 = "oauth2" jsonMimeApplicationJSON = "application/json" ) diff --git a/security_requirements.go b/security_requirements.go new file mode 100644 index 0000000..b6c10f3 --- /dev/null +++ b/security_requirements.go @@ -0,0 +1,99 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "strings" + + "github.com/go-openapi/spec" +) + +// validateSecurityRequirements checks the security requirements declared by the document and by +// each of its operations against the security definitions. +// +// A requirement names a security scheme and lists the scopes an operation needs from it: +// +// security: +// - petstore_auth: [ "write:pets" ] +// - api_key: [] +// +// Three rules apply: +// +// - the name must be declared in securityDefinitions (error) +// - only an oauth2 requirement carries scopes; every other scheme type must list none (error) +// - an oauth2 requirement should only name scopes its scheme declares (warning) +// +// The JSON meta-schema types a requirement as an object of string arrays and never reads +// securityDefinitions, so it can express none of the three. +// +// An empty requirement object ({}) names no scheme and passes. So does an empty security array, +// which an operation uses to drop the requirements the document sets for every operation. +func (s *SpecValidator) validateSecurityRequirements() *Result { + res := validatorPools.results.Borrow() + definitions := s.spec.Spec().SecurityDefinitions + + res.Merge(checkSecurityRequirements( + newPathSegments(swaggerSecurity), + s.spec.Spec().Security, + definitions, + )) + + operations := s.expandedAnalyzer().Operations() + for _, method := range sortedKeys(operations) { + byPath := operations[method] + for _, path := range sortedKeys(byPath) { + op := byPath[path] + if op == nil { + continue + } + + res.Merge(checkSecurityRequirements( + operationPath(path, method).child(swaggerSecurity), + op.Security, + definitions, + )) + } + } + + return res +} + +// checkSecurityRequirements checks the list of security requirements held at the given location. +// +// Requirements are checked in the order the document lists them, and the schemes one requirement +// names in sorted order: a requirement is a map, so the document's own order is lost (see +// [sortedKeys]). +func checkSecurityRequirements(at pathSegments, requirements []map[string][]string, definitions spec.SecurityDefinitions) *Result { + res := validatorPools.results.Borrow() + + for i, requirement := range requirements { + for _, name := range sortedKeys(requirement) { + scopes := requirement[name] + schemeAt := at.item(i).child(name) + + scheme, isDeclared := definitions[name] + if !isDeclared || scheme == nil { + res.addErrorsAt(schemeAt, securitySchemeNotDeclaredMsg(name)) + + continue + } + + if scheme.Type != securitySchemeOAuth2 { + if len(scopes) > 0 { + res.addErrorsAt(schemeAt, securityScopesNotEmptyMsg(name, strings.Join(scopes, ", "), scheme.Type)) + } + + continue + } + + for _, scope := range scopes { + if _, isKnown := scheme.Scopes[scope]; !isKnown { + res.addWarningsAt(schemeAt, securityScopeNotDeclaredMsg(name, scope)) + } + } + } + } + + return res +} diff --git a/security_requirements_test.go b/security_requirements_test.go new file mode 100644 index 0000000..9944c7d --- /dev/null +++ b/security_requirements_test.go @@ -0,0 +1,204 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/go-openapi/analysis" + "github.com/go-openapi/loads" + "github.com/go-openapi/strfmt" + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +// securitySpec builds a specification whose security definitions cover the three scheme shapes a +// requirement can name: an apiKey, a basic auth, and an oauth2 scheme declaring one scope. +// +// rootSecurity and opSecurity are spliced in as authored JSON so that a test can write an empty +// array, an empty requirement object, or no member at all. +func securitySpec(rootSecurity, opSecurity string) string { + return fmt.Sprintf(`{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "securityDefinitions": { + "api_key": {"type": "apiKey", "name": "api_key", "in": "header"}, + "basic_auth": {"type": "basic"}, + "petstore_auth": { + "type": "oauth2", + "flow": "implicit", + "authorizationUrl": "https://example.com/auth", + "scopes": {"read:pets": "read your pets"} + } + }, + %s + "paths": { + "/pets": { + "get": { + "responses": {"200": {"description": "ok"}}%s + } + } + } + }`, rootSecurity, opSecurity) +} + +func rootSecurity(requirements string) string { + return `"security": ` + requirements + `,` +} + +func opSecurity(requirements string) string { + return `, "security": ` + requirements +} + +// securityValidatorFromJSON builds a SpecValidator wired the way Validate does, up to the point +// validateSecurityRequirements needs: the document and an analyzer over it. +func securityValidatorFromJSON(t *testing.T, doc string) *SpecValidator { + t.Helper() + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + s := NewSpecValidator(d.Schema(), strfmt.Default) + s.spec = d + s.analyzer = analysis.New(d.Spec()) + + return s +} + +func TestValidateSecurityRequirements(t *testing.T) { + t.Parallel() + + const undeclaredScheme = `security requirement "unknown_scheme" is not declared in securityDefinitions` + + tests := []struct { + name string + doc string + wantErrors []string + wantWarnings []string + }{ + { + name: "scheme declared in securityDefinitions", + doc: securitySpec(rootSecurity(`[{"api_key": []}]`), opSecurity(`[{"petstore_auth": ["read:pets"]}]`)), + }, + { + name: "undeclared scheme at the document level", + doc: securitySpec(rootSecurity(`[{"unknown_scheme": []}]`), ""), + wantErrors: []string{undeclaredScheme}, + }, + { + name: "undeclared scheme on an operation", + doc: securitySpec("", opSecurity(`[{"unknown_scheme": []}]`)), + wantErrors: []string{undeclaredScheme}, + }, + { + name: "scopes on an apiKey scheme", + doc: securitySpec("", opSecurity(`[{"api_key": ["read:pets"]}]`)), + wantErrors: []string{ + `security requirement "api_key" lists scopes (read:pets), but the security scheme it names is of type "apiKey": only oauth2 requirements carry scopes`, + }, + }, + { + name: "scopes on a basic scheme, at the document level", + doc: securitySpec(rootSecurity(`[{"basic_auth": ["a", "b"]}]`), ""), + wantErrors: []string{ + `security requirement "basic_auth" lists scopes (a, b), but the security scheme it names is of type "basic": only oauth2 requirements carry scopes`, + }, + }, + { + name: "oauth2 scope the scheme does not declare", + doc: securitySpec("", opSecurity(`[{"petstore_auth": ["read:pets", "write:pets"]}]`)), + wantWarnings: []string{ + `security requirement "petstore_auth" requires scope "write:pets", which the security scheme does not declare`, + }, + }, + { + name: "empty requirement object makes security optional", + doc: securitySpec(rootSecurity(`[{"api_key": []}, {}]`), ""), + }, + { + name: "empty security array drops the document requirements", + doc: securitySpec(rootSecurity(`[{"api_key": []}]`), opSecurity(`[]`)), + }, + { + name: "no security member at all", + doc: securitySpec("", ""), + }, + { + name: "several requirements are all checked", + doc: securitySpec(rootSecurity(`[{"first_unknown": []}, {"second_unknown": []}]`), ""), + wantErrors: []string{ + `security requirement "first_unknown" is not declared in securityDefinitions`, + `security requirement "second_unknown" is not declared in securityDefinitions`, + }, + }, + { + name: "several schemes in one requirement are all checked", + doc: securitySpec("", opSecurity(`[{"api_key": ["nope"], "unknown_scheme": []}]`)), + wantErrors: []string{ + `security requirement "api_key" lists scopes (nope), but the security scheme it names is of type "apiKey": only oauth2 requirements carry scopes`, + undeclaredScheme, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + res := securityValidatorFromJSON(t, tc.doc).validateSecurityRequirements() + assert.Equal(t, tc.wantErrors, nonEmpty(errorMessages(res))) + assert.Equal(t, tc.wantWarnings, nonEmpty(warningMessages(res))) + }) + } +} + +// nonEmpty reports an empty list of messages as nil, so that a test case may leave its +// expectation out altogether. +func nonEmpty(messages []string) []string { + if len(messages) == 0 { + return nil + } + + return messages +} + +// TestSecurityRequirementLocations pins down where a security finding says it happened. The +// pointers are produced by a full Validate, which is what trims a location down to a node the +// document holds. +func TestSecurityRequirementLocations(t *testing.T) { + t.Parallel() + + doc := securitySpec( + rootSecurity(`[{"api_key": []}, {"unknown_scheme": []}]`), + opSecurity(`[{"basic_auth": ["nope"]}, {"petstore_auth": ["write:pets"]}]`), + ) + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + errs, warns := NewSpecValidator(d.Schema(), strfmt.Default).Validate(d) + + assert.SliceContainsT(t, pointersOf(errs.LocatedErrors()), "/security/1/unknown_scheme") + assert.SliceContainsT(t, pointersOf(errs.LocatedErrors()), "/paths/~1pets/get/security/0/basic_auth") + assert.SliceContainsT(t, pointersOf(warns.LocatedErrors()), "/paths/~1pets/get/security/1/petstore_auth") +} + +// TestSecurityScopeWarningKeepsSpecValid guards the choice made for the third rule: an oauth2 +// requirement naming an undeclared scope warns, and the specification stays valid. +func TestSecurityScopeWarningKeepsSpecValid(t *testing.T) { + t.Parallel() + + doc := securitySpec("", opSecurity(`[{"petstore_auth": ["write:pets"]}]`)) + + d, err := loads.Analyzed(json.RawMessage(doc), "") + require.NoError(t, err) + + require.NoError(t, Spec(d, strfmt.Default)) + + _, warns := NewSpecValidator(d.Schema(), strfmt.Default).Validate(d) + assert.SliceContainsT(t, errorMessages(warns), + `security requirement "petstore_auth" requires scope "write:pets", which the security scheme does not declare`) +} diff --git a/spec.go b/spec.go index 0dc0dd3..fea2bc0 100644 --- a/spec.go +++ b/spec.go @@ -10,7 +10,6 @@ import ( "fmt" "slices" "sort" - "strconv" "strings" "github.com/go-openapi/analysis" @@ -31,7 +30,6 @@ import ( // // - Proposal for enhancement: $ref should not have siblings // - Proposal for enhancement: make sure documentation reflects all checks and warnings -// - Proposal for enhancement: check on discriminators // - Proposal for enhancement: explicit message on unsupported keywords (better than "forbidden property"...) // - Proposal for enhancement: full list of unresolved refs // - Proposal for enhancement: validate numeric constraints (issue#581): this should be handled like defaults and examples @@ -153,6 +151,9 @@ func (s *SpecValidator) Validate(data any) (*Result, *Result) { errs.Merge(s.validateDuplicatePropertyNames()) // error - errs.Merge(s.validateParameters()) // error - errs.Merge(s.validateItems()) // error - + errs.Merge(s.validateSecurityRequirements()) // error and warning + errs.Merge(s.validateDiscriminators()) // error - + errs.Merge(s.validateCollectionFormats()) // warning only // Properties in required definition MUST validate their schema // Properties SHOULD NOT be declared as both required and readOnly (warning) @@ -458,26 +459,7 @@ func (s *SpecValidator) validateItems() *Result { } } - type codedResponse struct { - code string - resp spec.Response - } - var responses []codedResponse - if op.Responses != nil { - if op.Responses.Default != nil { - responses = append(responses, codedResponse{code: jsonDefault, resp: *op.Responses.Default}) - } - if op.Responses.StatusCodeResponses != nil { - for _, code := range sortedKeys(op.Responses.StatusCodeResponses) { - responses = append(responses, codedResponse{ - code: strconv.Itoa(code), - resp: op.Responses.StatusCodeResponses[code], - }) - } - } - } - - for _, resp := range responses { + for _, resp := range responsesOf(op) { at := responsePath(path, method, resp.code) // Response headers with array for _, hn := range sortedKeys(resp.resp.Headers) { diff --git a/spec_messages.go b/spec_messages.go index 0a0739a..541af4d 100644 --- a/spec_messages.go +++ b/spec_messages.go @@ -44,6 +44,15 @@ const ( // DefaultValueInDoesNotValidateError ... DefaultValueInDoesNotValidateError = "in operation %q, default value in %s does not validate its schema" + // DiscriminatorNotDefinedError indicates a schema whose discriminator names a property the + // schema does not declare. A discriminator tells subtypes apart by the value of that property, + // so an instance has nowhere to carry the value when the property is not declared. + DiscriminatorNotDefinedError = "discriminator %q of %q is not defined as a property of that schema" + + // DiscriminatorNotRequiredError indicates a schema whose discriminator property is declared but + // left optional. An instance that omits it cannot be resolved to a subtype. + DiscriminatorNotRequiredError = "discriminator %q of %q is not in the required property list" + // DuplicateParamNameError ... DuplicateParamNameError = "duplicate parameter name %q for %q in operation %q" @@ -136,6 +145,14 @@ const ( // in the definition itself. RequiredButNotDefinedInSchemaError = "%q is present in required but not defined as property in schema %q" + // SecuritySchemeNotDeclaredError indicates a security requirement naming a scheme that + // securityDefinitions does not declare. + SecuritySchemeNotDeclaredError = "security requirement %q is not declared in securityDefinitions" + + // SecurityScopesNotEmptyError indicates a security requirement listing scopes on a scheme that is + // not oauth2. Only an oauth2 requirement carries scopes; every other type must list none. + SecurityScopesNotEmptyError = "security requirement %q lists scopes (%s), but the security scheme it names is of type %q: only oauth2 requirements carry scopes" + // SomeParametersBrokenError indicates that some parameters could not be resolved, which might result in partial checks to be carried on. SomeParametersBrokenError = "some parameters definitions are broken in %q.%s. Cannot carry on full checks on parameters for operation %s" @@ -171,6 +188,11 @@ const ( // RequiredHasDefaultWarning indicates that a required parameter property should not have a default. RequiredHasDefaultWarning = "%s in %s has a default value and is required as parameter" + // SecurityScopeNotDeclaredWarning flags an oauth2 security requirement asking for a scope that the + // scheme does not list in its scopes. Swagger 2.0 does not spell out that the two must agree, so + // this is reported as a warning: a specification that names an undeclared scope stays valid. + SecurityScopeNotDeclaredWarning = "security requirement %q requires scope %q, which the security scheme does not declare" + // UnusedDefinitionWarning ... UnusedDefinitionWarning = "definition %q is not used anywhere" @@ -180,6 +202,12 @@ const ( // UnusedResponseWarning ... UnusedResponseWarning = "response %q is not used anywhere" + // CollectionFormatIgnoredWarning flags a collectionFormat on a parameter, header or items whose + // type is not array. collectionFormat says how to join the members of an array into one value, so + // it does nothing anywhere else. It is a warning, not an error: Swagger 2.0 defines the member as + // applying when the type is array, and does not forbid writing it elsewhere. + CollectionFormatIgnoredWarning = "collectionFormat %q is ignored in %s: it joins the members of an array, and the type is %q" + // DubiousAbsoluteRefWarning flags a $ref pointing to an absolute local file location that escapes the // spec's base path. Absolute local references are legitimate when they stay beneath the base path // (flattening/expansion introduces such anchors for cyclical $refs), but an absolute reference that @@ -416,6 +444,31 @@ func invalidObjectMsg(path, in string) errors.Error { // func invalidResponseDefinitionAsSchemaMsg(path, method string) errors.Error { // return errors.New(errors.CompositeErrorCode, InvalidResponseDefinitionAsSchemaError, path, method) // } + +func collectionFormatIgnoredMsg(collectionFormat, in, typ string) errors.Error { + return errors.New(errors.CompositeErrorCode, CollectionFormatIgnoredWarning, collectionFormat, in, typ) +} + +func discriminatorNotDefinedMsg(discriminator, in string) errors.Error { + return errors.New(errors.CompositeErrorCode, DiscriminatorNotDefinedError, discriminator, in) +} + +func discriminatorNotRequiredMsg(discriminator, in string) errors.Error { + return errors.New(errors.CompositeErrorCode, DiscriminatorNotRequiredError, discriminator, in) +} + +func securitySchemeNotDeclaredMsg(name string) errors.Error { + return errors.New(errors.CompositeErrorCode, SecuritySchemeNotDeclaredError, name) +} + +func securityScopesNotEmptyMsg(name, scopes, schemeType string) errors.Error { + return errors.New(errors.CompositeErrorCode, SecurityScopesNotEmptyError, name, scopes, schemeType) +} + +func securityScopeNotDeclaredMsg(name, scope string) errors.Error { + return errors.New(errors.CompositeErrorCode, SecurityScopeNotDeclaredWarning, name, scope) +} + func someParametersBrokenMsg(path, method, operationID string) errors.Error { return errors.New(errors.CompositeErrorCode, SomeParametersBrokenError, path, method, operationID) }