From 1b3e5368a4d2fb0973db80fa663b93ce4c3a0a6f Mon Sep 17 00:00:00 2001 From: Fuad Daoud Date: Sat, 5 Sep 2026 19:49:35 +0300 Subject: [PATCH] fix(compilers/openapi): detect a version key past the sniff cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Detect capped its search for `openapi`/`swagger` at the first 64 KiB, so a valid document that writes a large object before its version key was reported as an unrecognized format. Stripe's published spec3.json is one: `components` runs to megabytes and `openapi` lands at byte 2,593,401. Mapping key order carries no meaning, so the same document with its keys the other way round compiled fine — the format answer depended on where a writer put a key. The 64 KiB prefix keeps its place as the fast path, and every document that declares a key there is still answered without a full parse. When the prefix declares neither key, a byte scan of the whole source decides whether to read it whole: only bytes that name `openapi:` or `swagger:` as a top-level key reach the parse, so a source of another format still gets the fast path's silence and never a complaint from this compiler. That scan is what Detect already used to tell its own broken source from another format's, and it is no longer bounded to the prefix either. A document whose prefix does not parse and whose declaration sits past the cap is now reported as an undecodable OpenAPI source rather than declined, which is the answer the surrounding rule always intended; detection now reads the bytes it would have had to read to say otherwise. Fixes #420 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SYeBgDsskwnyitLgPGCPn1 --- compilers/openapi/detect.go | 100 ++++++++++++++++-------- compilers/openapi/detect_test.go | 129 +++++++++++++++++++++++++------ 2 files changed, 173 insertions(+), 56 deletions(-) diff --git a/compilers/openapi/detect.go b/compilers/openapi/detect.go index a5befa0a..c54fc80d 100644 --- a/compilers/openapi/detect.go +++ b/compilers/openapi/detect.go @@ -11,15 +11,20 @@ import ( "github.com/dexpace/morphic/ir" ) -// maxSniffBytes bounds the bytes Detect parses. Detection reads two top-level -// keys, and 64 KiB reaches them in any document a person wrote, so the cost of -// asking stays flat while spec size does not: a full parse of a 10 MB document -// costs hundreds of milliseconds before the compiler's own parse begins. +// maxSniffBytes bounds the prefix Detect parses on its fast path. Detection +// reads two top-level keys, and 64 KiB reaches them in any document a person +// wrote, so the cost of asking stays flat while spec size does not: a full parse +// of a 10 MB document costs hundreds of milliseconds before the compiler's own +// parse begins. It is a bound on the fast path, not on detection — a document +// whose prefix declares neither key while its bytes name one is read whole, per +// sniffWhole. const maxSniffBytes = 64 << 10 -// maxSniffEntries bounds the top-level entries read from a flow-style prefix. -// The keys being looked for are declared among a document's first few, and a -// prefix full of nothing else is not one this compiler will take. +// maxSniffEntries bounds the top-level entries read from a flow-style mapping. +// A document declares few top-level keys however large it grows, so a mapping +// that runs past this without naming either key is not one this compiler will +// take. The bound is on entries, not bytes: one of them may be megabytes long, +// which is the whole reason the byte cap alone does not answer the question. const maxSniffEntries = 512 // sniffProbe holds the two discriminating top-level keys. Which one is present @@ -66,16 +71,16 @@ func (*Compiler) Detect(src compilers.Source) (compilers.SourceFormat, []ir.Diag } // declaresProbeKey reports whether data names one of the discriminating keys as -// a top-level key. It is what separates a source of this compiler's own that -// will not parse from one of another format that was never its business: a -// parse failure alone says only "not YAML", which a Protobuf or Smithy source -// is not either. +// a top-level key. It is what separates a source of this compiler's own from one +// of another format that was never its business, and it is asked twice: before +// sniff parses a large document whole, and after a parse failed, where "not +// YAML" alone says only what a Protobuf or Smithy source would also say. // -// Only the bounded prefix is read, for the reason sniff bounds its own reads. +// The whole of data is read. A byte scan costs a fraction of the parse it stands +// in front of, and the key it looks for is exactly the one that can sit +// megabytes into a document — bounding this to the prefix would blind it in +// precisely the case it exists to catch. func declaresProbeKey(data []byte) bool { - if len(data) > maxSniffBytes { - data = data[:maxSniffBytes] - } return declaresKey(data, "openapi") || declaresKey(data, "swagger") } @@ -110,26 +115,58 @@ func followedByColon(data, name []byte) bool { } } -// sniff reads the discriminating keys out of at most maxSniffBytes bytes, and -// returns the zero probe and the parser's error for anything it cannot read. -// Whether that error is worth reporting is Detect's question, not this one's: -// here it is only the record of what happened. +// sniff reads the discriminating keys out of data, and returns the zero probe +// and the parser's error for anything it cannot read. Whether that error is +// worth reporting is Detect's question, not this one's: here it is only the +// record of what happened. // -// A document within the cap is decoded whole and exactly. A larger one is -// decoded from a prefix, which cannot simply be cut: flow style — JSON is the -// common case — is one token stream with no line structure, so its entries are -// streamed instead, and block style is cut at its last complete line. +// A document within the cap is decoded whole and exactly. A larger one is read +// from its prefix first, and only from all of itself when that prefix answered +// nothing and the bytes past it name a key this compiler serves. func sniff(data []byte) (sniffProbe, error) { if len(data) <= maxSniffBytes { return decodeYAML(data) } - prefix := data[:maxSniffBytes] - if probe, ok := decodeFlowPrefix(prefix); ok { + + probe, err := sniffPrefix(data[:maxSniffBytes]) + if probe.OpenAPI != "" || probe.Swagger != "" { + return probe, nil + } + if declaresProbeKey(data) { + return sniffWhole(data) + } + return probe, err +} + +// sniffPrefix reads the probe keys from the first maxSniffBytes of a document +// too large to decode whole. The prefix cannot simply be cut: flow style — JSON +// is the common case — is one token stream with no line structure, so its +// entries are streamed instead, and block style is cut at its last complete +// line. +func sniffPrefix(prefix []byte) (sniffProbe, error) { + if probe, ok := decodeFlowEntries(prefix); ok { return probe, nil } return decodeYAML(wholeLines(prefix)) } +// sniffWhole reads the probe keys from a whole document past the cap, for the +// one case that earns the parse: the prefix declared neither key, yet the bytes +// name one further in. Mapping key order carries no meaning, so a document that +// writes a multi-megabyte `components` before its `openapi` is as valid as one +// that writes them the other way round, and declining it would reject a valid +// document over nothing. +// +// Nothing another format wrote reaches here — declaresProbeKey guards the call — +// so the cost is paid only for bytes this compiler is about to parse in full +// anyway, and the answer for everyone else is still the fast path's silence. +func sniffWhole(data []byte) (sniffProbe, error) { + if probe, ok := decodeFlowEntries(data); ok { + return probe, nil + } + return decodeYAML(data) +} + // decodeYAML reads the probe keys from a complete YAML (or JSON, its subset) // document. func decodeYAML(data []byte) (sniffProbe, error) { @@ -140,12 +177,13 @@ func decodeYAML(data []byte) (sniffProbe, error) { return probe, nil } -// decodeFlowPrefix reads the top-level entries of a prefix that opens a flow -// mapping, and reports whether it was one. The JSON decoder is used because it -// streams: a prefix cut mid-document still yields every entry it completed, -// where decoding those same bytes whole reports only that they end early. -func decodeFlowPrefix(prefix []byte) (sniffProbe, bool) { - dec := json.NewDecoder(bytes.NewReader(prefix)) +// decodeFlowEntries reads the top-level entries of data, which may be a whole +// document or a prefix of one, and reports whether it opened a flow mapping. The +// JSON decoder is used because it streams: a prefix cut mid-document still +// yields every entry it completed, where decoding those same bytes whole reports +// only that they end early. +func decodeFlowEntries(data []byte) (sniffProbe, bool) { + dec := json.NewDecoder(bytes.NewReader(data)) tok, err := dec.Token() if err != nil || tok != json.Delim('{') { return sniffProbe{}, false diff --git a/compilers/openapi/detect_test.go b/compilers/openapi/detect_test.go index c2bc3063..7523650e 100644 --- a/compilers/openapi/detect_test.go +++ b/compilers/openapi/detect_test.go @@ -1,6 +1,7 @@ package openapi import ( + "fmt" "strings" "testing" @@ -65,19 +66,19 @@ func TestDetect_Formats(t *testing.T) { // key, so the parse error describes a parser that was wrong to be asked. {"unparseable, key only mentioned", "svc.proto", "syntax = \"openapi\";\n{[", compilers.SourceFormat{}, false, nil}, - // Past the sniff cap and still this compiler's: the key search reads the - // same bounded prefix the decode did, so a document too large to parse in - // full is still recognized as broken rather than as somebody else's. + // Past the sniff cap and still this compiler's: the key it declares is in + // the prefix, so the fast path alone is enough to call it broken rather + // than somebody else's. {"unparseable past the cap", "api.yaml", padTo("openapi: [unterminated\n", "filler: x\n"), compilers.SourceFormat{}, false, []string{diag.UndecodableSource}}, // Declares the key only past the cap, on a prefix that does not parse. The - // key search reads the same bounded prefix the decode did and so does not - // see it either; claiming the source would assert something about bytes - // detection never read. + // key search reads every byte, so the declaration is found and the source + // is this compiler's own — broken, and said so, rather than declined as + // somebody else's for want of looking. {"key past the cap on an unparseable prefix", "api.yaml", padTo("bad: [unterminated\n", "filler: x\n") + "openapi: 3.1.0\n", - compilers.SourceFormat{}, false, nil}, + compilers.SourceFormat{}, false, []string{diag.UndecodableSource}}, {"empty", "empty.yaml", "", compilers.SourceFormat{}, false, nil}, } for _, tc := range cases { @@ -92,8 +93,54 @@ func TestDetect_Formats(t *testing.T) { } } -// padTo returns src grown past the sniff cap by appending filler, so sniff takes -// its bounded-prefix path rather than decoding the source whole. +// TestDetect_KeyOrderDoesNotDecideTheFormat is the shape this bound was getting +// wrong: a published spec whose `components` object runs to megabytes and whose +// `openapi` key sits behind it. A JSON object's keys are unordered, so the same +// document written with its version key first and with it last is one document, +// and detection has to answer the same for both. Only the version-last spellings +// go past the prefix — they are the cases a prefix-only sniff declines. +func TestDetect_KeyOrderDoesNotDecideTheFormat(t *testing.T) { + t.Parallel() + flow, block := bigComponents() + cases := []struct{ name, path, src string }{ + {"flow json, version first", "spec3.json", `{"openapi":"3.0.3",` + flow + `}`}, + {"flow json, version last", "spec3.json", `{` + flow + `,"openapi":"3.0.3"}`}, + {"block yaml, version first", "api.yaml", "openapi: 3.0.3\n" + block}, + {"block yaml, version last", "api.yaml", block + "openapi: 3.0.3\n"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + require.Greater(t, len(tc.src), maxSniffBytes, "the case must exceed the cap to test it") + got, diags, ok := New().Detect(compilers.Source{Path: tc.path, Data: []byte(tc.src)}) + assert.True(t, ok, "a valid document must not be declined over where it declares its version") + assert.Equal(t, compilers.SourceFormat{Name: "openapi", Version: "3.0"}, got) + assert.Nil(t, codesOf(diags), "a document this compiler recognizes carries no complaint") + }) + } +} + +// bigComponents returns a `components` entry whose value alone runs past the +// sniff cap, in flow and in block style. It stands in for the schema catalogue a +// published spec leads with; what matters is only that it is one entry too large +// to read past. +func bigComponents() (flow, block string) { + var f, b strings.Builder + f.WriteString(`"components":{"schemas":{`) + b.WriteString("components:\n schemas:\n") + for i := 0; f.Len() <= maxSniffBytes || b.Len() <= maxSniffBytes; i++ { + if i > 0 { + f.WriteByte(',') + } + fmt.Fprintf(&f, `"S%d":{"type":"object","description":"a schema"}`, i) + fmt.Fprintf(&b, " S%d:\n type: object\n description: a schema\n", i) + } + f.WriteString(`}}`) + return f.String(), b.String() +} + +// padTo returns src grown past the sniff cap by appending filler, so sniff reads +// a prefix first rather than decoding the source whole on sight. func padTo(src, filler string) string { var b strings.Builder b.WriteString(src) @@ -103,13 +150,16 @@ func padTo(src, filler string) string { return b.String() } -// TestSniff_BeyondTheCap pins what the bound buys and what it costs. A document -// larger than the cap is read from its first maxSniffBytes in whichever style it -// is written, and a declaration past that point is not seen — detection stays -// flat in document size rather than paying a full parse to read two keys. +// TestSniff_BeyondTheCap pins both paths a document larger than the cap can +// take. The prefix answers on its own whenever it names a key, in whichever +// style the document is written; when it names neither, a document whose bytes +// name one further in is read whole rather than declined, because where a writer +// put a key in a mapping says nothing about what the document is. Bytes that +// name neither key anywhere never leave the prefix. func TestSniff_BeyondTheCap(t *testing.T) { t.Parallel() const filler = "# a line of padding that says nothing about the format\n" + pad := strings.Repeat("p", maxSniffBytes) cases := []struct { name, src string want sniffProbe @@ -117,20 +167,24 @@ func TestSniff_BeyondTheCap(t *testing.T) { {"block yaml declaring first", padTo("openapi: 3.1.0\n", filler), sniffProbe{OpenAPI: "3.1.0"}}, {"block yaml declaring past the cap", - padTo("", filler) + "openapi: 3.1.0\n", sniffProbe{}}, + padTo("", filler) + "openapi: 3.1.0\n", sniffProbe{OpenAPI: "3.1.0"}}, {"flow json declaring first", - `{"openapi":"3.1.0","x":"` + strings.Repeat("p", maxSniffBytes) + `"}`, - sniffProbe{OpenAPI: "3.1.0"}}, + `{"openapi":"3.1.0","x":"` + pad + `"}`, sniffProbe{OpenAPI: "3.1.0"}}, {"flow json declaring past the cap", - `{"x":"` + strings.Repeat("p", maxSniffBytes) + `","openapi":"3.1.0"}`, - sniffProbe{}}, + `{"x":"` + pad + `","openapi":"3.1.0"}`, sniffProbe{OpenAPI: "3.1.0"}}, {"flow json swagger first", - `{"swagger":"2.0","x":"` + strings.Repeat("p", maxSniffBytes) + `"}`, - sniffProbe{Swagger: "2.0"}}, + `{"swagger":"2.0","x":"` + pad + `"}`, sniffProbe{Swagger: "2.0"}}, + {"flow json swagger past the cap", + `{"x":"` + pad + `","swagger":"2.0"}`, sniffProbe{Swagger: "2.0"}}, // Neither YAML nor JSON, and larger than the cap: the prefix is parsed, // fails, and the answer is silence rather than a parser's complaint. {"protobuf past the cap", padTo("syntax = \"proto3\";\n", "message M { string a = 1; }\n"), sniffProbe{}}, + // The word is there past the cap and is not a key, so the whole read is + // never reached — asserted on the guard itself below, since the probe a + // whole read would return here is the zero one either way. + {"the word past the cap is not a key", + `{"x":"` + pad + `","note":"openapi"}`, sniffProbe{}}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -143,7 +197,32 @@ func TestSniff_BeyondTheCap(t *testing.T) { } } -func TestDecodeFlowPrefix_ReadsWhatTheCutLeft(t *testing.T) { +// TestDeclaresProbeKey_GuardsTheWholeRead pins the one decision that keeps a +// document of another format off the slow path: the whole of a source is scanned +// for a key, and only a declaration — the name with the colon that makes it one +// — counts as having found it. +func TestDeclaresProbeKey_GuardsTheWholeRead(t *testing.T) { + t.Parallel() + pad := strings.Repeat("p", maxSniffBytes) + cases := []struct { + name, src string + want bool + }{ + {"declared past the cap in flow style", `{"x":"` + pad + `","openapi":"3.1.0"}`, true}, + {"declared past the cap in block style", "x: " + pad + "\nswagger: \"2.0\"\n", true}, + {"named past the cap as a value", `{"x":"` + pad + `","note":"openapi"}`, false}, + {"named past the cap in prose", "x: " + pad + "\n# openapi is a format\n", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + require.Greater(t, len(tc.src), maxSniffBytes, "the case must exceed the cap to test it") + assert.Equal(t, tc.want, declaresProbeKey([]byte(tc.src))) + }) + } +} + +func TestDecodeFlowEntries_ReadsWhatTheCutLeft(t *testing.T) { t.Parallel() cases := []struct { name, prefix string @@ -166,17 +245,17 @@ func TestDecodeFlowPrefix_ReadsWhatTheCutLeft(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { t.Parallel() - got, flow := decodeFlowPrefix([]byte(tc.prefix)) + got, flow := decodeFlowEntries([]byte(tc.prefix)) assert.Equal(t, tc.wantFlow, flow) assert.Equal(t, tc.want, got) }) } } -// TestDecodeFlowPrefix_StopsAtTheEntryCap proves the walk is bounded by its own +// TestDecodeFlowEntries_StopsAtTheEntryCap proves the walk is bounded by its own // count and not only by the byte cap: a declaration after maxSniffEntries other // entries is not read. -func TestDecodeFlowPrefix_StopsAtTheEntryCap(t *testing.T) { +func TestDecodeFlowEntries_StopsAtTheEntryCap(t *testing.T) { t.Parallel() var b strings.Builder b.WriteByte('{') @@ -192,7 +271,7 @@ func TestDecodeFlowPrefix_StopsAtTheEntryCap(t *testing.T) { } b.WriteString(`,"openapi":"3.1.0"}`) - got, flow := decodeFlowPrefix([]byte(b.String())) + got, flow := decodeFlowEntries([]byte(b.String())) require.True(t, flow) assert.Equal(t, sniffProbe{}, got, "the entry past the cap is not read") }