diff --git a/compilers/openapi/danglingcheck_test.go b/compilers/openapi/danglingcheck_test.go index 88d8270..a144e46 100644 --- a/compilers/openapi/danglingcheck_test.go +++ b/compilers/openapi/danglingcheck_test.go @@ -147,15 +147,18 @@ func TestDanglingRefs_f07(t *testing.T) { } // TestDanglingRefs_f30 pins the auth case: a security requirement naming an -// undeclared scheme is dropped and diagnosed, never written as a dangling AuthID. +// undeclared scheme is dropped whole and diagnosed, never written as a dangling +// AuthID and never left behind as AuthRequirement{Schemes: nil} — the "no auth +// is one acceptable choice" encoding a broken requirement must not collapse +// into (issue #41). f30's list has exactly this one, sole option, so the list +// itself collapses to nil rather than surfacing as [] ("explicitly public"). func TestDanglingRefs_f30(t *testing.T) { t.Parallel() doc, diags := compileFile(t, danglingDir, "f30-protocol-surface.yaml", "f30.yaml") assert.Empty(t, danglingRefs(doc)) assert.Empty(t, doc.Auth, "no scheme is declared") require.Len(t, doc.Services, 1) - require.Len(t, doc.Services[0].Auth, 1, "the requirement option is kept") - assert.Empty(t, doc.Services[0].Auth[0].Schemes, "its undeclared scheme is dropped") + assert.Nil(t, doc.Services[0].Auth, "the sole option is dropped whole, collapsing the list to nil") assert.True(t, hasErrorRef(diags), "the drop is diagnosed") } diff --git a/compilers/openapi/internal/auth/auth.go b/compilers/openapi/internal/auth/auth.go index 93f399d..0a55d7b 100644 --- a/compilers/openapi/internal/auth/auth.go +++ b/compilers/openapi/internal/auth/auth.go @@ -10,6 +10,7 @@ package auth import ( "maps" + "strconv" "strings" soa "github.com/speakeasy-api/openapi/openapi" @@ -26,6 +27,15 @@ import ( // LowerSecuritySchemes interns every declared security scheme into the auth // registry keyed by ids.Auth(name) (ir-design §9). Run before the service walk // so operation- and document-level requirements reference registered IDs. +// +// An entry whose $ref resolves to nothing is reported at its own components +// pointer and interned nowhere. It is reported here rather than left to the +// requirements that name it, because nothing has to name it: the load phase's +// report of the same failure carries no pointer at all (issue #235), so an +// 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. func LowerSecuritySchemes(c lowering.Ctx) (map[ir.AuthID]ir.AuthScheme, []ir.Diagnostic) { comps := c.Doc.Components if comps == nil { @@ -40,6 +50,7 @@ func LowerSecuritySchemes(c lowering.Ctx) (map[ir.AuthID]ir.AuthScheme, []ir.Dia for name, rs := range schemes.All() { ss := resolve.Object[soa.SecurityScheme](rs) if ss == nil { + diags = append(diags, unresolvableSchemeDiags(c, name, rs)...) continue } scheme, schemeDiags := lowerSecurityScheme(c, name, ss) @@ -52,6 +63,33 @@ func LowerSecuritySchemes(c lowering.Ctx) (map[ir.AuthID]ir.AuthScheme, []ir.Dia return out, diags } +// unresolvableSchemeDiags reports a securitySchemes entry that lowered to no +// scheme — but only the one shape that nothing else places. +// +// Two kinds of entry reach the caller's nil: one written as something other +// than an object (null, a scalar, a sequence), and one whose $ref resolves to +// nothing — a missing internal target, or an external one this compile refuses. +// Only the second is unplaced. The first already draws the loader's +// type-mismatch, which names both the entry and what was wrong with it, so a +// second report here would send the reader to the same position to learn less. +// +// rs is the entry as the document wrote it, which is what separates the two: +// the reference is empty for everything that is not one. Its own nil is not +// reachable from a parsed document — a malformed entry still arrives as an +// object — so that guard is for a hand-built node, matching resolve.Object's. +func unresolvableSchemeDiags(c lowering.Ctx, name string, rs *soa.ReferencedSecurityScheme) []ir.Diagnostic { + if rs == nil { + return nil + } + ref := rs.GetReference().String() + if ref == "" { + return nil + } + return []ir.Diagnostic{c.DiagAt(ir.SeverityError, diag.UnresolvedRef, + ids.Ptr("components", "securitySchemes", name), + "security scheme %q has a $ref that resolves to nothing: %q", name, ref)} +} + // 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) { @@ -170,45 +208,98 @@ func scopeMap(f *soa.OAuthFlow) map[string]string { return out } -// LowerSecurityRequirements lowers an OR-of-ANDs security list (ir-design §9): a -// nil list inherits the enclosing default; a non-nil list yields one -// AuthRequirement per option in source order. An empty option object {} means -// "no auth is one acceptable choice". -func LowerSecurityRequirements(c lowering.Ctx, reqs []*soa.SecurityRequirement) ([]ir.AuthRequirement, []ir.Diagnostic) { +// LowerSecurityRequirements lowers an OR-of-ANDs security list (ir-design §9) +// declared under base — the pointer of the node carrying the list, which is "" +// at the document root and an operation's own declaration pointer, never its +// mount, otherwise: a nil list inherits the enclosing default; a non-nil list +// yields one AuthRequirement per surviving option, each diagnosed if need be at +// its own base+/security/ pointer. An empty option object {} means "no +// auth is one acceptable choice". +// +// An entry the source wrote as something other than an object reaches here as +// an empty option rather than as a nil one, so it lowers to that same encoding +// and the collapse below never sees it. Telling the two apart needs the parse +// the loader already rejected, so it is issue #284's to fix and deliberately +// out of scope here — which is also why lowerSecurityRequirement's nil guard is +// not that site, however much it looks like it. +// +// A requirement is a conjunction: every member must resolve for the option to +// mean anything, so an option naming even one undeclared scheme is dropped in +// full rather than surviving with just that member gone (issue #41) — the +// latter would silently rewrite "this option requires an undeclared scheme" as +// "no auth is also fine", the empty-option encoding above. When every option in +// an originally non-empty list drops this way, the list itself collapses to nil +// — "inherits the enclosing default" — rather than surfacing as [], which +// ir-design §9 reserves for a deliberate "explicitly public" declaration. A +// list the source declared empty to begin with is left untouched: that [] is +// real, not a byproduct of dropping. +// +// What that collapse costs, deliberately: a carrier whose every option drops +// becomes indistinguishable from one that never declared security, so an +// operation reads as requiring whatever the service default requires — a scheme +// it never named — or as unauthenticated where there is no default. Both +// misstate the source, because the IR has no encoding for "auth is required but +// its scheme is undeclared" and issue #14 forbids minting an AuthID nothing +// backs. nil is chosen because it is the only spelling that never reduces a +// demanded requirement to explicitly public, and every collapse carries an +// error diagnostic. The dropped text is not kept under Unmodeled: a name that +// resolves to nothing is a defect in the document rather than a construct the +// IR declines to model, which is the call an unresolvable $ref in a schema +// position and an unresolvable discriminator mapping already get here. +func LowerSecurityRequirements(c lowering.Ctx, reqs []*soa.SecurityRequirement, base string) ([]ir.AuthRequirement, []ir.Diagnostic) { if reqs == nil { return nil, nil } out := make([]ir.AuthRequirement, 0, len(reqs)) var diags []ir.Diagnostic - for _, req := range reqs { - r, reqDiags := lowerSecurityRequirement(c, req) - out = append(out, r) + for i, req := range reqs { + pointer := base + ids.Ptr("security", strconv.Itoa(i)) + r, ok, reqDiags := lowerSecurityRequirement(c, req, pointer) diags = append(diags, reqDiags...) + if ok { + out = append(out, r) + } + } + if len(reqs) > 0 && len(out) == 0 { + return nil, diags } return out, diags } -// lowerSecurityRequirement lowers one requirement option: each member is a -// scheme reference plus the scopes required of it within this option. A member -// naming a scheme that is not declared under components.securitySchemes (or one -// that failed to resolve into the auth registry) is dropped with one error -// diagnostic rather than writing a dangling AuthID (issue #14). -func lowerSecurityRequirement(c lowering.Ctx, req *soa.SecurityRequirement, -) (ir.AuthRequirement, []ir.Diagnostic) { +// lowerSecurityRequirement lowers one requirement option declared at pointer: +// each member is a scheme reference plus the scopes required of it within this +// option. A member naming a scheme the auth registry does not hold invalidates +// the whole option, which the caller must drop in full rather than just that +// member — never a dangling AuthID (issue #14), and never an unintended +// empty-option encoding (issue #41). ok reports whether the option survives; +// every unresolved member is still diagnosed individually, at the shared +// 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. +func lowerSecurityRequirement(c lowering.Ctx, req *soa.SecurityRequirement, pointer string, +) (r ir.AuthRequirement, ok bool, diags []ir.Diagnostic) { if req == nil { - return ir.AuthRequirement{}, nil + return ir.AuthRequirement{}, true, nil } var uses []ir.SchemeUse - var diags []ir.Diagnostic + ok = true for name, scopes := range req.All() { id := ids.Auth(name) if !c.DeclaresAuth(id) { - diags = append(diags, c.DiagAt(ir.SeverityError, diag.UnresolvedRef, - ids.Ptr("components", "securitySchemes", name), - "security requirement references undeclared scheme %q", name)) + diags = append(diags, c.DiagAt(ir.SeverityError, diag.UnresolvedRef, pointer, + "security requirement references unresolved scheme %q", name)) + ok = false continue } uses = append(uses, ir.SchemeUse{Scheme: id, Scopes: scopes}) } - return ir.AuthRequirement{Schemes: uses}, diags + if !ok { + return ir.AuthRequirement{}, false, diags + } + return ir.AuthRequirement{Schemes: uses}, true, diags } diff --git a/compilers/openapi/internal/auth/auth_internal_test.go b/compilers/openapi/internal/auth/auth_internal_test.go index c754934..523ef1c 100644 --- a/compilers/openapi/internal/auth/auth_internal_test.go +++ b/compilers/openapi/internal/auth/auth_internal_test.go @@ -10,8 +10,9 @@ import ( func TestLowerSecurityRequirement_Nil(t *testing.T) { t.Parallel() - got, diags := lowerSecurityRequirement(lowering.Ctx{}, nil) + got, ok, diags := lowerSecurityRequirement(lowering.Ctx{}, nil, "/security/0") assert.Empty(t, got.Schemes) + assert.True(t, ok, "a nil requirement entry is not a resolution failure") assert.Empty(t, diags) } diff --git a/compilers/openapi/internal/auth/auth_test.go b/compilers/openapi/internal/auth/auth_test.go index 98b5056..98cceed 100644 --- a/compilers/openapi/internal/auth/auth_test.go +++ b/compilers/openapi/internal/auth/auth_test.go @@ -1,6 +1,7 @@ package auth_test import ( + "slices" "testing" soa "github.com/speakeasy-api/openapi/openapi" @@ -270,6 +271,12 @@ components: // keeps an empty map out of the document. A components.securitySchemes block // whose every entry fails to resolve declares no scheme the IR can carry, and // an empty Auth map would be a field the source never wrote. +// +// The entry is a hand-built nil, which no parsed document produces — a +// malformed entry still arrives as an object. That makes this the nil-guard +// case rather than the reporting one, so nothing is reported: the entry carries +// no $ref to have failed. TestLowerSecuritySchemes_OnlyABrokenRefIsSitedHere +// covers the shapes a document can write, through the compiler. func TestLowerSecuritySchemes_NothingLoweredIsNilNotEmpty(t *testing.T) { t.Parallel() doc := &soa.OpenAPI{Components: &soa.Components{ @@ -280,7 +287,70 @@ func TestLowerSecuritySchemes_NothingLoweredIsNilNotEmpty(t *testing.T) { got, diags := auth.LowerSecuritySchemes(lowering.Ctx{Doc: doc}) assert.Nil(t, got, "an unresolvable entry leaves no map behind") - assert.Empty(t, diags) + assert.Empty(t, diags, "a nil entry names no reference that could have failed") +} + +// TestLowerSecuritySchemes_OnlyABrokenRefIsSitedHere pins which unresolvable +// entries this package reports and which it leaves alone, through the compiler +// rather than a hand-built node — the shapes below are what a document can +// actually write, and a hand-built one is not among them. +// +// Every case here drops the entry from the registry, and none is named by any +// security requirement, so nothing downstream would report it either. What +// separates them is whether anything else already places the fault. A $ref that +// resolves to nothing is reported by the load phase at no pointer at all +// (issue #235), leaving the entry unplaced — that is the gap this package +// fills. An entry written as something other than an object already draws the +// loader's type-mismatch, which names both the entry and what was wrong with +// it, so a second report would send the reader to the same place to learn less. +func TestLowerSecuritySchemes_OnlyABrokenRefIsSitedHere(t *testing.T) { + t.Parallel() + cases := []struct { + name string + entry string + // wantRef is the reference the report must name, or "" when this + // package is expected to report nothing at all. + wantRef string + }{ + { + name: "a $ref naming no target in this document", + entry: `{$ref: '#/components/securitySchemes/Missing'}`, + wantRef: "#/components/securitySchemes/Missing", + }, + { + name: "a $ref out to a document this compile refuses to read", + entry: `{$ref: 'other.yaml#/components/securitySchemes/X'}`, + wantRef: "other.yaml#/components/securitySchemes/X", + }, + {name: "an entry written as null", entry: `null`}, + {name: "an entry written as a scalar", entry: `42`}, + {name: "an entry written as a sequence", entry: `[a]`}, + } + 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 resolvable sibling still does") + + got := messagesAtPointer(diags, "/components/securitySchemes/ghost") + if tc.wantRef == "" { + assert.Empty(t, got, + "the loader already names this entry and its fault: %+v", diags) + return + } + require.Len(t, got, 1, "the entry is placed exactly once: %+v", diags) + assert.Contains(t, got[0], `"ghost"`, "the report names the entry") + assert.Contains(t, got[0], tc.wantRef, "and the reference that failed") + }) + } } // TestLowerSecuritySchemes_NoComponentsAtAll pins the two earlier exits: a @@ -329,6 +399,68 @@ func countDiagsAt(diags []ir.Diagnostic, code string, sev ir.Severity) int { return n } +// firstDiagAt returns the first diagnostic carrying code, so a test can assert +// on its provenance pointer. +func firstDiagAt(diags []ir.Diagnostic, code string) (ir.Diagnostic, bool) { + for _, d := range diags { + if d.Code == code { + return d, true + } + } + return ir.Diagnostic{}, false +} + +// messagesAt returns the message of every diagnostic carrying code, in the order +// they were reported, so a caller can pin which names were reported and not only +// how many. +func messagesAt(diags []ir.Diagnostic, code string) []string { + var out []string + for _, d := range diags { + if d.Code == code { + out = append(out, d.Message) + } + } + return out +} + +// messagesAtPointer returns the message of every diagnostic whose provenance +// names pointer, in report order. It filters on the pointer alone, so a report +// arriving at the right place under the wrong code is still returned. +func messagesAtPointer(diags []ir.Diagnostic, pointer string) []string { + var out []string + for _, d := range diags { + if d.Provenance.Pointer == pointer { + out = append(out, d.Message) + } + } + return out +} + +// sortedPointersAt returns every provenance pointer carried by a diagnostic +// with code, sorted so a caller pins the set rather than the walk's order. +func sortedPointersAt(diags []ir.Diagnostic, code string) []string { + var out []string + for _, d := range diags { + if d.Code == code { + out = append(out, d.Provenance.Pointer) + } + } + slices.Sort(out) + return out +} + +// operationsByDeclaration indexes every operation in svc by its provenance +// pointer, which is where the operation is declared rather than where it mounts. +func operationsByDeclaration(svc ir.Service) map[string]ir.Operation { + out := make(map[string]ir.Operation) + for _, g := range svc.Groups { + for _, op := range g.Operations { + out[op.Provenance.Pointer] = op + } + } + return out +} + // indexBy builds a lookup keyed by key(item). func indexBy[T any, K comparable](items []T, key func(T) K) map[K]T { out := make(map[K]T, len(items)) @@ -345,12 +477,15 @@ func pathsSpec(paths string) string { "paths:\n" + paths } -// TestSecurityRequirement_UndeclaredSchemeIsDroppedNotDangling pins the refusal -// issue #14 exists for. A requirement may name any string; only a name the -// document declares has an AuthID behind it, and writing one for a name it does -// not declare would put a reference into the IR that resolves to nothing. The -// requirement survives without that scheme, and the drop is reported. -func TestSecurityRequirement_UndeclaredSchemeIsDroppedNotDangling(t *testing.T) { +// TestSecurityRequirement_OneUndeclaredMemberDropsTheWholeOption pins the +// refusal issue #14 exists for, corrected per issue #41. A requirement may name +// any string; only a name the document declares has an AuthID behind it, and +// writing one for a name it does not declare would put a reference into the IR +// that resolves to nothing. But a requirement is a conjunction — "ghost" and +// "key" must both be satisfied — so ghost failing to resolve makes the whole +// option unsatisfiable. Keeping the option with only ghost removed would leave +// behind a requirement for "key" alone, which is not what the source declared. +func TestSecurityRequirement_OneUndeclaredMemberDropsTheWholeOption(t *testing.T) { t.Parallel() _, svc, diags := serviceSpec(t, `openapi: 3.1.0 info: {title: T, version: "1"} @@ -362,15 +497,227 @@ components: securitySchemes: key: {type: apiKey, in: header, name: X-Key} `) - require.Len(t, svc.Auth, 1, "the requirement itself survives") - named := make([]ir.AuthID, 0, len(svc.Auth[0].Schemes)) - for _, use := range svc.Auth[0].Schemes { - named = append(named, use.Scheme) - } - assert.Equal(t, []ir.AuthID{ids.Auth("key")}, named, - "only the declared scheme is referenced; the other is dropped rather than dangling") + assert.Nil(t, svc.Auth, + "the sole option named an AND of ghost+key; ghost failing to resolve drops it whole, key included") assert.Equal(t, 1, countDiagsAt(diags, diag.UnresolvedRef, ir.SeverityError), - "and the drop is reported exactly once: %+v", diags) + "the drop is reported exactly once: %+v", diags) +} + +// TestSecurityRequirement_EveryUndeclaredMemberOfAnOptionIsReported pins the one +// thing dropping the option whole must not cost. The option is refused once, but +// each name that failed is still named — so a reader fixing the document sees +// every scheme it has to declare, not only the first one the walk tripped over. +// +// Nothing about the compiled Auth can see this: stopping at the first bad member +// drops exactly the same option and collapses exactly the same list, so only the +// diagnostics separate the two. Both names are undeclared here for that reason, +// and they are read in source order, which is the order the IR promises. +func TestSecurityRequirement_EveryUndeclaredMemberOfAnOptionIsReported(t *testing.T) { + t.Parallel() + _, svc, diags := serviceSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +security: + - ghostA: [] + ghostB: [] +paths: {} +`) + assert.Nil(t, svc.Auth, "neither name resolves, so the sole option drops and the list collapses") + reported := messagesAt(diags, diag.UnresolvedRef) + require.Len(t, reported, 2, "both undeclared names are reported, not just the first: %+v", diags) + assert.Contains(t, reported[0], `"ghostA"`) + assert.Contains(t, reported[1], `"ghostB"`, "reported in the order the source names them") + assert.Equal(t, []string{"/security/0", "/security/0"}, sortedPointersAt(diags, diag.UnresolvedRef), + "each names the option that declared it, which both members share") +} + +// TestSecurityRequirement_ADeclaredSchemeIsNeverCalledUndeclared pins that the +// two reports about one broken scheme agree with each other. The document does +// declare "ghost" — its $ref is what fails — so the requirement naming it must +// not be told the scheme is undeclared, which contradicts the entry-level report +// standing right beside it and sends a reader to add a declaration already +// there. Both now say the name resolves to nothing, which is true whether the +// document wrote the entry or not. +func TestSecurityRequirement_ADeclaredSchemeIsNeverCalledUndeclared(t *testing.T) { + t.Parallel() + _, svc, diags := serviceSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +security: + - ghost: [] +paths: {} +components: + securitySchemes: + ghost: {$ref: '#/components/securitySchemes/Missing'} +`) + assert.Nil(t, svc.Auth, "the sole option names a scheme that resolves to nothing") + + entry := messagesAtPointer(diags, "/components/securitySchemes/ghost") + require.Len(t, entry, 1, "the entry whose $ref failed is reported: %+v", diags) + assert.Contains(t, entry[0], `"ghost"`, "naming the scheme, so it stands without its pointer") + assert.Contains(t, entry[0], "resolves to nothing") + + req := messagesAtPointer(diags, "/security/0") + require.Len(t, req, 1, "so is the requirement that names it: %+v", diags) + assert.Contains(t, req[0], "unresolved", "which is true of a broken $ref and a typo alike") + assert.NotContains(t, req[0], "undeclared", + "the document declares this scheme; only its $ref is broken") +} + +// TestSecurityRequirement_SoleOptionCollapsesListToNil reproduces issue #41 +// directly: a document-level security list whose only option names an +// undeclared scheme must not leave behind AuthRequirement{Schemes: nil}, which +// ir-design §9 reads as "no auth is one acceptable choice" — the opposite of +// what a demanded-but-undeclared scheme means. Nor may it surface as [], which +// ir-design §9 reserves for a deliberate "explicitly public" declaration +// (Service.Auth's doc comment). The list collapses to nil instead: no usable +// default was declared, so operations fall back exactly as if security had +// never been written. +func TestSecurityRequirement_SoleOptionCollapsesListToNil(t *testing.T) { + t.Parallel() + doc, svc, diags := serviceSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +security: + - missing: [] +paths: {} +`) + assert.Nil(t, svc.Auth, "the only option is broken; the list is not left as [{}] or as []") + assert.Empty(t, doc.Auth, "no scheme is declared at all") + d, ok := firstDiagAt(diags, diag.UnresolvedRef) + require.True(t, ok, "an unresolved-ref diagnostic: %+v", diags) + assert.Equal(t, "/security/0", d.Provenance.Pointer, + "points at the requirement that named the missing scheme, not a nonexistent components entry") +} + +// TestSecurityRequirement_PartialListDropOnlyRemovesTheBrokenOption pins that a +// broken option is dropped in isolation: surviving options keep their source +// order, and a genuinely declared empty option ("no auth is also fine") is +// untouched by the drop of an unrelated, broken option. +// +// The broken option sits at index 1 on purpose. A diagnostic that named the +// list rather than the option inside it — or that spelled every option's index +// as 0 — would send a reader to a requirement that is perfectly valid, so the +// index is load-bearing and only a non-zero one can show that it is carried. +func TestSecurityRequirement_PartialListDropOnlyRemovesTheBrokenOption(t *testing.T) { + t.Parallel() + _, svc, diags := serviceSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +security: + - key: [] + - missing: [] + - {} +paths: {} +components: + securitySchemes: + key: {type: apiKey, in: header, name: X-Key} +`) + require.Len(t, svc.Auth, 2, "only the middle, broken option drops") + require.Len(t, svc.Auth[0].Schemes, 1) + assert.Equal(t, ids.Auth("key"), svc.Auth[0].Schemes[0].Scheme, "the first option survives in place") + assert.Empty(t, svc.Auth[1].Schemes, "the trailing empty option still means no-auth-is-fine") + assert.Equal(t, 1, countDiagsAt(diags, diag.UnresolvedRef, ir.SeverityError)) + d, ok := firstDiagAt(diags, diag.UnresolvedRef) + require.True(t, ok, "an unresolved-ref diagnostic: %+v", diags) + assert.Equal(t, "/security/1", d.Provenance.Pointer, + "the pointer names the broken option's own index, not the list or a constant 0") +} + +// TestSecurityRequirement_OperationLevelSoleOptionCollapsesToNil is the +// operation-level counterpart of TestSecurityRequirement_SoleOptionCollapsesListToNil: +// an operation's own security override degrades the same way the service +// default does — collapsing to nil, so the operation falls back to inheriting +// the service default rather than reading as explicitly public — and its +// diagnostic points at the operation's own declaration site rather than the +// document root or a nonexistent components entry. +func TestSecurityRequirement_OperationLevelSoleOptionCollapsesToNil(t *testing.T) { + t.Parallel() + _, svc, diags := serviceSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: + /x: + get: + operationId: x + security: [{missing: []}] + responses: {"200": {description: ok}} +`) + require.NotEmpty(t, svc.Groups, "the operation was lowered into a group") + require.NotEmpty(t, svc.Groups[0].Operations, "the operation was lowered") + op := svc.Groups[0].Operations[0] + assert.Nil(t, op.Auth, "the operation's sole option is broken; it now inherits the service default") + d, ok := firstDiagAt(diags, diag.UnresolvedRef) + require.True(t, ok, "an unresolved-ref diagnostic: %+v", diags) + assert.Equal(t, "/paths/~1x/get/security/0", d.Provenance.Pointer, + "points at the operation's own requirement, not a nonexistent components entry") +} + +// TestSecurityRequirement_EveryOperationCarrierDiagnosesAtItsDeclaration pins +// the base pointer for the carriers an operation-level security list sits on +// besides an inline path operation. All of them reach one LowerSecurityRequirements +// call, so a single wrong argument there misplaces every one of their +// diagnostics at once — and an inline path operation cannot show that, because +// it is mounted exactly where it is declared, leaving the two pointers equal. A +// $ref'd path item separates them: it is mounted under /paths and declared under +// /components, and only the declaration addresses a node the security list is +// written at. Its broken option sits at index 1 so the survivor is kept too. +// +// The pointers are compared as a sorted set: the claim is that each carrier +// reports at its own declaration, not that the walk visits them in some order. +func TestSecurityRequirement_EveryOperationCarrierDiagnosesAtItsDeclaration(t *testing.T) { + t.Parallel() + _, svc, diags := serviceSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: + /cb: + post: + operationId: cbOp + responses: {"200": {description: ok}} + callbacks: + onEvent: + '{$request.body#/url}': + post: + operationId: callbackOp + security: [{missing: []}] + responses: {"200": {description: ok}} + /reffed: + $ref: '#/components/pathItems/shared' +webhooks: + hook: + post: + operationId: hookOp + security: [{missing: []}] + responses: {"200": {description: ok}} +components: + pathItems: + shared: + get: + operationId: sharedOp + security: [{key: []}, {missing: []}] + responses: {"200": {description: ok}} + securitySchemes: + key: {type: apiKey, in: header, name: X-Key} +`) + want := []string{ + "/components/pathItems/shared/get/security/1", + "/paths/~1cb/post/callbacks/onEvent/{$request.body#~1url}/post/security/0", + "/webhooks/hook/post/security/0", + } + assert.Equal(t, want, sortedPointersAt(diags, diag.UnresolvedRef), + "each carrier reports at its own declaration site") + + // Each lookup is required to hit before it is asserted on: a missing key + // yields the zero Operation, whose Auth is nil, so an absent carrier would + // satisfy the nil assertions below without ever having been compiled. + ops := operationsByDeclaration(svc) + require.Len(t, ops, len(want)+1, "one operation per carrier, plus the callback's own parent") + hook, ok := ops["/webhooks/hook/post"] + require.True(t, ok, "the webhook operation was lowered") + assert.Nil(t, hook.Auth, "the webhook's sole option is broken, so it inherits") + callback, ok := ops["/paths/~1cb/post/callbacks/onEvent/{$request.body#~1url}/post"] + require.True(t, ok, "the callback operation was lowered") + assert.Nil(t, callback.Auth, "the callback operation's sole option is broken, so it inherits") + shared, ok := ops["/components/pathItems/shared/get"] + require.True(t, ok, "the $ref'd path item's operation was lowered") + require.Len(t, shared.Auth, 1, "it keeps its one good option") + require.Len(t, shared.Auth[0].Schemes, 1) + assert.Equal(t, ids.Auth("key"), shared.Auth[0].Schemes[0].Scheme) } // TestSecurityRequirements_AnEmptyListIsNotAnAbsentOne pins the difference @@ -386,11 +733,11 @@ func TestSecurityRequirements_AnEmptyListIsNotAnAbsentOne(t *testing.T) { t.Parallel() var c lowering.Ctx - absent, absentDiags := auth.LowerSecurityRequirements(c, nil) + absent, absentDiags := auth.LowerSecurityRequirements(c, nil, "") assert.Nil(t, absent, "no security key at all inherits the enclosing default") assert.Empty(t, absentDiags) - empty, emptyDiags := auth.LowerSecurityRequirements(c, []*soa.SecurityRequirement{}) + empty, emptyDiags := auth.LowerSecurityRequirements(c, []*soa.SecurityRequirement{}, "") assert.NotNil(t, empty, "an empty list is a declaration, not an absence") assert.Empty(t, empty, "and it declares no options") assert.Empty(t, emptyDiags) diff --git a/compilers/openapi/internal/operation/operations.go b/compilers/openapi/internal/operation/operations.go index 49df478..a3b7173 100644 --- a/compilers/openapi/internal/operation/operations.go +++ b/compilers/openapi/internal/operation/operations.go @@ -57,7 +57,7 @@ func LowerService(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorIndex svc.Name = compile.NamingFor(title) svc.Docs.Description = info.GetDescription() } - svcAuth, diags := auth.LowerSecurityRequirements(c, c.Doc.GetSecurity()) + svcAuth, diags := auth.LowerSecurityRequirements(c, c.Doc.GetSecurity(), "") svc.Auth = svcAuth groups := newServiceGroups() diags = append(diags, lowerPaths(c, ts, anchors, operationIDs, groups)...) @@ -249,7 +249,7 @@ func lowerOperation(c lowering.Ctx, ts *compile.Types, anchors *schema.AnchorInd // marked inferred — the one provenance in this compiler that is. opProv := c.ProvenanceAt(decl) opProv.Inferred = opCtx.inferred - opAuth, diags := auth.LowerSecurityRequirements(c, src.Security) + opAuth, diags := auth.LowerSecurityRequirements(c, src.Security, decl) op := ir.Operation{ ID: ids.Op(mount), Name: operationName(src, opCtx.method, opCtx.uriTemplate),