From 8ebc0781f1a62743782d130b1e73fe7e21ed5c51 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 2 Aug 2026 06:41:52 +0300 Subject: [PATCH 1/3] fix(compilers/openapi): refuse a $ref resolving through itself The pre-parse cycle scan let two shapes reach speakeasy's resolver that hang the process rather than producing a diagnostic. Both are reachable from under a hundred bytes, and both are validation-clean: $ref is a legal Path Item field in 3.1 and a pointer that fails to resolve is a resolution problem, not a schema one, so a caller that validates first is not protected. A pointer that passes through the reference being resolved deadlocks. Reference.resolve holds that reference's own write lock across the pointer walk, and the walk read-locks every reference it traverses, so re-entering one waits on a lock its own goroutine holds. sync.RWMutex is not reentrant, and no concurrency is involved. The scan resolved the pointer and asked only where it landed, so it read paths: /a: {$ref: '#/paths/~1a/t'} as a dangling reference and let it through -- but the resolver never evaluates the last segment; it deadlocks walking the '#/paths/~1a' prefix. followRefChain now reports a three-valued verdict instead of (cyclic, leftComponents), which also retires the "only meaningful alongside cyclic=true" footgun. The split between them is what the resolver can see, not where the pointer points: its guard extends its reference chain only once a hop completes, so a closed cycle is caught there -- and the all-components spelling of one carries the better message, so those stay its to report. A hop that re-enters a reference never completes, so nothing upstream can report it, and the components spelling deadlocks exactly like the document-position one. Re-entrancy is therefore refused whatever the pointer names. Re-entrancy is a property of a pointer and the chain reading it, not of a node alone, so the safe-memo is narrowed: a chain is recorded terminating only when no hop passed through a reference. Without that the refusal depends on declaration order -- declaring the dangling reference first records it as safe and the later chain short-circuits past the re-entrant hop. Every hop in a real document passes through mappings like components and schemas and no $ref node at all, so the memo stays in force where it earns its keep. A pointer the scan read differently from the resolver stack-overflows. The resolver trims and percent-decodes the pointer half of a $ref (references/reference.go GetURI and GetJSONPointer), so '#/paths/~1a ' names /a there while a scan reading the raw value called it dangling. That self-reference left the resolution cache pointing at its own reference, and GetObject's delegation recursed until the stack was exhausted. Reading the pointer the way the resolver reads it is not a nicety here: any divergence is a reference the scan cannot see, and one trailing space was enough to be one. nodeview.InternalPointer now mirrors that normalization and PointerPath walks the normalized form, keeping every node it passes through -- a target alone cannot express a danger that is not at the destination. ResolvePointer had no callers left and is gone. Six reproducers are added and seeded into the fuzz corpus, with four negative controls pinning the shapes that must stay accepted -- each measured against the resolver first, since refusing one would be refusing a document that compiles. cycle_path_item_prefix_chain.yaml declares its paths in the order that hides the bug, which is the order the memo narrowing exists for. Both defects were found by FuzzCycleDetector, where they presented only as workers dying with "EOF": go test -fuzz wires worker stdio to /dev/null, so the worker's 10s watchdog panic is discarded and the input recorded as the crasher is whichever one the worker happened to hold, not the one at fault. Reported upstream as speakeasy-api/openapi#231, with a fix in speakeasy-api/openapi#230; the scan stays regardless, since a replace directive would not reach anyone consuming this module as a library. --- compilers/openapi/cycles_test.go | 36 +++++ .../openapi/internal/nodeview/nodeview.go | 78 +++++++-- .../nodeview/nodeview_internal_test.go | 109 +++++++++++-- compilers/openapi/internal/scan/scan.go | 150 ++++++++++++++---- .../internal/scan/scan_internal_test.go | 95 ++++++++++- internal/harness/corpus_test.go | 20 +++ .../cycle_component_path_item_prefix.yaml | 11 ++ .../openapi/cycle_path_item_prefix_chain.yaml | 13 ++ .../openapi/cycle_path_item_prefix_self.yaml | 8 + .../cycle_path_item_prefix_sibling.yaml | 10 ++ .../cycle_pointer_whitespace_self.yaml | 10 ++ .../openapi/cycle_webhook_prefix_self.yaml | 6 + 12 files changed, 483 insertions(+), 63 deletions(-) create mode 100644 testdata/openapi/cycle_component_path_item_prefix.yaml create mode 100644 testdata/openapi/cycle_path_item_prefix_chain.yaml create mode 100644 testdata/openapi/cycle_path_item_prefix_self.yaml create mode 100644 testdata/openapi/cycle_path_item_prefix_sibling.yaml create mode 100644 testdata/openapi/cycle_pointer_whitespace_self.yaml create mode 100644 testdata/openapi/cycle_webhook_prefix_self.yaml diff --git a/compilers/openapi/cycles_test.go b/compilers/openapi/cycles_test.go index 5ed9967..5600203 100644 --- a/compilers/openapi/cycles_test.go +++ b/compilers/openapi/cycles_test.go @@ -35,6 +35,15 @@ var cycleReproducers = []struct{ name, file string }{ {"webhook-mutual", "cycle_webhook_mutual"}, {"response-via-path", "cycle_response_via_path"}, {"path-item-via-component", "cycle_path_item_via_component"}, + // A reference whose pointer passes through a reference already being + // resolved. Distinct from the cycles above: the hop never completes, so + // speakeasy's own guard cannot see it and it deadlocks rather than faulting. + {"path-item-prefix-self", "cycle_path_item_prefix_self"}, + {"path-item-prefix-sibling", "cycle_path_item_prefix_sibling"}, + {"path-item-prefix-chain", "cycle_path_item_prefix_chain"}, + {"component-path-item-prefix", "cycle_component_path_item_prefix"}, + {"webhook-prefix-self", "cycle_webhook_prefix_self"}, + {"pointer-whitespace-self", "cycle_pointer_whitespace_self"}, } func TestCompile_CyclicSpecDoesNotCrash(t *testing.T) { @@ -249,3 +258,30 @@ func FuzzCycleDetector(f *testing.F) { []compilers.Source{{Path: "fuzz.yaml", Data: data}}, compilers.Options{}) }) } + +// TestCompile_ReentrantPrefixRefusedInBothDeclarationOrders pins what the +// safe-memo narrowing exists for. Whether a re-entrant hop is seen depends on +// which reference the walk reaches as a chain root first, so a single order +// proves nothing: with a memo keyed on the node alone, declaring the dangling +// reference first records it as terminating and the later chain short-circuits +// straight past the hop. Both orders deadlock in the resolver, so both must be +// refused. +func TestCompile_ReentrantPrefixRefusedInBothDeclarationOrders(t *testing.T) { + t.Parallel() + const head = "openapi: 3.1.0\ninfo: {title: t, version: '1'}\npaths:\n" + orders := []struct{ name, body string }{ + {"dangling-declared-first", " /b: {$ref: '#/paths/~1a/t'}\n /a: {$ref: '#/paths/~1b'}\n"}, + {"dangling-declared-last", " /a: {$ref: '#/paths/~1b'}\n /b: {$ref: '#/paths/~1a/t'}\n"}, + } + for _, tc := range orders { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + doc, diags, err := New().Compile(t.Context(), + []compilers.Source{{Path: "order.yaml", Data: []byte(head + tc.body)}}, + compilers.Options{}) + require.NoError(t, err, "a re-entrant reference is a spec problem, not a Go error") + assert.Nil(t, doc, "the compiler refuses a re-entrant reference") + assertHasErrorCode(t, diags, diag.CyclicRef) + }) + } +} diff --git a/compilers/openapi/internal/nodeview/nodeview.go b/compilers/openapi/internal/nodeview/nodeview.go index be2a237..dc1e066 100644 --- a/compilers/openapi/internal/nodeview/nodeview.go +++ b/compilers/openapi/internal/nodeview/nodeview.go @@ -9,6 +9,7 @@ package nodeview import ( + "net/url" "strconv" "strings" @@ -27,6 +28,12 @@ import ( // borrowed upward would invert the dependency. const maxAliasChain = 10000 +// maxPointerSegments bounds how many tokens one JSON pointer walk follows. A +// pointer names a position in the document, so a real one is a handful of +// segments deep; the bound is what keeps the walk terminating on a pointer built +// to be long rather than to name anything, per the bounded-recursion rule. +const maxPointerSegments = 1024 + // MergeDepthLimit bounds how deep a chain of `<<` merge keys the mapping view // expands. It is far tighter than maxAliasChain: each merge level // re-materializes every pair beneath it, so expanding a chain of depth d costs @@ -301,35 +308,84 @@ func (v *View) PureRefTarget(n *yaml.Node) (string, bool) { } // PureRefTargetOf is PureRefTarget over an already-expanded pair list, so a -// caller that needs both the pairs and the target expands the mapping once. +// caller that needs both the pairs and the target expands the mapping once. The +// target is normalized by InternalPointer, so it is a bare pointer ('/a/b'), +// not the '#/a/b' the source spells. func PureRefTargetOf(pairs []Pair) (string, bool) { for _, p := range pairs { if p.Key != "$ref" { continue } - if p.Val == nil || p.Val.Kind != yaml.ScalarNode || !strings.HasPrefix(p.Val.Value, "#/") { + if p.Val == nil || p.Val.Kind != yaml.ScalarNode { return "", false } - return p.Val.Value, true + return InternalPointer(p.Val.Value) } return "", false } -// ResolvePointer resolves an internal JSON pointer ('#/a/b') against the root -// node, returning the targeted node or nil when the path does not exist. Alias -// nodes along the path are dereferenced so navigation follows structure. -func (v *View) ResolvePointer(root *yaml.Node, ref string) *yaml.Node { +// InternalPointer reports the JSON pointer a $ref value names inside this +// document, and whether it names this document at all. +// +// It mirrors the resolver exactly: speakeasy splits a $ref on '#', treats what +// precedes it as a URI and what follows as the pointer, trims whitespace from +// both, and percent-decodes the pointer (references/reference.go GetURI and +// GetJSONPointer, v1.24.0). A ref whose URI half is empty names this document. +// +// Reading the raw value instead is not a near-enough approximation, it is a hole +// in the cycle scan: a pointer this package calls dangling but the resolver +// resolves is a reference the scan cannot see, and '#/paths/~1a ' — one trailing +// space — is enough to be one. A dependency bump should re-check those two +// methods, as MergeDepthLimit's comment does for the behavior it tracks. +func InternalPointer(ref string) (string, bool) { + parts := strings.Split(ref, "#") + if len(parts) < 2 || strings.TrimSpace(parts[0]) != "" { + return "", false // no fragment, or a fragment in another document + } + pointer := strings.TrimSpace(parts[1]) + if decoded, err := url.QueryUnescape(pointer); err == nil { + pointer = decoded + } + return pointer, true +} + +// PointerPath walks a normalized internal JSON pointer ('/a/b', as +// InternalPointer returns it) against the root node, keeping every node it +// passes through: element 0 is the root and each later element is the node +// reached by one more token. complete reports whether every token resolved; +// when it is false the walk stopped at the last element returned, and there is +// no destination. Alias nodes along the path are dereferenced so navigation +// follows structure. +// +// It yields the whole path rather than just the target because a pointer's +// danger is not always at its destination. speakeasy resolves a reference while +// holding that reference's own lock and read-locks every reference the pointer +// walk passes through, so a pointer that traverses a reference already being +// resolved deadlocks before it ever arrives (v1.24.0, openapi/reference.go +// resolve/GetObject). A target alone cannot express that. +func (v *View) PointerPath(root *yaml.Node, pointer string) (path []*yaml.Node, complete bool) { cur := Deref(root) - for raw := range strings.SplitSeq(strings.TrimPrefix(ref, "#"), "/") { + if cur == nil { + return nil, false + } + path = append(path, cur) + + segments := 0 + for raw := range strings.SplitSeq(pointer, "/") { if raw == "" { continue } - cur = v.ChildByToken(Deref(cur), ids.UnescapeSegment(raw)) + segments++ + if segments > maxPointerSegments { + return path, false + } + cur = Deref(v.ChildByToken(cur, ids.UnescapeSegment(raw))) if cur == nil { - return nil + return path, false } + path = append(path, cur) } - return Deref(cur) + return path, true } // ChildByToken returns the child of a mapping (by key) or sequence (by index) diff --git a/compilers/openapi/internal/nodeview/nodeview_internal_test.go b/compilers/openapi/internal/nodeview/nodeview_internal_test.go index 97b555f..de8affb 100644 --- a/compilers/openapi/internal/nodeview/nodeview_internal_test.go +++ b/compilers/openapi/internal/nodeview/nodeview_internal_test.go @@ -1,6 +1,7 @@ package nodeview import ( + "strings" "testing" "github.com/stretchr/testify/assert" @@ -59,7 +60,7 @@ func TestChildByToken_NilNode(t *testing.T) { assert.Nil(t, New().ChildByToken(nil, "anything")) } -func TestResolvePointer_Cases(t *testing.T) { +func TestPointerPath_Cases(t *testing.T) { t.Parallel() leaf := yscalar("leaf") target := ymap(yscalar("b"), leaf) @@ -70,27 +71,54 @@ func TestResolvePointer_Cases(t *testing.T) { yscalar("c~d"), yscalar("tilde"), ) tests := []struct { - name string - ref string - want *yaml.Node + name string + pointer string + want *yaml.Node }{ - {"sequence index in range", "#/arr/1", root.Content[1].Content[1]}, - {"sequence index out of range", "#/arr/9", nil}, - {"sequence index non-numeric", "#/arr/x", nil}, - {"alias dereferenced along path", "#/via/b", leaf}, - {"escaped slash token", "#/a~1b", root.Content[5]}, - {"escaped tilde token", "#/c~0d", root.Content[7]}, - {"missing key", "#/nope", nil}, + {"sequence index in range", "/arr/1", root.Content[1].Content[1]}, + {"sequence index out of range", "/arr/9", nil}, + {"sequence index non-numeric", "/arr/x", nil}, + {"alias dereferenced along path", "/via/b", leaf}, + {"escaped slash token", "/a~1b", root.Content[5]}, + {"escaped tilde token", "/c~0d", root.Content[7]}, + {"missing key", "/nope", nil}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - got := New().ResolvePointer(root, tc.ref) + path, complete := New().PointerPath(root, tc.pointer) if tc.want == nil { - assert.Nil(t, got) + assert.False(t, complete, "the pointer names nothing") return } - assert.Same(t, tc.want, got) + require.True(t, complete) + assert.Same(t, tc.want, path[len(path)-1], "the last element is the destination") + }) + } +} + +func TestInternalPointer_MatchesTheResolversNormalization(t *testing.T) { + t.Parallel() + tests := []struct { + name, ref, want string + internal bool + }{ + {name: "plain", ref: "#/components/schemas/A", want: "/components/schemas/A", internal: true}, + {name: "trailing space", ref: "#/paths/~1a ", want: "/paths/~1a", internal: true}, + {name: "leading space", ref: " #/paths/~1a", want: "/paths/~1a", internal: true}, + {name: "percent-decoded", ref: "#/paths/%7E1a", want: "/paths/~1a", internal: true}, + {name: "second hash ends the pointer", ref: "#/a#b", want: "/a", internal: true}, + {name: "bare hash names the root", ref: "#", want: "", internal: true}, + {name: "undecodable escape kept raw", ref: "#/a%zz", want: "/a%zz", internal: true}, + {name: "no fragment", ref: "other.yaml", internal: false}, + {name: "another document", ref: "other.yaml#/components/schemas/A", internal: false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, internal := InternalPointer(tc.ref) + assert.Equal(t, tc.internal, internal) + assert.Equal(t, tc.want, got) }) } } @@ -200,7 +228,7 @@ func TestPureRefTarget_Cases(t *testing.T) { want string }{ {"sibling key before the ref", ymap(yscalar("type"), yscalar("object"), - yscalar("$ref"), yscalar("#/components/schemas/A")), "#/components/schemas/A"}, + yscalar("$ref"), yscalar("#/components/schemas/A")), "/components/schemas/A"}, {"external ref is not internal", ymap(yscalar("$ref"), yscalar("other.yaml#/A")), ""}, {"non-scalar ref value", ymap(yscalar("$ref"), ymap(yscalar("a"), yscalar("b"))), ""}, {"nil ref value via broken alias", ymap(yscalar("$ref"), yalias(nil)), ""}, @@ -426,3 +454,54 @@ func yamlDoc(t *testing.T, src string) *yaml.Node { require.NoError(t, yaml.Unmarshal([]byte(src), &doc)) return DocumentRoot(&doc) } + +func TestPointerPath_KeepsTheNodesTheWalkPassesThrough(t *testing.T) { + t.Parallel() + leaf := yscalar("leaf") + inner := ymap(yscalar("b"), leaf) + root := ymap(yscalar("a"), inner) + + path, complete := New().PointerPath(root, "/a/b") + require.True(t, complete, "every token resolves") + assert.Equal(t, []*yaml.Node{root, inner, leaf}, path, + "element 0 is the root and each later element is one more token") +} + +func TestPointerPath_IncompleteStopsAtTheLastNodeReached(t *testing.T) { + t.Parallel() + inner := ymap(yscalar("b"), yscalar("leaf")) + root := ymap(yscalar("a"), inner) + + path, complete := New().PointerPath(root, "/a/missing/deeper") + assert.False(t, complete, "a token that names nothing stops the walk") + assert.Equal(t, []*yaml.Node{root, inner}, path, + "an unresolvable pointer has no destination, so every node it reached is one it passed through") +} + +func TestPointerPath_RootTokenlessAndNil(t *testing.T) { + t.Parallel() + root := ymap(yscalar("a"), yscalar("v")) + + path, complete := New().PointerPath(root, "") + assert.True(t, complete, "a pointer with no tokens names the root") + assert.Equal(t, []*yaml.Node{root}, path) + + path, complete = New().PointerPath(nil, "/a") + assert.False(t, complete) + assert.Nil(t, path, "a nil root reaches nothing") +} + +func TestPointerPath_SegmentCapStopsTheWalk(t *testing.T) { + t.Parallel() + // A mapping whose only key is "a" and whose value is itself cannot be built + // from parsed YAML, but an alias can stand in: the walk follows "a" as long + // as tokens last, so only the cap can end it. + root := ymap(yscalar("a"), nil) + root.Content[1] = root + + ref := strings.Repeat("/a", maxPointerSegments+1) + path, complete := New().PointerPath(root, ref) + assert.False(t, complete, "a pointer past the segment cap does not resolve") + assert.Len(t, path, maxPointerSegments+1, + "the walk stops at the cap: the root plus one node per followed token") +} diff --git a/compilers/openapi/internal/scan/scan.go b/compilers/openapi/internal/scan/scan.go index ad5afb6..9a7c4a1 100644 --- a/compilers/openapi/internal/scan/scan.go +++ b/compilers/openapi/internal/scan/scan.go @@ -176,7 +176,7 @@ func refCycles(srcIndex int, root *yaml.Node) []ir.Diagnostic { s := newRefScan() s.collect(root) for _, start := range s.out { - if cyclic, _ := s.followRefChain(root, start); cyclic { + if verdict, _ := s.followRefChain(root, start); verdict == chainCycles { return []ir.Diagnostic{cyclicDiag(srcIndex, start, "cyclic $ref: reference chain never reaches a node without a $ref")} } @@ -194,22 +194,33 @@ func refCycles(srcIndex int, root *yaml.Node) []ir.Diagnostic { return nil } -// outsideCycle reports the first $ref cycle among the reference objects that -// live outside any schema — a path item, a response, a parameter — but only -// when the chain leaves the components section on some hop. +// outsideCycle reports the first degenerate chain among the reference objects +// that live outside any schema — a path item, a response, a parameter. Two rules +// apply, and they are split on what speakeasy's resolver can see rather than on +// where the pointer points. // -// The split is not cosmetic. speakeasy's resolver refuses a cycle whose every -// hop names a component ('#/components/responses/A' -> '.../B' -> '.../A') with -// its own diagnostic, and that message names the chain, so it is the better one -// to keep. It has no such guard for a hop that names a node by document -// position ('#/paths/~1a', '#/webhooks/onA', '#/paths/~1a/get/responses/200'): -// resolving one recurses until the stack overflows and takes the process with -// it. Those are the chains this must refuse before soa.Unmarshal ever sees them, -// and refusing only those leaves the resolver's coverage exactly as it was. +// Its cycle guard tracks *completed* hops: resolveObjectWithTracking appends a +// reference to its chain only after Reference.resolve returns, then compares the +// next one against that chain. So a cycle whose every hop resolves to a whole +// node is caught there, and for the all-components spelling +// ('#/components/responses/A' -> '.../B' -> '.../A') its message names the chain +// and is the better one to keep. A hop that names a node by document position +// ('#/paths/~1a', '#/webhooks/onA') is not caught, so chainCycles is refused +// here once the chain has left components. +// +// A hop that passes *through* a reference never completes at all: the pointer +// walk read-locks a reference whose resolve already holds its write lock, and +// the process deadlocks before the tracker is consulted. Nothing upstream can +// report that, and the components spelling deadlocks exactly like the +// document-position one, so chainReenters is refused whatever it names. func (s *refScan) outsideCycle(srcIndex int, root *yaml.Node) (ir.Diagnostic, bool) { for _, start := range s.outside { - cyclic, leftComponents := s.followRefChain(root, start) - if cyclic && leftComponents { + verdict, leftComponents := s.followRefChain(root, start) + switch { + case verdict == chainReenters: + return cyclicDiag(srcIndex, start, + "cyclic $ref: reference resolves through itself"), true + case verdict == chainCycles && leftComponents: return cyclicDiag(srcIndex, start, "cyclic $ref: reference chain never reaches a node without a $ref"), true } @@ -219,10 +230,38 @@ func (s *refScan) outsideCycle(srcIndex int, root *yaml.Node) (ir.Diagnostic, bo // componentsRef reports whether a same-document $ref names a node in the // components section, the only shape speakeasy's resolver refuses on its own. -func componentsRef(ref string) bool { - return strings.HasPrefix(ref, "#/components/") +// The pointer is the normalized one nodeview.InternalPointer returns, so it +// carries no leading '#'. +func componentsRef(pointer string) bool { + return strings.HasPrefix(pointer, "/components/") } +// chainVerdict is how following a pure-$ref chain ends. The two failing cases +// are kept apart because the resolver treats them differently, not for +// description's sake: only one of them is a shape speakeasy can report itself. +type chainVerdict int + +const ( + // chainTerminates: the chain reached a node with no top-level $ref, or a + // pointer that names nothing. The resolver stops either way. + chainTerminates chainVerdict = iota + + // chainCycles: every hop resolved to a whole node, and the chain revisited + // one already on it. Each hop completes, so speakeasy extends its own + // reference chain and its cycle check sees the loop. + chainCycles + + // chainReenters: a hop's pointer passes *through* a node already on the + // chain. speakeasy cannot see this one. Reference.resolve holds that + // reference's write lock across the pointer walk, and the walk read-locks + // every reference it passes through, so re-entering one self-deadlocks on a + // non-reentrant RWMutex — inside a hop that never completes, which is why + // the resolver's own cycle check never runs (v1.24.0, openapi/reference.go + // resolve at :537 and GetObject at :293). Refused whatever the pointer's + // spelling: unlike chainCycles, a components-only chain deadlocks too. + chainReenters +) + // walkRole is how the ref-collection walk reads the node it is visiting. The // same node can legally occupy more than one role — an anchored pure-$ref // mapping aliased once into a "properties" position and once used directly as a @@ -419,44 +458,95 @@ func (s *refScan) visitSchemaList(n *yaml.Node) { // stays linear in the number of collected refs instead of re-walking shared // tails. A node on a cycle is never marked safe, so memoization can't hide a // real cycle. -func (s *refScan) followRefChain(root, start *yaml.Node) (cyclic, leftComponents bool) { +func (s *refScan) followRefChain(root, start *yaml.Node) (chainVerdict, bool) { onPath := make(map[*yaml.Node]bool) var path []*yaml.Node + leftComponents, memoizable := false, true cur := start + for depth := 0; depth <= maxCycleDepth; depth++ { if s.safe[cur] { - markSafe(path, s.safe) - return false, leftComponents // cur already proved chain-terminating — no cycle + s.markSafe(path, memoizable) + return chainTerminates, leftComponents // cur already proved chain-terminating } if onPath[cur] { - return true, leftComponents // revisited a node on this chain — cyclic + return chainCycles, leftComponents // revisited a node on this chain } ref, ok := s.view.PureRefTarget(cur) if !ok { s.safe[cur] = true - markSafe(path, s.safe) - return false, leftComponents // reached a node without a top-level $ref — legal recursion + s.markSafe(path, memoizable) + return chainTerminates, leftComponents // no top-level $ref — legal recursion } if !componentsRef(ref) { leftComponents = true } onPath[cur] = true path = append(path, cur) - next := s.view.ResolvePointer(root, ref) + + next, reenters, viaRef := s.traverse(root, ref, onPath) + if reenters { + return chainReenters, leftComponents + } + if viaRef { + memoizable = false + } if next == nil { - markSafe(path, s.safe) - return false, leftComponents // dangling ref — reported downstream as unresolved + s.markSafe(path, memoizable) + return chainTerminates, leftComponents // dangling ref — unresolved downstream } cur = next } - return false, leftComponents // depth cap reached without a verdict — mark nothing + return chainTerminates, leftComponents // depth cap reached without a verdict } -// markSafe records every node on a proven chain-terminating path so a later chain -// that reaches one stops immediately instead of re-walking it. -func markSafe(path []*yaml.Node, safe map[*yaml.Node]bool) { +// traverse follows one pointer from the chain position carrying it and reports +// where it lands (nil when it names nothing), whether it passed through a node +// already on the chain, and whether any node it passed through is itself a +// reference. +// +// The destination is excluded from "passed through": arriving at an on-chain +// node is chainCycles, which speakeasy reports itself. A pointer that does not +// resolve has no destination, so every node it reached counts — that is the case +// the old dangling-ref branch called harmless, and the one that hangs. +func (s *refScan) traverse(root *yaml.Node, ref string, onPath map[*yaml.Node]bool) (dest *yaml.Node, reenters, viaRef bool) { + hop, complete := s.view.PointerPath(root, ref) + through := hop + if complete { + through = hop[:len(hop)-1] + } + for _, n := range through { + if onPath[n] { + return nil, true, viaRef + } + if _, isRef := s.view.PureRefTarget(n); isRef { + viaRef = true + } + } + if !complete { + return nil, false, viaRef + } + return hop[len(hop)-1], false, viaRef +} + +// markSafe records every node on a proven chain-terminating path so a later +// chain that reaches one stops immediately instead of re-walking it. +// +// It declines when a hop passed through a reference. Re-entrancy is a property +// of a pointer and the chain reading it, not of a node alone, so a chain proved +// terminating from one start says nothing about a chain that reaches it by +// another route — memoizing it there would make the refusal depend on which +// declaration order the walk happened to take. A hop that passes through no +// reference can never re-enter one whichever chain follows it, which is every +// hop in a real document: pointers pass through mappings like `components` and +// `schemas`, never through a $ref node. So the memo stays in force exactly where +// it earns its keep, and lapses only on the shapes it cannot answer for. +func (s *refScan) markSafe(path []*yaml.Node, memoizable bool) { + if !memoizable { + return + } for _, n := range path { - safe[n] = true + s.safe[n] = true } } diff --git a/compilers/openapi/internal/scan/scan_internal_test.go b/compilers/openapi/internal/scan/scan_internal_test.go index 7f654c8..25a43c5 100644 --- a/compilers/openapi/internal/scan/scan_internal_test.go +++ b/compilers/openapi/internal/scan/scan_internal_test.go @@ -37,6 +37,15 @@ var cycleReproducers = []struct{ name, file string }{ {"webhook-mutual", "cycle_webhook_mutual"}, {"response-via-path", "cycle_response_via_path"}, {"path-item-via-component", "cycle_path_item_via_component"}, + // A reference whose pointer passes through a reference already being + // resolved. Distinct from the cycles above: the hop never completes, so + // speakeasy's own guard cannot see it and it deadlocks rather than faulting. + {"path-item-prefix-self", "cycle_path_item_prefix_self"}, + {"path-item-prefix-sibling", "cycle_path_item_prefix_sibling"}, + {"path-item-prefix-chain", "cycle_path_item_prefix_chain"}, + {"component-path-item-prefix", "cycle_component_path_item_prefix"}, + {"webhook-prefix-self", "cycle_webhook_prefix_self"}, + {"pointer-whitespace-self", "cycle_pointer_whitespace_self"}, } func TestDetectCycles_Reproducers(t *testing.T) { @@ -144,6 +153,39 @@ components: schemas: A: {$ref: *r} B: {type: object} +`}, + // The four cases below are the negative controls for the re-entrant-prefix + // refusal. Each carries a pointer whose prefix names a reference, which is + // the shape that rule keys on — but not one on the chain reading it, so the + // resolver handles them and reports an unresolved ref. Each was measured + // against speakeasy v1.24.0 before being written down here: refusing any of + // them would be refusing a document that compiles. + {"legal-prefix-both-hops-dangle", `openapi: 3.1.0 +info: {title: t, version: '1'} +paths: + /a: {$ref: '#/paths/~1b/t'} + /b: {$ref: '#/paths/~1a/t'} +`}, + {"legal-prefix-names-offchain-ref", `openapi: 3.1.0 +info: {title: t, version: '1'} +paths: + /a: {$ref: '#/paths/~1b/t'} + /b: {get: {operationId: b, responses: {"200": {description: ok}}}} +`}, + {"legal-schema-prefix-self", `openapi: 3.1.0 +info: {title: t, version: '1'} +paths: {} +components: + schemas: + A: {$ref: '#/components/schemas/A/properties', properties: {p: {type: string}}} +`}, + {"legal-schema-prefix-into-path-item", `openapi: 3.1.0 +info: {title: t, version: '1'} +paths: + /a: {$ref: '#/components/schemas/S/t'} +components: + schemas: + S: {$ref: '#/paths/~1a'} `}, } @@ -270,8 +312,8 @@ func TestFollowRefChain_DepthCapReturnsFalse(t *testing.T) { nodes[i].Content = []*yaml.Node{yscalar("$ref"), yscalar("#/schemas/" + strconv.Itoa(i+1))} } } - cyclic, _ := newRefScan().followRefChain(root, nodes[0]) - assert.False(t, cyclic, + verdict, _ := newRefScan().followRefChain(root, nodes[0]) + assert.Equal(t, chainTerminates, verdict, "a chain longer than the depth cap exits without flagging a cycle") } @@ -282,13 +324,13 @@ func TestFollowRefChain_SafeMemoShortCircuits(t *testing.T) { schemas := ymap(yscalar("A"), a, yscalar("B"), b) root := ymap(yscalar("schemas"), schemas) - cyclic, _ := newRefScan().followRefChain(root, a) - assert.True(t, cyclic, "A -> B -> A is cyclic with an empty memo") + verdict, _ := newRefScan().followRefChain(root, a) + assert.Equal(t, chainCycles, verdict, "A -> B -> A is cyclic with an empty memo") s := newRefScan() s.safe[b] = true memoed, _ := s.followRefChain(root, a) - assert.False(t, memoed, "a chain reaching a memoized-safe node is not a cycle") + assert.Equal(t, chainTerminates, memoed, "a chain reaching a memoized-safe node is not a cycle") assert.True(t, s.safe[a], "the walk records the reaching node as terminating too") } @@ -297,8 +339,8 @@ func TestFollowRefChain_DanglingRefIsNotCycle(t *testing.T) { a := ymap(yscalar("$ref"), yscalar("#/schemas/Missing")) root := ymap(yscalar("schemas"), ymap(yscalar("A"), a)) s := newRefScan() - cyclic, _ := s.followRefChain(root, a) - assert.False(t, cyclic, "a dangling $ref is not a cycle") + verdict, _ := s.followRefChain(root, a) + assert.Equal(t, chainTerminates, verdict, "a dangling $ref is not a cycle") assert.True(t, s.safe[a], "the dangling node is recorded terminating") } @@ -659,3 +701,42 @@ func TestRefScanCollect_OutsidePositions(t *testing.T) { assert.False(t, s.seen[roleOutside][notASchema], "nor walked as an outside position") } + +func TestDetectCycles_PointerThroughOwnReferenceIsRefused(t *testing.T) { + t.Parallel() + const src = `openapi: 3.1.0 +info: {title: t, version: '1'} +paths: + /a: {$ref: '#/paths/~1a/t'} +` + diags := Cycles(0, []byte(src)) + require.NotEmpty(t, diags, "a pointer that resolves through its own reference must be refused") + assert.Equal(t, diag.CyclicRef, diags[0].Code) + assert.Equal(t, ir.SeverityError, diags[0].Severity) +} + +// TestDetectCycles_PointerIsNormalizedLikeTheResolver pins the scan's pointer +// reading to speakeasy's. It splits a $ref on '#', trims whitespace from both +// halves and percent-decodes the pointer (references/reference.go GetURI and +// GetJSONPointer, v1.24.0), so '#/paths/~1a ' names /a there. A scan that reads +// the raw value instead calls that pointer dangling and lets a self-reference +// through — one trailing space was enough to walk past the refusal. +func TestDetectCycles_PointerIsNormalizedLikeTheResolver(t *testing.T) { + t.Parallel() + const head = "openapi: 3.1.0\ninfo: {title: t, version: '1'}\npaths:\n /a:\n $ref: " + const tail = "\n get: {operationId: a, responses: {\"200\": {description: ok}}}\n" + + refs := map[string]string{ + "trailing space": "'#/paths/~1a '", + "leading space": "' #/paths/~1a'", + "percent-encoded": "'#/paths/%7E1a'", + } + for name, ref := range refs { + t.Run(name, func(t *testing.T) { + t.Parallel() + diags := Cycles(0, []byte(head+ref+tail)) + require.NotEmpty(t, diags, "the resolver reads this pointer as naming /a") + assert.Equal(t, diag.CyclicRef, diags[0].Code) + }) + } +} diff --git a/internal/harness/corpus_test.go b/internal/harness/corpus_test.go index 335f4eb..832ada0 100644 --- a/internal/harness/corpus_test.go +++ b/internal/harness/corpus_test.go @@ -45,6 +45,20 @@ import ( // document position ('#/paths/~1a', '#/webhooks/onA') rather than through // components. Speakeasy guards the components spelling and faults on these, // so the pre-parse scan refuses them too. +// - cycle_path_item_prefix_self.yaml, cycle_path_item_prefix_sibling.yaml, +// cycle_path_item_prefix_chain.yaml, cycle_component_path_item_prefix.yaml, +// and cycle_webhook_prefix_self.yaml: a $ref whose pointer passes *through* +// a reference already being resolved. Speakeasy resolves a reference while +// holding its own write lock and read-locks every reference the pointer walk +// traverses, so re-entering one deadlocks the process on a non-reentrant +// RWMutex — and inside a hop that never completes, so its own cycle guard +// never runs. Unlike the cycles above, the components spelling deadlocks +// too, so all spellings are refused. +// - cycle_pointer_whitespace_self.yaml: the same self-reference, visible only +// once the pointer is normalized the way the resolver normalizes it. +// Speakeasy trims whitespace around the pointer half of a $ref, so +// '#/paths/~1a ' names /a there; a scan reading the raw value called it +// dangling and let a stack-overflowing self-reference through. // - amplification_alias_bomb.yaml: a 10-level x 10-way YAML alias fan-out // ("billion laughs"). Every alias's target is acyclic, so neither the // anchor nor $ref cycle detector catches it, and unguarded it exhausts @@ -90,6 +104,12 @@ func knownInvalid() map[string]bool { filepath.FromSlash("../../testdata/openapi/cycle_webhook_mutual.yaml"): true, filepath.FromSlash("../../testdata/openapi/cycle_response_via_path.yaml"): true, filepath.FromSlash("../../testdata/openapi/cycle_path_item_via_component.yaml"): true, + filepath.FromSlash("../../testdata/openapi/cycle_path_item_prefix_self.yaml"): true, + filepath.FromSlash("../../testdata/openapi/cycle_path_item_prefix_sibling.yaml"): true, + filepath.FromSlash("../../testdata/openapi/cycle_path_item_prefix_chain.yaml"): true, + filepath.FromSlash("../../testdata/openapi/cycle_component_path_item_prefix.yaml"): true, + filepath.FromSlash("../../testdata/openapi/cycle_webhook_prefix_self.yaml"): true, + filepath.FromSlash("../../testdata/openapi/cycle_pointer_whitespace_self.yaml"): true, filepath.FromSlash("../../testdata/openapi/amplification_alias_bomb.yaml"): true, filepath.FromSlash("../../testdata/dangling/openapi/f04-composition.yaml"): true, filepath.FromSlash("../../testdata/dangling/openapi/f05-discriminator.yaml"): true, diff --git a/testdata/openapi/cycle_component_path_item_prefix.yaml b/testdata/openapi/cycle_component_path_item_prefix.yaml new file mode 100644 index 0000000..ed392df --- /dev/null +++ b/testdata/openapi/cycle_component_path_item_prefix.yaml @@ -0,0 +1,11 @@ +# The components spelling of the re-entrant prefix. speakeasy reports a +# components-only cycle itself when every hop completes, which is why those are +# left to it — but a hop that re-enters a reference mid-resolve never completes, +# so this deadlocks exactly like the /paths spelling and is refused here. +openapi: 3.1.0 +info: {title: Component Path Item Prefix, version: "1"} +paths: + /a: {$ref: '#/components/pathItems/A'} +components: + pathItems: + A: {$ref: '#/components/pathItems/A/t'} diff --git a/testdata/openapi/cycle_path_item_prefix_chain.yaml b/testdata/openapi/cycle_path_item_prefix_chain.yaml new file mode 100644 index 0000000..f3f65c6 --- /dev/null +++ b/testdata/openapi/cycle_path_item_prefix_chain.yaml @@ -0,0 +1,13 @@ +# The re-entrant prefix reached on the second hop rather than the first: /b's +# pointer passes through /a, which is on the chain by then. +# +# The declaration order is load-bearing. Declaring /b first is what makes the +# walk reach it as a chain root before /a is ever on a chain, so a safe-memo +# keyed on the node alone records /b as terminating and the later /a -> /b walk +# short-circuits past the re-entrant hop. Both orders deadlock in the resolver, +# so both must be refused; this is the order that hides it. +openapi: 3.1.0 +info: {title: Path Item Prefix Chain, version: "1"} +paths: + /b: {$ref: '#/paths/~1a/t'} + /a: {$ref: '#/paths/~1b'} diff --git a/testdata/openapi/cycle_path_item_prefix_self.yaml b/testdata/openapi/cycle_path_item_prefix_self.yaml new file mode 100644 index 0000000..22a3baa --- /dev/null +++ b/testdata/openapi/cycle_path_item_prefix_self.yaml @@ -0,0 +1,8 @@ +# A path item whose $ref names a position *inside itself*. The pointer does not +# resolve — there is no `t` key — so the chain looks dangling, but the resolver +# never evaluates the last segment: it deadlocks walking the prefix +# '#/paths/~1a', which is the reference it is already resolving. +openapi: 3.1.0 +info: {title: Path Item Prefix Self, version: "1"} +paths: + /a: {$ref: '#/paths/~1a/t'} diff --git a/testdata/openapi/cycle_path_item_prefix_sibling.yaml b/testdata/openapi/cycle_path_item_prefix_sibling.yaml new file mode 100644 index 0000000..4cfb31a --- /dev/null +++ b/testdata/openapi/cycle_path_item_prefix_sibling.yaml @@ -0,0 +1,10 @@ +# The same re-entrant prefix, but with a sibling that makes the pointer resolve +# cleanly. A scan that only questioned pointers which fail to resolve would let +# this one through; the resolver still deadlocks on the '#/paths/~1a' prefix +# before it ever reaches the `get` the pointer names. +openapi: 3.1.0 +info: {title: Path Item Prefix Sibling, version: "1"} +paths: + /a: + $ref: '#/paths/~1a/get' + get: {operationId: a, responses: {"200": {description: ok}}} diff --git a/testdata/openapi/cycle_pointer_whitespace_self.yaml b/testdata/openapi/cycle_pointer_whitespace_self.yaml new file mode 100644 index 0000000..956d744 --- /dev/null +++ b/testdata/openapi/cycle_pointer_whitespace_self.yaml @@ -0,0 +1,10 @@ +# A self-reference that only the resolver's pointer normalization reveals. +# speakeasy trims whitespace around the pointer half of a $ref +# (references/reference.go GetJSONPointer), so '#/paths/~1a ' names /a there +# while a scan reading the raw value calls it dangling and lets it through. +openapi: 3.1.0 +info: {title: Pointer Whitespace Self, version: "1"} +paths: + /a: + $ref: '#/paths/~1a ' + get: {operationId: a, responses: {"200": {description: ok}}} diff --git a/testdata/openapi/cycle_webhook_prefix_self.yaml b/testdata/openapi/cycle_webhook_prefix_self.yaml new file mode 100644 index 0000000..db9510d --- /dev/null +++ b/testdata/openapi/cycle_webhook_prefix_self.yaml @@ -0,0 +1,6 @@ +# The webhooks arm of the re-entrant prefix. +openapi: 3.1.0 +info: {title: Webhook Prefix Self, version: "1"} +paths: {} +webhooks: + onA: {$ref: '#/webhooks/onA/t'} From a3caef0bc6b7bf7bf100bdc8ca78f04321e11448 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 2 Aug 2026 07:03:29 +0300 Subject: [PATCH 2/3] fix(compilers/openapi): correct a control that did not test its name Review follow-ups on the re-entrant-reference refusal. legal-prefix-names-offchain-ref claimed to cover a pointer whose prefix names a reference off the chain reading it, but its /b carried a `get` and so was not a reference at all. The fixture exercised a weaker shape than its name, and a mutation that refused on any reference prefix -- the natural way to get this rule too wide -- passed it. /b now references a component path item, which is what the name always said, and the mutation reddens it. Renamed to legal-prefix-through-offchain-ref, since what matters is the traversal, not the naming. legal-schema-prefix-into-path-item read backwards: the chain it covers starts at the schema and re-enters it through a path item, not the other way round. Renamed to legal-schema-chain-reentry, and the block comment now says why each control is safe -- two pass through a reference that is not on the chain, two re-enter from a schema position, which resolves through jsonschema rather than the openapi Reference wrapper and never takes the lock. The reproducer list described cycle_pointer_whitespace_self as a deadlock under the shared re-entrancy comment. It is a stack overflow, and its cause is the pointer normalization rather than a re-entrant hop, so it carries its own note. The package comment, Cycles, refCycles and diag.CyclicRef all described the refusals as they were before this change. Each now covers the re-entrant one, and the counts ("two refusals", "three classes") are gone rather than incremented -- a maintained count is one more thing to get wrong, and neither comment needed the number. --- compilers/openapi/cycles_test.go | 3 ++ compilers/openapi/internal/diag/diag.go | 8 ++-- compilers/openapi/internal/scan/scan.go | 43 +++++++++++-------- .../internal/scan/scan_internal_test.go | 31 +++++++++---- 4 files changed, 55 insertions(+), 30 deletions(-) diff --git a/compilers/openapi/cycles_test.go b/compilers/openapi/cycles_test.go index 5600203..4a18066 100644 --- a/compilers/openapi/cycles_test.go +++ b/compilers/openapi/cycles_test.go @@ -43,6 +43,9 @@ var cycleReproducers = []struct{ name, file string }{ {"path-item-prefix-chain", "cycle_path_item_prefix_chain"}, {"component-path-item-prefix", "cycle_component_path_item_prefix"}, {"webhook-prefix-self", "cycle_webhook_prefix_self"}, + // A self-reference only the resolver's pointer normalization reveals, which + // overflows the stack rather than deadlocking: the resolution cache ends up + // pointing at its own reference and GetObject's delegation recurses. {"pointer-whitespace-self", "cycle_pointer_whitespace_self"}, } diff --git a/compilers/openapi/internal/diag/diag.go b/compilers/openapi/internal/diag/diag.go index cd7d9ed..fd54124 100644 --- a/compilers/openapi/internal/diag/diag.go +++ b/compilers/openapi/internal/diag/diag.go @@ -26,9 +26,11 @@ const ( UnsupportedVersion = "openapi/unsupported-version" // UnresolvedRef reports a $ref that could not be resolved. UnresolvedRef = "openapi/unresolved-ref" - // CyclicRef reports a degenerate reference cycle — a recursive YAML anchor or - // a chain of $ref-only schemas that never reaches a concrete type — caught - // before it can crash the parser with a stack overflow. + // CyclicRef reports a degenerate reference cycle — a recursive YAML anchor, a + // chain of $ref-only schemas that never reaches a concrete type, or a + // reference whose pointer resolves through a reference already being resolved + // — caught before it can crash the parser with a stack overflow or deadlock + // the resolver on a lock its own goroutine holds. CyclicRef = "openapi/cyclic-ref" // CycleScanFailed reports that the pre-parse cycle scan did not run to // completion — either it aborted (a detector bug) or the document exceeded one diff --git a/compilers/openapi/internal/scan/scan.go b/compilers/openapi/internal/scan/scan.go index 9a7c4a1..61caa44 100644 --- a/compilers/openapi/internal/scan/scan.go +++ b/compilers/openapi/internal/scan/scan.go @@ -1,10 +1,12 @@ // Package scan refuses a source document before any of it is lowered. // -// Two refusals share a phase and a subject: a reference cycle that never reaches -// a concrete schema, which would recurse without bound inside the resolver, and a -// YAML alias fan-out that expands to far more nodes than the document declares, -// which would exhaust memory inside the parser. Both read the raw text through -// nodeview, and both run before the document is handed to either. +// The refusals share a phase and a subject. A reference cycle that never reaches +// a concrete schema recurses without bound inside the resolver. A reference whose +// pointer passes through a reference already being resolved deadlocks it, since +// the resolver holds that reference's own lock across the pointer walk. A YAML +// alias fan-out that expands to far more nodes than the document declares +// exhausts memory inside the parser. Each reads the raw text through nodeview, +// and each runs before the document is handed to either. package scan import ( @@ -56,15 +58,16 @@ var schemaDataKeys = map[string]bool{ "const": true, "enum": true, } -// Cycles scans raw source bytes for degenerate reference structures that -// would otherwise crash or exhaust memory in the third-party parser (GitHub -// #12, GitHub #27), before soa.Unmarshal ever runs. It reports three classes as -// error diagnostics: a recursive YAML anchor, a pure-$ref cycle (a chain of -// schema $refs that never reaches a node without one), and alias amplification -// (a billion-laughs expansion). A source that doesn't decode as YAML yields no -// cycles — the main parser reports that as a parse problem — and the scan runs -// under recoverCycleScan so a detector bug degrades to "no cycle found" rather -// than aborting. +// Cycles scans raw source bytes for degenerate reference structures that would +// otherwise crash, hang or exhaust memory in the third-party parser and resolver +// (GitHub #12, GitHub #27, speakeasy-api/openapi#231), before soa.Unmarshal ever +// runs. It reports as error diagnostics: a recursive YAML anchor, a pure-$ref +// cycle (a chain of schema $refs that never reaches a node without one), a +// reference whose pointer resolves through a reference already being resolved, +// and alias amplification (a billion-laughs expansion). A source that doesn't +// decode as YAML yields no cycles — the main parser reports that as a parse +// problem — and the scan runs under recoverCycleScan so a detector bug degrades +// to "no cycle found" rather than aborting. func Cycles(srcIndex int, data []byte) []ir.Diagnostic { return recoverCycleScan(srcIndex, func() []ir.Diagnostic { return scanCycles(srcIndex, data) @@ -163,10 +166,14 @@ func anchorName(alias *yaml.Node) string { return alias.Value } -// refCycles reports the first pure-$ref cycle: a chain of schema $refs followed -// until it revisits a node already on the chain, without ever reaching a node -// that carries no top-level $ref (which terminates the chain, matching where -// speakeasy stops resolving). +// refCycles reports the first degenerate chain among the collected references: +// one followed until it revisits a node already on it, without ever reaching a +// node that carries no top-level $ref (which terminates the chain, matching +// where speakeasy stops resolving). +// +// Schema positions are refused on that alone. Reference-object positions carry +// a second rule and one exemption, both of which turn on what the resolver can +// see rather than on where the pointer points — see outsideCycle. // // If the mapping view hit nodeview.MergeDepthLimit, it returns a diag.CycleScanFailed // warning instead of a clean nil: truncation only ever drops pairs, so a cycle diff --git a/compilers/openapi/internal/scan/scan_internal_test.go b/compilers/openapi/internal/scan/scan_internal_test.go index 25a43c5..8074949 100644 --- a/compilers/openapi/internal/scan/scan_internal_test.go +++ b/compilers/openapi/internal/scan/scan_internal_test.go @@ -45,6 +45,9 @@ var cycleReproducers = []struct{ name, file string }{ {"path-item-prefix-chain", "cycle_path_item_prefix_chain"}, {"component-path-item-prefix", "cycle_component_path_item_prefix"}, {"webhook-prefix-self", "cycle_webhook_prefix_self"}, + // A self-reference only the resolver's pointer normalization reveals, which + // overflows the stack rather than deadlocking: the resolution cache ends up + // pointing at its own reference and GetObject's delegation recurses. {"pointer-whitespace-self", "cycle_pointer_whitespace_self"}, } @@ -154,23 +157,33 @@ components: A: {$ref: *r} B: {type: object} `}, - // The four cases below are the negative controls for the re-entrant-prefix - // refusal. Each carries a pointer whose prefix names a reference, which is - // the shape that rule keys on — but not one on the chain reading it, so the - // resolver handles them and reports an unresolved ref. Each was measured - // against speakeasy v1.24.0 before being written down here: refusing any of - // them would be refusing a document that compiles. + // The cases below are the negative controls for the re-entrant-prefix + // refusal, and they are what keeps it from widening into an over-refusal. + // Each carries a pointer whose prefix names a reference — the shape the rule + // keys on — without being the shape that hangs: + // + // - the first two pass through a reference that is not on the chain + // reading it, so no lock is re-entered; + // - the last two re-enter from a schema position, which resolves through + // jsonschema rather than the openapi Reference wrapper and so never takes + // the lock at all. + // + // Each was measured against speakeasy v1.24.0 before being written down: + // refusing any of them would be refusing a document that compiles today. {"legal-prefix-both-hops-dangle", `openapi: 3.1.0 info: {title: t, version: '1'} paths: /a: {$ref: '#/paths/~1b/t'} /b: {$ref: '#/paths/~1a/t'} `}, - {"legal-prefix-names-offchain-ref", `openapi: 3.1.0 + {"legal-prefix-through-offchain-ref", `openapi: 3.1.0 info: {title: t, version: '1'} paths: /a: {$ref: '#/paths/~1b/t'} - /b: {get: {operationId: b, responses: {"200": {description: ok}}}} + /b: {$ref: '#/components/pathItems/C'} +components: + pathItems: + C: {get: {operationId: c, responses: {"200": {description: ok}}}} `}, {"legal-schema-prefix-self", `openapi: 3.1.0 info: {title: t, version: '1'} @@ -179,7 +192,7 @@ components: schemas: A: {$ref: '#/components/schemas/A/properties', properties: {p: {type: string}}} `}, - {"legal-schema-prefix-into-path-item", `openapi: 3.1.0 + {"legal-schema-chain-reentry", `openapi: 3.1.0 info: {title: t, version: '1'} paths: /a: {$ref: '#/components/schemas/S/t'} From e3f50dffafcf3551efa3f3a2cdd2ee684d99c17a Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 2 Aug 2026 07:16:33 +0300 Subject: [PATCH 3/3] test(harness): measure the reference refusal against the resolver The refusal in compilers/openapi/internal/scan models one version of speakeasy's behavior. Until now the evidence that the model was right was four hand-written negative controls and a set of reproducers -- an argument, not a measurement, and the same kind of argument that produced the components carve-out this branch had to correct. So ask the resolver. For each of thirty-five generated shapes (five reference positions x seven pointer forms) the oracle compares what the compiler does with what the resolver actually does, and holds the compiler to both directions a model can be wrong in: a shape the resolver cannot survive must be refused, and a shape it resolves cleanly must not be. A shape it survives but reports an error on binds neither way, and getting that distinction wrong is not hypothetical -- the oracle's first run "failed" on four shapes because it read "the resolver returned" as "the document is fine". An exact self-reference at a document position returns, reporting a circular reference. Refusing it early is a choice about which message is better, not an over-refusal, so the helper reports what the resolver said rather than only that it returned. Both halves run in a subprocess. A deadlock and a stack overflow cannot be observed from inside the process they happen to, and the compiler is subject to both if its refusal regresses -- measured in-process, the oracle would hang on exactly the regression it exists to catch rather than failing. Planting that regression now reddens six shapes in ten seconds, each naming the spec. It lives entirely in a _test.go file, so neither the coverage gate nor the architecture test is affected by a harness that talks to the dependency directly. Alongside it, check-coverage.sh sets an explicit -timeout. A regression in this refusal is a test that never returns rather than one that fails, and go test's ten-minute default is a long time to wait to be told which goroutine is stuck. --- compilers/openapi/internal/scan/scan.go | 3 +- internal/harness/resolver_oracle_test.go | 288 +++++++++++++++++++++++ scripts/check-coverage.sh | 7 +- 3 files changed, 296 insertions(+), 2 deletions(-) create mode 100644 internal/harness/resolver_oracle_test.go diff --git a/compilers/openapi/internal/scan/scan.go b/compilers/openapi/internal/scan/scan.go index 61caa44..8ecd5df 100644 --- a/compilers/openapi/internal/scan/scan.go +++ b/compilers/openapi/internal/scan/scan.go @@ -22,7 +22,8 @@ import ( // maxCycleDepth bounds every recursive descent in the cycle detector. It guards // the walk against a runaway structure per the bounded-recursion rule; real -// specs nest far shallower, so the cap only ever fires on a detector bug. +// specs nest far shallower, so nothing short of a document built to reach it — +// or a detector bug — ever does. const maxCycleDepth = 10000 // schemaEntryMapKeys name a mapping of schemas encountered outside a schema diff --git a/internal/harness/resolver_oracle_test.go b/internal/harness/resolver_oracle_test.go new file mode 100644 index 0000000..489aabe --- /dev/null +++ b/internal/harness/resolver_oracle_test.go @@ -0,0 +1,288 @@ +package harness_test + +// The differential oracle for the compiler's reference refusals. +// +// compilers/openapi/internal/scan refuses documents that would hang or crash +// speakeasy's resolver. That refusal is a *model* of one version's behavior, and +// a model can be wrong in two directions: too narrow lets a hang through, too +// wide refuses a document that compiles. Neither shows up in a test that only +// asserts what the scan says, because the scan is the thing under test. +// +// So this asks the resolver instead. For each generated shape it compares what +// the compiler does against what the resolver actually does, both measured in a +// subprocess — because a deadlock and a stack overflow cannot be observed from +// inside the process they happen to, and because the compiler is subject to both +// if its refusal ever regresses. An oracle that hangs instead of failing on the +// regression it exists for would be no oracle at all. +// +// It lives entirely in a _test.go file: the coverage gate counts statements in +// non-test files and the architecture test reads only non-test imports, so +// neither is affected by a harness that talks to the dependency directly. + +import ( + "context" + "fmt" + "os" + "os/exec" + "strings" + "testing" + "time" + + soa "github.com/speakeasy-api/openapi/openapi" + "github.com/stretchr/testify/assert" + + "github.com/dexpace/morphic/compilers" + "github.com/dexpace/morphic/compilers/openapi" + "github.com/dexpace/morphic/ir" +) + +// probeEnv carries the spec to the helper process. Its presence is also what +// tells the helper it is the helper. +const probeEnv = "MORPHIC_RESOLVER_PROBE" + +// probeTimeout is how long a shape gets before it counts as hanging. A shape +// that resolves at all resolves in milliseconds, so this only ever bounds a +// deadlock, and only the shapes that deadlock pay it. Subtests run in parallel, +// so it is the wall-clock cost of the slowest shape, not their sum. +const probeTimeout = 10 * time.Second + +// The helper reports each half of its work on stdout as it finishes, so a shape +// that never returns is still attributable: losing both markers means the +// compiler did not survive, losing only the second means the resolver did not. +const ( + refusedMarker = "MORPHIC_COMPILER_REFUSED=" + errorMarker = "MORPHIC_RESOLVE_ERRORS=" +) + +// What the resolver did with a shape. +const ( + resolverClean = "resolved-clean" // returned, reporting nothing + resolverErrors = "resolved-errors" // returned, reporting a problem with the spec + resolverHung = "hung" // still running at probeTimeout + resolverDied = "died" // exited non-zero: a runtime fatal error +) + +const specHead = "openapi: 3.1.0\ninfo: {title: t, version: '1'}\n" + +// referencePosition is a document shape with one reference object in it, named +// by the pointer that reaches it. The template's %s takes the reference body. +type referencePosition struct { + name string + pointer string + tmpl string +} + +func referencePositions() []referencePosition { + return []referencePosition{ + { + name: "path-item", + pointer: "#/paths/~1a", + tmpl: specHead + "paths:\n /a: %s\n", + }, + { + name: "webhook", + pointer: "#/webhooks/onA", + tmpl: specHead + "paths: {}\nwebhooks:\n onA: %s\n", + }, + { + name: "component-path-item", + pointer: "#/components/pathItems/A", + tmpl: specHead + "paths:\n /a: {$ref: '#/components/pathItems/A'}\ncomponents:\n pathItems:\n A: %s\n", + }, + { + name: "component-response", + pointer: "#/components/responses/A", + tmpl: specHead + "paths:\n /a:\n get:\n operationId: a\n responses:\n" + + " \"200\": {$ref: '#/components/responses/A'}\ncomponents:\n responses:\n A: %s\n", + }, + { + name: "schema", + pointer: "#/components/schemas/A", + tmpl: specHead + "paths: {}\ncomponents:\n schemas:\n A: %s\n", + }, + } +} + +// pointerForm builds a reference body from the pointer that reaches it, so each +// form can be applied at every position. +type pointerForm struct { + name string + body func(pointer string) string +} + +func pointerForms() []pointerForm { + return []pointerForm{ + {"exact-self", func(p string) string { return fmt.Sprintf("{$ref: '%s'}", p) }}, + {"prefix-self-dangling", func(p string) string { return fmt.Sprintf("{$ref: '%s/t'}", p) }}, + {"prefix-self-trailing-space", func(p string) string { return fmt.Sprintf("{$ref: '%s '}", p) }}, + {"prefix-self-percent-encoded", func(p string) string { + return fmt.Sprintf("{$ref: '%s'}", strings.Replace(p, "#/", "#/%", 1)) + }}, + {"prefix-self-resolving-sibling", func(p string) string { + return fmt.Sprintf("{$ref: '%s/description', description: d}", p) + }}, + {"dangling-elsewhere", func(string) string { return "{$ref: '#/nope/nope'}" }}, + {"external-document", func(string) string { return "{$ref: 'other.yaml#/x'}" }}, + } +} + +// TestResolverOracle_RefusalMatchesResolverBehavior is the oracle. Across every +// generated shape it holds the compiler to two things, in the two directions a +// model of someone else's behavior can be wrong: +// +// - A shape the resolver cannot survive must be refused. Letting one through +// is the bug this guard exists for: a hang, or a process taken down. +// - A shape the resolver resolves cleanly must not be refused. Refusing one is +// an over-refusal — a document that compiled yesterday and does not today. +// +// A shape the resolver survives but reports an error on binds neither way. The +// document fails whichever path it takes, so refusing it early with a clearer +// message and leaving the resolver to name it are both defensible, and that +// choice belongs to the scan rather than to this oracle. The distinction is why +// the helper reports what the resolver *said* rather than only that it returned: +// an exact self-reference at a document position resolves with a +// circular-reference error, and reading that as "fine" would have this oracle +// demand an over-refusal be introduced. +func TestResolverOracle_RefusalMatchesResolverBehavior(t *testing.T) { + t.Parallel() + if os.Getenv(probeEnv) != "" { + t.Skip("helper process") + } + + for _, pos := range referencePositions() { + for _, form := range pointerForms() { + name, spec := pos.name+"/"+form.name, fmt.Sprintf(pos.tmpl, form.body(pos.pointer)) + t.Run(name, func(t *testing.T) { + t.Parallel() + + refused, outcome := probe(t, spec) + switch outcome { + case resolverHung, resolverDied: + assert.True(t, refused, + "the resolver cannot survive this, so the compiler must refuse it before "+ + "reaching it\nresolver outcome: %s\nspec:\n%s", outcome, spec) + case resolverClean: + assert.False(t, refused, + "the resolver resolves this cleanly, so refusing it is an over-refusal"+ + "\nspec:\n%s", spec) + default: + t.Logf("resolver reported an error; refusing is the scan's call (refused=%v)", refused) + } + }) + } + } +} + +// probe runs one shape in a subprocess and reports the compiler's verdict and +// what the resolver did. +func probe(t *testing.T, spec string) (refused bool, outcome string) { + t.Helper() + + ctx, cancel := context.WithTimeout(context.Background(), probeTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, os.Args[0], + "-test.run=^TestResolverOracle_ProbeHelper$", "-test.timeout="+probeTimeout.String()) + cmd.Env = append(os.Environ(), probeEnv+"="+spec) + + out, err := cmd.CombinedOutput() + text := string(out) + + verdict, sawCompiler := markerValue(text, refusedMarker) + if !sawCompiler { + t.Fatalf("the compiler did not survive this shape (%s), so its refusal has regressed"+ + "\nspec:\n%s\n%s", stopReason(ctx, err), spec, truncate(out)) + } + refused = verdict == "true" + + errCount, sawResolver := markerValue(text, errorMarker) + switch { + case !sawResolver && ctx.Err() != nil: + return refused, resolverHung + case !sawResolver: + t.Logf("resolver process exited non-zero (%v):\n%s", err, truncate(out)) + return refused, resolverDied + case errCount == "0": + return refused, resolverClean + default: + return refused, resolverErrors + } +} + +// TestResolverOracle_ProbeHelper is the subprocess body. It compiles the spec in +// probeEnv, reports the verdict, then resolves the same spec with nothing in the +// way. It asserts nothing: hanging or dying is the signal, and the parent reads +// it from which markers arrived. +func TestResolverOracle_ProbeHelper(t *testing.T) { + spec := os.Getenv(probeEnv) + if spec == "" { + t.Skip("not the helper process") + } + ctx := context.Background() + + // The compiler first, its verdict printed before anything else runs: if this + // call is what hangs, the missing marker is the finding. + _, diags, err := openapi.New().Compile(ctx, + []compilers.Source{{Path: "probe.yaml", Data: []byte(spec)}}, compilers.Options{}) + fmt.Printf("%s%v\n", refusedMarker, err == nil && hasCyclicRef(diags)) + + // Then the resolver with nothing in its way, which is the measurement the + // compiler's refusal is a model of. + doc, _, err := soa.Unmarshal(ctx, strings.NewReader(spec)) + if err != nil || doc == nil { + fmt.Printf("%s1\n", errorMarker) // never reached the resolver: a parse problem + return + } + resolveErrs, err := doc.ResolveAllReferences(ctx, soa.ResolveAllOptions{ + OpenAPILocation: "probe.yaml", + DisableExternalRefs: true, + }) + count := len(resolveErrs) + if err != nil { + count++ + } + fmt.Printf("%s%d\n", errorMarker, count) +} + +// hasCyclicRef reports whether the compiler refused a spec as a degenerate +// reference. Other error diagnostics — an unresolved ref, a validation failure — +// are not refusals of this kind and do not count. +func hasCyclicRef(diags []ir.Diagnostic) bool { + for _, d := range diags { + if d.Code == "openapi/cyclic-ref" && d.Severity == ir.SeverityError { + return true + } + } + return false +} + +// markerValue reads the value the helper printed for a marker. +func markerValue(text, marker string) (string, bool) { + i := strings.Index(text, marker) + if i < 0 { + return "", false + } + rest := text[i+len(marker):] + if j := strings.IndexByte(rest, '\n'); j >= 0 { + rest = rest[:j] + } + return strings.TrimSpace(rest), true +} + +// stopReason names why a child stopped, for the failure message. +func stopReason(ctx context.Context, err error) string { + if ctx.Err() != nil { + return "hung" + } + return fmt.Sprintf("died: %v", err) +} + +// truncate bounds a failing subprocess's output so a stack dump does not bury +// the assertion that reported it. +func truncate(out []byte) string { + const max = 400 + if len(out) <= max { + return string(out) + } + return string(out[:max]) + "\n... (truncated)" +} diff --git a/scripts/check-coverage.sh b/scripts/check-coverage.sh index 329cfbb..b21ea9f 100755 --- a/scripts/check-coverage.sh +++ b/scripts/check-coverage.sh @@ -17,7 +17,12 @@ cover_file="${COVER_FILE:-cover.out}" # blocks, and the first screenful is what gets read. max_reported=25 -go test ./... -covermode=atomic -coverprofile="$cover_file" +# -timeout is explicit rather than left to go test's 10-minute default. The +# compiler refuses documents that would otherwise hang the third-party resolver, +# so a regression in that refusal is a test that never returns, not one that +# fails. Ninety seconds is several times the suite's normal wall time and turns +# that failure mode into a prompt stack dump naming the stuck goroutine. +go test ./... -timeout 90s -covermode=atomic -coverprofile="$cover_file" # Profile body, one block per line: "/.go: ". # Sorted so the same failure reads the same way on every run.