Repository navigation
fix(compilers/openapi): report an oauth2 scheme with no flow - #727
Open
fuad-daoud wants to merge 2 commits into
Open
fuad-daoud wants to merge 2 commits into
fuad-daoud wants to merge 2 commits into
Conversation
Resolve the conflicts with main and bring the doc comments this branch adds under the 100-word cap main now enforces. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GCzyGPq9DQ7RLfagZm2JB5
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
{type: oauth2, flows: {}}is structurally valid — every member ofOAuthFlowsis optional — so it lowers to anAuthKindOAuth2scheme with an empty flow list and no report from anywhere. An oauth2 scheme states no token endpoint without a flow, so the entry is unusable, and a requirement naming it keeps a liveAuthIDfor a scheme that says nothing an emitter can implement (#646).Rule. An entry that lowers to
AuthKindOAuth2whose lowered flow list is empty is reported once, at the entry's owncomponents/securitySchemes/<name>pointer. One rule for every spelling:flowsabsent,flows: {}, or a flows object holding only keys this model does not name.Code, severity, placement. New code
openapi/oauth2-no-flow(diag.OAuth2NoFlow), warning, at the entry. The placement is the half the load phase cannot supply:flows: {}is structurally valid, so the loader has nothing to refuse there, and the spelling it does refuse,flowsabsent, it reports asvalidation-required-fieldcarrying no pointer at all, which sites an unreferenced entry nowhere.The scheme still interns — deliberately not the refusal #294 points at. #294 refused an entry naming no mechanism because
ir.AuthKindhas no value for "the document did not say which mechanism": interning would have asserted that the API is authenticated by nothing in particular. Here the IR states exactly what the document said —oauth2, with no flows — so nothing is misstated and nothing must be refused. Refusing would also delete declared text (description,x-*,oauth2MetadataUrl) and collapse every requirement naming the scheme, for a document defect the IR can hold. The compiler already interns the two equally unusable siblings —apiKeywith noname,openIdConnectwith no URL — so wire-level usability is not the line it draws; the mechanism token is (fillSchemeKind), and that line is unchanged.Warning, not error. The document lowers whole, and the shape is the same class as
DisjointVisibility(exactly represented, unusable),EmptyEnum,ReservedHeaderName,InvalidMethodKeyandInvalidLocationKeyword. An error would also stopharness.Checkat the first error diagnostic and keep any fixture carrying the shape out ofirverify;--fail-on warningis the caller's lever.The metadata URL does not exempt the entry. The finding is that the flows object declares no flow, true whatever else the entry says, and the loader's requiredness rule is dialect-independent, firing for the absent spelling on 3.2 too. The URL is preserved on the scheme either way.
Where it lives.
compilers/openapi/internal/diag/diag.go:diag.OAuth2NoFlowwith its design record, immediately afterIncompleteSecurityScheme.compilers/openapi/internal/auth/auth.go:oauthNoFlowDiagbesidemechanismRefusalDiag, and the branch after thenamedguard inlowerSecurityScheme.fillSchemeKind's oauth2 case records that an absent or empty flows object lowers to nil flows, andLowerSecuritySchemes' doc now names both reporters of an entry that did intern.ir/type, noOptionsswitch, noirverifyrule: nothing is inferred, and holding flow fields inirverifywould re-validate the source.Test plan
go test ./compilers/openapi/internal/auth/ -run 'TestLowerSecuritySchemes|TestAuth_|TestSecurityRequirement'green.TestLowerSecuritySchemes_AnOAuth2SchemeWithNoFlowIsReportedpins the rule by spelling:flows: {}(the issue repro — one warning at/components/securitySchemes/s, severity warning, message names the scheme),flowsabsent (the same warning beside the loader's unpointed finding),flows: {application: …}(the new warning plus the existingunknown-object-keyat/components/securitySchemes/s/flows/application),flows: {}withoauth2MetadataUrl(the warning still fires and the URL is kept), one declared flow and one declared device flow (the false arm — no diagnostic with the code), and an entry with notype(onlyincomplete-security-scheme). Every interned row also pins that the requirement naming the scheme keeps a liveAuthID.TestLowerSecuritySchemes_AFlowlessRefEntryIsReportedAtEachEntry: a$refalias to a flowless declaration reports at both/components/securitySchemes/aliasand/components/securitySchemes/target, each naming its own entry — openapi: referenced non-schema components hoist once per use site under fabricated pointers #107's "an alias and its target are two named schemes".TestAuth_OAuthNoFlowsUnknownTypeAndGhostRefis extended with the placed-warning assertions; it was the seed's "pins today's silence" case.TestAuth_OAuthNoFlowsUnknownTypeAndGhostRef,TestLowerSecuritySchemes_AFlowlessRefEntryIsReportedAtEachEntryand all four reporting rows of the table test redden;ss.GetFlows() == nil) → theflows: {},{application: …}and metadata rows redden, plusTestLowerSecuritySchemes_ARefdEntryRecordsFieldsWhereTheyAreWritten;entry→ the pointer assertions redden;namedguard with noKindtest → the declared-flow rows redden, and the untyped-entry row too once the refusal's early return is made to carry the accumulator, since until then it discards what was appended.scripts/check-coverage.sh→Coverage gate passed: all 7599 statements covered.Both arms of the new branch are exercised: the declared-flow rows reach it false, the rest true.make gate(fmt, vet, lint, nolint-grammar, nolint, build, coverage-count, coverage, fuzz, bench-smoke) passes withgolangci-lintreporting 0 issues.testdata/that declarestype: oauth2declares a flow —golden/openapi/petstore.yaml(implicit),conformance/openapi/security-schemes.yaml(clientCredentials),conformance/openapi/extensions-x.yaml(implicit),openapi/unknown_keys.yaml(implicit) — so no golden, diagnostic list, conformance assertion or fixture changes, and notestdata/file is touched by this PR.Deliberately out of scope
tokenUrl,authorizationUrlorscopes): that is the loader's own finding, and not what "declares no flow" means.irverifyrule: holding flow fields there would re-validate the source, which its own doc rules out.securityScheme.flows is requiredfinding is left exactly as it is. This change adds a placed report beside it rather than replacing or suppressing it.Closes #646.