Uh oh!
There was an error while loading. Please reload this page.
fix(compilers/openapi): treat an empty pointer segment as a token - #304
Merged
Conversation
TestCompile_EmptyPointerSegmentTerminates asserted exactly what TestCompile_CyclicSpecDoesNotCrash/path-item-empty-segment already asserts over the same fixture: no Go error, a nil document, and an openapi/cyclic-ref error diagnostic. Its only additional content was a 10s bound, justified as turning a regression from a suite that stops dead into a test that fails fast and names the cause. It does not do that. The same fixture is compiled without a bound by the reproducer table beside it and by internal/harness's corpus sweep, so a regression still runs both test binaries into `go test`'s timeout and prints a stack dump. Reverting the pointer fix with the bounded test in place was measured: it failed at 10.00s and the package still panicked at the timeout, and internal/harness panicked on its own. The bound the repository actually relies on for this failure mode is already recorded and tuned — scripts/check-coverage.sh passes `-timeout 90s` explicitly, and its comment says why. That script is the gate's only test invocation, so 90s is always in force in CI.
Every other third-party citation in nodeview.go names the speakeasy version it was read at, so a dependency bump has something to re-check against. The two added with the empty-token rule did not. tokenless also asserted that both tokenless spellings name the root but cited the resolver only for the lone separator. The empty pointer takes a different path there: references/resolution.go resolveAgainstDocument short-circuits it to the root document before the pointer parser runs, which is worth naming because getNavigationStack rejects an empty pointer outright.
Reading the empty token changes a second class of input, and it was neither pinned nor stated. A schema $ref spelled '#/components/schemas/A/' used to be refused openapi/cyclic-ref and is now reported openapi/unresolved-ref, because the walk no longer stops at A: refCycles refuses a schema chain on chainCycles alone, and the reclassified shape is chainReenters, which only the reference objects outside a schema are checked for. That asymmetry is correct — speakeasy resolves a schema $ref as an oas3.JSONSchema rather than through the Reference lock that deadlocks — and the new verdict is the more accurate one, since the pointer names a key that is not declared. Six schema re-entrancy spellings were compiled to confirm the shape reports rather than hangs. Pinning it makes the change deliberate: with the pointer fix reverted the test fails naming openapi/cyclic-ref.
The comment stated why refCycles consults chainReenters only for the reference objects outside a schema, as though the code recorded that rationale. It does not. State the verified fact instead and name the revision it was read at: openapi/reference.go's cacheMutex is the only lock held across a pointer walk in v1.24.0, and jsonschema/oas3 carries no per-reference lock at all, which is why the shape this hands the resolver reports rather than blocks.
Uh oh!
There was an error while loading. Please reload this page.
OmarAlJarrah added a commit
that referenced
this pull request
Aug 9, 2026
The conflict this branch predicted, resolved as it said: #304's TestDetectCycles_EmptyPointerSegmentIsRefused calls Cycles(0, []byte(src)), and Cycles now takes a sourceindex.Index, so the call becomes scanBytes(t, ...). Two the branch could not predict. cycles_test.go: #304 and #310 added a test where this branch adds a scanIndex helper, both at the same offset sharing a closing brace — kept both. And #328 consolidated the openapi test scaffolding into internal/openapitest after this branch was written, so its new entry_internal_test.go calls sourceOf by the package-local name that no longer exists; repointed, with the import added. Byte-identical output re-proven against the merged tree rather than carried over from the branch: 115 sources under testdata compiled through binaries built from main and from this merge, capturing document, stderr and exit code each. diff -r over the two trees is empty.
OmarAlJarrah added a commit
that referenced
this pull request
Aug 11, 2026
The resource-boundary walk grew a pointer tokenizer of its own, which duplicated nodeview.PointerPath — already fixed for this same empty-token bug class in #304, in the layer below, a month earlier. Reusing it drops the second tokenizer and with it three defects the copy carried: it read a lone "/" as one empty token where the sibling walk reads it as the root, it lost every boundary above a pointer with no leading "/", and its loop had no explicit bound where PointerPath counts against maxPointerSegments. The walk also built a nodeview.View per call, discarding the expansion memo the type exists to hold. The view now lives on AnchorIndex beside the anchor memo, so one document builds one. Compiling merge-heavy specs timed the same either way and the IR is byte-identical; what changes is that the memo is reachable at all. Sweeping the same empty-name mechanism turned up a second site the original sweep missed. ids.ComponentEntry rejects an empty name, which is deliberate — an entry keyed "" earns no named TypeID, as testdata/conformance/openapi/empty-names.yaml records — but the $dynamicAnchor path reported it as "declared at /components/schemas/ rather than on a component schema", which the document contradicts: that pointer addresses a component schema. The verdict is unchanged; the reason now says an empty name earns no named type to expand to. Coverage: no committed spec combined an empty component name with $id and $dynamicRef, so no oracle drove the fixed path. dynamic-ref.yaml now writes that combination, and reverting the fix reddens the conformance case. The empty-pointer unit case asserted nothing before — both readings produced false — so the fixture now parks an $id under a root member keyed "", which a walk that took "" for a token would descend into and find.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A path item whose
$refnames its own location with a trailing separator —#/components/pathItems/A/— madeCompilenever return. No diagnostic, no error,no progress: the process blocks forever inside the resolver. That is a denial of
service on untrusted spec input, since the document is legal YAML and legal OpenAPI
as far as anything upstream of the resolver is concerned. Under the CLI it surfaces
as Go's
fatal error: all goroutines are asleep - deadlock!only because nothingelse is running; with any other goroutine alive the runtime detector cannot fire and
it is a plain hang.
The cycle pre-scan already has a guard for exactly this shape.
chainReentersrefusesa reference whose pointer walks through a reference that is mid-resolve, because
speakeasy holds a reference's write lock across the pointer walk and read-locks every
reference the walk passes through, so re-entering one self-deadlocks on a
non-reentrant
RWMutex. The guard is wired in and it is reached. It was being fed alaundered input.
nodeview.PointerPathskipped every empty segment of a pointer:Only the leading empty string is an artifact of splitting on
/. Every later one isa real reference token. RFC 6901 §3 defines a pointer as
*( "/" reference-token )with
reference-token = *( unescaped / escaped )— the token may be empty, and itnames a member whose key is the empty string. speakeasy reads it the same way; its
parser splits
strings.TrimPrefix(pointer, "/")and comments the case explicitly("Empty parts are valid according to RFC 6901 - they represent empty string keys",
jsonpointer/navigation.go).So
#/components/pathItems/A/does not stop atA; it descends throughAlookingfor the key
""and never arrives. Skipping the token reported the walk as completeand ending at
A— andtraverseexcludes a complete walk's destination from there-entry test, correctly, because arriving at an on-chain node is an ordinary cycle
that speakeasy reports with a better message. The through-node was reclassified as a
destination, so the one node that had to be tested was the one node exempted.
The fix is in the tokenizer, not in the guard: split the way the resolver splits, and
let the empty token resolve or fail on its own terms. It fails here, which makes the
walk incomplete, which puts
Aback into the set of nodes the pointer passed through,which yields
chainReenters. The document is now refused withopenapi/cyclic-refbefore the resolver ever runs.
Two smaller points that came with it:
pointer, which is how a bare
#arrives, and a lone/. The second is a deliberatedeparture from RFC 6901, which reads
/as one empty token —getNavigationStackspecial-cases it to an empty navigation stack, and this walk models what the
resolver walks rather than what the grammar admits. It is called out in the code.
maxPointerSegments,so a pointer built to be long rather than to name anything is bounded exactly as
before, without materializing its segments first.
Mechanism sweep
PointerPathis the compiler's only JSON-pointer walker, andscan.traverseis itsonly production caller, so the deadlock's blast radius is closed. Every other site
that splits a pointer on
/was checked:schema.declaresResourceIDAboveskips empty segments the same way. Its consequenceis different — a
$idat a position whose pointer ends in an empty token is missed,so a
$dynamicRefexpansion crosses a resource boundary it should have degraded at— and so is its fix, since that walk reads
$idbefore descending and must keepskipping the leading segment. Verified by probe and filed as openapi: $id resource-boundary walk skips empty pointer segments #302 rather than folded
in here.
schema.branchPointerHintreads the last two segments without skipping anything;an empty index fails
isDecimalIndexand the hint is declined. Correct as is.overlaybuilds pointers by concatenation and looks them up exactly, never walkingone. Not affected.
operation's split is over a URL path template, not a JSON pointer.One verdict changes besides the hang
Reading the empty token also moves a schema-position ref of the same shape from
openapi/cyclic-reftoopenapi/unresolved-ref.refCyclesrefuses a schema chain onchainCyclesalone;chainReentersis consulted only for the reference objects outsidea schema, since only those resolve through the
Referencelock that deadlocks. Reading/A/as stopping atAmade the shape a cycle, and reading it as descending throughAmakes it what it is — a pointer naming a key that is not declared.
That is the more accurate verdict, and it is not a new way to hang: speakeasy resolves a
schema
$refas anoas3.JSONSchemarather than through that lock, and six schemare-entrancy spellings were compiled to confirm the class reports rather than blocks.
TestCompile_SchemaEmptyPointerSegmentIsUnresolvedpins it, and reverting the pointer fixfails it naming
openapi/cyclic-ref.Test plan
compilers/openapi/cycles_test.go, which the fixture joins: no Go error, a nildocument, an
openapi/cyclic-referror diagnostic. There is no per-test bound onit, deliberately. A regression here is a permanent deadlock rather than a failed
assertion, and the bound this repository uses for that is already recorded and
tuned —
scripts/check-coverage.shpasses-timeout 90sexplicitly, and it is thegate's only test invocation. A bespoke bound cannot improve on it while the same
fixture is also compiled unbounded by the table and by
internal/harness's corpussweep: reverting the fix with one in place was measured, and both binaries still ran
to the timeout.
TestPointerPath_EmptyTokenIsARealTokenandTestPointerPath_EmptyTokenNamesTheEmptyKeypin both sides of the token rule:/a/does not resolve when no
""key is declared and reportsaas passed through, andit resolves to the value when one is.
TestPointerPath_LoneSeparatorNamesTheRootpins the resolver-matching special case.
TestDetectCycles_EmptyPointerSegmentIsRefusedencodes the reproducer at the scanlevel, next to the whitespace-normalization test that covers the sibling escape.
testdata/openapi/cycle_path_item_empty_segment.yamljoins the re-entrant-prefixfamily in both
cycleReproducerstables and in the harness corpus'sknownInvalidlist, which also makes it a
FuzzCycleDetectorseed. Deleting the trailing/fromthe fixture reddens
TestDetectCycles_Reproducers/path-item-empty-segment, so thefixture is testing what its name says.
(
#/components/pathItems/A/and#/paths/~1a/) were compiled through the CLI andrefused with
openapi/cyclic-ref; before the change the same inputs deadlocked.gofmt,go vet,golangci-lint,go build, and the coveragegate at 100%.
Closes#238