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