From 79ebba2d8372c6a3f211b285f810b2a700e025c9 Mon Sep 17 00:00:00 2001 From: Robbie Coleman Date: Sat, 1 Aug 2026 20:48:38 -0700 Subject: [PATCH 1/2] feat(validator): enforce header names, allowReserved placement and non-empty server enums Six of the eleven missed negatives in A2, chosen because none of them needs machinery oastools does not have. The OAS 3.2 negative suite moves from 18/29 to 23/29, the first movement on that axis in this project. Positives hold at 34/37 and no corpus verdict changes. Header names must be RFC 9110 tokens. The rule covers header map keys and a header parameter's name, both of which are field names on the wire. allowReserved is rejected where in and style do not permit it, and on a Header Object, where no version permits it at all. A server variable enum that is present but empty permits no value, so not even the required default could satisfy it. Every one of the three turned out to be version-scoped, and two changed between versions: - the RFC 9110 token definition does not exist in the 3.1 schema, so the header-name rule is 3.2+ only - allowReserved permits in: query only at 3.1, and widened at 3.2 to in: path, in: query, and in: cookie with style: form - the server variable enum gained minItems: 1 at 3.1 A single ungated rule would have produced a new false positive in one direction or a miss in the other, which is exactly the defect class #433 fixed. #440 is the issue that makes this class representable rather than checked by hand each time. The rules hook into schema_traversal.go rather than individual call sites, so they inherit the structural reachability that walk exists to provide: a header is a header wherever it occurs. Asserted across all six positions rather than assumed, since #423 was the case where a correct rule simply never ran in most places its object appears. specBaseURL is extracted from validateOAS3, which held the version to URL switch inline and so kept it out of reach of traversals that carry no baseURL parameter. That is why the first draft of this change hardcoded 3.2 URLs. The convention now has one statement: cite the document's own version when a rule varies by version, and oas32SpecRef when a rule exists only at 3.2. parser.Parameter.AllowReserved gains a doc comment recording where it diverges from the specification. The rule is about the key's presence rather than its value, and a bool cannot express presence, so an illegal `allowReserved: false` decodes the same as an absent field. The OAI fixture that uses false is therefore still uncaught. Changing the type would break the v1 API. Refs #434 --- parser/parameters.go | 13 +- validator/oas3.go | 44 +- validator/schema_traversal.go | 16 +- validator/serialization_constraints.go | 137 ++++ validator/serialization_constraints_test.go | 765 ++++++++++++++++++++ validator/spec_refs.go | 67 ++ validator/spec_refs_test.go | 79 ++ 7 files changed, 1091 insertions(+), 30 deletions(-) create mode 100644 validator/serialization_constraints.go create mode 100644 validator/serialization_constraints_test.go create mode 100644 validator/spec_refs.go create mode 100644 validator/spec_refs_test.go diff --git a/parser/parameters.go b/parser/parameters.go index f115e85c..02b36998 100644 --- a/parser/parameters.go +++ b/parser/parameters.go @@ -17,8 +17,17 @@ type Parameter struct { Deprecated bool `yaml:"deprecated,omitempty" json:"deprecated,omitempty"` // OAS 3.0+ // OAS 3.0+ fields - Style string `yaml:"style,omitempty" json:"style,omitempty"` - Explode *bool `yaml:"explode,omitempty" json:"explode,omitempty"` + Style string `yaml:"style,omitempty" json:"style,omitempty"` + Explode *bool `yaml:"explode,omitempty" json:"explode,omitempty"` + + // AllowReserved lets RFC 3986 reserved characters pass through a parameter + // value unencoded. Where it may legally appear depends on `in` and `style`; + // the validator's allowReservedPermitted holds the per-version table. + // + // Note: the specification constrains the field's presence rather than its + // value, which a bool cannot express. An illegal `allowReserved: false` + // decodes the same as an absent field, so oastools catches the violation + // only when the value is true. Changing the type would break the v1 API. AllowReserved bool `yaml:"allowReserved,omitempty" json:"allowReserved,omitempty"` Schema *Schema `yaml:"schema,omitempty" json:"schema,omitempty"` Example any `yaml:"example,omitempty" json:"example,omitempty"` diff --git a/validator/oas3.go b/validator/oas3.go index 0748a8a5..9869ef29 100644 --- a/validator/oas3.go +++ b/validator/oas3.go @@ -13,32 +13,7 @@ import ( // validateOAS3 performs OAS 3.x specific validation func (v *Validator) validateOAS3(doc *parser.OAS3Document, result *ValidationResult) { - version := doc.OpenAPI - var baseURL string - - // Determine the correct spec URL based on version - switch doc.OASVersion { - case parser.OASVersion300: - baseURL = "https://spec.openapis.org/oas/v3.0.0.html" - case parser.OASVersion301: - baseURL = "https://spec.openapis.org/oas/v3.0.1.html" - case parser.OASVersion302: - baseURL = "https://spec.openapis.org/oas/v3.0.2.html" - case parser.OASVersion303: - baseURL = "https://spec.openapis.org/oas/v3.0.3.html" - case parser.OASVersion304: - baseURL = "https://spec.openapis.org/oas/v3.0.4.html" - case parser.OASVersion310: - baseURL = "https://spec.openapis.org/oas/v3.1.0.html" - case parser.OASVersion311: - baseURL = "https://spec.openapis.org/oas/v3.1.1.html" - case parser.OASVersion312: - baseURL = "https://spec.openapis.org/oas/v3.1.2.html" - case parser.OASVersion320: - baseURL = "https://spec.openapis.org/oas/v3.2.0.html" - default: - baseURL = fmt.Sprintf("https://spec.openapis.org/oas/v%s.html", version) - } + baseURL := specBaseURL(doc.OASVersion, doc.OpenAPI) // Validate required fields in info object v.validateOAS3Info(doc, result, baseURL) @@ -173,6 +148,23 @@ func (v *Validator) validateOAS3Servers(doc *parser.OAS3Document, result *Valida ) } + // An enum that is present but empty permits no value at all, which + // cannot be satisfied (the default itself could never be a member). + // OAS 3.1 added `minItems: 1` to the Server Variable Object's enum; + // 3.0's schema has no such constraint, so this is gated. + // + // Presence, not length, is the test: an absent enum means "any + // value" and is fine. The parser keeps the two apart: absent + // decodes to a nil slice, `enum: []` to an empty non-nil one. + if varObj.Enum != nil && len(varObj.Enum) == 0 && + v.oasVersion.IsValid() && v.oasVersion >= parser.OASVersion310 { + v.addError(result, varPath, + "Server variable enum must not be empty; omit it to allow any value", + withSpecRef(fmt.Sprintf("%s#server-variable-object", baseURL)), + withField("enum"), + ) + } + // If enum is specified, default must be in enum if len(varObj.Enum) > 0 && !slices.Contains(varObj.Enum, varObj.Default) { v.addError(result, varPath, diff --git a/validator/schema_traversal.go b/validator/schema_traversal.go index fe835ef9..13f1b4c6 100644 --- a/validator/schema_traversal.go +++ b/validator/schema_traversal.go @@ -198,6 +198,13 @@ func (v *Validator) validateParameterSchemas(param *parser.Parameter, path strin if param == nil { return } + // Hooked here rather than at each call site so these rules inherit the + // structural reachability this traversal exists to provide: a parameter is + // a parameter wherever it occurs. + v.validateParameterAllowReserved(param, path, result) + if param.In == parser.ParamInHeader { + v.validateHeaderName(param.Name, path, "name", result) + } if param.Schema != nil { v.validateSchema(param.Schema, path+".schema", result) } @@ -210,7 +217,9 @@ func (v *Validator) validateHeaderMapSchemas(headers map[string]*parser.Header, return } for name, header := range headers { - v.validateHeaderSchemas(header, path+".headers."+name, result) + headerPath := path + ".headers." + name + v.validateHeaderName(name, headerPath, "headers", result) + v.validateHeaderSchemas(header, headerPath, result) } } @@ -219,6 +228,7 @@ func (v *Validator) validateHeaderSchemas(header *parser.Header, path string, re if header == nil { return } + v.validateHeaderAllowReserved(header, path, result) if header.Schema != nil { v.validateSchema(header.Schema, path+".schema", result) } @@ -243,7 +253,9 @@ func (v *Validator) validateOAS3ComponentSchemas(c *parser.Components, result *V v.validateParameterSchemas(param, "components.parameters."+name, result) } for name, header := range c.Headers { - v.validateHeaderSchemas(header, "components.headers."+name, result) + headerPath := "components.headers." + name + v.validateHeaderName(name, headerPath, "headers", result) + v.validateHeaderSchemas(header, headerPath, result) } for name, mt := range c.MediaTypes { v.validateMediaTypeSchemas(mt, "components.mediaTypes."+name, result) diff --git a/validator/serialization_constraints.go b/validator/serialization_constraints.go new file mode 100644 index 00000000..47884ccd --- /dev/null +++ b/validator/serialization_constraints.go @@ -0,0 +1,137 @@ +package validator + +import ( + "fmt" + "regexp" + + "github.com/erraggy/oastools/parser" +) + +// rfc9110Token matches the `token` production of RFC 9110 ยง5.6.2, which is what +// a field name must be. The official OAS 3.2 schema states it as a `$defs/token` +// referenced from `propertyNames` on every header map and from a header +// parameter's `name`. +// +// https://www.rfc-editor.org/rfc/rfc9110.html#section-5.6.2 +var rfc9110Token = regexp.MustCompile(`^[0-9A-Za-z!#$%&'*+.^_` + "`" + `|~-]+$`) + +// headerNameRulesApply reports whether the RFC 9110 token constraint on header +// names is in force. +// +// OAS 3.2 introduced it: the `token` definition does not exist in the 3.1 schema +// at all, so enforcing it there would reject documents 3.1 considers valid. An +// unrecognized version counts as in scope, matching oas32TraversalApplies. +func headerNameRulesApply(version parser.OASVersion) bool { + return !version.IsValid() || version >= parser.OASVersion320 +} + +// validateHeaderName checks one header name against the RFC 9110 token rule. +// The name is a map key for a Header Object and the `name` field for a header +// parameter; both are field names on the wire, so both carry the constraint. +func (v *Validator) validateHeaderName(name, path, field string, result *ValidationResult) { + if !headerNameRulesApply(v.oasVersion) { + return + } + if name == "" || rfc9110Token.MatchString(name) { + return + } + v.addError(result, path, + fmt.Sprintf("Header name %q is not a valid HTTP field name; RFC 9110 allows only token characters (alphanumerics and !#$%%&'*+.^_`|~-)", name), + withSpecRef(oas32SpecRef+"#header-object"), + withField(field), + withValue(name), + ) +} + +// allowReservedPermitted reports whether `allowReserved` may appear on a +// parameter with the given `in` and `style`, for the document's version. +// +// The permitted set widened in 3.2, so this cannot be a single rule: +// +// - 3.0: the schema lists allowReserved as a plain Parameter property with no +// conditional, and the prose says it "only applies to" query parameters, +// which is a statement about effect rather than validity. Not enforced. +// - 3.1: the schema evaluates allowReserved only inside the `in: query` +// branch, and `unevaluatedProperties: false` makes it invalid anywhere else. +// - 3.2+: widened to the `in` and `style` combinations that percent-encode: +// `in: path`, `in: query`, and `in: cookie` with `style: form`. +// +// This is the shape of defect the project keeps finding: a constraint that a +// later version relaxed. Enforcing the 3.1 rule everywhere would reject valid +// 3.2 documents; enforcing the 3.2 rule everywhere would accept invalid 3.1 ones. +func allowReservedPermitted(version parser.OASVersion, in, style string) bool { + // 3.0 and earlier: structurally permitted, so nothing to enforce. + if version.IsValid() && version < parser.OASVersion310 { + return true + } + + if version.IsValid() && version < parser.OASVersion320 { + return in == parser.ParamInQuery + } + + switch in { + case parser.ParamInPath, parser.ParamInQuery: + return true + case parser.ParamInCookie: + // `form` is the default style for a cookie parameter, so an unset + // style is the permitted case rather than the forbidden one. + return style == "" || style == "form" + default: + return false + } +} + +// validateParameterAllowReserved rejects `allowReserved` where the parameter's +// `in` and `style` do not permit it. See allowReservedPermitted for the +// per-version table. +func (v *Validator) validateParameterAllowReserved(param *parser.Parameter, path string, result *ValidationResult) { + if !param.AllowReserved || allowReservedPermitted(v.oasVersion, param.In, param.Style) { + return + } + v.addError(result, path, + fmt.Sprintf("allowReserved is not permitted on a parameter with in: %q%s", param.In, styleSuffix(param.Style)), + withSpecRef(v.specRef("#parameter-object")), + withField("allowReserved"), + withValue(true), + ) +} + +// validateHeaderAllowReserved rejects `allowReserved` on a Header Object, where +// no OAS 3.x version permits it: the field appears nowhere in the Header +// Object's schema, and `unevaluatedProperties: false` closes the object. +// +// Only enforced for 3.1+, which is where the schema makes it structural; 3.0's +// draft-04 schema does not close the object the same way. +func (v *Validator) validateHeaderAllowReserved(header *parser.Header, path string, result *ValidationResult) { + // parser.Header has no AllowReserved field, because no OAS version defines + // one for a Header Object. An `allowReserved` key therefore lands in Extra, + // which the inline decoder fills with every unmatched field rather than only + // x- extensions. + // + // That is a narrow read of a broader gap: the specification closes these + // objects with `unevaluatedProperties: false`, so *any* unmatched field is + // invalid, and oastools has no general check for that. Closing it properly + // needs the field/version matrix (#439); this handles the one case the + // conformance suite exercises. + if _, present := header.Extra["allowReserved"]; !present { + return + } + if v.oasVersion.IsValid() && v.oasVersion < parser.OASVersion310 { + return + } + v.addError(result, path, + "allowReserved is not permitted on a Header Object; it applies only to parameters whose in and style percent-encode", + withSpecRef(v.specRef("#header-object")), + withField("allowReserved"), + withValue(true), + ) +} + +// styleSuffix renders the style clause of an allowReserved message, which is +// only informative when a style is actually set. +func styleSuffix(style string) string { + if style == "" { + return "" + } + return fmt.Sprintf(" and style: %q", style) +} diff --git a/validator/serialization_constraints_test.go b/validator/serialization_constraints_test.go new file mode 100644 index 00000000..a0608ad1 --- /dev/null +++ b/validator/serialization_constraints_test.go @@ -0,0 +1,765 @@ +package validator + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestHeaderNameMustBeToken covers the RFC 9110 token constraint on header +// names. OAS 3.2 introduced it (the `token` definition does not exist in the +// 3.1 schema), so a 3.1 document with the same name must still validate clean. +func TestHeaderNameMustBeToken(t *testing.T) { + const wantMsg = "is not a valid HTTP field name" + + tests := []struct { + name string + spec string + wantErr bool + }{ + { + name: "3.2 rejects a component header name with an illegal character", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +components: + headers: + 'Bad=Header': + schema: {} +`, + wantErr: true, + }, + { + name: "3.2 rejects a header parameter name with an illegal character", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +components: + parameters: + BadHeader: + name: 'Bad[Header]' + in: header + schema: {} +`, + wantErr: true, + }, + { + name: "3.2 accepts the token punctuation RFC 9110 allows", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +components: + headers: + "X-Rate-Limit_v2.1": + schema: {} +`, + }, + { + // The rule is 3.2+. Enforcing it at 3.1 would reject documents that + // version's own schema considers valid. + name: "3.1 accepts a name 3.2 would reject", + spec: ` +openapi: 3.1.0 +info: + title: API + version: 1.0.0 +components: + headers: + 'Bad=Header': + schema: {} +`, + }, + { + // A query parameter's name is not a field name, so the token rule + // does not reach it. + name: "3.2 does not constrain a query parameter name", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +components: + parameters: + Weird: + name: 'not[a]token' + in: query + schema: {} +`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := validateSpec(t, tt.spec) + assert.Equal(t, tt.wantErr, resultHasMessage(result, wantMsg), + "header-name error presence; errors: %v", result.Errors) + }) + } +} + +// TestAllowReservedPlacement covers where `allowReserved` may appear. The +// permitted set widened in 3.2, so the rule is version-scoped in both +// directions: enforcing 3.1's rule everywhere would reject valid 3.2 documents, +// and enforcing 3.2's would accept invalid 3.1 ones. +// +// Each case carries its own document rather than assembling one from fragments, +// so the `in` and `style` under test sit in the document at the indentation +// they are actually read from. +func TestAllowReservedPlacement(t *testing.T) { + const wantMsg = "allowReserved is not permitted" + + tests := []struct { + name string + spec string + wantErr bool + }{ + // 3.2 widened the permitted set to the `in` and `style` combinations + // that percent-encode: in: path, in: query, and in: cookie with + // style: form. + { + name: "3.2 permits allowReserved on a query parameter", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +components: + parameters: + p: + name: p + in: query + allowReserved: true + schema: {} +`, + }, + { + name: "3.2 permits allowReserved on a path parameter", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +components: + parameters: + p: + name: p + in: path + required: true + allowReserved: true + schema: {} +`, + }, + { + // form is the default style for a cookie parameter, so an unset + // style is the permitted case rather than the forbidden one. + name: "3.2 permits allowReserved on a cookie parameter with the default style", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +components: + parameters: + p: + name: p + in: cookie + allowReserved: true + schema: {} +`, + }, + { + name: "3.2 permits allowReserved on a cookie parameter with style form", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +components: + parameters: + p: + name: p + in: cookie + style: form + allowReserved: true + schema: {} +`, + }, + { + name: "3.2 rejects allowReserved on a cookie parameter with style cookie", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +components: + parameters: + p: + name: p + in: cookie + style: cookie + allowReserved: true + schema: {} +`, + wantErr: true, + }, + { + name: "3.2 rejects allowReserved on a header parameter", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +components: + parameters: + p: + name: p + in: header + allowReserved: true + schema: {} +`, + wantErr: true, + }, + + // 3.1 permits it on query parameters only. + { + name: "3.1 permits allowReserved on a query parameter", + spec: ` +openapi: 3.1.0 +info: + title: API + version: 1.0.0 +components: + parameters: + p: + name: p + in: query + allowReserved: true + schema: {} +`, + }, + { + name: "3.1 rejects allowReserved on a path parameter", + spec: ` +openapi: 3.1.0 +info: + title: API + version: 1.0.0 +components: + parameters: + p: + name: p + in: path + required: true + allowReserved: true + schema: {} +`, + wantErr: true, + }, + { + name: "3.1 rejects allowReserved on a cookie parameter", + spec: ` +openapi: 3.1.0 +info: + title: API + version: 1.0.0 +components: + parameters: + p: + name: p + in: cookie + allowReserved: true + schema: {} +`, + wantErr: true, + }, + { + name: "3.1 rejects allowReserved on a header parameter", + spec: ` +openapi: 3.1.0 +info: + title: API + version: 1.0.0 +components: + parameters: + p: + name: p + in: header + allowReserved: true + schema: {} +`, + wantErr: true, + }, + + { + // 3.0's schema lists allowReserved as a plain Parameter property + // with no conditional, and its prose describes effect rather than + // validity, so placement is not enforced there. + name: "3.0 does not enforce placement", + spec: ` +openapi: 3.0.3 +info: + title: API + version: 1.0.0 +paths: {} +components: + parameters: + p: + name: p + in: header + allowReserved: true + schema: {} +`, + }, + + { + name: "3.2 rejects allowReserved on a Header Object", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +components: + headers: + Style: + schema: + type: array + style: simple + explode: true + allowReserved: true +`, + wantErr: true, + }, + { + name: "3.1 also rejects allowReserved on a Header Object", + spec: ` +openapi: 3.1.0 +info: + title: API + version: 1.0.0 +components: + headers: + Style: + schema: + type: array + allowReserved: true +`, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := validateSpec(t, tt.spec) + assert.Equal(t, tt.wantErr, resultHasMessage(result, wantMsg), + "allowReserved error presence; errors: %v", result.Errors) + }) + } +} + +// TestServerVariableEnumMustNotBeEmpty covers the `minItems: 1` OAS 3.1 added to +// the Server Variable Object's enum. An empty enum permits no value at all, so +// not even the required default could satisfy it. +// +// Presence is the test, not length: an absent enum means "any value". The parser +// keeps the two apart, and the difference between `enum: []`, an absent enum +// line, and a populated block is visible in each document below rather than +// hidden in an escaped fragment. +func TestServerVariableEnumMustNotBeEmpty(t *testing.T) { + const wantMsg = "Server variable enum must not be empty" + + tests := []struct { + name string + spec string + wantErr bool + }{ + { + name: "3.2 rejects an empty enum", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +servers: + - url: https://example.com/{var} + variables: + var: + enum: [] + default: a +paths: {} +`, + wantErr: true, + }, + { + name: "3.1 rejects an empty enum", + spec: ` +openapi: 3.1.0 +info: + title: API + version: 1.0.0 +servers: + - url: https://example.com/{var} + variables: + var: + enum: [] + default: a +paths: {} +`, + wantErr: true, + }, + { + // 3.0's schema has no minItems on the enum. + name: "3.0 does not enforce it", + spec: ` +openapi: 3.0.3 +info: + title: API + version: 1.0.0 +servers: + - url: https://example.com/{var} + variables: + var: + enum: [] + default: a +paths: {} +`, + }, + { + name: "an absent enum is fine", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +servers: + - url: https://example.com/{var} + variables: + var: + default: a +paths: {} +`, + }, + { + name: "a populated enum is fine", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +servers: + - url: https://example.com/{var} + variables: + var: + enum: + - a + default: a +paths: {} +`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := validateSpec(t, tt.spec) + assert.Equal(t, tt.wantErr, resultHasMessage(result, wantMsg), + "empty-enum error presence; errors: %v", result.Errors) + }) + } +} + +// TestSpecRefTracksDocumentVersion pins the citation standard: a rule whose +// applicability varies by version must point at the version the document is +// actually being held to. Citing 3.2 at a 3.1 document describes a rule that +// document is not subject to. +func TestSpecRefTracksDocumentVersion(t *testing.T) { + tests := []struct { + name string + spec string + wantRef string + }{ + { + name: "3.1", + spec: ` +openapi: 3.1.0 +info: + title: API + version: 1.0.0 +components: + parameters: + p: + name: p + in: header + allowReserved: true + schema: {} +`, + wantRef: "https://spec.openapis.org/oas/v3.1.0.html#parameter-object", + }, + { + name: "3.2", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +components: + parameters: + p: + name: p + in: header + allowReserved: true + schema: {} +`, + wantRef: "https://spec.openapis.org/oas/v3.2.0.html#parameter-object", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := validateSpec(t, tt.spec) + + var refs []string + for _, e := range result.Errors { + if strings.Contains(e.Message, "allowReserved is not permitted") { + refs = append(refs, e.SpecRef) + } + } + require.Len(t, refs, 1, "expected exactly one allowReserved error") + assert.Equal(t, tt.wantRef, refs[0]) + }) + } +} + +// TestServerVariableEnumEmptyInJSON covers the JSON decode path. The rule turns +// on nil versus empty, which is decode behaviour, and parser keeps separate YAML +// and JSON implementations: a YAML-only test covers half the surface. +func TestServerVariableEnumEmptyInJSON(t *testing.T) { + tests := []struct { + name string + spec string + wantErr bool + }{ + { + name: "empty enum", + spec: `{ + "openapi": "3.2.0", + "info": {"title": "API", "version": "1.0.0"}, + "servers": [ + {"url": "https://example.com/{var}", "variables": {"var": {"enum": [], "default": "a"}}} + ], + "paths": {} +}`, + wantErr: true, + }, + { + name: "absent enum", + spec: `{ + "openapi": "3.2.0", + "info": {"title": "API", "version": "1.0.0"}, + "servers": [ + {"url": "https://example.com/{var}", "variables": {"var": {"default": "a"}}} + ], + "paths": {} +}`, + }, + { + name: "populated enum", + spec: `{ + "openapi": "3.2.0", + "info": {"title": "API", "version": "1.0.0"}, + "servers": [ + {"url": "https://example.com/{var}", "variables": {"var": {"enum": ["a"], "default": "a"}}} + ], + "paths": {} +}`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := validateSpec(t, tt.spec) + assert.Equal(t, tt.wantErr, resultHasMessage(result, "Server variable enum must not be empty"), + "empty-enum error presence; errors: %v", result.Errors) + }) + } +} + +// TestServerVariableEmptyEnumErrorDetail checks the error a caller actually +// receives, not just that one was raised: the path names the variable, the field +// names the offending key, and the citation points at the document's version. +func TestServerVariableEmptyEnumErrorDetail(t *testing.T) { + spec := ` +openapi: 3.1.0 +info: + title: API + version: 1.0.0 +servers: + - url: https://example.com/{var} + variables: + var: + enum: [] + default: a +paths: {} +` + result := validateSpec(t, spec) + + var found *ValidationError + for i, e := range result.Errors { + if strings.Contains(e.Message, "Server variable enum must not be empty") { + found = &result.Errors[i] + break + } + } + require.NotNil(t, found, "expected an empty-enum error; errors: %v", result.Errors) + + assert.Equal(t, "servers[0].variables.var", found.Path) + assert.Equal(t, "enum", found.Field) + assert.Equal(t, "https://spec.openapis.org/oas/v3.1.0.html#server-variable-object", found.SpecRef) +} + +// TestHeaderRulesReachEveryPosition is the reachability guard for the rules this +// file adds. They hook into the structural traversal rather than into individual +// call sites, so a Header Object is checked wherever it occurs: the point of that +// choice is only worth anything if it is asserted. +// +// This is the shape of defect #423 was: the rule was right, and simply never ran +// in most of the places the object can appear. +func TestHeaderRulesReachEveryPosition(t *testing.T) { + tests := []struct { + name string + spec string + wantMsg string + }{ + { + name: "components.headers", + wantMsg: "is not a valid HTTP field name", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +components: + headers: + 'Bad=Header': + schema: {} +`, + }, + { + name: "response headers on an inline path", + wantMsg: "is not a valid HTTP field name", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +paths: + /x: + get: + responses: + "200": + description: ok + headers: + 'Bad=Header': + schema: {} +`, + }, + { + name: "components.responses headers", + wantMsg: "is not a valid HTTP field name", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +components: + responses: + R: + description: ok + headers: + 'Bad=Header': + schema: {} +`, + }, + { + name: "encoding headers", + wantMsg: "is not a valid HTTP field name", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +paths: + /x: + post: + requestBody: + content: + multipart/form-data: + schema: + type: object + encoding: + part: + headers: + 'Bad=Header': + schema: {} + responses: + "200": + description: ok +`, + }, + { + name: "operation parameter", + wantMsg: "allowReserved is not permitted", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +paths: + /x: + get: + parameters: + - name: h + in: header + allowReserved: true + schema: {} + responses: + "200": + description: ok +`, + }, + { + name: "path-item parameter", + wantMsg: "is not a valid HTTP field name", + spec: ` +openapi: 3.2.0 +info: + title: API + version: 1.0.0 +paths: + /x: + parameters: + - name: 'Bad[Header]' + in: header + schema: {} + get: + responses: + "200": + description: ok +`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := validateSpec(t, tt.spec) + assert.True(t, resultHasMessage(result, tt.wantMsg), + "the rule did not reach this position; errors: %v", result.Errors) + }) + } +} diff --git a/validator/spec_refs.go b/validator/spec_refs.go new file mode 100644 index 00000000..2ed0283e --- /dev/null +++ b/validator/spec_refs.go @@ -0,0 +1,67 @@ +package validator + +import ( + "fmt" + + "github.com/erraggy/oastools/parser" +) + +// Specification citations follow one of two forms, and which one a rule uses is +// decided by the rule, not by convenience: +// +// - A rule whose wording or applicability varies by version cites the +// document's own version, via [Validator.specRef] or a threaded baseURL. +// Pointing a 3.1 document at the 3.2 text describes a rule it is not being +// held to. +// - A rule that exists at exactly one version cites that version directly. +// [oas32SpecRef] is the 3.2 case; see the comment on it. +// +// Most of the validator threads baseURL down from [Validator.validateOAS3], +// which is fine where the parameter already exists. specRef covers the rules +// reached through traversals that carry no baseURL, using the version +// [Validator.oasVersion] already records for exactly this purpose. + +// specBaseURL returns the specification URL for an OAS version. +// +// raw is the document's own version string, used only when the version is not +// one this build recognizes, so a citation for a future 3.x release still points +// somewhere plausible rather than nowhere. +func specBaseURL(version parser.OASVersion, raw string) string { + switch version { + case parser.OASVersion20: + return "https://spec.openapis.org/oas/v2.0.html" + case parser.OASVersion300: + return "https://spec.openapis.org/oas/v3.0.0.html" + case parser.OASVersion301: + return "https://spec.openapis.org/oas/v3.0.1.html" + case parser.OASVersion302: + return "https://spec.openapis.org/oas/v3.0.2.html" + case parser.OASVersion303: + return "https://spec.openapis.org/oas/v3.0.3.html" + case parser.OASVersion304: + return "https://spec.openapis.org/oas/v3.0.4.html" + case parser.OASVersion310: + return "https://spec.openapis.org/oas/v3.1.0.html" + case parser.OASVersion311: + return "https://spec.openapis.org/oas/v3.1.1.html" + case parser.OASVersion312: + return "https://spec.openapis.org/oas/v3.1.2.html" + case parser.OASVersion320: + return "https://spec.openapis.org/oas/v3.2.0.html" + default: + if raw == "" { + raw = version.String() + } + return fmt.Sprintf("https://spec.openapis.org/oas/v%s.html", raw) + } +} + +// specRef returns a citation for the document under validation, anchored at the +// given fragment (including its leading "#"). +// +// For rules reached through a traversal that carries no baseURL. The version +// comes from [Validator.oasVersion], which exists so version-sensitive checks +// need not be plumbed through every call. +func (v *Validator) specRef(anchor string) string { + return specBaseURL(v.oasVersion, "") + anchor +} diff --git a/validator/spec_refs_test.go b/validator/spec_refs_test.go new file mode 100644 index 00000000..52f113d0 --- /dev/null +++ b/validator/spec_refs_test.go @@ -0,0 +1,79 @@ +package validator + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/erraggy/oastools/parser" +) + +// TestSpecBaseURL pins the version-to-URL mapping. It is a switch, so the risk +// is a version silently falling through to the fallback when a new one is added +// to parser and not here: the citation would still look plausible, which is why +// every version is listed rather than spot-checked. +func TestSpecBaseURL(t *testing.T) { + tests := []struct { + name string + version parser.OASVersion + raw string + want string + }{ + {"2.0", parser.OASVersion20, "2.0", "https://spec.openapis.org/oas/v2.0.html"}, + {"3.0.0", parser.OASVersion300, "3.0.0", "https://spec.openapis.org/oas/v3.0.0.html"}, + {"3.0.1", parser.OASVersion301, "3.0.1", "https://spec.openapis.org/oas/v3.0.1.html"}, + {"3.0.2", parser.OASVersion302, "3.0.2", "https://spec.openapis.org/oas/v3.0.2.html"}, + {"3.0.3", parser.OASVersion303, "3.0.3", "https://spec.openapis.org/oas/v3.0.3.html"}, + {"3.0.4", parser.OASVersion304, "3.0.4", "https://spec.openapis.org/oas/v3.0.4.html"}, + {"3.1.0", parser.OASVersion310, "3.1.0", "https://spec.openapis.org/oas/v3.1.0.html"}, + {"3.1.1", parser.OASVersion311, "3.1.1", "https://spec.openapis.org/oas/v3.1.1.html"}, + {"3.1.2", parser.OASVersion312, "3.1.2", "https://spec.openapis.org/oas/v3.1.2.html"}, + {"3.2.0", parser.OASVersion320, "3.2.0", "https://spec.openapis.org/oas/v3.2.0.html"}, + { + // A version this build does not recognize still gets a citation, + // built from the document's own version string. + name: "unrecognized version uses the raw string", + version: parser.OASVersion(0), + raw: "3.3.0", + want: "https://spec.openapis.org/oas/v3.3.0.html", + }, + { + // With no raw string to fall back on, the version's own String() + // stands in rather than producing a malformed URL. + name: "unrecognized version with no raw string", + version: parser.OASVersion(0), + raw: "", + want: "https://spec.openapis.org/oas/vunknown.html", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, specBaseURL(tt.version, tt.raw)) + }) + } +} + +// TestValidatorSpecRef covers the accessor the traversal-reached rules use. It +// takes its version from Validator.oasVersion, which exists so version-sensitive +// checks need not be plumbed through every call. +func TestValidatorSpecRef(t *testing.T) { + tests := []struct { + name string + version parser.OASVersion + anchor string + want string + }{ + {"3.1 parameter object", parser.OASVersion310, "#parameter-object", "https://spec.openapis.org/oas/v3.1.0.html#parameter-object"}, + {"3.2 header object", parser.OASVersion320, "#header-object", "https://spec.openapis.org/oas/v3.2.0.html#header-object"}, + {"2.0 schema object", parser.OASVersion20, "#schema-object", "https://spec.openapis.org/oas/v2.0.html#schema-object"}, + {"no anchor", parser.OASVersion320, "", "https://spec.openapis.org/oas/v3.2.0.html"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v := &Validator{oasVersion: tt.version} + assert.Equal(t, tt.want, v.specRef(tt.anchor)) + }) + } +} From 3d1361db7925c411fa0887c6a76182f49c14a78f Mon Sep 17 00:00:00 2001 From: Robbie Coleman Date: Sat, 1 Aug 2026 21:22:05 -0700 Subject: [PATCH 2/2] fix(validator): treat unrecognized versions as in scope for the empty-enum rule The empty server enum check required a recognized version before it would fire, while the other three version gates in this change treat an unrecognized one as in scope. That matches oas32TraversalApplies, which states the reasoning: a document the parser could not classify is still checked. Not reachable today. ParseBytes rejects an unknown version string and ValidateParsed rejects an unset OASVersion, so nothing arrives at the validator unclassified. It is consistency rather than a live defect, and worth correcting because the four gates are written separately and OAS 3.3 is in development. Extracted as emptyServerEnumApplies beside the other gates so all four can be read together, with a test covering each. Verified the test is not vacuous by reverting the gate, which fails the empty_server_enum_rule subtest. Also removes project narrative from four comments. Code comments document the code; the significance of a change belongs elsewhere. Each kept its technical half and dropped the rest, with #423 and #439 replacing the retellings. Refs #434 --- validator/oas3.go | 8 +++- validator/serialization_constraints.go | 28 +++++++----- validator/serialization_constraints_test.go | 47 ++++++++++++++++++--- 3 files changed, 65 insertions(+), 18 deletions(-) diff --git a/validator/oas3.go b/validator/oas3.go index 9869ef29..352f6416 100644 --- a/validator/oas3.go +++ b/validator/oas3.go @@ -156,8 +156,12 @@ func (v *Validator) validateOAS3Servers(doc *parser.OAS3Document, result *Valida // Presence, not length, is the test: an absent enum means "any // value" and is fine. The parser keeps the two apart: absent // decodes to a nil slice, `enum: []` to an empty non-nil one. - if varObj.Enum != nil && len(varObj.Enum) == 0 && - v.oasVersion.IsValid() && v.oasVersion >= parser.OASVersion310 { + // + // An unrecognized version counts as in scope, matching + // oas32TraversalApplies and the other version gates: a constraint + // introduced at a threshold is assumed to hold in later versions + // this build does not yet know about. + if varObj.Enum != nil && len(varObj.Enum) == 0 && emptyServerEnumApplies(v.oasVersion) { v.addError(result, varPath, "Server variable enum must not be empty; omit it to allow any value", withSpecRef(fmt.Sprintf("%s#server-variable-object", baseURL)), diff --git a/validator/serialization_constraints.go b/validator/serialization_constraints.go index 47884ccd..076c8085 100644 --- a/validator/serialization_constraints.go +++ b/validator/serialization_constraints.go @@ -25,6 +25,16 @@ func headerNameRulesApply(version parser.OASVersion) bool { return !version.IsValid() || version >= parser.OASVersion320 } +// emptyServerEnumApplies reports whether the non-empty Server Variable enum +// constraint is in force. OAS 3.1 added `minItems: 1`; 3.0's schema has no such +// constraint. +// +// Declared beside the other version gates so all four can be compared in one +// place. An unrecognized version counts as in scope, as it does for the rest. +func emptyServerEnumApplies(version parser.OASVersion) bool { + return !version.IsValid() || version >= parser.OASVersion310 +} + // validateHeaderName checks one header name against the RFC 9110 token rule. // The name is a map key for a Header Object and the `name` field for a header // parameter; both are field names on the wire, so both carry the constraint. @@ -56,9 +66,8 @@ func (v *Validator) validateHeaderName(name, path, field string, result *Validat // - 3.2+: widened to the `in` and `style` combinations that percent-encode: // `in: path`, `in: query`, and `in: cookie` with `style: form`. // -// This is the shape of defect the project keeps finding: a constraint that a -// later version relaxed. Enforcing the 3.1 rule everywhere would reject valid -// 3.2 documents; enforcing the 3.2 rule everywhere would accept invalid 3.1 ones. +// Applying one version's rule to all of them fails in both directions: 3.1's +// would reject valid 3.2 documents, and 3.2's would accept invalid 3.1 ones. func allowReservedPermitted(version parser.OASVersion, in, style string) bool { // 3.0 and earlier: structurally permitted, so nothing to enforce. if version.IsValid() && version < parser.OASVersion310 { @@ -104,15 +113,12 @@ func (v *Validator) validateParameterAllowReserved(param *parser.Parameter, path // draft-04 schema does not close the object the same way. func (v *Validator) validateHeaderAllowReserved(header *parser.Header, path string, result *ValidationResult) { // parser.Header has no AllowReserved field, because no OAS version defines - // one for a Header Object. An `allowReserved` key therefore lands in Extra, - // which the inline decoder fills with every unmatched field rather than only - // x- extensions. + // one for a Header Object, so the key lands in Extra: the inline decoder + // fills it with every unmatched field, not only x- extensions. // - // That is a narrow read of a broader gap: the specification closes these - // objects with `unevaluatedProperties: false`, so *any* unmatched field is - // invalid, and oastools has no general check for that. Closing it properly - // needs the field/version matrix (#439); this handles the one case the - // conformance suite exercises. + // This detects the one field rather than every unmatched one. A general + // check for `unevaluatedProperties: false` needs the field/version matrix + // (#439). if _, present := header.Extra["allowReserved"]; !present { return } diff --git a/validator/serialization_constraints_test.go b/validator/serialization_constraints_test.go index a0608ad1..627e3d16 100644 --- a/validator/serialization_constraints_test.go +++ b/validator/serialization_constraints_test.go @@ -6,6 +6,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/erraggy/oastools/parser" ) // TestHeaderNameMustBeToken covers the RFC 9110 token constraint on header @@ -625,11 +627,8 @@ paths: {} // TestHeaderRulesReachEveryPosition is the reachability guard for the rules this // file adds. They hook into the structural traversal rather than into individual -// call sites, so a Header Object is checked wherever it occurs: the point of that -// choice is only worth anything if it is asserted. -// -// This is the shape of defect #423 was: the rule was right, and simply never ran -// in most of the places the object can appear. +// call sites, so a Header Object is checked wherever it occurs. See #423 for the +// case where a rule was correct but never ran in most of those positions. func TestHeaderRulesReachEveryPosition(t *testing.T) { tests := []struct { name string @@ -763,3 +762,41 @@ paths: }) } } + +// TestVersionGatesTreatUnrecognizedVersionsAsInScope pins the convention shared +// by every version gate here and by oas32TraversalApplies: a constraint +// introduced at a threshold is assumed to hold in later versions this build does +// not yet recognize. +// +// The alternative, skipping the rule, means a document oastools cannot classify +// is held to fewer rules than one it can. The gates are written separately, so +// this pins them to one answer. +func TestVersionGatesTreatUnrecognizedVersionsAsInScope(t *testing.T) { + unrecognized := parser.OASVersion(0) + require.False(t, unrecognized.IsValid(), "the test needs a version this build does not know") + + t.Run("header name rule", func(t *testing.T) { + assert.True(t, headerNameRulesApply(unrecognized)) + }) + + t.Run("empty server enum rule", func(t *testing.T) { + assert.True(t, emptyServerEnumApplies(unrecognized)) + }) + + t.Run("allowReserved on a parameter uses the newest table", func(t *testing.T) { + // The permitted set widened in 3.2, so "in scope" means the 3.2 table + // rather than 3.1's narrower one. + assert.True(t, allowReservedPermitted(unrecognized, parser.ParamInPath, ""), + "3.2 permits allowReserved on a path parameter") + assert.False(t, allowReservedPermitted(unrecognized, parser.ParamInHeader, ""), + "no version permits allowReserved on a header parameter") + }) + + t.Run("allowReserved on a Header Object", func(t *testing.T) { + v := &Validator{oasVersion: unrecognized} + result := &ValidationResult{} + header := &parser.Header{Extra: map[string]any{"allowReserved": true}} + v.validateHeaderAllowReserved(header, "components.headers.X", result) + assert.True(t, resultHasMessage(result, "allowReserved is not permitted")) + }) +}