From d72939f2018e3bbcf2ea8c31c725842e95815249 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Fri, 7 Aug 2026 02:12:22 +0300 Subject: [PATCH] fix(compilers/openapi): refuse a scheme that names no mechanism MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A securitySchemes entry with no `type` fell into fillSchemeKind's default branch, where the unrecognised-type degradation interned it as a custom scheme whose mechanism was the empty string. A requirement naming it got a live AuthID, so the IR stated that the API was authenticated by a scheme that named nothing. `type: http` with no `scheme` token reached the same state through fillHTTPScheme's default branch. Both are now refused: nothing is interned, the entry is reported at its own components pointer under a new openapi/incomplete-security-scheme code, and a requirement naming it drops whole as one naming an undeclared scheme already does. AuthKind has no value meaning "the document did not say" — AuthKindCustom names a mechanism the IR does not model, not one the entry never gave — and inventing one would put a scheme no emitter can implement in Document.Auth, recognisable only by an empty Scheme field. The same branches also dropped whatever else the entry declared. Each mechanism's lowering reads only the fields its own type defines — `in` and `name` under apiKey, `flows` under oauth2 — so a document writing them elsewhere lost them with no field, no Unmodeled entry and no diagnostic. Every field a type does not define is now kept verbatim under Unmodeled with ReasonDegradedLowering, located at the field itself and announced. Filling the IR field of the same name instead would say the mechanism has a property it does not define. The list of per-type fields is held to the upstream model by reflection, so a field that model gains fails a test rather than silently vanishing. --- compilers/openapi/conformance_test.go | 8 + compilers/openapi/internal/auth/auth.go | 185 +++++++++++-- .../internal/auth/auth_internal_test.go | 52 ++++ compilers/openapi/internal/auth/auth_test.go | 261 ++++++++++++++++++ compilers/openapi/internal/diag/diag.go | 12 + compilers/openapi/internal/diag/diag_test.go | 3 +- .../openapi/security-schemes.golden.json | 47 +++- .../conformance/openapi/security-schemes.yaml | 9 + 8 files changed, 552 insertions(+), 25 deletions(-) diff --git a/compilers/openapi/conformance_test.go b/compilers/openapi/conformance_test.go index ac29af5..17ae7d9 100644 --- a/compilers/openapi/conformance_test.go +++ b/compilers/openapi/conformance_test.go @@ -2153,6 +2153,14 @@ func assertSchemeDetail(t *testing.T, doc *ir.Document) { assert.Equal(t, ir.AuthKindCustom, digest.Kind, "only basic and bearer get first-class kinds") assert.Equal(t, "digest", digest.Scheme, "the token itself is kept rather than dropped") + stray, ok := byName["strayFieldAuth"] + require.True(t, ok) + assert.Empty(t, stray.BearerFormat, "an apiKey has no bearer-token format to fill") + kept, ok := stray.Unmodeled["openapi:bearerFormat"] + require.True(t, ok, "a field the type does not define is kept, not dropped; got %v", stray.Unmodeled) + assert.Equal(t, ir.ReasonDegradedLowering, kept.Reason) + assert.Equal(t, ir.RawValue(`"JWT"`), kept.Value, "kept as the document wrote it") + oauth, ok := byName["oauth2Auth"] require.True(t, ok) assert.Equal(t, "https://example.com/.well-known/oauth-authorization-server", diff --git a/compilers/openapi/internal/auth/auth.go b/compilers/openapi/internal/auth/auth.go index 0a55d7b..5112aba 100644 --- a/compilers/openapi/internal/auth/auth.go +++ b/compilers/openapi/internal/auth/auth.go @@ -10,6 +10,7 @@ package auth import ( "maps" + "slices" "strconv" "strings" @@ -35,7 +36,10 @@ import ( // unreferenced entry would otherwise be a scheme the document declares, the IR // silently drops, and no diagnostic sites. // -// That is the only shape reported here — see unresolvableSchemeDiags. +// An entry that resolves to an object but names no mechanism is refused for the +// same reason and reported the same way — see mechanismRefusalDiag. Those two +// are the only entries reported as interning nothing; every other diagnostic +// from here is about a scheme that did intern (see preserveUnreadFields). func LowerSecuritySchemes(c lowering.Ctx) (map[ir.AuthID]ir.AuthScheme, []ir.Diagnostic) { comps := c.Doc.Components if comps == nil { @@ -53,9 +57,12 @@ func LowerSecuritySchemes(c lowering.Ctx) (map[ir.AuthID]ir.AuthScheme, []ir.Dia diags = append(diags, unresolvableSchemeDiags(c, name, rs)...) continue } - scheme, schemeDiags := lowerSecurityScheme(c, name, ss) - out[ids.Auth(name)] = scheme + scheme, ok, schemeDiags := lowerSecurityScheme(c, name, ss) diags = append(diags, schemeDiags...) + if !ok { + continue + } + out[ids.Auth(name)] = scheme } if len(out) == 0 { return nil, diags @@ -91,35 +98,77 @@ func unresolvableSchemeDiags(c lowering.Ctx, name string, rs *soa.ReferencedSecu } // lowerSecurityScheme lowers one named security scheme into its AuthScheme, -// dispatching the mechanism-specific fields by type. -func lowerSecurityScheme(c lowering.Ctx, name string, ss *soa.SecurityScheme) (ir.AuthScheme, []ir.Diagnostic) { - scheme := ir.AuthScheme{ +// dispatching the mechanism-specific fields by type. ok reports whether the +// entry named a mechanism at all; when it did not, the caller interns nothing. +func lowerSecurityScheme(c lowering.Ctx, name string, ss *soa.SecurityScheme, +) (scheme ir.AuthScheme, ok bool, diags []ir.Diagnostic) { + pointer := ids.Ptr("components", "securitySchemes", name) + scheme = ir.AuthScheme{ ID: ids.Auth(name), Name: compile.NamingFor(name), Docs: ir.Docs{Description: ss.GetDescription()}, - Provenance: c.ProvenanceAt(ids.Ptr("components", "securitySchemes", name)), + Provenance: c.ProvenanceAt(pointer), } if ss.GetDeprecated() { scheme.Deprecation = &ir.Deprecation{} } - fillSchemeKind(&scheme, ss) - ext, diags := annotation.ExtensionsFrom(ss.GetExtensions(), c.SrcIndex, scheme.Provenance.Pointer) - if len(ext) > 0 { - scheme.Unmodeled = ext + missing, named := fillSchemeKind(&scheme, ss) + if !named { + return ir.AuthScheme{}, false, []ir.Diagnostic{mechanismRefusalDiag(c, name, missing, pointer)} } - return scheme, diags + diags = preserveUnreadFields(c, &scheme, ss, pointer) + ext, extDiags := annotation.ExtensionsFrom(ss.GetExtensions(), c.SrcIndex, pointer) + scheme.Unmodeled = annotation.MergeUnmodeled(scheme.Unmodeled, ext) + return scheme, true, append(diags, extDiags...) +} + +// mechanismRefusalDiag reports a securitySchemes entry that declares a scheme +// without saying what it is, having omitted the field named by missing. +// +// It is refused rather than interned because ir.AuthKind has no value for "the +// document did not say": every kind names a mechanism, and AuthKindCustom names +// one the IR does not model rather than one the entry never gave. Interning it +// would put a scheme no emitter can implement in Document.Auth, recognisable +// only by an empty Scheme — an in-band error for every consumer to rediscover — +// and assert that the API is authenticated by nothing in particular (#294). +// +// Refusing reuses the remedy #41 gave a requirement naming an undeclared +// scheme, and does so deliberately rather than by inheritance: the two faults +// differ — that entry was never declared, this one was declared and left +// unsaid — but the IR has no more room for the second than for the first, and +// the cost is one already documented and diagnosed at every step. A requirement +// naming this entry drops whole, and a list whose every option drops collapses +// to nil (see LowerSecurityRequirements). +// +// What is not kept, deliberately: the fields the entry did declare. An entry +// that names no mechanism is a defect in the document rather than a construct +// the IR declines to model, which is the call an unresolvable $ref already gets +// here — and there is no scheme left to hang an Unmodeled map on. The entry +// that *does* intern keeps everything it wrote; see preserveUnreadFields. +func mechanismRefusalDiag(c lowering.Ctx, name, missing, pointer string) ir.Diagnostic { + return c.DiagAt(ir.SeverityError, diag.IncompleteSecurityScheme, pointer, + "security scheme %q declares no %s, so it names no authentication mechanism: "+ + "no scheme is interned for it, and every requirement naming it is dropped", name, missing) } // fillSchemeKind sets the mechanism kind and its per-kind fields (ir-design §9). -// Unknown or unmodeled types degrade to a custom scheme carrying the raw type. -func fillSchemeKind(scheme *ir.AuthScheme, ss *soa.SecurityScheme) { +// An unrecognized type degrades to a custom scheme carrying the raw type, which +// a later OpenAPI version's own type reaches as readily as a typo does. +// +// ok reports whether the entry named a mechanism; missing is the field it had +// to declare to name one and did not. An absent type names nothing at all, and +// no degradation is available for it: the custom kind carries the token a type +// was spelled with, and there is no token. +func fillSchemeKind(scheme *ir.AuthScheme, ss *soa.SecurityScheme) (missing string, ok bool) { switch ss.GetType() { + case "": + return "type", false case soa.SecuritySchemeTypeAPIKey: scheme.Kind = ir.AuthKindAPIKey scheme.In = string(ss.GetIn()) scheme.KeyName = ss.GetName() case soa.SecuritySchemeTypeHTTP: - fillHTTPScheme(scheme, ss) + return fillHTTPScheme(scheme, ss) case soa.SecuritySchemeTypeOAuth2: scheme.Kind = ir.AuthKindOAuth2 scheme.Flows = oauthFlows(ss.GetFlows()) @@ -133,22 +182,111 @@ func fillSchemeKind(scheme *ir.AuthScheme, ss *soa.SecurityScheme) { scheme.Kind = ir.AuthKindCustom scheme.Scheme = string(ss.GetType()) } + return "", true } // fillHTTPScheme classifies an HTTP scheme by its RFC 7235 scheme token: basic // and bearer get first-class kinds; any other scheme is custom with the token // preserved. BearerFormat rides along regardless (ir-design §9). -func fillHTTPScheme(scheme *ir.AuthScheme, ss *soa.SecurityScheme) { +// +// `type: http` alone is the second shape that names no mechanism: the token is +// what an HTTP scheme *is*, and a custom kind carrying an empty one says no +// more than the typeless entry does. A token the RFC would reject is still a +// token and still interns — comparing it against the grammar is validation this +// compiler does not do, and trimming it would be a guess at what was meant. +func fillHTTPScheme(scheme *ir.AuthScheme, ss *soa.SecurityScheme) (missing string, ok bool) { + token := ss.GetScheme() + if token == "" { + return "scheme", false + } scheme.BearerFormat = ss.GetBearerFormat() - switch strings.ToLower(ss.GetScheme()) { + switch strings.ToLower(token) { case "basic": scheme.Kind = ir.AuthKindHTTPBasic case "bearer": scheme.Kind = ir.AuthKindHTTPBearer default: scheme.Kind = ir.AuthKindCustom - scheme.Scheme = ss.GetScheme() + scheme.Scheme = token + } + return "", true +} + +// mechanismFieldNames are the securityScheme fields only some types define, +// sorted — which is both the order preserveUnreadFields walks them in and what +// makes the list comparable to the source model it must keep pace with. +// +// type, description, deprecated and the x-* extensions are absent because every +// type defines them, so no type can leave one unread. Nothing here derives that +// split; TestMechanismFieldNames_AccountForEverySourceField holds it to the +// upstream struct, so a field that model gains fails a test rather than +// vanishing from the IR. +func mechanismFieldNames() []string { + return []string{ + "bearerFormat", "flows", "in", "name", + "oauth2MetadataUrl", "openIdConnectUrl", "scheme", + } +} + +// fieldsDefinedBy returns the mechanism fields OpenAPI gives t a meaning for, +// which are exactly the ones fillSchemeKind reads for it. +// +// mutualTLS is the whole mechanism and defines none of them. Nor does an +// unrecognized type: the custom kind it degrades to already spends its one +// Scheme field on the type token, so a `scheme` written beside it has no home +// left even though the same field would have held it under `type: http`. +func fieldsDefinedBy(t soa.SecuritySchemaType) []string { + switch t { + case soa.SecuritySchemeTypeAPIKey: + return []string{"in", "name"} + case soa.SecuritySchemeTypeHTTP: + return []string{"scheme", "bearerFormat"} + case soa.SecuritySchemeTypeOAuth2: + return []string{"flows", "oauth2MetadataUrl"} + case soa.SecuritySchemeTypeOpenIDConnect: + return []string{"openIdConnectUrl"} + default: + return nil + } +} + +// preserveUnreadFields keeps every mechanism field the entry declared that +// its own type gives no meaning to — `in` on an oauth2 scheme, `flows` on an +// apiKey — verbatim under Unmodeled rather than dropping it (#294). +// +// Each mechanism's lowering reads only its own fields, so before this the rest +// reached no IR field and no Unmodeled entry either: declared source text gone +// with no diagnostic, which invariant 2 forbids. ir.AuthScheme is flat and does +// hold a field of each name, but filling one would say the mechanism has a +// property it does not define — an apiKey location on a scheme that is not an +// apiKey — so the declaration is kept beside the scheme instead of inside it. +// ReasonDegradedLowering for that reason: the entry is lowered to the weaker +// shape its type can hold, with what did not fit recoverable beside it. +// +// Presence, not truth: a field the entry did not write records nothing, and one +// it wrote records whatever it wrote. RawChildNode reads the entry as the +// document spelled it, so an explicit `in: ""` is a declaration like any other. +func preserveUnreadFields(c lowering.Ctx, scheme *ir.AuthScheme, ss *soa.SecurityScheme, + pointer string, +) []ir.Diagnostic { + defined := fieldsDefinedBy(ss.GetType()) + var diags []ir.Diagnostic + for _, field := range mechanismFieldNames() { + if slices.Contains(defined, field) { + continue + } + at := pointer + ids.Ptr(field) + kept, keptDiags := annotation.PreserveNodeInto(&scheme.Unmodeled, "openapi:"+field, + annotation.RawChildNode(ss.GetRootNode(), field), ir.ReasonDegradedLowering, at, c.SrcIndex) + diags = append(diags, keptDiags...) + if !kept { + continue + } + diags = append(diags, c.DiagAt(ir.SeverityInfo, diag.DegradedConstruct, at, + "security scheme %s is not defined by type %q; kept verbatim under Unmodeled", + field, ss.GetType())) } + return diags } // oauthFlows lowers each present OAuth2 flow in a fixed, deterministic order. @@ -276,11 +414,12 @@ func LowerSecurityRequirements(c lowering.Ctx, reqs []*soa.SecurityRequirement, // requirement-level pointer, so a multi-member option reports each of its bad // names. // -// Two documents reach that state: one that never declared the name, and one -// that declared it as a $ref resolving to nothing. What is said here is only -// that the name resolves to no scheme, which is true of both — calling it -// undeclared would contradict the entry-level report LowerSecuritySchemes -// leaves beside it in the second case. +// Three documents reach that state: one that never declared the name, one that +// declared it as a $ref resolving to nothing, and one whose entry named no +// mechanism (#294). What is said here is only that the name resolves to no +// scheme, which is true of all three — calling it undeclared would contradict +// the entry-level report LowerSecuritySchemes leaves beside it in the latter +// two cases. func lowerSecurityRequirement(c lowering.Ctx, req *soa.SecurityRequirement, pointer string, ) (r ir.AuthRequirement, ok bool, diags []ir.Diagnostic) { if req == nil { diff --git a/compilers/openapi/internal/auth/auth_internal_test.go b/compilers/openapi/internal/auth/auth_internal_test.go index 523ef1c..4df3886 100644 --- a/compilers/openapi/internal/auth/auth_internal_test.go +++ b/compilers/openapi/internal/auth/auth_internal_test.go @@ -1,9 +1,13 @@ package auth import ( + "reflect" + "slices" "testing" + soa "github.com/speakeasy-api/openapi/openapi" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/dexpace/morphic/compilers/openapi/internal/lowering" ) @@ -16,3 +20,51 @@ func TestLowerSecurityRequirement_Nil(t *testing.T) { assert.True(t, ok, "a nil requirement entry is not a resolution failure") assert.Empty(t, diags) } + +// TestMechanismFieldNames_AccountForEverySourceField holds mechanismFieldNames +// to the source model it is a list of, so a field the upstream model gains is a +// failure here rather than a construct the IR drops without trace. +// +// That is the shape of #294 itself: the lowering read the fields it knew about +// and dropped whatever else the entry declared, and no test compared the two +// lists. Reading the source struct is what makes the comparison possible at all +// — a hand-written list checked against another hand-written list would agree +// with itself forever. +// +// The wire names are spelled out rather than derived, because the two differ by +// more than a leading case change: OAuth2MetadataUrl is oauth2MetadataUrl. +func TestMechanismFieldNames_AccountForEverySourceField(t *testing.T) { + t.Parallel() + // alwaysRead are the fields lowerSecurityScheme reads whatever the type is, + // so no type can leave one unread and none needs preserving. + alwaysRead := map[string]bool{ + "Type": true, "Description": true, "Deprecated": true, "Extensions": true, + } + // perType are the fields only some types define, keyed by Go field name and + // valued by the wire name mechanismFieldNames must list. + perType := map[string]string{ + "Name": "name", "In": "in", "Scheme": "scheme", "BearerFormat": "bearerFormat", + "Flows": "flows", "OpenIdConnectUrl": "openIdConnectUrl", + "OAuth2MetadataUrl": "oauth2MetadataUrl", + } + + var want []string + st := reflect.TypeOf(soa.SecurityScheme{}) + for i := range st.NumField() { + f := st.Field(i) + if f.Anonymous || !f.IsExported() { + continue // the embedded marshaller model is not a document field + } + wire, perTypeField := perType[f.Name] + require.True(t, alwaysRead[f.Name] || perTypeField, + "securityScheme field %q is neither read for every type nor listed as a mechanism "+ + "field, so a document declaring it loses it: add it to one or the other", f.Name) + if perTypeField { + want = append(want, wire) + } + } + + slices.Sort(want) + assert.Equal(t, want, mechanismFieldNames(), + "mechanismFieldNames must list every per-type field, sorted") +} diff --git a/compilers/openapi/internal/auth/auth_test.go b/compilers/openapi/internal/auth/auth_test.go index 98cceed..95e419b 100644 --- a/compilers/openapi/internal/auth/auth_test.go +++ b/compilers/openapi/internal/auth/auth_test.go @@ -2,6 +2,7 @@ package auth_test import ( "slices" + "strings" "testing" soa "github.com/speakeasy-api/openapi/openapi" @@ -366,6 +367,185 @@ func TestLowerSecuritySchemes_NoComponentsAtAll(t *testing.T) { assert.Empty(t, diags) } +// TestLowerSecuritySchemes_AnEntryNamingNoMechanismIsRefused pins the two +// shapes that declare a scheme without saying what it is: an entry with no +// `type`, and an http entry with no `scheme` token. Both used to intern a +// custom scheme whose mechanism was the empty string, which states that the API +// is authenticated by nothing in particular (GitHub #294). +// +// Each is reported at the entry's own components pointer and interned nowhere, +// exactly as an entry whose $ref resolves to nothing already is. +func TestLowerSecuritySchemes_AnEntryNamingNoMechanismIsRefused(t *testing.T) { + t.Parallel() + cases := []struct { + name string + entry string + }{ + {name: "no type at all", entry: `{}`}, + {name: "no type, but fields an apiKey would use", entry: `{in: header, name: X-Key}`}, + {name: "an explicitly empty type", entry: `{type: ""}`}, + {name: "http with no scheme token", entry: `{type: http}`}, + {name: "http with an empty scheme token", entry: `{type: http, scheme: ""}`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + doc, _, diags := serviceSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: {} +components: + securitySchemes: + ghost: `+tc.entry+` + key: {type: apiKey, in: header, name: X-Key} +`) + assert.NotContains(t, doc.Auth, ids.Auth("ghost"), "the entry interns no scheme") + assert.Contains(t, doc.Auth, ids.Auth("key"), "its complete sibling still does") + + d, ok := firstDiagAt(diags, diag.IncompleteSecurityScheme) + require.True(t, ok, "the refusal is reported: %+v", diags) + assert.Equal(t, ir.SeverityError, d.Severity) + assert.Equal(t, "/components/securitySchemes/ghost", d.Provenance.Pointer, + "reported at the entry, not at the requirement that names it") + assert.Contains(t, d.Message, `"ghost"`, "the report names the entry") + }) + } +} + +// TestLowerSecuritySchemes_ARefusedEntryDropsTheRequirementNamingIt pins the +// downstream half of the refusal. Nothing is interned, so a requirement naming +// the entry resolves to no scheme and drops whole under the rule #41 +// established, collapsing a sole-option list to nil rather than leaving the +// empty-option encoding that reads as "no auth is also fine". +func TestLowerSecuritySchemes_ARefusedEntryDropsTheRequirementNamingIt(t *testing.T) { + t.Parallel() + doc, svc, diags := serviceSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +security: + - ghost: [] +paths: {} +components: + securitySchemes: + ghost: {in: header, name: X-Key} +`) + assert.Empty(t, doc.Auth, "a document whose only scheme is refused carries no auth registry") + assert.Nil(t, svc.Auth, "the sole option drops, collapsing the list rather than emptying it") + assert.NotEmpty(t, messagesAt(diags, diag.UnresolvedRef), + "the requirement that named it is reported too: %+v", diags) +} + +// TestLowerSecuritySchemes_FieldsTheTypeDoesNotDefineSurvive pins that no field +// a securityScheme entry declares is dropped. Each mechanism's lowering reads +// only the fields its own type defines, so everything else the entry wrote — +// `in` on an oauth2 scheme, `flows` on an apiKey — reached no IR field and no +// Unmodeled entry either, which "lossless by default" forbids (GitHub #294). +// +// Every case declares all seven mechanism fields, so each row states both +// halves of §12.1's one-home rule at once: what the type defines reaches its IR +// field and is not also kept raw, and what it does not define is kept raw and +// does not silently fill a field of another mechanism. +func TestLowerSecuritySchemes_FieldsTheTypeDoesNotDefineSurvive(t *testing.T) { + t.Parallel() + // everyField is written on each entry below; a row names the ones its type + // defines and the rest must survive under Unmodeled. + everyField := []string{"bearerFormat", "flows", "in", "name", "oauth2MetadataUrl", "openIdConnectUrl", "scheme"} + cases := []struct { + name string + typ string + defines []string + check func(t *testing.T, s ir.AuthScheme) + }{ + { + name: "apiKey", typ: "apiKey", defines: []string{"in", "name"}, + check: func(t *testing.T, s ir.AuthScheme) { + assert.Equal(t, ir.AuthKindAPIKey, s.Kind) + assert.Equal(t, "header", s.In) + assert.Equal(t, "X-Key", s.KeyName) + }, + }, + { + name: "http", typ: "http", defines: []string{"scheme", "bearerFormat"}, + check: func(t *testing.T, s ir.AuthScheme) { + assert.Equal(t, ir.AuthKindCustom, s.Kind) + assert.Equal(t, "digest", s.Scheme, "the RFC 7235 token its own type defines") + assert.Equal(t, "JWT", s.BearerFormat) + }, + }, + { + name: "oauth2", typ: "oauth2", defines: []string{"flows", "oauth2MetadataUrl"}, + check: func(t *testing.T, s ir.AuthScheme) { + assert.Equal(t, ir.AuthKindOAuth2, s.Kind) + require.Len(t, s.Flows, 1) + assert.Equal(t, "https://meta", s.OAuth2MetadataURL) + }, + }, + { + name: "openIdConnect", typ: "openIdConnect", defines: []string{"openIdConnectUrl"}, + check: func(t *testing.T, s ir.AuthScheme) { + assert.Equal(t, ir.AuthKindOpenIDConnect, s.Kind) + assert.Equal(t, "https://oidc", s.OpenIDConnectURL) + }, + }, + { + // mutualTLS is the whole mechanism; nothing else on the entry is part + // of it, so all seven fields are kept raw. + name: "mutualTLS", typ: "mutualTLS", defines: nil, + check: func(t *testing.T, s ir.AuthScheme) { + assert.Equal(t, ir.AuthKindMutualTLS, s.Kind) + }, + }, + { + // An unrecognised type degrades to a custom scheme carrying the token + // itself, which takes the one IR field a declared `scheme` would have + // reached — so that declaration is kept raw beside the six others. + name: "an unrecognised type", typ: "bananas", defines: nil, + check: func(t *testing.T, s ir.AuthScheme) { + assert.Equal(t, ir.AuthKindCustom, s.Kind) + assert.Equal(t, "bananas", s.Scheme, "the unrecognised token, not the declared scheme") + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + doc, _, _ := serviceSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: {} +components: + securitySchemes: + s: + type: `+tc.typ+` + in: header + name: X-Key + scheme: digest + bearerFormat: JWT + openIdConnectUrl: https://oidc + oauth2MetadataUrl: https://meta + flows: + implicit: {authorizationUrl: 'https://a', scopes: {}} +`) + s, ok := doc.Auth[ids.Auth("s")] + require.True(t, ok) + tc.check(t, s) + + var wantKept []string + for _, f := range everyField { + if !slices.Contains(tc.defines, f) { + wantKept = append(wantKept, "openapi:"+f) + } + } + assert.Equal(t, wantKept, unmodeledKeys(s.Unmodeled), + "every field the type does not define is kept, and no field it does define is kept twice") + for _, key := range wantKept { + field := strings.TrimPrefix(key, "openapi:") + assert.Equal(t, ir.ReasonDegradedLowering, s.Unmodeled[key].Reason, + "%s was lowered to a weaker shape, not left unmodelled for want of a field", key) + assert.Equal(t, "/components/securitySchemes/s/"+field, s.Unmodeled[key].Provenance.Pointer, + "the entry locates the field itself, not the scheme that carried it") + } + }) + } +} + // serviceSpec compiles src and returns the document, its single service, and // every diagnostic. It drives the public entry point rather than this package // directly: the requirements lowered here are reached through the service walk, @@ -470,6 +650,87 @@ func indexBy[T any, K comparable](items []T, key func(T) K) map[K]T { return out } +// TestLowerSecuritySchemes_KeptFieldsAndExtensionsShareOneMap pins that the two +// writers of a scheme's Unmodeled map do not overwrite each other. The x-* +// extensions used to be the only one and were assigned over the whole map, so a +// preserved field written before them would vanish without trace — the same +// silent drop this all exists to close, one layer up. +func TestLowerSecuritySchemes_KeptFieldsAndExtensionsShareOneMap(t *testing.T) { + t.Parallel() + doc, _, _ := serviceSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: {} +components: + securitySchemes: + s: {type: mutualTLS, bearerFormat: JWT, x-note: n} +`) + s, ok := doc.Auth[ids.Auth("s")] + require.True(t, ok) + assert.Equal(t, []string{"openapi:bearerFormat", "openapi:x-note"}, unmodeledKeys(s.Unmodeled)) + assert.Equal(t, ir.ReasonVendorExtension, s.Unmodeled["openapi:x-note"].Reason) + assert.Equal(t, ir.ReasonDegradedLowering, s.Unmodeled["openapi:bearerFormat"].Reason) +} + +// TestLowerSecuritySchemes_AKeptFieldIsAnnounced pins that a field kept under +// Unmodeled is also reported, so a reader learns to look for it rather than +// discovering the entry by reading the IR. The report names the field and sits +// at the field's own pointer, not at the scheme that carried it. +func TestLowerSecuritySchemes_AKeptFieldIsAnnounced(t *testing.T) { + t.Parallel() + _, _, diags := serviceSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: {} +components: + securitySchemes: + s: {type: mutualTLS, in: header} +`) + got := messagesAtPointer(diags, "/components/securitySchemes/s/in") + require.Len(t, got, 1, "the kept field is announced exactly once: %+v", diags) + assert.Contains(t, got[0], "in") + assert.Contains(t, got[0], "mutualTLS", "and names the type that gave it no meaning") + d, ok := firstDiagAt(diags, diag.DegradedConstruct) + require.True(t, ok) + assert.Equal(t, ir.SeverityInfo, d.Severity, "the field survives, so this records rather than warns") +} + +// TestLowerSecuritySchemes_AnUnkeepableFieldIsReportedNotDropped pins the third +// outcome of keeping a field: one whose value JSON cannot hold reaches the IR in +// no form at all, so it is reported as the losslessness failure it is rather +// than announced as kept (GitHub #144). `.nan` is such a value, and a scheme +// declaring it is enough to reach this — the loader parses the entry, warns that +// the field is not used for the type, and hands it here intact. +func TestLowerSecuritySchemes_AnUnkeepableFieldIsReportedNotDropped(t *testing.T) { + t.Parallel() + doc, _, diags := serviceSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: {} +components: + securitySchemes: + s: {type: mutualTLS, name: .nan} +`) + s, ok := doc.Auth[ids.Auth("s")] + require.True(t, ok, "the scheme still lowers; only the one field could not be kept") + assert.Empty(t, s.Unmodeled, "nothing was kept, so nothing claims to have been") + + d, ok := firstDiagAt(diags, diag.UnpreservableConstruct) + require.True(t, ok, "the loss is reported: %+v", diags) + assert.Equal(t, ir.SeverityError, d.Severity) + assert.Equal(t, "/components/securitySchemes/s/name", d.Provenance.Pointer) + assert.Empty(t, messagesAtPointer(diags, "/components/securitySchemes/s/name")[1:], + "and is not also announced as kept") +} + +// unmodeledKeys returns u's keys sorted, so a caller pins the whole set rather +// than probing for the ones it happened to think of. +func unmodeledKeys(u ir.Unmodeled) []string { + out := make([]string, 0, len(u)) + for k := range u { + out = append(out, k) + } + slices.Sort(out) + return out +} + // pathsSpec wraps a paths block in a minimal 3.1 document with no components. func pathsSpec(paths string) string { return "openapi: 3.1.0\n" + diff --git a/compilers/openapi/internal/diag/diag.go b/compilers/openapi/internal/diag/diag.go index f159c9e..5902708 100644 --- a/compilers/openapi/internal/diag/diag.go +++ b/compilers/openapi/internal/diag/diag.go @@ -143,6 +143,18 @@ const ( // operation, which OpenAPI forbids. A path item mounted at two paths is the // shape that reaches this without the document repeating the id in source. DuplicateOperationID = "openapi/duplicate-operation-id" + // IncompleteSecurityScheme reports a securitySchemes entry that omits the + // field naming which authentication mechanism it is — `type`, or the RFC 7235 + // `scheme` token that is the mechanism when the type is http. The entry + // declares a scheme without saying what it does, so nothing is interned for + // it and every requirement naming it is dropped (GitHub #294). + // + // Error rather than a degradation, for the reason UnresolvedRef is one at the + // neighbouring shape: the entry reached the IR in no form at all, so a reader + // told only that it was degraded would go looking for a scheme that is not + // there. The document is invalid either way — OpenAPI requires both fields — + // so this hides no later finding the loader's own refusal would not have. + IncompleteSecurityScheme = "openapi/incomplete-security-scheme" // ReservedHeaderName reports a header declaration OpenAPI says SHALL be // ignored, because the name restates something the protocol layer already // owns. The specification states the rule at three positions, and this code diff --git a/compilers/openapi/internal/diag/diag_test.go b/compilers/openapi/internal/diag/diag_test.go index 2a3754f..26e351a 100644 --- a/compilers/openapi/internal/diag/diag_test.go +++ b/compilers/openapi/internal/diag/diag_test.go @@ -136,7 +136,8 @@ func codes() []string { diag.CompositionLowering, diag.DynamicRefExpanded, diag.ConflictingRedecl, diag.DisjointVisibility, diag.AliasAmplification, diag.UnattachableRequired, diag.InternalInvariant, - diag.DuplicateOperationID, diag.ReservedHeaderName, diag.UnpreservableConstruct, + diag.DuplicateOperationID, diag.IncompleteSecurityScheme, + diag.ReservedHeaderName, diag.UnpreservableConstruct, } } diff --git a/testdata/conformance/openapi/security-schemes.golden.json b/testdata/conformance/openapi/security-schemes.golden.json index e984e3d..54f7fec 100644 --- a/testdata/conformance/openapi/security-schemes.golden.json +++ b/testdata/conformance/openapi/security-schemes.golden.json @@ -137,6 +137,31 @@ "source": 0, "pointer": "/components/securitySchemes/oidcAuth" } + }, + "auth/openapi/components/securitySchemes/strayFieldAuth": { + "id": "auth/openapi/components/securitySchemes/strayFieldAuth", + "name": { + "source": "strayFieldAuth", + "canonical": "stray_field_auth" + }, + "kind": "apiKey", + "docs": {}, + "in": "header", + "keyName": "X-Stray", + "unmodeled": { + "openapi:bearerFormat": { + "reason": "degraded_lowering", + "value": "JWT", + "provenance": { + "source": 0, + "pointer": "/components/securitySchemes/strayFieldAuth/bearerFormat" + } + } + }, + "provenance": { + "source": 0, + "pointer": "/components/securitySchemes/strayFieldAuth" + } } }, "servers": [ @@ -149,11 +174,31 @@ "auth": null } ], + "diagnostics": [ + { + "severity": "warning", + "code": "openapi/validation/validation-allowed-values", + "message": "[46:21] warning validation-allowed-values securityScheme.bearerFormat is not used for type=apiKey (only valid for type=http)", + "provenance": { + "source": 0, + "pointer": "46:21" + } + }, + { + "severity": "info", + "code": "openapi/degraded-construct", + "message": "security scheme bearerFormat is not defined by type \"apiKey\"; kept verbatim under Unmodeled", + "provenance": { + "source": 0, + "pointer": "/components/securitySchemes/strayFieldAuth/bearerFormat" + } + } + ], "sources": [ { "format": "openapi@3.2", "path": "security-schemes.yaml", - "hash": "427d72a88e3f9473be8b7be4b2a36bfc6fa8de4fc989057261f62ebaa8d43969" + "hash": "77a9a876541b6fa112aa3def29f818cb2d299eeaf3a74928a8eb39cae663ec64" } ] } diff --git a/testdata/conformance/openapi/security-schemes.yaml b/testdata/conformance/openapi/security-schemes.yaml index 820290c..b961625 100644 --- a/testdata/conformance/openapi/security-schemes.yaml +++ b/testdata/conformance/openapi/security-schemes.yaml @@ -35,3 +35,12 @@ components: openIdConnectUrl: https://example.com/.well-known/openid-configuration mtlsAuth: type: mutualTLS + # A field the declared type gives no meaning to survives beside the scheme + # rather than in it. bearerFormat describes an HTTP bearer token, so filling + # AuthScheme.BearerFormat here would say this apiKey has one; dropping it + # would lose text the document wrote. + strayFieldAuth: + type: apiKey + in: header + name: X-Stray + bearerFormat: JWT