diff --git a/.github/workflows/go-test.yml b/.github/workflows/go-test.yml index 1722ebc..ec67258 100644 --- a/.github/workflows/go-test.yml +++ b/.github/workflows/go-test.yml @@ -14,6 +14,4 @@ on: jobs: test: uses: go-openapi/ci-workflows/.github/workflows/go-test.yml@befbc52f24215555cb601b0d824b2987c1d49d0b # v0.4.3 - with: - extra-flags: '-tags poolsdebug' secrets: inherit diff --git a/.github/workflows/integration-test.yml b/.github/workflows/integration-test.yml new file mode 100644 index 0000000..bffa4e5 --- /dev/null +++ b/.github/workflows/integration-test.yml @@ -0,0 +1,53 @@ +name: integration test + +permissions: + pull-requests: read + contents: read + +on: + push: + branches: + - master + + pull_request: + +jobs: + test: + name: integration (${{ matrix.os }}) + runs-on: ${{ matrix.os }} + timeout-minutes: 30 + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest] + + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version: stable + check-latest: true + cache: true + cache-dependency-path: '**/go.sum' + + - name: Run integration tests + run: > + go test + -tags poolsdebug + -count 1 + -timeout 30m + + # Single gate job for branch protection rules + integration-test: + name: integration test + needs: test + if: always() + runs-on: ubuntu-latest + steps: + - name: Check matrix results + run: | + if [ "${{ needs.test.result }}" != "success" ]; then + echo "Matrix jobs failed or were cancelled" + exit 1 + fi diff --git a/.gitignore b/.gitignore index d8f4186..fbb78de 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ .idea .env .mcp.json +.worktrees diff --git a/.golangci.yml b/.golangci.yml index 4d6b36e..b8875d7 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -7,6 +7,8 @@ linters: - gochecknoglobals - gochecknoinits - godox + - gomodguard + - gomodguard_v2 - exhaustruct - ireturn - nlreturn diff --git a/default_validator.go b/default_validator.go index 010a10f..5902cee 100644 --- a/default_validator.go +++ b/default_validator.go @@ -76,8 +76,11 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { res := validatorPools.results.Borrow() // will redeem when merged s := d.SpecValidator - for method, pathItem := range s.expandedAnalyzer().Operations() { - for path, op := range pathItem { + operations := s.expandedAnalyzer().Operations() + for _, method := range sortedKeys(operations) { + pathItem := operations[method] + for _, path := range sortedKeys(pathItem) { + op := pathItem[path] // parameters for _, param := range paramHelp.safeExpandedParamsFor(path, method, op.ID, res, s) { if param.Default != nil && param.Required { @@ -92,6 +95,7 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { if param.Default != nil && param.Schema == nil { // check param default value is valid red := newParamValidator(¶m, s.KnownFormats, d.schemaOptions).Validate(param.Default) //#nosec + red.relocate(s.parameterPath(path, method, param.In, param.Name).child(jsonDefault)) if red.HasErrorsOrWarnings() { res.addErrorsAt(s.parameterPath(path, method, param.In, param.Name), defaultValueDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) @@ -113,7 +117,7 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { if param.Schema != nil { // Validate default value against schema - red := d.validateDefaultValueSchemaAgainstSchema(s.parameterPath(path, method, param.In, param.Name), param.In, param.Schema) + red := d.validateDefaultValueSchemaAgainstSchema(s.parameterPath(path, method, param.In, param.Name).structuralChild(jsonSchema), param.In, param.Schema) if red.HasErrorsOrWarnings() { res.addErrorsAt(s.parameterPath(path, method, param.In, param.Name), defaultValueDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) @@ -130,8 +134,9 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { } // Same constraint on regular Responses if op.Responses.StatusCodeResponses != nil { // Safeguard - for code, r := range op.Responses.StatusCodeResponses { - res.Merge(d.validateDefaultInResponse(&r, "response", path, method, code, op.ID)) //#nosec + for _, code := range sortedKeys(op.Responses.StatusCodeResponses) { + r := op.Responses.StatusCodeResponses[code] + res.Merge(d.validateDefaultInResponse(&r, "response", path, method, code, op.ID)) } } } else if op.ID != "" { @@ -143,8 +148,10 @@ func (d *defaultValidator) validateDefaultValueValidAgainstSchema() *Result { if s.spec.Spec().Definitions != nil { // Safeguard // reset explored schemas to get depth-first recursive-proof exploration d.resetVisited() - for nm, sch := range s.spec.Spec().Definitions { - res.Merge(d.validateDefaultValueSchemaAgainstSchema(newPathSegments(swaggerDefinitions, nm), "body", &sch)) //#nosec + definitions := s.spec.Spec().Definitions + for _, nm := range sortedKeys(definitions) { + sch := definitions[nm] + res.Merge(d.validateDefaultValueSchemaAgainstSchema(newPathSegments(swaggerDefinitions, nm), "body", &sch)) } } return res @@ -155,20 +162,21 @@ func (d *defaultValidator) validateDefaultInResponse( ) *Result { s := d.SpecValidator - response, res := responseHelp.expandResponseRef(resp, path, s) + responseName, responseCodeAsStr := responseHelp.responseMsgVariants(responseType, responseCode) + response, res := responseHelp.expandResponseRef(resp, path, responsePath(path, method, responseCodeAsStr), s) if !res.IsValid() { return res } - responseName, responseCodeAsStr := responseHelp.responseMsgVariants(responseType, responseCode) - if response.Headers != nil { // Safeguard - for nm, h := range response.Headers { + for _, nm := range sortedKeys(response.Headers) { + h := response.Headers[nm] // reset explored schemas to get depth-first recursive-proof exploration d.resetVisited() if h.Default != nil { red := newHeaderValidator(nm, &h, s.KnownFormats, d.schemaOptions).Validate(h.Default) //#nosec + red.relocate(responseHeaderPath(path, method, responseCodeAsStr, nm).child(jsonDefault)) if red.HasErrorsOrWarnings() { res.addErrorsAt(responseHeaderPath(path, method, responseCodeAsStr, nm), defaultValueHeaderDoesNotValidateMsg(operationID, nm, responseName)) res.Merge(red) @@ -244,11 +252,13 @@ func (d *defaultValidator) validateDefaultValueSchemaAgainstSchema(path pathSegm // NOTE: we keep validating values, even though additionalItems is not supported by Swagger 2.0 (and 3.0 as well) res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(jsonAdditionalItems), in, schema.AdditionalItems.Schema)) } - for propName, prop := range schema.Properties { - res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.structuralChild(jsonProperties).child(propName), in, &prop)) //#nosec + for _, propName := range sortedKeys(schema.Properties) { + prop := schema.Properties[propName] + res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.structuralChild(jsonProperties).child(propName), in, &prop)) } - for propName, prop := range schema.PatternProperties { - res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.structuralChild(jsonPatternProperties).child(propName), in, &prop)) //#nosec + for _, propName := range sortedKeys(schema.PatternProperties) { + prop := schema.PatternProperties[propName] + res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.structuralChild(jsonPatternProperties).child(propName), in, &prop)) } if schema.AdditionalProperties != nil && schema.AdditionalProperties.Schema != nil { res.Merge(d.validateDefaultValueSchemaAgainstSchema(path.child(jsonAdditionalProperties), in, schema.AdditionalProperties.Schema)) diff --git a/deterministic_test.go b/deterministic_test.go new file mode 100644 index 0000000..6d0a20d --- /dev/null +++ b/deterministic_test.go @@ -0,0 +1,194 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "path/filepath" + "testing" + + "github.com/go-openapi/loads" + "github.com/go-openapi/strfmt" + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +// repeats is how many times a document is validated when checking that its +// report does not move. Go randomises map iteration per range statement, so a +// two-key map picks the same starting bucket often enough that a handful of +// runs would not notice; a few dozen make an unstable report near-certain to +// show itself. +const repeats = 50 + +// twoFaultyDefinitionsFixture holds one fault in each of two definitions: Pet +// requires a property it never declares (an error), Tag marks one both required +// and readOnly (a warning). +// +// Walking definitions in map order, the check stops on the first fault it meets, +// so whether Tag was heard from at all used to depend on where the map started. +const twoFaultyDefinitionsFixture = `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": { + "/pets": {"get": {"operationId": "getPets", + "responses": {"200": {"description": "ok", "schema": {"$ref": "#/definitions/Pet"}}}}} + }, + "definitions": { + "Pet": {"type": "object", "required": ["notDeclared"], "properties": {"name": {"type": "string"}}}, + "Tag": {"type": "object", "required": ["readOnlyToo"], "properties": {"readOnlyToo": {"type": "string", "readOnly": true}}} + } +}` + +// twoCircularDefinitionsFixture holds two definitions that are each their own +// ancestor. Only one circular ancestry is reported, and which one it is used to +// be drawn from the map iteration order. +const twoCircularDefinitionsFixture = `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": { + "/a": {"get": {"operationId": "getA", + "responses": {"200": {"description": "ok", "schema": {"$ref": "#/definitions/A"}}}}}, + "/b": {"get": {"operationId": "getB", + "responses": {"200": {"description": "ok", "schema": {"$ref": "#/definitions/B"}}}}} + }, + "definitions": { + "A": {"type": "object", "allOf": [{"$ref": "#/definitions/A"}]}, + "B": {"type": "object", "allOf": [{"$ref": "#/definitions/B"}]} + } +}` + +func TestDeterministic_RequiredDefinitions(t *testing.T) { + t.Parallel() + + t.Run("stopping on the first fault always stops on the same one", func(t *testing.T) { + t.Parallel() + + reports := repeatedReports(t, twoFaultyDefinitionsFixture, func(v *SpecValidator) { + v.Options.ContinueOnErrors = false + }) + + assertSameReport(t, reports) + assert.SliceContainsT(t, reports[0], "ERR /definitions/Pet/required/0") + }) + + t.Run("reporting everything reports both definitions", func(t *testing.T) { + t.Parallel() + + reports := repeatedReports(t, twoFaultyDefinitionsFixture, func(v *SpecValidator) { + v.Options.ContinueOnErrors = true + }) + + assertSameReport(t, reports) + assert.SliceContainsT(t, reports[0], "ERR /definitions/Pet/required/0") + assert.SliceContainsT(t, reports[0], "WARN /definitions/Tag/required/0") + }) +} + +func TestDeterministic_CircularAncestry(t *testing.T) { + t.Parallel() + + t.Run("the definition named is the first in name order", func(t *testing.T) { + t.Parallel() + + reports := repeatedReports(t, twoCircularDefinitionsFixture, func(v *SpecValidator) { + v.Options.ContinueOnErrors = false + }) + + assertSameReport(t, reports) + assert.SliceContainsT(t, reports[0], "ERR /definitions/A") + }) + + t.Run("reporting everything reports both circular definitions", func(t *testing.T) { + t.Parallel() + + // the check used to return on the first circular ancestry whatever the + // options said, so the second definition could never be heard from + reports := repeatedReports(t, twoCircularDefinitionsFixture, func(v *SpecValidator) { + v.Options.ContinueOnErrors = true + }) + + assertSameReport(t, reports) + assert.SliceContainsT(t, reports[0], "ERR /definitions/A") + assert.SliceContainsT(t, reports[0], "ERR /definitions/B") + }) +} + +// TestDeterministic_FullReport guards the whole sweep rather than the two known +// sites: a document exercising most checks must produce the very same report, +// in the very same order, on every run. +func TestDeterministic_FullReport(t *testing.T) { + t.Parallel() + + // a whole report is a much finer probe than a single finding: every map a + // check walks contributes to it, so a handful of runs suffice + const runs = 20 + + for _, fixture := range []string{ + filepath.Join("fixtures", "validation", "fixture-1231.yaml"), + filepath.Join("fixtures", "validation", "fixture-additional-items-invalid-values.yaml"), + filepath.Join("fixtures", "validation", "fixture-342.yaml"), + } { + t.Run(filepath.Base(fixture), func(t *testing.T) { + t.Parallel() + + reports := make([][]string, 0, runs) + for range runs { + doc, err := loads.Spec(fixture) + require.NoError(t, err) + + validator := NewSpecValidator(doc.Schema(), strfmt.Default) + validator.Options.ContinueOnErrors = true + res, _ := validator.Validate(doc) + reports = append(reports, report(res)) + } + + require.NotEmpty(t, reports[0], "expected the fixture to yield findings") + assertSameReport(t, reports) + }) + } +} + +// repeatedReports validates the same document [repeats] times and returns one +// report per run, each in the order the checks emitted it. +func repeatedReports(t *testing.T, raw string, configure func(*SpecValidator)) [][]string { + t.Helper() + + reports := make([][]string, 0, repeats) + for range repeats { + // reloading each time so that no state carried by the document or its + // analyzer can smooth over an unstable walk + doc, err := loads.Analyzed(json.RawMessage(raw), "") + require.NoError(t, err) + + validator := NewSpecValidator(doc.Schema(), strfmt.Default) + configure(validator) + res, _ := validator.Validate(doc) + reports = append(reports, report(res)) + } + + return reports +} + +// report renders the located findings of a result as one line each, so that two +// runs can be compared on both what they found and where they said it was. +func report(res *Result) []string { + lines := make([]string, 0, len(res.Errors)+len(res.Warnings)) + for _, located := range res.LocatedErrors() { + lines = append(lines, "ERR "+located.Pointer) + } + for _, located := range res.LocatedWarnings() { + lines = append(lines, "WARN "+located.Pointer) + } + + return lines +} + +func assertSameReport(t *testing.T, reports [][]string) { + t.Helper() + + require.NotEmpty(t, reports) + for i, got := range reports[1:] { + assert.Equal(t, reports[0], got, "run %d reported differently from run 0", i+1) + } +} diff --git a/example_validator.go b/example_validator.go index 1f830a6..0e193d5 100644 --- a/example_validator.go +++ b/example_validator.go @@ -66,8 +66,11 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { res := validatorPools.results.Borrow() s := ex.SpecValidator - for method, pathItem := range s.expandedAnalyzer().Operations() { - for path, op := range pathItem { + operations := s.expandedAnalyzer().Operations() + for _, method := range sortedKeys(operations) { + pathItem := operations[method] + for _, path := range sortedKeys(pathItem) { + op := pathItem[path] // parameters for _, param := range paramHelp.safeExpandedParamsFor(path, method, op.ID, res, s) { @@ -82,6 +85,7 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { if param.Example != nil && param.Schema == nil { // check param default value is valid red := newParamValidator(¶m, s.KnownFormats, ex.schemaOptions).Validate(param.Example) //#nosec + red.relocate(s.parameterPath(path, method, param.In, param.Name).child(swaggerExample)) if red.HasErrorsOrWarnings() { res.addWarningsAt(s.parameterPath(path, method, param.In, param.Name), exampleValueDoesNotValidateMsg(param.Name, param.In)) res.MergeAsWarnings(red) @@ -103,7 +107,7 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { if param.Schema != nil { // Validate example value against schema - red := ex.validateExampleValueSchemaAgainstSchema(s.parameterPath(path, method, param.In, param.Name), param.In, param.Schema) + red := ex.validateExampleValueSchemaAgainstSchema(s.parameterPath(path, method, param.In, param.Name).structuralChild(jsonSchema), param.In, param.Schema) if red.HasErrorsOrWarnings() { res.addWarningsAt(s.parameterPath(path, method, param.In, param.Name), exampleValueDoesNotValidateMsg(param.Name, param.In)) res.Merge(red) @@ -120,8 +124,9 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { } // Same constraint on regular Responses if op.Responses.StatusCodeResponses != nil { // Safeguard - for code, r := range op.Responses.StatusCodeResponses { - res.Merge(ex.validateExampleInResponse(&r, "response", path, method, code, op.ID)) //#nosec + for _, code := range sortedKeys(op.Responses.StatusCodeResponses) { + r := op.Responses.StatusCodeResponses[code] + res.Merge(ex.validateExampleInResponse(&r, "response", path, method, code, op.ID)) } } } else if op.ID != "" { @@ -133,8 +138,10 @@ func (ex *exampleValidator) validateExampleValueValidAgainstSchema() *Result { if s.spec.Spec().Definitions != nil { // Safeguard // reset explored schemas to get depth-first recursive-proof exploration ex.resetVisited() - for nm, sch := range s.spec.Spec().Definitions { - res.Merge(ex.validateExampleValueSchemaAgainstSchema(newPathSegments(swaggerDefinitions, nm), "body", &sch)) //#nosec + definitions := s.spec.Spec().Definitions + for _, nm := range sortedKeys(definitions) { + sch := definitions[nm] + res.Merge(ex.validateExampleValueSchemaAgainstSchema(newPathSegments(swaggerDefinitions, nm), "body", &sch)) } } return res @@ -145,20 +152,21 @@ func (ex *exampleValidator) validateExampleInResponse( ) *Result { s := ex.SpecValidator - response, res := responseHelp.expandResponseRef(resp, path, s) + responseName, responseCodeAsStr := responseHelp.responseMsgVariants(responseType, responseCode) + response, res := responseHelp.expandResponseRef(resp, path, responsePath(path, method, responseCodeAsStr), s) if !res.IsValid() { // Safeguard return res } - responseName, responseCodeAsStr := responseHelp.responseMsgVariants(responseType, responseCode) - if response.Headers != nil { // Safeguard - for nm, h := range response.Headers { + for _, nm := range sortedKeys(response.Headers) { + h := response.Headers[nm] // reset explored schemas to get depth-first recursive-proof exploration ex.resetVisited() if h.Example != nil { red := newHeaderValidator(nm, &h, s.KnownFormats, ex.schemaOptions).Validate(h.Example) //#nosec + red.relocate(responseHeaderPath(path, method, responseCodeAsStr, nm).child(swaggerExample)) if red.HasErrorsOrWarnings() { res.addWarningsAt(responseHeaderPath(path, method, responseCodeAsStr, nm), exampleValueHeaderDoesNotValidateMsg(operationID, nm, responseName)) res.MergeAsWarnings(red) @@ -253,11 +261,13 @@ func (ex *exampleValidator) validateExampleValueSchemaAgainstSchema(path pathSeg // NOTE: we keep validating values, even though additionalItems is unsupported in Swagger 2.0 (and 3.0 as well) res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.child(jsonAdditionalItems), in, schema.AdditionalItems.Schema)) } - for propName, prop := range schema.Properties { - res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.structuralChild(jsonProperties).child(propName), in, &prop)) //#nosec + for _, propName := range sortedKeys(schema.Properties) { + prop := schema.Properties[propName] + res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.structuralChild(jsonProperties).child(propName), in, &prop)) } - for propName, prop := range schema.PatternProperties { - res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.structuralChild(jsonPatternProperties).child(propName), in, &prop)) //#nosec + for _, propName := range sortedKeys(schema.PatternProperties) { + prop := schema.PatternProperties[propName] + res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.structuralChild(jsonPatternProperties).child(propName), in, &prop)) } if schema.AdditionalProperties != nil && schema.AdditionalProperties.Schema != nil { res.Merge(ex.validateExampleValueSchemaAgainstSchema(path.child(jsonAdditionalProperties), in, schema.AdditionalProperties.Schema)) diff --git a/formats.go b/formats.go index bb58f79..23359e3 100644 --- a/formats.go +++ b/formats.go @@ -78,7 +78,7 @@ func (f *formatValidator) Validate(val any) *Result { } if err := FormatOf(f.Path.dotted(), f.In, f.Format, str, f.KnownFormats); err != nil { - result.AddErrors(err) + result.addErrorsAt(f.Path, err) } return result diff --git a/helpers.go b/helpers.go index b8f331e..62deb97 100644 --- a/helpers.go +++ b/helpers.go @@ -44,6 +44,9 @@ const ( jsonDefault = "default" jsonAllOf = "allOf" + jsonAnyOf = "anyOf" + jsonOneOf = "oneOf" + jsonNot = "not" jsonAdditionalItems = "additionalItems" jsonAdditionalProperties = "additionalProperties" @@ -52,6 +55,7 @@ const ( swaggerResponses = "responses" swaggerParameters = "parameters" swaggerHeaders = "headers" + swaggerOperationID = "operationId" jsonMimeApplicationJSON = "application/json" ) @@ -293,15 +297,19 @@ func (h *paramHelper) safeExpandedParamsFor(path, method, operationID string, re // remove params with invalid expansion from Slice operation.Parameters = resolvedParams - for _, ppr := range s.expandedAnalyzer().SafeParamsFor(method, path, + // the analyzer keys parameters by name and location: walk those keys in + // order, so that findings about an operation's parameters come out the + // same way on every run + safeParams := s.expandedAnalyzer().SafeParamsFor(method, path, func(_ spec.Parameter, err error) bool { // since params have already been expanded, there are few causes for error res.addErrorsAt(operationPath(path, method), someParametersBrokenMsg(path, method, operationID)) // original error from analyzer res.addErrorsAt(operationPath(path, method), err) return true - }) { - params = append(params, ppr) + }) + for _, k := range sortedKeys(safeParams) { + params = append(params, safeParams[k]) } } return @@ -361,7 +369,7 @@ type responseHelper struct { func (r *responseHelper) expandResponseRef( response *spec.Response, - path string, s *SpecValidator, + path string, at pathSegments, s *SpecValidator, ) (*spec.Response, *Result) { // Ensure response is expanded var err error @@ -374,7 +382,7 @@ func (r *responseHelper) expandResponseRef( } if err != nil { // Safeguard // NOTE: we may enter here when the whole response is an unresolved $ref. - errorHelp.addPointerError(res, err, response.Ref.String(), path) + errorHelp.addPointerErrorAt(res, at, err, response.Ref.String(), path) return nil, res } diff --git a/object_validator.go b/object_validator.go index 2d14557..4f1dd15 100644 --- a/object_validator.go +++ b/object_validator.go @@ -103,7 +103,8 @@ func (o *objectValidator) Validate(data any) *Result { // Check patternProperties // NOTE: it looks like we have done that twice in many cases - for key, value := range val { + for _, key := range sortedKeys(val) { + value := val[key] _, regularProperty := o.Properties[key] matched, _, patterns := o.validatePatternProperty(key, value, res) // applies to regular properties as well if regularProperty || !matched { @@ -214,7 +215,7 @@ func (o *objectValidator) precheck(res *Result, val map[string]any) { } func (o *objectValidator) validateNoAdditionalProperties(val map[string]any, res *Result) { - for k := range val { + for _, k := range sortedKeys(val) { if k == "$schema" || k == "id" { // special properties "$schema" and "id" are ignored continue @@ -263,7 +264,8 @@ func (o *objectValidator) validateNoAdditionalProperties(val map[string]any, res continue } - for headerKey, headerBody := range headers { + for _, headerKey := range sortedKeys(headers) { + headerBody := headers[headerKey] if headerBody == nil { continue } @@ -296,7 +298,8 @@ func (o *objectValidator) validateNoAdditionalProperties(val map[string]any, res } func (o *objectValidator) validateAdditionalProperties(val map[string]any, res *Result) { - for key, value := range val { + for _, key := range sortedKeys(val) { + value := val[key] _, regularProperty := o.Properties[key] if regularProperty { continue @@ -333,7 +336,7 @@ func (o *objectValidator) validatePropertiesSchema(val map[string]any, res *Resu validatorPools.schemas.Redeem(pSchema) }() - for pName := range o.Properties { + for _, pName := range sortedKeys(o.Properties) { *pSchema = o.Properties[pName] rName := o.Path.child(pName) @@ -392,7 +395,7 @@ func (o *objectValidator) validatePatternProperty(key string, value any, result validatorPools.schemas.Redeem(schema) }() - for k := range o.PatternProperties { + for _, k := range sortedKeys(o.PatternProperties) { re, err := compileRegexp(k) if err != nil { continue diff --git a/param_locations.go b/param_locations.go index e8b4b22..703d6ca 100644 --- a/param_locations.go +++ b/param_locations.go @@ -59,8 +59,8 @@ func newParamLocations(sp *spec.Swagger) paramLocations { // // A parameter the operation does not declare itself may come from its path // item. When neither knows it, which happens for a parameter too broken to be -// identified, the name stands in for the index: the location no longer -// resolves, but it still says what it is about. +// identified, the pointer stops on the array holding it and the name is kept +// for the message alone: an index no one could work out would be a guess. func (l paramLocations) at(path, method, in, name string) pathSegments { if found, isDeclared := l[paramKey{path: path, method: methodToken(method), in: in, name: name}]; isDeclared { return found @@ -70,7 +70,7 @@ func (l paramLocations) at(path, method, in, name string) pathSegments { return found } - return operationPath(path, method).children(swaggerParameters, name) + return operationPath(path, method).child(swaggerParameters).cosmeticChild(name) } func (l paramLocations) collect(sp *spec.Swagger, key paramKey, at pathSegments, params []spec.Parameter) { diff --git a/param_locations_test.go b/param_locations_test.go index 2f7971a..b308190 100644 --- a/param_locations_test.go +++ b/param_locations_test.go @@ -108,11 +108,11 @@ func TestParamLocations_UnknownParameterKeepsItsName(t *testing.T) { locations := paramLocationsOf(t) - // a parameter too broken to be identified has no index to point at, so - // the name stands in: the location no longer resolves, but a message - // built from it still says what it is about + // a parameter too broken to be identified has no index to point at, so the + // pointer stops on the array that holds it — a node the document does have + // — while the message still says which parameter it is about unknown := locations.at("/pets", methodGet, inQuery, "neverDeclared") - assert.EqualT(t, "/paths/~1pets/get/parameters/neverDeclared", unknown.pointer()) + assert.EqualT(t, "/paths/~1pets/get/parameters", unknown.pointer()) assert.EqualT(t, "paths./pets.get.parameters.neverDeclared", unknown.dotted()) } diff --git a/path.go b/path.go index 7bbe2ec..940fc4b 100644 --- a/path.go +++ b/path.go @@ -40,6 +40,12 @@ type pathToken struct { // of its members, say. It is part of the pointer and absent from the // dotted form. structural bool + + // cosmetic is the converse: a token messages name but that the document + // does not address, such as a parameter too broken to be found by name in + // the array holding it. It is part of the dotted form and absent from the + // pointer, which then stops at the deepest node the document does contain. + cosmetic bool } // readable renders a token the way a message should spell it. @@ -92,20 +98,37 @@ func (p pathSegments) structuralChild(token string) pathSegments { return p.appendToken(pathToken{token: token, structural: true}) } +// cosmeticChild returns a location that messages spell as a member named token, +// while the pointer stays on p. +func (p pathSegments) cosmeticChild(token string) pathSegments { + return p.appendToken(pathToken{token: token, cosmetic: true}) +} + func (p pathSegments) appendToken(token pathToken) pathSegments { child := make(pathSegments, len(p)+1) copy(child, p) - child[len(p)] = token + child[len(p)] = p.inherit(token) return child } +// inherit passes down what a parent token says about addressability: nothing +// below a token the document does not address is addressable either, so the +// pointer has to stop at the same place. +func (p pathSegments) inherit(token pathToken) pathToken { + if len(p) > 0 && p[len(p)-1].cosmetic { + token.cosmetic = true + } + + return token +} + // children returns the location of a chain of named members below p. func (p pathSegments) children(tokens ...string) pathSegments { child := make(pathSegments, len(p)+len(tokens)) copy(child, p) for i, token := range tokens { - child[len(p)+i] = pathToken{token: token} + child[len(p)+i] = child[:len(p)+i].inherit(pathToken{token: token}) } return child @@ -234,6 +257,10 @@ func (p pathSegments) pointer() string { var w strings.Builder for _, token := range p { + if token.cosmetic { + continue + } + w.WriteByte('/') w.WriteString(jsonpointer.Escape(token.token)) } diff --git a/pointer_conformance_test.go b/pointer_conformance_test.go new file mode 100644 index 0000000..1285675 --- /dev/null +++ b/pointer_conformance_test.go @@ -0,0 +1,262 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "testing" + + "github.com/go-openapi/testify/v2/assert" +) + +// conformanceCase is one minimal document with one fault, and the node a reader +// has to go to in order to amend it. +type conformanceCase struct { + name string + doc string + // says which finding of the document the case is about + message string + pointer string +} + +// The grid below pins the location of one finding per kind of fault, over the +// whole shape of a Swagger document: the root, the info block, an operation, a +// parameter wherever it may be declared, a response, a definition. +// +// It is a conformance matrix rather than a set of regression tests. Each row +// works today; the point is that none of them may quietly stop working, since +// a location is only ever as useful as it is stable. +var conformanceCases = []conformanceCase{ + { + name: "a document with no info block", + doc: `{"swagger":"2.0","paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok"}}}}}}`, + message: "info in body is required", + pointer: "", + }, + { + name: "a document with no paths", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}}`, + message: "paths in body is required", + pointer: "", + }, + { + name: "an info block with no title", + doc: `{"swagger":"2.0","info":{"version":"1"},"paths":{}}`, + message: "info.title in body is required", + pointer: "/info", + }, + { + name: "a license with no name", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1","license":{"url":"http://x"}},"paths":{}}`, + message: "info.license.name in body is required", + pointer: "/info/license", + }, + { + name: "external documentation with no url", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"},"paths":{},"externalDocs":{"description":"d"}}`, + message: "externalDocs.url in body is required", + pointer: "/externalDocs", + }, + { + name: "a tag with no name", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"},"paths":{},"tags":[{"description":"d"}]}`, + message: "tags.0.name in body is required", + pointer: "/tags/0", + }, + { + name: "a response with no description", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{}}}}}}`, + message: "paths./a.get.responses.200.description in body is required", + pointer: "/paths/~1a/get/responses/200", + }, + { + name: "a shared response with no description", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"$ref":"#/responses/shared"}}}}}, + "responses":{"shared":{}}}`, + message: "responses.shared.description in body is required", + pointer: "/responses/shared", + }, + { + name: "an operation with no responses", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a"}}}}`, + message: "paths./a.get.responses in body is required", + pointer: "/paths/~1a/get", + }, + { + name: "a body parameter with no schema", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"post":{"operationId":"a","parameters":[{"name":"p","in":"body"}], + "responses":{"200":{"description":"ok"}}}}}}`, + message: "invalid definition for parameter p in body in operation \"a\"", + pointer: "/paths/~1a/post/parameters/0", + }, + { + name: "a parameter with no name", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","parameters":[{"in":"query","type":"string"}], + "responses":{"200":{"description":"ok"}}}}}}`, + message: "paths./a.get.parameters.0.name in body is required", + pointer: firstParam, + }, + { + name: "a shared parameter with no type", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","parameters":[{"$ref":"#/parameters/shared"}], + "responses":{"200":{"description":"ok"}}}}}, + "parameters":{"shared":{"name":"s","in":"query"}}}`, + message: "parameters.shared.type in body is required", + pointer: "/parameters/shared", + }, + { + name: "a path-item parameter with no type", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"parameters":[{"name":"s","in":"query"}], + "get":{"operationId":"a","responses":{"200":{"description":"ok"}}}}}}`, + message: "paths./a.parameters.0.type in body is required", + pointer: "/paths/~1a/parameters/0", + }, + { + name: "an array parameter with no items", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","parameters":[{"name":"s","in":"query","type":"array"}], + "responses":{"200":{"description":"ok"}}}}}}`, + message: "param \"s\" for \"a\" is a collection without an element type (array requires item definition)", + pointer: firstParam, + }, + { + name: "a response header with no type", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "headers":{"X":{"format":"int32"}}}}}}}}`, + message: "paths./a.get.responses.200.headers.X.type in body is required", + pointer: "/paths/~1a/get/responses/200/headers/X", + }, + { + name: "an array response schema with no items", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"type":"array"}}}}}}}`, + message: "response body for \"a\" is a collection without an element type (array requires items definition)", + pointer: "/paths/~1a/get/responses/200/schema", + }, + { + name: "an array definition property with no items", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"$ref":"#/definitions/A"}}}}}}, + "definitions":{"A":{"type":"object","properties":{"list":{"type":"array"}}}}}`, + message: "items in definitions.A.properties.list is required", + pointer: "/definitions/A/properties/list", + }, + { + name: "a security definition with no type", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"},"paths":{}, + "securityDefinitions":{"k":{"name":"api_key","in":"header"}}}`, + message: "securityDefinitions.k.type in body is required", + pointer: "/securityDefinitions/k", + }, + { + name: "a required entry naming an undeclared property", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"$ref":"#/definitions/A"}}}}}}, + "definitions":{"A":{"type":"object","required":["ghost"],"properties":{"real":{"type":"string"}}}}}`, + message: `"ghost" is present in required but not defined as property in definition "A"`, + pointer: "/definitions/A/required/0", + }, + { + name: "a path parameter absent from the path template", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a", + "parameters":[{"name":"id","in":"path","required":true,"type":"string"}], + "responses":{"200":{"description":"ok"}}}}}}`, + message: `path param "id" is not present in path "/a"`, + pointer: "/paths/~1a", + }, + { + name: "more than one body parameter", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"post":{"operationId":"a","parameters":[ + {"name":"one","in":"body","schema":{"type":"string"}}, + {"name":"two","in":"body","schema":{"type":"string"}}], + "responses":{"200":{"description":"ok"}}}}}}`, + message: `operation "a" has more than 1 body param: ["one" "two"]`, + pointer: "/paths/~1a/post", + }, + { + name: "a nested array parameter with no items", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a", + "parameters":[{"name":"s","in":"query","type":"array","items":{"type":"array"}}], + "responses":{"200":{"description":"ok"}}}}}}`, + message: "items in paths./a.get.parameters.s.items is required", + pointer: "/paths/~1a/get/parameters/0/items", + }, + { + name: "an array additionalProperties with no items", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"$ref":"#/definitions/A"}}}}}}, + "definitions":{"A":{"type":"object","additionalProperties":{"type":"array"}}}}`, + message: "items in definitions.A.additionalProperties is required", + pointer: "/definitions/A/additionalProperties", + }, + { + name: "an array property of an allOf member with no items", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"$ref":"#/definitions/A"}}}}}}, + "definitions":{"A":{"allOf":[{"type":"object","properties":{"l":{"type":"array"}}}]}}}`, + message: "items in definitions.A.allOf.0.properties.l is required", + pointer: "/definitions/A/allOf/0/properties/l", + }, + { + name: "a property declared twice through allOf", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"$ref":"#/definitions/Child"}}}}}}, + "definitions":{ + "Base":{"type":"object","properties":{"dup":{"type":"string"}}}, + "Child":{"allOf":[{"$ref":"#/definitions/Base"},{"type":"object","properties":{"dup":{"type":"string"}}}]}}}`, + message: `definition "Child" contains duplicate properties: [Child.dup]`, + pointer: "/definitions/Child", + }, + { + name: "a parameter declared twice in an operation", + doc: `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","parameters":[ + {"name":"s","in":"query","type":"string"}, + {"name":"s","in":"query","type":"string"}], + "responses":{"200":{"description":"ok"}}}}}}`, + message: `duplicate parameter name "s" for "query" in operation "a"`, + // the second declaration is the offending one + pointer: "/paths/~1a/get/parameters/1", + }, +} + +func TestPointerConformance(t *testing.T) { + t.Parallel() + + for _, testCase := range conformanceCases { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, testCase.doc) + + matched := false + for message, pointer := range found { + if message != testCase.message { + continue + } + + matched = true + assert.EqualT(t, testCase.pointer, pointer, "for %q", message) + } + + assert.TrueT(t, matched, "expected a finding containing %q, got %v", testCase.message, found) + }) + } +} diff --git a/pointer_resolution_test.go b/pointer_resolution_test.go new file mode 100644 index 0000000..098c7ad --- /dev/null +++ b/pointer_resolution_test.go @@ -0,0 +1,254 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/go-openapi/jsonpointer" + "github.com/go-openapi/loads" + "github.com/go-openapi/strfmt" + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +// The tests below pin down that a reported location addresses a node the +// document contains, and which node that is. +// +// They exist because a check walks an expanded, model-level view of a +// specification: parameters merged in from a path item, schemata reached +// through a $ref, values validated by the runtime validators a generated client +// uses. Each of those used to produce a pointer that walked off the document. + +// resolvedPointers pairs every finding of a document with the pointer reported +// for it, having first verified that the pointer addresses something. +type resolvedPointers map[string]string + +func TestPointerResolution_ValueBelowItsHolder(t *testing.T) { + t.Parallel() + + t.Run("a simple parameter default is located on the default", func(t *testing.T) { + t.Parallel() + + // the parameter is named zzz so that a pointer built from the name + // rather than from the document is unmistakable + found := locatedFindings(t, `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a", + "parameters":[{"name":"zzz","in":"query","type":"integer","default":"nope"}], + "responses":{"200":{"description":"ok"}}}}}}`) + + assert.EqualT(t, "/paths/~1a/get/parameters/0/default", + found[`zzz in query must be of type integer: "string"`]) + assert.EqualT(t, "/paths/~1a/get/parameters/0", + found["default value for zzz in query does not validate its schema"]) + }) + + t.Run("a response header default is located on the default", func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "headers":{"X-Count":{"type":"integer","default":"nope"}}}}}}}}`) + + assert.EqualT(t, "/paths/~1a/get/responses/200/headers/X-Count/default", + found[`X-Count in header must be of type integer: "string"`]) + }) + + t.Run("a body parameter is entered through its schema", func(t *testing.T) { + t.Parallel() + + // the response side gained this token first; a body parameter holds its + // schema under the same member and needs the same one + found := locatedFindings(t, `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"post":{"operationId":"a", + "parameters":[{"name":"payload","in":"body", + "schema":{"type":"object","properties":{"n":{"type":"integer","default":"nope"}}}}], + "responses":{"200":{"description":"ok"}}}}}}`) + + assert.EqualT(t, "/paths/~1a/post/parameters/0/schema/properties/n/default", + found[`paths./a.post.parameters.payload.n.default in body must be of type integer: "string"`]) + }) +} + +func TestPointerResolution_PathTemplate(t *testing.T) { + t.Parallel() + + // the variable name used to stand where the path key belongs, unescaped: + // ghostvar makes it plain which token was taken + found := locatedFindings(t, `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/deep/nested/{ghostvar}":{"get":{"operationId":"a", + "responses":{"200":{"description":"ok"}}}}}}`) + + assert.EqualT(t, "/paths/~1deep~1nested~1{ghostvar}", + found[`path param "{ghostvar}" has no parameter definition`]) +} + +func TestPointerResolution_FindingsWithADefiniteSite(t *testing.T) { + t.Parallel() + + t.Run("a duplicate operationId names the first operation using it", func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"dup","responses":{"200":{"description":"ok"}}}}, + "/b":{"get":{"operationId":"dup","responses":{"200":{"description":"ok"}}}}}}`) + + assert.EqualT(t, "/paths/~1a/get/operationId", found[`"dup" is defined 2 times`]) + }) + + t.Run("an unresolvable reference is located where it is declared", func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"$ref":"#/definitions/Missing"}}}}}}}`) + + // expansion reports the whole document in one message, so the finding + // is given the site of the reference it could not follow + require.NotEmpty(t, found) + for message, pointer := range found { + if strings.Contains(message, "could not be resolved") { + assert.EqualT(t, "/paths/~1a/get/responses/200/schema", pointer) + } + } + }) +} + +func TestPointerResolution_ThroughARef(t *testing.T) { + t.Parallel() + + // the response is a bare $ref, so nothing exists below it: the location has + // to be followed to what the document does hold, hopping twice on the way + found := locatedFindings(t, `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"$ref":"#/responses/shared"}}}}}, + "responses":{"shared":{"description":"ok","schema":{"$ref":"#/definitions/A"}}}, + "definitions":{"A":{"type":"object","default":{"n":"not-an-object"}, + "properties":{"n":{"type":"object"}}}}}`) + + assert.EqualT(t, "/definitions/A/default/n", + found[`paths./a.get.responses.200.default.n in body must be of type object: "string"`]) + assert.EqualT(t, "/paths/~1a/get/responses/200", + found[`in operation "a", default value in response 200 does not validate its schema`]) +} + +// TestPointerResolution_EveryFixture is the standing guarantee: no document in +// the corpus may report a location that addresses nothing. +func TestPointerResolution_EveryFixture(t *testing.T) { + t.Parallel() + + var files []string + for _, pattern := range []string{ + filepath.Join("fixtures", "validation", "*.json"), + filepath.Join("fixtures", "validation", "*.yaml"), + filepath.Join("fixtures", "bugs", "*", "*.json"), + filepath.Join("fixtures", "bugs", "*", "*.yaml"), + filepath.Join("fixtures", "go-swagger", "*", "*", "*.json"), + filepath.Join("fixtures", "go-swagger", "*", "*", "*.yaml"), + filepath.Join("fixtures", "petstore", "*.json"), + } { + matched, err := filepath.Glob(pattern) + require.NoError(t, err) + files = append(files, matched...) + } + require.NotEmpty(t, files) + + var specs, findings int + for _, file := range files { + doc, err := loads.Spec(file) + if err != nil { + // a fixture that does not even load says nothing about locations + continue + } + + var document any + if err := json.Unmarshal(doc.Raw(), &document); err != nil { + continue + } + + func() { + // a handful of fixtures are deliberately degenerate + defer func() { _ = recover() }() + + validator := NewSpecValidator(doc.Schema(), strfmt.Default) + validator.Options.ContinueOnErrors = true + res, _ := validator.Validate(doc) + if res == nil { + return + } + specs++ + + for _, located := range append(res.LocatedErrors(), res.LocatedWarnings()...) { + findings++ + assert.TrueT(t, addresses(document, located.Pointer), + "%s: %q addresses nothing (%v)", filepath.Base(file), located.Pointer, located.Err) + } + }() + } + + t.Logf("%d specifications, %d findings", specs, findings) + require.NotZero(t, findings) +} + +// locatedFindings validates a document and returns its findings by message, +// asserting first that every pointer addresses a node the document holds. +func locatedFindings(t *testing.T, raw string) resolvedPointers { + t.Helper() + + var document any + require.NoError(t, json.Unmarshal([]byte(raw), &document)) + + doc, err := loads.Analyzed(json.RawMessage(raw), "") + require.NoError(t, err) + + validator := NewSpecValidator(doc.Schema(), strfmt.Default) + validator.Options.ContinueOnErrors = true + res, _ := validator.Validate(doc) + require.NotNil(t, res) + + found := make(resolvedPointers) + for _, located := range append(res.LocatedErrors(), res.LocatedWarnings()...) { + assert.TrueT(t, addresses(document, located.Pointer), + "%q addresses nothing (%v)", located.Pointer, located.Err) + found[located.Err.Error()] = located.Pointer + } + + return found +} + +// addresses walks a JSON pointer over a decoded document, independently of the +// code that produced it. +func addresses(document any, pointer string) bool { + if pointer == "" { + return true // the whole document + } + if !strings.HasPrefix(pointer, "/") { + return false + } + + node := document + for token := range strings.SplitSeq(pointer[1:], "/") { + switch held := node.(type) { + case map[string]any: + member, isHeld := held[jsonpointer.Unescape(token)] + if !isHeld { + return false + } + node = member + case []any: + index, err := strconv.Atoi(token) + if err != nil || index < 0 || index >= len(held) { + return false + } + node = held[index] + default: + return false + } + } + + return true +} diff --git a/ref_redirects.go b/ref_redirects.go new file mode 100644 index 0000000..95fe96c --- /dev/null +++ b/ref_redirects.go @@ -0,0 +1,71 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "strings" + + "github.com/go-openapi/analysis" +) + +// maxRefHops bounds how many $ref a pointer may be followed through, so that a +// document referring to itself cannot spin here. +const maxRefHops = 10 + +// refRedirects maps the location of a $ref to the location it points at, for +// the local references of a document. +// +// Checks walk the expanded document, so a finding below a $ref comes out with a +// pointer that descends into a node the authored document does not contain: a +// bare "$ref" member has nothing under it. Following the reference turns such a +// pointer back into one the document addresses. +type refRedirects map[string]string + +func newRefRedirects(analyzer *analysis.Spec) refRedirects { + redirects := make(refRedirects) + for location, ref := range analyzer.AllRefsByLocation() { + target := ref.String() + if !strings.HasPrefix(target, "#/") { + // only a local reference has a location in this document + continue + } + + redirects[strings.TrimPrefix(location, "#")] = strings.TrimPrefix(target, "#") + } + + return redirects +} + +// through rewrites a pointer that descends below a $ref. +// +// A pointer that stops at the $ref itself is left alone: that node exists, and +// it is where a reader has to go to amend the reference. +func (r refRedirects) through(pointer string) string { + if len(r) == 0 { + return pointer + } + + for range maxRefHops { + prefix, rest, ok := r.crossing(pointer) + if !ok { + return pointer + } + + pointer = prefix + rest + } + + return pointer +} + +// crossing finds the longest prefix of pointer that holds a $ref, and returns +// the location that reference points at together with what is left below it. +func (r refRedirects) crossing(pointer string) (target, rest string, ok bool) { + for at := strings.LastIndex(pointer, "/"); at > 0; at = strings.LastIndex(pointer[:at], "/") { + if target, isRef := r[pointer[:at]]; isRef { + return target, pointer[at:], true + } + } + + return "", "", false +} diff --git a/ref_redirects_test.go b/ref_redirects_test.go new file mode 100644 index 0000000..ddb7474 --- /dev/null +++ b/ref_redirects_test.go @@ -0,0 +1,97 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "testing" + + "github.com/go-openapi/analysis" + "github.com/go-openapi/loads" + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +const chainedRefsFixture = `{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": { + "/a": {"get": {"operationId": "a", + "responses": {"200": {"$ref": "#/responses/shared"}}}}, + "/loop": {"get": {"operationId": "loop", + "responses": {"200": {"description": "ok", "schema": {"$ref": "#/definitions/Recursive"}}}}} + }, + "responses": {"shared": {"description": "ok", "schema": {"$ref": "#/definitions/A"}}}, + "definitions": { + "A": {"type": "object", "properties": {"n": {"type": "string"}}}, + "Recursive": {"type": "object", "properties": {"self": {"$ref": "#/definitions/Recursive"}}} + } +}` + +func redirectsOf(t *testing.T) refRedirects { + t.Helper() + + doc, err := loads.Analyzed(json.RawMessage(chainedRefsFixture), "") + require.NoError(t, err) + + return newRefRedirects(analysis.New(doc.Spec())) +} + +func TestRefRedirects_FollowsToWhatTheDocumentHolds(t *testing.T) { + t.Parallel() + + redirects := redirectsOf(t) + + t.Run("a pointer below a $ref is followed, as many times as it takes", func(t *testing.T) { + t.Parallel() + + // the response is a $ref to a shared response whose schema is itself a + // $ref: two hops before the pointer lands on something written down + assert.EqualT(t, "/definitions/A/properties/n", + redirects.through("/paths/~1a/get/responses/200/schema/properties/n")) + }) + + t.Run("a pointer that stops on the $ref is left alone", func(t *testing.T) { + t.Parallel() + + // that node exists, and it is where a reader goes to amend the reference + assert.EqualT(t, "/paths/~1a/get/responses/200", + redirects.through("/paths/~1a/get/responses/200")) + }) + + t.Run("a pointer that crosses no $ref is left alone", func(t *testing.T) { + t.Parallel() + + assert.EqualT(t, "/definitions/A/properties/n", + redirects.through("/definitions/A/properties/n")) + assert.EqualT(t, "", redirects.through("")) + }) + + t.Run("a definition referring to itself does not spin", func(t *testing.T) { + t.Parallel() + + // each hop shortens the pointer by "properties/self", so this bottoms + // out; the hop bound is what guarantees it whatever the document says + assert.EqualT(t, "/definitions/Recursive/properties/self", + redirects.through("/definitions/Recursive/properties/self/properties/self")) + }) +} + +func TestRefRedirects_IgnoresRemoteReferences(t *testing.T) { + t.Parallel() + + doc, err := loads.Analyzed(json.RawMessage(`{ + "swagger": "2.0", + "info": {"title": "t", "version": "1"}, + "paths": {"/a": {"get": {"operationId": "a", + "responses": {"200": {"description": "ok", "schema": {"$ref": "elsewhere.json#/definitions/A"}}}}}} +}`), "") + require.NoError(t, err) + + // nothing in this document says what a remote reference leads to, so the + // pointer is left as it stands and trimming has the last word + redirects := newRefRedirects(analysis.New(doc.Spec())) + assert.EqualT(t, "/paths/~1a/get/responses/200/schema/properties/n", + redirects.through("/paths/~1a/get/responses/200/schema/properties/n")) +} diff --git a/required_walk.go b/required_walk.go new file mode 100644 index 0000000..b3fd3cd --- /dev/null +++ b/required_walk.go @@ -0,0 +1,179 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "strings" + + "github.com/go-openapi/errors" + "github.com/go-openapi/spec" +) + +// schemaIdentity is how a message refers to the schema a required entry +// belongs to. +// +// A definition is named on its own, the way it always has been. A schema the +// definition holds is named by the way down to it, relative to the definition: +// "A.inner" rather than "A", which would send a reader to the wrong place. +type schemaIdentity struct { + name string + nested bool +} + +// identify names the schema a location leads to. +func identify(at pathSegments) schemaIdentity { + const definitionDepth = 2 // "definitions", then the name of one + + return schemaIdentity{ + name: strings.TrimPrefix(at.dotted(), swaggerDefinitions+"."), + nested: len(at) > definitionDepth, + } +} + +func (i schemaIdentity) requiredButNotDefined(property string) errors.Error { + if i.nested { + return requiredButNotDefinedInSchemaMsg(property, i.name) + } + + return requiredButNotDefinedMsg(property, i.name) +} + +// maxCompositionHops bounds how far the search for a declared property follows +// allOf members and the local $ref they may be written as. +const maxCompositionHops = 20 + +// walkRequired checks the required entries of a schema, then those of every +// schema it holds inline. +// +// A definition is not the only place a document says an object must hold a +// property: so does the schema of a property, of an array item, of an +// additionalProperties. Each of those is a self-contained object definition, +// and a required entry naming something it never declares is the same slip +// wherever it sits. +// +// A schema written as a $ref is left alone: it is checked where it is defined, +// and following it here would report the same slip twice and, for a recursive +// definition, would not terminate. +// +// It reports whether the walk should carry on, which is how the caller stops on +// the first fault unless it was asked for everything. +func (s *SpecValidator) walkRequired(at pathSegments, v *spec.Schema, res *Result) bool { + if v == nil || v.Ref.String() != "" { + return true + } + + for i, pn := range v.Required { + // the offending entry of the required array, not the schema holding + // it: that is what a reader has to go and amend + red := s.validateRequiredProperties(pn, identify(at), at, at.child(jsonRequired).item(i), v) + // NOTE: capture validity before merging: Merge may redeem `red` to the + // pool (wantsRedeemOnMerge), after which reading it races with a + // concurrent BorrowResult().cleared() in another goroutine. + isValid := red.IsValid() + res.Merge(red) + if !isValid && !s.Options.ContinueOnErrors { + return false + } + } + + return s.walkInlineSchemas(at, v, res) +} + +// walkInlineSchemas descends into every schema a schema holds, without checking +// the required entries of a composition member. +// +// Inside allOf, anyOf, oneOf or not, a member is a fragment of a constraint +// rather than a complete definition: its required entries speak of the instance +// the whole composition describes, and are legitimately met by a sibling member +// or by no declaration at all. Those are honoured when data is validated, and +// saying anything about them here would be wrong. Their own members are still +// walked, because a property schema nested in one of them is a definition like +// any other. +func (s *SpecValidator) walkInlineSchemas(at pathSegments, v *spec.Schema, res *Result) bool { + for _, name := range sortedKeys(v.Properties) { + held := v.Properties[name] + if !s.walkRequired(at.structuralChild(jsonProperties).child(name), &held, res) { + return false + } + } + + for _, pattern := range sortedKeys(v.PatternProperties) { + held := v.PatternProperties[pattern] + if !s.walkRequired(at.structuralChild(jsonPatternProperties).child(pattern), &held, res) { + return false + } + } + + if v.Items != nil { + if v.Items.Schema != nil && !s.walkRequired(at.child(jsonItems), v.Items.Schema, res) { + return false + } + for i := range v.Items.Schemas { + if !s.walkRequired(at.child(jsonItems).item(i), &v.Items.Schemas[i], res) { + return false + } + } + } + + if v.AdditionalProperties != nil && v.AdditionalProperties.Schema != nil && + !s.walkRequired(at.child(jsonAdditionalProperties), v.AdditionalProperties.Schema, res) { + return false + } + + // a composition member is walked for the schemas it holds, never for its + // own required entries + for _, composition := range []struct { + keyword string + members []spec.Schema + }{ + {jsonAllOf, v.AllOf}, + {jsonAnyOf, v.AnyOf}, + {jsonOneOf, v.OneOf}, + } { + for i := range composition.members { + if !s.walkInlineSchemas(at.child(composition.keyword).item(i), &composition.members[i], res) { + return false + } + } + } + + if v.Not != nil { + return s.walkInlineSchemas(at.child(jsonNot), v.Not, res) + } + + return true +} + +// declaresProperty reports whether a schema, or any schema composed into it by +// allOf, declares the named property, and whether that declaration is readOnly. +// +// An allOf member may be written as a $ref, which is followed here: a property +// contributed by a base definition is declared just as plainly as one written +// in place. +func (s *SpecValidator) declaresProperty(v *spec.Schema, name string, hops int) (readOnly, declared bool) { + if v == nil || hops <= 0 { + return false, false + } + + if held, ok := v.Properties[name]; ok { + return held.ReadOnly, 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 readOnly, ok := s.declaresProperty(member, name, hops-1); ok { + return readOnly, true + } + } + + return false, false +} diff --git a/required_walk_test.go b/required_walk_test.go new file mode 100644 index 0000000..1809b97 --- /dev/null +++ b/required_walk_test.go @@ -0,0 +1,218 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "testing" + + "github.com/go-openapi/testify/v2/assert" +) + +// definitionsDoc wraps a definitions block into the smallest document that +// reaches it, so that a case reads as the definitions it is about. +func definitionsDoc(definitions string) string { + return `{"swagger":"2.0","info":{"title":"t","version":"1"}, + "paths":{"/a":{"get":{"operationId":"a","responses":{"200":{"description":"ok", + "schema":{"$ref":"#/definitions/A"}}}}}}, + "definitions":` + definitions + `}` +} + +// A required entry naming a property the schema never declares is a modelling +// slip wherever the schema sits, not only at the top of a definition. +func TestRequiredWalk_DescendsIntoNestedSchemas(t *testing.T) { + t.Parallel() + + for _, testCase := range []struct { + name string + definitions string + // the message names the schema by the way down to it, so that it does + // not send a reader to the definition holding it + schema string + pointer string + }{ + { + name: "the schema of a property", + definitions: `{"A":{"type":"object","properties":{ + "inner":{"type":"object","required":["ghost"],"properties":{"real":{"type":"string"}}}}}}`, + schema: "A.inner", + pointer: "/definitions/A/properties/inner/required/0", + }, + { + name: "the schema of an array item", + definitions: `{"A":{"type":"object","properties":{ + "list":{"type":"array","items":{"type":"object","required":["ghost"],"properties":{"real":{"type":"string"}}}}}}}`, + schema: "A.list.items", + pointer: "/definitions/A/properties/list/items/required/0", + }, + { + name: "the schema of additionalProperties", + definitions: `{"A":{"type":"object","additionalProperties": + {"type":"object","required":["ghost"],"properties":{"real":{"type":"string"}}}}}`, + schema: "A.additionalProperties", + pointer: "/definitions/A/additionalProperties/required/0", + }, + { + name: "a property schema nested in an allOf member", + definitions: `{"A":{"allOf":[{"type":"object","properties":{ + "inner":{"type":"object","required":["ghost"],"properties":{"real":{"type":"string"}}}}}]}}`, + schema: "A.allOf.0.inner", + pointer: "/definitions/A/allOf/0/properties/inner/required/0", + }, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, definitionsDoc(testCase.definitions)) + assert.EqualT(t, testCase.pointer, + found[`"ghost" is present in required but not defined as property in schema "`+testCase.schema+`"`]) + }) + } +} + +// Inside a composition, a required entry speaks of the instance the whole +// composition describes: it is legitimately met by a sibling member, or by no +// declaration at all, and data validation is what enforces it. +func TestRequiredWalk_LeavesCompositionAlone(t *testing.T) { + t.Parallel() + + for _, testCase := range []struct { + name string + definitions string + }{ + { + name: "allOf members each requiring what the other does not declare", + definitions: `{"A":{"allOf":[ + {"type":"object","required":["a"]}, + {"type":"object","required":["b"]}]}}`, + }, + { + name: "a required entry declared by a sibling allOf member", + definitions: `{"A":{"allOf":[ + {"type":"object","required":["a"]}, + {"type":"object","properties":{"a":{"type":"string"}}}]}}`, + }, + { + name: "a oneOf member requiring what it does not declare", + definitions: `{"A":{"type":"object","properties":{ + "inner":{"oneOf":[{"type":"object","required":["a"]},{"type":"object","required":["b"]}]}}}}`, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + for message := range locatedFindings(t, definitionsDoc(testCase.definitions)) { + assert.StringNotContainsT(t, message, "is present in required but not defined") + } + }) + } +} + +// A property contributed by a base definition is declared just as plainly as +// one written in place. +func TestRequiredWalk_CountsPropertiesContributedByAllOf(t *testing.T) { + t.Parallel() + + t.Run("declared by an allOf member written in place", func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, definitionsDoc(`{"A":{"type":"object","required":["a"], + "allOf":[{"type":"object","properties":{"a":{"type":"string"}}}]}}`)) + + for message := range found { + assert.StringNotContainsT(t, message, "is present in required but not defined") + } + }) + + t.Run("declared by an allOf member written as a $ref", func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, definitionsDoc(`{ + "Base":{"type":"object","properties":{"a":{"type":"string"}}}, + "A":{"type":"object","required":["a"],"allOf":[{"$ref":"#/definitions/Base"}]}}`)) + + for message := range found { + assert.StringNotContainsT(t, message, "is present in required but not defined") + } + }) + + t.Run("still reported when nothing in the composition declares it", func(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, definitionsDoc(`{ + "Base":{"type":"object","properties":{"other":{"type":"string"}}}, + "A":{"type":"object","required":["a"],"allOf":[{"$ref":"#/definitions/Base"}]}}`)) + + assert.EqualT(t, "/definitions/A/required/0", + found[`"a" is present in required but not defined as property in definition "A"`]) + }) +} + +func TestRequiredWalk_QuietWhenTheSchemaIsSound(t *testing.T) { + t.Parallel() + + for _, testCase := range []struct { + name string + definitions string + }{ + { + name: "the nested property is declared", + definitions: `{"A":{"type":"object","properties":{ + "inner":{"type":"object","required":["real"],"properties":{"real":{"type":"string"}}}}}}`, + }, + { + name: "the nested schema takes any property", + definitions: `{"A":{"type":"object","properties":{ + "inner":{"type":"object","required":["anything"],"additionalProperties":true}}}}`, + }, + { + name: "the nested schema is a $ref, checked where it is defined", + definitions: `{ + "Inner":{"type":"object","required":["real"],"properties":{"real":{"type":"string"}}}, + "A":{"type":"object","properties":{"inner":{"$ref":"#/definitions/Inner"}}}}`, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + for message := range locatedFindings(t, definitionsDoc(testCase.definitions)) { + assert.StringNotContainsT(t, message, "is present in required but not defined") + } + }) + } +} + +// A definition names itself, exactly as it always has. +func TestRequiredWalk_ADefinitionStillNamesItself(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, definitionsDoc(`{"A":{"type":"object","required":["ghost"], + "properties":{"real":{"type":"string"}}}}`)) + + assert.EqualT(t, "/definitions/A/required/0", + found[`"ghost" is present in required but not defined as property in definition "A"`]) +} + +// A recursive definition is reached through a $ref, which the walk does not +// follow, so it cannot spin — and the slip is still reported once, where the +// definition writes it. +func TestRequiredWalk_ReportsARecursiveDefinitionOnce(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, definitionsDoc(`{"A":{"type":"object","required":["ghost"], + "properties":{"self":{"$ref":"#/definitions/A"}}}}`)) + + assert.EqualT(t, "/definitions/A/required/0", + found[`"ghost" is present in required but not defined as property in definition "A"`]) +} + +// The readOnly warning follows the entry it is about, wherever that sits. +func TestRequiredWalk_ReadOnlyWarningIsLocatedToo(t *testing.T) { + t.Parallel() + + found := locatedFindings(t, definitionsDoc(`{"A":{"type":"object","properties":{ + "inner":{"type":"object","required":["ro"],"properties":{"ro":{"type":"string","readOnly":true}}}}}}`)) + + assert.EqualT(t, "/definitions/A/properties/inner/required/0", + found[`Required property ro in "A.inner" should not be marked as both required and readOnly`]) +} diff --git a/resolvable.go b/resolvable.go new file mode 100644 index 0000000..bbd35d3 --- /dev/null +++ b/resolvable.go @@ -0,0 +1,70 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "strconv" + "strings" + + "github.com/go-openapi/jsonpointer" +) + +// resolvable trims a pointer down to the deepest node the document holds. +// +// Checks walk an expanded, model-level view of a specification, which holds +// members the document itself never wrote: a parameter merged in from a path +// item, a member of a schema reached through a $ref. A pointer built along the +// way may therefore end on something a reader cannot go to. +// +// Trimming is the last word on a location, applied once every check has had its +// say: it only ever shortens, so a pointer that already addressed a node comes +// back untouched, and one that did not still says as much as it truthfully can. +// This is what makes [Located.Pointer] always resolve. +func (s *SpecValidator) resolvable(pointer string) string { + if pointer == "" || s.document == nil { + return pointer + } + + node := s.document + for at := 0; at < len(pointer); { + end := strings.IndexByte(pointer[at+1:], '/') + token := pointer[at+1:] + if end >= 0 { + token = pointer[at+1 : at+1+end] + } + + member, isHeld := memberOf(node, jsonpointer.Unescape(token)) + if !isHeld { + return pointer[:at] + } + node = member + + if end < 0 { + break + } + at += end + 1 + } + + return pointer +} + +// memberOf returns the member a reference token addresses in a decoded JSON +// node, and whether the node holds one at all. +func memberOf(node any, token string) (any, bool) { + switch held := node.(type) { + case map[string]any: + member, isHeld := held[token] + + return member, isHeld + case []any: + index, err := strconv.Atoi(token) + if err != nil || index < 0 || index >= len(held) { + return nil, false + } + + return held[index], true + default: + return nil, false + } +} diff --git a/resolvable_test.go b/resolvable_test.go new file mode 100644 index 0000000..687319f --- /dev/null +++ b/resolvable_test.go @@ -0,0 +1,85 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "encoding/json" + "testing" + + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +const ( + firstParam = "/paths/~1a/get/parameters/0" + firstParamName = firstParam + "/name" +) + +func validatorHoldingDocument(t *testing.T, raw string) *SpecValidator { + t.Helper() + + var document any + require.NoError(t, json.Unmarshal([]byte(raw), &document)) + + return &SpecValidator{document: document} +} + +func TestResolvable_TrimsToWhatTheDocumentHolds(t *testing.T) { + t.Parallel() + + s := validatorHoldingDocument(t, `{ + "paths": {"/a": {"get": {"parameters": [{"name": "p", "in": "query"}]}}}, + "definitions": {"a~b": {"type": "object"}} +}`) + + for _, testCase := range []struct { + name string + pointer string + want string + }{ + {"a pointer that addresses a node is untouched", firstParamName, firstParamName}, + {"a member the node does not hold is cut off", firstParam + "/type", firstParam}, + {"everything below the first miss goes too", firstParam + "/schema/properties/n", firstParam}, + {"an index past the end of an array is cut off", "/paths/~1a/get/parameters/1", "/paths/~1a/get/parameters"}, + {"a member of a scalar is cut off", firstParamName + "/deeper", firstParamName}, + {"an escaped token is unescaped before the lookup", "/definitions/a~0b/type", "/definitions/a~0b/type"}, + {"a miss at the first token leaves the root", "/nowhere/at/all", ""}, + {"the root is the root", "", ""}, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + assert.EqualT(t, testCase.want, s.resolvable(testCase.pointer)) + }) + } +} + +func TestResolvable_WithoutADocumentSaysNothing(t *testing.T) { + t.Parallel() + + // nothing is known about what holds what, so no location may be shortened + s := new(SpecValidator) + assert.EqualT(t, "/paths/~1a", s.resolvable("/paths/~1a")) +} + +func TestPathSegments_CosmeticTokensStayOutOfThePointer(t *testing.T) { + t.Parallel() + + at := newPathSegments(swaggerPaths, "/a", "get").child(swaggerParameters).cosmeticChild("broken") + + assert.EqualT(t, "/paths/~1a/get/parameters", at.pointer()) + assert.EqualT(t, "paths./a.get.parameters.broken", at.dotted()) + + t.Run("nothing below a cosmetic token is addressable either", func(t *testing.T) { + t.Parallel() + + below := at.child(jsonType) + assert.EqualT(t, "/paths/~1a/get/parameters", below.pointer()) + assert.EqualT(t, "paths./a.get.parameters.broken.type", below.dotted()) + + deeper := at.children(jsonSchema, jsonProperties) + assert.EqualT(t, "/paths/~1a/get/parameters", deeper.pointer()) + assert.EqualT(t, "paths./a.get.parameters.broken.schema.properties", deeper.dotted()) + }) +} diff --git a/result.go b/result.go index a4834b4..5684774 100644 --- a/result.go +++ b/result.go @@ -362,6 +362,14 @@ func (r *Result) AsError() error { return errors.CompositeValidationError(r.Errors...) } +// Reset clears this result so it may be reused, keeping allocated capacity. +// +// It implements the hook the pool calls when a result is borrowed and when it +// is redeemed. Calling it on a result still in use loses its findings. +func (r *Result) Reset() { + _ = r.cleared() +} + // addErrorsAt adds errors located at the given path. func (r *Result) addErrorsAt(at pathSegments, errors ...error) { r.addLocatedErrors(at.pointer(), errors...) @@ -402,6 +410,43 @@ func (r *Result) addLocatedWarnings(pointer string, warnings ...error) { } } +// relocate rewrites every location this result recorded. +// +// The parameter and header validators are the ones a generated client uses at +// runtime, so they locate a finding by the name of the parameter or header it +// concerns: a name is all the caller has. When spec validation borrows them to +// check a default or an example, that name addresses nothing in the document, +// and the value's own node is the best location available for everything the +// borrowed validator found. +func (r *Result) relocate(at pathSegments) { + if r == nil { + return + } + + pointer := at.pointer() + r.errorLocations = fillLocations(r.errorLocations[:0], len(r.Errors), pointer) + r.warningLocations = fillLocations(r.warningLocations[:0], len(r.Warnings), pointer) +} + +// fillLocations records the same location for a whole run of findings. +func fillLocations(locations []string, count int, pointer string) []string { + for range count { + locations = append(locations, pointer) + } + + return locations +} + +// redirect rewrites every location this result recorded with the given mapping. +func (r *Result) redirect(through func(string) string) { + for i, pointer := range r.errorLocations { + r.errorLocations[i] = through(pointer) + } + for i, pointer := range r.warningLocations { + r.warningLocations[i] = through(pointer) + } +} + // carryErrors adds errors from another result as errors, one by one, so that // each keeps the location that result recorded for it. func (r *Result) carryErrors(errs []error, locations []string) { @@ -585,14 +630,6 @@ func (r *Result) keepRelevantErrors() *Result { return strippedResult } -// Reset clears this result so it may be reused, keeping allocated capacity. -// -// It implements the hook the pool calls when a result is borrowed and when it -// is redeemed. Calling it on a result still in use loses its findings. -func (r *Result) Reset() { - _ = r.cleared() -} - func (r *Result) cleared() *Result { // clear the Result to be reusable. Keep allocated capacity. r.Errors = r.Errors[:0] diff --git a/schema_props.go b/schema_props.go index 83a3735..d7ecb14 100644 --- a/schema_props.go +++ b/schema_props.go @@ -279,7 +279,7 @@ func (s *schemaPropsValidator) validateNot(data any, mainResult *Result) { func (s *schemaPropsValidator) validateDependencies(data any, mainResult *Result) { val := data.(map[string]any) //nolint:forcetypeassert // caller guarantees map[string]any - for key := range val { + for _, key := range sortedKeys(val) { dep, ok := s.Dependencies[key] if !ok { continue diff --git a/sorted.go b/sorted.go new file mode 100644 index 0000000..829a042 --- /dev/null +++ b/sorted.go @@ -0,0 +1,39 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package validate + +import ( + "cmp" + "maps" + "slices" + "strings" + + "github.com/go-openapi/spec" +) + +// sortedKeys returns the keys of a map in ascending order. +// +// Findings are reported in the order the checks walk the document, and several +// of them walk a map: definitions, paths, response codes, headers, properties. +// Go randomises map iteration, so the same bytes validated twice would list +// their findings in a different order — and where a check stops on the first +// fault it meets, would name a different offender altogether. +// +// Walking keys in sorted order makes both defined: findings come out in +// definition-name (or path, or status-code) order, on every run. +func sortedKeys[K cmp.Ordered, V any](m map[K]V) []K { + return slices.Sorted(maps.Keys(m)) +} + +// sortedRefs orders references by the location they point at. +// +// The analyzer gathers references in a map, so the slice it hands back comes +// out in a different order on every run. See [sortedKeys]. +func sortedRefs(refs []spec.Ref) []spec.Ref { + slices.SortFunc(refs, func(a, b spec.Ref) int { + return strings.Compare(a.String(), b.String()) + }) + + return refs +} diff --git a/spec.go b/spec.go index f2ced4c..e5d16b6 100644 --- a/spec.go +++ b/spec.go @@ -54,7 +54,9 @@ type SpecValidator struct { analyzer *analysis.Spec expanded *loads.Document refLocations refLocations + refRedirects refRedirects paramLocations paramLocations + document any // the document as decoded, to tell what it holds KnownFormats strfmt.Registry Options Opts // validation options schemaOptions *SchemaValidatorOptions @@ -107,6 +109,9 @@ func (s *SpecValidator) Validate(data any) (*Result, *Result) { // where each operation declares its parameters: the document addresses // them by index, and expansion loses that s.paramLocations = newParamLocations(sd.Spec()) + // where each $ref leads: checks walk the expanded document, and a finding + // below a $ref has to be brought back to a node the document contains + s.refRedirects = newRefRedirects(s.analyzer) // Raw spec unmarshalling errors var obj any @@ -115,8 +120,13 @@ func (s *SpecValidator) Validate(data any) (*Result, *Result) { // So this one is just a paranoid check on the behavior of the spec package panic(InvalidDocumentError) } + s.document = obj defer func() { + // bring findings reached through a $ref back onto the document, then + // hold every location to what the document actually addresses + errs.redirect(s.refRedirects.through) + errs.redirect(s.resolvable) // errs holds all errors and warnings, // warnings only warnings errs.MergeAsWarnings(warnings) @@ -180,8 +190,9 @@ func (s *SpecValidator) SetContinueOnErrors(c bool) { func (s *SpecValidator) validateNonEmptyPathParamNames() *Result { res := validatorPools.results.Borrow() if s.spec.Spec().Paths == nil { - // There is no Paths object: error - res.addErrorsAt(newPathSegments(swaggerPaths), noValidPathMsg()) + // There is no Paths object: the document itself is what lacks it, so + // there is no node below it to point at + res.addErrorsAt(rootPath(), noValidPathMsg()) return res } @@ -193,7 +204,7 @@ func (s *SpecValidator) validateNonEmptyPathParamNames() *Result { return res } - for k := range s.spec.Spec().Paths.Paths { + for _, k := range sortedKeys(s.spec.Spec().Paths.Paths) { if strings.Contains(k, "{}") { res.addErrorsAt(newPathSegments(swaggerPaths, k), emptyPathParameterMsg(k)) } @@ -213,20 +224,54 @@ func (s *SpecValidator) validateDuplicateOperationIDs() *Result { analyzer = s.analyzer } res := validatorPools.results.Borrow() + + // the message says how many times an identifier is used, so the count is + // what it needs; a reader needs somewhere to go, so the first operation to + // declare the identifier is remembered along with it known := make(map[string]int) - for _, v := range analyzer.OperationIDs() { - if v != "" { - known[v]++ + declaredAt := make(map[string]pathSegments) + operations := analyzer.Operations() + for _, method := range sortedKeys(operations) { + byPath := operations[method] + for _, path := range sortedKeys(byPath) { + op := byPath[path] + id := operationIdentity(method, path, op) + known[id]++ + if _, isKnown := declaredAt[id]; !isKnown { + declaredAt[id] = operationIDPath(path, method, op) + } } } - for k, v := range known { - if v > 1 { - res.AddErrors(nonUniqueOperationIDMsg(k, v)) + + for _, k := range sortedKeys(known) { + if v := known[k]; v > 1 { + res.addErrorsAt(declaredAt[k], nonUniqueOperationIDMsg(k, v)) } } return res } +// operationIdentity names an operation the way the analyzer does: by its +// operationId, or by method and path when it declares none. +func operationIdentity(method, path string, op *spec.Operation) string { + if op == nil || op.ID == "" { + return strings.ToUpper(method) + " " + path + } + + return op.ID +} + +// operationIDPath locates the operationId of an operation, or the operation +// itself when it declares none. +func operationIDPath(path, method string, op *spec.Operation) pathSegments { + at := operationPath(path, method) + if op == nil || op.ID == "" { + return at + } + + return at.child(swaggerOperationID) +} + type dupProp struct { Name string Definition string @@ -235,7 +280,9 @@ type dupProp struct { func (s *SpecValidator) validateDuplicatePropertyNames() *Result { // definition can't declare a property that's already defined by one of its ancestors res := validatorPools.results.Borrow() - for k, sch := range s.spec.Spec().Definitions { + definitions := s.spec.Spec().Definitions + for _, k := range sortedKeys(definitions) { + sch := definitions[k] if len(sch.AllOf) == 0 { continue } @@ -250,7 +297,14 @@ func (s *SpecValidator) validateDuplicatePropertyNames() *Result { } if len(ancs) > 0 { res.addErrorsAt(newPathSegments(swaggerDefinitions, k), circularAncestryDefinitionMsg(k, ancs)) - return res + if !s.Options.ContinueOnErrors { + return res + } + + // the ancestry loops back on itself: searching it for duplicate + // property names would not terminate, so this definition stops here + // and the next one is examined. + continue } knowns := make(map[string]struct{}) @@ -307,7 +361,7 @@ func (s *SpecValidator) validateSchemaPropertyNames(nm string, sch spec.Schema, return dups, res } - for k := range schc.Properties { + for _, k := range sortedKeys(schc.Properties) { _, ok := knowns[k] if ok { dups = append(dups, dupProp{Name: k, Definition: schn}) @@ -373,8 +427,11 @@ func (s *SpecValidator) validateItems() *Result { // validate parameter, items, schema and response objects for presence of item if type is array res := validatorPools.results.Borrow() - for method, pi := range s.analyzer.Operations() { - for path, op := range pi { + operations := s.analyzer.Operations() + for _, method := range sortedKeys(operations) { + pi := operations[method] + for _, path := range sortedKeys(pi) { + op := pi[path] for _, param := range paramHelp.safeExpandedParamsFor(path, method, op.ID, res, s) { if param.TypeName() == arrayType && param.ItemsTypeName() == "" { @@ -411,8 +468,11 @@ func (s *SpecValidator) validateItems() *Result { responses = append(responses, codedResponse{code: jsonDefault, resp: *op.Responses.Default}) } if op.Responses.StatusCodeResponses != nil { - for code, v := range op.Responses.StatusCodeResponses { - responses = append(responses, codedResponse{code: strconv.Itoa(code), resp: v}) + for _, code := range sortedKeys(op.Responses.StatusCodeResponses) { + responses = append(responses, codedResponse{ + code: strconv.Itoa(code), + resp: op.Responses.StatusCodeResponses[code], + }) } } } @@ -420,8 +480,8 @@ func (s *SpecValidator) validateItems() *Result { for _, resp := range responses { at := responsePath(path, method, resp.code) // Response headers with array - for hn, hv := range resp.resp.Headers { - if hv.TypeName() == arrayType && hv.ItemsTypeName() == "" { + for _, hn := range sortedKeys(resp.resp.Headers) { + if hv := resp.resp.Headers[hn]; hv.TypeName() == arrayType && hv.ItemsTypeName() == "" { res.addErrorsAt(at.children(swaggerHeaders, hn), arrayInHeaderRequiresItemsMsg(hn, op.ID)) } } @@ -470,7 +530,7 @@ func (s *SpecValidator) validatePathParamPresence(path string, fromPath, fromOpe } } if !matched { - res.addErrorsAt(newPathSegments(swaggerPaths, l), noParameterInPathMsg(l)) + res.addErrorsAt(newPathSegments(swaggerPaths, path), noParameterInPathMsg(l)) } } @@ -514,7 +574,7 @@ func (s *SpecValidator) validateReferencedParameters() *Result { return nil } result := validatorPools.results.Borrow() - for k := range expected { + for _, k := range sortedKeys(expected) { result.addWarningsAt(localRefPath(k), unusedParamMsg(k)) } return result @@ -538,10 +598,12 @@ func (s *SpecValidator) validateReferencedResponses() *Result { if len(expected) == 0 { return nil } + result := validatorPools.results.Borrow() - for k := range expected { + for _, k := range sortedKeys(expected) { result.addWarningsAt(localRefPath(k), unusedResponseMsg(k)) } + return result } @@ -565,7 +627,7 @@ func (s *SpecValidator) validateReferencedDefinitions() *Result { } result := new(Result) - for k := range expected { + for _, k := range sortedKeys(expected) { result.addWarningsAt(localRefPath(k), unusedDefinitionMsg(k)) } return result @@ -575,37 +637,30 @@ func (s *SpecValidator) validateRequiredDefinitions() *Result { // Each property listed in the required array must be defined in the properties of the model res := validatorPools.results.Borrow() + definitions := s.spec.Spec().Definitions + DEFINITIONS: - for d, schema := range s.spec.Spec().Definitions { - if schema.Required != nil { // Safeguard - definitionAt := newPathSegments(swaggerDefinitions, d) - for i, pn := range schema.Required { - // the offending entry of the required array, not the definition - // holding it: that is what a reader has to go and amend - requiredAt := definitionAt.child(jsonRequired).item(i) - red := s.validateRequiredProperties(pn, d, definitionAt, requiredAt, &schema) //#nosec - // NOTE: capture validity before merging: Merge may redeem `red` to the - // pool (wantsRedeemOnMerge), after which reading it races with a concurrent - // BorrowResult().cleared() in another goroutine sharing the global pool. - isValid := red.IsValid() - res.Merge(red) - if !isValid && !s.Options.ContinueOnErrors { - break DEFINITIONS // there is an error, let's stop that bleeding - } - } + for _, d := range sortedKeys(definitions) { + schema := definitions[d] + red := validatorPools.results.Borrow() + keepGoing := s.walkRequired(newPathSegments(swaggerDefinitions, d), &schema, red) //#nosec + res.Merge(red) + if !keepGoing { + break DEFINITIONS // there is an error, let's stop that bleeding } } return res } -// validateRequiredProperties checks one entry of a definition's required array. +// validateRequiredProperties checks one entry of a required array. // // schemaAt locates the schema being searched for the property, which moves as // the search descends into additionalProperties. requiredAt locates the entry // of the required array that started it, and stays put. func (s *SpecValidator) validateRequiredProperties( - path, in string, schemaAt, requiredAt pathSegments, v *spec.Schema, + path string, of schemaIdentity, schemaAt, requiredAt pathSegments, v *spec.Schema, ) *Result { + in := of.name // Takes care of recursive property definitions, which may be nested in additionalProperties schemas res := validatorPools.results.Borrow() propertyMatch := false @@ -613,22 +668,22 @@ func (s *SpecValidator) validateRequiredProperties( additionalPropertiesMatch := false isReadOnly := false - // Regular properties - if _, ok := v.Properties[path]; ok { + // Regular properties, including those a base definition contributes + if readOnly, declared := s.declaresProperty(v, path, maxCompositionHops); declared { propertyMatch = true - isReadOnly = v.Properties[path].ReadOnly + isReadOnly = readOnly } // NOTE: patternProperties are not supported in swagger. Even though, we continue validation here // We check all defined patterns: if one regexp is invalid, croaks an error - for pp, pv := range v.PatternProperties { + for _, pp := range sortedKeys(v.PatternProperties) { re, err := compileRegexp(pp) if err != nil { res.addErrorsAt(schemaAt, invalidPatternMsg(pp, in)) } else if re.MatchString(path) { patternMatch = true if !propertyMatch { - isReadOnly = pv.ReadOnly + isReadOnly = v.PatternProperties[pp].ReadOnly } } } @@ -641,7 +696,7 @@ func (s *SpecValidator) validateRequiredProperties( // additionalProperties as schema are upported in swagger // recursively validates additionalProperties schema // Proposal for enhancement: anyOf, allOf, oneOf like in schemaPropsValidator - red := s.validateRequiredProperties(path, in, schemaAt.child(jsonAdditionalProperties), requiredAt, v.AdditionalProperties.Schema) + red := s.validateRequiredProperties(path, of, schemaAt.child(jsonAdditionalProperties), requiredAt, v.AdditionalProperties.Schema) if red.IsValid() { additionalPropertiesMatch = true if !propertyMatch && !patternMatch { @@ -654,7 +709,7 @@ func (s *SpecValidator) validateRequiredProperties( } if !propertyMatch && !patternMatch && !additionalPropertiesMatch { - res.addErrorsAt(requiredAt, requiredButNotDefinedMsg(path, in)) + res.addErrorsAt(requiredAt, of.requiredButNotDefined(path)) } if isReadOnly { @@ -676,9 +731,12 @@ func (s *SpecValidator) validateParameters() *Result { // - path param must be required res := validatorPools.results.Borrow() rexGarbledPathSegment := mustCompileRegexp(`.*[{}\s]+.*`) - for method, pi := range s.expandedAnalyzer().Operations() { + operations := s.expandedAnalyzer().Operations() + for _, method := range sortedKeys(operations) { + pi := operations[method] methodPaths := make(map[string]map[string]string) - for path, op := range pi { + for _, path := range sortedKeys(pi) { + op := pi[path] if s.Options.StrictPathParamUniqueness { pathToAdd := pathHelp.stripParametersInPath(path) @@ -817,7 +875,7 @@ func (s *SpecValidator) validateParameters() *Result { func (s *SpecValidator) validateReferencesValid() *Result { // each reference must point to a valid object res := validatorPools.results.Borrow() - for _, r := range s.analyzer.AllRefs() { + for _, r := range sortedRefs(s.analyzer.AllRefs()) { if !r.IsValidURI(s.spec.SpecFilePath()) { // Safeguard - spec should always yield a valid URI res.addErrorsAt(s.refLocations.at(r.String()), invalidRefMsg(r.String())) } @@ -831,13 +889,48 @@ func (s *SpecValidator) validateReferencesValid() *Result { // is set, this is a no-op: loads falls back to the document's own loader. exp, err := s.spec.Expanded(s.schemaOptions.expandOptions("")) if err != nil { - res.AddErrors(unresolvedReferencesMsg(err)) + res.addErrorsAt(s.firstUnresolvableRef(), unresolvedReferencesMsg(err)) } s.expanded = exp } return res } +// firstUnresolvableRef locates the declaration of the first local $ref, in +// document order, that points at a node the document does not hold. +// +// Expansion reports the whole document in a single message, naming only the +// reference it happened to trip on, so the finding has no location of its own. +// A document usually has one broken reference; when it has several, this is the +// first one a reader would meet. +func (s *SpecValidator) firstUnresolvableRef() pathSegments { + first := rootPath() + found := false + + for _, r := range s.analyzer.AllRefs() { + value := r.String() + if !strings.HasPrefix(value, "#/") { + // a remote reference cannot be checked against the document alone + continue + } + + pointer, err := jsonpointer.New(strings.TrimPrefix(value, "#")) + if err != nil { + continue + } + if _, _, err := pointer.Get(s.document); err == nil { + continue + } + + at := s.refLocations.at(value) + if !found || at.pointer() < first.pointer() { + first, found = at, true + } + } + + return first +} + func (s *SpecValidator) checkUniqueParams(path, method string, op *spec.Operation) *Result { // Check for duplicate parameters declaration in param section. // Each parameter should have a unique `name` and `type` combination diff --git a/spec_messages.go b/spec_messages.go index eeb8a86..0a0739a 100644 --- a/spec_messages.go +++ b/spec_messages.go @@ -132,6 +132,9 @@ const ( // RequiredButNotDefinedError ... RequiredButNotDefinedError = "%q is present in required but not defined as property in definition %q" + // RequiredButNotDefinedInSchemaError is the same slip, in a schema a definition holds rather than + // in the definition itself. + RequiredButNotDefinedInSchemaError = "%q is present in required but not defined as property in schema %q" // 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" @@ -260,6 +263,10 @@ func requiredButNotDefinedMsg(path, definition string) errors.Error { return errors.New(errors.CompositeErrorCode, RequiredButNotDefinedError, path, definition) } +func requiredButNotDefinedInSchemaMsg(path, schema string) errors.Error { + return errors.New(errors.CompositeErrorCode, RequiredButNotDefinedInSchemaError, path, schema) +} + func pathParamGarbledMsg(path, param string) errors.Error { return errors.New(errors.CompositeErrorCode, PathParamGarbledWarning, path, param) } diff --git a/spec_ref_warnings.go b/spec_ref_warnings.go index 3c8608f..d56cb17 100644 --- a/spec_ref_warnings.go +++ b/spec_ref_warnings.go @@ -39,7 +39,7 @@ func (s *SpecValidator) validateDubiousRefs() *Result { baseDir, hasBase := s.localBaseDir() remoteHosts := make(map[string]struct{}) - for _, r := range s.analyzer.AllRefs() { + for _, r := range sortedRefs(s.analyzer.AllRefs()) { u := r.GetURL() if u == nil { // Safeguard: a valid spec always yields parseable refs continue