diff --git a/compilers/openapi/cycles_test.go b/compilers/openapi/cycles_test.go index 5ed9967..4a18066 100644 --- a/compilers/openapi/cycles_test.go +++ b/compilers/openapi/cycles_test.go @@ -35,6 +35,18 @@ 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"}, + // 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"}, } func TestCompile_CyclicSpecDoesNotCrash(t *testing.T) { @@ -249,3 +261,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/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/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..8ecd5df 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 ( @@ -20,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 @@ -56,15 +59,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 +167,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 @@ -176,7 +184,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 +202,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. +// +// 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. // -// 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. +// 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 +238,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 +466,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..8074949 100644 --- a/compilers/openapi/internal/scan/scan_internal_test.go +++ b/compilers/openapi/internal/scan/scan_internal_test.go @@ -37,6 +37,18 @@ 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"}, + // 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"}, } func TestDetectCycles_Reproducers(t *testing.T) { @@ -144,6 +156,49 @@ components: schemas: A: {$ref: *r} B: {type: object} +`}, + // 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-through-offchain-ref", `openapi: 3.1.0 +info: {title: t, version: '1'} +paths: + /a: {$ref: '#/paths/~1b/t'} + /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'} +paths: {} +components: + schemas: + A: {$ref: '#/components/schemas/A/properties', properties: {p: {type: string}}} +`}, + {"legal-schema-chain-reentry", `openapi: 3.1.0 +info: {title: t, version: '1'} +paths: + /a: {$ref: '#/components/schemas/S/t'} +components: + schemas: + S: {$ref: '#/paths/~1a'} `}, } @@ -270,8 +325,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 +337,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 +352,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 +714,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/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. 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'}