fix(compilers/openapi): recover from a resolver panic instead of faulting - #96
Merged
Merged
Conversation
…ting
unmarshal has guarded the third-party parser since the first crash fix, but
ResolveAllReferences was left bare. The resolver faults on shapes the parser
accepts, so a document that parsed cleanly could still take the caller's process
down:
openapi: 3.0
components:
responses:
000: {$ref: '#/B'}
B: {$ref}
`B: {$ref}` is a mapping whose $ref key carries no value. Populating the response
reference that points at it nil-dereferences inside speakeasy's Reference.Populate,
and the panic propagated out of Compile — breaking both the no-panics-escape rule
and the contract the compiler is built around, that a malformed spec yields
diagnostics rather than a fault.
resolveAll wraps the call the way unmarshal wraps the parser. The recovered panic
joins the resolve errors the caller already turns into diagnostics rather than
aborting the compile, because a document that trips this is a malformed spec, not
an I/O or programmer error: it is now refused with an unresolved-ref error, and
the process survives.
The reproducer came from FuzzCycleDetector and is committed as a corpus entry, so
a plain `go test` replays it.
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
unmarshalhas guarded the third-party parser since the first crash fix, butResolveAllReferenceswas left bare. The resolver faults on shapes the parser accepts, so a document that parsed cleanly
could still take the caller's process down:
B: {$ref}is a mapping whose$refkey carries no value. Populating the response reference thatpoints at it nil-dereferences inside speakeasy's
Reference.Populate(
openapi/reference.go:468), and the panic propagated out ofCompile. That breaks both thestyleguide's no-panics-escape rule and the contract this compiler is built around — a malformed spec
yields diagnostics, never a fault.
This is not a cycle-detector gap. The input is a response reference, a non-schema position the
pre-parse scan deliberately does not walk, and the failure is a nil dereference rather than the
unbounded recursion that scan exists to catch.
The change
resolveAllwraps the resolve call the wayunmarshalwraps the parser. The recovered panic joinsthe resolve errors the caller already converts into diagnostics rather than aborting the compile,
because a document that trips this is a malformed spec, not an I/O or programmer error. The
document is now refused with an
openapi/unresolved-referror and the process survives.How it was found
FuzzCycleDetector, at ~5m45s. It reproduces onmainat4e0c0ed, so it predates the referencework in #94/#95 — this is an independent gap, not a regression from either.
Worth reporting upstream: a
$refkey with a null value crashingReference.Populateis aspeakeasy bug, and the barrier here only stops it from being our crash.
Test plan
TestResolveAll_RecoversResolverPanicpins the barrier directly: the parser accepts the document,the resolver panics, and the panic comes back as an
errParse-wrapped error with no partiallypopulated result leaking.
TestCompile_ResolverPanicIsADiagnosticpins the end-to-end behavior: no Go error, anopenapi/unresolved-referror diagnostic, no fault.compilers/openapi/testdata/fuzz/FuzzCycleDetector/, so aplain
go testreplays it.gofmt -l .,go vet ./...,golangci-lint run(0 issues),go test ./..., and./scripts/check-coverage.sh(100.0% total, 100.0% every package) all pass.