From 050344d3b4e39e237fd4e6527c48263d2af49a2d Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 03:54:08 +0300 Subject: [PATCH 1/7] fix(compilers/openapi): drop broken security requirements whole MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A security requirement is a conjunction: every scheme it names must be satisfied together. When one of those schemes was not declared under components.securitySchemes, the lowering only dropped that member and kept the rest of the requirement — which for a single-scheme requirement left AuthRequirement{Schemes: nil}, the encoding ir-design §9 reserves for "no auth is one acceptable choice". A requirement that demanded an undeclared scheme was compiling to IR that said the opposite: auth is optional. Drop the whole requirement instead, mirroring how a dangling type reference is refused elsewhere in this compiler. If every option in an originally non-empty security list drops this way, the list itself now collapses to nil ("no default declared") rather than surfacing as an empty, non-nil slice, which ir-design §9 reserves for a deliberate "explicitly public" declaration — a list the source declared empty to begin with is left untouched, since that emptiness is real rather than a byproduct of dropping. Also point the diagnostic at the requirement's own /security/ pointer (document- and operation-level) instead of the nonexistent components.securitySchemes entry it used to report. --- compilers/openapi/danglingcheck_test.go | 9 +- compilers/openapi/internal/auth/auth.go | 67 +++++++--- .../internal/auth/auth_internal_test.go | 3 +- compilers/openapi/internal/auth/auth_test.go | 118 +++++++++++++++--- .../openapi/internal/operation/operations.go | 4 +- 5 files changed, 160 insertions(+), 41 deletions(-) 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..d8e06e2 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" @@ -170,45 +171,73 @@ func scopeMap(f *soa.OAuthFlow) map[string]string { return out } -// LowerSecurityRequirements lowers an OR-of-ANDs security list (ir-design §9): a +// LowerSecurityRequirements lowers an OR-of-ANDs security list (ir-design §9), +// under base — the pointer of the node the list is declared on ("" for the +// document root, an operation's own decl pointer otherwise — see ids.Ptr): 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) { +// 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". +// +// 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 reads +// as the operator's own deliberate "explicitly public" (ir-design §9). A list +// the source declared empty to begin with is left untouched: that [] is real, +// not a byproduct of dropping. +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 that is not declared under +// components.securitySchemes (or one that failed to resolve into the auth +// registry) invalidates the whole option, which the caller must drop rather +// than write out short a 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. +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), + diags = append(diags, c.DiagAt(ir.SeverityError, diag.UnresolvedRef, pointer, "security requirement references undeclared 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..af3399f 100644 --- a/compilers/openapi/internal/auth/auth_test.go +++ b/compilers/openapi/internal/auth/auth_test.go @@ -329,6 +329,17 @@ 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 +} + // 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 +356,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 +376,87 @@ 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_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. +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)) +} + +// 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") } // TestSecurityRequirements_AnEmptyListIsNotAnAbsentOne pins the difference @@ -386,11 +472,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), From db7c125c7bc5c7072b4239ba8be877f983b3e062 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 04:06:12 +0300 Subject: [PATCH 2/7] fix(compilers/openapi): clarify auth.go doc comment wording Follow-up polish on the previous commit: tighten two doc comments left awkwardly worded after the security-requirement fix. --- compilers/openapi/internal/auth/auth.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/compilers/openapi/internal/auth/auth.go b/compilers/openapi/internal/auth/auth.go index d8e06e2..b87a437 100644 --- a/compilers/openapi/internal/auth/auth.go +++ b/compilers/openapi/internal/auth/auth.go @@ -185,10 +185,10 @@ func scopeMap(f *soa.OAuthFlow) map[string]string { // 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 reads -// as the operator's own deliberate "explicitly public" (ir-design §9). A list -// the source declared empty to begin with is left untouched: that [] is real, -// not a byproduct of dropping. +// — "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. func LowerSecurityRequirements(c lowering.Ctx, reqs []*soa.SecurityRequirement, base string) ([]ir.AuthRequirement, []ir.Diagnostic) { if reqs == nil { return nil, nil @@ -213,8 +213,8 @@ func LowerSecurityRequirements(c lowering.Ctx, reqs []*soa.SecurityRequirement, // 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) invalidates the whole option, which the caller must drop rather -// than write out short a member — never a dangling AuthID (issue #14), and +// registry) 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 From a6144cfbcb185025b3169da4430e1d7da24b1821 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 11:23:34 +0300 Subject: [PATCH 3/7] test(compilers/openapi): pin the security diagnostic's own declaration site MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The requirement pointer this change introduced was only ever asserted for an option at index 0 of an inline path operation, and both halves of the pointer are load-bearing. Spelling every option's index as 0, or basing an operation's pointer on its mount rather than its declaration, left the whole suite green while sending a reader to a requirement that is perfectly valid — or, for a $ref'd path item, to a node that addresses nothing at all. Pin the broken option's own index on the partial-drop case, and add a case covering the carriers an inline path operation cannot stand in for: a webhook, a callback operation, and a $ref'd path item, whose mount and declaration differ. Also write down what collapsing a fully dropped list to nil costs. A carrier whose every option drops becomes indistinguishable from one that never declared security, so an operation reads as requiring the service default's scheme — one it never named — or as unauthenticated where there is no default. nil is still the right spelling, being the only one that never reduces a demanded requirement to explicitly public, but the residue belongs next to the decision rather than in a reviewer's head. --- compilers/openapi/internal/auth/auth.go | 13 +++ compilers/openapi/internal/auth/auth_test.go | 107 +++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/compilers/openapi/internal/auth/auth.go b/compilers/openapi/internal/auth/auth.go index b87a437..4925839 100644 --- a/compilers/openapi/internal/auth/auth.go +++ b/compilers/openapi/internal/auth/auth.go @@ -189,6 +189,19 @@ func scopeMap(f *soa.OAuthFlow) map[string]string { // 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 diff --git a/compilers/openapi/internal/auth/auth_test.go b/compilers/openapi/internal/auth/auth_test.go index af3399f..deb4514 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" @@ -340,6 +341,31 @@ func firstDiagAt(diags []ir.Diagnostic, code string) (ir.Diagnostic, bool) { return ir.Diagnostic{}, false } +// 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)) @@ -411,6 +437,11 @@ paths: {} // 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 @@ -429,6 +460,10 @@ components: 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 @@ -459,6 +494,78 @@ paths: "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 // between a document that says nothing about security and one that says // explicitly that none is required. From 23dcce82d827e3e9e56cb8f13cbe40240e374487 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 15:27:32 +0300 Subject: [PATCH 4/7] test(compilers/openapi): report every undeclared scheme an option names lowerSecurityRequirement documents that a multi-member option reports each of its bad names, but nothing held it to that: stopping at the first bad member drops the same option and collapses the same list, so the compiled Auth is identical either way and only the diagnostics tell them apart. Replacing continue with break left the whole suite green. Pin it with an option naming two undeclared schemes, asserting both messages in source order and the pointer they share. Also record at LowerSecurityRequirements that a non-object security entry arrives as an empty option rather than a nil one, so it still lowers to the empty-option encoding. That sibling is issue #284's, and saying so here keeps the next reader off the nil guard, which is not its site. --- compilers/openapi/internal/auth/auth.go | 20 ++++++---- compilers/openapi/internal/auth/auth_test.go | 40 ++++++++++++++++++++ 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/compilers/openapi/internal/auth/auth.go b/compilers/openapi/internal/auth/auth.go index 4925839..607585b 100644 --- a/compilers/openapi/internal/auth/auth.go +++ b/compilers/openapi/internal/auth/auth.go @@ -171,13 +171,19 @@ func scopeMap(f *soa.OAuthFlow) map[string]string { return out } -// LowerSecurityRequirements lowers an OR-of-ANDs security list (ir-design §9), -// under base — the pointer of the node the list is declared on ("" for the -// document root, an operation's own decl pointer otherwise — see ids.Ptr): 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". +// 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 the nil guard below is not that site. // // 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 diff --git a/compilers/openapi/internal/auth/auth_test.go b/compilers/openapi/internal/auth/auth_test.go index deb4514..b615311 100644 --- a/compilers/openapi/internal/auth/auth_test.go +++ b/compilers/openapi/internal/auth/auth_test.go @@ -341,6 +341,19 @@ func firstDiagAt(diags []ir.Diagnostic, code string) (ir.Diagnostic, bool) { 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 +} + // 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 { @@ -408,6 +421,33 @@ components: "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_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 From 72af7d319d8ccac6911a2f0a1b09ea07ee7a516d Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 15:40:43 +0300 Subject: [PATCH 5/7] docs(compilers/openapi): name the guard the security note points away from --- compilers/openapi/internal/auth/auth.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compilers/openapi/internal/auth/auth.go b/compilers/openapi/internal/auth/auth.go index 607585b..1385105 100644 --- a/compilers/openapi/internal/auth/auth.go +++ b/compilers/openapi/internal/auth/auth.go @@ -183,7 +183,8 @@ func scopeMap(f *soa.OAuthFlow) map[string]string { // 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 the nil guard below is not that site. +// 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 From a913b4addf587b4efc34c11be5f13d079ace5e0f Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 16:44:45 +0300 Subject: [PATCH 6/7] fix(compilers/openapi): site the security scheme a $ref cannot reach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A components.securitySchemes entry whose $ref resolves to nothing was skipped without a word. The load phase does report the resolution failure, but with no pointer at all (#235), and the only sited report came from a requirement naming the scheme — which nothing has to do. An unreferenced entry was therefore a scheme the document declares, the IR drops, and no diagnostic places. Report it where it is written, at /components/securitySchemes/. That exposed a contradiction between the two reports. A requirement naming such a scheme was told it "references undeclared scheme", while the entry beside it says the document declares one whose $ref is broken — sending a reader to add a declaration already there. Both now say the name resolves to no scheme, which is true whether the entry was written or not. --- compilers/openapi/internal/auth/auth.go | 33 +++++--- compilers/openapi/internal/auth/auth_test.go | 79 +++++++++++++++++++- 2 files changed, 99 insertions(+), 13 deletions(-) diff --git a/compilers/openapi/internal/auth/auth.go b/compilers/openapi/internal/auth/auth.go index 1385105..b00d309 100644 --- a/compilers/openapi/internal/auth/auth.go +++ b/compilers/openapi/internal/auth/auth.go @@ -27,6 +27,13 @@ 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. func LowerSecuritySchemes(c lowering.Ctx) (map[ir.AuthID]ir.AuthScheme, []ir.Diagnostic) { comps := c.Doc.Components if comps == nil { @@ -41,6 +48,9 @@ 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, c.DiagAt(ir.SeverityError, diag.UnresolvedRef, + ids.Ptr("components", "securitySchemes", name), + "security scheme %q resolves to nothing this document declares", name)) continue } scheme, schemeDiags := lowerSecurityScheme(c, name, ss) @@ -231,14 +241,19 @@ func LowerSecurityRequirements(c lowering.Ctx, reqs []*soa.SecurityRequirement, // 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 that is not declared under -// components.securitySchemes (or one that failed to resolve into the auth -// registry) 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. +// 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 { @@ -250,7 +265,7 @@ func lowerSecurityRequirement(c lowering.Ctx, req *soa.SecurityRequirement, poin id := ids.Auth(name) if !c.DeclaresAuth(id) { diags = append(diags, c.DiagAt(ir.SeverityError, diag.UnresolvedRef, pointer, - "security requirement references undeclared scheme %q", name)) + "security requirement references unresolved scheme %q", name)) ok = false continue } diff --git a/compilers/openapi/internal/auth/auth_test.go b/compilers/openapi/internal/auth/auth_test.go index b615311..1fa8666 100644 --- a/compilers/openapi/internal/auth/auth_test.go +++ b/compilers/openapi/internal/auth/auth_test.go @@ -268,9 +268,11 @@ components: } // TestLowerSecuritySchemes_NothingLoweredIsNilNotEmpty pins the guard that -// 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. +// keeps an empty map out of the document, and that the entry it drops is +// reported. 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 — but dropping the entry without a word would +// leave the document's own declaration unaccounted for. func TestLowerSecuritySchemes_NothingLoweredIsNilNotEmpty(t *testing.T) { t.Parallel() doc := &soa.OpenAPI{Components: &soa.Components{ @@ -281,7 +283,39 @@ 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) + require.Len(t, diags, 1, "but it is reported, not dropped in silence: %+v", diags) + assert.Equal(t, ir.SeverityError, diags[0].Severity) + assert.Equal(t, diag.UnresolvedRef, diags[0].Code) + assert.Equal(t, "/components/securitySchemes/ghost", diags[0].Provenance.Pointer, + "at the entry the document wrote, which is a position that exists") +} + +// TestLowerSecuritySchemes_AnUnreferencedBrokenEntryIsStillSited pins the case +// with no other reporter, through the compiler rather than a hand-built node. +// +// A scheme whose $ref resolves to nothing is dropped from the registry. When a +// requirement names it, that requirement's own diagnostic already sites the +// trouble and this one reads as redundant — but nothing has to name it, and +// then the entry is a scheme the document declares, the IR drops, and no sited +// diagnostic accounts for. The load phase does report the underlying resolution +// failure, at no pointer at all (issue #235), which is why the assertion below +// is on the pointer rather than on how many reports there are. +func TestLowerSecuritySchemes_AnUnreferencedBrokenEntryIsStillSited(t *testing.T) { + t.Parallel() + doc, _, diags := serviceSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: {} +components: + securitySchemes: + ghost: {$ref: '#/components/securitySchemes/Missing'} + key: {type: apiKey, in: header, name: X-Key} +`) + require.Len(t, doc.Auth, 1, "the resolvable sibling is still interned") + assert.Contains(t, doc.Auth, ids.Auth("key")) + assert.NotContains(t, doc.Auth, ids.Auth("ghost"), "the broken one is not") + assert.Contains(t, sortedPointersAt(diags, diag.UnresolvedRef), + "/components/securitySchemes/ghost", + "the drop is sited at the entry that was written: %+v", diags) } // TestLowerSecuritySchemes_NoComponentsAtAll pins the two earlier exits: a @@ -448,6 +482,43 @@ paths: {} "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") + + byPointer := make(map[string]string) + for _, d := range diags { + if d.Code == diag.UnresolvedRef { + byPointer[d.Provenance.Pointer] = d.Message + } + } + entry, ok := byPointer["/components/securitySchemes/ghost"] + require.True(t, ok, "the entry that failed to resolve is reported: %+v", diags) + assert.Contains(t, entry, "resolves to nothing") + assert.Contains(t, entry, `"ghost"`, "and names the scheme, so the sentence stands without its pointer") + + req, ok := byPointer["/security/0"] + require.True(t, ok, "so is the requirement that names it: %+v", diags) + assert.NotContains(t, req, "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 From bc191ebc554f30156d96fea169484387f76d24ab Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 6 Aug 2026 16:58:33 +0300 Subject: [PATCH 7/7] fix(compilers/openapi): report only the scheme entry nothing else places The report added for an unresolvable securitySchemes entry claimed more than it could know. Two kinds of entry lower to no scheme: one whose $ref resolves to nothing, and one written as something other than an object. Both were told their $ref resolved to nothing, which for a null or a scalar names a reference the document never wrote. Only the first is unplaced, and only it is reported here. An entry that is not an object already draws the loader's type-mismatch, which names the entry and the fault, so a second report sends the reader to the same position to learn less. The message now names the failing reference as well as the scheme, so it stands on its own once the two are separated. --- compilers/openapi/internal/auth/auth.go | 33 ++++- compilers/openapi/internal/auth/auth_test.go | 127 +++++++++++++------ 2 files changed, 115 insertions(+), 45 deletions(-) diff --git a/compilers/openapi/internal/auth/auth.go b/compilers/openapi/internal/auth/auth.go index b00d309..0a55d7b 100644 --- a/compilers/openapi/internal/auth/auth.go +++ b/compilers/openapi/internal/auth/auth.go @@ -34,6 +34,8 @@ import ( // 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 { @@ -48,9 +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, c.DiagAt(ir.SeverityError, diag.UnresolvedRef, - ids.Ptr("components", "securitySchemes", name), - "security scheme %q resolves to nothing this document declares", name)) + diags = append(diags, unresolvableSchemeDiags(c, name, rs)...) continue } scheme, schemeDiags := lowerSecurityScheme(c, name, ss) @@ -63,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) { diff --git a/compilers/openapi/internal/auth/auth_test.go b/compilers/openapi/internal/auth/auth_test.go index 1fa8666..98cceed 100644 --- a/compilers/openapi/internal/auth/auth_test.go +++ b/compilers/openapi/internal/auth/auth_test.go @@ -268,11 +268,15 @@ components: } // TestLowerSecuritySchemes_NothingLoweredIsNilNotEmpty pins the guard that -// keeps an empty map out of the document, and that the entry it drops is -// reported. 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 — but dropping the entry without a word would -// leave the document's own declaration unaccounted for. +// 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{ @@ -283,39 +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") - require.Len(t, diags, 1, "but it is reported, not dropped in silence: %+v", diags) - assert.Equal(t, ir.SeverityError, diags[0].Severity) - assert.Equal(t, diag.UnresolvedRef, diags[0].Code) - assert.Equal(t, "/components/securitySchemes/ghost", diags[0].Provenance.Pointer, - "at the entry the document wrote, which is a position that exists") + assert.Empty(t, diags, "a nil entry names no reference that could have failed") } -// TestLowerSecuritySchemes_AnUnreferencedBrokenEntryIsStillSited pins the case -// with no other reporter, through the compiler rather than a hand-built node. +// 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. // -// A scheme whose $ref resolves to nothing is dropped from the registry. When a -// requirement names it, that requirement's own diagnostic already sites the -// trouble and this one reads as redundant — but nothing has to name it, and -// then the entry is a scheme the document declares, the IR drops, and no sited -// diagnostic accounts for. The load phase does report the underlying resolution -// failure, at no pointer at all (issue #235), which is why the assertion below -// is on the pointer rather than on how many reports there are. -func TestLowerSecuritySchemes_AnUnreferencedBrokenEntryIsStillSited(t *testing.T) { +// 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() - doc, _, diags := serviceSpec(t, `openapi: 3.1.0 + 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: {$ref: '#/components/securitySchemes/Missing'} + ghost: `+tc.entry+` key: {type: apiKey, in: header, name: X-Key} `) - require.Len(t, doc.Auth, 1, "the resolvable sibling is still interned") - assert.Contains(t, doc.Auth, ids.Auth("key")) - assert.NotContains(t, doc.Auth, ids.Auth("ghost"), "the broken one is not") - assert.Contains(t, sortedPointersAt(diags, diag.UnresolvedRef), - "/components/securitySchemes/ghost", - "the drop is sited at the entry that was written: %+v", diags) + 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 @@ -388,6 +423,19 @@ func messagesAt(diags []ir.Diagnostic, code string) []string { 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 { @@ -502,20 +550,15 @@ components: `) assert.Nil(t, svc.Auth, "the sole option names a scheme that resolves to nothing") - byPointer := make(map[string]string) - for _, d := range diags { - if d.Code == diag.UnresolvedRef { - byPointer[d.Provenance.Pointer] = d.Message - } - } - entry, ok := byPointer["/components/securitySchemes/ghost"] - require.True(t, ok, "the entry that failed to resolve is reported: %+v", diags) - assert.Contains(t, entry, "resolves to nothing") - assert.Contains(t, entry, `"ghost"`, "and names the scheme, so the sentence stands without its pointer") - - req, ok := byPointer["/security/0"] - require.True(t, ok, "so is the requirement that names it: %+v", diags) - assert.NotContains(t, req, "undeclared", + 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") }