fix(compilers/openapi): drop and site broken security references - #273
Merged
Conversation
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/<index>
pointer (document- and operation-level) instead of the nonexistent
components.securitySchemes entry it used to report.
Follow-up polish on the previous commit: tighten two doc comments left awkwardly worded after the security-requirement fix.
…n site 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.
OmarAlJarrah
force-pushed
the
fix/openapi-undeclared-security-scheme
branch
from
August 6, 2026 08:27
9176689 to
a6144cf
Compare
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.
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/<name>. 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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A security requirement is an AND of every scheme it names — all of them have to be
satisfied together. When one named scheme wasn't declared under
components.securitySchemes, the lowering dropped only that member and kept the restof the requirement. For a requirement naming just the one undeclared scheme, that left
AuthRequirement{Schemes: nil}— the exact encodingir-design.md§9 defines for "noauth is one acceptable choice". So a document demanding an undeclared scheme compiled
to IR saying auth was optional: the requirement's meaning was inverted. An error
diagnostic was emitted, but a consumer that doesn't treat diagnostics as fatal would
still walk away with the inverted IR. The diagnostic itself also pointed at
/components/securitySchemes/<name>— a location that, by definition, does not exist —rather than the requirement that made the mistake.
This changes the drop granularity from "member" to "requirement": a requirement naming
even one undeclared scheme is now dropped in full, the same way a dangling type
reference is already refused elsewhere in this compiler (#14). If every option in an
originally non-empty
securitylist drops this way, the list itself now collapses tonil("no default declared") rather than surfacing as[], whichir-design.md§9reserves for a deliberate "explicitly public" declaration — collapsing to
[]insteadof
nilwould have silently traded one inverted meaning for another. A list the sourcedeclared empty to begin with (
security: []) is left untouched, since that emptinessis real rather than a byproduct of dropping. The diagnostic now carries the
requirement's own
/security/<index>pointer, at both the document-level default andeach operation's own override. Every carrier of an operation-level list — a path
operation, a webhook, a callback operation, an operation reached through a
$ref'd pathitem — funnels through the same
lowerOperationcall, so they all get the fix from onechange, and each reports at its own declaration site rather than where it mounts.
One consequence of the finer pointer is worth stating, because it changes what a reader
sees rather than only where they are pointed. The compiler collapses byte-identical
diagnostics, so two broken options naming the same undeclared scheme used to arrive as a
single report — same code, same
/components/securitySchemes/<name>, same message. Theynow differ by index and are reported separately, one per option that has to be fixed.
Two mounts of a single
$ref'd path item still collapse to one report, which is right:there it really is one declaration, and the pointer says so.
Closes #41.
The scheme entry a
$refcannot reachA
components.securitySchemesentry whose$refresolves to nothing was skippedwithout a word. The load phase does report the underlying resolution failure, but with
no pointer at all (#235), and the only sited report came from a requirement naming the
scheme — which nothing in a document has to do. An entry nothing references was
therefore a scheme the document declares, the IR drops, and no diagnostic places. It is
now reported where it is written, at
/components/securitySchemes/<name>, naming thereference that failed.
Only that shape is reported. An entry written as something other than an object — a
null, a scalar, a sequence — also lowers to no scheme, but the loader's type-mismatch
already names both the entry and the fault, so a second report would send a reader to
the same position to learn less. The two are told apart by whether the entry carries a
reference at all.
That exposed a contradiction between the two reports about one broken scheme. A
requirement naming it was told it "references undeclared scheme", while the entry beside
it says the document declares one whose
$refis broken — which sends a reader to add adeclaration that is already there. Both now say the name resolves to no scheme, true
whether the document wrote the entry or not.
This does not close #235, and is not meant to. That issue wants every unresolvable
reference sited, across the six positions that have no sited diagnostic at all, by
driving the resolver's own walk or scanning raw YAML — its own change, with its own
acceptance list. What lands here is one row of it, in the file this PR already rewrites.
Worth noting for whoever takes #235: its table currently credits the
securitySchemeposition with a sited diagnostic, which was only ever the requirement's, and only when
some requirement happened to name the scheme.
What the collapse costs
Dropping is not free, and the residue is worth stating rather than leaving for the next
reader to rediscover. A carrier whose every option drops becomes indistinguishable from
one that never declared
securityat all, so an operation that overrode the servicedefault with a broken requirement now reads as requiring the service default's scheme —
one it never named — or as unauthenticated where there is no default. Both misstate the
source. There is no third answer available: the IR has no encoding for "auth is required
but its scheme is undeclared", and #14 forbids minting an
AuthIDthat nothing backs.nilis chosen because it is the only spelling of the degradation that never reducesa demanded requirement to explicitly public, and every collapse carries an error
diagnostic naming the requirement that caused it.
One sibling degradation is deliberately left standing. A
securityentry the sourcewrote as something other than an object —
security: [null]— reaches the lowering asan empty option rather than as a nil one, so it lowers to the very
"no auth is one acceptable choice" encoding this change refuses for an undeclared name,
and the collapse never sees it. Telling the two apart needs the parse the loader has
already rejected rather than the drop rule changed here, which is what #284 asks for.
That is written down at
LowerSecurityRequirements, including the part a reader wouldotherwise get wrong: the nil-requirement guard just below it is not the site, because a
malformed entry never arrives as nil.
The dropped text is deliberately not preserved under
Unmodeled. A scheme name thatresolves to nothing is a defect in the document rather than a construct the IR declines
to model, so it belongs in the diagnostic channel — which is the call an unresolvable
$refin a schema position and an unresolvable discriminator mapping already get inthis compiler. This is written down at
LowerSecurityRequirementsso it sits next tothe decision.
Test plan
service auth: [{}](i.e.Schemes: nil) plus a diagnostic pointing at/components/securitySchemes/missing, which doesn't exist. After the fix, the samespec compiles to
service auth: nulland the diagnostic points at/security/0.security(absent,[],a sole broken option, a broken option among survivors,
{}beside a broken option,and an AND option where only one member resolves) against the same six shapes at
operation level, reading
Service.Auth,Operation.Authand every diagnostic out ofthe compiled IR in each cell. A genuinely declared
[]survives at both levels inevery row; a survivor keeps its source position;
{}is never confused with a droppedoption; and no cell leaves a dangling
AuthIDbehind.compilers/openapi/internal/auth/auth_test.go: a sole broken optioncollapsing the list to
nil(document- and operation-level, each pinning itsdiagnostic's pointer), a broken option among survivors dropping only itself while the
others keep source order and the diagnostic names that option's own index, one bad
member inside an AND option dropping the whole option (including its otherwise-valid
co-members), every non-inline carrier of an operation-level list reporting at its own
declaration site, an option naming two undeclared schemes reporting both of them in
source order at the pointer they share, and the pre-existing "an empty declared list
is not an absent one"
boundary re-verified against the new collapse logic so it doesn't regress.
compilers/openapi/danglingcheck_test.go'sTestDanglingRefs_f30, which hadpinned the old (buggy) behavior as expected output.
member-only drop instead of whole-requirement drop, no list-to-nil collapse, an
over-eager collapse that also eats a genuinely declared empty list
(
TestSecurityRequirements_AnEmptyListIsNotAnAbsentOne,TestOperation_ExplicitlyPublicSecurity,TestConformance/security-or-and), thediagnostic pointer reverted to the old nonexistent-components-entry, a nil requirement
entry treated as a resolution failure, the carrier's base pointer dropped from the
diagnostic, every option's index spelled as a constant
0, an operation's pointerbased on its mount rather than its declaration, and the member loop stopping at the
first unresolved name instead of continuing past it. The last three were what the added
assertions were written for: each left the whole suite green beforehand. The pointer
pair was invisible because every case until then used an inline path operation — whose
mount and declaration are equal — with its broken option at index 0; the early stop was
invisible because it drops the same option and collapses the same list, so only the
diagnostics tell the two apart.
compile.Diags' dedup key in both directions: twomembers of one option share a pointer but differ by message and both survive; two
options differ by index and both survive; two mounts of one
$ref'd path item areidentical in every field and collapse to one.
the compiler rather than a hand-built node — a
$refnaming no target here, a$refout to a refused document, and entries written as null, a scalar, and a sequence. Each
interns nothing while its resolvable sibling still does; the two
$refshapes arereported once at their own components pointer, naming both the scheme and the failing
reference; the other three are left to the loader. Separately, the entry-level and
requirement-level reports about one broken scheme are read together and asserted to
agree rather than contradict. Mutations planted and confirmed red: the silent skip
restored, the report widened back to every nil entry, the pointer missing the scheme
name, the message dropping the scheme name, the message dropping the reference, the
severity downgraded to a warning, the empty registry returned instead of nil, and the
requirement message reverted to "undeclared".
gofmt -l,go vet ./...,golangci-lint run,go build ./..., and./scripts/check-coverage.sh(100% statement coverage) all pass.